Transfer media::VideoFrame using mojo shared memory

Transfer media::VideoFrame using mojo shared memory

Now that MojoSharedBufferVideoFrame exists and uses mojo shared memory,
pass the shared memory handle when passing using mojo. TypeConverter
for this interface updated to handle the shared memory.

BUG=510088
TEST=update media_mojo_unittests pass

Committed: https://crrev.com/5e18b5d78b3bf914052759d5ab3bd39778f0bceb
Cr-Commit-Position: refs/heads/master@{#380200}

[jrummell, 2016-03-04 22:25:19]

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

[jrummell, 2016-03-04 22:25:19]

PTAL.

[xhwang, 2016-03-07 19:29:20]

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

[xhwang, 2016-03-07 19:29:21]

Mostly looking good. I do have one question about the failure handling in
TypeConverter.

rockot: Please see my question about what's the convention to handle
TypeConverter failures. Should the caller check the return value of
Foo::From(foo), or should this be transparent to the mojo transport layer and
have the other side handling it?

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
File media/mojo/common/media_type_converters.cc (right):

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:600: media::interfaces::VideoFramePtr
buffer(media::interfaces::VideoFrame::New());
I don't know why we call it a |buffer|. Could you please s/buffer/frame?

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:606: // Handle non EOS buffer. It
must be a MojoVideoFrame.
Update MojoVideoFrame to the new name.

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:608:
media::VideoFrame::STORAGE_MOJO_SHARED_BUFFER);
Add a TODO to support other types.

We use DCEHCK_EQ(expected_value, real_value)

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:610: return buffer;
This will be treated as a EOS frame, which is fine. Can we add a comment to make
this clear?

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:618: return buffer;
hmm, this failure will be translated to EOS again, which sounds bad. Since this
conversion can really fail for legit reasons, does it make sense to let it
return null? This would require the caller to check the result of
media::interfaces::VideoFrame::From(frame)?

+rockot for comments since I don't usually see callers checking Foo::From(foo).

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:633: buffer->v_offset =
frame->PlaneOffset(media::VideoFrame::kVPlane);
Not related to this CL: I wish we could make this (and similar code around
VideoFrame) more eye-friendly :)

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
File media/mojo/common/media_type_converters_unittest.cc (right):

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters_unittest.cc:377:
scoped_refptr<VideoFrame> buffer(
nit: s/buffer/video_frame

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters_unittest.cc:391: // Create a color
VideoFrame to use as reference (as conversion requires
Could you please wrap this (line 391 to 430) into a helper function? Something
like CreateMojoSharedBufferColorFrame()?

[Ken Rockot(use gerrit already), 2016-03-07 19:36:29]

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
File media/mojo/common/media_type_converters.cc (right):

https://chromiumcodereview.appspot.com/1769673002/diff/1/media/mojo/common/me...
media/mojo/common/media_type_converters.cc:618: return buffer;
On 2016/03/07 at 19:29:21, xhwang wrote:
> hmm, this failure will be translated to EOS again, which sounds bad. Since
this conversion can really fail for legit reasons, does it make sense to let it
return null? This would require the caller to check the result of
media::interfaces::VideoFrame::From(frame)?
> 
> +rockot for comments since I don't usually see callers checking
Foo::From(foo).

I think the right thing to do in this case is to just CHECK. If a user isn't
aware that this can be null, and they end up passing a null ptr in a
non-nullable VideoFramePtr field, the bindings will CHECK anyway.

Failure to duplicate a buffer handle is effectively equivalent to running out of
file descriptors (or I guess the frame having an invalid handle to begin with).
Seems like something that should be a fatal error.

[jrummell, 2016-03-07 21:07:33]

Updated.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
File media/mojo/common/media_type_converters.cc (right):

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:600: media::interfaces::VideoFramePtr
buffer(media::interfaces::VideoFrame::New());
On 2016/03/07 19:29:21, xhwang wrote:
> I don't know why we call it a |buffer|. Could you please s/buffer/frame?

Done.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:606: // Handle non EOS buffer. It
must be a MojoVideoFrame.
On 2016/03/07 19:29:21, xhwang wrote:
> Update MojoVideoFrame to the new name.

Done.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:608:
media::VideoFrame::STORAGE_MOJO_SHARED_BUFFER);
On 2016/03/07 19:29:20, xhwang wrote:
> Add a TODO to support other types.
> 
> We use DCEHCK_EQ(expected_value, real_value)

Done.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:610: return buffer;
On 2016/03/07 19:29:21, xhwang wrote:
> This will be treated as a EOS frame, which is fine. Can we add a comment to
make
> this clear?

Done.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:618: return buffer;
On 2016/03/07 19:36:29, Ken Rockot wrote:
> On 2016/03/07 at 19:29:21, xhwang wrote:
> > hmm, this failure will be translated to EOS again, which sounds bad. Since
> this conversion can really fail for legit reasons, does it make sense to let
it
> return null? This would require the caller to check the result of
> media::interfaces::VideoFrame::From(frame)?
> > 
> > +rockot for comments since I don't usually see callers checking
> Foo::From(foo).
> 
> I think the right thing to do in this case is to just CHECK. If a user isn't
> aware that this can be null, and they end up passing a null ptr in a
> non-nullable VideoFramePtr field, the bindings will CHECK anyway.
> 
> Failure to duplicate a buffer handle is effectively equivalent to running out
of
> file descriptors (or I guess the frame having an invalid handle to begin
with).
> Seems like something that should be a fatal error.

Added CHECK()

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters.cc:633: buffer->v_offset =
frame->PlaneOffset(media::VideoFrame::kVPlane);
On 2016/03/07 19:29:21, xhwang wrote:
> Not related to this CL: I wish we could make this (and similar code around
> VideoFrame) more eye-friendly :)

