Make RTCVideoEncoder usable from non-postable thread.

content/renderer/media/rtc_video_encoder.cc

Index: content/renderer/media/rtc_video_encoder.cc
diff --git a/content/renderer/media/rtc_video_encoder.cc b/content/renderer/media/rtc_video_encoder.cc
index c9406f1bfc97fbad675904eb5b1ec9cc0c25f0de..2eeb34821338c6d44c5f0eec6bf2d957048deb5f 100644
--- a/content/renderer/media/rtc_video_encoder.cc
+++ b/content/renderer/media/rtc_video_encoder.cc
@@ -103,8 +103,8 @@ class RTCVideoEncoder::Impl
     : public media::VideoEncodeAccelerator::Client,
       public base::RefCountedThreadSafe<RTCVideoEncoder::Impl> {
  public:
-  Impl(const base::WeakPtr<RTCVideoEncoder>& weak_encoder,
-       media::GpuVideoAcceleratorFactories* gpu_factories);
+  Impl(media::GpuVideoAcceleratorFactories* gpu_factories,
+       webrtc::VideoCodecType video_codec_type);
 
   // Create the VEA and call Initialize() on it.  Called once per instantiation,
   // and then the instance is bound forevermore to whichever thread made the
@@ -134,7 +134,7 @@ class RTCVideoEncoder::Impl
 
   // Destroy this Impl's encoder.  The destructor is not explicitly called, as
   // Impl is a base::RefCountedThreadSafe.
-  void Destroy();
+  void Destroy(base::WaitableEvent* waiter);
 
   // media::VideoEncodeAccelerator::Client implementation.
   void RequireBitstreamBuffers(unsigned int input_count,
@@ -145,6 +145,9 @@ class RTCVideoEncoder::Impl
                             bool key_frame) override;
   void NotifyError(media::VideoEncodeAccelerator::Error error) override;
 
+  void RegisterEncodeCompleteCallback(base::WaitableEvent* waiter,
+                                      webrtc::EncodedImageCallback* callback);
+
  private:
   friend class base::RefCountedThreadSafe<Impl>;
 
@@ -180,14 +183,12 @@ class RTCVideoEncoder::Impl
   // requirements.
   bool RequiresSizeChange(const scoped_refptr<media::VideoFrame>& frame) const;
 
-  base::ThreadChecker thread_checker_;
-
-  // Weak pointer to the parent RTCVideoEncoder, for posting back VEA::Client
-  // notifications.
-  const base::WeakPtr<RTCVideoEncoder> weak_encoder_;
+  // Return an encoded output buffer to WebRTC.
+  void ReturnEncodedImage(scoped_ptr<webrtc::EncodedImage> image,
+                          int32_t bitstream_buffer_id,
+                          uint16_t picture_id);
 
-  // The message loop on which to post callbacks to |weak_encoder_|.
-  const scoped_refptr<base::SingleThreadTaskRunner> encoder_task_runner_;
+  base::ThreadChecker thread_checker_;
 
   // Factory for creating VEAs, shared memory buffers, etc.
   media::GpuVideoAcceleratorFactories* gpu_factories_;
@@ -228,19 +229,30 @@ class RTCVideoEncoder::Impl
   // 15 bits running index of the VP8 frames. See VP8 RTP spec for details.
   uint16_t picture_id_;
 
+  // webrtc::VideoEncoder encode complete callback.
+  webrtc::EncodedImageCallback* encoded_image_callback_;
+
+  // The video codec type, as reported to WebRTC.
+  const webrtc::VideoCodecType video_codec_type_;
+
+  // If video encode accelerator posts an error, cache it here and return it
+  // the next time Enqueue is called.
+  int32_t status_;
+
   DISALLOW_COPY_AND_ASSIGN(Impl);
 };
 
-RTCVideoEncoder::Impl::Impl(const base::WeakPtr<RTCVideoEncoder>& weak_encoder,
-                            media::GpuVideoAcceleratorFactories* gpu_factories)
-    : weak_encoder_(weak_encoder),
-      encoder_task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      gpu_factories_(gpu_factories),
+RTCVideoEncoder::Impl::Impl(media::GpuVideoAcceleratorFactories* gpu_factories,
+                            webrtc::VideoCodecType video_codec_type)
+    : gpu_factories_(gpu_factories),
       async_waiter_(NULL),
       async_retval_(NULL),
       input_next_frame_(NULL),
       input_next_frame_keyframe_(false),
-      output_buffers_free_count_(0) {
+      output_buffers_free_count_(0),
+      encoded_image_callback_(nullptr),
+      video_codec_type_(video_codec_type),
+      status_(WEBRTC_VIDEO_CODEC_OK) {
   thread_checker_.DetachFromThread();
   // Picture ID should start on a random number.
   picture_id_ = static_cast<uint16_t>(base::RandInt(0, 0x7FFF));
@@ -268,6 +280,7 @@ void RTCVideoEncoder::Impl::CreateAndInitializeVEA(
     return;
   }
   input_visible_size_ = input_visible_size;
+
   if (!video_encoder_->Initialize(media::PIXEL_FORMAT_I420, input_visible_size_,
                                   profile, bitrate * 1000, this)) {
     LogAndNotifyError(FROM_HERE, "Error initializing video_encoder",
@@ -285,6 +298,9 @@ void RTCVideoEncoder::Impl::Enqueue(const webrtc::VideoFrame* input_frame,
   DCHECK(!input_next_frame_);
 
   RegisterAsyncWaiter(async_waiter, async_retval);
+  if (status_ != WEBRTC_VIDEO_CODEC_OK)
+    SignalAsyncWaiter(status_);

[kcwu, 2016/04/01 12:33:05]

return here?

[wuchengli, 2016/04/03 14:28:01]

Done.

+
   // If there are no free input and output buffers, drop the frame to avoid a
   // deadlock. If there is a free input buffer, EncodeOneFrame will run and
   // unblock Encode(). If there are no free input buffers but there is a free
@@ -344,10 +360,11 @@ void RTCVideoEncoder::Impl::RequestEncodingParametersChange(
     video_encoder_->RequestEncodingParametersChange(bitrate * 1000, framerate);
 }
 
-void RTCVideoEncoder::Impl::Destroy() {
+void RTCVideoEncoder::Impl::Destroy(base::WaitableEvent* waiter) {
   DVLOG(3) << "Impl::Destroy()";
   DCHECK(thread_checker_.CalledOnValidThread());
   video_encoder_.reset();
+  waiter->Signal();
 }
 
 void RTCVideoEncoder::Impl::RequireBitstreamBuffers(
@@ -441,10 +458,7 @@ void RTCVideoEncoder::Impl::BitstreamBufferReady(int32_t bitstream_buffer_id,
       (key_frame ? webrtc::kVideoFrameKey : webrtc::kVideoFrameDelta);
   image->_completeFrame = true;
 
-  encoder_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&RTCVideoEncoder::ReturnEncodedImage, weak_encoder_,
-                 base::Passed(&image), bitstream_buffer_id, picture_id_));
+  ReturnEncodedImage(std::move(image), bitstream_buffer_id, picture_id_);
   // Picture ID must wrap after reaching the maximum.
   picture_id_ = (picture_id_ + 1) & 0x7FFF;
 }
@@ -466,9 +480,7 @@ void RTCVideoEncoder::Impl::NotifyError(
   if (async_waiter_) {
     SignalAsyncWaiter(retval);
   } else {
-    encoder_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&RTCVideoEncoder::NotifyError, weak_encoder_, retval));
+    status_ = retval;

[kcwu, 2016/04/01 12:33:05]

In the original code, if any error occurs, RTCVideoEncoder will keep the error
code and destroy Impl.

With your change, Impl will be still alive after NotifyError. 
RTCVideoEncoder may still response OK for certain methods, ex. SetRates.

[wuchengli, 2016/04/03 14:28:01]

Moved impl_status_ to Impl. Now all methods will check Impl.status_.

   }
 }
 
@@ -599,21 +611,82 @@ bool RTCVideoEncoder::Impl::RequiresSizeChange(
           frame->visible_rect() != gfx::Rect(input_visible_size_));
 }
 
+void RTCVideoEncoder::Impl::RegisterEncodeCompleteCallback(
+    base::WaitableEvent* waiter,
+    webrtc::EncodedImageCallback* callback) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(3) << "RegisterEncodeCompleteCallback()";
+  encoded_image_callback_ = callback;
+  waiter->Signal();
+}
+
+void RTCVideoEncoder::Impl::ReturnEncodedImage(
+    scoped_ptr<webrtc::EncodedImage> image,

[pbos, 2016/03/31 09:47:21]

And this could be a const webrtc::EncodedImage&, no?

[wuchengli, 2016/04/01 10:19:51]

Yes. I'll update the code in the next patchset.

[wuchengli, 2016/04/03 14:28:01]

Done.

+    int32_t bitstream_buffer_id,
+    uint16_t picture_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DVLOG(3) << "ReturnEncodedImage(): "
+           << "bitstream_buffer_id=" << bitstream_buffer_id
+           << ", picture_id=" << picture_id;
+
+  if (!encoded_image_callback_)
+    return;
+
+  webrtc::RTPFragmentationHeader header;
+  memset(&header, 0, sizeof(header));
+  switch (video_codec_type_) {
+    case webrtc::kVideoCodecVP8:
+      // Generate a header describing a single fragment.
+      header.VerifyAndAllocateFragmentationHeader(1);
+      header.fragmentationOffset[0] = 0;
+      header.fragmentationLength[0] = image->_length;
+      header.fragmentationPlType[0] = 0;
+      header.fragmentationTimeDiff[0] = 0;
+      break;
+    case webrtc::kVideoCodecH264:
+      if (!GetRTPFragmentationHeaderH264(&header, image->_buffer,
+                                         image->_length)) {
+        DLOG(ERROR) << "Failed to get RTP fragmentation header for H264";
+        NotifyError(
+            (media::VideoEncodeAccelerator::Error)WEBRTC_VIDEO_CODEC_ERROR);
+        return;
+      }
+      break;
+    default:
+      NOTREACHED() << "Invalid video codec type";
+      return;
+  }
+
+  webrtc::CodecSpecificInfo info;
+  memset(&info, 0, sizeof(info));
+  info.codecType = video_codec_type_;
+  if (video_codec_type_ == webrtc::kVideoCodecVP8) {
+    info.codecSpecific.VP8.pictureId = picture_id;
+    info.codecSpecific.VP8.tl0PicIdx = -1;
+    info.codecSpecific.VP8.keyIdx = -1;
+  }
+
+  int32_t retval = encoded_image_callback_->Encoded(*image, &info, &header);
+  if (retval < 0) {
+    DVLOG(2) << "ReturnEncodedImage(): encoded_image_callback_ returned "
+             << retval;
+  }
+
+  UseOutputBitstreamBufferId(bitstream_buffer_id);
+}
+
 RTCVideoEncoder::RTCVideoEncoder(
     webrtc::VideoCodecType type,
     media::GpuVideoAcceleratorFactories* gpu_factories)
     : video_codec_type_(type),
       gpu_factories_(gpu_factories),
       gpu_task_runner_(gpu_factories->GetTaskRunner()),
-      encoded_image_callback_(NULL),
-      impl_status_(WEBRTC_VIDEO_CODEC_UNINITIALIZED),
-      weak_factory_(this) {
+      impl_status_(WEBRTC_VIDEO_CODEC_UNINITIALIZED) {

[kcwu, 2016/04/01 12:33:05]

impl_status_ becomes useless

[wuchengli, 2016/04/03 14:28:01]

Removed. Added Impl.status_.

   DVLOG(1) << "RTCVideoEncoder(): codec type=" << type;
 }
 
 RTCVideoEncoder::~RTCVideoEncoder() {
   DVLOG(3) << "~RTCVideoEncoder";
-  DCHECK(thread_checker_.CalledOnValidThread());
   Release();
   DCHECK(!impl_.get());
 }
@@ -625,14 +698,12 @@ int32_t RTCVideoEncoder::InitEncode(const webrtc::VideoCodec* codec_settings,
            << ", width=" << codec_settings->width
            << ", height=" << codec_settings->height
            << ", startBitrate=" << codec_settings->startBitrate;
-  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!impl_.get());
 
   const media::VideoCodecProfile profile =
       WebRTCVideoCodecToVideoCodecProfile(video_codec_type_, codec_settings);
 
-  weak_factory_.InvalidateWeakPtrs();
-  impl_ = new Impl(weak_factory_.GetWeakPtr(), gpu_factories_);
+  impl_ = new Impl(gpu_factories_, video_codec_type_);
   base::WaitableEvent initialization_waiter(true, false);
   int32_t initialization_retval = WEBRTC_VIDEO_CODEC_UNINITIALIZED;
   gpu_task_runner_->PostTask(
@@ -683,25 +754,30 @@ int32_t RTCVideoEncoder::Encode(
 int32_t RTCVideoEncoder::RegisterEncodeCompleteCallback(
     webrtc::EncodedImageCallback* callback) {
   DVLOG(3) << "RegisterEncodeCompleteCallback()";
-  DCHECK(thread_checker_.CalledOnValidThread());
   if (!impl_.get()) {
     DVLOG(3) << "RegisterEncodeCompleteCallback(): returning " << impl_status_;
     return impl_status_;
   }
 
-  encoded_image_callback_ = callback;
+  base::WaitableEvent encode_waiter(true, false);
+  gpu_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&RTCVideoEncoder::Impl::RegisterEncodeCompleteCallback, impl_,
+                 &encode_waiter, callback));
+  encode_waiter.Wait();
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
 int32_t RTCVideoEncoder::Release() {
   DVLOG(3) << "Release()";
-  DCHECK(thread_checker_.CalledOnValidThread());
 
   if (impl_.get()) {
-    gpu_task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&RTCVideoEncoder::Impl::Destroy, impl_));
+    base::WaitableEvent encode_waiter(true, false);
+    gpu_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&RTCVideoEncoder::Impl::Destroy, impl_, &encode_waiter));

[kcwu, 2016/04/01 12:33:05]

I'm wondering maybe we need to get async_retval here. This is the last chance
for client to know anything going wrong.

Consider this case:
1. client call ::Encode() and the frame is successfully enqueued.
2. something wrong in the encoder, it triggered NotifyError. But we just keep
the error in status_.
3. if the client doesn't call ::Encode again, the client never know something
wrong.

Anyway, this is just corner case, may not worth to handle. up to you.

[wuchengli, 2016/04/03 14:28:01]

I don't think we should return error in Release. Release should always succeed.
But I haven't looked at webrtc code to confirm.

+    encode_waiter.Wait();
     impl_ = NULL;
-    weak_factory_.InvalidateWeakPtrs();
     impl_status_ = WEBRTC_VIDEO_CODEC_UNINITIALIZED;
   }
   return WEBRTC_VIDEO_CODEC_OK;
@@ -732,75 +808,6 @@ int32_t RTCVideoEncoder::SetRates(uint32_t new_bit_rate, uint32_t frame_rate) {
   return WEBRTC_VIDEO_CODEC_OK;
 }
 
-void RTCVideoEncoder::ReturnEncodedImage(scoped_ptr<webrtc::EncodedImage> image,
-                                         int32_t bitstream_buffer_id,
-                                         uint16_t picture_id) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DVLOG(3) << "ReturnEncodedImage(): "
-           << "bitstream_buffer_id=" << bitstream_buffer_id
-           << ", picture_id=" << picture_id;
-
-  if (!encoded_image_callback_)
-    return;
-
-  webrtc::RTPFragmentationHeader header;
-  memset(&header, 0, sizeof(header));
-  switch (video_codec_type_) {
-    case webrtc::kVideoCodecVP8:
-      // Generate a header describing a single fragment.
-      header.VerifyAndAllocateFragmentationHeader(1);
-      header.fragmentationOffset[0] = 0;
-      header.fragmentationLength[0] = image->_length;
-      header.fragmentationPlType[0] = 0;
-      header.fragmentationTimeDiff[0] = 0;
-      break;
-    case webrtc::kVideoCodecH264:
-      if (!GetRTPFragmentationHeaderH264(
-          &header, image->_buffer, image->_length)) {
-        DLOG(ERROR) << "Failed to get RTP fragmentation header for H264";
-        NotifyError(WEBRTC_VIDEO_CODEC_ERROR);
-        return;
-      }
-      break;
-    default:
-      NOTREACHED() << "Invalid video codec type";
-      return;
-  }
-
-  webrtc::CodecSpecificInfo info;
-  memset(&info, 0, sizeof(info));
-  info.codecType = video_codec_type_;
-  if (video_codec_type_ == webrtc::kVideoCodecVP8) {
-    info.codecSpecific.VP8.pictureId = picture_id;
-    info.codecSpecific.VP8.tl0PicIdx = -1;
-    info.codecSpecific.VP8.keyIdx = -1;
-  }
-
-  int32_t retval = encoded_image_callback_->Encoded(*image, &info, &header);
-  if (retval < 0) {
-    DVLOG(2) << "ReturnEncodedImage(): encoded_image_callback_ returned "
-             << retval;
-  }
-
-  // The call through webrtc::EncodedImageCallback is synchronous, so we can
-  // immediately recycle the output buffer back to the Impl.
-  gpu_task_runner_->PostTask(
-      FROM_HERE,
-      base::Bind(&RTCVideoEncoder::Impl::UseOutputBitstreamBufferId,
-                 impl_,
-                 bitstream_buffer_id));
-}
-
-void RTCVideoEncoder::NotifyError(int32_t error) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DVLOG(1) << "NotifyError(): error=" << error;
-
-  impl_status_ = error;
-  gpu_task_runner_->PostTask(FROM_HERE,
-                         base::Bind(&RTCVideoEncoder::Impl::Destroy, impl_));
-  impl_ = NULL;
-}
-
 void RTCVideoEncoder::RecordInitEncodeUMA(
     int32_t init_retval, media::VideoCodecProfile profile) {
   UMA_HISTOGRAM_BOOLEAN("Media.RTCVideoEncoderInitEncodeSuccess",