Improve cache handling in blur filter #4792

danxuliu · 2020-12-17T21:24:50Z

Fixes #4827
Follow up to #4678

Instead of using an array as the cache now a LRU cache is used, which should prevent overflowing the cache.

Pending:

Make the cache global so the backgrounds are reused when switching between speaker mode and grid view (as VideoBackground is recreated in that case)

danxuliu · 2020-12-17T21:25:02Z

/backport to stable20.1

danxuliu · 2020-12-17T21:25:10Z

/backport to stable20

The blurred background images were cached directly in an array, so the cache could grow without limits. Although it should be a rare case, this could lead to a high memory consumption if the user slowly resizes during a long time the browser window. Therefore the cache now uses a least recently used cache limited to just five elements (so it can store the blurred backgrounds when the sidebars are open or closed, plus having a bit of room). "node-lru-cache" was selected as the library to use due to being small, widely used and, apparently, matching the expected behaviour ("quick-lru", which is smaller and also widely used, although not so much, unfortunately does not seem to honour the "maximum size" parameter). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Before the cache for blurred video backgrounds was local to each VideoBackground instance. During a call different VideoBackground instances can be mounted and destroyed for the same participant (for example, when switching between speaker mode and grid view, or when a promoted speaker changes), so the cache was lost and the blurred background had to be recalculated every time that happened. Now the cache is global and shared between VideoBackground instances, so previously cached backgrounds can be used by new VideoBackground instances. The global cache is lazily initialized when needed and cleared each time a call ends. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

nickvergessen · 2021-01-05T18:03:11Z

package.json

@@ -36,6 +36,7 @@
 		"escape-html": "^1.0.3",
 		"hark": "^1.2.3",
 		"lodash": "^4.17.20",
+		"lru-cache": "^6.0.0",


This is reaching a level where i think it stops being useful/worth it

It is only ~16 KiB unpacked including its single dependency, so I do not think it is so bad :-) If I had written my own with just the needed features it would have been (I hope...) smaller, but this was faster and easier 🤷

well its not about the bytes or whatever, its about the complexity off all this. 3rdparty lib, caching, loading images and blurring them with JS, …

nickvergessen · 2021-01-07T15:55:46Z

src/store/callViewStore.js

+			return
+		}
+
+		Vue.set(state.blurredBackgroundImageCache, videoBackgroundId, new LRU({ max: 10 }))


so what happens in a call with 12 users and they speak all one sentence after another, it will have to calculate again and again?

No, each participant has (or should :-P ) its own cache; 10 is the number of backgrounds for each participant (as the blurred backgrounds are different depending on the size of the video background).

nickvergessen · 2021-01-07T15:56:19Z

src/store/callViewStore.js

@@ -26,6 +26,8 @@ import {
 	CONVERSATION,
 } from '../constants'

+const LRU = require('lru-cache')


normally we use import? not require?

The library does not support import, only require 🤷

nickvergessen · 2021-01-07T16:15:49Z

I don't know, might be me or something. I set the LRU size to 2, joined with 3 people and when the 3rd one joins the avatar background of the first one changes.

I have to admit I don't care enough and as said before tend more and more to just have black backgrounds in chrome/chromium if the blurring sucks too much. but like storing blurred images in the vue cache, etc....
I leave the decision to Vincent and Marco

PVince81 · 2021-01-11T14:43:34Z

I'm also not thrilled about the added complexity.

Now I'm also wondering whether the performance issue is happening only on Linux or also on Windows and Mac ?
Because some browsers on Linux are lacking hardware acceleration. In my Firefox on openSUSE I had to turn on some obscure about:config option to enable acceleration and get smooth animations.

Depending the affected demographics it might justify the added complexity. But if it only affects browsers on Linux, browsers which will likely get improvements for hardware acceleration (usually those are experimental first), then maybe not worth the extras and in those cases we could go with black background (or use the avatar letters background color instead).

Thoughts ? @danxuliu @ma12-co

marcoambrosini · 2021-01-12T07:05:16Z

But if it only affects browsers on Linux

Does it? I thought that it affected chrome in general!
In that case yes, I think it'd be fine to go with the usernameToColor function for the backgrounds as an exception for linux chromium based browsers only.

PVince81 · 2021-01-12T07:15:42Z

