Decompose tiled images during frame building (v4) #2742

jrmuizel · 2018-05-08T22:08:57Z

This is a continuation of #2704

This change is

kvark · 2018-05-09T14:56:05Z

Reviewed 11 of 11 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.

webrender/src/batch.rs, line 613 at r3 (raw file):

        // use some of the per-primitive data typically stored in PrimitiveMetadata and get
        // it from each sub-primitive instead.
        let is_multiple_primitives = match prim_metadata.prim_kind {

is this a new concept introduced by the PR? We already have segments, so this is a bit confusing

webrender/src/batch.rs, line 995 at r3 (raw file):

                        }
                    }
                    BrushKind::Image { request, ref visible_tiles, .. } if !visible_tiles.is_empty() => {

does it make sense to take the _ arm if no visible tiles are there? It seems that we are better just going the for loop unconditionally here

webrender/src/batch.rs, line 1365 at r3 (raw file):

    );

    if cache_item.texture_id == SourceTexture::Invalid {

I'd really like to nuke that Invalid variant, perhaps later... but it's unfortunate to see more use of it

webrender/src/image.rs, line 183 at r3 (raw file):

    // Position of the first visible tile (top-left) in layer space.
    let x0 = prim_rect.origin.x + t0.x as f32 * layer_tile_size.width;

nit: I don't think we need those, the x_count computation is not getting more complex if we just do it all in there

webrender/src/prim_store.rs, line 2480 at r3 (raw file):

}

fn compute_conservative_visible_rect(

as mentioned in #2704 (comment) we shouldn't have this method

webrender/src/renderer.rs, line 139 at r3 (raw file):

    color: debug_colors::LIGHTGREY,
};
const GPU_TAG_PRIM_IMAGE: GpuProfileTag = GpuProfileTag {

🎉

Comments from Reviewable

jrmuizel · 2018-05-09T21:24:45Z

Review status: all files reviewed at latest revision, 6 unresolved discussions.

webrender/src/prim_store.rs, line 2480 at r3 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

as mentioned in #2704 (comment) we shouldn't have this method

This seems insufficient. In my rawtest test case prim_run_context.clip_chain_rect is None and so we end up not clipping at all instead of clipping to the screen bounds.

Comments from Reviewable

jrmuizel · 2018-05-09T22:38:56Z

This has one of the review comments applied. The others seemed less actionable, or things we can do in a follow up once nical is back.

nical · 2018-05-10T12:15:32Z

is this a new concept introduced by the PR? We already have segments, so this is a bit confusing

Yep, version 2 of this tile decomposition saga was an attempt to use segments and it went wrong in various ways:

We ran into situations where we need more than a couple hundred tiles which blows up the max amount of segments (which are always contiguously allocated in the gpu cache).
I had to add a new way way to select the source rect per segment which added a fair bit of code to the shaders.
I had to change the way segments selected their batches in that particular case (because tiles of a given primitive wouldn't necessarily end up sampling the same texture).
At the end none of the segment infrastructure ended up being useful to the tiling case and the implementation ended up twice as complicated as this one even though they pretty much do the same thing.

That said, with some extra work we could actually segment individual tiles just like we do for regular primitives to move more pixels to the opaque pass when there is aa or clipping involved.

does it make sense to take the _ arm if no visible tiles are there? It seems that we are better just going the for loop unconditionally here

This arm is specific to the tiled case. we could rewrite it to handle both tiled and non-tiled images but the non-tiled image path is shares more with the rest of the primitives than it shares with the tiled image path so it wouldn't buy us much.

bors-servo · 2018-05-10T16:29:35Z

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

kvark · 2018-05-10T18:25:18Z

webrender/src/image.rs

+    use euclid::{TypedRect, TypedSize2D, TypedPoint2D};
+
+    fn point<T: Copy, U>(x: T, y: T) -> TypedPoint2D<T, U> {
+        TypedPoint2D::new(x, y)


let's just use existing euclid's point2, size2, and possibly rect helpers

kvark · 2018-05-10T18:28:12Z

webrender/src/image.rs

+                      device_tile_size,
+                      &mut |tile_rect, tile_offset, tile_flags| {
+                          // make sure we don't get sent duplicate tiles
+                          assert!(!tiles.contains(&tile_offset));


can assert that prim_rect.contains(&tile_rect) as well as
!tiles.any(|t| t.intersects(&tile_rect)) which is superior to !tiles.contains(&tile_offset)

kvark · 2018-05-10T18:28:57Z

webrender/src/image.rs

+    #[test]
+    fn basic() {
+        let mut count = 0;
+        checked_for_each_tile(&rect(0., 0., 1000., 1000.),


we can just make it to count things right away instead of accepting a generic closure

kvark · 2018-05-10T18:29:05Z

webrender/src/image.rs

+        );
+        assert_eq!(count, 0);
+    }
+}


kvark · 2018-05-10T19:04:53Z

🚢 it
@bors-servo r+

bors-servo · 2018-05-10T19:04:54Z

📌 Commit 9d5a0e3 has been approved by kvark

bors-servo · 2018-05-10T19:04:57Z

⌛ Testing commit 9d5a0e3 with merge d588119...

Decompose tiled images during frame building (v4) This is a continuation of #2704  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2742)

bors-servo · 2018-05-10T20:28:11Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: kvark
Pushing d588119 to master...

jrmuizel force-pushed the tile-decomposition branch from 258ebcf to c2ec519 Compare May 8, 2018 22:27

jrmuizel force-pushed the tile-decomposition branch from c2ec519 to 4711191 Compare May 9, 2018 22:34

jrmuizel force-pushed the tile-decomposition branch 2 times, most recently from e30ba77 to 7b557af Compare May 10, 2018 18:15

kvark reviewed May 10, 2018

View reviewed changes

jrmuizel and others added 3 commits May 10, 2018 14:57

Decompose tiled images during frame building.

066eeb3

Remove the non-brush image primitive and shader.

8c1768e

Add some tests for for_each_tile.

9d5a0e3

jrmuizel force-pushed the tile-decomposition branch from 7b557af to 9d5a0e3 Compare May 10, 2018 18:58

bors-servo merged commit 9d5a0e3 into servo:master May 10, 2018

nical mentioned this pull request May 14, 2018

Lazily tile images #2370

Closed

Uh oh!

Decompose tiled images during frame building (v4) #2742

Decompose tiled images during frame building (v4) #2742

Uh oh!

Conversation

jrmuizel commented May 8, 2018 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kvark commented May 9, 2018

Uh oh!

jrmuizel commented May 9, 2018

Uh oh!

jrmuizel commented May 9, 2018

Uh oh!

nical commented May 10, 2018

Uh oh!

bors-servo commented May 10, 2018

Uh oh!

kvark May 10, 2018

Choose a reason for hiding this comment

Uh oh!

kvark May 10, 2018

Choose a reason for hiding this comment

Uh oh!

kvark May 10, 2018

Choose a reason for hiding this comment

Uh oh!

kvark May 10, 2018

Choose a reason for hiding this comment

Uh oh!

kvark commented May 10, 2018

Uh oh!

bors-servo commented May 10, 2018

Uh oh!

bors-servo commented May 10, 2018

Uh oh!

bors-servo commented May 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jrmuizel commented May 8, 2018 •

edited by larsbergstrom

Loading