Closed Bug 1321144 Opened 4 years ago Closed 4 years ago

use ImageClient/Host in WebRenderXXXXLayer

Categories

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

53 Branch
defect

Tracking

()

RESOLVED INCOMPLETE
Tracking Status
firefox53 --- affected

People

(Reporter: jerry, Assigned: jerry)

References

Details

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.
Blocks: 1321145
Blocks: 1320624
The bug becomes simpler if we limit to WebRenderImageLayer without using external image id.
Depends on: 1322043
Right now, the WebRenderPaintedLayer starts to use ImageClient/Host for composition. I will upload the patches soon.
Assignee: nobody → hshih
Status: NEW → ASSIGNED
Pushed by hshih@mozilla.com:
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?
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Pushed by hshih@mozilla.com:
https://hg.mozilla.org/projects/graphics/rev/49535a40df90
fix unified-build build break for WebRenderPaintedLayer. r=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);
Flags: needinfo?(hshih)
(In reply to Sotaro Ikeda [:sotaro] from comment #5)
> build seems still broken on linux, with the following error.

release build on linux.
Blocks: 1325349
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Flags: needinfo?(hshih) → needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(hshih)
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
Flags: needinfo?(hshih) → needinfo?(sotaro.ikeda.g)
(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.
Flags: needinfo?(sotaro.ikeda.g) → needinfo?(hshih)
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.
> +  void AddExternalImageIdForDiscard(uint64_t);

Other function name might be better by avoiding "AddExternalImage". It is a bit confusing.
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.
Status: REOPENED → RESOLVED
Closed: 4 years ago4 years ago
Flags: needinfo?(hshih)
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.