1349500 - Add webrender support for BulletFrame path type

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Ethan Lin[:ethlin]]

We'll fall back if the BulletFrame is path type. We should support it.

Comment 1

[Ethan Lin[:ethlin]]

According to [1], we need to support CIRCLE, DISC, SQUARE, DISCLOSURE_CLOSED, DISCLOSURE_OPEN. I think there are two approaches to support path type.
1. Let WR support these simple geometries.
2. Use glyphs to display these types.
Jeff, could you give me some suggestions? 

[1] https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsBulletFrame.cpp#351

Comment 2

[Ethan Lin[:ethlin]]

I prefer option 1 since it should be easy to support these simple geometries in webrender.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

My suggestion would be to try to take advantage of the vector image work that nical is doing. I'd at least like to have an idea why a vector image approach would be more painful before we discard it.

Comment 4

[Nicolas Silva [:nical]]

The vector (or blob) image stuff should work fine for this (not ready yet though), as long as the code on the content side makes sure to keep around the image keys to reuse them and (obviously) not leak them.
If the BulletFrame type is SQUARE we should probably directly insert a rectangle display item instead, though (just simpler and a tad more efficient).

Comment 5

[Nicolas Silva [:nical]]

Note that in the mean time, you can create a skia DrawTarget, render the bullet item in it, turn it into a WebRender image and send it to WebRender this way.
Once the BlobImageRender is integrated, the only difference will be that we'll use a recording DrawTarget instead and call  AddBlobImage with the recording instead of AddImageBuffer.

Comment 6

[Ethan Lin[:ethlin]]

(In reply to Nicolas Silva [:nical] from comment #5)
> Note that in the mean time, you can create a skia DrawTarget, render the
> bullet item in it, turn it into a WebRender image and send it to WebRender
> this way.
> Once the BlobImageRender is integrated, the only difference will be that
> we'll use a recording DrawTarget instead and call  AddBlobImage with the
> recording instead of AddImageBuffer.

Actually it will fall back to WRPaintedLayer if we don't build a layer for the Bullet. So should I create a skia DT for each item and then turn to WR image? or just let WRPaintedLayer to render them?

Comment 7

[Nicolas Silva [:nical]]

Up to you but what I would do:
 - draw a bullet image using a DrawTarget,
 - create a WR image with it and store it somewhere so that it can be used by all bullets of the same shape and color,
 - for all items that use the same bullet parameters, emit a WR image display item using that image.

This way you can already put the logic in place for drawing the bullet outside of WR, separating the rendering of a bullet from the various place where it is used (to avoid re-painting the bullet every time) and managing the lifetime of the image that represent that bullet.
With BlobImageRender we'll just change a tiny bit of the code so that the painting happens in WR instead of the content process but the same infrastructure will be needed to share the bullet image, etc.

Comment 8

[Mason Chang [Inactive] [:mchang]]

(In reply to Nicolas Silva [:nical] from comment #7)
> Up to you but what I would do:
>  - draw a bullet image using a DrawTarget,
>  - create a WR image with it and store it somewhere so that it can be used
> by all bullets of the same shape and color,
>  - for all items that use the same bullet parameters, emit a WR image
> display item using that image.
> 
> This way you can already put the logic in place for drawing the bullet
> outside of WR, separating the rendering of a bullet from the various place
> where it is used (to avoid re-painting the bullet every time) and managing
> the lifetime of the image that represent that bullet.
> With BlobImageRender we'll just change a tiny bit of the code so that the
> painting happens in WR instead of the content process but the same
> infrastructure will be needed to share the bullet image, etc.

Won't rendering things like this be much slower than directly supporting them in WR? It seems like the blob image renderer would force both our old layers infrastructure + new WR to render everything. Why not support these items directly in WR and have WR render them instead? Although I feel like I'm missing something critical here with the vector image stuff.

Comment 9

[Nicolas Silva [:nical]]

When using BlobImageRenderer you are just serializing drawing commands and replaying them on the WR side (using skia for now). Adding support for these items in WebRender directly is the end goal but it is a lot of work. In the mean time BlobImageRenderer moves painting to the compositor side which has the advantages (over the current layers infrastructure) of not being in the content process' main thread and removing the need for the double/triple buffering we do in layers (since you don't have to share a layer anymore). It doesn't imply forcing our old layer infrastructure (but I'm not 100% certain I understand what you meant by that).

