Add callback to copy texture backed frames in WebRtcVideoFrameAdapter

content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc

Index: content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc
diff --git a/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc b/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc
index 96b6fe32af607570b192f469204a6f3298979e64..9c7a7efc5de81ec71b2b0630060f672fa54ba86f 100644
--- a/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc
+++ b/content/renderer/media/webrtc/webrtc_video_capturer_adapter.cc
@@ -6,12 +6,18 @@
 
 #include "base/bind.h"
 #include "base/memory/aligned_memory.h"
+#include "base/synchronization/waitable_event.h"
 #include "base/trace_event/trace_event.h"
-#include "content/renderer/media/webrtc/webrtc_video_frame_adapter.h"
+#include "content/common/gpu/client/context_provider_command_buffer.h"
+#include "content/renderer/render_thread_impl.h"
+#include "media/base/bind_to_current_loop.h"
 #include "media/base/timestamp_constants.h"
 #include "media/base/video_util.h"
+#include "skia/ext/platform_canvas.h"
+#include "third_party/libyuv/include/libyuv/convert.h"
 #include "third_party/libyuv/include/libyuv/convert_from.h"
 #include "third_party/libyuv/include/libyuv/scale.h"
+#include "third_party/skia/include/core/SkSurface.h"
 #include "third_party/webrtc/common_video/include/video_frame_buffer.h"
 #include "third_party/webrtc/common_video/rotation.h"
 #include "third_party/webrtc/media/engine/webrtcvideoframe.h"
@@ -27,71 +33,21 @@ void ReleaseOriginalFrame(const scoped_refptr<media::VideoFrame>& frame) {
 }  // anonymous namespace
 
 WebRtcVideoCapturerAdapter::WebRtcVideoCapturerAdapter(bool is_screencast)
-    : is_screencast_(is_screencast),
-      running_(false) {
+    : main_thread_task_runner_(base::ThreadTaskRunnerHandle::Get()),
+      provider_(RenderThreadImpl::current()->SharedMainThreadContextProvider()),
+      is_screencast_(is_screencast),
+      running_(false),
+      weak_factory_(this) {
   thread_checker_.DetachFromThread();
+  copy_texture_callback_ =
+      base::Bind(&WebRtcVideoCapturerAdapter::CopyTextureFrame,
+                 weak_factory_.GetWeakPtr());
 }
 
 WebRtcVideoCapturerAdapter::~WebRtcVideoCapturerAdapter() {
   DVLOG(3) << " WebRtcVideoCapturerAdapter::dtor";

[mcasas, 2016/10/26 16:06:02]

unrelated nit: I'm trying to correct these statements to
DVLO(x) << __func__;

[emircan, 2016/10/27 00:37:56]

Done.

 }
 
-cricket::CaptureState WebRtcVideoCapturerAdapter::Start(
-    const cricket::VideoFormat& capture_format) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!running_);
-  DVLOG(3) << " WebRtcVideoCapturerAdapter::Start w = " << capture_format.width
-           << " h = " << capture_format.height;
-
-  running_ = true;
-  return cricket::CS_RUNNING;
-}
-
-void WebRtcVideoCapturerAdapter::Stop() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DVLOG(3) << " WebRtcVideoCapturerAdapter::Stop ";
-  DCHECK(running_);
-  running_ = false;
-  SetCaptureFormat(NULL);
-  SignalStateChange(this, cricket::CS_STOPPED);
-}
-
-bool WebRtcVideoCapturerAdapter::IsRunning() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return running_;
-}
-
-bool WebRtcVideoCapturerAdapter::GetPreferredFourccs(
-    std::vector<uint32_t>* fourccs) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(!fourccs || fourccs->empty());
-  if (fourccs)
-    fourccs->push_back(cricket::FOURCC_I420);
-  return fourccs != NULL;
-}
-
-bool WebRtcVideoCapturerAdapter::IsScreencast() const {
-  return is_screencast_;
-}
-
-bool WebRtcVideoCapturerAdapter::GetBestCaptureFormat(
-    const cricket::VideoFormat& desired,
-    cricket::VideoFormat* best_format) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DVLOG(3) << " GetBestCaptureFormat:: "
-           << " w = " << desired.width
-           << " h = " << desired.height;
-
-  // Capability enumeration is done in MediaStreamVideoSource. The adapter can
-  // just use what is provided.
-  // Use the desired format as the best format.
-  best_format->width = desired.width;
-  best_format->height = desired.height;
-  best_format->fourcc = cricket::FOURCC_I420;
-  best_format->interval = desired.interval;
-  return true;
-}
-
 void WebRtcVideoCapturerAdapter::OnFrameCaptured(
     const scoped_refptr<media::VideoFrame>& input_frame) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -99,9 +55,13 @@ void WebRtcVideoCapturerAdapter::OnFrameCaptured(
   if (!(input_frame->IsMappable() &&
         (input_frame->format() == media::PIXEL_FORMAT_I420 ||
          input_frame->format() == media::PIXEL_FORMAT_YV12 ||
-         input_frame->format() == media::PIXEL_FORMAT_YV12A))) {
+         input_frame->format() == media::PIXEL_FORMAT_YV12A)) &&
+      !input_frame->HasTextures()) {
     // Since connecting sources and sinks do not check the format, we need to
     // just ignore formats that we can not handle.
+    LOG(ERROR) << "We cannot send frame with storage type: "
+               << input_frame->storage_type()
+               << " format: " << input_frame->format();

[mcasas, 2016/10/26 16:06:02]

These are going to print numbers, right?
Consider using media::VIdeoCaptureFormat::ToString().

[emircan, 2016/10/27 00:37:56]

Done.

     NOTREACHED();
     return;
   }
@@ -133,11 +93,10 @@ void WebRtcVideoCapturerAdapter::OnFrameCaptured(
 
   // Return |frame| directly if it is texture backed, because there is no
   // cropping support for texture yet. See http://crbug/503653.
-  // Return |frame| directly if it is GpuMemoryBuffer backed, as we want to
-  // keep the frame on native buffers.
   if (frame->HasTextures()) {
     OnFrame(cricket::WebRtcVideoFrame(
-                new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(frame),
+                new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(
+                    frame, copy_texture_callback_),
                 webrtc::kVideoRotation_0, translated_camera_time_us),
             orig_width, orig_height);
     return;
@@ -164,7 +123,8 @@ void WebRtcVideoCapturerAdapter::OnFrameCaptured(
   // If no scaling is needed, return a wrapped version of |frame| directly.
   if (video_frame->natural_size() == video_frame->visible_rect().size()) {
     OnFrame(cricket::WebRtcVideoFrame(
-                new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(video_frame),
+                new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(
+                    video_frame, copy_texture_callback_),
                 webrtc::kVideoRotation_0, translated_camera_time_us),
             orig_width, orig_height);
     return;
@@ -192,9 +152,130 @@ void WebRtcVideoCapturerAdapter::OnFrameCaptured(
                     adapted_width, adapted_height, libyuv::kFilterBilinear);
 
   OnFrame(cricket::WebRtcVideoFrame(
-              new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(scaled_frame),
+              new rtc::RefCountedObject<WebRtcVideoFrameAdapter>(
+                  scaled_frame, copy_texture_callback_),
               webrtc::kVideoRotation_0, translated_camera_time_us),
           orig_width, orig_height);
 }
 
+cricket::CaptureState WebRtcVideoCapturerAdapter::Start(
+    const cricket::VideoFormat& capture_format) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!running_);
+  DVLOG(3) << " WebRtcVideoCapturerAdapter::Start w = " << capture_format.width
+           << " h = " << capture_format.height;
+
+  running_ = true;
+  return cricket::CS_RUNNING;
+}
+
+void WebRtcVideoCapturerAdapter::Stop() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(3) << " WebRtcVideoCapturerAdapter::Stop ";
+  DCHECK(running_);
+  running_ = false;
+  SetCaptureFormat(NULL);
+  SignalStateChange(this, cricket::CS_STOPPED);
+}
+
+bool WebRtcVideoCapturerAdapter::IsRunning() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return running_;
+}
+
+bool WebRtcVideoCapturerAdapter::GetPreferredFourccs(
+    std::vector<uint32_t>* fourccs) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!fourccs || fourccs->empty());

[mcasas, 2016/10/26 16:06:02]

nit: not your code, but here we should not DCHECK() a 
case we're handling later.  Suggestion for this method:

if (!fourccs)
  return false;
DCHECK(fourccs->empty());
fourcss->push_back(cricket::FOURCC_I420);
return true;

[emircan, 2016/10/27 00:37:56]

Done.

+  if (fourccs)
+    fourccs->push_back(cricket::FOURCC_I420);
+  return fourccs != NULL;
+}
+
+bool WebRtcVideoCapturerAdapter::IsScreencast() const {
+  return is_screencast_;
+}
+
+bool WebRtcVideoCapturerAdapter::GetBestCaptureFormat(
+    const cricket::VideoFormat& desired,
+    cricket::VideoFormat* best_format) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(3) << " GetBestCaptureFormat:: "
+           << " w = " << desired.width
+           << " h = " << desired.height;

[mcasas, 2016/10/26 16:06:02]

nit:
DVLOG(3) << __func__ << " desired: " << desired.ToString();

here and in l.165;

[emircan, 2016/10/27 00:37:56]

Done.

+
+  // Capability enumeration is done in MediaStreamVideoSource. The adapter can
+  // just use what is provided.
+  // Use the desired format as the best format.
+  best_format->width = desired.width;
+  best_format->height = desired.height;
+  best_format->fourcc = cricket::FOURCC_I420;
+  best_format->interval = desired.interval;
+  return true;
+}
+
+void WebRtcVideoCapturerAdapter::CopyTextureFrame(
+    const scoped_refptr<media::VideoFrame>& frame,
+    scoped_refptr<media::VideoFrame>* new_frame) {
+  base::WaitableEvent waiter(base::WaitableEvent::ResetPolicy::MANUAL,
+                             base::WaitableEvent::InitialState::NOT_SIGNALED);
+  main_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&WebRtcVideoCapturerAdapter::CopyTextureFrameOnMainThread,
+                 base::Unretained(this), frame, new_frame, &waiter));

