Add support for multiple pixel formats in VideoFrameBuffer

webrtc/api/video/video_frame_buffer.cc

Index: webrtc/api/video/video_frame_buffer.cc
diff --git a/webrtc/api/video/video_frame_buffer.cc b/webrtc/api/video/video_frame_buffer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..19685438e52f7ed537f31a5cc54f0551b39620b0
--- /dev/null
+++ b/webrtc/api/video/video_frame_buffer.cc
@@ -0,0 +1,131 @@
+/*
+ *  Copyright (c) 2017 The WebRTC project authors. All Rights Reserved.
+ *
+ *  Use of this source code is governed by a BSD-style license
+ *  that can be found in the LICENSE file in the root of the source
+ *  tree. An additional intellectual property rights grant can be found
+ *  in the file PATENTS.  All contributing project authors may
+ *  be found in the AUTHORS file in the root of the source tree.
+ */
+
+#include "webrtc/api/video/video_frame_buffer.h"
+
+#include "libyuv/convert_from.h"
+#include "webrtc/api/video/i420_buffer.h"
+#include "webrtc/base/checks.h"
+
+namespace webrtc {
+
+namespace {
+
+// Class for handling legacy implementations of NativeToI420Buffer() and
+// adapting them into the new ToI420() function.

[nisse-webrtc, 2017/05/04 08:15:29]

I'm afraid I don't understand what problem this class solves. It seems to just
delegate everything (but the hardcoded PixelFormat) to the wrapped buffer?

[magjed_webrtc, 2017/05/04 12:25:13]

The problem it solves is providing a default implementation of ToI420. I want to
return a PlanarYuvBuffer, but what I have is a VideoFrameBuffer. The plan is to
make ToI420 pure virtual, but we need to update external clients first. What we
have today is NativeToI420Buffer which returns a VideoFrameBuffer (which is not
of the type PlanarYuvBuffer). This adapter allows us to land this CL without
breaking external clients. They should switch from implementing
NativeToI420Buffer to implementing ToI420 asap.

[nisse-webrtc, 2017/05/05 11:33:04]

I see. If we keep this, I think the comment should explain the return type
mismatch.

But maybe we can do this in some simpler way? It would be fine with me to let
the (temporary) default implementation of ToI420 call NativeToI420, DCHECK the
type, and then do an explicit cast to YuvFrameBuffer. Or even simpler, any
reason why

rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::ToI420() {
   return NativeToI420Buffer()->GetI420();
}

isn't good enough?

[magjed_webrtc, 2017/05/05 12:25:24]

So the problem is that existing implementations do not yet implement the new
interface, i.e. they are not using PlanarYuvBuffer and they are not implementing
GetI420() or ToI420(). So the default implementation here can't rely on any of
those, i.e. we can't cast it to PlanarYuvBuffer and we can't call GetI420(). If
we cast it to PlanarYuvBuffer (even if type is kI420), it will cause corruption
since an external client might implement VideoFrameBuffer directly without
inheriting PlanarYuvBuffer. If we call GetI420() like in your second suggestion,
we will hit the default implementation of RTC_NOTREACHED(). The default
implementation I have provided here is the only one that is guaranteed to work
with existing clients. For me this class is a typical example of an adapter that
is needed when deprecating a function. I have tried to explain it as good as I
can, and I added an extra sentence about the return type mismatch that you
wanted. Also note that this is an implementation detail and not part of any API
and that it's supposed to be removed as soon as possible.

[nisse-webrtc, 2017/05/05 12:45:29]

I think I finally get it. So VideoFrameBuffer has default implementations of
ToI420 and NativeToI420, calling each other (so subclasses can implement one or
the other). But it doesn't have any working default implementations of GetI420
(see below) and GetI444.

+class LegacyI420Wrapper : public PlanarYuvBuffer {
+ public:
+  explicit LegacyI420Wrapper(rtc::scoped_refptr<VideoFrameBuffer> buffer)
+      : buffer_(buffer) {}
+
+  PixelFormat Format() const override { return PixelFormat::kI420; }
+
+  int width() const override { return buffer_->width(); }
+  int height() const override { return buffer_->height(); }
+
+  const uint8_t* DataY() const override { return buffer_->DataY(); }
+  const uint8_t* DataU() const override { return buffer_->DataU(); }
+  const uint8_t* DataV() const override { return buffer_->DataV(); }
+
+  int StrideY() const override { return buffer_->StrideY(); }
+  int StrideU() const override { return buffer_->StrideU(); }
+  int StrideV() const override { return buffer_->StrideV(); }
+
+ private:
+  rtc::scoped_refptr<VideoFrameBuffer> buffer_;
+};
+
+}  // namespace
+
+// TODO(magjed): The default implementations in VideoFrameBuffer are provided in
+// order to support the deprecated interface until external clients are updated.
+// Remove once done.
+VideoFrameBuffer::PixelFormat VideoFrameBuffer::Format() const {
+  return native_handle() ? PixelFormat::kNative : PixelFormat::kI420;
+}
+
+const uint8_t* VideoFrameBuffer::DataY() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->DataY();

[nisse-webrtc, 2017/05/04 08:15:29]

This is more forgiving in the native_handle case than the current code.
Maybe use GetI420 instead? 

It might be useful with for automatic conversion from I444 to I420, but I'm not
sure this is the right place.

[magjed_webrtc, 2017/05/04 12:25:13]

Done. Just to clarify, these functions will soon be removed anyway.

+}
+
+const uint8_t* VideoFrameBuffer::DataU() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->DataU();
+}
+
+const uint8_t* VideoFrameBuffer::DataV() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->DataV();
+}
+
+// Returns the number of bytes between successive rows for a given plane.
+int VideoFrameBuffer::StrideY() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->StrideY();
+}
+
+int VideoFrameBuffer::StrideU() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->StrideU();
+}
+
+int VideoFrameBuffer::StrideV() const {
+  return const_cast<VideoFrameBuffer*>(this)->ToI420()->StrideV();
+}
+
+void* VideoFrameBuffer::native_handle() const {
+  RTC_DCHECK(Format() != PixelFormat::kNative);
+  return nullptr;
+}
+
+rtc::scoped_refptr<VideoFrameBuffer> VideoFrameBuffer::NativeToI420Buffer() {
+  return ToI420();
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::ToI420() {
+  return new rtc::RefCountedObject<LegacyI420Wrapper>(NativeToI420Buffer());
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::GetI420() {
+  RTC_NOTREACHED();

[nisse-webrtc, 2017/05/05 12:45:28]

Wouldn't we need a working default implementation of this method, to start using
it? It could perhaps be implemented as 

  return new rtc::RefCountedObject<LegacyI420Wrapper>(this);

[magjed_webrtc, 2017/05/06 19:01:45]

Yes, done. When I think about, these GetXXX functions will not need to be
virtual since the returned subclass is of a known type, so we can just
static_cast. We will have to use the adapter class for GetI420() in the
transition period, but we will be able to move to a static_cast here as well. I
upgraded the DCHECKs to CHECKs so we always crash since it's now dangerous to
continue and updated the documentation accordingly.

[nisse-webrtc, 2017/05/08 06:43:10]

I still think it might make sense to have GetFoo be virtual and return null if
the object isn't of the right type. But we can do that generalization later,
if/when the need arises. 

+  return nullptr;
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> VideoFrameBuffer::GetI444() {
+  RTC_NOTREACHED();
+  return nullptr;
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> PlanarYuvBuffer::ToI420() {
+  rtc::scoped_refptr<I420Buffer> i420_buffer;

[nisse-webrtc, 2017/05/05 11:33:04]

Merge declaration with below assignment?

[magjed_webrtc, 2017/05/05 12:25:24]

Not allowed to have declarations inside switch-statements.

[nisse-webrtc, 2017/05/05 12:45:29]

So you'd need to add a pair of braces to declare variables local to one case,

case ...: {
  declarations
  ...
}

I think I'd prefer locality, but it's a question of taste so
I'll leave it to your judgement.

[magjed_webrtc, 2017/05/06 19:01:45]

Thanks, I didn't know about this. Done.

[nisse-webrtc, 2017/05/08 06:43:10]

I guess the reason you can't have declarations in a switch statement is that the
compiler can't infer which objects need destruction at the end of the block.
You'd get code similar to this:

#include <cstdio>

class Foo {
 public:
  Foo() {
    printf("Constructed\n");
  }
  ~Foo() {
    printf("Destructed?\n");
  }
};

int main (int argc, char **argv) {
  if (argc > 1)
    goto skip;
  Foo foo;
skip:
  return 0;
}

which fails to compile like this:

$ g++ -Wall goto.cc 
goto.cc: In function ‘int main(int, char**)’:
goto.cc:17:1: error: jump to label ‘skip’ [-fpermissive]
 skip:
 ^
goto.cc:15:10: error:   from here [-fpermissive]
     goto skip;
          ^
goto.cc:16:7: error:   crosses initialization of ‘Foo foo’
   Foo foo;
       ^

Adding braces eliminates the problem by restricting the lifetime to a block
without any jump targets (case or labels) in the middle. Which in this case is
the intended meaning.

+  switch (Format()) {
+    case PixelFormat::kI420:
+      return this;
+    case PixelFormat::kI444:
+      i420_buffer = I420Buffer::Create(width(), height());
+      libyuv::I420ToI444(DataY(), StrideY(), DataU(), StrideU(), DataV(),
+                         StrideV(), i420_buffer->MutableDataY(),
+                         i420_buffer->StrideY(), i420_buffer->MutableDataU(),
+                         i420_buffer->StrideU(), i420_buffer->MutableDataV(),
+                         i420_buffer->StrideV(), width(), height());
+      return i420_buffer;
+    default:
+      RTC_NOTREACHED();
+      return nullptr;
+  }
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> PlanarYuvBuffer::GetI420() {
+  RTC_DCHECK(Format() == PixelFormat::kI420);
+  return this;
+}
+
+rtc::scoped_refptr<PlanarYuvBuffer> PlanarYuvBuffer::GetI444() {
+  RTC_DCHECK(Format() == PixelFormat::kI444);
+  return this;
+}
+
+}  // namespace webrtc