media: Followup changes to VideoFrame StructTraits.

media/mojo/interfaces/video_frame_struct_traits.cc

 // Copyright 2017 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/mojo/interfaces/video_frame_struct_traits.h"
 
 #include <utility>
 #include <vector>
 
 #include "base/logging.h"
 #include "media/mojo/common/mojo_shared_buffer_video_frame.h"
 
 namespace mojo {
 
 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 =
         static_cast<media::MojoSharedBufferVideoFrame*>(input.get());
 
     mojo::ScopedSharedBufferHandle dup = mojo_frame->Handle().Clone();
     DCHECK(dup.is_valid());
 
     return media::mojom::VideoFrameData::NewSharedBufferData(
         media::mojom::SharedBufferVideoFrameData::New(
@@ skipping 47 matching lines @@
 
 // static
 bool StructTraits<media::mojom::VideoFrameDataView,
                   scoped_refptr<media::VideoFrame>>::
     Read(media::mojom::VideoFrameDataView input,
          scoped_refptr<media::VideoFrame>* output) {
   // View of the |data| member of the input media::mojom::VideoFrame.
   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;
   }
 
   media::VideoPixelFormat format;
   if (!input.ReadFormat(&format))
     return false;
 
   gfx::Size coded_size;
   if (!input.ReadCodedSize(&coded_size))
     return false;
 
   gfx::Rect visible_rect;
   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))
     return false;
 
   base::TimeDelta timestamp;
   if (!input.ReadTimestamp(&timestamp))
     return false;
 
   if (data.is_shared_buffer_data()) {
@@ skipping 30 matching lines @@
         coded_size, visible_rect, natural_size, timestamp);
     return !!*output;
   }
 
   // TODO(sandersd): Switch on the union tag to avoid this ugliness?
   NOTREACHED();
   return false;
 }
 
 }  // namespace mojo