[mcasas, 2016/10/26 16:06:02]

weak_factory_.GetWeakPtr() ?

[emircan, 2016/10/27 00:37:56]

I can add a WeakPtr here to be used on main thread and invalidate on it. But
then, I cannot use the weakptr for the |copy_texture_callback_| that runs on
some rtc platform thread or invalidate.

I thought again about the design and decided to extract these copy methods to
another class TextureCopier which will be ref counted. That way we can pass the
class to callbacks safely. Let me know if you would prefer to move it to a
seperate file and/or directory.

+  waiter.Wait();
+}
+
+void WebRtcVideoCapturerAdapter::CopyTextureFrameOnMainThread(
+    const scoped_refptr<media::VideoFrame>& frame,
+    scoped_refptr<media::VideoFrame>* new_frame,
+    base::WaitableEvent* waiter) {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+  DCHECK(frame->format() == media::PIXEL_FORMAT_ARGB ||
+         frame->format() == media::PIXEL_FORMAT_XRGB ||
+         frame->format() == media::PIXEL_FORMAT_I420 ||
+         frame->format() == media::PIXEL_FORMAT_UYVY ||
+         frame->format() == media::PIXEL_FORMAT_NV12);
+  *new_frame = media::VideoFrame::CreateFrame(
+      media::PIXEL_FORMAT_I420, frame->coded_size(), frame->visible_rect(),
+      frame->natural_size(), frame->timestamp());
+
+  sk_sp<SkSurface> surface = SkSurface::MakeRasterN32Premul(
+      frame->visible_rect().width(), frame->visible_rect().height());
+
+  if (surface && provider_) {
+    DCHECK(provider_->ContextGL());
+    canvas_video_renderer_.Copy(
+        frame.get(), surface->getCanvas(),
+        media::Context3D(provider_->ContextGL(), provider_->GrContext()));
+  } else {
+    // Return a black frame (yuv = {0, 0x80, 0x80}).
+    *new_frame = media::VideoFrame::CreateColorFrame(
+        frame->visible_rect().size(), 0u, 0x80, 0x80, frame->timestamp());

[mcasas, 2016/10/26 16:06:02]

Shouldn't you return here (and in l.259)? Ah but then |waiter| 
would not be signalled. I looked around but there is no 
ScopedWaitableEvent that would Signal on destruction.
Consider adding a class

class ScopedWaitableEvent {
 public:
  explicit ScopedWaitableEvent(base::WaitableEvent* event) : event_(event) {}
  ~ScopedWaitableEvent() {
    if (event_)
      event_->Wait();
  }

 private:
  base::WaitableEvent* const event_;
};

using it to wrap |waiter| in l.239 after a
DCHECK(waiter), and then just return as usual in every
error path, removing l.277-278, WDYT?

[emircan, 2016/10/27 00:37:56]

Thanks. I added it to this file and restructured the conditionals accordingly.
It would be better to move it to base, but I came across to a lot of opposition
when trying to add to base earlier :(

+  }
+
+  SkPixmap pixmap;
+  const bool result = surface->getCanvas()->peekPixels(&pixmap);
+  DCHECK(result) << "Error trying to access SkSurface's pixels";
+
+  const uint32 source_pixel_format =
+      (kN32_SkColorType == kRGBA_8888_SkColorType) ? cricket::FOURCC_ABGR
+                                                   : cricket::FOURCC_ARGB;
+  libyuv::ConvertToI420(static_cast<const uint8*>(pixmap.addr(0, 0)),
+                        pixmap.getSafeSize64(),
+                        (*new_frame)->visible_data(media::VideoFrame::kYPlane),
+                        (*new_frame)->stride(media::VideoFrame::kYPlane),
+                        (*new_frame)->visible_data(media::VideoFrame::kUPlane),
+                        (*new_frame)->stride(media::VideoFrame::kUPlane),
+                        (*new_frame)->visible_data(media::VideoFrame::kVPlane),
+                        (*new_frame)->stride(media::VideoFrame::kVPlane),
+                        0 /* crop_x */, 0 /* crop_y */, pixmap.width(),
+                        pixmap.height(), (*new_frame)->visible_rect().width(),
+                        (*new_frame)->visible_rect().height(), libyuv::kRotate0,
+                        source_pixel_format);
+
+  if (waiter)
+    waiter->Signal();

[mcasas, 2016/10/26 16:06:02]

Is there a chance that |waiter| might be nullptr?
if not, just DCHECK() it (otherwise we would not
crash, but playback would hold up).
Edit: forget this in light of my comment in l.254.

[emircan, 2016/10/27 00:37:56]

Done.

+}
+
 }  // namespace content