Closed Bug 1312316 Opened 4 years ago Closed 3 years ago

Add stubbed WebRenderCompositorOGL

Categories

(Core :: Graphics: WebRender, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- affected

People

(Reporter: sotaro, Assigned: sotaro)

References

Details

Attachments

(2 files, 14 obsolete files)

187.79 KB, application/pdf
Details
16.15 KB, patch
sotaro
: review+
Details | Diff | Splinter Review
With current webrender api, video rendering is not efficient enough. It is OK for now.
Assignee: nobody → sotaro.ikeda.g
Blocks: webrender
Component: Graphics → Graphics: WebRender
Summary: Render videos with current Webrender API → Render videos via ImageBridge with current Webrender API
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.
(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
Attached patch wip patch (obsolete) — Splinter Review
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?
Attachment #8808475 - Flags: feedback?(nical.bugzilla)
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
Attachment #8808475 - Flags: feedback?(nical.bugzilla)
Attached patch wip patch (obsolete) — Splinter Review
wip patch based on wr-option2.
Attachment #8808475 - Attachment is obsolete: true
Diagram is based on wr-option2.
Attachment #8806240 - Attachment is obsolete: true
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?
Attachment #8808959 - Flags: feedback?(nical.bugzilla)
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.
(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().
(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.
Attachment #8808959 - Flags: feedback?(nical.bugzilla)
Attached patch wip patch (obsolete) — Splinter Review
Use PLayer infra instead of PWebrenderImageContainer.
Attachment #8808959 - Attachment is obsolete: true
Attachment #8808961 - Attachment is obsolete: true
Attached patch wip patch (obsolete) — Splinter Review
update nits.
Attachment #8809309 - Attachment is obsolete: true
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 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.
Attachment #8809312 - Flags: feedback?(nical.bugzilla)
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.
Attachment #8809312 - Flags: feedback?(nical.bugzilla) → feedback+
Thanks for the feedback! For the short time, it is easier to use PLayer. We could replace it, if we do not need it.
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.
Replace WebrenderBasicCompositor with WebrenderCompositorOGL, though its implementation is mostly stub. And did some clean up.
Attachment #8809312 - Attachment is obsolete: true
wr-option2 branch seemed to become invalid, then use wr-e10s branch instead.
  https://github.com/staktrace/gecko-dev/tree/wr-e10s
Rebased on wr-e10s branch.
Attachment #8809726 - Attachment is obsolete: true
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8810356 - Attachment is obsolete: true
Update nits.
Attachment #8810728 - Attachment is obsolete: true
Attachment #8810733 - Flags: review?(nical.bugzilla)
If we want to land only long term thing, we need to wait Bug 1315621.
Attachment #8810733 - Flags: review?(nical.bugzilla)
Depends on: 1315621
It seems better to modify this bug to just adding WebrenderCompositorOGL.
(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.
:jerry, how do you think about it?
Flags: needinfo?(hshih)
No longer depends on: 1315621
Summary: Render videos via ImageBridge with current Webrender API → Add stubbed WebrenderCompositorOGL
Attachment #8810733 - Attachment is obsolete: true
Blocks: 1317893
Attachment #8811110 - Attachment is obsolete: true
Attachment #8811156 - Attachment is obsolete: true
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.
Attachment #8811157 - Flags: review?(nical.bugzilla)
Flags: needinfo?(hshih)
Comment on attachment 8811157 [details] [diff] [review]
patch - Add stubbed WebrenderCompositorOGL

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

(sorry for the review lag!)
Attachment #8811157 - Flags: review?(nical.bugzilla) → review+
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Attachment #8811157 - Attachment is obsolete: true
Attachment #8811542 - Flags: review+
Attachment #8811542 - Attachment description: patch - Add stubbed WebrenderCompositorOGL → patch - Add stubbed WebRenderCompositorOGL
Summary: Add stubbed WebrenderCompositorOGL → Add stubbed WebRenderCompositorOGL
No need to make a PR. You can land on the graphics branch now. See the email thread on dev-tech-gfx.
Thanks, I just noticed the mail.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.