Cleanup WebrtcVideoRendererAdapter

remoting/protocol/webrtc_video_renderer_adapter.cc

Index: remoting/protocol/webrtc_video_renderer_adapter.cc
diff --git a/remoting/protocol/webrtc_video_renderer_adapter.cc b/remoting/protocol/webrtc_video_renderer_adapter.cc
index a1a892e8583589d00bd7191c4c0dbb17b13f9235..0f3ed5c56c07dd8d8cd96e6daeba9aac34ddd4dd 100644
--- a/remoting/protocol/webrtc_video_renderer_adapter.cc
+++ b/remoting/protocol/webrtc_video_renderer_adapter.cc
@@ -10,8 +10,11 @@
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/location.h"
+#include "base/memory/ptr_util.h"
 #include "base/single_thread_task_runner.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/threading/worker_pool.h"
 #include "remoting/protocol/frame_consumer.h"
 #include "third_party/libyuv/include/libyuv/video_common.h"
 #include "third_party/webrtc/media/base/videoframe.h"
@@ -20,6 +23,23 @@
 namespace remoting {
 namespace protocol {
 
+namespace {
+
+std::unique_ptr<webrtc::DesktopFrame> ConvertYuvToRgb(
+    std::unique_ptr<cricket::VideoFrame> yuv_frame,
+    std::unique_ptr<webrtc::DesktopFrame> rgb_frame,
+    uint32_t format) {
+  yuv_frame->ConvertToRgbBuffer(
+      format, rgb_frame->data(),
+      std::abs(rgb_frame->stride()) * rgb_frame->size().height(),
+      rgb_frame->stride());
+  rgb_frame->mutable_updated_region()->AddRect(
+      webrtc::DesktopRect::MakeSize(rgb_frame->size()));
+  return rgb_frame;
+}
+
+}  // namespace
+
 WebrtcVideoRendererAdapter::WebrtcVideoRendererAdapter(
     scoped_refptr<webrtc::MediaStreamInterface> media_stream,
     FrameConsumer* frame_consumer)
@@ -53,29 +73,33 @@ WebrtcVideoRendererAdapter::~WebrtcVideoRendererAdapter() {
 }
 
 void WebrtcVideoRendererAdapter::OnFrame(const cricket::VideoFrame& frame) {
-  // TODO(sergeyu): WebRTC calls OnFrame on a separate thread it creates.
-  // FrameConsumer normally expects to be called on the network thread, so we
-  // cannot call FrameConsumer::AllocateFrame() here and instead
-  // BasicDesktopFrame is created directly. This will not work correctly with
-  // all FrameConsumer implementations. Fix this somehow.
-  std::unique_ptr<webrtc::DesktopFrame> rgb_frame(new webrtc::BasicDesktopFrame(
-      webrtc::DesktopSize(frame.width(), frame.height())));
-
-  base::TimeDelta render_delay = std::max(
-      base::TimeDelta(), base::TimeDelta::FromMicroseconds(static_cast<float>(
-                             frame.timestamp_us() - rtc::TimeMicros())));
-
-  frame.ConvertToRgbBuffer(
-      output_format_fourcc_, rgb_frame->data(),
-      std::abs(rgb_frame->stride()) * rgb_frame->size().height(),
-      rgb_frame->stride());
-  rgb_frame->mutable_updated_region()->AddRect(
-      webrtc::DesktopRect::MakeSize(rgb_frame->size()));
-  task_runner_->PostDelayedTask(
+  task_runner_->PostTask(
       FROM_HERE,
+      base::Bind(&WebrtcVideoRendererAdapter::HandleFrameOnMainThread,
+                 weak_factory_.GetWeakPtr(),
+                 base::Passed(base::WrapUnique(frame.Copy()))));

[Jamie, 2016/06/13 21:22:51]

Is the cost of this copy significant?

[Sergey Ulanov, 2016/06/13 22:34:55]

No. It doesn't copy the buffer.

[nisse-chromium (ooo August 14), 2016/06/21 11:40:02]

Hi, I'm trying to delete this Copy method (cl
https://codereview.webrtc.org/2080253002/, which I just had to revert). 

I thought I had removed the last use in Chrome in
https://codereview.chromium.org/2068703002/, but it seems I was wrong...

After a first look, it seems like in this case, where you don't care about the
timestamp and rotation info in the frame, you could do the timestamp check in
OnFrame, and only pass on frame.video_frame_buffer() (a ref-counted object) to
PostTask. Except that the ConvertToRgbBuffer method then isn't available.

I'm about to leave for some summer vacation, but perhaps you and perkj@ can sort
out the right way to handle this?

I think VideoFrame::ConvertToRgbBuffer is another method we'd like to
move/delete. Maybe a static helper method on VideoFrameBuffer would be more
appropriate.

[Sergey Ulanov, 2016/06/21 17:38:22]

Copy() isn't marked as deprecated, so I assumed it's safe to use. Anyway, this
will be easy to fix.

+}
+
+void WebrtcVideoRendererAdapter::HandleFrameOnMainThread(
+    std::unique_ptr<cricket::VideoFrame> frame) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  std::unique_ptr<webrtc::DesktopFrame> rgb_frame =
+      frame_consumer_->AllocateFrame(
+          webrtc::DesktopSize(frame->width(), frame->height()));
+
+  if (static_cast<uint64_t>(frame->timestamp_us()) >= rtc::TimeMicros()) {
+    // The host sets playout delay to 0, so all incoming frames are expected to
+    // be rendered as so as they are received.
+    LOG(WARNING) << "Received frame with playout delay greater than 0.";

[Jamie, 2016/06/13 21:22:51]

If this is never expected to happen, maybe it should be a DCHECK instead?

[Sergey Ulanov, 2016/06/13 22:34:55]

It's not supposed to happen, but it depends on the host, which is the other end
of the connection, so DCHECK() wouldn't be appropriate here. (in the future we
may relax playout delay, e.g. when detecting video playback, to make it
smoother).

+  }
+
+  base::PostTaskAndReplyWithResult(
+      base::WorkerPool::GetTaskRunner(false).get(), FROM_HERE,
+      base::Bind(&ConvertYuvToRgb, base::Passed(&frame),
+                 base::Passed(&rgb_frame), output_format_fourcc_),
       base::Bind(&WebrtcVideoRendererAdapter::DrawFrame,
-                 weak_factory_.GetWeakPtr(), base::Passed(&rgb_frame)),
-      render_delay);
+                 weak_factory_.GetWeakPtr()));
 }
 
 void WebrtcVideoRendererAdapter::DrawFrame(