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

Store picture primitives in a vec, and refer by index in prims. #3213

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@gw3583
Collaborator

gw3583 commented Oct 18, 2018

This is prep work for changing the structure of the prepare_prims
pass.

Currently, the code is (roughly):

  • Traverse primitive tree, recursing through pictures.
  • When visible, call prepare_prim to update GPU cache, clips etc.
  • Assign render tasks to passes.
  • Assign primitive lists for each picture to batches.

In future, it will instead be:

  • Traverse picture tree, but not individual primitives.
  • Determine surface allocation, plane-splitting, visibility.
  • For each visible picture, prepare_prims and add to batches.

This will simplify some existing code, as well as making possible
some requirements for picture caching (e.g. determining the scale
to rasterize a picture at before creating the child render tasks).


This change is Reviewable

Store picture primitives in a vec, and refer by index in prims.
This is prep work for changing the structure of the prepare_prims
pass.

Currently, the code is (roughly):
 - Traverse primitive tree, recursing through pictures.
  - When visible, call prepare_prim to update GPU cache, clips etc.
 - Assign render tasks to passes.
 - Assign primitive lists for each picture to batches.

In future, it will instead be:
 - Traverse picture tree, but not individual primitives.
  - Determine surface allocation, plane-splitting, visibility.
 - For each visible picture, prepare_prims and add to batches.

This will simplify some existing code, as well as making possible
some requirements for picture caching (e.g. determining the scale
to rasterize a picture at before creating the child render tasks).
@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 18, 2018

r? @kvark

This is a start point to what we were discussing earlier. A few points:

  • The code is going to be a bit messy in the interim in places - hopefully between the changes we discussed and your plane-splitting changes, it should be a lot tidier afterwards.
  • If this looks like it'll be difficult to rebase your plane-splitting patches over, we can just leave this open and I'll add further commits to this as I work through it.
@kvark

Looks good in general!

Traverse picture tree, but not individual primitives.

but we still need to figure out all the local rectangles for the items, so that we know the picture rect

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


webrender/src/display_list_flattener.rs, line 164 at r1 (raw file):

    /// The root picture index for this flattener. This is the picture
    /// to start the culling phase from.
    pub root_pic_index: PictureIndex,

yay, picture indices are back!


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

        prim: PicturePrimitive,
    ) -> PictureIndex {
        let index = PictureIndex(self.pictures.len());

So the new code creates pictures in prepare_prim_for_render, which is done during the frame building. And the primitive store is reset at the scene building. Does that mean we can keep adding more and more pictures to the picture vector when we rebuild the frame without rebuilding the scene? This may cause us leaking memory


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

        }
        // Reset clips from previous frames since we may clip differently each frame.
        self.reset_clip_task(prim_instance);

why do we no longer early return from here?

@gw3583

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


webrender/src/display_list_flattener.rs, line 164 at r1 (raw file):

Previously, kvark (Dzmitry Malyshau) wrote…

yay, picture indices are back!

Yep, hindsight :)


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

Previously, kvark (Dzmitry Malyshau) wrote…

So the new code creates pictures in prepare_prim_for_render, which is done during the frame building. And the primitive store is reset at the scene building. Does that mean we can keep adding more and more pictures to the picture vector when we rebuild the frame without rebuilding the scene? This may cause us leaking memory

It shouldn't be creating pictures during prepare_prim_for_render - they should only be created during flattening?


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

Previously, kvark (Dzmitry Malyshau) wrote…

why do we no longer early return from here?

Previously, this returned a specialized bool to say that a picture which had no surface would never need a clip mask, which was an optimization to avoid redundant clip mask generation.

I realized that with a change made a few weeks ago (the code above which branches on is_passthrough) that this code path never gets hit for pictures without surfaces, so it was redundant.

@gw3583

This comment has been minimized.

Collaborator

gw3583 commented Oct 19, 2018

@kvark Addressed most of those comments - I'm a little unsure about the second one though. I don't think there should be any leaks, but perhaps I'm misunderstanding the comment?

@kvark

kvark approved these changes Oct 19, 2018

I think we are good, thanks for clarifications!

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@kvark

This comment has been minimized.

Member

kvark commented Oct 19, 2018

Confident to go without a try push? Feel free to go ahead regardless ;)

@kvark

This comment has been minimized.

Member

kvark commented Oct 19, 2018

Closing in favor of #3218

@kvark kvark closed this Oct 19, 2018

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