Last Comment Bug 681033 - With GL Layers enabled, zooming in Fennec causes flash of out-of-position content
: With GL Layers enabled, zooming in Fennec causes flash of out-of-position con...
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Android
: -- normal (vote)
: mozilla9
Assigned To: Ali Juma [:ajuma]
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: opengl-mobile 637852
  Show dependency treegraph
 
Reported: 2011-08-22 13:29 PDT by Ali Juma [:ajuma]
Modified: 2011-09-26 13:09 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint (12.10 KB, patch)
2011-09-21 13:53 PDT, Ali Juma [:ajuma]
no flags Details | Diff | Splinter Review
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint, v2 (12.86 KB, patch)
2011-09-23 12:53 PDT, Ali Juma [:ajuma]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Ali Juma [:ajuma] 2011-08-22 13:29:57 PDT
When zooming in or out, the entire screen very briefly flashes out-of-position content.

This sounds similar to Bug 666385, but the patch that resolves that bug doesn't fix this issue.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-08-29 15:58:06 PDT
One possibility is that we get a PLayers:Update with content at the right resolution, but the frontend doesn't update the "viewport scale" in chrome before the next repaint.  Those updates need to be atomic in general.  However, if this were the case I'd be really surprised that it doesn't show up with basic layers, since the frontend/resolution/viewport code is all the same.  But maybe the framerate with GL is higher enough that we actually see this bug there.

Another possibility is that texture updates are not respecting GL pipeline semantics, which might happen e.g. if the driver just directly memcpy()s the new bits in a glTexSubImage2D() to texture memory.  If adding a glFinish() before and after the shadow-layer texture updates makes this problem go away, it would be suspicious.
Comment 2 Ali Juma [:ajuma] 2011-09-02 13:40:09 PDT
I used MOZ_GL_DEBUG to force a glFinish() after every GL call, but this had no effect on the problem.

This problem only happens at the end of a pinch zoom, after the screen is no longer being touched -- this is when the frontend is doing browser.finishFuzzyZoom().

When I set a breakpoint on LayerManagerOGL::Render to step through frame-by-frame, there's a single frame that's drawn with what appears to be twice the required zoom; that is, when zooming out this frame is drawn much larger than it should be, and when zooming in it's drawn much smaller than it should be. The previous and following frames look fine.

