Integrate VDA with WebRTC.

content/renderer/media/rtc_video_decoder.cc

+// Copyright (c) 2013 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 "content/renderer/media/rtc_video_decoder.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/message_loop_proxy.h"
+#include "content/renderer/media/native_handle_impl.h"
+#include "media/base/audio_decoder_config.h"
+#include "media/base/bind_to_loop.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/decoder_buffer_queue.h"
+#include "media/base/demuxer_stream.h"
+#include "media/base/video_decoder_config.h"
+#include "third_party/webrtc/system_wrappers/interface/ref_count.h"
+
+namespace content {
+
+class RTCDemuxerStream
+    : public media::DemuxerStream,
+      public base::SupportsWeakPtr<RTCDemuxerStream> {

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Exposing SupportsWeakPtr is a code smell.
In this case it's also unnecessary, because the only user of the facility is the
owner of this class (RTCVideoDecoder) who can maintain its own weakptr for it!

[wuchengli, 2013/05/15 15:30:46]

Removed SupportsWeakPtr. I added new methods in RTCVideoDecoder to call
RTCDemuxerStream::UpdateSize and QueueBuffer. Let me know if you have other
suggestions.

+ public:
+  RTCDemuxerStream(const scoped_refptr<base::MessageLoopProxy>& message_loop);

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Missing (virtual) dtor.
The dtor should maybe dispatch pending read_cb_.

+  // media::DemuxerStream implementation.
+  virtual void Read(const ReadCB& read_cb) OVERRIDE;
+  virtual const media::AudioDecoderConfig& audio_decoder_config() OVERRIDE;
+  virtual const media::VideoDecoderConfig& video_decoder_config() OVERRIDE;
+  virtual Type type() OVERRIDE;
+  virtual void EnableBitstreamConverter() OVERRIDE;
+
+  void UpdateSize(gfx::Size size);
+  void QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer);

[Pawel Osciak, 2013/05/15 17:26:32]

Documentation?

+  void ClearBuffer();

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Pluralize?

[wuchengli, 2013/05/15 15:30:46]

Done.

+
+ private:
+  media::DecoderBufferQueue buffer_queue_;
+  ReadCB read_cb_;
+  // Make sure all the methods are called by the same thread.
+  scoped_refptr<base::MessageLoopProxy> message_loop_;

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Since this is all you use it for, you should drop it from here and the ctor, and
instead use a member base::ThreadChecker instead.

+  media::VideoDecoderConfig video_decoder_config_;
+};

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

DISALLOW_COPY_AND_ASSIGN?

[wuchengli, 2013/05/15 15:30:46]

Done.

+
+RTCDemuxerStream::RTCDemuxerStream(
+    const scoped_refptr<base::MessageLoopProxy>& message_loop)
+    : message_loop_(message_loop) {
+}
+
+void RTCDemuxerStream::Read(const ReadCB& read_cb) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  CHECK(read_cb_.is_null());
+  read_cb_ = read_cb;
+
+  if(!buffer_queue_.IsEmpty()) {
+    base::ResetAndReturn(&read_cb_).Run(
+        DemuxerStream::kOk, buffer_queue_.Pop());
+  }
+}
+
+const media::AudioDecoderConfig& RTCDemuxerStream::audio_decoder_config() {
+  LOG(FATAL) << "Audio is not supported.";
+  return *(new media::AudioDecoderConfig);

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

return media::AudioDecoderConfig();
doesn't work?

[wuchengli, 2013/05/15 15:30:46]

Right. The compile error is "error: returning reference to temporary".

[Pawel Osciak, 2013/05/15 17:26:32]

Isn't this a leak? Isn't this class supposed to maintain ownership of created
config? Perhaps a dummy empty config in this class?

[wuchengli, 2013/05/16 16:05:45]

LOG(FATAL) will cause the program to terminate. This line will not be executed.
The original patchset uses a dummy config. I can change it back.

@Ami: The current patchset uses LOG(FATAL) and it doesn't remove the need of
returning an object.

+}
+
+const media::VideoDecoderConfig& RTCDemuxerStream::video_decoder_config() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  return video_decoder_config_;
+}
+
+media::DemuxerStream::Type RTCDemuxerStream::type() {
+  return media::DemuxerStream::VIDEO;
+}
+
+void RTCDemuxerStream::EnableBitstreamConverter() {
+  NOTREACHED();

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Why NOTREACHED here but LOG(FATAL) at l.64?

[wuchengli, 2013/05/15 15:30:46]

Agree. I changed NOTREACHED to LOG(FATAL). I just found NOTREACHED is DCHECK,
not CHECK.

+}
+
+void RTCDemuxerStream::UpdateSize(gfx::Size size) {
+  DCHECK(message_loop_->BelongsToCurrentThread());

[Pawel Osciak, 2013/05/15 17:26:32]

Did size update work for you with VDAs if posted from Decode()?

[wuchengli, 2013/05/16 16:05:45]

UpdateSize is posted from Decode() now. You mean if I changed to use
VideoDecodeAccelerator or?

+  DVLOG(2) << "Update config size to " << size.width() << "," << size.height();
+  gfx::Rect rect(size);
+  video_decoder_config_.Initialize(
+      media::kCodecVP8,
+      media::VP8PROFILE_MAIN,
+      media::VideoFrame::NATIVE_TEXTURE,
+      size, rect, size, NULL, 0, false, false);
+}
+
+void RTCDemuxerStream::QueueBuffer(
+    scoped_refptr<media::DecoderBuffer> buffer) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DVLOG(3) << "QueueBuffer";
+  buffer_queue_.Push(buffer);
+
+  if (!read_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(
+        DemuxerStream::kOk, buffer_queue_.Pop());
+  }
+  DVLOG(3) << "QueueBuffer end";
+}
+
+void RTCDemuxerStream::ClearBuffer() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  buffer_queue_.Clear();
+}
+
+RTCVideoDecoder::RTCVideoDecoder(
+    media::VideoDecoder* video_decoder,
+    const scoped_refptr<base::MessageLoopProxy>& message_loop)
+    : video_decoder_(video_decoder),
+      decoder_message_loop_(message_loop),
+      decode_complete_callback_(NULL),
+      pipeline_status_(media::PIPELINE_OK),
+      decoder_waiter_(false, false),
+      stream_(new RTCDemuxerStream(message_loop)),
+      state_(kUninitialized),
+      decoding_error_occurred_(false),
+      weak_factory_(this) {
+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&RTCVideoDecoder::InitWeakPtr, base::Unretained(this)));
+  decoder_waiter_.Wait();
+}
+
+void RTCVideoDecoder::InitWeakPtr() {

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

s/Ptr/Ptrs/

[wuchengli, 2013/05/15 15:30:46]

Done.

+  DCHECK(decoder_message_loop_->BelongsToCurrentThread());
+  weak_this_ = weak_factory_.GetWeakPtr();
+  weak_stream_ = stream_->AsWeakPtr();
+  decoder_waiter_.Signal();

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

What happens if the decoder thread is stopped before this task gets to execute? 

You may enjoy reading through
https://code.google.com/p/chromium/issues/detail?id=179551

+}
+
+RTCVideoDecoder::~RTCVideoDecoder() {
+}
+

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

You should 
DCHECK(decoder_message_loop_->BelongsToCurrentThread());
in the dtor since you're relying on the weak ptrs being invalidated there.

[wuchengli, 2013/05/15 15:30:46]

Done. Great suggestion.

+int32_t RTCVideoDecoder::InitDecode(
+    const webrtc::VideoCodec* codecSettings,
+    int32_t /*numberOfCores*/) {
+  DVLOG(2) << "InitDecode";
+  {
+    base::AutoLock auto_lock(lock_);
+    if (state_ != kUninitialized) {
+      LOG(ERROR) << "state_ != kUninitialized!";
+      return WEBRTC_VIDEO_CODEC_ERROR;
+    }
+  }
+  DCHECK(codecSettings->codecType == webrtc::kVideoCodecVP8);

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

DCHECK_EQ

[wuchengli, 2013/05/15 15:30:46]

Done.

+  if (codecSettings->codecSpecific.VP8.feedbackModeOn) {
+    LOG(ERROR) << "Feedback mode not supported";
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
+  frame_size_.SetSize(codecSettings->width, codecSettings->height);

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Didn't the internal thread result in saying these are the container's
dimensions, which may not be the coded size at all?  I think you need the
latter.  If you were using GVDAH directly you'd get to see what size the VDA
found in the bitstream, which would be the definitive place to know the coded
size.

[Pawel Osciak, 2013/05/15 17:26:32]

How does frame_size_ relate to visible size? Do we assume it's mb-aligned or
don't care?

[wuchengli, 2013/05/16 16:05:45]

@Ami: UpdateSize is needed here because GpuVideoDecoder::Initialize has a DCHECK
against (0,0) size VideoDecoderConfig. We need to set the size or move
GpuVideoDecoder::Initialize to Decode(). Using GVDAH is another story. I'll
study how to use it.

@Pawel: See the following discussion from an internal thread. Frame size in
Decode should be the same as visible size. Does GpuVideoDecoder have any
requirement about visible size? I don't see mb-aligned is required.


- VideoDecoder::InitDecode has parameter codecSettings. VideoDecoder::Decode has
parameter inputFrame. What's the relationship between
codecSettings->width/height and inputFrame._encodedWidth/_encodedHeight?

The width and height provided in codecSettings can be ignored. They are there
for codecs which doesn't have width/height in the bitstream.

- To use GpuVideoDecoder, I need to create a VideoDecoderConfig, whose
constructor needs coded_size, visible_rect, and natural_size. Are
EncodedImage._encodedWidth/_encodedHeight always multiples of 16? Is it correct
to use coded_size=visible_rect=natural_size=(_enocdedWidth,_encodedHeight)? I
took a quick look at VP8 data format. Maybe _encodedWidth and _encodedHeight are
actual image dimension and not necessarily evenly divisible by 16?

encodedWidth/Height are parsed from the vp8 bitstream and should represent the
actual image dimensions. Seems to make sense that those can be set to
(encodedWidth, encodedHeight), although it's not completely clear to me what
they represent.

+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&RTCDemuxerStream::UpdateSize, weak_stream_, frame_size_));
+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&media::VideoDecoder::Initialize,
+                 base::Unretained(video_decoder_.get()),

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Why is this (and the next) Unretained safe?

[wuchengli, 2013/05/16 16:05:45]

Unretained is safe because the WaitableEvent next line will wait until it
finishes.

+                 base::Unretained(stream_.get()),
+                 base::Bind(&RTCVideoDecoder::OnUpdatePipelineStatus,
+                            weak_this_),
+                 base::Bind(&RTCVideoDecoder::OnUpdateStatistics, weak_this_)));
+  decoder_waiter_.Wait();
+  if (pipeline_status_ != media::PIPELINE_OK) {

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Shouldn't this be protected by lock_?

+    LOG(ERROR) << "Initialize failed. pipeline_status_=" << pipeline_status_;
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t RTCVideoDecoder::Decode(
+    const webrtc::EncodedImage& inputImage,
+    bool missingFrames,
+    const webrtc::RTPFragmentationHeader* /*fragmentation*/,
+    const webrtc::CodecSpecificInfo* /*codecSpecificInfo*/,
+    int64_t /*renderTimeMs*/) {
+  DVLOG(3) << "Decode";
+
+  {
+    base::AutoLock auto_lock(lock_);
+    if (decoding_error_occurred_) {
+      LOG(ERROR) << "Decoding error occurred.";
+      return WEBRTC_VIDEO_CODEC_ERROR;
+    }
+    if (state_ == kUninitialized || decode_complete_callback_ == NULL) {

[Pawel Osciak, 2013/05/15 17:26:32]

As Ami suggested, it'd probably be enough to combine this and previous if into
one if state_ == kDecoding (kInitialized is not a valid state for Decode() call
either, right?).

[wuchengli, 2013/05/23 16:50:47]

I changed decoding_error_occurred_ to state_==kDecodeError. The number of states
are reduced and kInitialized is a valid state now.

+      LOG(ERROR) << "WebRTC video codec unintialized.";
+      return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+    }
+  }
+  if (missingFrames || !inputImage._completeFrame) {
+    LOG(ERROR) << "Missing frames or not completed frames.";

[Pawel Osciak, 2013/05/15 17:26:32]

This is not a fatal error, iiuc we might be running into this quite often during
a conference, perhaps this is too much spam for LOG(ERROR)?

[wuchengli, 2013/05/23 16:50:47]

Good point. Changed to DLOG.

+    // Unlike the SW decoder in libvpx, hw decoder cannot handle broken frames.
+    // Return an error to request a key frame.
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  }
+
+  // Only key frame has the size.
+  if (inputImage._frameType == webrtc::kKeyFrame) {
+    gfx::Size new_size(inputImage._encodedWidth, inputImage._encodedHeight);
+    if (frame_size_ != new_size) {
+      frame_size_ = new_size;
+      decoder_message_loop_->PostTask(
+          FROM_HERE,
+          base::Bind(&RTCDemuxerStream::UpdateSize, weak_stream_, new_size));
+    }
+  }
+
+  scoped_refptr<media::DecoderBuffer> buffer = media::DecoderBuffer::CopyFrom(
+      inputImage._buffer, inputImage._length);
+  // EncodedImage uses 90KHz timestamp and DecoderBuffer uses microseconds.
+  // WebRTC uses the timestamp as key of a map. But conversion may lose
+  // precision. Here we use TimeDelta as a holder to preverse the exact same

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

s/preverse/preserve/

[wuchengli, 2013/05/15 15:30:46]

Done.

+  // value.
+  buffer->SetTimestamp(base::TimeDelta::FromInternalValue(
+      inputImage._timeStamp));
+
+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&RTCDemuxerStream::QueueBuffer, weak_stream_, buffer));
+
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t RTCVideoDecoder::RegisterDecodeCompleteCallback(
+    webrtc::DecodedImageCallback* callback) {
+  decode_complete_callback_ = callback;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t RTCVideoDecoder::Release() {
+  DVLOG(2) << "Release";
+  {
+    base::AutoLock auto_lock(lock_);
+    if (state_ == kUninitialized) {
+      LOG(ERROR) << "Decoder not initialized.";
+      return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+    }
+  }
+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&media::VideoDecoder::Stop,
+                 base::Unretained(video_decoder_.get()),
+                 base::Bind(&RTCVideoDecoder::ReleaseComplete, weak_this_)));
+  decoder_waiter_.Wait();

[Pawel Osciak, 2013/05/15 17:26:32]

Would DCHECK(state_==kUninitialized) after this be useful?

[wuchengli, 2013/05/23 16:50:47]

Do you think state_ may not be updated? renderer_gpu_video_decoder_factories.cc
has several patterns like this. Actually I'm not very sure about the visibility
of the variables using WaitableEvent.

+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+int32_t RTCVideoDecoder::Reset() {
+  DVLOG(2) << "Reset";
+  {
+    base::AutoLock auto_lock(lock_);
+    if (state_ == kUninitialized) {
+      LOG(ERROR) << "Decoder not initialized.";
+      return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+    }
+  }
+  decoder_message_loop_->PostTask(
+      FROM_HERE,
+      base::Bind(&media::VideoDecoder::Reset,
+                 base::Unretained(video_decoder_.get()),
+                 base::Bind(&RTCVideoDecoder::ResetComplete, weak_this_)));
+  decoder_waiter_.Wait();
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+void RTCVideoDecoder::OnUpdateStatistics(
+    const media::PipelineStatistics& /*stats*/) {
+  // We don't use statistics for now.

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

TODO to do this.

[wuchengli, 2013/05/15 15:30:46]

Done.

+}
+
+void RTCVideoDecoder::OnUpdatePipelineStatus(
+    const media::PipelineStatus status) {
+  DVLOG(3) << "OnUpdatePipelineStatus. status=" << status;
+  DCHECK(decoder_message_loop_->BelongsToCurrentThread());
+
+  pipeline_status_ = status;

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

This is accessed from more than one thread so it should be under lock_.

[wuchengli, 2013/05/15 15:30:46]

This is accessed from two threads. But the sequencing is controlled by
WaitableEvent. Does the new value of pipeline_status guarantee to be visible to
the other thread?

+  lock_.Acquire();

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

FWIW I think you can replace l.281-289 with:

bool do_read = false;
{ 
  AutoLock ...;
  if (OK) {
    state_ = kInitialized;
    do_read = true;
  }
}
if (do_read)
  video_decoder->Read(...);

which runs less risk of forgetting to Release() the lock.

[wuchengli, 2013/05/15 15:30:46]

Done. Good suggestion.

+  decoding_error_occurred_ = false;
+  if (status == media::PIPELINE_OK) {
+    state_ = kInitialized;
+    lock_.Release();
+    video_decoder_->Read(base::Bind(&RTCVideoDecoder::FrameReady, weak_this_));
+  } else {
+    lock_.Release();
+  }
+  decoder_waiter_.Signal();
+}
+
+void RTCVideoDecoder::ReleaseComplete() {
+  DVLOG(2) << "ReleaseComplete";
+  DCHECK(decoder_message_loop_->BelongsToCurrentThread());
+  stream_->ClearBuffer();
+
+  base::AutoLock auto_lock(lock_);
+  state_ = kUninitialized;
+  decoding_error_occurred_ = false;
+  decoder_waiter_.Signal();
+}
+
+void RTCVideoDecoder::ResetComplete() {
+  DVLOG(2) << "ResetComplete";
+  DCHECK(decoder_message_loop_->BelongsToCurrentThread());
+  stream_->ClearBuffer();
+
+  base::AutoLock auto_lock(lock_);
+  state_ = kInitialized;
+  decoding_error_occurred_ = false;
+  decoder_waiter_.Signal();
+}
+
+void RTCVideoDecoder::FrameReady(
+    media::VideoDecoder::Status status,
+    const scoped_refptr<media::VideoFrame>& frame) {
+  DCHECK(decoder_message_loop_->BelongsToCurrentThread());
+  DVLOG(3) << "FrameReady. status=" << status;
+
+  if (status != media::VideoDecoder::kOk) {
+    LOG(ERROR) << "FrameReady error. status=" << status;

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

FWIW this is not necessarily an _error_ per se, just an unsupported path.

[wuchengli, 2013/05/15 15:30:46]

You are right. Changed to VLOG(1).

+    base::AutoLock auto_lock(lock_);
+    decoding_error_occurred_ = true;
+    return;
+  }
+  gfx::Rect rect = frame->visible_rect();
+
+  webrtc::I420VideoFrame decoded_image;
+  decoded_image.CreateEmptyFrame(
+      rect.width(), rect.height(),
+      rect.width(), rect.width() / 2, rect.width() / 2);
+  webrtc::RefCountImpl<NativeHandleImpl>* handle =
+      new webrtc::RefCountImpl<NativeHandleImpl>();
+  handle->SetHandle(frame.get());
+  decoded_image.set_native_handle(handle);
+  // No need to convert the timestamp because we use it as a holder. See
+  // RTCVideoDecoder::Decode() for more detail.
+  decoded_image.set_timestamp(
+      static_cast<uint32_t>(frame->GetTimestamp().InMicroseconds()));
+  decode_complete_callback_->Decoded(decoded_image);
+
+  lock_.Acquire();
+  if (state_ == kInitialized) {
+    lock_.Release();
+    video_decoder_->Read(base::Bind(&RTCVideoDecoder::FrameReady, weak_this_));
+  } else {
+    lock_.Release();
+  }

[Ami GONE FROM CHROMIUM, 2013/05/14 22:57:24]

Ditto could reformat this.

[wuchengli, 2013/05/15 15:30:46]

Done.

+}
+
+}  // namespace content
+