media: Followup changes to VideoFrame StructTraits.

media: Followup changes to VideoFrame StructTraits.

Included in this change:
  - Actually remove the TypeConverters.
  - Replace |end_of_stream| with a |data| union member.
  - Gentler handling of |visible_rect| problems.

BUG=611224
Review-Url: https://codereview.chromium.org/2933573003
Cr-Commit-Position: refs/heads/master@{#479488}
Committed: https://chromium.googlesource.com/chromium/src/+/2e986179979a89c8a2bdf55d139cd56baacd8573

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

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

[dcheng, 2017-06-10 00:40:55]

LGTM

I like this approach

https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_frame_struct_traits.cc:108:
visible_rect.Intersect(gfx::Rect(coded_size));
Hmm, the original code returned false in that case, maybe we should still do
that here? Maybe Contains would suffice?

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

https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
File media/mojo/interfaces/video_frame_struct_traits.cc (right):

https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
media/mojo/interfaces/video_frame_struct_traits.cc:108:
visible_rect.Intersect(gfx::Rect(coded_size));
On 2017/06/10 00:40:55, dcheng wrote:
> Hmm, the original code returned false in that case, maybe we should still do
> that here? Maybe Contains would suffice?

There is a trade-off here. Relevant facts:

  - |visible_rect| by definition must be contained by |coded_size|.
  - VideoFrame never actually checks that, and it's conceivable that a decoder
could output a frame that violates it. (In particular, decoders routinely
misbehave when the |visible_rect| origin is not 0, 0.)
  - The equivalent path in GpuVideoDecoder substitutes using data from the media
container when this check fails. We don't have that option here.
  - No code should be using |visible_rect| alone to determine memory bounds.
Despite that, I do see places in the code that seem to do so (GPU readback
paths). I am not confident that there will never be a memory safety issue if
this is ignored.
  - In practice, this struct is only used to send frames from more-privileged to
less-privileged processes. There are no plans otherwise, but it probably won't
be true forever.

I decided that it's probably better to silently fix it up rather than have
crashes or other failures at run time, but I'll wait for your second opinion.

[dcheng, 2017-06-10 05:43:47]

On 2017/06/10 01:15:31, sandersd wrote:
>
https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
> File media/mojo/interfaces/video_frame_struct_traits.cc (right):
> 
>
https://codereview.chromium.org/2933573003/diff/1/media/mojo/interfaces/video...
> media/mojo/interfaces/video_frame_struct_traits.cc:108:
> visible_rect.Intersect(gfx::Rect(coded_size));
> On 2017/06/10 00:40:55, dcheng wrote:
> > Hmm, the original code returned false in that case, maybe we should still do
> > that here? Maybe Contains would suffice?
> 
> There is a trade-off here. Relevant facts:
> 
>   - |visible_rect| by definition must be contained by |coded_size|.
>   - VideoFrame never actually checks that, and it's conceivable that a decoder
> could output a frame that violates it. (In particular, decoders routinely
> misbehave when the |visible_rect| origin is not 0, 0.)
>   - The equivalent path in GpuVideoDecoder substitutes using data from the
media
> container when this check fails. We don't have that option here.
>   - No code should be using |visible_rect| alone to determine memory bounds.
> Despite that, I do see places in the code that seem to do so (GPU readback
> paths). I am not confident that there will never be a memory safety issue if
> this is ignored.
>   - In practice, this struct is only used to send frames from more-privileged
to
> less-privileged processes. There are no plans otherwise, but it probably won't
> be true forever.
> 
> I decided that it's probably better to silently fix it up rather than have
> crashes or other failures at run time, but I'll wait for your second opinion.

Can you help me understand which processes the frame is being sent between? Is
it for sure that we'll never be sending the frames from a less privileged to a
more privileged process? If there's a chance, it doesn't hurt to return false
does it? It won't kill more privileged processes, but it will (usually) still
terminate less privileged  ones.

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

> Can you help me understand which processes the frame is being sent between? Is
> it for sure that we'll never be sending the frames from a less privileged to a
> more privileged process? If there's a chance, it doesn't hurt to return false
> does it? It won't kill more privileged processes, but it will (usually) still
> terminate less privileged  ones.

There are currently two users of this struct:
  - media.mojom.VideoDecoder runs in the GPU process and emits VideoFrames to
the renderer.
  - media.mojom.Decryptor runs in a media utility process and emits VideoFrames
to the renderer.

As for harm, failing at the IPC level for minor decoder bugs would be surprising
and difficult to debug. I'm not sure that Intersect() is the best, since it
masks any issue, but I do think that making the condition handle-able is the
better trade-off.

[dcheng, 2017-06-12 20:31:33]

On 2017/06/12 18:26:48, sandersd wrote:
> > Can you help me understand which processes the frame is being sent between?
Is
> > it for sure that we'll never be sending the frames from a less privileged to
a
> > more privileged process? If there's a chance, it doesn't hurt to return
false
> > does it? It won't kill more privileged processes, but it will (usually)
still
> > terminate less privileged  ones.
> 
> There are currently two users of this struct:
>   - media.mojom.VideoDecoder runs in the GPU process and emits VideoFrames to
> the renderer.
>   - media.mojom.Decryptor runs in a media utility process and emits
VideoFrames
> to the renderer.
> 
> As for harm, failing at the IPC level for minor decoder bugs would be
surprising
> and difficult to debug. I'm not sure that Intersect() is the best, since it
> masks any issue, but I do think that making the condition handle-able is the
> better trade-off.

I guess the concern here is that the assumption of sending from higher to
less-privileged might not always be true. Would DLOG()ing on failure be good
enough WRT debugging issues in this codepath?

[sandersd (OOO until July 31), 2017-06-12 22:06:06]

On 2017/06/12 20:31:33, dcheng wrote:
> On 2017/06/12 18:26:48, sandersd wrote:
> > > Can you help me understand which processes the frame is being sent
between?
> Is
> > > it for sure that we'll never be sending the frames from a less privileged
to
> a
> > > more privileged process? If there's a chance, it doesn't hurt to return
> false
> > > does it? It won't kill more privileged processes, but it will (usually)
> still
> > > terminate less privileged  ones.
> > 
> > There are currently two users of this struct:
> >   - media.mojom.VideoDecoder runs in the GPU process and emits VideoFrames
to
> > the renderer.
> >   - media.mojom.Decryptor runs in a media utility process and emits
> VideoFrames
> > to the renderer.
> > 
> > As for harm, failing at the IPC level for minor decoder bugs would be
> surprising
> > and difficult to debug. I'm not sure that Intersect() is the best, since it
> > masks any issue, but I do think that making the condition handle-able is the
> > better trade-off.
> 
> I guess the concern here is that the assumption of sending from higher to
> less-privileged might not always be true. Would DLOG()ing on failure be good
> enough WRT debugging issues in this codepath?

Here's a different approach: change the VideoFrame constructor so that there is
no path to trigger this behavior. PTAL.

[sandersd (OOO until July 31), 2017-06-12 22:06:36]

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

[sandersd (OOO until July 31), 2017-06-12 22:06:37]

xhwang@: Please review changes to video_frame.cc.

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

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

[commit-bot: I haz the power, 2017-06-12 22:11:52]

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-13 00:20:23]

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

