New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support picture caching where content rect moves between display lists. #3407

Merged
merged 3 commits into from Dec 13, 2018

Conversation

Projects
None yet
3 participants
@gw3583
Copy link
Collaborator

gw3583 commented Dec 13, 2018

Gecko bakes scroll offsets into new display lists. Consider the
following example:

(a) WR receives a new display list.
(b) Scroll the display list, generating frames but not a new scene.
(c) Generate a new scene, due to hover / click etc.

In this case, the local coordinates of the primitives (and thus the
enclosing content rect) will be different between (a) and (c), due to
the current scroll offsets being baked into the local primitive rects.

Since this is non-trivial to change in Gecko, we need to ensure that
picture caching handles this case correctly.

There are two parts to this:

(a) Interned primitives must be stored and keyed in a true primitive
local space, removing the local origin. Patches for this have
already landed, and provide an optimization in cases where the
same primitive template is instanced multiple times.
(b) Correctly create cache tiles even if the enclosing content rect
has different coordinates. This is addressed by this patch.

We need to ensure that when generating tiles for a given content rect
the number of tiles (and local offsets) is the same for identical content,
regardless of the location of the content rect. To do this, we need to know
the local bounding rect of the enclosing content, and make tiles placed
relative to that. This moves the tile cache update pass to be done
after the initial picture traversal, so we know the content bounding
rect.

This also means we can reconfigure the tile grid once during
update_transforms, rather than checking after adding each primitive
whether we need to re-allocate the tile grid.


This change is Reviewable

gw3583 added some commits Dec 13, 2018

Support picture caching where content rect moves between display lists.
Gecko bakes scroll offsets into new display lists. Consider the
following example:

(a) WR receives a new display list.
(b) Scroll the display list, generating frames but not a new scene.
(c) Generate a new scene, due to hover / click etc.

In this case, the local coordinates of the primitives (and thus the
enclosing content rect) will be different between (a) and (c), due to
the current scroll offsets being baked into the local primitive rects.

Since this is non-trivial to change in Gecko, we need to ensure that
picture caching handles this case correctly.

There are two parts to this:

(a) Interned primitives must be stored and keyed in a true primitive
    local space, removing the local origin. Patches for this have
    already landed, and provide an optimization in cases where the
    same primitive template is instanced multiple times.
(b) Correctly create cache tiles even if the enclosing content rect
    has different coordinates. This is addressed by this patch.

We need to ensure that when generating tiles for a given content rect
the number of tiles (and local offsets) is the same for identical content,
regardless of the location of the content rect. To do this, we need to know
the local bounding rect of the enclosing content, and make tiles placed
relative to that. This moves the tile cache update pass to be done
*after* the initial picture traversal, so we know the content bounding
rect.

This also means we can reconfigure the tile grid once during
update_transforms, rather than checking after adding each primitive
whether we need to re-allocate the tile grid.
@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 13, 2018

r? @kvark

@gw3583 gw3583 requested a review from kvark Dec 13, 2018

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 13, 2018

There's still a few glitches and incorrect invalidations (which I'll be working on tomorrow), and there seems to be some GPU stalls due to how often tiles get added / removed from the texture cache (got a patch in progress to fix / work around this). Apart from that it seems to be working quite well now.

@kvark
Copy link
Member

kvark left a comment

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gw3583)


webrender/src/frame_builder.rs, line 284 at r1 (raw file):

        // frames (it only does work the first time a new scene is built, or if
        // the tile-relative transform dependencies have changed).
        let mut tile_cache_state = TileCacheUpdateState::new();

would it make sense to just have this as a member of primitive store, so that we don't pass it around as much, and possibly re-use the allocations between updates?


webrender/src/picture.rs, line 341 at r1 (raw file):

    pub opacity_bindings: FastHashMap<PropertyBindingId, OpacityBindingInfo>,
    /// A helper struct to map local rects into picture coords.
    pub space_mapper: SpaceMapper<LayoutPixel, LayoutPixel>,

Unfortunate to see the types here being useless. Can we do better?


webrender/src/picture.rs, line 699 at r1 (raw file):

        // Get the tile coordinates in the picture space.
        let x0 = (rect.origin.x / self.local_tile_size.width).floor() as i32;

can we share this code with the part above?


webrender/src/picture.rs, line 2087 at r1 (raw file):

                }
            }
            self.local_rect = surface_rect;

nit: move this inside the if {} block


webrender/src/prim_store/mod.rs, line 2253 at r1 (raw file):

            // Build the dirty region(s) for this tile cache.
            tile_cache.build_dirty_regions(

why are we taking it out and putting back as opposed to just modifying in place?

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @kvark)


webrender/src/frame_builder.rs, line 284 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

would it make sense to just have this as a member of primitive store, so that we don't pass it around as much, and possibly re-use the allocations between updates?

It might do, yea. I think it'll become clearer what the right organization for this is as we work out how/when we plan to cache in different slices. Even now, we still re-use most allocations (since the tile cache exists in the picture which is persisted across frames, and the tiles are persisted across display lists).


webrender/src/picture.rs, line 341 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

Unfortunate to see the types here being useless. Can we do better?

