media: Create Mojo StructTraits for VideoFrame

media: Create Mojo StructTraits for VideoFrame

This replaces the existing TypeConverter code for VideoFrames.

BUG=611224
Review-Url: https://codereview.chromium.org/2908303003
Cr-Commit-Position: refs/heads/master@{#478413}
Committed: https://chromium.googlesource.com/chromium/src/+/5ad3de8f2d39653bdabf40403b1dc48ae92c8320

[sandersd (OOO until July 31), 2017-06-03 00:08:44]

Description was changed from

==========
StructTraits for VideoFrame
==========

to

==========
media: Create Mojo StructTraits for VideoFrame

This replaces the existing TypeConverter code for VideoFrames.

BUG=611224
==========

[sandersd (OOO until July 31), 2017-06-03 00:11:12]

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

[sandersd (OOO until July 31), 2017-06-03 00:11:12]

Requesting early review (unittests are missing, TypeConverter is still in the
codebase).

Looking for feedback right now on the implementations of video_frame.typemap and
video_frame_struct_traits.h.

I'm doing something wrong, because when I tried to move the implementations of
data() and Read() into a .cc, I got template expansion errors at the call sites.

[dcheng, 2017-06-05 20:33:02]

(I'm assuming this supersedes the other CL)

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.cc:227: const scoped_refptr<VideoFrame>&
video_frame,
Nit: Consider passing by value and using std::move() to transfer the refcount
around.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/common/mojo_...
File media/mojo/common/mojo_shared_buffer_video_frame.h (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/common/mojo_...
media/mojo/common/mojo_shared_buffer_video_frame.h:76: // handle. Caller should
call duplicate the handle if they want to keep a
Nit: delete "call"

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:250: // Size must be kept in sync with
media::VideoFrame::kMaxPlanes.
Ideally, we'd just define the constant in mojom, but I'm guessing we don't (or
can't) currently depend on mojo in that file.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
I think we'll need to use a context object for this. Otherwise, we create a
shared buffer twice =/

See
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:98:
shared_buffer_data.frame_data_size(), shared_buffer_data.y_offset(),
Do we need to do any basic range checking here?

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:120: coded_size,
visible_rect, natural_size, timestamp);
Similar question about range checks here--anything interesting needed here?

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.h (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:33: static gfx::Size
coded_size(const scoped_refptr<media::VideoFrame>& input) {
Nit: return non-trivial types by const ref: due to https://crbug.com/681080, we
currently call each serialization getter twice.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:46: return
input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM);
I'm assuming this is a cheap calculation (otherwise, we run into the
aforementioned serialization issue)

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:49: static base::TimeDelta
timestamp(
But returning base::Time* by value is fine.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:55: const
scoped_refptr<media::VideoFrame>& input) {
Hmm, isn't this in the .cc as well? I'm assuming this is duplicated by accident
and reviewing what's in .cc.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:95: static bool
Read(media::mojom::VideoFrameDataView input,
Ditto.

[sandersd (OOO until July 31), 2017-06-05 20:46:14]

(Replies to just a few comments, since some of the answers may be relevant
before my next PS.)

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.cc:227: const scoped_refptr<VideoFrame>&
video_frame,
On 2017/06/05 20:33:01, dcheng wrote:
> Nit: Consider passing by value and using std::move() to transfer the refcount
> around.

This would be the ideal setup, but I don't see a combination of attributes that
results in the intended signatures in the generated mojo bindings.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/m...
File media/mojo/interfaces/media_types.mojom (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/m...
media/mojo/interfaces/media_types.mojom:250: // Size must be kept in sync with
media::VideoFrame::kMaxPlanes.
On 2017/06/05 20:33:01, dcheng wrote:
> Ideally, we'd just define the constant in mojom, but I'm guessing we don't (or
> can't) currently depend on mojo in that file.

That's correct; //media is not allowed to depend on //media/mojo.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/05 20:33:01, dcheng wrote:
> I think we'll need to use a context object for this. Otherwise, we create a
> shared buffer twice =/
> 
> See
>
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...

Ack :-(

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.h (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:46: return
input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM);
On 2017/06/05 20:33:01, dcheng wrote:
> I'm assuming this is a cheap calculation (otherwise, we run into the
> aforementioned serialization issue)

This one should be fine.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:55: const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/05 20:33:02, dcheng wrote:
> Hmm, isn't this in the .cc as well? I'm assuming this is duplicated by
accident
> and reviewing what's in .cc.

Yes, it is, but they should have identical content. The version in this .h is
the one currently being used, I have not been able to get it to compile with the
.cc version yet. I'll follow up later today with my situation there to get
specific advice.

[sandersd (OOO until July 31), 2017-06-06 01:05:47]

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/common/mojo_...
File media/mojo/common/mojo_shared_buffer_video_frame.h (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/common/mojo_...
media/mojo/common/mojo_shared_buffer_video_frame.h:76: // handle. Caller should
call duplicate the handle if they want to keep a
On 2017/06/05 20:33:01, dcheng wrote:
> Nit: delete "call"

Done.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/05 20:33:01, dcheng wrote:
> I think we'll need to use a context object for this. Otherwise, we create a
> shared buffer twice =/
> 
> See
>
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...

I was unable to make this work because a SharedBufferHandle is not copyable.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:98:
shared_buffer_data.frame_data_size(), shared_buffer_data.y_offset(),
On 2017/06/05 20:33:01, dcheng wrote:
> Do we need to do any basic range checking here?

This should be checked by media::MojoSharedBufferVideoFrame::Create(), and the
existing TypeConverter relies on that for its safety.

It turns out the media::MojoSharedBufferVideoFrame::Create() was not actually
checking this.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:120: coded_size,
visible_rect, natural_size, timestamp);
On 2017/06/05 20:33:01, dcheng wrote:
> Similar question about range checks here--anything interesting needed here?

This one is fine. I added a check for visible_rect < coded_size regardless.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.h (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:33: static gfx::Size
coded_size(const scoped_refptr<media::VideoFrame>& input) {
On 2017/06/05 20:33:02, dcheng wrote:
> Nit: return non-trivial types by const ref: due to https://crbug.com/681080,
we
> currently call each serialization getter twice.

This is the first time I've heard gfx::Size referred to as "non-trivial", but
done.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:49: static base::TimeDelta
timestamp(
On 2017/06/05 20:33:01, dcheng wrote:
> But returning base::Time* by value is fine.

Acknowledged.

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.h:95: static bool
Read(media::mojom::VideoFrameDataView input,
On 2017/06/05 20:33:02, dcheng wrote:
> Ditto.

Acknowledged.

[sandersd (OOO until July 31), 2017-06-06 01:06:39]

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

[sandersd (OOO until July 31), 2017-06-06 01:07:01]

+xhwang@ to review changes to mojo_shared_buffer_video_frame.cc.

[xhwang, 2017-06-06 05:51:15]

looking good to me!

Do you need to remove the type converter code for VideoFrame?

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.h (right):

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.h:66: const scoped_refptr<VideoFrame>&
video_frame,
The current style guide discourages using const-ref for scoped_refptr. Just pass
by value and use std::move where needed?

[sandersd (OOO until July 31), 2017-06-06 18:07:37]

> Do you need to remove the type converter code for VideoFrame?

Yes, that will be removed and the test cases will be converted.

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.h (right):

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.h:66: const scoped_refptr<VideoFrame>&
video_frame,
On 2017/06/06 05:51:15, xhwang wrote:
> The current style guide discourages using const-ref for scoped_refptr. Just
pass
> by value and use std::move where needed?

Yes, but this callback is used as a Decryptor::DecryptAndDecodeVideoCallback,
which after the StructTraits change looks like this:

  using DecryptAndDecodeVideoCallback =
base::Callback<void(media::Decryptor::Status, const
scoped_refptr<media::VideoFrame>&, FrameResourceReleaserPtr)>;

There isn't really much choice here.

[xhwang, 2017-06-06 18:12:47]

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.h (right):

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.h:66: const scoped_refptr<VideoFrame>&
video_frame,
On 2017/06/06 18:07:37, sandersd wrote:
> On 2017/06/06 05:51:15, xhwang wrote:
> > The current style guide discourages using const-ref for scoped_refptr. Just
> pass
> > by value and use std::move where needed?
> 
> Yes, but this callback is used as a Decryptor::DecryptAndDecodeVideoCallback,
> which after the StructTraits change looks like this:
> 
>   using DecryptAndDecodeVideoCallback =
> base::Callback<void(media::Decryptor::Status, const
> scoped_refptr<media::VideoFrame>&, FrameResourceReleaserPtr)>;
> 
> There isn't really much choice here.

I see. So it's more like a binding generation issue. Nevermind then.

[sandersd (OOO until July 31), 2017-06-07 00:36:53]

I believe this CL is complete now. I'm not entirely sure that the includes and
deps are correct, but it works when building chrome and media_unittests at
least.

The death test is extremely noisy, to the point where it may be better to remove
it. The stack trace it prints could be confusing to others debugging it later.

[sandersd (OOO until July 31), 2017-06-07 00:37:08]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 00:37:29]

Dry run: 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-06-07 01:14:33]

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

[commit-bot: I haz the power, 2017-06-07 01:14:34]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[sandersd (OOO until July 31), 2017-06-07 20:17:08]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 20:17:34]

Dry run: 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-06-07 21:13:03]

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

[commit-bot: I haz the power, 2017-06-07 21:13:04]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sandersd (OOO until July 31), 2017-06-07 21:16:10]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 21:18:29]

Dry run: 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-06-07 21:36:34]

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

[commit-bot: I haz the power, 2017-06-07 21:36:35]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sandersd (OOO until July 31), 2017-06-07 22:26:51]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 22:27:35]

Dry run: 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-06-07 23:07:57]

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

[commit-bot: I haz the power, 2017-06-07 23:07:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sandersd (OOO until July 31), 2017-06-07 23:14:10]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-07 23:14:56]

Dry run: 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-06-08 00:29:28]

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

[commit-bot: I haz the power, 2017-06-08 00:29:32]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[dcheng, 2017-06-08 09:41:07]

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

[dcheng, 2017-06-08 09:41:08]

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/06 01:05:47, sandersd wrote:
> On 2017/06/05 20:33:01, dcheng wrote:
> > I think we'll need to use a context object for this. Otherwise, we create a
> > shared buffer twice =/
> > 
> > See
> >
>
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...
> 
> I was unable to make this work because a SharedBufferHandle is not copyable.

Hmm... so I guess this is tricky. Out of curiosity, what happens if you return a
const media::mojom::VideoFrameDataPtr&? Does Mojo handle this correctly?

+yzshen for any additional insight in the best way to handle this situation.

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
File media/mojo/clients/mojo_decryptor.h (right):

https://codereview.chromium.org/2908303003/diff/50001/media/mojo/clients/mojo...
media/mojo/clients/mojo_decryptor.h:66: const scoped_refptr<VideoFrame>&
video_frame,
On 2017/06/06 18:12:47, xhwang wrote:
> On 2017/06/06 18:07:37, sandersd wrote:
> > On 2017/06/06 05:51:15, xhwang wrote:
> > > The current style guide discourages using const-ref for scoped_refptr.
Just
> > pass
> > > by value and use std::move where needed?
> > 
> > Yes, but this callback is used as a
Decryptor::DecryptAndDecodeVideoCallback,
> > which after the StructTraits change looks like this:
> > 
> >   using DecryptAndDecodeVideoCallback =
> > base::Callback<void(media::Decryptor::Status, const
> > scoped_refptr<media::VideoFrame>&, FrameResourceReleaserPtr)>;
> > 
> > There isn't really much choice here.
> 
> I see. So it's more like a binding generation issue. Nevermind then.

