Add mailbox-based Mojo VideoFrame variant.

Add mailbox-based Mojo VideoFrame variant.

This CL creates a foundation for plumbing mailbox-based Mojo VideoFrames, but
does not fully implement all of the callbacks. The expected implementation (in
MojoVideoDecoderService) is:

  - Once a decoder running in the GPU process has decoded a frame,
MojoVideoDecoderService can attach a |release_token| to the
OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
generate one whenever the video frame is texture-backed, and it will store refs
to all such outstanding video frames (keyed by this generated |release_token|).

  - The renderer handles the video frame as usual, flowing through the regular
pipeline and compositing steps. Once compositing is done, the compositor sets
the |release_sync_token| based on its own command buffer. The frame is then
destructed (as usual).

  - MojoVideoDecoder receives the destruction callback. At this point the
|release_sync_token| has been verified as part of hopping to the media thread
and command buffer. It now forwards the sync point to the GPU process via the
OnReleaseMailbox() Mojo IPC.

  - MojoVideoDecoderService (on the GPU side) will check that the sync point is
valid, terminating the Mojo connection if it is not. It then looks up, updates,
and removes the video frame from its map. If there is an attached destruction
observer, it will receive the |release_sync_point|, and the decoder will know
that the textures can be re-used after waiting for that.

Note that if the media command buffer and compositor command buffer are ever
merged (again), the validation step won't happen as part of the hopping anymore,
and validation on the GPU side could fail (timing dependent). We would need to
switch to sending the OnReleaseMailbox() IPC via the GPU channel instead of
Mojo in that case (or take the performance hit of validating the sync point from
the Renderer explicitly).

BUG=522298
Review-Url: https://codereview.chromium.org/2640153004
Cr-Commit-Position: refs/heads/master@{#454427}
Committed: https://chromium.googlesource.com/chromium/src/+/ef768ffc5dbe5364af1db5c04da524109b4b5f08

[sandersd (OOO until July 31), 2017-01-19 19:40:47]

sandersd@chromium.org changed reviewers:
+ xhwang@chromium.org

[sandersd (OOO until July 31), 2017-01-19 19:40:48]

This outlines the last piece of MojoVideoDecoder, and I'm sending it now for
early feedback on the design.

After this lands, I will be able to build a GPU-side replacement for
GpuVideoDecoder over this Mojo API and we can drop the Chrome IPC version.
(Actual timeline TBD.)

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:361: void SetReleaseMailboxCB(const ReleaseMailboxCB&
release_mailbox_cb);
Option B is that the VideoFrame converter needs to become aware of this
callback, and therefore cannot be a standard Mojo converter.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/clients/mojo_vid...
File media/mojo/clients/mojo_video_decoder.cc (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/clients/mojo_vid...
media/mojo/clients/mojo_video_decoder.cc:116:
&MojoVideoDecoder::OnReleaseMailbox, base::Unretained(this), callback));
This isn't useful for non-mailbox VideoFrames, but it won't break either.

Option B is to have two different OnVideoFrameDecoded() methods, only one of
which would request destruction callbacks. We would probably want to switch away
from the union type to a different structure for that, so that we can require
mailbox-based VideoFrames for the callback variant.

Option C is to have an OnReleaseMailbox() Mojo method, and not use callbacks at
all. That would require matching frames to callbacks manually, but probably fits
our model better.

Note that the lifetime relationship implies that the callback may not be called
before the connection is torn down. (This supports Option C.)

