852754 - Tab favicon briefly switches image downscaling algorithms when being redrawn (glitch)

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Frank Yan (:fryn)]

Attachment: Screencast of glitch

Steps to reproduce on OS X not in HiDPI mode:
1. Navigate tab A to a PNG larger than 16x16 or a page that has a PNG favicon larger than 16x16 or a page that has an ICO favicon containing only images inside the ICO larger than 16x16.
Examples:
- http://images.wikia.com/lyricwiki/images/f/fd/The_J_Arthur_Keenes_Band.png
- http://rdio-a.akamaihd.net/media/images/2/app_icons/rdio_48.png
- about:home
- rdio.com when logged in
2. Select tab B.
3. Move cursor over favicon in tab A.
4. Move cursor away.

Expected result:
The favicon dims to opacity 0.8 with no transition.

Actual result:
While the favicon is dimming to opacity 0.8, image-rendering briefly changes to what seems to -moz-crisp-edges before reverting back to the default downscaling algorithm. This is perceived as a glitch.

Workaround implying that it isn't a front-end bug:
Adding `display: inline-block;` to the following CSS block prevents the glitch:
https://mxr.mozilla.org/mozilla-central/source/browser/themes/osx/browser.css#2187

CC'in gavin, because I'm not sure in which component this bug should go.

Comment 1

[Frank Yan (:fryn)]

This only occurs in-product on mozilla-central (regular nightlies) on OS X, not UX nightlies nor on other platforms. The bug may still exist on those other branches/platforms, but I haven't seen it manifest itself in the actual UI.

Comment 2

[Frank Yan (:fryn)]

Since I have no idea what is causing this bug, I asked Kyle Huey, who suggested that I ask Joe Drew, who suggested that I "maybe start with the style system".

David, do you know what might be causing this bug? Screencast is attached, and some relevant information is in the description.

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is because changing the opacity marks it as 'active' and puts the content within it (the image) into it's own layer on the GPU. Scaling happens on the GPU using OpenGL.

Once we hit the timeout (1 second) without any further changes to the opacity, we mark it as inactive, and flatten it into the layer behind it. Scaling now happens on the CPU.

Different scaling gives different results, which is what you see here.

Comment 4

[Frank Yan (:fryn)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> This is because changing the opacity marks it as 'active' and puts the
> content within it (the image) into it's own layer on the GPU. Scaling
> happens on the GPU using OpenGL.
> 
> Once we hit the timeout (1 second) without any further changes to the
> opacity, we mark it as inactive, and flatten it into the layer behind it.
> Scaling now happens on the CPU.
> 
> Different scaling gives different results, which is what you see here.

I see. Thank you for the explanation. :)

Is there a recommended workaround to force the image to "flatten" it immediately?
Setting a different value for display causes it to update of course, but that's not a landable workaround.

Comment 5

[Matt Woodrow (:mattwoodrow)]

There isn't a standard way to avoid it, no.

The reason we're scaling on GPU is because the image is the only thing within the layer. In this case we just upload the whole image at it's original size and let the GPU scale it to fit.

If the layer was made up of multiple pieces of content, then we'd draw it all in software at the required size and upload that.

Adding something else within the opacity would do this, but if it's not visible then layout will do it's best to optimize it away and it won't help. Alternatively, if it is visible, then it will change rendering and I doubt you want that.

Maybe a background color with an opacity of 0.01 would work?

CC'ing roc to see if he has any other ideas.

Comment 6

[Seth Fowler [:seth] [:s2h]]

The GPU-scaled version looks crummy, honestly. Matt, do you have an idea how big the perf impact would be to scale the image on the CPU before uploading the texture to the GPU in cases like this?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Is the GPU using nearest-neighbour or something? Why is it so different?

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Seth Fowler [:seth] from comment #6)
> The GPU-scaled version looks crummy, honestly. Matt, do you have an idea how
> big the perf impact would be to scale the image on the CPU before uploading
> the texture to the GPU in cases like this?

It shouldn't be any worse than drawing the scaled image in software.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #7)
> Is the GPU using nearest-neighbour or something? Why is it so different?

I believe so, yes.

Maybe we should just abort the ImageLayer optimization if there are large scale factors set.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Actually, no, we should definitely be using bilinear for OpenGL. Quartz does different high-quality downsampling though I believe.

Comment 11

[Dave Townsend [:mossop]]

Is this something that is easy to fix? It's really quite annoying

Comment 12

[Stephen Horlander [:shorlander]]

(In reply to Dave Townsend [:mossop] from comment #11)
> Is this something that is easy to fix? It's really quite annoying

It is also getting worse over time as more sites are adding hi-res favicons and just scaling them down.

Comment 13

[Markus Stange [:mstange]]

What's the scale factor in these cases? 2? In bug 1182929 I added a workaround so that we don't use ImageLayers (low quality resizing) if the scale factor is 5 or higher.

Comment 14

[Dave Townsend [:mossop]]

Here is an example image that is causing it: https://static.wunderlist.com/44685ca8cfaade90d24f718bf876257562a21214/images/favicon.ico

It's 128x128px and displayed in the tabstrip as a 16x16px image.

Comment 15

[Dave Townsend [:mossop]]

So the scale factors I'm seeing look to be higher than 5 unless I'm misunderstanding things.

Comment 16

[Dave Townsend [:mossop]]

Bueller?

Comment 17

[Markus Stange [:mstange]]

(In reply to Dave Townsend [:mossop] from comment #15)
> So the scale factors I'm seeing look to be higher than 5 unless I'm
> misunderstanding things.

Ok, in that case I don't know why this happens and can't say anything more without debugging it.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Me neither, and I can't reproduce on Linux with Basic compositor.

Comment 19

[Stephen Horlander [:shorlander]]

Even though there are already several examples in this bug I just noticed a particularly bad one on Amazon.com

Screencast: http://people.mozilla.org/~shorlander/bugs/Bug-852754-amazon-favicon-flicker.webm

Favicon file: http://www.amazon.com/favicon.ico

Comment 20

[Dave Townsend [:mossop]]

This affects Amazon, Facebook, Twitter, Reddit and Gmail at least. How do we get someone to look into this?

Comment 21

[Matt Woodrow (:mattwoodrow)]

How does one reproduce this? What version of firefox, platform etc.

I've tried Firefox 48 on both Windows and OSX (at various resolutions and DPIs), and can't reproduce this at all.

The amazon icon from comment 19 is only 48x48, so that would be well within the 5x limit though.

The 128x128 example from comment could also be, if we were on a screen that had 2x DPI scaling (like Retina).

Comment 22

[Stephen Horlander [:shorlander]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> How does one reproduce this? What version of firefox, platform etc.
> 
> I've tried Firefox 48 on both Windows and OSX (at various resolutions and
> DPIs), and can't reproduce this at all.
> 
> The amazon icon from comment 19 is only 48x48, so that would be well within
> the 5x limit though.
> 
> The 128x128 example from comment could also be, if we were on a screen that
> had 2x DPI scaling (like Retina).

I am on OS X on a normal DPI monitor. Open a background tab with an affected site and mouse over it some.

What's shown in this video is how I see it happen: http://people.mozilla.org/~shorlander/bugs/Bug-852754-amazon-favicon-flicker.webm

Comment 23

[Dave Townsend [:mossop]]

(In reply to Stephen Horlander [:shorlander] from comment #22)
> (In reply to Matt Woodrow (:mattwoodrow) from comment #21)
> > How does one reproduce this? What version of firefox, platform etc.
> > 
> > I've tried Firefox 48 on both Windows and OSX (at various resolutions and
> > DPIs), and can't reproduce this at all.
> > 
> > The amazon icon from comment 19 is only 48x48, so that would be well within
> > the 5x limit though.
> > 
> > The 128x128 example from comment could also be, if we were on a screen that
> > had 2x DPI scaling (like Retina).
> 
> I am on OS X on a normal DPI monitor. Open a background tab with an affected
> site and mouse over it some.
> 
> What's shown in this video is how I see it happen:
> http://people.mozilla.org/~shorlander/bugs/Bug-852754-amazon-favicon-flicker.
> webm

Ditto. Latest OSX, latest nightly but it's been happening on nightlies for months. Of the five different sites I have opened in pinned tabs permanently all but two exhibit this issue.

Comment 24

[Dave Townsend [:mossop]]

I can also see this on my MBP's retina display but the effect is far less pronounced and as far as I can see only affects one of my tabs, Wunderlist's favicon: https://static.wunderlist.com/44685ca8cfaade90d24f718bf876257562a21214/images/favicon.ico

Comment 25

[Matt Woodrow (:mattwoodrow)]

Oh, looks like Markus' code for limiting the scale only applies to <image>, not xul imagebox, nor css background images.

Comment 26

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 1: Share the code for limiting scale factors to all image types

This patch sort of got away from me a bit, I can try split it out if you'd like.

Comment 27

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 2: Share the implementation of GetContainer

Comment 28

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 3: Share the implementation of ConfigureLayer

Comment 29

[Matt Woodrow (:mattwoodrow)]

Attachment: Part 4: Reduce max downscaling allowed to <3

Apparently drawing a 48x48 favicon at 16x16 looks bad, so this new limit should block that.

Comment 30

[Matt Woodrow (:mattwoodrow)]

Fair bit of trailing whitespace snuck in, I'll fix those locally.

Comment 31

[Markus Stange [:mstange]]

Comment on attachment 8738836 [details] [diff] [review]
Part 4: Reduce max downscaling allowed to <3

Review of attachment 8738836 [details] [diff] [review]:
-----------------------------------------------------------------

<3

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f173242d0fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0911a6bd06
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad9c600a557c
https://hg.mozilla.org/integration/mozilla-inbound/rev/75ea4f74e030

Comment 33

[Joel Maher ( :jmaher ) (UTC -8)]

hey, I think we have a perf win on tscrollx for linux64:
https://treeherder.mozilla.org/perf.html#/alerts?id=766

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4f173242d0fb
https://hg.mozilla.org/mozilla-central/rev/2a0911a6bd06
https://hg.mozilla.org/mozilla-central/rev/ad9c600a557c
https://hg.mozilla.org/mozilla-central/rev/75ea4f74e030

Comment 35

[Stephen Horlander [:shorlander]]

Fixed on Nightly for me. Thank you!

Comment 36

[Stephen Horlander [:shorlander]]

(In reply to Stephen Horlander [:shorlander] from comment #35)
> Fixed on Nightly for me. Thank you!

So, it seems mostly fixed, but I discovered that Yahoo at least still exhibits this behavior.

STR: Do a Yahoo search, switch tabs, hover over background Yahoo search tab, notice flickering.

Comment 37

[Matt Woodrow (:mattwoodrow)]

I see this, it's not particularly bad though.

I guess we could lower the scaling threshold even more, but we're trading off performance for quality and I'm not sure it's worth it.

Comment 38

[Markus Stange [:mstange]]

(In reply to Joel Maher (:jmaher) from comment #33)
> hey, I think we have a perf win on tscrollx for linux64:
> https://treeherder.mozilla.org/perf.html#/alerts?id=766

Looks like this was just due to the test hitting bug 1263480. Now that bug 1263480 has been fixed, tscrollx has reverted to the old numbers.

Comment 39

[Ryan Feeley [:rfeeley]]

Related? https://bugzilla.mozilla.org/show_bug.cgi?id=1288486

Comment 41

[Timothy Nikkel (:tnikkel)]

Looks like all the patches landed here. If we still care about the issue mentioned in comment 36 then we should file a new bug for it. Leaving this bug open only serves to confuse things.