1038416 - [B2G][Browser] Zooming, panning, and scrolling through image sites causes a gray faded box to overlay the screen.

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Dean R. Gomez (DeanG)]

Attachment: Gray_Fade.png

Description:
When a user launches the Browser app, goes to a site with many images (imgur, pinterest, polycount image threads, etc.), and begins to zoom, pan, or scroll through the images at a large speed, a gray looking faded box overlays the images on the page for a long while.

Prerequisites: Be connected to the internet via WiFi and have a Pinterest account.

Repro Steps:
1) Update a Flame to 20140714000202
2) Launch the Browser app.
3) Go to https://www.pinterest.com and sign into a valid account.
4) Once access to images have been granted, scroll, pan, and zoom at fast speeds.
5) Observe the screen.

Actual:
A black / gray / white box overlays different sections of the webpage for upwards of 5 seconds.
Expected:
The webpage will load properly without having grayed out sections and pixelation for long extended periods of time.

2.0 Environmental Variables:
Device: Flame 2.0 (273MB)
BuildID: 20140714000202
Gaia: ca022f811bcbbda0f89086094a9e92bb220fea18
Gecko: 376889ab0e02
Version: 32.0a2 (2.0) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Repro frequency: 4/4 - 100%
See attached: Logcat_Gray_Fade.txt, Gray_Fade.png

Comment 1

[Dean R. Gomez (DeanG)]

Attachment: Logcat_Gray_Fade.txt

Attaching Logcat.

Comment 2

[Dean R. Gomez (DeanG)]

This issue DOES occur on Flame 2.0 (512MB), Flame 2.1 (273MB), Buri 2.0, and Buri 2.1, 


Flame 2.0 (512mb)

2.0 Environmental Variables:
Device: Flame 2.0
BuildID: 20140714000202
Gaia: ca022f811bcbbda0f89086094a9e92bb220fea18
Gecko: 376889ab0e02
Version: 32.0a2 (2.0) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Flame 2.1 (273MB)

2.1 Environmental Variables:
Device: Flame Master
Build ID: 20140714040201
Gaia: 88e0a972280bb35847c010b8c3f1481fa80f3847
Gecko: f7aef4fc9d47
Version: 33.0a1 (Master) 
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


Buri 2.1

2.1 Environmental Variables:
Device: Buri Master
Build ID: 20140714073123
Gaia: 88e0a972280bb35847c010b8c3f1481fa80f3847
Gecko: 340b19c14d3d
Version: 33.0a1 (Master) MOZ
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


Buri 2.0

2.0 Environmental Variables:
Device: Buri 2.0
BuildID: 20140714000202
Gaia: ca022f811bcbbda0f89086094a9e92bb220fea18
Gecko: 376889ab0e02
Version: 32.0a2 (2.0) 
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0


Result: A black / gray / white box overlays different sections of the webpage for upwards of 5 seconds.

-----------------------------------------------------------------------------------

This issue DOES NOT occur on Flame 1.4 (273MB) and Buri 1.4.


Flame 1.4 (273MB)

1.4 Environmental Variables:
Device: Flame 1.4
Build ID: 20140714000206
Gaia: b7d36622c7df92c976c37520ccab25199c7ada91
Gecko: dbebcdab47aa
Version: 30.0 (1.4)
Firmware Version: v122
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0


Buri 1.4

1.4 Environmental Variables:
Device: Buri 1.4
Build ID: 20140714000206
Gaia: b7d36622c7df92c976c37520ccab25199c7ada91
Gecko: dbebcdab47aa
Version: 30.0 (1.4)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0


Actual Result: No gray or white faded box appears overlaying the images. Instead, a white screen appears to demonstrate the page is loading.

Comment 3

[Peter Bylenga [:PBylenga]]

Nominating to block 2.0, is a regression and affects multiple top sites.  Requesting a window.

Comment 4

[Gregor Wagner [:gwagner]]

Why is this a 273MB bug? This also happens on the 512MB flame right?
Milan, this seems like a gfx regression.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Yeah, this is most likely the result of the opacity reduction on the low-res tiles. Setting the layers.low-precision-opacity pref to 1.0 should make it go away.

