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

Decrease resource usage of CSS animations #2021

Merged
merged 3 commits into from Feb 11, 2022

Conversation

Iksas
Copy link
Contributor

@Iksas Iksas commented Dec 24, 2021

By submitting this pull request, I confirm the following:

  • I have read and understood the contributors guide, as well as this entire template.
  • I have made only one major change in my proposed changes.
  • I have commented my proposed changes within the code.
  • I have tested my proposed changes.
  • I am willing to help maintain this change if there are issues with it later.
  • I give this submission freely and claim no ownership.
  • It is compatible with the EUPL 1.2 license
  • I have squashed any insignificant commits. (git rebase)
  • I have Signed Off all commits. (git commit --signoff)

What does this PR aim to accomplish?:

This PR decreases the resource usage of various CSS animations in all themes.

The effects of this PR are described in more detail in this comment: #2021 (comment)

Additionally, this PR fixes a small layout bug mentioned in #2098.

How does this PR accomplish the above?:

Pseudo elements are added to the animated elements, so that their opacity property can be animated instead of the actual colors.

Note that, after this PR, the text of the pseudo element used for the lookatme animation (the red flashing text) is defined by a lookatme-text attribute in the HTML. This attribute needs to be manually updated every time the text changes, for example:
https://github.com/pi-hole/AdminLTE/blob/a5291a1909e53bd7f66a3114ed4790d19b8ac557/scripts/pi-hole/js/settings.js#L218-L221

On top of that, the frame rate has been limited to 10fps for most animations. This doesn't seem to affect the quality of the animations, since only slow color changes without movement are taking place.

There are two exceptions: The frame rate of the letters animation in the LCARS theme has not been limited, because limiting it somehow worsens its efficiency. The frame rate of the fire animation in the LCARS theme has not been limited because limiting it breaks the animation.

What documentation changes (if any) are needed to support this PR?:

none

@rdwebdesign
Copy link
Member

I think Firefox always use GPU, if it is available.

Did you test it on other combinations of OSes and browsers?

@Iksas
Copy link
Contributor Author

Iksas commented Dec 24, 2021

  • Yes, that seems to be the case.
  • I tried it in Firefox (Win and Linux) as well as in MS Edge. There was no difference at all. Interestingly, Safari on iOS seems to be unaffected as well (I measured the CPU usage). Desktop Safari is the only affected browser that I know of.

@rdwebdesign
Copy link
Member

There is a mix of comments/articles on the internet saying opposite thinks about using CSS to trigger GPU rendering like this.

I'm not sure which one is right.

@Iksas
Copy link
Contributor Author

Iksas commented Dec 24, 2021

Yes, the solution with the transformation is weird.

I just got it to work with the will-change property though. Turns out you have to set it to opacity, and Safari is satisfied.
This now seems like the right solution to me, what do you think?

@rdwebdesign
Copy link
Member

As I said, I'm not sure, but this version looks better.

I like to get input from others before making a decision. Perhaps someone has a better understanding of these optimizations.

@XhmikosR
Copy link
Contributor

will-change needs to be used carefully. I personally still use the translate way.

BTW the issue here is that the animation is NOT using CSS properties which are offloaded to the GPU (border-color). I'd just change the animation effect, personally.

@DL6ER
Copy link
Member

DL6ER commented Dec 24, 2021

We know what the color will be when at both ends: #f89201 (bright) and #71480a (dim). Why not simply toggle between these colors without using any transparency. Maybe this is less heavy-lifting for Safari:

@keyframes warningPulse {
  0% {
    border-color: #71480a;
  }
  50% {
    border-color: #f89201;
  }
  100% {
    border-color: #71480a;
  }
}

.box-warning {
  animation: 3s infinite warningPulse;
}

Could you try this @Iksas ?

@Iksas
Copy link
Contributor Author

Iksas commented Dec 24, 2021

With these changes, both CPU and GPU are running at about 80%. So it's a bit better than with transparency, but not really where it should be.

@Iksas
Copy link
Contributor Author

Iksas commented Dec 24, 2021

Would it be acceptable to reduce the FPS of the CSS animation? When limiting it to 20fps, the GPU usage drops to only 4% in Firefox / 10% in Safari:

.box-warning {
  animation: 3s infinite warningPulse steps(30);
  will-change: opacity;
}

(30 steps * 2 transitions / 3 seconds = 20fps)

When the will-change property is removed, the GPU usage is 40% in Safari @ 20 fps.

@PromoFaux
Copy link
Member

Safari is basically Internet Explorer these days...

@PromoFaux
Copy link
Member

Would it be acceptable to reduce the FPS of the CSS animation?

Yeah, I think so

@Iksas Iksas marked this pull request as draft January 5, 2022 14:55
@Iksas Iksas force-pushed the safari-css-animation-fix branch 3 times, most recently from baa8e7c to 1f67c23 Compare January 22, 2022 12:30
Signed-off-by: Iksas <Iksas@users.noreply.github.com>
Signed-off-by: Iksas <Iksas@users.noreply.github.com>
@PromoFaux
Copy link
Member

@Iksas do you still have more to add to this, or is it in draft for another reason?

@Iksas
Copy link
Contributor Author

Iksas commented Jan 22, 2022

I'm still working on this at the moment. I just fixed one last bug (hopefully), and still have to test everything after that.
I'll probably finish this today.

@rdwebdesign
Copy link
Member

rdwebdesign commented Jan 22, 2022

@Iksas

Just a note:
until now, LCARS theme was create using only CSS to avoid breaking the default themes.

This new fix is changing HTML and JS bits for all themes.
Are you sure this is necessary?

Was this tested for all themes, with all main browsers?

@rdwebdesign
Copy link
Member

PS: I can't test this at least until tomorrow at night.

@Iksas
Copy link
Contributor Author

Iksas commented Jan 22, 2022

By limiting the frame rate to 10fps while adding some pseudo elements, and animating only their opacity instead of RGB values, the CPU usage could be reduced by over 90% for some animations.
This makes the web interface more accessible on weak devices like the Raspberry Pi 4. On a MacBook I tested on, the energy usage could be reduced by up to 10 Watts in some cases (on a 15 Watt CPU):

lookatme

warningPulse

However I understand that such an amount of changes might not be worth the improvement. For example, I used a lookatme-text HTML attribute to be able to create pseudo elements in CSS which contain a certain text:

https://github.com/pi-hole/AdminLTE/blob/27ddc2a506ad594a33e08677947bdd7e5507a581/scripts/pi-hole/php/footer.php#L85

Using this technique in the red update notification reduces CPU usage by about 50% on the same frame rate:

lookatme-comparison

If the changes are too much, reducing the frame rate alone could still improve the efficiency quite a bit with minimal changes. While testing, I couldn't perceive any stuttering for frame rates as low as 10fps (only color changes).

Of course, this PR is just a suggestion. I just wanted to see what's possible, now it's up to the team ;)

I have tested this in Firefox, Chromium and Safari. Later today, I will perform final tests, after which I will remove the draft status.

@Iksas Iksas marked this pull request as ready for review January 22, 2022 23:08
@Iksas Iksas changed the title Improve performance of CSS animations in Safari Decrease resource usage of CSS animations Jan 22, 2022
@rdwebdesign
Copy link
Member

@Iksas

I took a quick test. I think I found 2 problems:

  1. in .lookatme class:
    Using "display: inline-block" breaks the DHCP warning (try to enable/disable DHCP in settings).
    Suggestion: use inline-block only for footer .lookatme

  2. in LCARS theme, login warning:
    I'm seeing an alignment error between ::after and ::before psedo-classes, when not boxed.
    I think each element is using a different padding.

@Iksas
Copy link
Contributor Author

Iksas commented Jan 23, 2022

Thanks, I will take a look at this.
What is the issue you found with the DHCP warning? Is the warning supposed to disappear after you deactivate the checkbox, or did I miss something else?

@rdwebdesign
Copy link
Member

rdwebdesign commented Jan 23, 2022

Yes.

The current behavior is to show the message only when the checkbox is activated.

In my test, the checkbox started deactivated (and no message).
I checked the box and the message appeared. When a clicked again, the message did not disappeared.

If you remove the display: inline-block it will work as expected. You can add the display to the footer, if needed.

@rdwebdesign
Copy link
Member

@Iksas

On more thing:

I don't know if this improves something for the .lookatme class, but have you tried to move the text-shadow to the fixed element (without animation).
This way the red glow won't be animated, but I think it will use even less resources (I did not test this).

@Iksas Iksas force-pushed the safari-css-animation-fix branch 2 times, most recently from 925f1fa to d225748 Compare January 23, 2022 18:49
@Iksas
Copy link
Contributor Author

Iksas commented Jan 23, 2022

Both bugs have been fixed now.
However while doing this, I found another bug that only affects Safari. The pseudo element of td#cachelivefreed is slightly misaligned (about 0.5px):

safari-being-safari

zoomed in:
safari-zero

The height and width of both "DNS cache evictions:" elements in the left column are identical, and they are properly aligned in Safari. So I think this bug happens because the height and width of td#cachelivefreed:after are 1px smaller than the height and width of td#cachelivefreed.

Do you know how to get the td#cachelivefreed:after elements to have the same size as td#cachelivefreed, or should I ignore this bug as it is probably caused by Safari itself? Everything I tried to fix this doesn't work.

have you tried to move the text-shadow to the fixed element (without animation).

I think this doesn't work, because the text can't overlap the shadow.
As far as I know, there should be no performance gain either, because the shadow is only rendered once in the beginning. After that, only the opacity is changed, which should take about the same work in both cases.
If you really want to, I could try it out though.

@rdwebdesign
Copy link
Member

Do you know how to get the td#cachelivefreed:after elements to have the same size as td#cachelivefreed, or should I ignore this bug as it is probably caused by Safari itself? Everything I tried to fix this doesn't work.

I think is a really small thing and happens only in a specific place, in a specific browser, and it's not distorting the text or making difficult to read.

We can live with that, at lest for now.

@rdwebdesign rdwebdesign reopened this Jan 23, 2022
@rdwebdesign
Copy link
Member

I clicked in the wrong button. It is reopened.

@Iksas
Copy link
Contributor Author

Iksas commented Jan 23, 2022

We can live with that, at lest for now.

Alright. If this doesn't get fixed in the next few releases of Safari, I can look into it again.

@rdwebdesign
Copy link
Member

@Iksas

I'm testing the LCARS theme modifications and found a big difference in the font size.
The original alert uses almost the full width of the .panel. Now I'm seeing a really smaller alert. The new alert is too low.

lcars_alert_diff

@Iksas
Copy link
Contributor Author

Iksas commented Jan 24, 2022

Sorry about that - it's fixed now.

Signed-off-by: Iksas <Iksas@users.noreply.github.com>
@Iksas
Copy link
Contributor Author

Iksas commented Feb 9, 2022

@rdwebdesign do you have feedback on the changes?

@rdwebdesign
Copy link
Member

I think your changes are OK.

@PromoFaux PromoFaux merged commit bb4cafc into pi-hole:devel Feb 11, 2022
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/pi-hole-ftl-v5-14-web-v5-11-and-core-v5-9-released/53529/1

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

Successfully merging this pull request may close these issues.

None yet

6 participants