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..c68fed42ced567b19afa6ad3b4a084d7cb92a2b3 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)) {

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

Btw, can we use a switch for this to make sure we handle all the cases?

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

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.

+    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()) {
+  if (!gfx::Rect(coded_size).Contains(visible_rect))

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

I think Contains() has a const version: do we need the temporary?

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

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]

Sorry =(

I missed that coded_size is a gfx::Size!

     return false;
-  }
 
   gfx::Size natural_size;
   if (!input.ReadNaturalSize(&natural_size))