Closed Bug 681033 Opened 13 years ago Closed 13 years ago

With GL Layers enabled, zooming in Fennec causes flash of out-of-position content

Categories

(Core :: Graphics, defect)

Product:
Core
Component:
Graphics
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: ajuma, Assigned: ajuma)

References

Details

(Keywords: regression)

Attachments

(1 file, 1 obsolete file)

Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint, v2
12.86 KB, patch
cjones
: review+
Details | Diff | Splinter Review 
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.
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.
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.
This issue turns out to be a regression from the landing of Bug 637852.
Blocks: 637852
Keywords: regression
(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?
Did you try using extra glFinish()s to rule out a race condition?
(Or sleeps would work, too.)
(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.
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.
BTW, can you repro this in a desktop build?
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.
(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.
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.
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.
Is there any reason why we shouldn't just change EndTransaction to not repaint? Ali says he tried that with some success.
That's my vote, since it would match the behavior of basic layers.  roc, objections?

        Attachment #561560 -
        Flags: review?(jones.chris.g)

    
    

    
  
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 ;).

        Attachment #561560 -
        Flags: review?(jones.chris.g)

    
    

    
  


  

        Attachment #561560 -
        Attachment is obsolete: true

        Attachment #562139 -
        Flags: review?(jones.chris.g)

    
    

    
  
Comment on attachment 562139 [details] [diff] [review]
Prevent ShadowLayers::RecvUpdate from triggering an immediate repaint, v2

Thanks!

        Attachment #562139 -
        Flags: review?(jones.chris.g) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/8fdc98522174
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: