1553923 - Large background images loaded from non-web protocols flicker when loaded/changed

Status: NEW

(Core :: Graphics: ImageLib, defect, P3)

Description

[Kris Maglione [:kmag]]

We see this when using themes with very large header images, as were required for old-style lightweight themes. For "reasonably" sized images, there isn't a problem, but for the 3000px wide images used by older themes, the header image very visibly flickers any time a window gains or loses focus, triggered by the header styling change for active vs. inactive windows. This happens even though the background image itself doesn't change, only certain other aspects of the background styling.

Emilio thinks this is probably because we load these images from moz-extension: URLs (or formerly file: URLs), which we apparently don't cache the same way that we cache images from http:/https: URLs. I dug into this a bit before settling on just using a workaround, but wasn't able to learn much.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

FYI - I want to add that:

Theme Header Image Requirements:
Dimensions should be 3000px wide × 200px high

and

Theme Footer Image Requirements:
Dimensions should be 3000px wide × 100px high

is stated in https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes/Create_Your_Own_Firefox_Background_Theme

Comment 2

[Kris Maglione [:kmag]]

(In reply to Virtual_ManPL [:Virtual] - (please needinfo? me - so I will see your comment/reply/question/etc.) from comment #1)

FYI - I want to add that:

Theme Header Image Requirements:
Dimensions should be 3000px wide × 200px high

and

Theme Footer Image Requirements:
Dimensions should be 3000px wide × 100px high

is stated in https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/Themes/Create_Your_Own_Firefox_Background_Theme

Yes, that applies to legacy lightweight themes, not modern static themes. Static themes can specify background colors and multiple images that can be stacked and tiled, which means that huge images are no longer required.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

In particular, I think this is a dupe of bug 1406134, but Andrew would know for sure.

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Emilio, one thing Andrew and I noticed is that we were calling LoadImage on style changes even if the background image url was not actually changing. (In the case we were looking at the url happened to be a css variable). It seems like relying on the image cache to avoid image loads when background image url hasn't changed is the wrong way to do things. Thoughts?

Comment 5

[Emilio Cobos Álvarez (:emilio)]

When I fixed bug 1439285 I did notice that, see bug 1439285 comment 12. See all the discussion in that bug for more context up to the end, I'm happy to explain anything that may not be obvious.

I'm actually waiting for review for a refactoring of URL values in bug 1552708, and it should be trivial to cache requests per-URI in the image loader after that if we wanted. But that still doesn't help if my understanding is correct, because the two windows would be entirely different documents.

We could cache images at the style system level across documents, though that smells like it having security implications and complications that the image cache should already be dealing with, if my understanding is correct.

Anyhow, if we determine that the bug is the style system calling too much into loadImage, that's fixable, but seems like that's not all there is per the discussion in that bug.

Comment 6

[Kris Maglione [:kmag]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)

Emilio, one thing Andrew and I noticed is that we were calling LoadImage on style changes even if the background image url was not actually changing. (In the case we were looking at the url happened to be a css variable). It seems like relying on the image cache to avoid image loads when background image url hasn't changed is the wrong way to do things. Thoughts?

Just as a side note, we also use these header images in multiple windows, and I believe that the chrome image cache should be shared between all browser windows, and would therefore help with loading the header images in new browser windows, even if not the focus change case.

That said, while this was reported as an issue when opening new windows, I don't know if that's just from opening the window, or because we change focus after the window is painted.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

I'm actually waiting for review for a refactoring of URL values in bug 1552708, and it should be trivial to cache requests per-URI in the image loader after that if we wanted. But that still doesn't help if my understanding is correct, because the two windows would be entirely different documents.

To be clear, this isn't just an issue when dealing with two separate windows. When we change focus from one browser window to another, the header background image flickers in both windows, because we have different style rules for active vs. inactive browser windows. The header image URL doesn't change in either of those cases, but still results in us calling LoadImage again for both of the header elements. (This surprised me too, for what it's worth).

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kris Maglione [:kmag] from comment #7)

To be clear, this isn't just an issue when dealing with two separate windows. When we change focus from one browser window to another, the header background image flickers in both windows, because we have different style rules for active vs. inactive browser windows. The header image URL doesn't change in either of those cases, but still results in us calling LoadImage again for both of the header elements. (This surprised me too, for what it's worth).

Do we? That's extremely weird, that's what bug 1439285 fixed... Why don't we enter in the condition here?

https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/layout/style/nsStyleStruct.cpp#1867

Comment 9

[Kris Maglione [:kmag]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

Do we? That's extremely weird, that's what bug 1439285 fixed... Why don't we enter in the condition [here]{https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/layout/style/nsStyleStruct.cpp#1867).

I have no idea. When I was looking into it before, I think I came to the conclusion that that was meant to solve another problem, and I have a vague recollection that it may have had something to do with this code:

https://searchfox.org/mozilla-central/rev/662de518b1686c4769320d6b8825ce4864c4eda0/layout/generic/nsFrame.cpp#912-928

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)

Emilio, one thing Andrew and I noticed is that we were calling LoadImage on
style changes even if the background image url was not actually changing.
(In the case we were looking at the url happened to be a css variable). It
seems like relying on the image cache to avoid image loads when background
image url hasn't changed is the wrong way to do things. Thoughts?

Per spec that should work, it's our bug. If two documents are using the same image then a new load of the existing image in either document shouldn't cause validation; imagelib should just return the existing image immediately. But we don't because the imagelib cache stores one document on each entry for which the entry is valid for, if it's a different document we have to revalidate. See bug 919113.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

I want to take a look at the LoadImage call from the style system. Though I have no rr anymore: https://github.com/mozilla/rr/issues/2360

So maybe not today, but will keep it on the queue of things to look at.

Comment 12

[Timothy Nikkel (:tnikkel)]

Bug 919113 seems like the best way to fix this.

• we need to fix it anyway to be spec complaint
• it will work for all channel types without having to implement something for every different channel type
• it will work for all image types, not just style images. (Although the only other major type of image is img elements and we already have a lot of logic to prevent flashing for them)

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Kris Maglione [:kmag] from comment #9)

I have no idea. When I was looking into it before, I think I came to the conclusion that that was meant to solve another problem, and I have a vague recollection that it may have had something to do with this code:

The reason for this is that I accidentally broke it, see bug 1554405.