[commit-bot: I haz the power, 2017-06-13 00:20:24]

Dry run: This issue passed the CQ dry run.

[xhwang, 2017-06-13 00:37:41]

lgtm

[dcheng, 2017-06-13 07:10:16]

LGTM with nits

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

https://codereview.chromium.org/2933573003/diff/40001/media/base/video_frame....
media/base/video_frame.cc:29: gfx::Rect Intersection(gfx::Rect a, gfx::Rect b) {
|b| can be passed by const ref, right?

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

https://codereview.chromium.org/2933573003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:19: if
(input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM)) {
Btw, can we use a switch for this to make sure we handle all the cases?

https://codereview.chromium.org/2933573003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:107: if
(!gfx::Rect(coded_size).Contains(visible_rect))
I think Contains() has a const version: do we need the temporary?

[sandersd (OOO until July 31), 2017-06-13 17:50:15]

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

https://codereview.chromium.org/2933573003/diff/40001/media/base/video_frame....
media/base/video_frame.cc:29: gfx::Rect Intersection(gfx::Rect a, gfx::Rect b) {
On 2017/06/13 07:10:16, dcheng wrote:
> |b| can be passed by const ref, right?

Done.

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

https://codereview.chromium.org/2933573003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:19: if
(input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM)) {
On 2017/06/13 07:10:16, dcheng wrote:
> Btw, can we use a switch for this to make sure we handle all the cases?

Not that I can see; in particular the three cases right now are not mutually
exclusive (in practice they are, but VideoFrame doesn't enforce it).

Hopefully VideoFrame will evolve in a direction that fixes that. The Mojo struct
definition is a good model to replicate.

https://codereview.chromium.org/2933573003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:107: if
(!gfx::Rect(coded_size).Contains(visible_rect))
On 2017/06/13 07:10:16, dcheng wrote:
> I think Contains() has a const version: do we need the temporary?

I don't understand; Contains() is a member of gfx::Rect, there is no way to
operate between them without constructing a Rect for |coded_size|.

[dcheng, 2017-06-14 17:46:48]

still lgtm

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

https://codereview.chromium.org/2933573003/diff/40001/media/mojo/interfaces/v...
media/mojo/interfaces/video_frame_struct_traits.cc:107: if
(!gfx::Rect(coded_size).Contains(visible_rect))
On 2017/06/13 17:50:15, sandersd wrote:
> On 2017/06/13 07:10:16, dcheng wrote:
> > I think Contains() has a const version: do we need the temporary?
> 
> I don't understand; Contains() is a member of gfx::Rect, there is no way to
> operate between them without constructing a Rect for |coded_size|.

Sorry =(

I missed that coded_size is a gfx::Size!

[sandersd (OOO until July 31), 2017-06-14 18:12:39]

The CQ bit was checked by sandersd@chromium.org

[sandersd (OOO until July 31), 2017-06-14 18:12:40]

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/2933573003/#ps60001
(title: "const&")

[commit-bot: I haz the power, 2017-06-14 18:13:08]

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-14 20:29:18]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1497463959968420,
"parent_rev": "997174a8a84df91727a37259fb5ee7ea6ee7e1a2", "commit_rev":
"2e986179979a89c8a2bdf55d139cd56baacd8573"}

[commit-bot: I haz the power, 2017-06-14 20:29:31]

Description was changed from

==========
media: Followup changes to VideoFrame StructTraits.

Included in this change:
  - Actually remove the TypeConverters.
  - Replace |end_of_stream| with a |data| union member.
  - Gentler handling of |visible_rect| problems.

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

to

==========
media: Followup changes to VideoFrame StructTraits.

Included in this change:
  - Actually remove the TypeConverters.
  - Replace |end_of_stream| with a |data| union member.
  - Gentler handling of |visible_rect| problems.

BUG=611224

Review-Url: https://codereview.chromium.org/2933573003
Cr-Commit-Position: refs/heads/master@{#479488}
Committed:
https://chromium.googlesource.com/chromium/src/+/2e986179979a89c8a2bdf55d139c...
==========

[commit-bot: I haz the power, 2017-06-14 20:29:33]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2e986179979a89c8a2bdf55d139c...