Whenever we get to implementing the missing primitives directly in WebRender it'll let us potentially do some of these things on the GPU rather than having skia + texture upload, but that's the main difference, really (that and not having to add a dependency on skia to WR).

If you are talking about painting on the content thread and sending as an image, that's just a temporary thing while I'm implementing the BlobImageRenderer, the API is similar enough that it'll be easy to use blob images instead of real images.

Comment 10

[Mason Chang [Inactive] [:mchang]]

OK, I talked with Nical about this. Sorry I was confused. For simple bullet items (such as a circle or square), we should be able to express these directly as WR display items. For non-simple bullet items, blob image renderer makes sense.

Comment 11

[Ethan Lin[:ethlin]]

I think I can have a patch based on comment 7. So I'll let DisplayItemLayer can convert the display item to WRImage and then see how to optimize the bullet images (maybe in another bug).

Comment 12

[Ethan Lin[:ethlin]]

Attachment: wip

I add a function 'SendAsImage' in DisplayItemlayer. The function will use drawtarget to paint the item on an image and then push the image to WR. In this way we can convert an item which we don't want to put on the PaintedLayer. I will do more check before sending review request.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: paint item as an image for wr

Comment 14

[Mason Chang [Inactive] [:mchang]]

(In reply to Ethan Lin[:ethlin] from comment #12)
> Created attachment 8855247 [details] [diff] [review]
> wip
> 
> I add a function 'SendAsImage' in DisplayItemlayer. The function will use
> drawtarget to paint the item on an image and then push the image to WR. In
> this way we can convert an item which we don't want to put on the
> PaintedLayer. I will do more check before sending review request.

What's the advantage of doing this versus just keeping it in a painted layer instead? If we have to use a DrawTarget to paint something and it's in a painted layer, at least we get invalidation and don't have the overhead of tracking another image / texture upload.

Comment 15

[Ethan Lin[:ethlin]]

(In reply to Mason Chang [:mchang] from comment #14)
> (In reply to Ethan Lin[:ethlin] from comment #12)
> > Created attachment 8855247 [details] [diff] [review]
> > wip
> > 
> > I add a function 'SendAsImage' in DisplayItemlayer. The function will use
> > drawtarget to paint the item on an image and then push the image to WR. In
> > this way we can convert an item which we don't want to put on the
> > PaintedLayer. I will do more check before sending review request.
> 
> What's the advantage of doing this versus just keeping it in a painted layer
> instead? If we have to use a DrawTarget to paint something and it's in a
> painted layer, at least we get invalidation and don't have the overhead of
> tracking another image / texture upload.

Per comment 7, 'for all items that use the same bullet parameters, emit a WR image display item using that image'. If we want to do that, we may need to generate WR image for each display item. I think the path type of bullet item has a high chance to reuse the image, though I'm not sure how much performance we can gain.

Comment 16

[Mason Chang [Inactive] [:mchang]]

(In reply to Ethan Lin[:ethlin] from comment #15)
> (In reply to Mason Chang [:mchang] from comment #14)
> > (In reply to Ethan Lin[:ethlin] from comment #12)
> > > Created attachment 8855247 [details] [diff] [review]
> > > wip
> > > 
> > > I add a function 'SendAsImage' in DisplayItemlayer. The function will use
> > > drawtarget to paint the item on an image and then push the image to WR. In
> > > this way we can convert an item which we don't want to put on the
> > > PaintedLayer. I will do more check before sending review request.
> > 
> > What's the advantage of doing this versus just keeping it in a painted layer
> > instead? If we have to use a DrawTarget to paint something and it's in a
> > painted layer, at least we get invalidation and don't have the overhead of
> > tracking another image / texture upload.
> 
> Per comment 7, 'for all items that use the same bullet parameters, emit a WR
> image display item using that image'. If we want to do that, we may need to
> generate WR image for each display item. I think the path type of bullet
> item has a high chance to reuse the image, though I'm not sure how much
> performance we can gain.

Hmm ok. From https://bugzilla.mozilla.org/show_bug.cgi?id=1351131#c5, it doesn't look like this type is on the hot path. A lot of the image texture code seemed to be resued in WebRenderPaintedLayer. Can we refactor the two out please?

Comment 17

[Ethan Lin[:ethlin]]

(In reply to Mason Chang [:mchang] from comment #16)
> (In reply to Ethan Lin[:ethlin] from comment #15)
> > (In reply to Mason Chang [:mchang] from comment #14)
> > > (In reply to Ethan Lin[:ethlin] from comment #12)
> > > > Created attachment 8855247 [details] [diff] [review]
> > > > wip
> > > > 
> > > > I add a function 'SendAsImage' in DisplayItemlayer. The function will use
> > > > drawtarget to paint the item on an image and then push the image to WR. In
> > > > this way we can convert an item which we don't want to put on the
> > > > PaintedLayer. I will do more check before sending review request.
> > > 
> > > What's the advantage of doing this versus just keeping it in a painted layer
> > > instead? If we have to use a DrawTarget to paint something and it's in a
> > > painted layer, at least we get invalidation and don't have the overhead of
> > > tracking another image / texture upload.
> > 
> > Per comment 7, 'for all items that use the same bullet parameters, emit a WR
> > image display item using that image'. If we want to do that, we may need to
> > generate WR image for each display item. I think the path type of bullet
> > item has a high chance to reuse the image, though I'm not sure how much
> > performance we can gain.
> 
> Hmm ok. From https://bugzilla.mozilla.org/show_bug.cgi?id=1351131#c5, it
> doesn't look like this type is on the hot path. A lot of the image texture
> code seemed to be resued in WebRenderPaintedLayer. Can we refactor the two
> out please?

Okay, I'll see how to reuse these code.

Comment 18

[Ethan Lin[:ethlin]]

Attachment: Add webrender support for BulletFrame path type

I add a class UpdateImageHelper for getting drawtarget and updating image. So WRPaintedLayer and WRDisplayItemLayer can share the image code.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8857365 [details] [diff] [review]
Add webrender support for BulletFrame path type

Review of attachment 8857365 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
@@ +119,5 @@
> +  }
> +
> +  Rect dest = RelativeToParent(Rect(0, 0, imageSize.width, imageSize.height)) + offset;
> +  WrClipRegion clipRegion = aBuilder.BuildClipRegion(wr::ToWrRect(dest));
> +  WrImageKey key;

nit: Move this into WebRenderLayerManager::GetImageKey or something like that.

::: layout/generic/nsBulletFrame.cpp
@@ +486,4 @@
>                                                 layers::WebRenderDisplayItemLayer* aLayer)
>  {
>    MOZ_ASSERT(IsPathType());
> +  if (!aLayer->PushItemAsImage(aBuilder, aParentCommands)) {

nit: Should assert aLayer->mItem == aItem

Comment 20

[Ethan Lin[:ethlin]]

(In reply to Mason Chang [:mchang] from comment #19)
> Comment on attachment 8857365 [details] [diff] [review]
> Add webrender support for BulletFrame path type
> 
> Review of attachment 8857365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/wr/WebRenderDisplayItemLayer.cpp
> @@ +119,5 @@
> > +  }
> > +
> > +  Rect dest = RelativeToParent(Rect(0, 0, imageSize.width, imageSize.height)) + offset;
> > +  WrClipRegion clipRegion = aBuilder.BuildClipRegion(wr::ToWrRect(dest));
> > +  WrImageKey key;
> 
> nit: Move this into WebRenderLayerManager::GetImageKey or something like
> that.
>
Okay, I add a function in WebRenderLayer. 

> ::: layout/generic/nsBulletFrame.cpp
> @@ +486,4 @@
> >                                                 layers::WebRenderDisplayItemLayer* aLayer)
> >  {
> >    MOZ_ASSERT(IsPathType());
> > +  if (!aLayer->PushItemAsImage(aBuilder, aParentCommands)) {
> 
> nit: Should assert aLayer->mItem == aItem

I will add the assertion.

Comment 21

[Ethan Lin[:ethlin]]

I fixed some try failures. The try result is fine now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1519500893cb57e832e4e72c296516e324968173

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/af6a9987ba8d
Add webrender support for BulletFrame path type. r=mchang

Comment 23

[Kartikaya Gupta (email:[e-mail])]

https://hg.mozilla.org/mozilla-central/rev/af6a9987ba8d