Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace blur with average color in video backgrounds #4985

Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented Jan 20, 2021

Follow up to #4678
Closes #4827
Closes #4887

Safari suffers the same performance hit as Chromium when using the blur filter. However, Safari does not support using the blur filter on canvases, so the same workaround used for Chromium does not work in Safari. To solve this, and to still use a background with colors related to the avatar, the average of the colors in the avatar is used.

For simplicity, as discussed, instead of using a workaround to still show blurred backgrounds in Chromium despite its performance bug in some systems now the average color is used instead.

Moreover, as the CSS blur filter only works reliably in Firefox any other browser (which now also includes Chromium-based browsers, like the new Edge) uses the average color for the backgrounds.

Pending:

  • Fix background for guests
  • Test thoroughly
  • Test with Safari
  • Test with Edge before being based in Chromium
  • Test with Edge after being based in Chromium
  • Ensure that I did not forget to remove some blur related code

@danxuliu
Copy link
Member Author

/backport to stable20.1

@danxuliu
Copy link
Member Author

/backport to stable20

Safari suffers the same performance hit as Chromium when using the blur
filter. However, Safari does not support using the blur filter on
canvases, so the same workaround used for Chromium does not work in
Safari. To solve this, and to still use a background with colors related
to the avatar, the average of the colors in the avatar is used.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Before the average color for video backgrounds was locally stored in
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 previously calculated value was lost
and the average color had to be recalculated every time that happened.

Now the average colors are globally cached and shared between
VideoBackground instances, so previously calculated values can be used
by new VideoBackground instances.

The global cache is cleared each time a call ends.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
For simplicity, as discussed, instead of using a workaround to still
show blurred backgrounds in Chromium despite its performance bug in some
systems now the average color is used instead.

Moreover, as the CSS blur filter only works reliably in Firefox any
other browser (which now also includes Chromium-based browsers, like the
new Edge) uses the average color for the backgrounds.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the replace-blur-with-average-color-in-video-backgrounds branch from 4a5b8cd to 1bc9656 Compare January 21, 2021 10:41
@danxuliu danxuliu marked this pull request as ready for review January 21, 2021 10:44
Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works. Also looks nice, good call @nickvergessen :)

@nickvergessen
Copy link
Member

Test with Edge before being based in Chromium

We don't care about that one.

@nickvergessen
Copy link
Member

Safari:
#4995

@danxuliu
Copy link
Member Author

Safari:
#4995

But this is in master too, right? Or is it caused by this pull request?

@nickvergessen
Copy link
Member

Yeah master too, caused by the links in descriptions added with a nextcloud/vue update

@nickvergessen
Copy link
Member

let me comment out the regex in the 3rdparty temporarily, then it loads on safari.

@nickvergessen
Copy link
Member

Safari 13.1 (on MacOS) Edge 88 (on Windows 10)
Screen Shot 2021-01-22 at 10 51 39 Webaufnahme_22-1-2021_105522_192 168 178 101
Firefox 84 (on Ubuntu 20.10) Chromium (on Ubuntu 20.10)
Bildschirmfoto von 2021-01-22 10-59-05 Bildschirmfoto von 2021-01-22 11-00-30

@nickvergessen
Copy link
Member

Seems to work, but now I wonder if we should just always use that to be on the same design in all browsers/OS and to simplify our code.

Copy link
Member

@marcoambrosini marcoambrosini left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, there should be a div that tarkens the background and creates sort of a vignetting effect. I don't see it in @nickvergessen screenshots
Do you know what happened to it @danxuliu ?

@jancborchardt
Copy link
Member

Seems to work, but now I wonder if we should just always use that to be on the same design in all browsers/OS and to simplify our code.

That’s a bummer, but yeah if it’s a performance hit then that’s a priority.
And yep – if only Firefox would still use the blur effect that doesn’t really make sense → let’s keep it consistent in all browsers.

For consistency between browsers now the average color is shown in all
cases, even if CSS blur filters work fine in Firefox.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The darkener was shown behind the background, so the background was not
darkened.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu
Copy link
Member Author

Wait, there should be a div that tarkens the background and creates sort of a vignetting effect. I don't see it in @nickvergessen screenshots
Do you know what happened to it @danxuliu ?

The darkner div was behind the background, so it had no effect. Apparently it has been like that for nine months already 🤷 I have moved it in front and now it works again.

Seems to work, but now I wonder if we should just always use that to be on the same design in all browsers/OS and to simplify our code.

That’s a bummer, but yeah if it’s a performance hit then that’s a priority.
And yep – if only Firefox would still use the blur effect that doesn’t really make sense → let’s keep it consistent in all browsers.

Done.

@nickvergessen
Copy link
Member

Tested with:

  • Chromium on Ubuntu
  • Firefox on Ubuntu
  • Safari on iPad
  • Edge on Windows 10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants