737510 - Try to keep the buffer size easily tileable (tile snapping)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

Right now the Thebes buffer size in device pixels is sort of arbitrary. It would be nice if it could be rounded to the nearest 256 or so. This will make things better when tiling (PowerVR and Adreno)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

It sounds like we can do this in the front end and hope that our numbers survive the conversion to css pixels and back.

Comment 2

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

This should be trivial to do if we can modify the existing DOMWindowUtils.setDisplayPortForElement (or add another method) to take integer device pixel values. But we need to make sure Gecko doesn't draw outside the page bounds when we do this.

Comment 3

[Damon Sicore (:damons)]

Progress on this?  Been a while since we've seen an update here.

Comment 4

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

Attachment: WIP - Expand displayport to fill tiles

This compiles but I haven't tested it. Putting it here in case somebody else wants to try it before I get around to it.

Comment 5

[JP Rosevear [:jpr]]

(In reply to Damon Sicore (:damons) from comment #3)
> Progress on this?  Been a while since we've seen an update here.

Its dependent on the tiled thebes layer pieces.

Comment 6

[Benoit Girard (:BenWa)]

Attachment: Fixed bitrot

Comment 7

[Benoit Girard (:BenWa)]

Attachment: proper fix bitrot

Comment 8

[Benoit Girard (:BenWa)]

The drawing regions gets rounded +-1. Here's an example of panning up (decreasing Y):

I/Gecko   ( 1112): Not aligned to tile boundary 0, 767 <--- We miss the boundary by -1 pixels, thus we
I/Gecko   ( 1112): Upload 0, 767, 720, 256
I/Gecko   ( 1112): Upload tile 0, 512                  <--- touch tile (0, 512)
I/Gecko   ( 1112): Upload tile 0, 768
I/Gecko   ( 1112): Upload tile 256, 512                <--- and this one too
I/Gecko   ( 1112): Upload tile 256, 768
I/Gecko   ( 1112): Tile Time to upload 2
I/Gecko   ( 1112): Upload tile 512, 512                <--- and this one :(
I/Gecko   ( 1112): Upload tile 512, 768
I/Gecko   ( 1112): Tile Time to upload 2
D/dalvikvm( 1112): GC_CONCURRENT freed 790K, 8% free 15279K/16455K, paused 2ms+3ms
I/Gecko   ( 1112): Compositor: Composite took 30 ms.
I/Gecko   ( 1112): Upload 0, 512, 720, 2049            <--- now we're aligned on the boundary yay!
I/Gecko   ( 1112): Upload tile 0, 512                  <--- all x, 512 tiles are fully filled
I/Gecko   ( 1112): Upload tile 0, 2560
I/Gecko   ( 1112): Upload tile 256, 512
I/Gecko   ( 1112): Upload tile 256, 2560
I/Gecko   ( 1112): Upload tile 512, 512
I/Gecko   ( 1112): Upload tile 512, 2560

Comment 9

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

I've spent a while now digging into this, and I'm still not 100% sure why this is happening. Getting closer though. It's definitely rounding error. Right now we start with the displayport coordinates in page-relative device pixels in floats (in Java), convert it to viewport-relative CSS pixels in floats (to call setDisplayPortForElement in browser.js), which then gets converted to viewport-relative CSS pixels in app units, and then gets converted a bunch more times before finally being handed to the compositor in page-relative device pixels in ints. These conversions include floor, round-to-nearest, negation, and possibly even ceiling operations, which makes it really hard to predict what we should put in to get out integer multiples of 256.

Comment 10

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

Ok, so as best as I can tell, these are the operations that happen:

1. Java displayport calculator comes up with a 256-aligned displayport in device pixels relative to page top-left, e.g. {"x":1280,"y":1536,"w":1536,"h":1792}. At this point the viewport (aka gecko scroll position) is (511, 557) in CSS pixels relative to page top-left, and the current zoom is 3.2717385.

2. It converts it to CSS pixels relative to viewport top-left: {"x":-119.77068873322241,"y":-87.52482647986693,"w":469.47517352013307,"h":547.7210357734886} and passes that in to setDisplayPortForElement

3. setDisplayPortForElement immediately converts it into app units relative to viewport top-left and stores it as {"x":-7186,"y":-5251,"w":28169,"h":32863}

4. gecko at some point does a conversion back to integer device pixel coordinates, relative to viewport top-left but does so using ScaleToOutsidePixels, thus expanding the displayport into {"x":-392,"y":-287,"w":1537,"h":1793}

5. Meanwhile, the viewport which started off at (511, 557) in CSS pixels relative to page top-left gets negated to (-511, -557). This is then turned into device pixel coordinates as (-1672, -1822). The negation here is important because -floor(-x) != floor(x)

6. Finally the displayport from step 4 and the coordinates from step 5 are combined to give a displayport relative to the page top-left in device pixels of {"x":1280,"y":1535,"w":1537,"h":1793}, and this is handed to the compositor.

What started out as perfectly good 256-aligned displayport is now mangled almost beyond recognition.

Comment 11

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

> 2. It converts it ...

That should say "2. browser.js converts it ..."


I think we might be able to work around all of these conversions by doing two things:

(a) calculating the negated-floored-negated viewport values (step 5) in browser.js and making the displayport relative to that instead of the normal viewport in step 1, and
(b) maybe shrinking the displayport slightly in browser.js so that when it goes through the ScaleToOutsidePixels it doesn't over-expand into something that's larger than a multiple of 256, because that would be very difficult to recover from.

I'll try doing this next.

Comment 12

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

Attachment: WIP (1/3) add useful toString functions

Comment 13

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

Attachment: WIP (2/3) expand displayport to fill tiles

Comment 14

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

Attachment: WIP (3/3) hack around gecko conversions and rounding errors

Comment 15

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

Attachment: JS script that is a test harness for the _hackForGeckoConversions function

Comment 16

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

Attachment: WIP (3/3) hack around gecko conversions and rounding errors

Found some rounding errors leaking through in the last one, so made the hacks even more hacky. I hate myself.

Comment 17

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

Attachment: JS script that is a test harness

Now with random displayport generation for more thorough testing.

Comment 18

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

Attachment: (1/3) add useful toString function

Comment 19

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

Attachment: (2/3) expand displayport to fill tiles

Only applying this to FixedMargin and VelocityBias for now. Also this depends on the tiling patches in bug 739679 to be useful.

Comment 20

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

Attachment: (3/3) hack around gecko conversions and rounding errors

Added some comments to explain what's going on

Comment 21

[Chris Lord [:cwiiis]]

Comment on attachment 616453 [details] [diff] [review]
(1/3) add useful toString function

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

Looks good. Might be nice to change the DisplayPortMetrics toString method to output in the same, more readable format?

Comment 22

[Chris Lord [:cwiiis]]

Comment on attachment 616454 [details] [diff] [review]
(2/3) expand displayport to fill tiles

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

Looks good.

::: mobile/android/base/gfx/DisplayPortCalculator.java
@@ +16,5 @@
>  
>  final class DisplayPortCalculator {
>      private static final String LOGTAG = "GeckoDisplayPortCalculator";
>      private static final PointF ZERO_VELOCITY = new PointF(0, 0);
> +    private static final int TILE_SIZE = 256;

Probably good to add a comment referencing where this is defined in Gecko, to make sure they stay in sync. (gfx/layers/TiledLayerBuffer.h I think?)

@@ +146,5 @@
> +     * does not have any partial tiles, except when it is clipped by the page bounds. This assumes
> +     * the tiles are TILE_SIZE by TILE_SIZE and start at the origin, such that there will always be
> +     * a tile at (0,0)-(TILE_SIZE,TILE_SIZE)).
> +     */
> +    private static DisplayPortMetrics expandMarginsToFillTiles(RectF margins, float zoom, ImmutableViewportMetrics metrics) {

Might be better to call this function something along the lines of getTileAlignedDisplayPortMetrics? This is fine too, just it doesn't hint about its return type.

Comment 23

[Chris Lord [:cwiiis]]

Comment on attachment 616456 [details] [diff] [review]
(3/3) hack around gecko conversions and rounding errors

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

Hmm. Well, it's hard to get excited about this, but I suppose it's what needs to be done until we work out a proper fix... Could this code have been done on the native side in an android #ifdef perhaps? I guess it could take advantage of Gecko functions there, and so perhaps be a bit smaller and faster? Either way, this is performance critical stuff and it's well-isolated, so we should try and land this asap (after tiling).

::: mobile/android/chrome/content/browser.js
@@ +1707,5 @@
>        dump("Warning: setDisplayPort resolution did not match zoom for background tab!");
>      }
>  
>      // finally, we set the display port, taking care to convert everything into the CSS-pixel
> +    // coordinate space, because that is what the function accepts. also we have to fudge the

Capital 'a'.

@@ +1709,5 @@
>  
>      // finally, we set the display port, taking care to convert everything into the CSS-pixel
> +    // coordinate space, because that is what the function accepts. also we have to fudge the
> +    // displayport somewhat to make sure it gets through all the conversions gecko will do on it
> +    // without deforming too much. see https://bugzilla.mozilla.org/show_bug.cgi?id=737510#c10

Capital 's'.

Comment 24

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

Attachment: (1/3) Add useful toString function

Update with review comments, carrying r+

Comment 25

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

Attachment: (2/3) expand displayport to fill tiles

Update with review comments, carry r+

Comment 26

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

Attachment: (3/3) hack around gecko conversions and rounding errors

Ditto

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1ca7c95917fb
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd8619fdf56
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1bf084429e8

Comment 28

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/610a9df3875b - all your XUL unittests and your native crashtests timed out.

Comment 29

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

Turns out the damage was caused by bug 739769 (which this bug depends on, so it was good you backed it out regardless).

Re-landing now that bug 739769 has re-landed and is looking green.

https://hg.mozilla.org/integration/mozilla-inbound/rev/0fab1a28d7c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/1667efcf2ed6
https://hg.mozilla.org/integration/mozilla-inbound/rev/18c08ea97f01

Comment 30

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

Didn't make the aurora cut. :(

Comment 31

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

Comment on attachment 617738 [details] [diff] [review]
(1/3) Add useful toString function

[Approval Request Comment]
This goes with bug 739679 which got pulled into aurora.

Comment 32

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/2b354dbf7b30
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d9150554eaa
https://hg.mozilla.org/releases/mozilla-aurora/rev/fe347682de88

Comment 33

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/0fab1a28d7c3
https://hg.mozilla.org/mozilla-central/rev/1667efcf2ed6
https://hg.mozilla.org/mozilla-central/rev/18c08ea97f01