If I comment out the assignments to state.mXResolution and state.mYResolution in PresShell:SetResolution, then zooming occurs without any flash though the content stays fuzzy-looking.
Comment 3 Ali Juma [:ajuma] 2011-09-06 12:01:40 PDT
This issue turns out to be a regression from the landing of Bug 637852.
Comment 4 Ali Juma [:ajuma] 2011-09-08 15:06:26 PDT
(In reply to Ali Juma [:ajuma] from comment #3)
> This issue turns out to be a regression from the landing of Bug 637852.

To try to narrow this down, I backed out the patches for Bug 637852 from a relatively recent revision (experimenting with the code as it existed back when Bug 637852 landed isn't a great option, since the GL Layers code on mobile was in pretty rough shape back then, with many pages not rendering at all). It turns out that backing out patches 6...27 makes the issue go away (but backing out 7...27 does not). 

Patch 6 makes FrameLayerBuilder::BuildContainerLayerFor responsible for scaling. 

Since we're only seeing this zooming issue on mobile with GL Layers, it seems there's some problematic interaction between patch 6, RenderFrameParent::BuildLayer (more specifically, the call to TransformShadowTree()), and GL Layers. 

Any suggestions?
Comment 5 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-08 15:36:32 PDT
Did you try using extra glFinish()s to rule out a race condition?
Comment 6 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-08 15:36:54 PDT
(Or sleeps would work, too.)
Comment 7 Ali Juma [:ajuma] 2011-09-08 15:49:31 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5)
> Did you try using extra glFinish()s to rule out a race condition?

Yes, I defined the environment variable MOZ_GL_DEBUG (described at https://developer.mozilla.org/en/Debugging/Debugging_OpenGL) which forces a glFinish() after every GL call. To make sure this actually works on Android, I also tried defining MOZ_GL_DEBUG_VERBOSE and this did produce the expected messages before and after GL calls.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-08 17:07:01 PDT
Oh sorry, missed that above.

OK, my next guess is that the layer-tree update and view update (supplied by the frontend) aren't being applied atomically.  Here's my guess

 UI                              content
----------                      ---------
 pinch-zoom to 2x
 (1) set view scale to 2x
 send request to redraw@2x
                                 finish redrawing
                                 <-- PLayers:Update
 (2) layers have pixels@2x
 [**] (3) repaint with 2x scale
 (1) set view scale to 1x

If that's right, it's the [**] that would cause this, since it would upscale the content that was repainted with double resolution, resulting in a flash of 4x content.

You can log the numbered events above at (1) nsContentView::SetScale, (2) RenderFrameParent::ShadowLayersUpdated (3) LayerManagerOGL::Render.

If this is indeed the problem, then I'm not 100% sure why we're not seeing it with basic layers.  I have a hand-wavy guess.  Let's diagnose first though.
Comment 9 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-08 17:08:53 PDT
BTW, can you repro this in a desktop build?
Comment 10 Ali Juma [:ajuma] 2011-09-09 10:50:02 PDT
Logging events, here's a typical pinch-zoom:

LayerManagerOGL::Render 
nsContentView::SetScale 3.060300
nsContentView::SetScale 3.280700
LayerManagerOGL::Render 
LayerManagerOGL::Render 
nsContentView::SetScale 3.849700
nsContentView::SetScale 3.849700
LayerManagerOGL::Render <--- Correctly scaled after this completes
LayerManagerOGL::Render <--- Wrong scale after this completes
RenderFrameParent::ShadowLayersUpdated 
LayerManagerOGL::Render <--- Back to normal after this completes

Note that in every pinch-zoom-out: 
-the scale values are non-decreasing.
-content is correctly scaled after the first Render following the final SetScale, but content is incorrectly scaled after the next Render.
-ShadowLayersUpdated happens in between the content being incorrectly scaled and the content returning to normal scale.
Comment 11 Ali Juma [:ajuma] 2011-09-09 10:51:04 PDT
(In reply to Chris Jones [:cjones] [:warhammer] from comment #9)
> BTW, can you repro this in a desktop build?

Yes, the same flash occurs in a desktop build.
Comment 12 Ali Juma [:ajuma] 2011-09-20 12:13:49 PDT
Here's what's happening:

During the pinch zoom, the view scale (on the chrome side) increases (say to 2X). At the end of the pinch zoom, the content side increases resolution (to 2X) and sends an update to the chrome side. This update is received in ShadowLayersParent::RecvUpdate, where the higher resolution content is uploaded into the ShadowThebesLayer's buffer, and the ShadowContainerLayer's transform is set to 0.5X. So far, the story is the same with Basic and GL layers. 

Things diverge when RecvUpdate calls EndTransaction. For Basic layers, this does not trigger an immediate repaint, since the transaction was begun with a null target. For GL layers, however, a repaint does immediately occur. This is the repaint responsible for the bug. Since the ShadowContainerLayer's ShadowTransform has not yet been updated, its EffectiveTransform will still be 2X, meaning the ShadowThebesLayer also still has an effective transform of 2X. But the ShadowThebesLayer has content whose resolution has already been doubled, so the content is actually rendered at 4X rather than 2X.

Then things converge again. For both Basic and GL layers, nsDisplayList::PaintForFrame calls FrameLayerBuilder::BuildContainerLayerFor, which ultimately leads to a call to TransformShadowTree. This updates the ShadowContainerLayer's ShadowTransform to take into account the new transform that was set during RecvUpdate. Then, PaintForFrame calls EndTransaction, triggering a repaint. This time, the ShadowContainerLayer's effective transform will be 1X, and hence the ShadowThebesLayer's content is painted correctly. 

There are at least two possible fixes:

1) Add a variant of EndTransaction (say EndTransactionWithoutRepaint) that explicitly does not repaint, and have RecvUpdate call this instead of EndTransaction.  

2) Change the behaviour of RecvUpdate so that when it updates a layer's transform, it also updates the layer's ShadowTransform accordingly.


(1) has the benefit of being easier to implement, and makes the behaviour of GL layers consistent with Basic layers.

(2) makes sense if we really need RecvUpdate to trigger an immediate repaint when using GL layers.
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-20 14:26:42 PDT
#2 seems most logical to me.
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-20 14:58:46 PDT
Nice find!

We don't want to repaint while the content process is blocked on Update().  #1 makes more sense to me for now ;).  If we did #2, we would want to do the same for basic layers.
Comment 15 Joe Drew (not getting mail) 2011-09-20 15:02:43 PDT
Is there any reason why we shouldn't just change EndTransaction to not repaint? Ali says he tried that with some success.
Comment 16 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-20 15:04:38 PDT
That's my vote, since it would match the behavior of basic layers.  roc, objections?
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-09-20 15:28:54 PDT
No, that's fine.
Comment 18 Ali Juma [:ajuma] 2011-09-21 13:53:03 PDT
Created attachment 561560 [details] [diff] [review]
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint
Comment 19 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-22 17:50:53 PDT
Comment on attachment 561560 [details] [diff] [review]
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint

># HG changeset patch
># Parent 3d181775477ffbca5fe73d5b0d2d99a45ff1a94a
># User Ali Juma <ajuma@mozilla.com>
>Bug 681033 - Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint.
>
>diff --git a/gfx/layers/Layers.h b/gfx/layers/Layers.h
>--- a/gfx/layers/Layers.h
>+++ b/gfx/layers/Layers.h
>@@ -354,17 +354,18 @@ public:
>   /**
>    * Finish the construction phase of the transaction, perform the
>    * drawing phase, and end the transaction.
>    * During the drawing phase, all ThebesLayers in the tree are
>    * drawn in tree order, exactly once each, except for those layers
>    * where it is known that the visible region is empty.
>    */
>   virtual void EndTransaction(DrawThebesLayerCallback aCallback,
>-                              void* aCallbackData) = 0;
>+                              void* aCallbackData,
>+                              bool aPreventDrawing = false) = 0;
> 

"aPreventDrawing" is a little non-specific.  But see below.

Instead of using a boolean here, use a flags enum

 enum EndTransactionFlags {
   DEFAULT = 0,
   NO_IMMEDIATE_REDRAW = 1 << 1,
 };
 EndTransaction(..., PRUin32 flags);

with documentation, of course :).

>diff --git a/gfx/layers/d3d9/LayerManagerD3D9.cpp b/gfx/layers/d3d9/LayerManagerD3D9.cpp
>+  if (mRooti && !aPreventDrawing) {

I don't think that compiles ;).
Comment 20 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-22 17:51:56 PDT
er 1 << 0
Comment 21 Ali Juma [:ajuma] 2011-09-23 12:53:31 PDT
Created attachment 562139 [details] [diff] [review]
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint, v2
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-09-23 16:36:52 PDT
Comment on attachment 562139 [details] [diff] [review]
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint, v2

Thanks!
Comment 24 Benoit Girard (:BenWa) 2011-09-26 13:09:37 PDT
https://hg.mozilla.org/mozilla-central/rev/8fdc98522174

Note You need to log in before you can comment on or make changes to this bug.