Implement WebRTC in Chrome for TV

content/renderer/media/rtc_video_decoder_bridge_tv.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_bridge_tv.h"
+
+#include <queue>
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/location.h"
+#include "base/logging.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/singleton.h"
+#include "base/message_loop_proxy.h"
+#include "base/synchronization/lock.h"
+#include "base/time.h"
+#include "media/base/bind_to_loop.h"
+#include "media/base/decoder_buffer.h"
+#include "third_party/libjingle/source/talk/base/ratetracker.h"
+
+namespace content {
+
+// Helper function that makes sure |read_cb| runs on |message_loop_proxy|.

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

I think you converted all callsites to media::BindToLoop but forgot to delete
this.

[wonsik, 2013/05/13 14:03:48]

Removed.

+static void RunOnMessageLoop(
+    media::DemuxerStream::ReadCB read_cb,
+    scoped_refptr<base::MessageLoopProxy> message_loop_proxy,
+    media::DemuxerStream::Status status,
+    const scoped_refptr<media::DecoderBuffer>& buffer) {
+  if (!message_loop_proxy->BelongsToCurrentThread()) {
+    message_loop_proxy->PostTask(FROM_HERE, base::Bind(
+        &RunOnMessageLoop, read_cb, message_loop_proxy, status, buffer));
+    return;
+  }
+  read_cb.Run(status, buffer);
+}
+
+// RTCDemuxerStream ------------------------------------------------------------
+
+namespace {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

why?

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring

+
+class RTCDemuxerStream : public media::DemuxerStream {
+ public:
+  explicit RTCDemuxerStream(const gfx::Size& size);
+  virtual ~RTCDemuxerStream();
+  // 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 QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                   const base::Closure& done_cb,
+                   const gfx::Size& size);
+
+ private:
+  struct BufferEntry {
+    BufferEntry(const scoped_refptr<media::DecoderBuffer>& decoder_buffer_param,
+                const base::Closure& done_cb_param,
+                const gfx::Size& size_param)
+        : decoder_buffer(decoder_buffer_param),
+          done_cb(done_cb_param),
+          size(size_param) {}
+
+    scoped_refptr<media::DecoderBuffer> decoder_buffer;
+    base::Closure done_cb;
+    // When |!size.isEmpty()|, it means that config change with new size |size|
+    // happened.
+    gfx::Size size;

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

new_size ?

[wonsik, 2013/05/13 14:03:48]

Done.

+  };
+
+  void RunReadCallback_Locked();
+
+  base::Lock lock_;
+  std::queue<BufferEntry> buffer_queue_;
+  ReadCB read_cb_;
+  base::Closure pending_done_cb_;
+
+  media::AudioDecoderConfig dummy_audio_decoder_config_;

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

unnecessary; can simply
return media::AudioDecoderConfig();
in audio_decoder_config(), below.

[wonsik, 2013/05/13 14:03:48]

Because audio_decoder_config() returns const reference, it's not quite possible.

+  media::VideoDecoderConfig video_decoder_config_;
+  talk_base::RateTracker frame_rate_tracker_;
+};
+
+RTCDemuxerStream::RTCDemuxerStream(const gfx::Size& size)
+  : video_decoder_config_(

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

indent

[wonsik, 2013/05/13 14:03:48]

Done.

+      media::kCodecVP8,
+      media::VP8PROFILE_MAIN,
+      media::VideoFrame::NATIVE_TEXTURE,
+      size, gfx::Rect(size), size, NULL, 0, false) {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

This assumes that width & height are macroblock-aligned, so the coded stream has
no padding, and there is no cropping.  None of those are guaranteed by the
webrtc spec; why is it ok to manufacture these values here?

+}
+
+RTCDemuxerStream::~RTCDemuxerStream() {}
+
+const media::AudioDecoderConfig& RTCDemuxerStream::audio_decoder_config() {
+  LOG(FATAL) << "Does not support audio.";
+  return dummy_audio_decoder_config_;
+}
+
+const media::VideoDecoderConfig& RTCDemuxerStream::video_decoder_config() {
+  base::AutoLock lock(lock_);
+  return video_decoder_config_;
+}
+
+media::DemuxerStream::Type RTCDemuxerStream::type() {
+  return media::DemuxerStream::VIDEO;
+}
+
+void RTCDemuxerStream::EnableBitstreamConverter() {
+  LOG(FATAL) << "Not reachable.";
+}
+
+void RTCDemuxerStream::QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                                   const base::Closure& done_cb,
+                                   const gfx::Size& size) {
+  base::AutoLock lock(lock_);
+  buffer_queue_.push(BufferEntry(buffer, done_cb, size));
+  if (buffer)
+    frame_rate_tracker_.Update(1);
+  DLOG(INFO) << "frame rate received : " << frame_rate_tracker_.units_second();
+  RunReadCallback_Locked();
+}
+
+void RTCDemuxerStream::Read(const ReadCB& read_cb) {
+  base::AutoLock lock(lock_);
+  CHECK(read_cb_.is_null());
+  // A call to |Read| operation means that |MediaSourceDelegate| is done with
+  // the previous buffer.
+  if (!pending_done_cb_.is_null())
+    base::ResetAndReturn(&pending_done_cb_).Run();
+  read_cb_ = media::BindToLoop(base::MessageLoopProxy::current(), read_cb);
+  RunReadCallback_Locked();
+}
+
+void RTCDemuxerStream::RunReadCallback_Locked() {
+  if (!read_cb_.is_null() && !buffer_queue_.empty()) {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

If you reverse the test you can de-indent the rest of the function.

[wonsik, 2013/05/13 14:03:48]

Done.

+    BufferEntry& front = buffer_queue_.front();
+    DemuxerStream::Status status = DemuxerStream::kOk;
+    if (!front.size.IsEmpty()) {
+      DCHECK(!front.decoder_buffer);
+      // No VideoFrame actually reaches cc in Google TV case. We just make
+      // coded_size == visible_rect == natural_size here.

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

See comment elsewhere; this seems wrong since you don't get to dictate the
contents of the bitstream - it may well have padding that you don't want to
render.

+      video_decoder_config_.Initialize(media::kCodecVP8,
+                                       media::VP8PROFILE_MAIN,
+                                       media::VideoFrame::NATIVE_TEXTURE,
+                                       front.size,
+                                       gfx::Rect(front.size),
+                                       front.size,
+                                       NULL, 0, false, false);
+      status = DemuxerStream::kConfigChanged;
+    }
+
+    DCHECK(pending_done_cb_.is_null());
+    pending_done_cb_ = front.done_cb;
+    base::ResetAndReturn(&read_cb_).Run(status, front.decoder_buffer);
+    buffer_queue_.pop();
+  }
+}
+
+// RTCDemuxerProxy -------------------------------------------------------------
+
+class RTCDemuxerProxy : public base::RefCountedThreadSafe<RTCDemuxerProxy> {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Why does need to be separate from RTCDemuxer?

[wonsik, 2013/05/13 14:03:48]

Removed during refactoring.

+ public:
+  RTCDemuxerProxy(const scoped_refptr<base::MessageLoopProxy>& message_loop);
+
+  void Initialize(media::DemuxerHost* host, const media::PipelineStatusCB& cb);
+  media::DemuxerStream* GetStream(media::DemuxerStream::Type type);
+  void UpdateSize(const gfx::Size& size);
+  void QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                   const base::Closure& done_cb,
+                   const gfx::Size& size);
+
+ protected:
+  friend class base::RefCountedThreadSafe<RTCDemuxerProxy>;
+
+  virtual ~RTCDemuxerProxy();
+
+ private:
+  scoped_refptr<base::MessageLoopProxy> loop_proxy_;
+  scoped_ptr<RTCDemuxerStream> stream_;
+  media::DemuxerHost* host_;
+  media::PipelineStatusCB init_cb_;
+};
+
+RTCDemuxerProxy::RTCDemuxerProxy(
+    const scoped_refptr<base::MessageLoopProxy>& message_loop)
+  : loop_proxy_(message_loop),
+    host_(NULL) {
+}
+
+RTCDemuxerProxy::~RTCDemuxerProxy() {}
+
+void RTCDemuxerProxy::Initialize(media::DemuxerHost* host,
+                                 const media::PipelineStatusCB& cb) {
+  host_ = host;
+  init_cb_ = cb;
+}
+
+media::DemuxerStream* RTCDemuxerProxy::GetStream(
+    media::DemuxerStream::Type type) {
+  if (type == media::DemuxerStream::VIDEO)
+    return stream_.get();
+  return NULL;
+}
+
+void RTCDemuxerProxy::UpdateSize(const gfx::Size& size) {
+  if (!loop_proxy_->BelongsToCurrentThread()) {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Is the trampolining here and elsewhere in this file really necessary?  Can they
be replaced with unconditional PostTasks or DCHECKs (if the caller is always on
the right thread already).

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring.

+    loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &RTCDemuxerProxy::UpdateSize, this, size));
+    return;
+  }
+  DCHECK(!stream_);
+  stream_.reset(new RTCDemuxerStream(size));
+  if (!init_cb_.is_null())

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

It seems strange to me to be triggering init-done off the UpdateSize event.  How
is error handled? (i.e. if CreateDemuxer early-returns NULL, what notifies the
caller of Initialize()?)

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring.

+    base::ResetAndReturn(&init_cb_).Run(media::PIPELINE_OK);
+}
+
+void RTCDemuxerProxy::QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                                  const base::Closure& done_cb,
+                                  const gfx::Size& size) {
+  if (!loop_proxy_->BelongsToCurrentThread()) {
+    loop_proxy_->PostTask(FROM_HERE, base::Bind(
+        &RTCDemuxerProxy::QueueBuffer, this, buffer, done_cb, size));
+    return;
+  }
+  if (stream_)
+    stream_->QueueBuffer(buffer, done_cb, size);
+  else
+    done_cb.Run();
+}
+
+// RTCDemuxer ------------------------------------------------------------------
+
+class RTCDemuxer : public media::Demuxer {
+ public:
+  RTCDemuxer(const scoped_refptr<base::MessageLoopProxy>& message_loop);
+  virtual ~RTCDemuxer();
+
+  // media::Demuxer implementation.
+  virtual void Initialize(media::DemuxerHost* host,
+                          const media::PipelineStatusCB& cb) OVERRIDE;
+  virtual media::DemuxerStream* GetStream(
+      media::DemuxerStream::Type type) OVERRIDE;
+  virtual base::TimeDelta GetStartTime() const OVERRIDE;
+
+  void UpdateSize(const gfx::Size& size);
+  void QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                   const base::Closure& done_cb,
+                   const gfx::Size& size);
+
+ private:
+  friend class RTCVideoDecoderBridgeTv;

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Why does this make sense?

