[media] Avoid resource reuse in RenderableMappableSIVFPool if lost [chromium/src : main]
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 1 comment![]( "Open in Gerrit")
Related details
- Colin Blundell
- Dale Curtis
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 3 comments![]( "Open in Gerrit")
This change fixes it by plumbing a lost resource signal from theI don't quite follow the description here: the above paragraph indicates that the bug is that the client is writing to the SI while the DisplayCompositor is still reading from it, but this paragraph outlines the fix as plumbing through a signal that the DisplayCompositor sends only as part of its callback indicating that it is *done* with its read?
if (shared_image_release_cb) {Why this change?
if (shared_image_release_cb) {Why this change?
Related details
- Dale Curtis
- Saifuddin Hitawala
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell added 1 comment![]( "Open in Gerrit")
Thanks for working on this!
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 2 comments![]( "Open in Gerrit")
This change fixes it by plumbing a lost resource signal from theI don't quite follow the description here: the above paragraph indicates that the bug is that the client is writing to the SI while the DisplayCompositor is still reading from it, but this paragraph outlines the fix as plumbing through a signal that the DisplayCompositor sends only as part of its callback indicating that it is *done* with its read?
Sorry for the confusion. I believe before the frame resource is reused, it is marked as lost resource by the Display Compositor after Display Compositor is done with it. But I am actually not sure if the Display Compositor is really done with it? Maybe it is still being used somewhere in the viz pipeline. @vas...@chromium.org do you know?
And then when it is added back to reuse it is overwritten.
if (shared_image_release_cb) {Why this change?
I needed this because now it goes through the overloaded `SetReleaseMailboxCB` which adapts and returns a non-null wrapper on L#1438 always and so it is set. And so I was hitting the `DCHECK(release_mailbox_cb)`. I could instead just move this inside SetReleaseMailboxCB.
Related details
- Colin Blundell
- Dale Curtis
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Vasiliy Telezhnikov (Gerrit)
Vasiliy Telezhnikov added 2 comments![]( "Open in Gerrit")
(didn't look at the cl in details yet, just answering comment)
This change fixes it by plumbing a lost resource signal from theSaifuddin HitawalaI don't quite follow the description here: the above paragraph indicates that the bug is that the client is writing to the SI while the DisplayCompositor is still reading from it, but this paragraph outlines the fix as plumbing through a signal that the DisplayCompositor sends only as part of its callback indicating that it is *done* with its read?
Sorry for the confusion. I believe before the frame resource is reused, it is marked as lost resource by the Display Compositor after Display Compositor is done with it. But I am actually not sure if the Display Compositor is really done with it? Maybe it is still being used somewhere in the viz pipeline. @vas...@chromium.org do you know?
And then when it is added back to reuse it is overwritten.
So when we export resource to display compositor, we pass callback which usually says that display compositor is done with the resource. Callback is carefully threaded through a long pipeline.
But there are cases when we unable to receive callback, e.g if we are about to close mojo channel that is used to deliver this callback from the gpu process. There are other cases when we close the window for example.
When it happens, it's our last chance to tell client that things are gone, but due to asynchronous nature of the display compositor, it can't stop using resources immediately, so we can't force it.
That lead to the situation where display compositor is still using the resource, but it's unable to tell when it's done anymore. To live with it, there is concept of lost resources. We tell clients that "We are unable to determine when this resource will be safe to re-use, so don't reuse it".
This signal wasn't plumbed through the video frame, which wasn't a problem until we started rendering to it.
Related details
- Colin Blundell
- Dale Curtis
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Colin Blundell (Gerrit)
Colin Blundell voted and added 4 comments![]( "Open in Gerrit")
Votes added by Colin Blundell
| Code-Review | +1 |
4 comments
Thanks!
This change fixes it by plumbing a lost resource signal from theSaifuddin HitawalaI don't quite follow the description here: the above paragraph indicates that the bug is that the client is writing to the SI while the DisplayCompositor is still reading from it, but this paragraph outlines the fix as plumbing through a signal that the DisplayCompositor sends only as part of its callback indicating that it is *done* with its read?
Vasiliy TelezhnikovSorry for the confusion. I believe before the frame resource is reused, it is marked as lost resource by the Display Compositor after Display Compositor is done with it. But I am actually not sure if the Display Compositor is really done with it? Maybe it is still being used somewhere in the viz pipeline. @vas...@chromium.org do you know?
And then when it is added back to reuse it is overwritten.
So when we export resource to display compositor, we pass callback which usually says that display compositor is done with the resource. Callback is carefully threaded through a long pipeline.
But there are cases when we unable to receive callback, e.g if we are about to close mojo channel that is used to deliver this callback from the gpu process. There are other cases when we close the window for example.
When it happens, it's our last chance to tell client that things are gone, but due to asynchronous nature of the display compositor, it can't stop using resources immediately, so we can't force it.
That lead to the situation where display compositor is still using the resource, but it's unable to tell when it's done anymore. To live with it, there is concept of lost resources. We tell clients that "We are unable to determine when this resource will be safe to re-use, so don't reuse it".
This signal wasn't plumbed through the video frame, which wasn't a problem until we started rendering to it.
Ah got it, thanks. `lost_resource` here is really "resource is lost to YOU (the client) because I (display compositor) am not done with it *now* and will no longer be able to inform you when I *am* done with it". Worth extending the CL description with this info.
if (shared_image_release_cb) {Saifuddin HitawalaWhy this change?
I needed this because now it goes through the overloaded `SetReleaseMailboxCB` which adapts and returns a non-null wrapper on L#1438 always and so it is set. And so I was hitting the `DCHECK(release_mailbox_cb)`. I could instead just move this inside SetReleaseMailboxCB.
Thanks, that makes sense! Leaving it here sounds good.
if (shared_image_release_cb) {Colin BlundellWhy this change?
Acknowledged
Related details
- Dale Curtis
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala voted and added 2 comments![]( "Open in Gerrit")
Votes added by Saifuddin Hitawala
| Commit-Queue | +2 |
2 comments
This change fixes it by plumbing a lost resource signal from theSaifuddin HitawalaI don't quite follow the description here: the above paragraph indicates that the bug is that the client is writing to the SI while the DisplayCompositor is still reading from it, but this paragraph outlines the fix as plumbing through a signal that the DisplayCompositor sends only as part of its callback indicating that it is *done* with its read?
Vasiliy TelezhnikovSorry for the confusion. I believe before the frame resource is reused, it is marked as lost resource by the Display Compositor after Display Compositor is done with it. But I am actually not sure if the Display Compositor is really done with it? Maybe it is still being used somewhere in the viz pipeline. @vas...@chromium.org do you know?
And then when it is added back to reuse it is overwritten.
Colin BlundellSo when we export resource to display compositor, we pass callback which usually says that display compositor is done with the resource. Callback is carefully threaded through a long pipeline.
But there are cases when we unable to receive callback, e.g if we are about to close mojo channel that is used to deliver this callback from the gpu process. There are other cases when we close the window for example.
When it happens, it's our last chance to tell client that things are gone, but due to asynchronous nature of the display compositor, it can't stop using resources immediately, so we can't force it.
That lead to the situation where display compositor is still using the resource, but it's unable to tell when it's done anymore. To live with it, there is concept of lost resources. We tell clients that "We are unable to determine when this resource will be safe to re-use, so don't reuse it".
This signal wasn't plumbed through the video frame, which wasn't a problem until we started rendering to it.
Ah got it, thanks. `lost_resource` here is really "resource is lost to YOU (the client) because I (display compositor) am not done with it *now* and will no longer be able to inform you when I *am* done with it". Worth extending the CL description with this info.
Thanks for the details! Yes that all makes sense, there's just too many callbacks to keep track of here. I've updated the description with an extra paragraph on this and how lost resource is tracked.
Related details
- Dale Curtis
- Vasiliy Telezhnikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
8 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
[media] Avoid resource reuse in RenderableMappableSIVFPool if lost
On Google Meet with background effects and a Picture-in-Picture
extension, the video flickers due to a race condition in how the
VideoFrames are recycled.
With background effects, VideoFrames use RenderableMappableSIVFPool
which reuses SharedImages for performance. The issue arises when a
VideoFrame is returned to the pool because the VideoFrameSubmitter
and VideoResourceUpdater are destroyed (PiP extension stops sending
frames if Meet is also on on same window), while the Display
Compositor is still reading from it. If the SharedImage is reused by
the pool, it gets overwritten and then there's a mix of old and new
frame data being displayed which causes flickering.
Display Compositor usually sends a callback to clients which says it
is done with the resource. But sometimes the callback is not run
because of breakages in pipeline (mojo channel lost etc.). In such
cases DisplayCompositor tells the clients that the SharedImage resource
is lost and not to reuse it. This tells the clients to not reuse the
resource, but the DisplayCompositor might be still using it and the
display Compositor will not be able to tell the clients anymore about
when it will be done with it. This becomes a problem as VideoFrame is
reused for rendering and overwriting the resource for Meet effects in
RenderableMappableSIVFPool.
This change fixes it by plumbing the lost resource signal from the
Display Compositor to the RenderableMappableSIVFPool to prevent its
reuse. A tracking lost resource member is added on VideoFrame which is
set on ReuseTexture which is a callback that is run by viz Display
Compositor. This is then plumbed over new
ReleaseMailboxCBWithLostResource callback type to the
RenderableMappableSIVFPool which checks if resource is lost before
reusing it in OnVideoFrameDestroyed.
Tested locally on mac with PiP extension making sure the fix works
as expected.
- M media/base/video_frame.cc
- M media/base/video_frame.h
- M media/renderers/video_resource_updater.cc
- M media/renderers/video_resource_updater.h
- M media/video/renderable_mappable_shared_image_video_frame_pool.cc
Code-Review: +1 by Colin Blundell
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
void VideoFrame::SetLostSharedImageResource(bool lost_si_resource) {How is this thread safe? We usually try to avoid mutable carrying state on VideoFrame since it may be held in many places at once.y7
if (lost_shared_image_resource) {Seems like there should be a way for the pool to get this information directly since it created the SharedImage in the first place?
Related details
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Saifuddin Hitawala (Gerrit)
Saifuddin Hitawala added 2 comments![]( "Open in Gerrit")
void VideoFrame::SetLostSharedImageResource(bool lost_si_resource) {How is this thread safe? We usually try to avoid mutable carrying state on VideoFrame since it may be held in many places at once.y7
Sorry about that, I didn't realize this needed to be thread-safe as its only used from the destructor. I can look into adding a base::Lock on `lost_shared_image_resource_` in a follow-up.
if (lost_shared_image_resource) {Seems like there should be a way for the pool to get this information directly since it created the SharedImage in the first place?
This seems possible as we currently set the bool on VideoFrame but instead could do it on the ClientSharedImage as this should only happen in cases where VideoFrame is backed by SharedImage.
I chatted with @vas...@chromium.org about this and it seems more involved because of the callback mechanism currently plumbed through VideoFrame and VideoResourceUpdater, and how it works with ClientResourceProvider. Maybe we could update this as part of SharedImagePool support.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dale Curtis (Gerrit)
Dale Curtis added 2 comments![]( "Open in Gerrit")
void VideoFrame::SetLostSharedImageResource(bool lost_si_resource) {Saifuddin HitawalaHow is this thread safe? We usually try to avoid mutable carrying state on VideoFrame since it may be held in many places at once.y7
Sorry about that, I didn't realize this needed to be thread-safe as its only used from the destructor. I can look into adding a base::Lock on `lost_shared_image_resource_` in a follow-up.
Ah, that's likely okay then, but seems fragile in case someone tries to use the value elsewhere. I'd at least add a comment to the bool that it's only safe to read in the destructor.
if (lost_shared_image_resource) {Saifuddin HitawalaSeems like there should be a way for the pool to get this information directly since it created the SharedImage in the first place?
This seems possible as we currently set the bool on VideoFrame but instead could do it on the ClientSharedImage as this should only happen in cases where VideoFrame is backed by SharedImage.
I chatted with @vas...@chromium.org about this and it seems more involved because of the callback mechanism currently plumbed through VideoFrame and VideoResourceUpdater, and how it works with ClientResourceProvider. Maybe we could update this as part of SharedImagePool support.
- Saifuddin Hitawala
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |