Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()

RESOLVED FIXED in Firefox 65

Status

()

enhancement
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: sotaro, Assigned: sotaro)

Tracking

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(1 attachment, 6 obsolete attachments)

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.
Assignee: nobody → sotaro.ikeda.g
Summary: Fix Texture release timing of AsyncImagePipelineManager::NotifyPipelinesUpdated() → Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates()
AsyncImagePipelineManager::NotifyPipelinesUpdated() triggers AsyncImagePipelineManager::ProcessPipelineUpdates().
Attachment #9020989 - Flags: review?(nical.bugzilla)
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.
Attachment #9021092 - Flags: review+
Blocks: 1500017
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.
Attachment #9020989 - Flags: review?(nical.bugzilla)
Pushed by sikeda@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c3e0fc819ba
Fix Texture release timing of AsyncImagePipelineManager::ProcessPipelineUpdates() r=nical
https://hg.mozilla.org/mozilla-central/rev/5c3e0fc819ba
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
You need to log in before you can comment on or make changes to this bug.