MediaStreamRemoteVideoSource: Wrap cricket::VideoFrame instead of copying.

media/base/video_frame.h

Index: media/base/video_frame.h
diff --git a/media/base/video_frame.h b/media/base/video_frame.h
index cebed0af11a47aea5537b92346d97cebae1ef05f..784ef9db06c293b4bfa93ec5248b09ee4609ef53 100644
--- a/media/base/video_frame.h
+++ b/media/base/video_frame.h
@@ -147,6 +147,22 @@ class MEDIA_EXPORT VideoFrame : public base::RefCountedThreadSafe<VideoFrame> {
       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
@@ -269,6 +285,7 @@ class MEDIA_EXPORT VideoFrame : public base::RefCountedThreadSafe<VideoFrame> {
   // 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
@@ -276,6 +293,7 @@ class MEDIA_EXPORT VideoFrame : public base::RefCountedThreadSafe<VideoFrame> {
   // 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.
@@ -382,6 +400,7 @@ class MEDIA_EXPORT VideoFrame : public base::RefCountedThreadSafe<VideoFrame> {
 
   // 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_;