It is possible to annotate the type with move_only so you'll get move semantics:
https://cs.chromium.org/chromium/src/mojo/common/file.typemap?rcl=119d0e85eb5...

Though I think it's probably not necessary here (and it's not really even true,
since scoped_refptr is not move_only =)

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
File media/mojo/common/mojo_shared_buffer_video_frame.cc (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
media/mojo/common/mojo_shared_buffer_video_frame.cc:117:
y_bound.ValueOrDefault(0) > data_size ||
Nit: use ValueOrDie() here (since we just checked IsValid() above)

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
File media/mojo/common/mojo_shared_buffer_video_frame.h (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
media/mojo/common/mojo_shared_buffer_video_frame.h:77: // the shared memory.
Nit: the fact that this returns a const reference is a hint that no ownership
transfer happened. I would reword the last part to say something about "Callers
must duplicate the handle if they to extend the lifetime of the shared memory",
since we're not really copying it

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:133: // TODO(sandersd):
Switch on the union tag to avoid this ugliness?
Hmm... this is OK for now, but maybe we can try using UnionTraits for
VideoFrameData in a followup? I guess its's trickier though, since it's not
actually a union on the C++ side.

[yzshen1, 2017-06-08 17:45:14]

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/08 09:41:08, dcheng wrote:
> On 2017/06/06 01:05:47, sandersd wrote:
> > On 2017/06/05 20:33:01, dcheng wrote:
> > > I think we'll need to use a context object for this. Otherwise, we create
a
> > > shared buffer twice =/
> > > 
> > > See
> > >
> >
>
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...
> > 
> > I was unable to make this work because a SharedBufferHandle is not copyable.
> 
> Hmm... so I guess this is tricky. Out of curiosity, what happens if you return
a
> const media::mojom::VideoFrameDataPtr&? Does Mojo handle this correctly?
> 
> +yzshen for any additional insight in the best way to handle this situation.

Daniel is right that we should probably put this in a context object.

You could return a media::mojom::VideoFrameDataPtr& (i.e., a non-const
reference), it will be accessed twice, the first time is used to calculate the
size and unchanged; the second time will take the contents.

[sandersd (OOO until July 31), 2017-06-08 20:10:09]

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:16: data(const
scoped_refptr<media::VideoFrame>& input) {
On 2017/06/08 17:45:14, yzshen1 wrote:
> On 2017/06/08 09:41:08, dcheng wrote:
> > On 2017/06/06 01:05:47, sandersd wrote:
> > > On 2017/06/05 20:33:01, dcheng wrote:
> > > > I think we'll need to use a context object for this. Otherwise, we
create
> a
> > > > shared buffer twice =/
> > > > 
> > > > See
> > > >
> > >
> >
>
https://chromium.googlesource.com/chromium/src/mojo/+/master/public/cpp/bindi...
> > > 
> > > I was unable to make this work because a SharedBufferHandle is not
copyable.
> > 
> > Hmm... so I guess this is tricky. Out of curiosity, what happens if you
return
> a
> > const media::mojom::VideoFrameDataPtr&? Does Mojo handle this correctly?
> > 
> > +yzshen for any additional insight in the best way to handle this situation.
> 
> Daniel is right that we should probably put this in a context object.
> 
> You could return a media::mojom::VideoFrameDataPtr& (i.e., a non-const
> reference), it will be accessed twice, the first time is used to calculate the
> size and unchanged; the second time will take the contents.

Done.

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
File media/mojo/common/mojo_shared_buffer_video_frame.cc (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
media/mojo/common/mojo_shared_buffer_video_frame.cc:117:
y_bound.ValueOrDefault(0) > data_size ||
On 2017/06/08 09:41:08, dcheng wrote:
> Nit: use ValueOrDie() here (since we just checked IsValid() above)

Done.

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
File media/mojo/common/mojo_shared_buffer_video_frame.h (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/common/mojo...
media/mojo/common/mojo_shared_buffer_video_frame.h:77: // the shared memory.
On 2017/06/08 09:41:08, dcheng wrote:
> Nit: the fact that this returns a const reference is a hint that no ownership
> transfer happened. I would reword the last part to say something about
"Callers
> must duplicate the handle if they to extend the lifetime of the shared
memory",
> since we're not really copying it

Done.

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/190001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:133: // TODO(sandersd):
Switch on the union tag to avoid this ugliness?
On 2017/06/08 09:41:08, dcheng wrote:
> Hmm... this is OK for now, but maybe we can try using UnionTraits for
> VideoFrameData in a followup? I guess its's trickier though, since it's not
> actually a union on the C++ side.

Lack of a type on the C++ side is the main problem here. I expect that
VideoFrame will evolve to have a subclass for each case (with
MojoSharedBufferVideoFrame being the first example), but that will actually make
the problem worse.

[sandersd (OOO until July 31), 2017-06-08 20:10:24]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 20:11:04]

Dry run: CQ is trying da patch.

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

[dcheng, 2017-06-08 20:43:15]

LGTM with comments addressed.

https://codereview.chromium.org/2908303003/diff/210001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/210001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:86: if (!ctx->has_data) {
Rather than using has_data, just call MakeVideoFrameData in SetUpContext as
well.

[sandersd (OOO until July 31), 2017-06-08 21:04:19]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-08 21:05:02]

Dry run: CQ is trying da patch.

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

[sandersd (OOO until July 31), 2017-06-08 21:56:36]

xhwang@: Ping.

[commit-bot: I haz the power, 2017-06-08 23:36:14]

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

[commit-bot: I haz the power, 2017-06-08 23:36:15]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[sandersd (OOO until July 31), 2017-06-08 23:39:56]

dcheng@: Any explanation why the death test fails on Windows?

[dcheng, 2017-06-09 06:39:30]

On 2017/06/08 23:39:56, sandersd wrote:
> dcheng@: Any explanation why the death test fails on Windows?

Hmm... sorry I missed this in the review. I wouldn't expect a mojo
deserialization failure to result in process death: if anything, it'd call
mojo::ReportBadMessage().

Is it the DCHECK() that's killing the other death tests? We should probably take
the DCHECKs out, as these are legitimate things that a bad process can send. If
you want, we could turn them into DLOG.

However, if we do that, I'm not sure what a good way to test that
deserialization fails is...

[yzshen1, 2017-06-09 16:18:04]

On 2017/06/09 06:39:30, dcheng wrote:
> On 2017/06/08 23:39:56, sandersd wrote:
> > dcheng@: Any explanation why the death test fails on Windows?
> 
> Hmm... sorry I missed this in the review. I wouldn't expect a mojo
> deserialization failure to result in process death: if anything, it'd call
> mojo::ReportBadMessage().
> 
> Is it the DCHECK() that's killing the other death tests? We should probably
take
> the DCHECKs out, as these are legitimate things that a bad process can send.
If
> you want, we could turn them into DLOG.
> 
> However, if we do that, I'm not sure what a good way to test that
> deserialization fails is...

+1 that deserialization failure doesn't kill the receiving process. And whether
it doe  things like killing the sending process is totally up to the bad message
handler set by the embedder.
I don't think we do that in tests.

[xhwang, 2017-06-09 16:54:53]

lgtm % nit, thanks!

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
File media/mojo/services/mojo_cdm_allocator_unittest.cc (right):

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
media/mojo/services/mojo_cdm_allocator_unittest.cc:89: const int kWidth = 9;
nit: It's odd that width < height. Switch the values?

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
media/mojo/services/mojo_cdm_allocator_unittest.cc:91: const gfx::Size
kSize(kHeight, kWidth);
hmm, this should also be (kWidth, kHeight)?

[sandersd (OOO until July 31), 2017-06-09 18:26:33]

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
File media/mojo/services/mojo_cdm_allocator_unittest.cc (right):

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
media/mojo/services/mojo_cdm_allocator_unittest.cc:89: const int kWidth = 9;
On 2017/06/09 16:54:52, xhwang wrote:
> nit: It's odd that width < height. Switch the values?

Done.

https://codereview.chromium.org/2908303003/diff/230001/media/mojo/services/mo...
media/mojo/services/mojo_cdm_allocator_unittest.cc:91: const gfx::Size
kSize(kHeight, kWidth);
On 2017/06/09 16:54:52, xhwang wrote:
> hmm, this should also be (kWidth, kHeight)?

Done.

[sandersd (OOO until July 31), 2017-06-09 18:26:44]

The CQ bit was checked by sandersd@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-09 18:27:07]

Dry run: 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-06-09 20:52:28]

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

[commit-bot: I haz the power, 2017-06-09 20:52:29]

Dry run: This issue passed the CQ dry run.

[sandersd (OOO until July 31), 2017-06-09 21:00:42]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-06-09 21:00:43]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
xhwang@chromium.org
Link to the patchset: https://codereview.chromium.org/2908303003/#ps250001
(title: "Remove death test.")