I'm not sure if only Linux is affected. I think I now remember @danxuliu mentioning IE also being affected, but not sure if it was a guess.

nickvergessen · 2021-01-12T07:21:30Z

IE cant do calls

danxuliu · 2021-01-20T22:34:32Z

I think I now remember @danxuliu mentioning IE also being affected, but not sure if it was a guess.

Not IE, Edge based on Chromium.

For reference Chromium on MacOS seems to work fine. It does not on Windows (or, at least, on a virtual machine; maybe it does if graphic acceleration is available, I do not know).

Safari has the same performance issue as Chromium when using CSS blur filters, and it does not support the workaround used for Chromium (because it does not support CSS filters on canvases).

Anyway, as discussed this will be closed in favor of always using an average color instead of a blurred background (except on Firefox, as the CSS blur filter works fine).

danxuliu added 2. developing bug feature: frontend 🖌️ "Web UI" client feature: call 📹 Voice and video calls labels Dec 17, 2020

danxuliu added this to the 💚 Next Major (21) milestone Dec 17, 2020

backportbot-nextcloud bot added the backport-request label Dec 17, 2020

nickvergessen modified the milestones: 11.0.0-alpha.2, 💚 Next Alpha (21) Dec 18, 2020

danxuliu mentioned this pull request Dec 22, 2020

Blur lag introduced in Talk 10.0.4 on Chromium #4827

Closed

danxuliu added 2 commits January 5, 2021 10:51

danxuliu force-pushed the improve-cache-handling-in-blur-filter branch from 4037327 to 208b820 Compare January 5, 2021 17:10

danxuliu added 3. to review and removed 2. developing labels Jan 5, 2021

danxuliu marked this pull request as ready for review January 5, 2021 17:55

danxuliu requested a review from nickvergessen January 5, 2021 17:55

nickvergessen reviewed Jan 5, 2021

View reviewed changes

nickvergessen reviewed Jan 7, 2021

View reviewed changes

nickvergessen requested review from PVince81 and marcoambrosini January 7, 2021 16:16

nickvergessen modified the milestones: 11.0.0-alpha.3, 💚 Next Alpha (21) Jan 11, 2021

danxuliu closed this Jan 20, 2021

danxuliu deleted the improve-cache-handling-in-blur-filter branch January 20, 2021 22:34

danxuliu removed the backport-request label Jan 27, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve cache handling in blur filter #4792

Improve cache handling in blur filter #4792

danxuliu commented Dec 17, 2020 •

edited

Loading

danxuliu commented Dec 17, 2020

danxuliu commented Dec 17, 2020

nickvergessen Jan 5, 2021

danxuliu Jan 7, 2021

nickvergessen Jan 7, 2021

nickvergessen Jan 7, 2021

danxuliu Jan 20, 2021

nickvergessen Jan 7, 2021

danxuliu Jan 20, 2021

nickvergessen commented Jan 7, 2021

PVince81 commented Jan 11, 2021

marcoambrosini commented Jan 12, 2021 •

edited

Loading

PVince81 commented Jan 12, 2021

nickvergessen commented Jan 12, 2021

danxuliu commented Jan 20, 2021

Improve cache handling in blur filter #4792

Improve cache handling in blur filter #4792

Conversation

danxuliu commented Dec 17, 2020 • edited Loading

danxuliu commented Dec 17, 2020

danxuliu commented Dec 17, 2020

nickvergessen Jan 5, 2021

Choose a reason for hiding this comment

danxuliu Jan 7, 2021

Choose a reason for hiding this comment

nickvergessen Jan 7, 2021

Choose a reason for hiding this comment

nickvergessen Jan 7, 2021

Choose a reason for hiding this comment

danxuliu Jan 20, 2021

Choose a reason for hiding this comment

nickvergessen Jan 7, 2021

Choose a reason for hiding this comment

danxuliu Jan 20, 2021

Choose a reason for hiding this comment

nickvergessen commented Jan 7, 2021

PVince81 commented Jan 11, 2021

marcoambrosini commented Jan 12, 2021 • edited Loading

PVince81 commented Jan 12, 2021

nickvergessen commented Jan 12, 2021

danxuliu commented Jan 20, 2021

danxuliu commented Dec 17, 2020 •

edited

Loading

marcoambrosini commented Jan 12, 2021 •

edited

Loading