1312316 - Add stubbed WebRenderCompositorOGL

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

With current webrender api, video rendering is not efficient enough. It is OK for now.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: diagram of current WebrenderLayerManager

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: Diagram of implementation image of video rendering via ImageBridge

Comment 3

[Sotaro Ikeda [:sotaro]]

attachment 8806240 [details] just shows fist step status of video rendering via ImageBridge. I notice the following steps for now. They could be changed.

- Render videos via ImageBridge with current Webrender API
  + Add WebRenderAsyncImageContainer to get current ImageKey from ImageHost
  + Add WebRenderCompositor
  + Add Vsync handling
  + Add NotifyImageComposites() handling

- Split WebRenderImageLayer::RenderLayer() and video data update
  Modify Webrender API for rendering videos via ImageBridge
  Permit only image key allocation and update the image key via ImageBridge

- Add YCbCr rendering handling to Webrender

- Add shared texture support to Webrender
  + Add shared BufferTexture support to Webrender
  + Add shared direct binding Texture support to Webrender

- Add oop handling

- Move multiple Images handling to Webrender
   Actual TextureHost selection logic should be opaque from webrender.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #3)
> attachment 8806240 [details] just shows fist step status of video rendering
> via ImageBridge. I notice the following steps for now. They could be changed.

The link has more info.
  https://public.etherpad-mozilla.org/p/wrgfx-video

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Comment 6

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8808475 [details] [diff] [review]
wip patch

:nical, the patch is still early stage. Can you feedback to a direction of the patch?

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8808475 [details] [diff] [review]
wip patch

I am going to update the patch based on wr-option2
  https://github.com/staktrace/gecko-dev/tree/wr-option2

Comment 8

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

wip patch based on wr-option2.

Comment 9

[Sotaro Ikeda [:sotaro]]

Attachment: Diagram of implementation image of video rendering via ImageBridge

Diagram is based on wr-option2.

Comment 10

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8808959 [details] [diff] [review]
wip patch

:nical, I created a wip patch based on wr-option2. Can you feedback to a direction of the patch?

Comment 11

[Nicolas Silva [:nical]]

Comment on attachment 8808959 [details] [diff] [review]
wip patch

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

Sorry I haven't had the time to thoroughly review this yet but I have some initial remarks:

I think we can make things simpler by reusing the existing Compositable infrastructure instead of add PWebRenderImageContainer and the likes. We will probably need the same thing for the non-video content that isn't supported by webrender like xul and svg anyway. So rather than adding a new type of ImageContainer I think that we should keep using ImageClient/ImageHost, and make it so that instead of passing an asyncId to pair up with a layer we pass a webrender ExternalImageKey.

When the ImageHost receives a TextureHost, it will use the API that Jerry is working on to tell wr's renderer which texture to use (and upload if need be) for a given ExternalImageKey.

I realize that this relies on things that are not fully implemented yet, but I think that a large amount of code in this patch will not be used once the pieces come together (which I hope will happen soon-ish).

You Created a Basic WR Compositor, why is that? Is it to force the content side to believe it is a basic backend and allocate buffer textures?
We definitely need a WR compositor. It should manage the gl context so I'd rather call it GL instead of Basic, but that part of the patch looks useful.

Comment 12

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #11)
> Comment on attachment 8808959 [details] [diff] [review]
> wip patch
> 
> 
> I realize that this relies on things that are not fully implemented yet, but
> I think that a large amount of code in this patch will not be used once the
> pieces come together (which I hope will happen soon-ish).

I do not care about that many code is going to be replaced. But I am going to try to find a way that could reduce the replacement.

> 
> You Created a Basic WR Compositor, why is that? Is it to force the content
> side to believe it is a basic backend and allocate buffer textures?
> We definitely need a WR compositor. It should manage the gl context so I'd
> rather call it GL instead of Basic, but that part of the patch looks useful.

The patch is wip patch. The reason is that it was easier to use wr_add_image().

Comment 13

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #11)
> Comment on attachment 8808959 [details] [diff] [review]
> wip patch
> 
> Review of attachment 8808959 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Sorry I haven't had the time to thoroughly review this yet but I have some
> initial remarks:
> 
> I think we can make things simpler by reusing the existing Compositable
> infrastructure instead of add PWebRenderImageContainer and the likes. We
> will probably need the same thing for the non-video content that isn't
> supported by webrender like xul and svg anyway. So rather than adding a new
> type of ImageContainer I think that we should keep using
> ImageClient/ImageHost, and make it so that instead of passing an asyncId to
> pair up with a layer we pass a webrender ExternalImageKey.

I am going to update the patch as to use Compositable infrastructure. The patch did not cared about non-video content.

Comment 14

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

Use PLayer infra instead of PWebrenderImageContainer.

Comment 15

[Sotaro Ikeda [:sotaro]]

Attachment: wip patch

update nits.

Comment 16

[Sotaro Ikeda [:sotaro]]

attachment 8809312 [details] [diff] [review] uses LayerManagerComposite and ImageHost as much as possible. To do it PLayer is added to PWebRenderBridge. But PCompositor is not added to PWebRenderBridge for now since video uses ImageBridge's PCompositor.

