New issue
Advanced search Search tips

Issue 733828 link

Starred by 1 user
Status: Untriaged
Owner: ----
Cc:
sande...@chromium.org
xhw...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 3
Type: Feature



Sign in to add a comment

Unify handling of mojo VIdeoFrame destruction

Project Member Reported by sande...@chromium.org, Jun 15 2017 
media::mojom::VideoDecoder implements destruction callbacks with an UnguessableToken and an explicit method, OnReleaseMailbox().

media::mojom::Decryptor passes an empty interface (FrameControl) along with the DecryptAndDecodeVideo() response, the release of which triggers resource cleanup.

While the underlying requirements are different, both mechanisms reproduce a portion of VideoFrame destruction observers. It should be possible to implement the complete destruction observer interface using a non-empty interface, and the implementation should be a part of media::mojom::VideoFrame rather than a per-service side-channel. This is more consistent, and allows for lifetimes that exceed the services.
Project Member

Comment 1 by bugdroid1@chromium.org, Dec 5 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c

commit b795cd5ca7a49a8e5da74ff14abf4613ab50d59c
Author: Dan Sanders <sandersd@chromium.org>
Date: Tue Dec 05 21:14:35 2017

[media] Create channel for cross-process VideoFrame release.

This CL defines a new interface, VideoFrameReleaser, and adds a channel
in MojoVideoDecoder to use it, in a way that can outlive the
VideoDecoder.

On the renderer side, MojoVideoFrameReleaser is ref counted, and will
live as long as there are outstanding VideoFrames.

On the gpu side, VideoFrameHandleReleaserImpl maintains a map of
outstanding VideoFrames, and will live until the map is empty.

The old path, with special methods on mojom::VideoDecoder, is removed.
VideoDecoder implementations can now attach regular mailbox release
callbacks to their VideoFrames and expect normal results.

Bug:  737220 , 733828
Change-Id: I3c26e1e98c61f180246815fcc79f6731d8a26ed4
Reviewed-on: https://chromium-review.googlesource.com/762104
Commit-Queue: Dan Sanders <sandersd@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Cr-Commit-Position: refs/heads/master@{#521831}
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/base/video_frame.cc
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/base/video_frame.h
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/clients/mojo_video_decoder.cc
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/clients/mojo_video_decoder.h
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/interfaces/video_decoder.mojom
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/gpu_mojo_media_client.cc
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/gpu_mojo_media_client.h
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/mojo_media_client.cc
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/mojo_media_client.h
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/mojo_video_decoder_service.cc
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/services/mojo_video_decoder_service.h
[modify] https://crrev.com/b795cd5ca7a49a8e5da74ff14abf4613ab50d59c/media/mojo/test/mojo_video_decoder_integration_test.cc

Comment 2 by sande...@chromium.org, Dec 6 2017 

The above CL is a compromise. It still has to be bootstrapped by a different interface, and the release callback needs to be attached externally as well (on the client side).

The generic approach where the VideoFrame StructTraits handles everything (https://chromium-review.googlesource.com/c/chromium/src/+/604469) was rejected because the implementation created a channel for each VideoFrame, but the overhead of this (6 messages to set up) was deemed too high.

In the future we may want to provide an abstraction to wrap the details better, and more easily support cases other than the VideoDecoder interface.
Project Member

Comment 3 by sheriffbot@chromium.org, Dec 7

Labels: Hotlist-Recharge-Cold
Status: Untriaged (was: Available)
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue.

Sorry for the inconvenience if the bug really should have been left as Available.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
►

Sign in to add a comment