983169 - Panning and pinch zooming causes page to whiteout

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Lars Viklund]

Attachment: Screenshot_2014-03-13-15-46-14.png

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0 (Beta/Release)
Build ID: 20140312030201

Steps to reproduce:

Navigate to http://umumat.se
Pan towards the right side of the page
Pan around in general.


Actual results:

In certain page positions the page shows regions of white on the right and bottom, flickering in and out of existence as you move. It stays stable good/broken when not moving and repeats as you pass over the same locations.


Expected results:

No white position-dependent regions of white.

Comment 1

[Lars Viklund]

Attachment: Screenshot_2014-03-13-15-46-02.png

Comment 2

[Lars Viklund]

Attachment: Screenshot_2014-03-13-15-46-23.png

Comment 3

[Lars Viklund]

This is a Sony Ericson Xperia V (LT25i) running the recent Android 4.3, w/ Telia as telco.

Comment 4

[Aaron Train [:aaronmt]]

I do indeed see this after some panning and pinch-zooming on the content in the URL, content will essentially whiteout.

Screenshot from my Galaxy SIV (Android 4.4.2)

When this happens I do indeed see the typical 'Aborting draw due to resolution change' messages flood console. E.g, 

D/GeckoLayerClient( 3707): Aborting draw due to resolution change: 2.5859103 != 2.587752

CC'ing some graphics folks

Comment 5

[Aaron Train [:aaronmt]]

Attachment: Galaxy SIV (Android 4.4.42) - screenshot

Comment 6

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

Chris, can you look into this? Looks like a tiling/progressive update bug.

Comment 7

[Chris Lord [:cwiiis]]

Yup, might as well knock all of these on the head at once...

Comment 8

[Chris Lord [:cwiiis]]

It seems that the 'cancel if zoom has changed' behaviour causes this, but I've not looked into why that is yet. Will take a look after bug 83208, which unfortunately, does not fix this (at least, it doesn't seem that way at the moment).

Possibly there a situation that cancelling is affecting the valid region incorrectly somehow, which would also explain why there are sometimes odd partially-black/coloured tiles in the low precision buffer.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Fix up composition of tiled buffers

The main error previously was to use the valid region of the layer instead of the valid region of the tiled buffers, which more accurately represents what's been drawn when using progressive rendering.

This doesn't solve all the issues, there are still odd single-coloured blocks in the low precision buffer, and I've not managed to track down why that is yet. This makes things quite a bit better at least.

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8397170 [details] [diff] [review]
Fix up composition of tiled buffers

Removing the flags because more debugging shows that the regions this ends up calculating aren't right yet and I want to get this right :p

Comment 11

[Chris Lord [:cwiiis]]

Attachment: Fix up composition of tiled buffers v2

This appears to be correct in my testing.

I believe the other rendering error of odd patches of colour appearing in the low precision buffer is down to something on the client side that I'm continuing to investigate.

Comment 12

[Chris Lord [:cwiiis]]

Comment on attachment 8397305 [details] [diff] [review]
Fix up composition of tiled buffers v2

Moving review over to BenWa.

Comment 13

[Chris Lord [:cwiiis]]

Interestingly, the problem with bad-looking contents in the low precision buffer is much worse with per-tile drawing on. I don't know what this signifies just yet.

Comment 15

[Chris Lord [:cwiiis]]

Brill, the reason this is worse with per-tile drawing is that the error is client-side, and it's down to the resolution of the layer buffer not being taken into account in various areas in ValidateTile, I believe.

I'll try to fix up both paths.

Comment 16

[Chris Lord [:cwiiis]]

Attachment: Fix handling of TiledLayerBuffer resolution in TiledContentClient

I tested both paths and this seems to work correctly.

Comment 17

[Chris Lord [:cwiiis]]

Note: I still think there's some oddness with regards to the handling of the valid regions of high and low precision buffers, but this gets us into as good a state as we've ever been. It'll be tricky to fix everything without a comprehensive way of testing and tracking regressions.

With these patches applied, there is no more obvious brokenness on fennec for me.

Comment 18

[Benoit Girard (:BenWa)]

Comment on attachment 8397305 [details] [diff] [review]
Fix up composition of tiled buffers v2

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

::: gfx/layers/composite/TiledContentHost.cpp
@@ +382,3 @@
>    }
> +
> +  // If we aren't drawing the primary layer buffer, make sure it gets masked

I don't understand this comment. What's the primary layer buffer? Perhaps this should be:
If we're drawing the low res buffer...