It's not clear to me how - although we should discuss this. In this case we're mapping from a primitive's local space to get a local rect in the space of a picture. We could use PictureRect etc here, but that then gets messy when we start treating those coordinates as Layout coordinates when we go to draw the tiles.


webrender/src/picture.rs, line 699 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

can we share this code with the part above?

With the translate? I made it a bit tidier by just pre-calculating the origin and using that with the existing size. Not sure if that's really what you meant though.


webrender/src/picture.rs, line 2087 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

nit: move this inside the if {} block

Done/


webrender/src/prim_store/mod.rs, line 2253 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

why are we taking it out and putting back as opposed to just modifying in place?

Because after we've built the dirty regions we want to move it from the current state struct and restore it to the storage inside the picture. We might change how this is arranged in the future, based on your comment above.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 13, 2018

@kvark Thanks, comments addressed.

@kvark

kvark approved these changes Dec 13, 2018

Copy link
Member

kvark left a comment

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gw3583)


webrender/src/picture.rs, line 341 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

It's not clear to me how - although we should discuss this. In this case we're mapping from a primitive's local space to get a local rect in the space of a picture. We could use PictureRect etc here, but that then gets messy when we start treating those coordinates as Layout coordinates when we go to draw the tiles.

PictureRect would make most sense, I think. Where would it get messy?
I mean, if we use the same space everywhere, that surely would reduce the mess, but at a cost that we can't afford ;)


webrender/src/picture.rs, line 699 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

With the translate? I made it a bit tidier by just pre-calculating the origin and using that with the existing size. Not sure if that's really what you meant though.

I mean the identical piece at the end of update_transforms


webrender/src/prim_store/mod.rs, line 2253 at r1 (raw file):

Previously, gw3583 (Glenn Watson) wrote…

Because after we've built the dirty regions we want to move it from the current state struct and restore it to the storage inside the picture. We might change how this is arranged in the future, based on your comment above.

oh, I see it now

@gw3583
Copy link
Collaborator

gw3583 left a comment

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @kvark and @gw3583)


webrender/src/picture.rs, line 341 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

PictureRect would make most sense, I think. Where would it get messy?
I mean, if we use the same space everywhere, that surely would reduce the mess, but at a cost that we can't afford ;)

The confusion comes that sometimes we treat the output of that mapping as a Picture rect (e.g. when mapping primitives into it), but most of the time we treat it as a Layout rect (e.g. when batching, where the batching code expects layout rects, since we're now drawing the tiles as "local" rects).


webrender/src/picture.rs, line 699 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

I mean the identical piece at the end of update_transforms

Ah, that makes sense. I'll unify them into a shared fn as part of the stuff I'm working on today.

@gw3583

This comment has been minimized.

Copy link
Collaborator

gw3583 commented Dec 13, 2018

@bors-servo r=kvark (will fix those last couple of issues as part of the follow up work I'm doing today).

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 13, 2018

📌 Commit 665d69e has been approved by kvark

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 13, 2018

⌛️ Testing commit 665d69e with merge 22f3f35...

bors-servo added a commit that referenced this pull request Dec 13, 2018

Auto merge of #3407 - gw3583:new-tile-deps-8, r=kvark
Support picture caching where content rect moves between display lists.

Gecko bakes scroll offsets into new display lists. Consider the
following example:

(a) WR receives a new display list.
(b) Scroll the display list, generating frames but not a new scene.
(c) Generate a new scene, due to hover / click etc.

In this case, the local coordinates of the primitives (and thus the
enclosing content rect) will be different between (a) and (c), due to
the current scroll offsets being baked into the local primitive rects.

Since this is non-trivial to change in Gecko, we need to ensure that
picture caching handles this case correctly.

There are two parts to this:

(a) Interned primitives must be stored and keyed in a true primitive
    local space, removing the local origin. Patches for this have
    already landed, and provide an optimization in cases where the
    same primitive template is instanced multiple times.
(b) Correctly create cache tiles even if the enclosing content rect
    has different coordinates. This is addressed by this patch.

We need to ensure that when generating tiles for a given content rect
the number of tiles (and local offsets) is the same for identical content,
regardless of the location of the content rect. To do this, we need to know
the local bounding rect of the enclosing content, and make tiles placed
relative to that. This moves the tile cache update pass to be done
*after* the initial picture traversal, so we know the content bounding
rect.

This also means we can reconfigure the tile grid once during
update_transforms, rather than checking after adding each primitive
whether we need to re-allocate the tile grid.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/3407)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Dec 13, 2018

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

@bors-servo bors-servo merged commit 665d69e into servo:master Dec 13, 2018

3 of 4 checks passed

code-review/reviewable 3 discussions left (gw3583, kvark)
Details
Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 14, 2018

Bug 1514177 - Update webrender to commit 22f3f356ea1a9fe2760cd4a609cb…
…a78614e428c4 (WR PR #3407). r=kats

servo/webrender#3407

Differential Revision: https://phabricator.services.mozilla.com/D14535

--HG--
extra : moz-landing-system : lando

mykmelez pushed a commit to mykmelez/gecko that referenced this pull request Dec 14, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment