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

RESOLVED FIXED in Firefox 14

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: kats)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, blocking-fennec1.0 beta+)

Details

(Whiteboard: [viewport])

Attachments

(4 attachments, 11 obsolete attachments)

6.79 KB, application/x-javascript
Details
1.96 KB, patch
kats
: review+
Details | Diff | Splinter Review
7.47 KB, patch
kats
: review+
Details | Diff | Splinter Review
13.07 KB, patch
kats
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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)
(Reporter)

Comment 1

5 years ago
It sounds like we can do this in the front end and hope that our numbers survive the conversion to css pixels and back.
Assignee: nobody → bugmail.mozilla
(Reporter)

Updated

5 years ago
Blocks: 729391
(Reporter)

Updated

5 years ago
No longer blocks: 729391
(Reporter)

Updated

5 years ago
Blocks: 729391
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.
OS: Linux → Android
Hardware: x86_64 → ARM
Whiteboard: [viewport]
(Reporter)

Updated

5 years ago
Blocks: 717774
Depends on: 739679

Updated

5 years ago
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → beta+
Progress on this?  Been a while since we've seen an update here.
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

5 years ago
(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.
Created attachment 614899 [details] [diff] [review]
Fixed bitrot
Created attachment 614905 [details] [diff] [review]
proper fix bitrot
Attachment #614899 - Attachment is obsolete: true
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
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.
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.
> 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.
Created attachment 616139 [details] [diff] [review]
WIP (1/3) add useful toString functions
Attachment #616139 - Attachment description: WIP (1/4) add useful toString functions → WIP (1/3) add useful toString functions
Created attachment 616140 [details] [diff] [review]
WIP (2/3) expand displayport to fill tiles
Attachment #614216 - Attachment is obsolete: true
Attachment #614905 - Attachment is obsolete: true
Created attachment 616141 [details] [diff] [review]
WIP (3/3) hack around gecko conversions and rounding errors
Created attachment 616142 [details]
JS script that is a test harness for the _hackForGeckoConversions function
(Reporter)

Updated

5 years ago
Summary: Try to keep the buffer size easily tileable → Try to keep the buffer size easily tileable (tile snapping)
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.
Attachment #616141 - Attachment is obsolete: true
Created attachment 616304 [details]
JS script that is a test harness

Now with random displayport generation for more thorough testing.
Attachment #616142 - Attachment is obsolete: true
Created attachment 616453 [details] [diff] [review]
(1/3) add useful toString function
Attachment #616139 - Attachment is obsolete: true
Attachment #616453 - Flags: review?(chrislord.net)
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.
Attachment #616140 - Attachment is obsolete: true
Attachment #616454 - Flags: review?(chrislord.net)
Created attachment 616456 [details] [diff] [review]
(3/3) hack around gecko conversions and rounding errors

Added some comments to explain what's going on
Attachment #616302 - Attachment is obsolete: true
Attachment #616456 - Flags: review?(chrislord.net)
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?
Attachment #616453 - Flags: review?(chrislord.net) → review+
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.
Attachment #616454 - Flags: review?(chrislord.net) → review+
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'.
Attachment #616456 - Flags: review?(chrislord.net) → review+

Updated

5 years ago
Blocks: 745177
Created attachment 617738 [details] [diff] [review]
(1/3) Add useful toString function

Update with review comments, carrying r+
Attachment #616453 - Attachment is obsolete: true
Attachment #617738 - Flags: review+
Created attachment 617739 [details] [diff] [review]
(2/3) expand displayport to fill tiles

Update with review comments, carry r+
Attachment #616454 - Attachment is obsolete: true
Attachment #617739 - Flags: review+
Created attachment 617741 [details] [diff] [review]
(3/3) hack around gecko conversions and rounding errors

Ditto
Attachment #616456 - Attachment is obsolete: true
Attachment #617741 - Flags: review+
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
status-firefox14: --- → fixed
Target Milestone: --- → mozilla14
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/610a9df3875b - all your XUL unittests and your native crashtests timed out.
Target Milestone: mozilla14 → ---
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
Target Milestone: --- → mozilla14
Didn't make the aurora cut. :(
status-firefox14: fixed → affected
status-firefox15: --- → fixed
Target Milestone: mozilla14 → mozilla15
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.
Attachment #617738 - Flags: approval-mozilla-aurora?
Attachment #617739 - Flags: approval-mozilla-aurora?
Attachment #617741 - Flags: approval-mozilla-aurora?

Updated

5 years ago
Whiteboard: [viewport] → [viewport][inbound]

Updated

5 years ago
Attachment #617738 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #617739 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

5 years ago
Attachment #617741 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
https://hg.mozilla.org/mozilla-central/rev/0fab1a28d7c3
https://hg.mozilla.org/mozilla-central/rev/1667efcf2ed6
https://hg.mozilla.org/mozilla-central/rev/18c08ea97f01
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [viewport][inbound] → [viewport]

Updated

5 years ago
status-firefox14: affected → fixed
You need to log in before you can comment on or make changes to this bug.