I think what you really mean is:
If we're drawing the low res buffer, make sure we mask out area covered by the full res buffer to reduce overdraw.

@@ +385,5 @@
> +  // out of this draw.
> +  nsIntRegion maskRegion;
> +  if (resolution != mTiledBuffer.GetResolution()) {
> +    maskRegion = mTiledBuffer.GetValidRegion();
> +    maskRegion.ScaleRoundOut(layerScale.width, layerScale.height);

Shouldn't the mask round in?

@@ +389,5 @@
> +    maskRegion.ScaleRoundOut(layerScale.width, layerScale.height);
> +  }
> +
> +  // Make sure the resolution and difference in frame resolution are accounted
> +  // for in the layer transform.

unrelated: I wonder if we properly handle low res rotated layers. If we ever rotate around a point that isn't 0,0 I think we might not handle that transformation correctly.

Comment 19

[Chris Lord [:cwiiis]]

(In reply to Benoit Girard (:BenWa) from comment #18)
> Comment on attachment 8397305 [details] [diff] [review]
> Fix up composition of tiled buffers v2
> 
> Review of attachment 8397305 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/composite/TiledContentHost.cpp
> @@ +382,3 @@
> >    }
> > +
> > +  // If we aren't drawing the primary layer buffer, make sure it gets masked
> 
> I don't understand this comment. What's the primary layer buffer? Perhaps
> this should be:
> If we're drawing the low res buffer...
> 
> I think what you really mean is:
> If we're drawing the low res buffer, make sure we mask out area covered by
> the full res buffer to reduce overdraw.

Yup, I'll make this comment clearer.

> @@ +385,5 @@
> > +  // out of this draw.
> > +  nsIntRegion maskRegion;
> > +  if (resolution != mTiledBuffer.GetResolution()) {
> > +    maskRegion = mTiledBuffer.GetValidRegion();
> > +    maskRegion.ScaleRoundOut(layerScale.width, layerScale.height);
> 
> Shouldn't the mask round in?

Yes, but region has no RoundIn :( I'll add a comment so this doesn't get (even more) confusing over time...

> @@ +389,5 @@
> > +    maskRegion.ScaleRoundOut(layerScale.width, layerScale.height);
> > +  }
> > +
> > +  // Make sure the resolution and difference in frame resolution are accounted
> > +  // for in the layer transform.
> 
> unrelated: I wonder if we properly handle low res rotated layers. If we ever
> rotate around a point that isn't 0,0 I think we might not handle that
> transformation correctly.

I don't even want to think about transforms here right now :p I've got a feeling we disable progressive rendering/low precision when there are transforms, but maybe not...

Comment 20

[Chris Lord [:cwiiis]]

Pushed to inbound:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/3f0dd8c73cf6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/092a737e9451

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/3f0dd8c73cf6
https://hg.mozilla.org/mozilla-central/rev/092a737e9451

Comment 23

[Kevin Brosnan [Ex-Mozilla]]

This needs uplift into Beta.

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Repeating comment 23 with a ni? this time - please nom for uplift

Comment 25

[Chris Lord [:cwiiis]]

ack, sorry, somehow managed to miss this needinfo... I'm not sure if it's sensible to uplift this without also uplifting the work kats did after this. I'll n? him to see if that's correct or not and nominate this in the meantime.

Comment 26

[Chris Lord [:cwiiis]]

Comment on attachment 8397305 [details] [diff] [review]
Fix up composition of tiled buffers v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: White flickering when panning/zooming and during animations
Testing completed (on m-c, etc.): Been on m-c for a while now
Risk to taking this patch (and alternatives if risky): Risk of replacing current brokenness with a new, different brokenness.
String or IDL/UUID changes made by this patch: None

Comment 27

[Chris Lord [:cwiiis]]

Comment on attachment 8397903 [details] [diff] [review]
Fix handling of TiledLayerBuffer resolution in TiledContentClient

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: White flickering when panning/zooming and during animations
Testing completed (on m-c, etc.): Been on m-c for a while now
Risk to taking this patch (and alternatives if risky): Risk of replacing current brokenness with a new, different brokenness.
String or IDL/UUID changes made by this patch: None

Comment 28

[Chris Lord [:cwiiis]]

kats, are there any other bugs related to this that will need uplifting?

Comment 29

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

The two things I fixed (bug 993554 and bug 992218) are both already on beta. I'm not aware of anything else that needs uplifting.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/35fe7342d5aa
https://hg.mozilla.org/releases/mozilla-beta/rev/fd3adb4a2d16

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/35fe7342d5aa
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/fd3adb4a2d16