1321144 - use ImageClient/Host in WebRenderXXXXLayer

Status: RESOLVED INCOMPLETE

(Core :: Graphics: WebRender, defect, P3)

Description

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

Currently, all WebRenderXXXXLayer will just render all web-content at content process, and send the result as an image to host-side. So, I would like to make WebRenderXXXXLayer to use ImageClient/Host first. Then, we will get a lot of benefits from this compositable-pair.

Comment 1

[Sotaro Ikeda [:sotaro]]

The bug becomes simpler if we limit to WebRenderImageLayer without using external image id.

Comment 2

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

Right now, the WebRenderPaintedLayer starts to use ImageClient/Host for composition. I will upload the patches soon.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/c135ba14457a
add some operators for WRExternalImageId and WRImageKey. r=gfx?
https://hg.mozilla.org/projects/graphics/rev/a27aac30a168
add WebRender TextureHost related classes. r=sotaro?
https://hg.mozilla.org/projects/graphics/rev/a15ba97ab0e0
handle WR external image callback functions in WebRenderCompositorOGL. r=sotaro?
https://hg.mozilla.org/projects/graphics/rev/77b1f5473828
update AddExternalImageId/AddExternalImageIdForCompositable interface. r=sotaro?
https://hg.mozilla.org/projects/graphics/rev/2b87505b1f1f
update WR external image interface usage in WebRenderImageLayer. r=sotaro?
https://hg.mozilla.org/projects/graphics/rev/36ed3a39cf5e
use ImageClient in WebRenderPaintedLayer. r=sotaro?

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/projects/graphics/rev/49535a40df90
fix unified-build build break for WebRenderPaintedLayer. r=sotaro?

Comment 5

[Sotaro Ikeda [:sotaro]]

build seems still broken on linux, with the following error.

mozilla::layers::WebRenderBridgeParent::RecvAddImage(const uint32_t&, const uint32_t&, const uint32_t&, const WRImageFormat&, const ByteBuffer&, WRImageKey*)’:
 0:05.51 /home/sotaro/quantum_gfx_src_6/graphics/gfx/layers/wr/WebRenderBridgeParent.cpp:163:8: error: variable ‘result’ set but not used [-Werror=unused-but-set-variable]
 0:05.51    auto result = mImages.insert(*aOutImageKey);

Comment 6

[Sotaro Ikeda [:sotaro]]

(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> build seems still broken on linux, with the following error.

release build on linux.

Comment 7

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

The opt builds are still broken, and the debug QR reftests are showing crashes that weren't there before.

I effectively backed this out in https://hg.mozilla.org/projects/graphics/rev/972988713f16f09e9088e7d48f10b298a5c81874 by resetting the tree.

Please do a try push before re-landing to make sure they don't regress anything. You can run just the QR linux reftests by using:
  try: -b do -p linux64-qr -u all[linux64-qr] -t none

Comment 8

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

I fix the build break, but there is still a reftest crash from [1].
The crash shows that one BufferTextureHost is called lock() twice.
I think we might share the same "image" in different ImageContainer.

[1]
https://treeherder.mozilla.org/logviewer.html#?job_id=147304&repo=graphics#L3821

Comment 9

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #8)
> I fix the build break, but there is still a reftest crash from [1].
> The crash shows that one BufferTextureHost is called lock() twice.
> I think we might share the same "image" in different ImageContainer.

Can you explain why do you think that BufferTextureHost is called lock() twice from the log?


By the way, same "image" in different ImageContainer could happen normally on video and images. And it should not be a problem.

Japan is public holiday today. My response will become slower.

Comment 10

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

I just add more logs and test that reftest locally.
Then, it shows that gecko calls BufferTextureHost::Lock() twice within a composition. And the compositable are not the same.

This is what I do now:
---
RecvLayerTransaction:
  addExternalImage() for T1
  pushImage() for T1
  store compositor A
  addExternalImage() for T2
  pushImage() for T2
  store compositor B
Vsync schedule a composition:
  call wr_update() and wr_render()
    WR start to resolve all external image
      Use lock() callback to get T1 from compositable A
      Use lock() callback to get T2 from compositable B
    render()
      draw quad with T1
      draw quad with T2
---

If T1 and T2 are the same one, we will call lock() twice. Then, hit the assert at [1].
How about using a counter instead of a boolean value for checking the lock()/unlock() pair.
The WR drawing sequence is different from what gecko used. The WR will resolve all textures first and then do the draw call. So, if one "image" is shared by multiple compositables, the lock() will be called more than once.

[1]
https://dxr.mozilla.org/mozilla-central/rev/8460203bc93b9667cea1bc00f9d9990a4b1a9474/gfx/layers/composite/TextureHost.cpp#536

Comment 11

[Sotaro Ikeda [:sotaro]]

(In reply to Jerry Shih[:jerry] (UTC+8) from comment #10)
> I just add more logs and test that reftest locally.
> Then, it shows that gecko calls BufferTextureHost::Lock() twice within a
> composition. And the compositable are not the same.
> 
> This is what I do now:
> ---
> RecvLayerTransaction:
>   addExternalImage() for T1
>   pushImage() for T1
>   store compositor A
>   addExternalImage() for T2
>   pushImage() for T2
>   store compositor B
> Vsync schedule a composition:
>   call wr_update() and wr_render()
>     WR start to resolve all external image
>       Use lock() callback to get T1 from compositable A
>       Use lock() callback to get T2 from compositable B
>     render()
>       draw quad with T1
>       draw quad with T2
> ---
> 
> If T1 and T2 are the same one, we will call lock() twice. Then, hit the
> assert at [1].
> How about using a counter instead of a boolean value for checking the
> lock()/unlock() pair.
> The WR drawing sequence is different from what gecko used. The WR will
> resolve all textures first and then do the draw call. So, if one "image" is
> shared by multiple compositables, the lock() will be called more than once.

I am not fun of changing TextureHost locking semantics in this bug, since we need to keep compatibility to non webrender use case like Bug 1320284. We could check if TextureHost is locked by adding IsLocked() like TextureClient. And could share TextureImageTextureSourceWebRenderOGL between CompositableHosts.

If ImageHost does not fit well to webrender usage, we could add new functions to ImageHost or create WebrenderImageHost.

Comment 12

[Sotaro Ikeda [:sotaro]]

And can't we manage the timing of unlocking by using CompositableTextureSourceRef, can we? It is a original idea that we talked in Toronto with nical. Current ImageHost might be incompatible with it ,though.

Comment 13

[Sotaro Ikeda [:sotaro]]

> +  void AddExternalImageIdForDiscard(uint64_t);

Other function name might be better by avoiding "AddExternalImage". It is a bit confusing.

Comment 14

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

I'm going to close this on the assumption that it's specific to the WR-layers implementation, and now we really only care about WR-layers-free. Also it hasn't been touched in months.