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

fix(theming): Conitionally disable blur filter for performance #45395

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented May 17, 2024

Summary

As an alternative for #45049 but this one is not removing the blurry background for all users but only for chrome + edge by default.
User can force enable if it works for them (e.g. they have hardware acceleration) or force disable if they suffer performance issues.

Firefox and Safari are not affected by the performance issues, and Android should also be ok as they have proper hardware acceleration.
Mostly Windows is affected and Linux with faulty GPU drivers.

Screencast

a.mp4

Checklist

@susnux susnux requested a review from szaimen May 17, 2024 12:39
@susnux susnux added bug 3. to review Waiting for reviews labels May 17, 2024
@susnux susnux requested review from jancborchardt, a team, Pytal, sorbaugh and solracsf and removed request for a team May 17, 2024 14:57
$isChromium = $this->request !== null && $this->request->isUserAgent([Request::USER_AGENT_CHROME, Request::USER_AGENT_MS_EDGE]);
// Allow to force the blur filter
$forceEnableBlur = $this->config->getUserValue(
$this->userSession->getUser()->getUID(),

Check notice

Code scanning / Psalm

PossiblyNullReference Note

Cannot call method getUID on possibly null value
@susnux susnux marked this pull request as ready for review May 17, 2024 15:01
@susnux susnux requested review from a team, icewind1991, yemkareems and sorbaugh and removed request for sorbaugh and a team May 17, 2024 15:01
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>
@susnux susnux added this to the Nextcloud 30 milestone May 17, 2024
@susnux
Copy link
Contributor Author

susnux commented May 17, 2024

To be discussed by @nextcloud/designers

@kesselb
Copy link
Contributor

kesselb commented May 18, 2024

Thanks for working on it 👍

Apparently my chromium has no hardware acceleration any longer (it's a snap package now, maybe that's related).

It's basically impossible to use Nextcloud and especially navigation and dashboard.

Screencast.from.2024-05-18.16-46-36.webm
Screencast.from.2024-05-18.16-55-12.webm

The auto-detection worked.
If you selected yes or no once, you cannot go back to "auto" but that's acceptable, I guess.
The left navigation looks definitely cleaner with the blurred background.

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.

I like this approach in general :)
One thing: leaving the sharp edges of a picture below the text can make it hard to read. I think that when we disable the blur we should also add a solid background, not transparent. We can use an average of the background similar to the auto primary? But lighter or darker depending on the theme.
Can we also narrow this down to windows && chromium users only?

@susnux
Copy link
Contributor Author

susnux commented May 19, 2024

windows && chromium

It is not only Windows, I once got this (I run a rolling release distro) on Linux and had to wait like 2 days for bleeding edge Chromium fixing their GPU layer (not sure what they are using because I never had issues with Firefox).

One thing: leaving the sharp edges of a picture below the text can make it hard to read. I think that when we disable the blur we should also add a solid background, not transparent. We can use an average of the background similar to the auto primary? But lighter or darker depending on the theme.

For 30+ with #42977 this would work fine, but with older (we need to backport) we can basically only mix with primary color and hope it matches somewhat the background.

@marcoambrosini
Copy link
Member

marcoambrosini commented May 20, 2024

I once got this (I run a rolling release distro) on Linux and had to wait like 2 days

I think for such a case we can reasonably expect people to go and find the checkbox themselves

Another Idea that I had in the past was creating a blurred version of the background on the server and use that one with position: fixed behind all transparent elements. What do you think @susnux?
This would preserve the current look and remove the need for a switch.
It seems like this can be done with GD

 imagefilter($image, IMG_FILTER_GAUSSIAN_BLUR);
 imagejpeg($image, 'path/to/save/blurred_image.jpg');

The resulting compressed blurred image will also be much lighter than the actual background, so it shouldn't impact loading times too much

@kesselb
Copy link
Contributor

kesselb commented May 20, 2024

I think for such a case we can reasonably expect people to go and find the checkbox themselves

If chromium/chrome can use the hardware acceleration depends on the operating system, gpu, gpu drivers and packaging. We cannot just assume it does not work on Windows but everywhere else. I'm using Ubuntu 24.04 with Chromium (snap), and have no hardware acceleration either. It did work with Ubuntu 23.10 and Chromium (apt).

image

I think you should get an idea, how annoying the current state is, if you turn off the graphics acceleration in your chrome/chromium settings.

@marcoambrosini
Copy link
Member

marcoambrosini commented May 20, 2024

I'm using Ubuntu 24.04 with Chromium

That's the point, it seems like a very niche use case compared to the vast majority of chrome based browsers on platforms other than windows. People with setups similar to yours can surely reach into the settings and force-disable the blur.

That said, what do you think about this idea
also cc @juliushaertl and @ShGKme

@kesselb
Copy link
Contributor

kesselb commented May 20, 2024

That's the point, it seems like a very niche use case compared to the vast majority of chrome based browsers on platforms other than windows. People with setups similar to yours can surely reach into the settings and force-disable the blur.

How would someone know that for the sluggish appearance is caused by a blur filter that can be turned off by a configuration option? ;)

What are your thoughts on enabling the blur filter by default only for chrome/chromium on macosx and android?

That said, what do you think about #45395 (comment)

I am not sure if I understood the idea. Server will provide a blurred version of the background image. The default background is the not-blurred version. The blurred version is placed on top of the default background image and only visible if there is a transparent element to be shown? Is the background image then loaded twice, and how does an element know that the other background is needed?

Furthermore, I'm not familiar enough with the topic and therefore trust your decision.
I just wanted to add, as someone who has no working hardware acceleration, that the current state is annoying.

@susnux
Copy link
Contributor Author

susnux commented May 20, 2024

That said, what do you think about this idea

I think this is hard to be done correctly implementation-wise, meaning to ensure the blurred version is shown on the correct location with correct scaling.
As said with Nextcloud 30 we can have a fake blurhash that we can use there, for 28 and 29 I think we could instead just mix background and primary.

This would need no changes at all in all apps and library, we just need to patch server theming variables.

For example:

With blur Without blur With mixed-color
Screenshot_20240520_200400 Screenshot_20240520_200355 Screenshot_20240520_200543

@marcoambrosini
Copy link
Member

marcoambrosini commented May 22, 2024

@susnux I fiddled a bit with it and I think we can accomplish this with a pseudo element. Here's a code pen:

https://codepen.io/marukodesu/pen/RwmRbpr

If this can be solved without creating a setting, I think we should go down that route.

@ShGKme
Copy link
Contributor

ShGKme commented May 22, 2024

Another Idea that I had in the past was creating a blurred version of the background on the server

We can also start with creating it on frontend side once on load, but could be not very fast as well.

@marcoambrosini
Copy link
Member

I created an issue with the approach I propose for 30+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

scrolling in dashboard is laggy CSS blur filter seriously impacts performance in Chrome
6 participants