Closed Bug 793365 Opened 12 years ago Closed 12 years ago

Corruption when panning around in async pan/zoomed pages

Categories

(Core :: Graphics: Layers, defect)

Product:
Core
Component:
Graphics: Layers
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Project Flags:
blocking-basecamp +

People

(Reporter: cjones, Assigned: cjones)

References

Details

(Keywords: dogfood, Whiteboard: [LOE:S])

Attachments

(1 file, 2 obsolete files)

Make sure (again) that the entire buffer is valid when we might be asynchronously composited, v3
8.15 KB, patch
roc
: review+
Details | Diff | Splinter Review 
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.
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
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 ;).
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.
https://hg.mozilla.org/mozilla-central/rev/0bd66e216a43
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: