Cleanup WebrtcVideoRendererAdapter

remoting/protocol/webrtc_video_renderer_adapter.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/protocol/webrtc_video_renderer_adapter.h"
 
 #include <memory>
 #include <utility>
 
 #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"
 #include "third_party/webrtc/modules/desktop_capture/desktop_frame.h"
 
 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)
     : media_stream_(std::move(media_stream)),
       frame_consumer_(frame_consumer),
       output_format_fourcc_(frame_consumer_->GetPixelFormat() ==
                                     FrameConsumer::FORMAT_BGRA
                                 ? libyuv::FOURCC_ARGB
                                 : libyuv::FOURCC_ABGR),
       task_runner_(base::ThreadTaskRunnerHandle::Get()),
@@ skipping 13 matching lines @@
 
 WebrtcVideoRendererAdapter::~WebrtcVideoRendererAdapter() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   webrtc::VideoTrackVector video_tracks = media_stream_->GetVideoTracks();
   DCHECK(!video_tracks.empty());
   video_tracks[0]->RemoveSink(this);
 }
 
 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())));
+  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.

+}
 
-  base::TimeDelta render_delay = std::max(
-      base::TimeDelta(), base::TimeDelta::FromMicroseconds(static_cast<float>(
-                             frame.timestamp_us() - rtc::TimeMicros())));
+void WebrtcVideoRendererAdapter::HandleFrameOnMainThread(
+    std::unique_ptr<cricket::VideoFrame> frame) {
+  DCHECK(task_runner_->BelongsToCurrentThread());
 
-  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(
-      FROM_HERE,
+  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(
     std::unique_ptr<webrtc::DesktopFrame> frame) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   frame_consumer_->DrawFrame(std::move(frame), base::Closure());
 }
 
 }  // namespace protocol
 }  // namespace remoting