Support arbitrary tiling origins and negative tile offsets during tile decomposition #3409

nical · 2018-12-13T13:06:35Z

The tile decomposition code is rewritten to support:

arbitrary tiling origins in layout space,
negative tile offsets,
boundary tiles on each side.

This code is a bit trickier than it would seem at first due to how we have to tile in two spaces (image and layout) at the same time in order to get boundary tiles and culling right. Since the problem is exactly the same along each axis I reduced it to 1 dimension in to reduce the complexity and likelihood of copy-pasta accidents.

This change is

nical · 2018-12-13T13:12:56Z

At this stage this passes reftests locally but I still need a try push and some more changes in various other places to really support negative tile offsets in WebRender as well as some plumbing to let blob images pass the right information along and benefit from this.

The best way to review this might be to just look at the new code since it is similar but subtly different from the way it worked previously and almost every line in this change is important. I added some ascii diagrams in an attempt to make some things easier to understand but it's hard to tell whether it'll help anyone but me. Let me know if they make things worse and I'll remove my scribbles.

nical · 2018-12-14T15:28:43Z

Try: https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=216820099&revision=431f3e0bd5ec1189222f979cb19d9ab9b0c2ff82

nical · 2019-01-02T11:13:41Z

I think that I figured all of the remaining reftest failures out, let's see if this try push comes back green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b5be9a0bc5f9d4102340d90eb9766f1b30a39026

nical · 2019-01-02T13:59:30Z

The try push is green.

gw3583 · 2019-01-03T20:27:22Z

@nical Did you have someone specifically in mind to review this? I can take a look over it if needed, although I'm quite out of the loop on the blob re-coord changes, which I think this is for?

nical · 2019-01-03T22:40:24Z

@glennw I think that anybody who understands (or want to understand) how tile decomposition works is a good fit for this review. This PR doesn't have much of the blob recoordination in it but it takes a somewhat hairy bag of arithmetic and adds a few extra parameters so the tile decomposition aspect is more important than the overall recoordination stuff.

If nobody volunteers for the review I'd nominate either @kvark or you (as you two tend to do a lot of the webrender reviews) or @jrmuizel.

jamienicol

Mainly just nits, looks good to me. Probably worthwhile getting someone else to look too though.

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs


        let mut segment_rect = LayoutRect {
            origin: LayoutPoint::new(
-                self.local_origin.x + tile_offset.x as f32 * self.tile_size.width,
-                self.local_origin.y + tile_offset.y as f32 * self.tile_size.height,
+                self.local_origin.x + tile_offset.x as f32 * self.regular_tile_size.width,


Does this not have to take in to account the first_tile_layout_size?

Yes indeed, nice catch.

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+    // the latters can have image bounds with negative values (the blob image's
+    // visible area provided by gecko).
+    //
+    // Likewise, the alyout space tiling origin (layout position of tile offset


typo: alyout

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

-        layer_tile_size.height * leftover_device_size.height as f32 / device_tile_size_f32,
-    );
+    // The decomposition logic is exactly the same on each axis so we reduce
+    // this to a 1-dimmensional problem in an attempt to make the code simpler.


typo: 1 m in dimensional

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

        local_origin: prim_rect.origin,
    }
 }

+/// Decompose tiles along an arbitray axis.


I don't think the ascii art is necessary to explain what treating the problem in one dimension means. The "why" is more important (like you explained above)

Ok, removed it.

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+
+    // The visible tiles (because of culling).
+    //
+    // Here we don't need to do the off by one dance we did above because f32::floor


I think you're referring to the division in first_and_last_tile_1d? This isn't "above" (maybe it was then you refactored that in to a separate function?).

In any case, it'd be more useful to say what the behaviour is (round to -infinity) than just it's what we want

Indeed that comment is out of date.

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+    let first_visible_tile = f32::floor((layout_visible_range.start - layout_tiling_origin) / layout_tile_size) as i32;
+    let last_visible_tile = f32::floor((layout_visible_range.end - layout_tiling_origin) / layout_tile_size) as i32;
+
+    // Combine the above two to get the tiles in the image that are visible this frame.


