732756 - [maple] Checkerboard sometimes shows up with squashed aspect ratio

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 1

[Chris Lord [:cwiiis]]

I occasionally see this, but not always, which is odd... I'll see if I can track this down on Monday.

Comment 2

[Chris Lord [:cwiiis]]

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.

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Part 1 - Fix squashed drawing of layers

I'd forgotten to change the texture coordinates when writing the original patch, my mistake.

Comment 4

[Patrick Walton (:pcwalton)]

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.

Comment 5

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Fix overdraw of checkerboard layer (WIP)

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.

Comment 6

[Chris Lord [:cwiiis]]

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

Comment 7

[Chris Lord [:cwiiis]]

(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.

Comment 8

[JP Rosevear [:jpr]]

Straight up beta blocker it would appear.

Comment 9

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Fix overdraw of checkerboard layer

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.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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 11

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 12

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Fix overdraw of checkerboard layer (revised)

This addresses the review comments and discussion on IRC.

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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".

Comment 14

[Chris Lord [:cwiiis]]

Pushed to inbound with revised/extra comments: http://hg.mozilla.org/integration/mozilla-inbound/rev/d5b66e64cf44

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 17

[Chris Lord [:cwiiis]]

(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.

Comment 18

[Chris Lord [:cwiiis]]

Ah, a bad viewport description would cause this too, thanks kats.

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Part 2 - Fix overdraw of checkerboard layer (revised again)

I chickened out and made it less of a change, rather than more.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 21

[Chris Lord [:cwiiis]]

(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

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/d5b66e64cf44

Comment 23

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/9f00738030d7

Comment 24

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1239bd0779a6 :)

Comment 25

[Adrian Tamas (:AdrianT)]

Checker board is now white so this bug is no longer applicable. Will however close the issue as verified.