Last Comment Bug 737510 - Try to keep the buffer size easily tileable (tile snapping)
: Try to keep the buffer size easily tileable (tile snapping)
Status: RESOLVED FIXED
[viewport]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: Kartikaya Gupta (email:kats@mozilla.com)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on: 739679
Blocks: checkerboarding 729391 745177
  Show dependency treegraph
 
Reported: 2012-03-20 10:37 PDT by Jeff Muizelaar [:jrmuizel]
Modified: 2012-04-26 08:02 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
beta+


Attachments
WIP - Expand displayport to fill tiles (6.83 KB, patch)
2012-04-11 16:24 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
Fixed bitrot (6.78 KB, patch)
2012-04-13 13:31 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
proper fix bitrot (7.19 KB, patch)
2012-04-13 13:58 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
WIP (1/3) add useful toString functions (1.83 KB, patch)
2012-04-18 07:58 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
WIP (2/3) expand displayport to fill tiles (7.23 KB, patch)
2012-04-18 07:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
WIP (3/3) hack around gecko conversions and rounding errors (6.82 KB, patch)
2012-04-18 07:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
JS script that is a test harness for the _hackForGeckoConversions function (4.55 KB, application/x-javascript)
2012-04-18 08:00 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
WIP (3/3) hack around gecko conversions and rounding errors (8.01 KB, patch)
2012-04-18 14:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details | Diff | Splinter Review
JS script that is a test harness (6.79 KB, application/x-javascript)
2012-04-18 15:00 PDT, Kartikaya Gupta (email:kats@mozilla.com)
no flags Details
(1/3) add useful toString function (1.85 KB, patch)
2012-04-18 21:55 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(2/3) expand displayport to fill tiles (7.23 KB, patch)
2012-04-18 21:58 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(3/3) hack around gecko conversions and rounding errors (12.69 KB, patch)
2012-04-18 21:59 PDT, Kartikaya Gupta (email:kats@mozilla.com)
chrislord.net: review+
Details | Diff | Splinter Review
(1/3) Add useful toString function (1.96 KB, patch)
2012-04-23 18:56 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(2/3) expand displayport to fill tiles (7.47 KB, patch)
2012-04-23 18:57 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
(3/3) hack around gecko conversions and rounding errors (13.07 KB, patch)
2012-04-23 19:01 PDT, Kartikaya Gupta (email:kats@mozilla.com)
bugmail: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-03-20 10:37:23 PDT
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] 2012-03-20 10:42:09 PDT
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:kats@mozilla.com) 2012-03-20 10:47:49 PDT
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) 2012-04-11 16:09:35 PDT
Progress on this?  Been a while since we've seen an update here.
Comment 4 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-11 16:24:35 PDT
Created attachment 614216 [details] [diff] [review]
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] 2012-04-12 06:50:08 PDT
(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) 2012-04-13 13:31:56 PDT
Created attachment 614899 [details] [diff] [review]
Fixed bitrot
Comment 7 Benoit Girard (:BenWa) 2012-04-13 13:58:42 PDT
Created attachment 614905 [details] [diff] [review]
proper fix bitrot
Comment 8 Benoit Girard (:BenWa) 2012-04-13 14:40:24 PDT
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:kats@mozilla.com) 2012-04-17 11:48:08 PDT
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:kats@mozilla.com) 2012-04-17 12:16:59 PDT
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:kats@mozilla.com) 2012-04-17 12:21:58 PDT
> 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:kats@mozilla.com) 2012-04-18 07:58:09 PDT
Created attachment 616139 [details] [diff] [review]
WIP (1/3) add useful toString functions
Comment 13 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 07:59:06 PDT
Created attachment 616140 [details] [diff] [review]
WIP (2/3) expand displayport to fill tiles
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 07:59:48 PDT
Created attachment 616141 [details] [diff] [review]
WIP (3/3) hack around gecko conversions and rounding errors
Comment 15 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 08:00:32 PDT
Created attachment 616142 [details]
JS script that is a test harness for the _hackForGeckoConversions function
Comment 16 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 14:59:11 PDT
Created attachment 616302 [details] [diff] [review]
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:kats@mozilla.com) 2012-04-18 15:00:02 PDT
Created attachment 616304 [details]
JS script that is a test harness

Now with random displayport generation for more thorough testing.
Comment 18 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 21:55:03 PDT
Created attachment 616453 [details] [diff] [review]
(1/3) add useful toString function
Comment 19 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-18 21:58:12 PDT
Created attachment 616454 [details] [diff] [review]
(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:kats@mozilla.com) 2012-04-18 21:59:20 PDT
Created attachment 616456 [details] [diff] [review]
(3/3) hack around gecko conversions and rounding errors

Added some comments to explain what's going on
Comment 21 Chris Lord [:cwiiis] 2012-04-19 03:17:08 PDT
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] 2012-04-19 03:21:01 PDT
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] 2012-04-19 04:28:19 PDT
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:kats@mozilla.com) 2012-04-23 18:56:47 PDT
Created attachment 617738 [details] [diff] [review]
(1/3) Add useful toString function

Update with review comments, carrying r+
Comment 25 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 18:57:24 PDT
Created attachment 617739 [details] [diff] [review]
(2/3) expand displayport to fill tiles

Update with review comments, carry r+
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-23 19:01:32 PDT
Created attachment 617741 [details] [diff] [review]
(3/3) hack around gecko conversions and rounding errors

Ditto
Comment 28 Phil Ringnalda (:philor) 2012-04-24 00:12:27 PDT
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:kats@mozilla.com) 2012-04-24 07:26:07 PDT
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:kats@mozilla.com) 2012-04-24 11:00:36 PDT
Didn't make the aurora cut. :(
Comment 31 Kartikaya Gupta (email:kats@mozilla.com) 2012-04-24 11:02:50 PDT
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.

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