Acknowledged.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
File media/mojo/common/media_type_converters_unittest.cc (right):

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters_unittest.cc:377:
scoped_refptr<VideoFrame> buffer(
On 2016/03/07 19:29:21, xhwang wrote:
> nit: s/buffer/video_frame

Done.

https://codereview.chromium.org/1769673002/diff/1/media/mojo/common/media_typ...
media/mojo/common/media_type_converters_unittest.cc:391: // Create a color
VideoFrame to use as reference (as conversion requires
On 2016/03/07 19:29:21, xhwang wrote:
> Could you please wrap this (line 391 to 430) into a helper function? Something
> like CreateMojoSharedBufferColorFrame()?

Done.

[xhwang, 2016-03-07 21:33:15]

Looking good. Just two extra thoughts.

https://chromiumcodereview.appspot.com/1769673002/diff/20001/media/mojo/commo...
File media/mojo/common/media_type_converters.cc (right):

https://chromiumcodereview.appspot.com/1769673002/diff/20001/media/mojo/commo...
media/mojo/common/media_type_converters.cc:610: input->storage_type());
EOS doesn't really make sense. Maybe just CHECK here to be consistent with the
code below?

https://chromiumcodereview.appspot.com/1769673002/diff/20001/media/mojo/commo...
File media/mojo/common/media_type_converters_unittest.cc (right):

https://chromiumcodereview.appspot.com/1769673002/diff/20001/media/mojo/commo...
media/mojo/common/media_type_converters_unittest.cc:156:
color_frame->data(VideoFrame::kYPlane), allocation_size);
This copy is based on the fact that we know CreateColorFrame() allocates a
continuous memory. But since we have frame->data() for each plane, this is not
necessarily true based on the public VideoFrame API we have.

Maybe we should DCHECK that the |allocation_size| is larger than the data size
in |color_frame|, and copy each plane separately?

[jrummell, 2016-03-09 00:41:50]

Updated.

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

https://codereview.chromium.org/1769673002/diff/20001/media/mojo/common/media...
media/mojo/common/media_type_converters.cc:610: input->storage_type());
On 2016/03/07 21:33:15, xhwang wrote:
> EOS doesn't really make sense. Maybe just CHECK here to be consistent with the
> code below?

Done.

https://codereview.chromium.org/1769673002/diff/20001/media/mojo/common/media...
File media/mojo/common/media_type_converters_unittest.cc (right):

https://codereview.chromium.org/1769673002/diff/20001/media/mojo/common/media...
media/mojo/common/media_type_converters_unittest.cc:156:
color_frame->data(VideoFrame::kYPlane), allocation_size);
On 2016/03/07 21:33:15, xhwang wrote:
> This copy is based on the fact that we know CreateColorFrame() allocates a
> continuous memory. But since we have frame->data() for each plane, this is not
> necessarily true based on the public VideoFrame API we have.
> 
> Maybe we should DCHECK that the |allocation_size| is larger than the data size
> in |color_frame|, and copy each plane separately?

Copied each plane separately. Added checks to make sure the size matches.

[xhwang, 2016-03-09 17:18:06]

lgtm

[jrummell, 2016-03-09 19:13:00]

The CQ bit was checked by jrummell@chromium.org

[commit-bot: I haz the power, 2016-03-09 19:13:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769673002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769673002/40001

[commit-bot: I haz the power, 2016-03-09 19:16:24]

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

[commit-bot: I haz the power, 2016-03-09 19:16:25]

Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jrummell, 2016-03-09 19:45:34]

The CQ bit was checked by jrummell@chromium.org

[jrummell, 2016-03-09 19:45:34]

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/1769673002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-09 19:46:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1769673002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1769673002/60001

[commit-bot: I haz the power, 2016-03-09 20:24:47]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-09 20:25:46]

Description was changed from

==========
Transfer media::VideoFrame using mojo shared memory

Now that MojoSharedBufferVideoFrame exists and uses mojo shared memory,
pass the shared memory handle when passing using mojo. TypeConverter
for this interface updated to handle the shared memory.

BUG=510088
TEST=update media_mojo_unittests pass
==========

to

==========
Transfer media::VideoFrame using mojo shared memory

Now that MojoSharedBufferVideoFrame exists and uses mojo shared memory,
pass the shared memory handle when passing using mojo. TypeConverter
for this interface updated to handle the shared memory.

BUG=510088
TEST=update media_mojo_unittests pass

Committed: https://crrev.com/5e18b5d78b3bf914052759d5ab3bd39778f0bceb
Cr-Commit-Position: refs/heads/master@{#380200}
==========

[commit-bot: I haz the power, 2016-03-09 20:25:47]

Patchset 4 (id:??) landed as
https://crrev.com/5e18b5d78b3bf914052759d5ab3bd39778f0bceb
Cr-Commit-Position: refs/heads/master@{#380200}

[tsergeant, 2016-03-10 00:48:16]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1781923002/ by tsergeant@chromium.org.

The reason for reverting is: media_type_converters.cc is failing to compile on
Win X64 GN.

See:
https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN/builds/11580
https://build.chromium.org/p/chromium.win/builders/Win%20x64%20GN%20%28dbg%29....