1336021 - Add WebRenderImageHost

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[Sotaro Ikeda [:sotaro]]

If we want to add more webrender specific things to CompositableHost, we should add WebRenderImageHost. We are going to add more new things like renader thread handling.

Comment 1

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Add WebRenderImageHost

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

Hi Sotaro,
Is the WebRenderImageHost the final version of webrender specific things of ImageHost? There is no Compositor(e.g. WebRenderCompositorOGL) when we use renderer thread.

Comment 3

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> Hi Sotaro,
> Is the WebRenderImageHost the final version of webrender specific things of
> ImageHost? There is no Compositor(e.g. WebRenderCompositorOGL) when we use
> renderer thread.

It is not final version of handling webrender things. But it implements enough things to handle rendering of webrender things on current graphic branch with/without renderer thread.

Comment 4

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #2)
> Hi Sotaro,
> Is the WebRenderImageHost the final version of webrender specific things of
> ImageHost? There is no Compositor(e.g. WebRenderCompositorOGL) when we use
> renderer thread.

I did not understand well your question, can you explain more?

Comment 5

[Jerry Shih[:jerry] (UTC+8) (inactive)]

If we switch to renderer thread, we should use the gl_context in RendererOGL. So, I think should not use Compositor related interface inside WebRenderImageHost. And there is no layer and mask usage with WebRenderImageHost. That kind of interfaces could be cut off.

Comment 6

[Nicolas Silva [:nical]]

Comment on attachment 8834320 [details] [diff] [review]
patch - Add WebRenderImageHost

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

::: gfx/layers/composite/CompositableHost.cpp
@@ +132,5 @@
>    case CompositableType::IMAGE:
> +    if (gfxVars::UseWebRender()) {
> +      result = new WebRenderImageHost(aTextureInfo);
> +    } else {
> +      result = new ImageHost(aTextureInfo);

I don't think we can decide whether to use WebRenderImageHost based on a global preference, because we are still going to have widgets that don't use WebRender (tooltips, small popups, etc.) and while these should never use the video and ImageBridge, they can still have images (so need the non-wr ImageHost).

Comment 7

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #5)
> If we switch to renderer thread, we should use the gl_context in
> RendererOGL. So, I think should not use Compositor related interface inside
> WebRenderImageHost. And there is no layer and mask usage with
> WebRenderImageHost. That kind of interfaces could be cut off.

Sorry, I do not understand your comment. Compositor related interface just did nothing on render thread case. Why do you think attachment 8834320 [details] [diff] [review] could not be used for renderer thread case? I created attachment 8834320 [details] [diff] [review] to add render thread related things.

Comment 8

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #6)
> I don't think we can decide whether to use WebRenderImageHost based on a
> global preference, because we are still going to have widgets that don't use
> WebRender (tooltips, small popups, etc.) and while these should never use
> the video and ImageBridge, they can still have images (so need the non-wr
> ImageHost).

nical, can you explain more about it? In the above case, do we use ClientLayerManager? The patch affect only to ClientLayerManager case and current code does not have such things yet because of Bug 1337085.

If we do not use WebRenderLayerManager, we should use BasicLayerManager. The patch does not affect to BasicLayerManager.

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> If we do not use WebRenderLayerManager, we should use BasicLayerManager. The
> patch does not affect to BasicLayerManager.

With BasicLayerManager, all composition happens just by using client side APIs.
  https://dxr.mozilla.org/mozilla-central/source/gfx/layers/basic/BasicImageLayer.cpp#63

Comment 10

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #8)
> 
> If we do not use WebRenderLayerManager, we should use BasicLayerManager. The
> patch does not affect to BasicLayerManager.

If compositor is not created, BasicLayerManager is created instead.
  https://dxr.mozilla.org/mozilla-central/source/widget/nsBaseWidget.cpp#1412

Comment 11

[Nicolas Silva [:nical]]

Actually you are right, it seems that we can get away with using non-omtc basic layers for popups and WebRender for everything else.

Comment 12

[Nicolas Silva [:nical]]

Comment on attachment 8834320 [details] [diff] [review]
patch - Add WebRenderImageHost

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

I agree with Jerry in that we can simplify this a lot as soon as we enable the render thread by default, but we can start from there and simplify WebRenderImageHost along the way.

Comment 13

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5327a7b3af31b04f528e7b5e59f10b6dd04dc24

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/feb10d56b092
Add WebRenderImageHost r=nical