VideoFrameBuffer: Make non-const data access explicit

VideoFrameBuffer currently has two overloaded data() functions for pixel access, one for const and one for non-const. Unfortunately, it will default to the non-const version, even when 'const scoped_refptr<VideoFrameBuffer>&' is used. This is a problem, because many subclasses use RTC_NOTREACHED() in the non-const version.

This CL makes the non-const version of data() explicit with a different, longer function name MutableData().

R=tommi@webrtc.org

Review URL: https://codereview.webrtc.org/1304143003 .

Cr-Commit-Position: refs/heads/master@{#9787}

talk/media/webrtc/webrtcvideoframe.cc

@@ -117,35 +117,32 @@ size_t WebRtcVideoFrame::GetHeight() const {
 }
 
 const uint8* WebRtcVideoFrame::GetYPlane() const {
-  // Const cast to call the correct const-version of data.
-  const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get();
-  return const_ptr ? const_ptr->data(kYPlane) : nullptr;
-}
-
-const uint8* WebRtcVideoFrame::GetUPlane() const {
-  // Const cast to call the correct const-version of data.
-  const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get();
-  return const_ptr ? const_ptr->data(kUPlane) : nullptr;
-}
-
-const uint8* WebRtcVideoFrame::GetVPlane() const {
-  // Const cast to call the correct const-version of data.
-  const webrtc::VideoFrameBuffer* const_ptr = video_frame_buffer_.get();
-  return const_ptr ? const_ptr->data(kVPlane) : nullptr;
-}
-
-uint8* WebRtcVideoFrame::GetYPlane() {
   return video_frame_buffer_ ? video_frame_buffer_->data(kYPlane) : nullptr;
 }
 
-uint8* WebRtcVideoFrame::GetUPlane() {
+const uint8* WebRtcVideoFrame::GetUPlane() const {
   return video_frame_buffer_ ? video_frame_buffer_->data(kUPlane) : nullptr;
 }
 
-uint8* WebRtcVideoFrame::GetVPlane() {
+const uint8* WebRtcVideoFrame::GetVPlane() const {
   return video_frame_buffer_ ? video_frame_buffer_->data(kVPlane) : nullptr;
 }
 
+uint8* WebRtcVideoFrame::GetYPlane() {
+  return video_frame_buffer_ ? video_frame_buffer_->MutableData(kYPlane)
+                             : nullptr;
+}
+
+uint8* WebRtcVideoFrame::GetUPlane() {
+  return video_frame_buffer_ ? video_frame_buffer_->MutableData(kUPlane)
+                             : nullptr;
+}
+
+uint8* WebRtcVideoFrame::GetVPlane() {
+  return video_frame_buffer_ ? video_frame_buffer_->MutableData(kVPlane)
+                             : nullptr;
+}
+
 int32 WebRtcVideoFrame::GetYPitch() const {
   return video_frame_buffer_ ? video_frame_buffer_->stride(kYPlane) : 0;
 }
@@ -192,9 +189,10 @@ bool WebRtcVideoFrame::MakeExclusive() {
           video_frame_buffer_->stride(kUPlane),
           video_frame_buffer_->stride(kVPlane));
 
-  if (!CopyToPlanes(new_buffer->data(kYPlane), new_buffer->data(kUPlane),
-                    new_buffer->data(kVPlane), new_buffer->stride(kYPlane),
-                    new_buffer->stride(kUPlane), new_buffer->stride(kVPlane))) {
+  if (!CopyToPlanes(
+          new_buffer->MutableData(kYPlane), new_buffer->MutableData(kUPlane),
+          new_buffer->MutableData(kVPlane), new_buffer->stride(kYPlane),
+          new_buffer->stride(kUPlane), new_buffer->stride(kVPlane))) {
     return false;
   }
 

webrtc/common_video/i420_buffer_pool.cc

@@ -27,13 +27,13 @@ class PooledI420Buffer : public webrtc::VideoFrameBuffer {
   int width() const override { return buffer_->width(); }
   int height() const override { return buffer_->height(); }
   const uint8_t* data(webrtc::PlaneType type) const override {
-    const webrtc::I420Buffer* cbuffer = buffer_.get();
-    return cbuffer->data(type);
+    return buffer_->data(type);
   }
-  uint8_t* data(webrtc::PlaneType type) {
+  uint8_t* MutableData(webrtc::PlaneType type) override {
+    // Make the HasOneRef() check here instead of in |buffer_|, because the pool
+    // also has a reference to |buffer_|.
     DCHECK(HasOneRef());
-    const webrtc::I420Buffer* cbuffer = buffer_.get();
-    return const_cast<uint8_t*>(cbuffer->data(type));
+    return const_cast<uint8_t*>(buffer_->data(type));
   }
   int stride(webrtc::PlaneType type) const override {
     return buffer_->stride(type);

webrtc/common_video/i420_buffer_pool_unittest.cc

@@ -68,7 +68,7 @@ TEST(TestI420BufferPool, FrameValidAfterPoolDestruction) {
   EXPECT_EQ(16, buffer->width());
   EXPECT_EQ(16, buffer->height());
   // Try to trigger use-after-free errors by writing to y-plane.
-  memset(buffer->data(kYPlane), 0xA5, 16 * buffer->stride(kYPlane));
+  memset(buffer->MutableData(kYPlane), 0xA5, 16 * buffer->stride(kYPlane));
 }
 
 }  // namespace webrtc

webrtc/common_video/interface/video_frame_buffer.h

@@ -42,8 +42,9 @@ class VideoFrameBuffer : public rtc::RefCountInterface {
   // the VideoFrameBuffer object and must not be freed by the caller.
   virtual const uint8_t* data(PlaneType type) const = 0;
 
-  // Non-const data access is only allowed if |HasOneRef| is true.
-  virtual uint8_t* data(PlaneType type) = 0;
+  // Non-const data access is disallowed by default. You need to make sure you
+  // have exclusive access and a writable buffer before calling this function.
+  virtual uint8_t* MutableData(PlaneType type);
 
   // Returns the number of bytes between successive rows for a given plane.
   virtual int stride(PlaneType type) const = 0;
@@ -69,7 +70,9 @@ class I420Buffer : public VideoFrameBuffer {
   int width() const override;
   int height() const override;
   const uint8_t* data(PlaneType type) const override;
-  uint8_t* data(PlaneType type) override;
+  // Non-const data access is only allowed if HasOneRef() is true to protect
+  // against unexpected overwrites.
+  uint8_t* MutableData(PlaneType type) override;
   int stride(PlaneType type) const override;
   void* native_handle() const override;
   rtc::scoped_refptr<VideoFrameBuffer> NativeToI420Buffer() override;
@@ -97,7 +100,6 @@ class NativeHandleBuffer : public VideoFrameBuffer {
   int width() const override;
   int height() const override;
   const uint8_t* data(PlaneType type) const override;
-  uint8_t* data(PlaneType type) override;
   int stride(PlaneType type) const override;
   void* native_handle() const override;
 
@@ -122,7 +124,6 @@ class WrappedI420Buffer : public webrtc::VideoFrameBuffer {
   int height() const override;
 
   const uint8_t* data(PlaneType type) const override;
-  uint8_t* data(PlaneType type) override;
 
   int stride(PlaneType type) const override;
   void* native_handle() const override;

webrtc/common_video/video_frame.cc

@@ -152,13 +152,12 @@ void VideoFrame::Reset() {
 }
 
 uint8_t* VideoFrame::buffer(PlaneType type) {
-  return video_frame_buffer_ ? video_frame_buffer_->data(type) : nullptr;
+  return video_frame_buffer_ ? video_frame_buffer_->MutableData(type)
+                             : nullptr;
 }
 
 const uint8_t* VideoFrame::buffer(PlaneType type) const {
-  // Const cast to call the correct const-version of data.
-  const VideoFrameBuffer* const_buffer = video_frame_buffer_.get();
-  return const_buffer ? const_buffer->data(type) : nullptr;
+  return video_frame_buffer_ ? video_frame_buffer_->data(type) : nullptr;
 }
 
 int VideoFrame::allocated_size(PlaneType type) const {

webrtc/common_video/video_frame_buffer.cc

@@ -24,6 +24,11 @@ static void NoLongerUsedCallback(rtc::scoped_refptr<VideoFrameBuffer> dummy) {}
 
 }  // anonymous namespace
 
+uint8_t* VideoFrameBuffer::MutableData(PlaneType type) {
+  RTC_NOTREACHED();
+  return nullptr;
+}
+
 VideoFrameBuffer::~VideoFrameBuffer() {}
 
 I420Buffer::I420Buffer(int width, int height)
@@ -76,7 +81,7 @@ const uint8_t* I420Buffer::data(PlaneType type) const {
   }
 }
 
-uint8_t* I420Buffer::data(PlaneType type) {
+uint8_t* I420Buffer::MutableData(PlaneType type) {
   DCHECK(HasOneRef());
   return const_cast<uint8_t*>(
       static_cast<const VideoFrameBuffer*>(this)->data(type));
@@ -127,11 +132,6 @@ const uint8_t* NativeHandleBuffer::data(PlaneType type) const {
   return nullptr;
 }
 
-uint8_t* NativeHandleBuffer::data(PlaneType type) {
-  RTC_NOTREACHED();  // Should not be called.
-  return nullptr;
-}
-
 int NativeHandleBuffer::stride(PlaneType type) const {
   RTC_NOTREACHED();  // Should not be called.
   return 0;
@@ -187,11 +187,6 @@ const uint8_t* WrappedI420Buffer::data(PlaneType type) const {
   }
 }
 
-uint8_t* WrappedI420Buffer::data(PlaneType type) {
-  RTC_NOTREACHED();
-  return nullptr;
-}
-
 int WrappedI420Buffer::stride(PlaneType type) const {
   switch (type) {
     case kYPlane:
@@ -232,13 +227,11 @@ rtc::scoped_refptr<VideoFrameBuffer> ShallowCenterCrop(
   const int offset_x = uv_offset_x * 2;
   const int offset_y = uv_offset_y * 2;
 
-  // Const cast to call the correct const-version of data().
-  const VideoFrameBuffer* const_buffer(buffer.get());
-  const uint8_t* y_plane = const_buffer->data(kYPlane) +
+  const uint8_t* y_plane = buffer->data(kYPlane) +
                            buffer->stride(kYPlane) * offset_y + offset_x;
-  const uint8_t* u_plane = const_buffer->data(kUPlane) +
+  const uint8_t* u_plane = buffer->data(kUPlane) +
                            buffer->stride(kUPlane) * uv_offset_y + uv_offset_x;
-  const uint8_t* v_plane = const_buffer->data(kVPlane) +
+  const uint8_t* v_plane = buffer->data(kVPlane) +
                            buffer->stride(kVPlane) * uv_offset_y + uv_offset_x;
   return new rtc::RefCountedObject<WrappedI420Buffer>(
       cropped_width, cropped_height,

webrtc/modules/video_coding/codecs/h264/h264_video_toolbox_decoder.cc

@@ -64,9 +64,9 @@ rtc::scoped_refptr<webrtc::VideoFrameBuffer> VideoFrameBufferForPixelBuffer(
   int src_uv_stride = CVPixelBufferGetBytesPerRowOfPlane(pixel_buffer, 1);
   int ret = libyuv::NV12ToI420(
       src_y, src_y_stride, src_uv, src_uv_stride,
-      buffer->data(webrtc::kYPlane), buffer->stride(webrtc::kYPlane),
-      buffer->data(webrtc::kUPlane), buffer->stride(webrtc::kUPlane),
-      buffer->data(webrtc::kVPlane), buffer->stride(webrtc::kVPlane),
+      buffer->MutableData(webrtc::kYPlane), buffer->stride(webrtc::kYPlane),
+      buffer->MutableData(webrtc::kUPlane), buffer->stride(webrtc::kUPlane),
+      buffer->MutableData(webrtc::kVPlane), buffer->stride(webrtc::kVPlane),
       width, height);
   CVPixelBufferUnlockBaseAddress(pixel_buffer, kCVPixelBufferLock_ReadOnly);
   if (ret) {

webrtc/test/fake_texture_frame.h

@@ -34,9 +34,9 @@ class FakeNativeHandleBuffer : public NativeHandleBuffer {
         new rtc::RefCountedObject<I420Buffer>(width_, height_));
     int half_height = (height_ + 1) / 2;
     int half_width = (width_ + 1) / 2;
-    memset(buffer->data(kYPlane), 0, height_ * width_);
-    memset(buffer->data(kUPlane), 0, half_height * half_width);
-    memset(buffer->data(kVPlane), 0, half_height * half_width);
+    memset(buffer->MutableData(kYPlane), 0, height_ * width_);
+    memset(buffer->MutableData(kUPlane), 0, half_height * half_width);
+    memset(buffer->MutableData(kVPlane), 0, half_height * half_width);
     return buffer;
   }
 };