Corruption when panning around in async pan/zoomed pages

RESOLVED FIXED in mozilla18

Status

()

Core
Graphics: Layers
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

({dogfood})

unspecified
mozilla18
ARM
Gonk (Firefox OS)
dogfood
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 2 obsolete attachments)

Jeez, I thought for sure this had been filed, but went looking for it tonight and couldn't find it.  Anyway.

STR
 (1) Go to http://people.mozilla.com/~cjones/grid.html
 (2) Pan down so that top-left is around 0,100 (to get the damn titlebar out of the way)
 (3) Put your finger on 300x400
 (4) Quickly pan it to the top-left

There's corruption on the right side of the screen that looks like us messing up the computations of what to rotate.

It's very odd that this doesn't happen with non-async-pan/zoom content, which makes me suspect something to do with displayport calculations.  This code hasn't been tested heavily for a while now that fennec has a different painting path.  (Sigh.)
I suspect something here must be changing layers coordinate space but we manage to keep layers alive across the coordinate-space change.  Not sure yet what that might be; scrollTo and displayport shaping are the first two things I'm going to check.
Summary: Corruption when panning around in async pan/zoomed pages (buffer rotation going wrong?) → Corruption when panning around in async pan/zoomed pages (front->back update or buffer rotation going wrong?)
I was able to reproduce these artifacts in a debug build, but no assertions are failing :(.
Disabling buffer rotation doesn't make the artifacts go away.
Right.  PAINT_MIGHT_RESAMPLE is what we used to use with shadowable thebes layers.  We're not using that anymore.  We're seeing the artifacts of unpainted junk here.

Flipping that flag back on makes the bugs go away.
When writing the patch here, I realized we're not accounting for resampling of asynchronously animated ThebesLayers; we should set PAINT_MIGHT_RESAMPLE for them, but we're not.  It's pretty unlikely that will lead to artifacts but it can.
Created attachment 663659 [details] [diff] [review]
Make sure (again) that the entire buffer is valid when we might be asynchronously composited
Attachment #663659 - Flags: review?(roc)
I really want to write a test for this but I don't know how.
None of the buffer rotation or front->back update code seems to have broken, amazingly.  It's the controlling code that did.
Summary: Corruption when panning around in async pan/zoomed pages (front->back update or buffer rotation going wrong?) → Corruption when panning around in async pan/zoomed pages
Keywords: dogfood
Comment on attachment 663659 [details] [diff] [review]
Make sure (again) that the entire buffer is valid when we might be asynchronously composited

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

::: gfx/layers/basic/BasicThebesLayer.cpp
@@ +170,5 @@
> +    if (owner) {
> +      if (TabChild* window = owner->GetOwningTabChild()) {
> +        if (window->IsAsyncPanZoomEnabled()) {
> +          flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;
> +        }

I think it would be clearer to have a separate flag to indicate this, say PAINT_VALIDATE_ENTIRE_BUFFER. And does that need to disable rotation? I don't think it does.

Also, how about simply asking the layer manager if it has a shadow before setting that flag? Isn't that the right thing to do here, ultimately?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #9)
> Comment on attachment 663659 [details] [diff] [review]
> Make sure (again) that the entire buffer is valid when we might be
> asynchronously composited
> 
> Review of attachment 663659 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicThebesLayer.cpp
> @@ +170,5 @@
> > +    if (owner) {
> > +      if (TabChild* window = owner->GetOwningTabChild()) {
> > +        if (window->IsAsyncPanZoomEnabled()) {
> > +          flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;
> > +        }
> 
> I think it would be clearer to have a separate flag to indicate this, say
> PAINT_VALIDATE_ENTIRE_BUFFER. And does that need to disable rotation? I
> don't think it does.
> 

Do we force an intermediate fbo for resampled thebes layers with rotations during compositing?  If not, we can choose between not seaming and possibly a little more perf, I guess.  This code will only run currently for b2g, and there MovePixels() is quite cheap, so the current behavior of MIGHT_RESAMPLE is ok by me.

> Also, how about simply asking the layer manager if it has a shadow before
> setting that flag? Isn't that the right thing to do here, ultimately?

Sure, sounds good.  We used to be able to ask the layer, but that ability went away.  Old habits die hard ;).
Created attachment 663765 [details] [diff] [review]
Make sure (again) that the entire buffer is valid when we might be asynchronously composited, v2

This adds a BasicManager()->HasShadowManager() check.

I'd rather defer the decision about an additional PAINT_VALIDATE_ENTIRE_BUFFER flag to another bug, because there are some tradeoffs involved that I don't think we have enough data to evaluate well yet.
Attachment #663659 - Attachment is obsolete: true
Attachment #663659 - Flags: review?(roc)
Attachment #663765 - Flags: review?(roc)
I do think it's a good idea though.
Comment on attachment 663765 [details] [diff] [review]
Make sure (again) that the entire buffer is valid when we might be asynchronously composited, v2

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

::: gfx/layers/basic/BasicThebesLayer.cpp
@@ +171,5 @@
> +      nsIWidget* owner = BasicManager()->GetRetainerWidget();
> +      if (owner) {
> +        if (TabChild* window = owner->GetOwningTabChild()) {
> +          if (window->IsAsyncPanZoomEnabled()) {
> +            flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;

Actually I was suggesting we just set flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE whenever BasicManager()->HasShadowManager(), on the assumption that there will be async animations or zooming that requires it. And then take out rotations altogether when everything is OMTC. Maybe we shouldn't do that though. Let's do this for now.

But, how about moving this code to LayerManager::IsAsyncPanZoomEnabled and, optionally, caching this value in there (computing it once per transaction)?
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> Comment on attachment 663765 [details] [diff] [review]
> Make sure (again) that the entire buffer is valid when we might be
> asynchronously composited, v2
> 
> Review of attachment 663765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/basic/BasicThebesLayer.cpp
> @@ +171,5 @@
> > +      nsIWidget* owner = BasicManager()->GetRetainerWidget();
> > +      if (owner) {
> > +        if (TabChild* window = owner->GetOwningTabChild()) {
> > +          if (window->IsAsyncPanZoomEnabled()) {
> > +            flags |= ThebesLayerBuffer::PAINT_WILL_RESAMPLE;
> 
> Actually I was suggesting we just set flags |=
> ThebesLayerBuffer::PAINT_WILL_RESAMPLE whenever
> BasicManager()->HasShadowManager(), on the assumption that there will be
> async animations or zooming that requires it. And then take out rotations
> altogether when everything is OMTC. Maybe we shouldn't do that though. Let's
> do this for now.
> 

I don't think we should do that.  But we do need to fix async animations.

> But, how about moving this code to LayerManager::IsAsyncPanZoomEnabled and,
> optionally, caching this value in there (computing it once per transaction)?

Do you mean literally LayerManager, or BasicLayerManager?  LayerManager doesn't know anything about nsIWidget.  Will assume you meant BasicLayerManager.
Created attachment 664210 [details] [diff] [review]
Make sure (again) that the entire buffer is valid when we might be asynchronously composited, v3
Attachment #663765 - Attachment is obsolete: true
Attachment #663765 - Flags: review?(roc)
Attachment #664210 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/0bd66e216a43
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18

Updated

6 years ago
Duplicate of this bug: 792638
You need to log in before you can comment on or make changes to this bug.