The patch does not update WebrenderBasicCompositor part. I am going to update it tomorrow.

Comment 17

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8809312 [details] [diff] [review]
wip patch

:nical, can you comment to a direction of the wip patch? PWebrenderImageContainer is removed based on the comment. Though other comments are not applied yet.

Comment 18

[Nicolas Silva [:nical]]

Comment on attachment 8809312 [details] [diff] [review]
wip patch

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

At a glance I like this approach a lot more than the previous version, thanks for making these changes. In the long run if we keep "webrender option-2" I think that we won't need PLayer to interact with PWebRender (and use PCompositable directly instead) but if in the mean time that makes it easier for you it's fine, and it does not add large amounts of code anyway.

Comment 19

[Sotaro Ikeda [:sotaro]]

Thanks for the feedback! For the short time, it is easier to use PLayer. We could replace it, if we do not need it.

Comment 20

[Sotaro Ikeda [:sotaro]]

By the way, ipdl seems to have some quirks about include ordering. I tried several times to Add PLayer to PWebRenderBridge by modifying protocol include ordering. Just simply adding the PLayer caused ipdl compile error.

Comment 21

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Render videos via ImageBridge with current Webrender API

Replace WebrenderBasicCompositor with WebrenderCompositorOGL, though its implementation is mostly stub. And did some clean up.

Comment 22

[Sotaro Ikeda [:sotaro]]

wr-option2 branch seemed to become invalid, then use wr-e10s branch instead.
  https://github.com/staktrace/gecko-dev/tree/wr-e10s

Comment 23

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Render videos via ImageBridge with current Webrender API

Rebased on wr-e10s branch.

Comment 24

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

All the patches have been merged to Gecko-WebRenderer, you can use that directly. Are you subscribed to the dev-tech-gfx mailing list? You should subscribe if you have not yet.

https://lists.mozilla.org/listinfo/dev-tech-gfx

Comment 25

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> All the patches have been merged to Gecko-WebRenderer, you can use that
> directly. Are you subscribed to the dev-tech-gfx mailing list? You should
> subscribe if you have not yet.
> 
> https://lists.mozilla.org/listinfo/dev-tech-gfx

Thanks for the info! I already subscribed to dev-tech-gfx, but I did not check it.

Comment 26

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Render videos via ImageBridge with current Webrender API

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Render videos via ImageBridge with current Webrender API

Update nits.

Comment 28

[Sotaro Ikeda [:sotaro]]

If we want to land only long term thing, we need to wait Bug 1315621.

Comment 29

[Sotaro Ikeda [:sotaro]]

It seems better to modify this bug to just adding WebrenderCompositorOGL.

Comment 30

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> It seems better to modify this bug to just adding WebrenderCompositorOGL.

Since all other parts are temporary implementation.

Comment 31

[Sotaro Ikeda [:sotaro]]

:jerry, how do you think about it?

Comment 32

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add stubbed WebrenderCompositorOGL

Comment 33

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add stubbed WebrenderCompositorOGL

Comment 34

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add stubbed WebrenderCompositorOGL

Comment 35

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8811157 [details] [diff] [review]
patch - Add stubbed WebrenderCompositorOGL

:nica, can you review the patch. I changed the scope of the bug only to adding stubbed WebrenderCompositorOGL.

Comment 36

[Nicolas Silva [:nical]]

Comment on attachment 8811157 [details] [diff] [review]
patch - Add stubbed WebrenderCompositorOGL

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

(sorry for the review lag!)

Comment 37

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

Comment on attachment 8811157 [details] [diff] [review]
patch - Add stubbed WebrenderCompositorOGL

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

Please rename the compositor class to WebRenderCompositorOGL (uppercase R in WebRender). When I migrate the github branch to the graphics project branch I will be renaming the other files as well.

Comment 38

[Sotaro Ikeda [:sotaro]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #37)
> Comment on attachment 8811157 [details] [diff] [review]
> 
> Please rename the compositor class to WebRenderCompositorOGL (uppercase R in
> WebRender). When I migrate the github branch to the graphics project branch
> I will be renaming the other files as well.

Ok, I am going to rename it.

Comment 39

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add stubbed WebRenderCompositorOGL

Comment 40

[Sotaro Ikeda [:sotaro]]

https://github.com/jrmuizel/gecko-dev/pull/54

Comment 41

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

No need to make a PR. You can land on the graphics branch now. See the email thread on dev-tech-gfx.

Comment 42

[Sotaro Ikeda [:sotaro]]

Thanks, I just noticed the mail.

Comment 43

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add stubbed WebRenderCompositorOGL

Rebased to https://hg.mozilla.org/projects/graphics

Comment 44

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/projects/graphics/rev/db70478b254ccb80716e9fc9ae0506a7564669e7

Comment 45

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

Build bustage fix: https://hg.mozilla.org/projects/graphics/rev/7606e52195d7c9d95e4c4894af3352aceca0faf7

Also trivial comment and namespace cleanup: https://hg.mozilla.org/projects/graphics/rev/f27e26039a4d9d4f4041347d341b98054fb10612