https://codereview.chromium.org/2640153004/diff/1/media/mojo/common/BUILD.gn
File media/mojo/common/BUILD.gn (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/common/BUILD.gn#...
media/mojo/common/BUILD.gn:17: "//gpu/ipc/common:struct_traits",
I'm not actually sure whether this is required. That said, I think there are
some GPU things I used without pulling them in, and that worked, so I'm not sure
what the dependency tree actually looks like.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/BUILD.gn
File media/mojo/interfaces/BUILD.gn (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/BUILD...
media/mojo/interfaces/BUILD.gn:29: "//gpu/ipc/common:interfaces",
(This one is required for sure.)

[xhwang, 2017-01-23 22:05:41]

sorry for the delay. I'll take a look and provide feedback at the latest by EOD
tomorrow.

[xhwang, 2017-01-25 18:25:09]

Sorry again for the delay.

Overall it looks very reasonable. I just have a few nits.

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:775: mailbox_holders_release_cb_ =
std::move(release_mailbox_cb);
OOC nit, not related to your CL: do we need to worry about the thread safty of
VideoFrame?

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:361: void SetReleaseMailboxCB(const ReleaseMailboxCB&
release_mailbox_cb);
On 2017/01/19 19:40:48, sandersd wrote:
> Option B is that the VideoFrame converter needs to become aware of this
> callback, and therefore cannot be a standard Mojo converter.

This is fine IMHO. VideoFrame is already a mix of everything :(

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_decoder.mojom:75: // reclaim backing resources.)
+jrummell since we need to solve the same problem in Decryptor for shared memory
based VideoFrames. But in the Decryptor case, we'd need a callback to a
callback, which is probably not supported in mojo today.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_decoder.mojom:77: (gpu.mojom.SyncToken?
release_sync_token);
What does it mean to call the callback with a null token (since
|release_sync_token| is optional)?

https://codereview.chromium.org/2640153004/diff/1/media/mojo/services/mojo_vi...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/services/mojo_vi...
media/mojo/services/mojo_video_decoder_service.h:57: void OnVideoFrameDestroyed(
nit: In VideoFrame we have DestructionObserver, how about s/Destroy/Destruct/?

[sandersd (OOO until July 31), 2017-01-26 23:49:05]

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.cc
File media/base/video_frame.cc (right):

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.cc#n...
media/base/video_frame.cc:775: mailbox_holders_release_cb_ =
std::move(release_mailbox_cb);
On 2017/01/25 18:25:09, xhwang_slow wrote:
> OOC nit, not related to your CL: do we need to worry about the thread safty of
> VideoFrame?

I suppose we do. I added a warning to the header; for now it just says that you
should only call this method in constructor-like situations.

We can fix it with locking if necessary, but we really don't need that for the
current use case.

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h
File media/base/video_frame.h (right):

https://codereview.chromium.org/2640153004/diff/1/media/base/video_frame.h#ne...
media/base/video_frame.h:361: void SetReleaseMailboxCB(const ReleaseMailboxCB&
release_mailbox_cb);
On 2017/01/25 18:25:09, xhwang_slow wrote:
> On 2017/01/19 19:40:48, sandersd wrote:
> > Option B is that the VideoFrame converter needs to become aware of this
> > callback, and therefore cannot be a standard Mojo converter.
> 
> This is fine IMHO. VideoFrame is already a mix of everything :(

Acknowledged.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
File media/mojo/interfaces/video_decoder.mojom (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_decoder.mojom:75: // reclaim backing resources.)
On 2017/01/25 18:25:09, xhwang_slow wrote:
> +jrummell since we need to solve the same problem in Decryptor for shared
memory
> based VideoFrames. But in the Decryptor case, we'd need a callback to a
> callback, which is probably not supported in mojo today.

FYI the solution proposed here only works properly for mailboxes given certain
assumptions about the command buffer. If your solution also involves mailboxes
then we should meet to verify that the same restrictions apply.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_decoder.mojom:77: (gpu.mojom.SyncToken?
release_sync_token);
On 2017/01/25 18:25:09, xhwang_slow wrote:
> What does it mean to call the callback with a null token (since
> |release_sync_token| is optional)?

Note: I've replaced this mechanism in the current CL.

But the same thing applies in the new code: if the frame was not a
mailbox-backed frame, or the release sync token was never set, then we don't
have a sync point to pass back.

https://codereview.chromium.org/2640153004/diff/1/media/mojo/services/mojo_vi...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/2640153004/diff/1/media/mojo/services/mojo_vi...
media/mojo/services/mojo_video_decoder_service.h:57: void OnVideoFrameDestroyed(
On 2017/01/25 18:25:09, xhwang_slow wrote:
> nit: In VideoFrame we have DestructionObserver, how about s/Destroy/Destruct/?

Renamed to OnReleaseMailbox().

[sandersd (OOO until July 31), 2017-01-26 23:57:13]

Patchset #3 (id:40001) has been deleted

[sandersd (OOO until July 31), 2017-01-27 00:13:26]

sandersd@chromium.org changed reviewers:
+ dcheng@chromium.org

[sandersd (OOO until July 31), 2017-01-27 00:13:26]

dcheng@: Please security review changes to .mojom files and the type converters.

Since I'm once again asking you to security review code without an impl, I'll
try to describe the global view of how this is intended to work:

  - Once a decoder running in the GPU process has decoded a frame,
MojoVideoDecoderService can attach a |release_token| to the
OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
generate one whenever the video frame is texture-backed, and it will store refs
to all such outstanding video frames (keyed by this generated |release_token|).

  - The renderer handles the video frame as usual, flowing through the regular
pipeline and compositing steps. Once compositing is done, the compositor sets
the |release_sync_token| based on its own command buffer. The frame is then
destructed (as usual).

  - MojoVideoDecoder receives the destruction callback. At this point the
|release_sync_token| has been verified as part of hopping to the media thread
and command buffer. It now forwards the sync point to the GPU process via the
OnReleaseMailbox() IPC.

  - MojoVideoDecoderService (on the GPU side) will check that the sync point is
valid, terminating the Mojo connection if it is not. It then looks up, updates,
and removes the video frame from its map. If there is an attached destruction
observer, it will receive the |release_sync_point|, and the decoder will know
that the textures can be re-used after waiting for that.

Note that if the media command buffer and compositor command buffer are ever
merged (again), the validation step won't happen as part of the hopping anymore,
and validation on the GPU side could fail (timing dependent). We would need to
switch to sending the IPC via the GPU channel instead of Mojo in that case (or
take the performance hit of validating the sync point from the Renderer
explicitly).

[sandersd (OOO until July 31), 2017-02-07 19:08:44]

On 2017/01/27 00:13:26, sandersd wrote:
> dcheng@: Please security review changes to .mojom files and the type
converters.
> 
> Since I'm once again asking you to security review code without an impl, I'll
> try to describe the global view of how this is intended to work:
> 
>   - Once a decoder running in the GPU process has decoded a frame,
> MojoVideoDecoderService can attach a |release_token| to the
> OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
> generate one whenever the video frame is texture-backed, and it will store
refs
> to all such outstanding video frames (keyed by this generated
|release_token|).
> 
>   - The renderer handles the video frame as usual, flowing through the regular
> pipeline and compositing steps. Once compositing is done, the compositor sets
> the |release_sync_token| based on its own command buffer. The frame is then
> destructed (as usual).
> 
>   - MojoVideoDecoder receives the destruction callback. At this point the
> |release_sync_token| has been verified as part of hopping to the media thread
> and command buffer. It now forwards the sync point to the GPU process via the
> OnReleaseMailbox() IPC.
> 
>   - MojoVideoDecoderService (on the GPU side) will check that the sync point
is
> valid, terminating the Mojo connection if it is not. It then looks up,
updates,
> and removes the video frame from its map. If there is an attached destruction
> observer, it will receive the |release_sync_point|, and the decoder will know
> that the textures can be re-used after waiting for that.
> 
> Note that if the media command buffer and compositor command buffer are ever
> merged (again), the validation step won't happen as part of the hopping
anymore,
> and validation on the GPU side could fail (timing dependent). We would need to
> switch to sending the IPC via the GPU channel instead of Mojo in that case (or
> take the performance hit of validating the sync point from the Renderer
> explicitly).

xhwang, dcheng: ping.

[xhwang, 2017-02-08 06:30:47]

sorry I missed this one

lgtm

[sandersd (OOO until July 31), 2017-02-13 22:25:01]

Description was changed from

==========
Add mailbox-based Mojo VideoFrame variant.

BUG=522298
==========

to

==========
Add mailbox-based Mojo VideoFrame variant.

BUG=522298
==========

[sandersd (OOO until July 31), 2017-02-13 22:25:01]

sandersd@chromium.org changed reviewers:
+ nasko@chromium.org
- dcheng@chromium.org

[sandersd (OOO until July 31), 2017-02-13 22:25:52]

Over to nasko@ for security review. (See original message to dcheng@ for
background on how this works.)

[nasko, 2017-02-16 21:19:05]

On 2017/02/07 19:08:44, sandersd wrote:
> On 2017/01/27 00:13:26, sandersd wrote:
> > dcheng@: Please security review changes to .mojom files and the type
> converters.
> > 
> > Since I'm once again asking you to security review code without an impl,
I'll
> > try to describe the global view of how this is intended to work:
> > 
> >   - Once a decoder running in the GPU process has decoded a frame,
> > MojoVideoDecoderService can attach a |release_token| to the
> > OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
> > generate one whenever the video frame is texture-backed, and it will store
> refs
> > to all such outstanding video frames (keyed by this generated
> |release_token|).
> > 
> >   - The renderer handles the video frame as usual, flowing through the
regular
> > pipeline and compositing steps. Once compositing is done, the compositor
sets
> > the |release_sync_token| based on its own command buffer. The frame is then
> > destructed (as usual).
> > 
> >   - MojoVideoDecoder receives the destruction callback. At this point the
> > |release_sync_token| has been verified as part of hopping to the media
thread
> > and command buffer. It now forwards the sync point to the GPU process via
the
> > OnReleaseMailbox() IPC.
> > 
> >   - MojoVideoDecoderService (on the GPU side) will check that the sync point
> is
> > valid, terminating the Mojo connection if it is not. It then looks up,
> updates,
> > and removes the video frame from its map. If there is an attached
destruction
> > observer, it will receive the |release_sync_point|, and the decoder will
know
> > that the textures can be re-used after waiting for that.
> > 
> > Note that if the media command buffer and compositor command buffer are ever
> > merged (again), the validation step won't happen as part of the hopping
> anymore,
> > and validation on the GPU side could fail (timing dependent). We would need
to
> > switch to sending the IPC via the GPU channel instead of Mojo in that case
(or
> > take the performance hit of validating the sync point from the Renderer
> > explicitly).
> 
> xhwang, dcheng: ping.

I would suggest putting this explanation in the CL description above, so it is
actually visible to anyone who opens the CL without having to hunt inside
comments.

[nasko, 2017-02-16 21:37:29]

I will be out next week, so please use tsepez@ or dcheng@ for further follow up.

Since you mention this interface is not live yet, it should really be converted
to use struct traits instead of using type mappers.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_video_decoder.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/clients/mojo...
media/mojo/clients/mojo_video_decoder.cc:112: void
MojoVideoDecoder::OnVideoFrameDecoded(
Can this be called on multiple threads?

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:326:
TypeConverter<media::mojom::VideoFramePtr, scoped_refptr<media::VideoFrame>>::
In general, please avoid using type converters and move them over to struct
traits.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:375:
CHECK(input->data->is_shared_buffer_data());
You should not be using CHECK in here, as the data can be arbitrary and hostile.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.cc:136:
client_->OnVideoFrameDecoded(mojom::VideoFrame::From(frame), base::nullopt);
You probably want base/optional.h included here instead of the header file.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.h:12: #include "base/optional.h"
Where is base::optional used here? Do we really need this include?

[sandersd (OOO until July 31), 2017-02-16 21:38:00]

Description was changed from

==========
Add mailbox-based Mojo VideoFrame variant.

BUG=522298
==========

to

==========
Add mailbox-based Mojo VideoFrame variant.

This CL creates a foundation for plumbing mailbox-based Mojo VideoFrames, but
does not fully implement all of the callbacks. The expected implementation (in
MojoVideoDecoderService) is:

  - Once a decoder running in the GPU process has decoded a frame,
MojoVideoDecoderService can attach a |release_token| to the
OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
generate one whenever the video frame is texture-backed, and it will store refs
to all such outstanding video frames (keyed by this generated |release_token|).

  - The renderer handles the video frame as usual, flowing through the regular
pipeline and compositing steps. Once compositing is done, the compositor sets
the |release_sync_token| based on its own command buffer. The frame is then
destructed (as usual).

  - MojoVideoDecoder receives the destruction callback. At this point the
|release_sync_token| has been verified as part of hopping to the media thread
and command buffer. It now forwards the sync point to the GPU process via the
OnReleaseMailbox() Mojo IPC.

  - MojoVideoDecoderService (on the GPU side) will check that the sync point is
valid, terminating the Mojo connection if it is not. It then looks up, updates,
and removes the video frame from its map. If there is an attached destruction
observer, it will receive the |release_sync_point|, and the decoder will know
that the textures can be re-used after waiting for that.

Note that if the media command buffer and compositor command buffer are ever
merged (again), the validation step won't happen as part of the hopping anymore,
and validation on the GPU side could fail (timing dependent). We would need to
switch to sending the OnReleaseMailbox() IPC via the GPU channel instead of
Mojo in that case (or take the performance hit of validating the sync point from
the Renderer explicitly).

BUG=522298
==========

[sandersd (OOO until July 31), 2017-02-25 01:19:38]

> Since you mention this interface is not live yet, it should really be
converted
> to use struct traits instead of using type mappers.

FYI: the new VideoDecoder interface is not, but the EME interfaces using
MojoVideoFrame are live.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_video_decoder.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/clients/mojo...
media/mojo/clients/mojo_video_decoder.cc:112: void
MojoVideoDecoder::OnVideoFrameDecoded(
On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> Can this be called on multiple threads?

No, it is a Mojo IPC callback and so should only be called on the bound thread.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:326:
TypeConverter<media::mojom::VideoFramePtr, scoped_refptr<media::VideoFrame>>::
On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> In general, please avoid using type converters and move them over to struct
> traits.

Yes, this in a slowly ongoing conversion for the media mojo code.

I'm doubtful that VideoFrame in particular will ever be a StructTrait though;
some things can legitimately only be exposed via its constructor, and
StructTraits don't give us the power to control that.

VideoFrame is more likely to move to a fully custom converter, so that we can
pass additional arguments to this method.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:375:
CHECK(input->data->is_shared_buffer_data());
On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> You should not be using CHECK in here, as the data can be arbitrary and
hostile.
> 

Done.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.cc:136:
client_->OnVideoFrameDecoded(mojom::VideoFrame::From(frame), base::nullopt);
On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> You probably want base/optional.h included here instead of the header file.

Done.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
File media/mojo/services/mojo_video_decoder_service.h (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/services/moj...
media/mojo/services/mojo_video_decoder_service.h:12: #include "base/optional.h"
On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> Where is base::optional used here? Do we really need this include?

Done.

[nasko, 2017-03-01 19:48:47]

Overall LGTM, but please share the bug which tracks converting the code to using
StructTrait and ensure it is being done, not deprioritized.

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:326:
TypeConverter<media::mojom::VideoFramePtr, scoped_refptr<media::VideoFrame>>::
On 2017/02/25 01:19:38, sandersd wrote:
> On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> > In general, please avoid using type converters and move them over to struct
> > traits.
> 
> Yes, this in a slowly ongoing conversion for the media mojo code.

Is there a bug tracking that?

> I'm doubtful that VideoFrame in particular will ever be a StructTrait though;
> some things can legitimately only be exposed via its constructor, and
> StructTraits don't give us the power to control that.

I'm curious why is that? It is unrelated to this CL itself, but constructor with
params and parameterless constructor with a bunch of setters or init method
should be identical.

> VideoFrame is more likely to move to a fully custom converter, so that we can
> pass additional arguments to this method.

I'll defer to dcheng@ and tsepez@ on this, but it seems undesirable direction
overall.

[sandersd (OOO until July 31), 2017-03-01 20:08:11]

On 2017/03/01 19:48:47, nasko (slow) wrote:
> Overall LGTM, but please share the bug which tracks converting the code to
using
> StructTrait and ensure it is being done, not deprioritized.
> 
>
https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
> File media/mojo/common/media_type_converters.cc (right):
> 
>
https://codereview.chromium.org/2640153004/diff/60001/media/mojo/common/media...
> media/mojo/common/media_type_converters.cc:326:
> TypeConverter<media::mojom::VideoFramePtr, scoped_refptr<media::VideoFrame>>::
> On 2017/02/25 01:19:38, sandersd wrote:
> > On 2017/02/16 21:37:29, nasko (out until Feb 27th) wrote:
> > > In general, please avoid using type converters and move them over to
struct
> > > traits.
> > 
> > Yes, this in a slowly ongoing conversion for the media mojo code.
> 
> Is there a bug tracking that?

https://bugs.chromium.org/p/chromium/issues/detail?id=611224

> > I'm doubtful that VideoFrame in particular will ever be a StructTrait
though;
> > some things can legitimately only be exposed via its constructor, and
> > StructTraits don't give us the power to control that.
> 
> I'm curious why is that? It is unrelated to this CL itself, but constructor
with
> params and parameterless constructor with a bunch of setters or init method
> should be identical.

Since VideoFrames are ref-counted, cross-thread objects, the choice is to lock
everything or restrict some state to be set only in the constructor. VideoFrame
sets a heck of a lot of state in the constructor and that's not going to change
soon.

> > VideoFrame is more likely to move to a fully custom converter, so that we
can
> > pass additional arguments to this method.
> 
> I'll defer to dcheng@ and tsepez@ on this, but it seems undesirable direction
> overall.

That's probably true, but on the plus side //media has specifically chosen not
to depend on Mojo types. As a result, the conversions happen at a small number
of clear boundaries.

Separately, I designed the Mojo version of the VideoFrame type quite differently
from the C++ implementation (the Mojo version is a composed tagged union, vs.
the real thing being an uber class). The current direction of the C++ code seems
to be towards subclassing, and StructTraits do not seem to have any provision
for that.

Combining these two things together, an explicit conversion / factory function
makes the most sense, at least for now.

[sandersd (OOO until July 31), 2017-03-02 02:07:05]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-03-02 02:07:05]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2640153004/#ps80001
(title: "")

[commit-bot: I haz the power, 2017-03-02 02:07:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 05:57:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-02 05:57:47]

Failed to apply patch for media/mojo/services/mojo_video_decoder_service.h:
While running git apply --index -p1;
  error: patch failed: media/mojo/services/mojo_video_decoder_service.h:12
  error: media/mojo/services/mojo_video_decoder_service.h: patch does not apply

Patch:       media/mojo/services/mojo_video_decoder_service.h
Index: media/mojo/services/mojo_video_decoder_service.h
diff --git a/media/mojo/services/mojo_video_decoder_service.h
b/media/mojo/services/mojo_video_decoder_service.h
index
ce824eee56c76b9293fb7244705deb671068eb4f..4eec46dbe0f81c02e0182b2ca7f36528eaced40b
100644
--- a/media/mojo/services/mojo_video_decoder_service.h
+++ b/media/mojo/services/mojo_video_decoder_service.h
@@ -12,6 +12,10 @@
 #include "media/base/decode_status.h"
 #include "media/mojo/interfaces/video_decoder.mojom.h"
 
+namespace gpu {
+struct SyncToken;
+};
+
 namespace media {
 
 class MojoDecoderBufferReader;
@@ -36,6 +40,8 @@ class MojoVideoDecoderService : public mojom::VideoDecoder {
   void Decode(mojom::DecoderBufferPtr buffer,
               const DecodeCallback& callback) final;
   void Reset(const ResetCallback& callback) final;
+  void OnReleaseMailbox(const base::UnguessableToken& release_token,
+                        const gpu::SyncToken& release_sync_token) final;
 
  private:
   // Helper methods so that we can bind them with a weak pointer to avoid

[sandersd (OOO until July 31), 2017-03-02 21:37:15]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-03-02 21:37:16]

The patchset sent to the CQ was uploaded after l-g-t-m from xhwang@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2640153004/#ps100001
(title: "Rebase.")

[commit-bot: I haz the power, 2017-03-02 21:37:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-02 23:21:53]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1488490635932350,
"parent_rev": "d79e11f6225265fcf1d5884b969796a0377dee8a", "commit_rev":
"ef768ffc5dbe5364af1db5c04da524109b4b5f08"}

[commit-bot: I haz the power, 2017-03-02 23:22:30]

Description was changed from

==========
Add mailbox-based Mojo VideoFrame variant.

This CL creates a foundation for plumbing mailbox-based Mojo VideoFrames, but
does not fully implement all of the callbacks. The expected implementation (in
MojoVideoDecoderService) is:

  - Once a decoder running in the GPU process has decoded a frame,
MojoVideoDecoderService can attach a |release_token| to the
OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
generate one whenever the video frame is texture-backed, and it will store refs
to all such outstanding video frames (keyed by this generated |release_token|).

  - The renderer handles the video frame as usual, flowing through the regular
pipeline and compositing steps. Once compositing is done, the compositor sets
the |release_sync_token| based on its own command buffer. The frame is then
destructed (as usual).

  - MojoVideoDecoder receives the destruction callback. At this point the
|release_sync_token| has been verified as part of hopping to the media thread
and command buffer. It now forwards the sync point to the GPU process via the
OnReleaseMailbox() Mojo IPC.

  - MojoVideoDecoderService (on the GPU side) will check that the sync point is
valid, terminating the Mojo connection if it is not. It then looks up, updates,
and removes the video frame from its map. If there is an attached destruction
observer, it will receive the |release_sync_point|, and the decoder will know
that the textures can be re-used after waiting for that.

Note that if the media command buffer and compositor command buffer are ever
merged (again), the validation step won't happen as part of the hopping anymore,
and validation on the GPU side could fail (timing dependent). We would need to
switch to sending the OnReleaseMailbox() IPC via the GPU channel instead of
Mojo in that case (or take the performance hit of validating the sync point from
the Renderer explicitly).

BUG=522298
==========

to

==========
Add mailbox-based Mojo VideoFrame variant.

This CL creates a foundation for plumbing mailbox-based Mojo VideoFrames, but
does not fully implement all of the callbacks. The expected implementation (in
MojoVideoDecoderService) is:

  - Once a decoder running in the GPU process has decoded a frame,
MojoVideoDecoderService can attach a |release_token| to the
OnVideoFrameDecoded() IPC along with the frame. In practice, the impl will
generate one whenever the video frame is texture-backed, and it will store refs
to all such outstanding video frames (keyed by this generated |release_token|).

  - The renderer handles the video frame as usual, flowing through the regular
pipeline and compositing steps. Once compositing is done, the compositor sets
the |release_sync_token| based on its own command buffer. The frame is then
destructed (as usual).

  - MojoVideoDecoder receives the destruction callback. At this point the
|release_sync_token| has been verified as part of hopping to the media thread
and command buffer. It now forwards the sync point to the GPU process via the
OnReleaseMailbox() Mojo IPC.

  - MojoVideoDecoderService (on the GPU side) will check that the sync point is
valid, terminating the Mojo connection if it is not. It then looks up, updates,
and removes the video frame from its map. If there is an attached destruction
observer, it will receive the |release_sync_point|, and the decoder will know
that the textures can be re-used after waiting for that.

Note that if the media command buffer and compositor command buffer are ever
merged (again), the validation step won't happen as part of the hopping anymore,
and validation on the GPU side could fail (timing dependent). We would need to
switch to sending the OnReleaseMailbox() IPC via the GPU channel instead of
Mojo in that case (or take the performance hit of validating the sync point from
the Renderer explicitly).

BUG=522298

Review-Url: https://codereview.chromium.org/2640153004
Cr-Commit-Position: refs/heads/master@{#454427}
Committed:
https://chromium.googlesource.com/chromium/src/+/ef768ffc5dbe5364af1db5c04da5...
==========

[commit-bot: I haz the power, 2017-03-02 23:22:31]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/ef768ffc5dbe5364af1db5c04da5...