Closed Bug 732756 Opened 10 years ago Closed 10 years ago

[maple] Checkerboard sometimes shows up with squashed aspect ratio

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox13 verified, firefox14 verified, blocking-fennec1.0 beta+)

VERIFIED FIXED
Firefox 14
Tracking Status
firefox13 --- verified
firefox14 --- verified
blocking-fennec1.0 --- beta+

People

(Reporter: kats, Assigned: cwiiis)

References

Details

(Keywords: regression, Whiteboard: maple)

Attachments

(2 files, 3 obsolete files)

With a build from https://hg.mozilla.org/projects/maple/rev/5c3d11c465f1 I am sometimes seeing odd behaviour with the checkerboard. Say I fling into to a new area of the page, and it has checkerboarding. After the fling stops I sometimes see the checkerboard get "squished" vertically so that the boxes are shorter than they are wide. The content then fills in on top of it. I'm pretty sure I wasn't seeing this before, but I don't know if it's a regression from bug 732013 or something else. I can usually reproduce this by going to support.mozilla.org, zooming in, and then flinging around the page.
I occasionally see this, but not always, which is odd... I'll see if I can track this down on Monday.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
erk, so the mask set on the checkerboard layer isn't always correct - when you see it squished, it is correct. Just working through this now, fixing this will result in a slight performance win, as it'll more completely solve bug 732013.
I'd forgotten to change the texture coordinates when writing the original patch, my mistake.
Attachment #605004 - Flags: review?(pwalton)
Comment on attachment 605004 [details] [diff] [review]
Part 1 - Fix squashed drawing of layers

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

I still don't fully understand this code, but it looks fine to me.
Attachment #605004 - Flags: review?(pwalton) → review+
This patch fixes overdraw of the checkerboard layer, but Gecko's displayport lags behind the one stored in GeckoLayerClient, so this causes checkerboard to sometimes not appear where it should.

We need to add the displayport to the draw metadata, which will involve making it readable via DOMWindowUtils.
Part 1 landed on maple: http://hg.mozilla.org/projects/maple/rev/2dd6ac8eb1a4

Part 2 requires more work, leaving the bug open for that (which I'll finish off soon).
(In reply to Chris Lord [:cwiiis] from comment #6)
> Part 1 landed on maple: http://hg.mozilla.org/projects/maple/rev/2dd6ac8eb1a4
> 
> Part 2 requires more work, leaving the bug open for that (which I'll finish
> off soon).

Just to follow up on this, after IRC we've decided on another method - rather than reading the displayport back via DOMWindowUtils and bundling it with the draw metadata, we'll read it directly from the layer in CompositorParent and send it across when drawing the under/overlay.
blocking-fennec1.0: --- → ?
Straight up beta blocker it would appear.
blocking-fennec1.0: ? → beta+
Sorry to take so long on this, got caught up on another bug. This does as we've discussed on IRC and seems to work for me.
Attachment #605027 - Attachment is obsolete: true
Attachment #606546 - Flags: review?(bugmail.mozilla)
Comment on attachment 606546 [details] [diff] [review]
Part 2 - Fix overdraw of checkerboard layer

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

::: gfx/layers/ipc/CompositorParent.cpp
@@ +313,5 @@
>  }
>  
>  #ifdef MOZ_WIDGET_ANDROID
>  void
>  CompositorParent::RequestViewTransform()

Maybe rename CompositorParent::RequestViewTransform as well?

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +384,5 @@
> +        // Calculate the display-port from the given margins
> +        Rect viewport = RectUtils.round(mGeckoViewport.getViewport());
> +        x += viewport.left;
> +        y += viewport.top;
> +        mGeckoDisplayPort = new Rect(x, y, x + width, y + height);

As discussed on IRC, I'd much rather pass in the absolute display port here, rather than a relative one that needs to be resolved against mGeckoVieewport. Partly because that's set from a different thread (possibly racing with this) and partly because I want to get rid of endDrawing. The x and y of mGeckoViewport should be the same as metrics->mViewportScrollOffset in the compositor.

Finally, I would like to see as few memory allocations as possible during this call; rather than creating a new Rect just setting the fields directly would be better. If you're passing in the absolute coordinates then they could be set directly on the root layer rather than storing them in this class as mGeckoDisplayPort.

::: mobile/android/base/gfx/Layer.java
@@ +117,5 @@
>       * may be overridden.
>       */
>      public Region getValidRegion(RenderContext context) {
>          return new Region(RectUtils.round(getBounds(context)));
>      }

I think getValidRegion should return a region of getDisplayPortBounds rather than of getBounds
Comment on attachment 606546 [details] [diff] [review]
Part 2 - Fix overdraw of checkerboard layer


>diff --git a/mobile/android/base/gfx/GeckoLayerClient.java b/mobile/android/base/gfx/GeckoLayerClient.java
>--- a/mobile/android/base/gfx/GeckoLayerClient.java
>+++ b/mobile/android/base/gfx/GeckoLayerClient.java
>@@ -71,22 +71,25 @@ public class GeckoLayerClient implements
>     private RectF mDisplayPort;
>     private RectF mReturnDisplayPort;
> 
>     private VirtualLayer mRootLayer;
> 
>     /* The viewport that Gecko is currently displaying. */
>     private ViewportMetrics mGeckoViewport;
> 
>+    /* The displayport that Gecko is current displaying. */
>+    private Rect mGeckoDisplayPort;
>+

Do we need to store this? Isn't it only relevant for the current composite?
This addresses the review comments and discussion on IRC.
Attachment #606546 - Attachment is obsolete: true
Attachment #606592 - Flags: review?(bugmail.mozilla)
Attachment #606546 - Flags: review?(bugmail.mozilla)
Comment on attachment 606592 [details] [diff] [review]
Part 2 - Fix overdraw of checkerboard layer (revised)

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

The code looks good. I think adding more comments would be helpful just so we don't get confused again in the future. I've pointed out a couple of places to add comments below, but feel free to add some more wherever you feel is appropriate.

::: mobile/android/base/gfx/GeckoLayerClient.java
@@ +76,5 @@
>      /* The viewport that Gecko is currently displaying. */
>      private ViewportMetrics mGeckoViewport;
>  
> +    /* The viewport metrics being used to draw the current frame. */
> +    private ImmutableViewportMetrics mFrameMetrics;

Add a comment about how this is only touched on the compositor thread and therefore needs no synchronization.

@@ +372,3 @@
>          // getViewportMetrics is thread safe so we don't need to synchronize
>          // on myLayerController.
> +        mFrameMetrics = mLayerController.getViewportMetrics();

Add a comment here about why we're saving the metrics here and passing them in during createFrame, rather than just getting them from the layer controller in createFrame. (Mostly just summarizing our findings on IRC so that we don't forget). Also the existing comment has a typo: "myLayerController" -> "mLayerController".
Attachment #606592 - Flags: review?(bugmail.mozilla) → review+
Pushed to inbound with revised/extra comments: http://hg.mozilla.org/integration/mozilla-inbound/rev/d5b66e64cf44
The logs have a LOT of this:

03-16 09:52:27.759 E/GeckoConsole( 1487): [JavaScript Error: "this.browser.contentWindow is undefined" {file: "chrome://browser/content/browser.js" line: 1623}]
03-16 09:52:27.759 E/GeckoLayerClient( 1487): Aborting draw, bad viewport description: 

but I don't see why that would happen with this change. browser.js and the page-loading codepath are both untouched so contentWindow should be no different than it was before.
(In reply to Ryan VanderMeulen from comment #15)
> Backed out due to Android-native crashes.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9f00738030d7
> 
> Some examples:
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10132967&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10132784&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10133121&tree=Mozilla-
> Inbound
> https://tbpl.mozilla.org/php/getParsedLog.php?id=10131995&tree=Mozilla-
> Inbound

ugh, I think this is a race-condition because we use mGeckoViewport, but that gets initialised in beginDrawing (which is called in a different thread) - it's the only reason I can think of that the object returned would be null from syncViewportInfo.

I'm simplifying the patch a bit and removing the dependency on mGeckoViewport (hopefully). We could work around this with a null check, but it ought to be faster and safer this way.
Ah, a bad viewport description would cause this too, thanks kats.
I chickened out and made it less of a change, rather than more.
Attachment #606592 - Attachment is obsolete: true
Attachment #606851 - Flags: review?(bugmail.mozilla)
Comment on attachment 606851 [details] [diff] [review]
Part 2 - Fix overdraw of checkerboard layer (revised again)

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

r=me. Kinda ugly but I'll clean it up when I take out beginDrawing/endDrawing.
Attachment #606851 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (:kats) from comment #20)
> Comment on attachment 606851 [details] [diff] [review]
> Part 2 - Fix overdraw of checkerboard layer (revised again)
> 
> Review of attachment 606851 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Kinda ugly but I'll clean it up when I take out
> beginDrawing/endDrawing.

Agreed, and thanks :)

Try was green: https://tbpl.mozilla.org/?tree=Try&rev=98eed8486fda
Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/1239bd0779a6
https://hg.mozilla.org/mozilla-central/rev/d5b66e64cf44
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/1239bd0779a6 :)
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Checker board is now white so this bug is no longer applicable. Will however close the issue as verified.
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.