Generate a |release_token| for VideoFrames with destruction observers. [chromium/src : main]
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
Patch set 3:Commit-Queue +1
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()Looks exactly correct to me, except that you also need to check |mailbox_holders_and_gmb_release_cb_ […]
Ok, I think the latest patchset has your suggestions.
I can't say I'm fully comfortable with this though :) mostly because of the uncertainty around thread safety: it feels like even if ToT is thread-unsafe, it's only because of a race between `AddDestructionObserver()` and the destructor. I think that may be an easier failure to detect compared to potential defects introduced by this CL: I'm not only adding another possible point of failure, but if ToT is thread-unsafe, I may be adding more undefined behavior because I'm adding a call to std::vector::empty() which may race with std::vector::push_back() ([this answer](https://stackoverflow.com/a/4205568) made me worry about undefined behavior, not just raciness).
Would there be anything safer? Maybe telling the MojoVideoDecoderService at construction-time to always register frames? Or maybe adding a lock around `done_callbacks_` if only to rule out undefined behavior?
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
Patch set 4:Code-Review +1
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
Ok, I think the latest patchset has your suggestions. […]
I did search through the code for calls to AddDestructionObserver() and didn't find anything suspicious, I think this change will be fine.
FWIW if it came to it I'd rather have an inverse lock to freeze the state once configuration of the frame is done. I'd make a regular builder but I'm not sure if I can make the Mojo integration ergonomic with that.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
Patch set 5:Commit-Queue +1
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
Sounds good! I went ahead and also added the following:
```
// WARNING: This method is not thread safe; it should only be called if you
// are still the only owner of this VideoFrame.
```To `HasReleaseMailboxCB()` and `HasDestructionObservers()` which seems to make sense because if the setters are not thread safe, then the accessors shouldn't be either. What do you think?
Additionally, can you elaborate more on what you meant by this:
FWIW if it came to it I'd rather have an inverse lock to freeze the state once configuration of the frame is done.
I'm not sure I understood.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
Patch set 5:Code-Review +1
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
if the setters are not thread safe, then the accessors shouldn't be either. What do you think?
The setters are not safe *because* the accessors are called on multiple threads. The model is that this data is read-only after initialization.
FWIW if it came to it I'd rather have an inverse lock [...]
I'm not sure I understood.
This is a theoretical proposal to be able to seal media::VideoFrames so that the 'read-only after initialization' property is enforced by the setters.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
Patch set 6:Commit-Queue +1
2 comments:
File media/base/video_frame.cc:
bool VideoFrame::HasDestructionObservers() const {
return wrapped_frame_ ? wrapped_frame_->HasDestructionObservers()
: HasReleaseMailboxCB() || !done_callbacks_.empty();
}I just noticed something:
`SetReleaseMailboxCB()` and `SetReleaseMailboxAndGpuMemoryBufferCB()` are not allowed to be called for a frame that wraps another frame. Therefore, `HasReleaseMailboxCB()` is well defined: it should not be possible for an outer frame to have such a callback while the inner frame doesn't.
`AddDestructionObserver()` is different because it's possible for the outer frame to have destruction observers while the inner frame doesn't. I think `HasDestructionObservers()` should return true for such a situation.
I went ahead and changed this to account for that situation (it's also consistent with the documentation for the method in the .h). I also added a unit test for that.
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
The setters are not safe *because* the accessors are called on multiple threads. The model is that this data is read-only after initialization.
I see. Then the warning comments I added to `HasReleaseMailboxCB()` and `HasDestructionObservers()` don't make too much sense. I've removed them.
One thing, however, is that there are at least a couple of places where we don't follow this read-only-after-initialization model:
1) For out-of-process video decoding: when we receive a decoded VideoFrame from the video decoder process, we add a destruction observer [here](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/oop_video_decoder.cc;l=412-417;drc=ca8dbbeb748efb71f6fb791675851a18f3f66cb2). After that, we give it to the `MailboxVideoFrameConverter` which later adds another destruction observer to that frame [here](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/mailbox_video_frame_converter.cc;l=405-423;drc=e80563b9eff7a0d41593b8fc955278fc726e6c99) (this is actually not happening on ToT, but it's the intention with CL:3759116).
2) For regular in-GPU-process video decoding: we create GpuMemoryBuffer VideoFrames to decode onto [here](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/platform_video_frame_utils.cc;l=216-218;drc=e3267d772fb05c2fdc64180d3c71c71280dd18e7). That frame later gets to the `MailboxVideoFrameConverter` where we add a destruction observer just like in case (1).
I figured these scenarios were fine because we shouldn't be accessing the destruction observer state concurrently in them. But are we abusing the destruction observer mechanism?
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
Patch set 6:Code-Review +1
2 comments:
File media/base/video_frame.cc:
bool VideoFrame::HasDestructionObservers() const {
return wrapped_frame_ ? wrapped_frame_->HasDestructionObservers()
: HasReleaseMailboxCB() || !done_callbacks_.empty();
}
I just noticed something: […]
Good catch, I totally missed that error!
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
> The setters are not safe *because* the accessors are called on multiple threads. […]
Oops, I suppose these didn't show up with a symbol search so I missed them with my earlier query.
OOPVideoDecoder: This should be safe since OnVideoFrameDecoded() is called by Mojo while the |frame| has just been deserialized and therefore is still uniquely-owned.
MailboxVideoFrameConverter: This is more suspicious because it's difficult to prove the frame hasn't also been referenced elsewhere. The standard approach here is to use WrapVideoFrame(), and then the registration would live as long as the wrapper instead of the |origin_frame|.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
MailboxVideoFrameConverter: This is more suspicious because it's difficult to prove the frame hasn't also been referenced elsewhere. The standard approach here is to use WrapVideoFrame(), and then the registration would live as long as the wrapper instead of the |origin_frame|.
The frame has in fact been referenced elsewhere :) The way it works is that we have a pool that allocates GMB VideoFrames. When it hands them out to the decoder, it uses WrapVideoFrame() to [add a destruction observer](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/platform_video_frame_pool.cc;l=128-140;drc=101402b62c1190fc5e0aa88c4e7432b98ed8d521) to the wrapper so that when the wrapper is destroyed, the GMB frame is returned to the pool. It transfers ownership of the GMB frame to the wrapper, but [it still holds onto a raw pointer](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/platform_video_frame_pool.cc;l=136-137;drc=101402b62c1190fc5e0aa88c4e7432b98ed8d521).
The MailboxVideoFrameConverter gets the wrapper and gains access to the GMB frame by calling an [unwrap callback](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/mailbox_video_frame_converter.cc;l=171-173;drc=e80563b9eff7a0d41593b8fc955278fc726e6c99) (which basically is [this method](https://source.chromium.org/chromium/chromium/src/+/main:media/gpu/chromeos/platform_video_frame_pool.cc;l=260-267;drc=101402b62c1190fc5e0aa88c4e7432b98ed8d521)).
My gut feeling tells me that this situation is still safe because I don't expect concurrent access to the destruction observer state (of either the wrapper frame or the GMB frame). However, given the complexity and difficulty of reasoning, I was very interested in your suggestion to make this safer/harder to mess up :) There are other places (e.g., [these](https://source.chromium.org/search?q=AddDestructionObserver&sq=&ss=chromium%2Fchromium%2Fsrc:media%2Fgpu%2Fv4l2%2F)) that I'm even less familiar with and might be suspicious.
The standard approach here is to use WrapVideoFrame(), and then the registration would live as long as the wrapper instead of the |origin_frame|.
The reason we add this observer to the inner frame (i.e., the GMB frame given out by our pool) is that we want to keep the corresponding SharedImage created by the MailboxVideoFrameConverter alive for as long as the GMB frame lives. So, if the wrapper of that frame dies, we want it to be returned to the pool, but we also want the SharedImage to survive so that we can re-use it if we see that GMB frame again.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
So, if the wrapper of that frame dies, we want it to be returned to the pool, but we also want the SharedImage to survive so that we can re-use it if we see that GMB frame again.
This makes some sense, but I would recommend either combining the pools into one that has knowledge of both things, or creating a new pooling layer to hold these modified frames. It also seems possible to create a pool just for the SharedImages and map them to new frames as necessary?
For things that are more like 'cacheable value computed from a VideoFrame', I would consider adding locked members to VideoFrame, but so far we don't have anything quite like that.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
It also seems possible to create a pool just for the SharedImages and map them to new frames as necessary?
Ignore that, clearly unworkable.
I'm not sure which of the sub-pool vs. merge with root pool is better in this case, but it's worth noting that the VDAVideoDecoder/PictureBufferManager version of this has a pool that maintains both, and wraps them into VideoFrames only right as they are output.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
> It also seems possible to create a pool just for the SharedImages and map them to new frames as ne […]
We've also considered a pool that manages SharedImages and underlying buffers together, but that has lost some relevance for us due to the out-of-process video decoding project: in that use case, the underlying buffers are allocated in one process and the SharedImages are created in another (the GPU process).
That also implies that the particular concern we've been discussing (MailboxVideoFrameConverter adding a destruction observer to a frame that's referenced elsewhere) would be moot once I enable out-of-process decoding by default. There might however be other suspicious cases we haven't discussed hidden in the code.
With all this considered, I see various courses of action:
1) Just YOLO it and land this CL: however, at the very least, the documentation I added in media/base/video_frame.h would need to be changed because we know for sure there's a case where the thing that adds the destruction observer is not the only owner of the frame, even though it's probably safe.
2) Guard the new code I'm adding with the out-of-process decoding feature flag: I don't like this too much because it pollutes the code.
3) Don't rely on the state of the VideoFrame to decide whether to register it or not -- instead, we could tell the MojoVideoDecoderService at creation time whether to register frames or not.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
We've also considered a pool that manages SharedImages and underlying buffers together, but that has […]
(1) is basically how things are working right now so I'm basically okay with that plan. We can add locking if it turns out to be necessary.
(3) is fine, although I'm not sure I understand how it's sufficient.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
(1) is basically how things are working right now so I'm basically okay with that plan. We can add locking if it turns out to be necessary.
(3) is fine, although I'm not sure I understand how it's sufficient.
We may be thinking about different things for (3), but it would work because the only reason I need this CL is for the video decoder that lives in its own process. The `MojoVideoDecoderService` that lives there is created [here](https://source.chromium.org/chromium/chromium/src/+/main:media/mojo/services/stable_video_decoder_factory_service.cc;l=117-119;drc=c290c2b0e9d7946b2fde3b93b81de0f4c4e2705e). So if I pass a parameter like `must_register_all_outgoing_frames = true` to the ctor, the condition in the `MojoVideoDecoderService` would look like
```
if ((must_register_all_outgoing_frames_ || frame->HasReleaseMailboxCB()) &&
video_frame_handle_releaser_) {
```Not sure how ugly that is though.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
> (1) is basically how things are working right now so I'm basically okay with that plan. […]
Oh I see. The problem of late-adding a destruction observer still exists, but it doesn't affect this code any more.
That's fine, the "ugly" part is that you need a way to configure it, which probably means adding another getter method to the media::VideoDecoder interface so that MojoVideoDecoderService can inspect it.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Andres Calderon Jaramillo (Gerrit)
Attention is currently required from: Dan Sanders.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
Oh I see. The problem of late-adding a destruction observer still exists, but it doesn't affect this code any more.
That's fine, the "ugly" part is that you need a way to configure it, which probably means adding another getter method to the media::VideoDecoder interface so that MojoVideoDecoderService can inspect it.
I think it's simpler than that: it could be provided as a parameter to the MojoVideoDecoderService. It can be made `true` [here](https://source.chromium.org/chromium/chromium/src/+/main:media/mojo/services/stable_video_decoder_factory_service.cc;l=117;drc=c290c2b0e9d7946b2fde3b93b81de0f4c4e2705e) (which is only used for the out-of-process decoder) and `false` elsewhere. WDYT?
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.
Dan Sanders (Gerrit)
Attention is currently required from: Andres Calderon Jaramillo.
1 comment:
File media/mojo/services/mojo_video_decoder_service.cc:
Patch Set #1, Line 418:
if ((frame->HasReleaseMailboxCB() || frame->HasGpuMemoryBuffer()
> Oh I see. […]
It could be done that way, but it's the MojoMediaClient that then picks a decoder and the decoder that cares about the frame lifetime; I'd rather not spread the information across extra layers.
To view, visit change 3741131. To unsubscribe, or for help writing mail filters, visit settings.