media: Followup changes to VideoFrame StructTraits.

media/mojo/interfaces/video_frame_struct_traits.cc

Index: media/mojo/interfaces/video_frame_struct_traits.cc
diff --git a/media/mojo/interfaces/video_frame_struct_traits.cc b/media/mojo/interfaces/video_frame_struct_traits.cc
index 4d6aebf49d67f4a3a451e5f71ced6dbeb552ed73..76fee7c14744457f7110f842c3f2ff775afe5c8f 100644
--- a/media/mojo/interfaces/video_frame_struct_traits.cc
+++ b/media/mojo/interfaces/video_frame_struct_traits.cc
@@ -16,9 +16,10 @@ namespace {
 
 media::mojom::VideoFrameDataPtr MakeVideoFrameData(
     const scoped_refptr<media::VideoFrame>& input) {
-  // EOS frames contain no data.
-  if (input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM))
-    return nullptr;
+  if (input->metadata()->IsTrue(media::VideoFrameMetadata::END_OF_STREAM)) {
+    return media::mojom::VideoFrameData::NewEosData(
+        media::mojom::EosVideoFrameData::New());
+  }
 
   if (input->storage_type() == media::VideoFrame::STORAGE_MOJO_SHARED_BUFFER) {
     media::MojoSharedBufferVideoFrame* mojo_frame =
@@ -86,8 +87,7 @@ bool StructTraits<media::mojom::VideoFrameDataView,
   media::mojom::VideoFrameDataDataView data;
   input.GetDataDataView(&data);
 
-  DCHECK_EQ(input.end_of_stream(), data.is_null());
-  if (input.end_of_stream()) {
+  if (data.is_eos_data()) {
     *output = media::VideoFrame::CreateEOSFrame();
     return !!*output;
   }
@@ -104,11 +104,8 @@ bool StructTraits<media::mojom::VideoFrameDataView,
   if (!input.ReadVisibleRect(&visible_rect))
     return false;
 
-  // Coded size must contain the visible rect.
-  if (visible_rect.right() > coded_size.width() ||
-      visible_rect.bottom() > coded_size.height()) {
-    return false;
-  }
+  // Ensure that |visible_rect| is contained inside |coded_size|.
+  visible_rect.Intersect(gfx::Rect(coded_size));

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

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]

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.

 
   gfx::Size natural_size;
   if (!input.ReadNaturalSize(&natural_size))