MediaStreamRemoteVideoSource: Wrap cricket::VideoFrame instead of copying.

media/base/video_frame.h

 // Copyright (c) 2012 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.
 
 #ifndef MEDIA_BASE_VIDEO_FRAME_H_
 #define MEDIA_BASE_VIDEO_FRAME_H_
 
 #include <vector>
 
 #include "base/callback.h"
@@ skipping 129 matching lines @@
       const gfx::Size& natural_size,
       int32 y_stride,
       int32 u_stride,
       int32 v_stride,
       uint8* y_data,
       uint8* u_data,
       uint8* v_data,
       base::TimeDelta timestamp,
       const base::Closure& no_longer_needed_cb);
 
+  // Similar to the above function except with const pointers. The pointers will
+  // be protected with run-time checks.
+  static scoped_refptr<VideoFrame> WrapExternalYuvConstData(
+      Format format,
+      const gfx::Size& coded_size,
+      const gfx::Rect& visible_rect,
+      const gfx::Size& natural_size,
+      int32 y_stride,
+      int32 u_stride,
+      int32 v_stride,
+      const uint8* y_data,
+      const uint8* u_data,
+      const uint8* v_data,
+      base::TimeDelta timestamp,
+      const base::Closure& no_longer_needed_cb);
+
 #if defined(OS_POSIX)
   // Wraps provided dmabufs
   // (https://www.kernel.org/doc/Documentation/dma-buf-sharing.txt) with a
   // VideoFrame. The dmabuf fds are dup()ed on creation, so that the VideoFrame
   // retains a reference to them, and are automatically close()d on destruction,
   // dropping the reference. The caller may safely close() its reference after
   // calling WrapExternalDmabufs().
   // The image data is only accessible via dmabuf fds, which are usually passed
   // directly to a hardware device and/or to another process, or can also be
   // mapped via mmap() for CPU access.
@@ skipping 102 matching lines @@
   // Returns the number of bytes per row and number of rows for a given plane.
   //
   // As opposed to stride(), row_bytes() refers to the bytes representing
   // frame data scanlines (coded_size.width() pixels, without stride padding).
   int row_bytes(size_t plane) const;
   int rows(size_t plane) const;
 
   // Returns pointer to the buffer for a given plane. The memory is owned by
   // VideoFrame object and must not be freed by the caller.
   const uint8* data(size_t plane) const;
+  // Non-const data access is only allowed if |is_read_only_| is false.
   uint8* data(size_t plane);
 
   // Returns pointer to the data in the visible region of the frame. I.e. the
   // returned pointer is offsetted into the plane buffer specified by
   // visible_rect().origin(). Memory is owned by VideoFrame object and must not
   // be freed by the caller.
   const uint8* visible_data(size_t plane) const;
+  // Non-const data access is only allowed if |is_read_only_| is false.
   uint8* visible_data(size_t plane);
 
   // Returns the mailbox holder of the native texture wrapped by this frame.
   // Only valid to call if this is a NATIVE_TEXTURE frame. Before using the
   // mailbox, the caller must wait for the included sync point.
   const gpu::MailboxHolder* mailbox_holder() const;
 
   // Returns the shared-memory handle, if present
   base::SharedMemoryHandle shared_memory_handle() const;
 
@@ skipping 86 matching lines @@
   // (|visible_rect_.size()|) with aspect ratio taken into account.
   const gfx::Size natural_size_;
 
   // Array of strides for each plane, typically greater or equal to the width
   // of the surface divided by the horizontal sampling period.  Note that
   // strides can be negative.
   int32 strides_[kMaxPlanes];
 
   // Array of data pointers to each plane.
   uint8* data_[kMaxPlanes];
+  bool is_read_only_;

[mcasas, 2015/03/06 17:29:59]

Having a flag and |data_[]| separated might be an
accident waiting to happen? Specially since you
only use it for DCHECKing.

I see two options, one is biting the bullet and
substituting data_+is_read_only_ with a class
doing policy check and access, another is throwing
away this check. As it is, is not adding much...

Or if you feel ready for a big CL, add a policy or
traits to VideoFrame, being that Read-Only or
ReadWrite, which would control the access to it :)

[magjed_chromium, 2015/03/07 14:15:10]

DCHECKing a read_only flag adds enough to find places where we incorrectly
extract non-const pointers. For example, here is the first place for
WebRtcBrowserTest.CanForwardRemoteStream720p:
https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/skcanv...
Of course, I would prefer static checks, but that would require a huge CL. For
example, no one is using 'scoped_refptr<const VideoFrame>’.

 
   // Native texture mailbox, if this is a NATIVE_TEXTURE frame.
   const scoped_ptr<gpu::MailboxHolder> mailbox_holder_;
   ReleaseMailboxCB mailbox_holder_release_cb_;
 
   // Shared memory handle, if this frame was allocated from shared memory.
   base::SharedMemoryHandle shared_memory_handle_;
 
   // Offset in shared memory buffer.
   size_t shared_memory_offset_;
@@ skipping 21 matching lines @@
   VideoFrameMetadata metadata_;
 
   bool allow_overlay_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(VideoFrame);
 };
 
 }  // namespace media
 
 #endif  // MEDIA_BASE_VIDEO_FRAME_H_