1502789 - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement)

Description

[Sotaro Ikeda [:sotaro]]

Since Bug 1495352, AsyncImagePipelineManager::NotifyPipelinesUpdated() is called even when WebRender rendering is skipped. But when the rendering is skipped, we basically should not release TextureHosts for recycling.

If we release TextureHosts during skipping the rendering, There could be a case that TextureHosts are still used by gpu.

Comment 1

[Sotaro Ikeda [:sotaro]]

AsyncImagePipelineManager::NotifyPipelinesUpdated() triggers AsyncImagePipelineManager::ProcessPipelineUpdates().

Comment 2

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 3

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 4

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 5

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 6

[Sotaro Ikeda [:sotaro]]

https://treeherder.mozilla.org/#/jobs?repo=try&selectedJob=208566830&revision=eb689c2ee83d01289de5df9174b887972fd20805

Comment 7

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 8

[Nicolas Silva [:nical]]

Comment on attachment 9021092 [details] [diff] [review]
patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

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

I'll be honest, I'm having a hard time following the logic here, most likely because this code isn't fresh in my memory. Still, if there are comments you can add that explain in the code what we are doing and more importantly why, it would be great.
I'll give this an r+ because it fixes a bug, but eventually I hope that we can drastically simplify it.

Note for the future:
I added to WebRender somewhat recently a notification system that lets you register handlers that will be notified at certain parts of the pipeline. You can do txn->Notify(wr::Checkpoint::FrameRendered, handler); and the handler will get notified on the renderer thread after a frame is rendered that contains the transaction (so if rendering is skipped, it will wait til next frame, etc which I think matches what we want here). I'm hoping that by using this mechanism we can avoid counting frames etc and just and a handler to the next transaction after a texture is unused to do the deletion.

Comment 9

[Sotaro Ikeda [:sotaro]]

Thanks for the review. I am going to add more comments. The patch is created as preparation of Bug 1500017. It requests counting frames. We could revisit usage of txn->Notify(wr::Checkpoint::FrameRendered, handler) after fixing Bug 1500017.

Comment 10

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 11

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

Comment 12

[Sotaro Ikeda [:sotaro]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3e0fc819ba
Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates() r=nical

Comment 14

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/5c3e0fc819ba