[commit-bot: I haz the power, 2017-06-09 21:01:06]

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-06-09 21:06:28]

CQ is committing da patch.

Bot data: {"patchset_id": 250001, "attempt_start_ts": 1497042042526090,
"parent_rev": "ad6b46068f1239aa816fc542d2821c91c6fea436", "commit_rev":
"5ad3de8f2d39653bdabf40403b1dc48ae92c8320"}

[commit-bot: I haz the power, 2017-06-09 21:06:41]

Description was changed from

==========
media: Create Mojo StructTraits for VideoFrame

This replaces the existing TypeConverter code for VideoFrames.

BUG=611224
==========

to

==========
media: Create Mojo StructTraits for VideoFrame

This replaces the existing TypeConverter code for VideoFrames.

BUG=611224

Review-Url: https://codereview.chromium.org/2908303003
Cr-Commit-Position: refs/heads/master@{#478413}
Committed:
https://chromium.googlesource.com/chromium/src/+/5ad3de8f2d39653bdabf40403b1d...
==========

[commit-bot: I haz the power, 2017-06-09 21:06:43]

Committed patchset #14 (id:250001) as
https://chromium.googlesource.com/chromium/src/+/5ad3de8f2d39653bdabf40403b1d...

[dcheng, 2017-06-09 21:52:23]

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:51: NOTREACHED() <<
"Unsupported VideoFrame conversion";
As mentioned, there shouldn't be any CHECKs/DCHECKs in struct traits
deserialization, so let's remove these in a followup--we need to handle these
cases gracefully. I think we can assume that the Clone() on line 28 works though
(though yzshen should correct me on that if I'm wrong)

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:89:
DCHECK_EQ(input.end_of_stream(), data.is_null());
Ditto here: this needs to be handled gracefully (which I think it already is,
but there shouldn't be a DCHECK)

[sandersd (OOO until July 31), 2017-06-09 22:07:01]

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:51: NOTREACHED() <<
"Unsupported VideoFrame conversion";
On 2017/06/09 21:52:23, dcheng wrote:
> As mentioned, there shouldn't be any CHECKs/DCHECKs in struct traits
> deserialization, so let's remove these in a followup--we need to handle these
> cases gracefully. I think we can assume that the Clone() on line 28 works
though
> (though yzshen should correct me on that if I'm wrong)

I disagree here; we really don't support serializing all the kinds of
VideoFrames yet. A DCHECK to ensure we don't try seems entirely reasonable to me
(and the release behavior is safe too).

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:89:
DCHECK_EQ(input.end_of_stream(), data.is_null());
On 2017/06/09 21:52:23, dcheng wrote:
> Ditto here: this needs to be handled gracefully (which I think it already is,
> but there shouldn't be a DCHECK)

Yes, in this case we just trust |end_of_stream|, which is not dangerous. There
is no serialization path for them to be out of sync, so this DCHECK isn't adding
anything beyond a sanity check of the serialization code.

That said, this also looks like a correct use of a DCHECK to me (that is, it
verifies a precondition). Why is it so important to remove DCHECKs?

I would propose that the correct solution here is actually to add another entry
to the VideoFrameData enum to signal EOS; then these fields cannot possibly be
out of sync. (EOS frames would still contain redundant fields, but there would
be no possible source for confusion.)

[dcheng, 2017-06-10 00:39:33]

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:51: NOTREACHED() <<
"Unsupported VideoFrame conversion";
On 2017/06/09 22:07:01, sandersd wrote:
> On 2017/06/09 21:52:23, dcheng wrote:
> > As mentioned, there shouldn't be any CHECKs/DCHECKs in struct traits
> > deserialization, so let's remove these in a followup--we need to handle
these
> > cases gracefully. I think we can assume that the Clone() on line 28 works
> though
> > (though yzshen should correct me on that if I'm wrong)
> 
> I disagree here; we really don't support serializing all the kinds of
> VideoFrames yet. A DCHECK to ensure we don't try seems entirely reasonable to
me
> (and the release behavior is safe too).

OK, I got confused and missed that this was for serialization. This is fine.

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:89:
DCHECK_EQ(input.end_of_stream(), data.is_null());
On 2017/06/09 22:07:01, sandersd wrote:
> On 2017/06/09 21:52:23, dcheng wrote:
> > Ditto here: this needs to be handled gracefully (which I think it already
is,
> > but there shouldn't be a DCHECK)
> 
> Yes, in this case we just trust |end_of_stream|, which is not dangerous. There
> is no serialization path for them to be out of sync, so this DCHECK isn't
adding
> anything beyond a sanity check of the serialization code.
> 
> That said, this also looks like a correct use of a DCHECK to me (that is, it
> verifies a precondition). Why is it so important to remove DCHECKs?
> 
> I would propose that the correct solution here is actually to add another
entry
> to the VideoFrameData enum to signal EOS; then these fields cannot possibly be
> out of sync. (EOS frames would still contain redundant fields, but there would
> be no possible source for confusion.)

But this is deserialization, and a bad process can write whatever it wants. If
/when we have mojo fuzzing, this DCHECK() will start getting tripped.

[sandersd (OOO until July 31), 2017-06-10 01:18:21]

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2908303003/diff/250001/media/mojo/interfaces/...
media/mojo/interfaces/video_frame_struct_traits.cc:89:
DCHECK_EQ(input.end_of_stream(), data.is_null());
On 2017/06/10 00:39:32, dcheng wrote:
> On 2017/06/09 22:07:01, sandersd wrote:
> > On 2017/06/09 21:52:23, dcheng wrote:
> > > Ditto here: this needs to be handled gracefully (which I think it already
> is,
> > > but there shouldn't be a DCHECK)
> > 
> > Yes, in this case we just trust |end_of_stream|, which is not dangerous.
There
> > is no serialization path for them to be out of sync, so this DCHECK isn't
> adding
> > anything beyond a sanity check of the serialization code.
> > 
> > That said, this also looks like a correct use of a DCHECK to me (that is, it
> > verifies a precondition). Why is it so important to remove DCHECKs?
> > 
> > I would propose that the correct solution here is actually to add another
> entry
> > to the VideoFrameData enum to signal EOS; then these fields cannot possibly
be
> > out of sync. (EOS frames would still contain redundant fields, but there
would
> > be no possible source for confusion.)
> 
> But this is deserialization, and a bad process can write whatever it wants. If
> /when we have mojo fuzzing, this DCHECK() will start getting tripped.

Fuzzing false-positives is a very convincing answer :-)