Move VideoDestinationHandler processing to the IO-thread.

content/renderer/media/webrtc/video_destination_handler.cc

Index: content/renderer/media/webrtc/video_destination_handler.cc
diff --git a/content/renderer/media/webrtc/video_destination_handler.cc b/content/renderer/media/webrtc/video_destination_handler.cc
index 02dfe3284a69e4903a986bf8151fa77e59f12d58..04acbf4564039a7c2403409731b58f46c01e7c30 100644
--- a/content/renderer/media/webrtc/video_destination_handler.cc
+++ b/content/renderer/media/webrtc/video_destination_handler.cc
@@ -29,112 +29,97 @@ class PpFrameWriter::FrameWriterDelegate
     : public base::RefCountedThreadSafe<FrameWriterDelegate> {
  public:
   FrameWriterDelegate(
-      const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy,
-      const VideoCaptureDeliverFrameCB& new_frame_callback);
+      const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy);
+
+  // Starts forwarding frames to |frame_callback| on the  IO-thread that are
+  // delivered to this class by calling DeliverFrame on the main render thread.
+  void StartDeliver(const VideoCaptureDeliverFrameCB& frame_callback);
+  void StopDeliver();
+
+  void DeliverFrame(const scoped_refptr<PPB_ImageData_Impl>& image_data,
+                    int64 time_stamp_ns);
 
-  void DeliverFrame(const scoped_refptr<media::VideoFrame>& frame,
-                    const media::VideoCaptureFormat& format);
  private:
   friend class base::RefCountedThreadSafe<FrameWriterDelegate>;
   virtual ~FrameWriterDelegate();
 
-  void DeliverFrameOnIO(const scoped_refptr<media::VideoFrame>& frame,
-                        const media::VideoCaptureFormat& format);
+  void DeliverFrameOnIO(const VideoCaptureDeliverFrameCB& frame_callback,
+                        uint8* data, int stride, int width, int height,
+                        int64 time_stamp_ns);
+  void FrameDelivered(const scoped_refptr<PPB_ImageData_Impl>& image_data);
 
   scoped_refptr<base::MessageLoopProxy> io_message_loop_;
   VideoCaptureDeliverFrameCB new_frame_callback_;
+
+  // |frame_pool_| is only used on the IO-thread.
+  media::VideoFramePool frame_pool_;
+
+  // Used to DCHECK that we are called on the main render thread.
+  base::ThreadChecker thread_checker_;
 };
 
 PpFrameWriter::FrameWriterDelegate::FrameWriterDelegate(
-    const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy,
-    const VideoCaptureDeliverFrameCB& new_frame_callback)
-    : io_message_loop_(io_message_loop_proxy),
-      new_frame_callback_(new_frame_callback) {
+    const scoped_refptr<base::MessageLoopProxy>& io_message_loop_proxy)
+    : io_message_loop_(io_message_loop_proxy) {
 }
 
 PpFrameWriter::FrameWriterDelegate::~FrameWriterDelegate() {
 }
 
-void PpFrameWriter::FrameWriterDelegate::DeliverFrame(
-    const scoped_refptr<media::VideoFrame>& frame,
-    const media::VideoCaptureFormat& format) {
-  io_message_loop_->PostTask(
-      FROM_HERE,
-      base::Bind(&FrameWriterDelegate::DeliverFrameOnIO,
-                 this, frame, format));
-}
-
-void PpFrameWriter::FrameWriterDelegate::DeliverFrameOnIO(
-     const scoped_refptr<media::VideoFrame>& frame,
-     const media::VideoCaptureFormat& format) {
-  DCHECK(io_message_loop_->BelongsToCurrentThread());
-  // The local time when this frame is generated is unknown so give a null
-  // value to |estimated_capture_time|.
-  new_frame_callback_.Run(frame, format, base::TimeTicks());
-}
-
-PpFrameWriter::PpFrameWriter() {
-  DVLOG(3) << "PpFrameWriter ctor";
-}
-
-PpFrameWriter::~PpFrameWriter() {
-  DVLOG(3) << "PpFrameWriter dtor";
-}
-
-void PpFrameWriter::GetCurrentSupportedFormats(
-    int max_requested_width,
-    int max_requested_height,
-    double max_requested_frame_rate,
-    const VideoCaptureDeviceFormatsCB& callback) {
-  DCHECK(CalledOnValidThread());
-  DVLOG(3) << "PpFrameWriter::GetCurrentSupportedFormats()";
-  // Since the input is free to change the resolution at any point in time
-  // the supported formats are unknown.
-  media::VideoCaptureFormats formats;
-  callback.Run(formats);
-}
-
-void PpFrameWriter::StartSourceImpl(
-    const media::VideoCaptureFormat& format,
+void PpFrameWriter::FrameWriterDelegate::StartDeliver(
     const VideoCaptureDeliverFrameCB& frame_callback) {
-  DCHECK(CalledOnValidThread());
-  DCHECK(!delegate_.get());
-  DVLOG(3) << "PpFrameWriter::StartSourceImpl()";
-  delegate_ = new FrameWriterDelegate(io_message_loop(), frame_callback);
-  OnStartDone(MEDIA_DEVICE_OK);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  new_frame_callback_ = frame_callback;
 }
 
-void PpFrameWriter::StopSourceImpl() {
-  DCHECK(CalledOnValidThread());
+void PpFrameWriter::FrameWriterDelegate::StopDeliver() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  new_frame_callback_.Reset();
 }
 
-void PpFrameWriter::PutFrame(PPB_ImageData_Impl* image_data,
-                             int64 time_stamp_ns) {
-  DCHECK(CalledOnValidThread());
-  TRACE_EVENT0("video", "PpFrameWriter::PutFrame");
-  DVLOG(3) << "PpFrameWriter::PutFrame()";
-
-  if (!image_data) {
-    LOG(ERROR) << "PpFrameWriter::PutFrame - Called with NULL image_data.";
+void PpFrameWriter::FrameWriterDelegate::DeliverFrame(
+    const scoped_refptr<PPB_ImageData_Impl>& image_data,
+    int64 time_stamp_ns) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  TRACE_EVENT0("video", "PpFrameWriter::FrameWriterDelegate::DeliverFrame");
+  if (new_frame_callback_.is_null())
     return;
-  }
-  ImageDataAutoMapper mapper(image_data);
-  if (!mapper.is_valid()) {
+  if (!image_data->Map()) {
     LOG(ERROR) << "PpFrameWriter::PutFrame - "
                << "The image could not be mapped and is unusable.";
     return;
   }
+
   const SkBitmap* bitmap = image_data->GetMappedBitmap();
   if (!bitmap) {
     LOG(ERROR) << "PpFrameWriter::PutFrame - "
                << "The image_data's mapped bitmap is NULL.";
     return;
   }
+  io_message_loop_->PostTaskAndReply(
+      FROM_HERE,
+      base::Bind(&FrameWriterDelegate::DeliverFrameOnIO, this,
+                 new_frame_callback_,

[tommi (sloooow) - chröme, 2014/10/08 09:11:24]

Are you sending a pointer to the callback to the IO thread?
It looks like the callback is otherwise only accessed on this thread
(thread_checker_) and there's no synchronization, so we would need to send a
copy here I think unless there's some magic happening here that I'm missing.

Alternatively, could the callback be touched only on the IO thread?

[perkj_chrome, 2014/10/08 12:20:18]

Changed so that StartDeliver post the cb to the io thread. 

+                 static_cast<uint8*>(bitmap->getPixels()),
+                 bitmap->rowBytes(),
+                 bitmap->width(),
+                 bitmap->height(),
+                 time_stamp_ns),
+      base::Bind(&FrameWriterDelegate::FrameDelivered, this,
+                 image_data));
+}
 
-  const gfx::Size frame_size(bitmap->width(), bitmap->height());
+void PpFrameWriter::FrameWriterDelegate::FrameDelivered(
+    const scoped_refptr<PPB_ImageData_Impl>& image_data) {
+  image_data->Unmap();

[tommi (sloooow) - chröme, 2014/10/08 09:11:24]

should there be a thread check in this function?

[perkj_chrome, 2014/10/08 12:20:18]

Done.

+}
 
-  if (state() != MediaStreamVideoSource::STARTED)
-    return;
+void PpFrameWriter::FrameWriterDelegate::DeliverFrameOnIO(
+    const VideoCaptureDeliverFrameCB& frame_callback,
+    uint8* data, int stride, int width, int height, int64 time_stamp_ns) {
+  DCHECK(io_message_loop_->BelongsToCurrentThread());
+  TRACE_EVENT0("video", "PpFrameWriter::FrameWriterDelegate::DeliverFrameOnIO");
+  const gfx::Size frame_size(width, height);
 
   const base::TimeDelta timestamp = base::TimeDelta::FromMicroseconds(
       time_stamp_ns / base::Time::kNanosecondsPerMicrosecond);
@@ -151,8 +136,8 @@ void PpFrameWriter::PutFrame(PPB_ImageData_Impl* image_data,
       MediaStreamVideoSource::kUnknownFrameRate,
       media::PIXEL_FORMAT_YV12);
 
-  libyuv::BGRAToI420(reinterpret_cast<uint8*>(bitmap->getPixels()),
-                     bitmap->rowBytes(),
+  libyuv::BGRAToI420(data,
+                     stride,
                      new_frame->data(media::VideoFrame::kYPlane),
                      new_frame->stride(media::VideoFrame::kYPlane),
                      new_frame->data(media::VideoFrame::kUPlane),
@@ -161,36 +146,58 @@ void PpFrameWriter::PutFrame(PPB_ImageData_Impl* image_data,
                      new_frame->stride(media::VideoFrame::kVPlane),
                      frame_size.width(), frame_size.height());
 
-  delegate_->DeliverFrame(new_frame, format);
+  // The local time when this frame is generated is unknown so give a null
+  // value to |estimated_capture_time|.
+  frame_callback.Run(new_frame, format, base::TimeTicks());
 }
 
-// PpFrameWriterProxy is a helper class to make sure the user won't use
-// PpFrameWriter after it is released (IOW its owner - WebMediaStreamSource -
-// is released).
-class PpFrameWriterProxy : public FrameWriterInterface {
- public:
-  explicit PpFrameWriterProxy(const base::WeakPtr<PpFrameWriter>& writer)
-      : writer_(writer) {
-    DCHECK(writer_ != NULL);
-  }
+PpFrameWriter::PpFrameWriter() {
+  DVLOG(3) << "PpFrameWriter ctor";
+  delegate_ = new FrameWriterDelegate(io_message_loop());

[tommi (sloooow) - chröme, 2014/10/08 09:11:24]

initializer list?

[perkj_chrome, 2014/10/08 12:20:18]

I guess I can but I have a vague memory that you didnt like calling a base class
method in an initializer list?    io_message_loop()

[tommi (sloooow) - chröme, 2014/10/08 14:50:36]

oh, delegate_ is a base class member.  If so, leave as is.

+}
 
-  virtual ~PpFrameWriterProxy() {}
+PpFrameWriter::~PpFrameWriter() {
+  DVLOG(3) << "PpFrameWriter dtor";
+}
 
-  virtual void PutFrame(PPB_ImageData_Impl* image_data,
-                        int64 time_stamp_ns) OVERRIDE {
-    writer_->PutFrame(image_data, time_stamp_ns);
-  }
+VideoDestinationHandler::FrameWriterCallback
+PpFrameWriter::GetFrameWriterCallback() {
+  DCHECK(CalledOnValidThread());
+  return base::Bind(&PpFrameWriter::FrameWriterDelegate::DeliverFrame,
+                    delegate_);
+}
 
- private:
-  base::WeakPtr<PpFrameWriter> writer_;
+void PpFrameWriter::GetCurrentSupportedFormats(
+    int max_requested_width,
+    int max_requested_height,
+    double max_requested_frame_rate,
+    const VideoCaptureDeviceFormatsCB& callback) {
+  DCHECK(CalledOnValidThread());
+  DVLOG(3) << "PpFrameWriter::GetCurrentSupportedFormats()";
+  // Since the input is free to change the resolution at any point in time
+  // the supported formats are unknown.
+  media::VideoCaptureFormats formats;
+  callback.Run(formats);
+}
 
-  DISALLOW_COPY_AND_ASSIGN(PpFrameWriterProxy);
-};
+void PpFrameWriter::StartSourceImpl(
+    const media::VideoCaptureFormat& format,
+    const VideoCaptureDeliverFrameCB& frame_callback) {
+  DCHECK(CalledOnValidThread());
+  DVLOG(3) << "PpFrameWriter::StartSourceImpl()";
+  delegate_->StartDeliver(frame_callback);
+  OnStartDone(MEDIA_DEVICE_OK);
+}
+
+void PpFrameWriter::StopSourceImpl() {
+  DCHECK(CalledOnValidThread());
+  delegate_->StopDeliver();
+}
 
 bool VideoDestinationHandler::Open(
     MediaStreamRegistryInterface* registry,
     const std::string& url,
-    FrameWriterInterface** frame_writer) {
+    FrameWriterCallback* frame_writer) {
   DVLOG(3) << "VideoDestinationHandler::Open";
   blink::WebMediaStream stream;
   if (registry) {
@@ -214,6 +221,7 @@ bool VideoDestinationHandler::Open(
   base::Base64Encode(base::RandBytesAsString(64), &track_id);
 
   PpFrameWriter* writer = new PpFrameWriter();
+  *frame_writer = writer->GetFrameWriterCallback();
 
   // Create a new webkit video track.
   blink::WebMediaStreamSource webkit_source;
@@ -231,7 +239,6 @@ bool VideoDestinationHandler::Open(
       writer, constraints, MediaStreamVideoSource::ConstraintsCallback(),
       track_enabled));
 
-  *frame_writer = new PpFrameWriterProxy(writer->AsWeakPtr());
   return true;
 }