I guess we should back out bug 1020778 and find some other solution :(

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Bug 1020778 does need a solution, so rather than just backing things out, let's first figure out what fixes both.  If we can't find a solution that deals with both of these issues, we'll have to get UX involved to make a call as to which one should be kept.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

From comment 0 it sounds like this happens on a number of websites. Do you have a specific URL that reproduces this, that doesn't require creating an account to view?

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP

Comment 9

[Dean R. Gomez (DeanG)]

Here is a link to a specific forum thread on Polycount:
http://www.polycount.com/2014/07/07/polycount-recap-29/

If that doesn't seem to work, you can try:
http://www.imgur.com

Hope this helps.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

That's perfect, thanks. My patch does improve the behavior on this website significantly. The low-res background ends up being a slightly different shade of grey and there's still a clear border but it's less noticeable than before.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Not really sure who should review this.

Comment 12

[Benoit Girard (:BenWa)]

Comment on attachment 8458926 [details] [diff] [review]
Patch

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

r- because I'm not convinced this properly handles semi-transparent layers.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +302,5 @@
>  
> +  gfxRGBA backgroundColor(0);
> +  if (gfxPrefs::LowPrecisionOpacity() < 1.0f) {
> +    // In cases where we are drawing the low-precision buffer with a reduced
> +    // opacity (see comment below) we want to draw the layer's background color

I don't like 'see comment below' because it's easy to break that with refactoring without noticing but I don't have a better suggestion.

@@ +317,5 @@
>    // Render the low and high precision buffers. Reduce the opacity of the
>    // low-precision buffer to make it a little more subtle and less jarring.
>    // In particular, text rendered at low-resolution and scaled tends to look
>    // pretty heavy and this helps mitigate that.
> +  RenderLayerBuffer(mLowPrecisionTiledBuffer, backgroundColor, aEffectChain,

Have you tested this if the layer is semi-transparent? I'd assume the background color will still be opaque if the opacity is applied via an animation for example. In this case the layer will no longer be transparent and will instead just blend with the background.

@@ +340,5 @@
>  
>  
>  void
>  TiledContentHost::RenderTile(const TileHost& aTile,
> +                             gfxRGBA aBackgroundColor,

So this background color is only used for low res stuff? Meanwhile there's still the code to draw the high-res background color? This isn't clean.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to Benoit Girard (:BenWa) from comment #12)
> Have you tested this if the layer is semi-transparent? I'd assume the
> background color will still be opaque if the opacity is applied via an
> animation for example. In this case the layer will no longer be transparent
> and will instead just blend with the background.

My brain is a little fried but I wanted to mention before I forget: I mostly copied this background-drawing code from the overscroll background-drawing code. I did a quick test and the page at http://people.mozilla.org/~kgupta/bug/1038416-1.html suffers from a problem that if you overscroll the div the background becomes darker. So you're right that we don't handle this case properly, and that it will affect both overscroll and low-res (with the patch on this bug). I haven't tested playing with the opacity CSS property yet; I assume that will behave differently from an rgba background with non-1.0 alpha component.

Comment 14

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> I mostly
> copied this background-drawing code from the overscroll background-drawing
> code.

Note that the overscroll background-drawing code suffers from the problems described in bug 1037624. It's possible that these problems, and their suggested solutions, apply to this as well.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Kill opacity reduction on content that already has alpha

After some experimenting I realized that in cases where the layer already has opacity < 1.0 or where the background is partially/fully transparent, we probably don't want to bother reducing the opacity of low-res content at all. The problem this was intended to fix (that of heavy text, see bug 1020778) is only really visible when the layer is opaque and with a solid background to begin with.

This approach also has the benefit of removing the opacity reduction from the Gaia homescreen which a lot of people (including partners) were complaining about, and that UX wanted to fix.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Draw the background color under reduced-opacity content

With part 1 in place we should now be safe to just draw the background color under the low-res content whenever we are reducing the opacity. It will only ever come into play when there's no pre-existing opacity/alpha.

Comment 17

[Benoit Girard (:BenWa)]

Comment on attachment 8460236 [details] [diff] [review]
Part 1 - Kill opacity reduction on content that already has alpha

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +344,5 @@
> +  // already has some opacity, we want to skip this behaviour. Otherwise
> +  // we end up changing the expected overall transparency of the content,
> +  // and it just looks wrong.
> +  float lowPrecisionOpacityReduction = gfxPrefs::LowPrecisionOpacity();
> +  if (aOpacity < 1.0f) {

What you explain the comment is fairly simple but I think that the code is harder to read. How about removeing this if and changing the one below to something like this:

if (gfxPrefs::LowPrecisionOpacity() < 1.0f && aOpacity == 1f) {
  ...
} else {
  lowPrecisionOpacityReduction = 1.0f;
}

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8460239 [details] [diff] [review]
Part 2 - Draw the background color under reduced-opacity content

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

Patch looks fine to me but ultimately I think it should be Botond to sign off on this since he's aware of the problems and I think also has a plan.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +391,5 @@
>  
>  
>  void
>  TiledContentHost::RenderTile(const TileHost& aTile,
> +                             gfxRGBA* aBackgroundColor,

why not pass by const ref and check for .a == 0?

Comment 19

[Kartikaya Gupta (email:[e-mail])]

(In reply to Benoit Girard (:BenWa) from comment #18)
> >  TiledContentHost::RenderTile(const TileHost& aTile,
> > +                             gfxRGBA* aBackgroundColor,
> 
> why not pass by const ref and check for .a == 0?

That's what I was doing in the original patch but it means that I pass around a gfxRGBA(0) in a bunch of cases that it's not used in (including the high-precision RenderTile calls). I thought that's the ugliness you were referring to at the bottom of comment 12.

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8460236 [details] [diff] [review]
Part 1 - Kill opacity reduction on content that already has alpha

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +352,5 @@
> +  if (lowPrecisionOpacityReduction < 1.0f) {
> +    gfxRGBA backgroundColor(0);
> +    // Find the background color
> +    for (ContainerLayer* ancestor = GetLayer()->GetParent(); ancestor; ancestor = ancestor->GetParent()) {
> +      if (ancestor->GetFrameMetrics().IsScrollable()) {

A reader unfamiliar with the back story might wonder why we have to find a scrollable layer to get the background color.

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8460239 [details] [diff] [review]
Part 2 - Draw the background color under reduced-opacity content

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

This looks reasonable to me. I believe this is not affected by the issues described in bug 1037624, as those issues only apply if the layer is transparent, but here we are only drawing the background separately if the layer is opaque.

::: gfx/layers/composite/TiledContentHost.cpp
@@ +391,5 @@
>  
>  
>  void
>  TiledContentHost::RenderTile(const TileHost& aTile,
> +                             gfxRGBA* aBackgroundColor,

I could go either way on this one, but if you keep the pointer, make it a pointer to const please.

@@ +420,5 @@
> +    aEffectChain.mPrimaryEffect = new EffectSolidColor(ToColor(*aBackgroundColor));
> +    nsIntRegionRectIterator it(aScreenRegion);
> +    for (const nsIntRect* rect = it.Next(); rect != nullptr; rect = it.Next()) {
> +      Rect graphicsRect(rect->x, rect->y, rect->width, rect->height);
> +      mCompositor->DrawQuad(graphicsRect, aClipRect, aEffectChain, 1.0, aTransform);

My understanding is that we expect aOpacity to be 0 here (otherwise lowPrecisionOpacityReduction would have been set to 1, and aBackgroundColor to nullptr). Do you think it's worth adding a warning here if that's not the case (in case future code changes break this invariant)?

Comment 22

[Kartikaya Gupta (email:[e-mail])]

(In reply to Benoit Girard (:BenWa) from comment #17)
> What you explain the comment is fairly simple but I think that the code is
> harder to read. How about removeing this if and changing the one below to
> something like this:
> 
> if (gfxPrefs::LowPrecisionOpacity() < 1.0f && aOpacity == 1f) {
>   ...
> } else {
>   lowPrecisionOpacityReduction = 1.0f;
> }

I rearranged the code like you suggested and took it a little bit further. This allowed me to pull out the lowPrecisionOpacityReduction assignment into a single line in part 2.

(In reply to Botond Ballo [:botond] from comment #20)
> > +      if (ancestor->GetFrameMetrics().IsScrollable()) {
> 
> A reader unfamiliar with the back story might wonder why we have to find a
> scrollable layer to get the background color.

Updated the comment to mention this.

(In reply to Botond Ballo [:botond] from comment #21)
> This looks reasonable to me. I believe this is not affected by the issues
> described in bug 1037624, as those issues only apply if the layer is
> transparent, but here we are only drawing the background separately if the
> layer is opaque.

Correct.

> >  TiledContentHost::RenderTile(const TileHost& aTile,
> > +                             gfxRGBA* aBackgroundColor,
> 
> I could go either way on this one, but if you keep the pointer, make it a
> pointer to const please.

Good call, made it a const gfxRGBA*

> My understanding is that we expect aOpacity to be 0 here (otherwise
> lowPrecisionOpacityReduction would have been set to 1, and aBackgroundColor
> to nullptr). Do you think it's worth adding a warning here if that's not the
> case (in case future code changes break this invariant)?

We expect aOpacity to be 1 there (I assume that's what you meant). I added a MOZ_ASSERT to this effect.

Comment 23

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c0569c1951f2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/bff51424155f

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c0569c1951f2
https://hg.mozilla.org/mozilla-central/rev/bff51424155f

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f2b359e60a73
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/1df6838feb5b

Comment 28

[Paladin]

Could you please provide the video for me to verify this bug. :)