[wonsik, 2013/05/13 14:03:48]

Removed

+
+  scoped_refptr<RTCDemuxerProxy> proxy_;
+};
+
+RTCDemuxer::RTCDemuxer(
+    const scoped_refptr<base::MessageLoopProxy>& message_loop)
+  : proxy_(new RTCDemuxerProxy(message_loop)) {}
+
+RTCDemuxer::~RTCDemuxer() {}
+
+void RTCDemuxer::Initialize(media::DemuxerHost* host,
+                            const media::PipelineStatusCB& cb) {
+  proxy_->Initialize(host, cb);
+}
+
+media::DemuxerStream* RTCDemuxer::GetStream(media::DemuxerStream::Type type) {
+  return proxy_->GetStream(type);
+}
+
+base::TimeDelta RTCDemuxer::GetStartTime() const {
+  return base::TimeDelta();

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

what happens if one joins the middle of a stream?

+}
+
+void RTCDemuxer::UpdateSize(const gfx::Size& size) {
+  proxy_->UpdateSize(size);
+}
+
+void RTCDemuxer::QueueBuffer(scoped_refptr<media::DecoderBuffer> buffer,
+                             const base::Closure& done_cb,
+                             const gfx::Size& size) {
+  proxy_->QueueBuffer(buffer, done_cb, size);
+}
+
+// RTCVideoDecoderBridgeTvImpl -------------------------------------------------
+
+class RTCVideoDecoderBridgeTvImpl : public RTCVideoDecoderBridgeTv {

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Is it important that this Impl be a separate class from the non-Impl version? 
What does that gain (other than boilerplate duplication)

[wonsik, 2013/05/13 14:03:48]

Done.

+ public:
+  static RTCVideoDecoderBridgeTvImpl* GetInstance() {
+    return Singleton<RTCVideoDecoderBridgeTvImpl>::get();
+  }
+  RTCVideoDecoderBridgeTvImpl();
+  virtual ~RTCVideoDecoderBridgeTvImpl();
+
+  // webrtc::VideoDecoder implementation.
+  virtual WebRtc_Word32 InitDecode(
+      const webrtc::VideoCodec* codecSettings,
+      WebRtc_Word32 numberOfCores) OVERRIDE;
+  virtual WebRtc_Word32 Decode(
+      const webrtc::EncodedImage& inputImage,
+      bool missingFrames,
+      const webrtc::RTPFragmentationHeader* fragmentation,
+      const webrtc::CodecSpecificInfo* codecSpecificInfo = NULL,
+      WebRtc_Word64 renderTimeMs = -1) OVERRIDE;
+  virtual WebRtc_Word32 RegisterDecodeCompleteCallback(
+      webrtc::DecodedImageCallback* callback) OVERRIDE;
+  virtual WebRtc_Word32 Release() OVERRIDE;
+  virtual WebRtc_Word32 Reset() OVERRIDE;
+
+  virtual media::Demuxer* CreateDemuxer(
+      const MediaStreamDependencyFactory* media_stream_dependency_factory,
+      const scoped_refptr<base::MessageLoopProxy>& message_loop) OVERRIDE;
+  virtual void DestroyDemuxer(const media::Demuxer* demuxer) OVERRIDE;
+  virtual bool AcquireOwnership(
+      const MediaStreamDependencyFactory* media_stream_dependency_factory)
+      OVERRIDE;
+  virtual void ReleaseOwnership(
+      const MediaStreamDependencyFactory* media_stream_dependency_factory)
+      OVERRIDE;
+
+  static void RunDecodeCompleteCallback(webrtc::DecodedImageCallback* callback,
+                                        WebRtc_Word64 timestamp);
+
+ private:
+  friend struct DefaultSingletonTraits<RTCVideoDecoderBridgeTv>;
+
+  static const base::TimeDelta kDecoderTimeOut;
+  enum Status {
+    kNoInit,
+    kInitialized,
+  };

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

bool is_initialized;
instead?

[wonsik, 2013/05/13 14:03:48]

Done.

+
+  // Lock protected, since these can be accessed in CreateDemuxer,
+  // DestroyDemuxer, AcquireOwnership, and ReleaseOwnership. No guarantee of
+  // calling thread for these methods.
+  base::Lock lock_;
+  const MediaStreamDependencyFactory* ownership_tag_;
+  const MediaStreamDependencyFactory* demuxer_tag_;

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

is this only necessary because you don't trust the owner to call CreateDemuxer
exactly once?  That seems strange.

[wonsik, 2013/05/13 14:03:48]

Removed.

+  scoped_ptr<RTCDemuxer> demuxer_;
+  gfx::Size size_;
+  Status status_;
+
+  // Only used by decoder thread.

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Please DCHECK running on the correct thread at the top of methods that touch
this state.

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring

+  bool first_frame_;
+  webrtc::DecodedImageCallback* decode_complete_callback_;
+  WebRtc_Word64 timestamp_offset_;

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Use "millis" in the name if this is storing millis.

[wonsik, 2013/05/13 14:03:48]

Done.

+
+  DISALLOW_COPY_AND_ASSIGN(RTCVideoDecoderBridgeTvImpl);
+};
+
+RTCVideoDecoderBridgeTvImpl::RTCVideoDecoderBridgeTvImpl()
+    : ownership_tag_(NULL),
+      demuxer_tag_(NULL),
+      status_(kNoInit),
+      first_frame_(true),
+      decode_complete_callback_(NULL) {}
+
+RTCVideoDecoderBridgeTvImpl::~RTCVideoDecoderBridgeTvImpl() {}
+
+media::Demuxer* RTCVideoDecoderBridgeTvImpl::CreateDemuxer(
+    const MediaStreamDependencyFactory* media_stream_dependency_factory,
+    const scoped_refptr<base::MessageLoopProxy>& message_loop) {
+  base::AutoLock lock(lock_);
+  if (demuxer_tag_ != NULL ||
+      (ownership_tag_ != NULL &&
+       ownership_tag_ != media_stream_dependency_factory))
+    return NULL;
+  DCHECK(!demuxer_);
+  demuxer_tag_ = media_stream_dependency_factory;
+  demuxer_.reset(new RTCDemuxer(message_loop));
+  // If this is initialized before demuxer is set, update the demuxer with size
+  // information.
+  if (status_ == kInitialized)
+    demuxer_->UpdateSize(size_);
+  return demuxer_.get();
+}
+
+void RTCVideoDecoderBridgeTvImpl::DestroyDemuxer(
+    const media::Demuxer* demuxer) {
+  base::AutoLock lock(lock_);
+  DCHECK(demuxer_.get() == demuxer);
+  demuxer_.reset();
+  demuxer_tag_ = NULL;
+}
+
+bool RTCVideoDecoderBridgeTvImpl::AcquireOwnership(
+    const MediaStreamDependencyFactory* media_stream_dependency_factory) {
+  base::AutoLock lock(lock_);
+  if (ownership_tag_ != NULL ||
+      (demuxer_tag_ != NULL && demuxer_tag_ != media_stream_dependency_factory))
+    return false;
+
+  ownership_tag_ = media_stream_dependency_factory;
+  return true;
+}
+
+void RTCVideoDecoderBridgeTvImpl::ReleaseOwnership(
+    const MediaStreamDependencyFactory* media_stream_dependency_factory) {
+  base::AutoLock lock(lock_);
+  DCHECK(ownership_tag_ == media_stream_dependency_factory);
+  ownership_tag_ = NULL;
+}
+
+WebRtc_Word32 RTCVideoDecoderBridgeTvImpl::InitDecode(
+    const webrtc::VideoCodec* codecSettings,
+    WebRtc_Word32 numberOfCores) {
+  if (codecSettings->codecType != webrtc::kVideoCodecVP8)
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  // We don't support feedback mode.
+  if (codecSettings->codecSpecific.VP8.feedbackModeOn)
+    return WEBRTC_VIDEO_CODEC_ERROR;
+
+  base::AutoLock lock(lock_);

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

putting this at the top of the function is more robust to future changes.

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring; RTCVideoDecoderBridgeTv is only accessed by the decoder
thread.

+  if (status_ != kNoInit)
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  size_ = gfx::Size(codecSettings->width, codecSettings->height);
+  status_ = kInitialized;
+  first_frame_ = true;
+  if (demuxer_)

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

How can this fail?  (isn't it a programming error to call InitDecode() before
CreateDemuxer()?)

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring

+    demuxer_->UpdateSize(size_);
+
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+WebRtc_Word32 RTCVideoDecoderBridgeTvImpl::Decode(
+    const webrtc::EncodedImage& inputImage,
+    bool missingFrames,
+    const webrtc::RTPFragmentationHeader* /* fragmentation */,
+    const webrtc::CodecSpecificInfo* /* codecSpecificInfo */,

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

do you not need to assert that these aren't specified, since you can't deal with
them?  Or are they optional output params?

+    WebRtc_Word64 renderTimeMs) {
+  // Unlike the SW decoder in libvpx, hw decoder can not handle broken frames.
+  // Here, we return an error in order to request a key frame.
+  if (missingFrames || !inputImage._completeFrame)
+    return WEBRTC_VIDEO_CODEC_ERROR;
+
+  base::AutoLock lock(lock_);

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

ditto move lock to top of function unless there's a good reason not to (I'll
stop commenting on this now; please make a pass)

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring

+  if (status_ == kNoInit || decode_complete_callback_ == NULL)
+    return WEBRTC_VIDEO_CODEC_UNINITIALIZED;
+  if (status_ != kInitialized)
+    return WEBRTC_VIDEO_CODEC_ERROR;
+  // Drop frames until demuxer is up and running. We'll request key frame
+  // once demuxer is ready.
+  if (!demuxer_)

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Ditto l.420 this seems like a programming error.

Perhaps this is coded with a view that you don't know who's creating the demuxer
first, the WMPA or webrtc (which is fed by the idea that MSDF needs to track
ownership agnostically).  But I think you can rationalize this significantly by
enforcing a setup order, which will (as a side-effect) make code like this
unnecessary.

[wonsik, 2013/05/13 14:03:48]

N/A after refactoring

+    return WEBRTC_VIDEO_CODEC_NO_OUTPUT;
+
+  if (first_frame_) {
+    // If the first frame is not the key frame, return an error to request a key

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

s/the key/a key/

[wonsik, 2013/05/13 14:03:48]

Done.

+    // frame.
+    if (inputImage._frameType != webrtc::kKeyFrame)
+      return WEBRTC_VIDEO_CODEC_ERROR;
+
+    // Google TV expects timestamp from 0.

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

This is a cryptic comment unless one sees the subtraction at l.469.  Can you
make it clearer?

[wonsik, 2013/05/13 14:03:48]

Done.

+    timestamp_offset_ = renderTimeMs;
+  }
+  first_frame_ = false;
+  gfx::Size new_size;
+  if (inputImage._frameType == webrtc::kKeyFrame &&
+      inputImage._encodedWidth != 0 && inputImage._encodedHeight != 0) {
+    // Only key frame has the size.

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

This comment is at odds with the lines of code preceding it.
If the comment is correct then the test for keyframe is unnecessary (and the
comment should probably precede the if test)

[wonsik, 2013/05/13 14:03:48]

Modified the comment.

+    new_size.SetSize(inputImage._encodedWidth, inputImage._encodedHeight);
+    size_ = new_size;
+  }
+  scoped_refptr<media::DecoderBuffer> buffer;
+  buffer = media::DecoderBuffer::CopyFrom(

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

move assignment to declaration at previous line

[wonsik, 2013/05/13 14:03:48]

Done.

+      inputImage._buffer, inputImage._length);

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

is it necessary to make a copy here?

[wonsik, 2013/05/13 14:03:48]

Yes since inputImage's lifetime is not guaranteed after this method returns.
Left a comment.

+  if (renderTimeMs != -1) {
+    buffer->SetTimestamp(
+        base::TimeDelta::FromMilliseconds(renderTimeMs - timestamp_offset_));
+  }
+
+  if (!new_size.IsEmpty()) {
+      demuxer_->QueueBuffer(
+          NULL,
+          base::Bind(&base::DoNothing),
+          new_size);

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

Why not simply pass new_size into the call at l.478 below?

[wonsik, 2013/05/13 14:03:48]

Done.

+  }
+  demuxer_->QueueBuffer(
+      buffer,
+      base::Bind(&RTCVideoDecoderBridgeTvImpl::RunDecodeCompleteCallback,
+                 decode_complete_callback_,
+                 renderTimeMs),
+      gfx::Size());
+
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+WebRtc_Word32 RTCVideoDecoderBridgeTvImpl::RegisterDecodeCompleteCallback(
+    webrtc::DecodedImageCallback* callback) {
+  decode_complete_callback_ = callback;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+WebRtc_Word32 RTCVideoDecoderBridgeTvImpl::Release() {
+  base::AutoLock lock(lock_);
+  status_ = kNoInit;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+WebRtc_Word32 RTCVideoDecoderBridgeTvImpl::Reset() {
+  first_frame_ = true;
+  return WEBRTC_VIDEO_CODEC_OK;
+}
+
+// static
+void RTCVideoDecoderBridgeTvImpl::RunDecodeCompleteCallback(
+    webrtc::DecodedImageCallback* callback, WebRtc_Word64 timestamp) {
+  // We call the decode complete callback function to notify libjingle that
+  // decoding is finished.
+  webrtc::I420VideoFrame dummy_video_frame;
+  dummy_video_frame.CreateEmptyFrame(2, 1, 2, 1, 1);

[Ami GONE FROM CHROMIUM, 2013/05/08 20:26:44]

These are some magical magic values.  Doco meaning?

[wonsik, 2013/05/13 14:03:48]

Done.

+  dummy_video_frame.set_timestamp(timestamp);
+  callback->Decoded(dummy_video_frame);
+}
+
+}  // anonymous namespace
+
+// RTCVideoDecoderBridgeTv -----------------------------------------------------
+
+// static
+RTCVideoDecoderBridgeTv* RTCVideoDecoderBridgeTv::Get() {
+  return RTCVideoDecoderBridgeTvImpl::GetInstance();
+}
+
+}  // namespace content