nit: Maybe "intersect" instead of "combine"? And delete the blank line

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+        layout_tile_size
+    };
+
+    // Idem.


My latin isn't very good, I had to google that :)

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+    image_range: &Range<i32>,
+    regular_tile_size: i32,
+) -> i32 {
+    // We have to account for how the modulo operation behaves for negative values.


I think technically the issue is % is a remainder operator rather than modulo, and we want modulo, so we need the extra code. might be better to say that instead.

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+
+    // Most tiles are going to have base_size as width and height,
+    // except for tiles around the edges that are shrunk to fit the image data
+    // (See decompose_tiled_image in frame.rs).


neither decompose_tiled_image nor frame.rs exist. should this just be tiles() (in the same file now this function has moved)

jamienicol · 2019-01-04T14:02:37Z

webrender/src/image.rs

+
+    #[test]
+    #[ignore]
+    fn failing_doubly_partial_tiles() {


can we not just always min(m, image_size) for all cases of the match statements in first/last_tile_size_1d?

Oh yes indeed!

nical · 2019-01-04T14:56:08Z

I addressed the review comments. I won't be near a computer this weekend so let's wait until Monday before landing this, just to be on the safe side.

kvark

Looks pretty neat! Left my notes below

Reviewed 1 of 1 files at r2.
Reviewable status: 1 of 2 files reviewed, 12 unresolved discussions (waiting on @jamienicol and @nical)

webrender/src/image.rs, line 187 at r2 (raw file):

        if self.current_tile.x == self.x.tile_range.end {
            self.current_tile.y += 1;
            if self.current_tile.y >= self.y.tile_range.end {

would be good to also return None if next() is called more times

webrender/src/image.rs, line 193 at r2 (raw file):

            self.row_flags = EdgeAaSegmentMask::empty();
            if self.current_tile.y == self.y.tile_range.end - 1 {
                self.row_flags |= EdgeAaSegmentMask::BOTTOM;

might be cleaner to just compute the row flags each iteration instead of trying to preserve them

webrender/src/image.rs, line 282 at r2 (raw file):

            return TileIterator {
                current_tile: TileOffset::zero(),
                x: TileIteratorExtent {

looks like a constructor method would be helpful

webrender/src/image.rs, line 360 at r2 (raw file):

/// This does most of the heavy lifing needed for `tiles` but in a single dimension for
/// the sake of simplicity since the problem is independent on the x and y axes.
fn tiles_1d(

good stuff 👍 🎖️

webrender/src/image.rs, line 383 at r2 (raw file):

    // The visible tiles (because of culling).
    let first_visible_tile = f32::floor((layout_visible_range.start - layout_tiling_origin) / layout_tile_size) as i32;
    let last_visible_tile = f32::floor((layout_visible_range.end - layout_tiling_origin) / layout_tile_size) as i32;

there is something disturbing to me in the way the code handles inclusive/exclusive ranges. So layout_visible_range.end is exclusive given the basic semantics of Range. But the computed last_visible_tile is inclusive? Wouldn't we then end up processing more tiles that are visible?

For example:

let layout_tiling_origin = 0;
let layout_tile_size = 10.0;
let layout_visible_range.end = 20;

What the algorithm needs to produce is that 2 tiles are visible, but it will end up considering 3 tiles to be visible.

webrender/src/image.rs, line 388 at r2 (raw file):

    let first_tile = i32::max(first_image_tile, first_visible_tile);
    let last_tile = i32::min(last_image_tile, last_visible_tile);

should we assert!(first_tile <= last_tile) here for sanity?

webrender/src/image.rs, line 406 at r2 (raw file):

    TileIteratorExtent {
        tile_range: first_tile..(last_tile + 1),

nit: first_tile..=last_tile :)

webrender/src/image.rs, line 441 at r2 (raw file):

    let mut last_image_tile = image_range.end / regular_tile_size;
    if image_range.end % regular_tile_size == 0 || image_range.end < 0 {
        last_image_tile -= 1;

This is not entirely correct. If both image_range.end % regular_tile_size == 0 and image_range.end < 0 are true, we shouldn't be decreasing the last tile index

webrender/src/image.rs, line 504 at r2 (raw file):

// Compute the width and height in pixels of a tile depending on its position in the image.
pub fn compute_tile_size(

it looks like most of this function could also be well separated by axis

webrender/src/image.rs, line 519 at r2 (raw file):

    // (See decompose_tiled_image in frame.rs).
    let actual_width = match tile.x as i32 {
        x if x == x_first => first_tile_size_1d(&img_range_x, regular_tile_size),

this is not an elegant use of match, I think. Did you consider just an old good if/else?

nical

Reviewable status: 1 of 2 files reviewed, 12 unresolved discussions (waiting on @jamienicol, @kvark, and @nical)

webrender/src/image.rs, line 187 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would be good to also return None if next() is called more times

Done.

webrender/src/image.rs, line 193 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

might be cleaner to just compute the row flags each iteration instead of trying to preserve them

Agreed.

webrender/src/image.rs, line 282 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

looks like a constructor method would be helpful

If it's used in only one place in the same file I don't feel much need for it.

webrender/src/image.rs, line 383 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

there is something disturbing to me in the way the code handles inclusive/exclusive ranges. So layout_visible_range.end is exclusive given the basic semantics of Range. But the computed last_visible_tile is inclusive? Wouldn't we then end up processing more tiles that are visible?

For example:
let layout_tiling_origin = 0;
let layout_tile_size = 10.0;
let layout_visible_range.end = 20;
What the algorithm needs to produce is that 2 tiles are visible, but it will end up considering 3 tiles to be visible.

You are right. In fixing that i rewrote it to use [a..b[ kind of exclusive ranges everywhere. Not sure whether it's simpler but it's at least more consistent.

webrender/src/image.rs, line 388 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

should we assert!(first_tile <= last_tile) here for sanity?

I wonder if this can happen in edge-casey clipping conditions, I'd rather not assert in doubt. If this happens it means everything is clipped out and the tile iterator will None at its first iteration which is what one would expect.

webrender/src/image.rs, line 441 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

This is not entirely correct. If both image_range.end % regular_tile_size == 0 and image_range.end < 0 are true, we shouldn't be decreasing the last tile index

Nice catch.

webrender/src/image.rs, line 519 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

this is not an elegant use of match, I think. Did you consider just an old good if/else?

I did originally and found this version easier to read but I don't feel strongly enough to get into an argument about it.

kvark

Reviewed 1 of 2 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jamienicol and @nical)

webrender/src/image.rs, line 383 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

You are right. In fixing that i rewrote it to use [a..b[ kind of exclusive ranges everywhere. Not sure whether it's simpler but it's at least more consistent.

much better now, thank you!
would be good to ensure that the unit tests cover the case

webrender/src/image.rs, line 441 at r2 (raw file):

Previously, nical (Nicolas Silva) wrote…

Nice catch.

I think it's still wrong though:

let regular_tile_size = 10;
let image_range.end = -10;
let end = -1; // computed
if (image_range.end % regular_tile_size == 0) == (image_range.end < 0) { } // returns true

we'll end up with end == 0 ,which is incorrect...

I believe the condition should just be if (image_range.end % regular_tile_size != 0) { end += 1; }


        Auto merge of #313 - nical:rect-range, r=kvark

Add Rect::x_range/y_range. A little goodie that would be useful in servo/webrender#3409.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/313)


        Auto merge of #313 - nical:rect-range, r=kvark

Add Rect::x_range/y_range. A little goodie that would be useful in servo/webrender#3409.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/euclid/313)

nical

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @jamienicol, @nical, and @kvark)

webrender/src/image.rs, line 383 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

much better now, thank you!
would be good to ensure that the unit tests cover the case

Yeah I'll add some tests shortly.

webrender/src/image.rs, line 441 at r2 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I think it's still wrong though:
let regular_tile_size = 10;
let image_range.end = -10;
let end = -1; // computed
if (image_range.end % regular_tile_size == 0) == (image_range.end < 0) { } // returns true
we'll end up with end == 0 ,which is incorrect...

I believe the condition should just be if (image_range.end % regular_tile_size != 0) { end += 1; }

Indeed, thanks!

kvark

Alright, looks like only some extra tests are missing, and the logic is good.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @jamienicol and @nical)


        Support arbitrary tiling origins and negative tile offsets in the til…

…e decomposition algorithm.


        Address review comments.


        Add a few more tests.

bors-servo · 2019-01-30T20:25:45Z

☔ The latest upstream changes (presumably #3530) made this pull request unmergeable. Please resolve the merge conflicts.

nical force-pushed the nical:tile-decomposition branch 4 times, most recently from b3ab003 to 5a73e29 Dec 13, 2018

nical mentioned this pull request Dec 14, 2018

Blob image re-coordination #3418

Open

5 of 10 tasks complete

nical force-pushed the nical:tile-decomposition branch from 5a73e29 to 4227a2c Jan 2, 2019

nical changed the title ~~[WIP] Support arbitrary tiling origins and negative tile offsets during tile decomposition~~ Support arbitrary tiling origins and negative tile offsets during tile decomposition Jan 2, 2019

nical mentioned this pull request Jan 3, 2019

[WIP] hook up the blob visible area in tile-decomposition #3461

Closed

jamienicol reviewed Jan 4, 2019

View changes

nical force-pushed the nical:tile-decomposition branch from 4227a2c to 292bc89 Jan 4, 2019

nical force-pushed the nical:tile-decomposition branch from 292bc89 to d8683d9 Jan 4, 2019

kvark requested changes Jan 8, 2019

View changes

nical reviewed Jan 9, 2019

View changes

nical force-pushed the nical:tile-decomposition branch from 0ebaf6b to 37a9aed Jan 9, 2019

nical mentioned this pull request Jan 9, 2019

Add Rect::x_range/y_range. servo/euclid#313

Merged

kvark requested changes Jan 9, 2019

View changes

nical reviewed Jan 9, 2019

View changes

nical force-pushed the nical:tile-decomposition branch from 37a9aed to 604222b Jan 9, 2019

kvark reviewed Jan 10, 2019

View changes

nical added 2 commits Nov 26, 2018

Support arbitrary tiling origins and negative tile offsets in the til…

3183c41

…e decomposition algorithm.

Address review comments.

4b5c87d

Add a few more tests.

Loading status checks…

c179ece

nical force-pushed the nical:tile-decomposition branch from 604222b to c179ece Jan 21, 2019

nical closed this Jan 30, 2019

servo / webrender

Support arbitrary tiling origins and negative tile offsets during tile decomposition #3409

Support arbitrary tiling origins and negative tile offsets during tile decomposition #3409

nical commented Dec 13, 2018 •

edited by larsbergstrom

nical commented Dec 13, 2018

nical commented Dec 14, 2018

nical commented Jan 2, 2019

nical commented Jan 2, 2019

gw3583 commented Jan 3, 2019

nical commented Jan 3, 2019

jamienicol left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Jan 4, 2019

kvark left a comment

nical left a comment

kvark left a comment

nical left a comment

kvark left a comment

bors-servo commented Jan 30, 2019

servo / webrender

Join GitHub today

Support arbitrary tiling origins and negative tile offsets during tile decomposition #3409

Support arbitrary tiling origins and negative tile offsets during tile decomposition #3409

Conversation

nical commented Dec 13, 2018 • edited by larsbergstrom

nical commented Dec 13, 2018

nical commented Dec 14, 2018

nical commented Jan 2, 2019

nical commented Jan 2, 2019

gw3583 commented Jan 3, 2019

nical commented Jan 3, 2019

jamienicol left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nical commented Jan 4, 2019

kvark left a comment

nical left a comment

kvark left a comment

nical left a comment

kvark left a comment

bors-servo commented Jan 30, 2019

nical commented Dec 13, 2018 •

edited by larsbergstrom