Add video decoding methods in Decryptor and add DecryptingVideoDecoder.

media/filters/decrypting_video_decoder.cc

Index: media/filters/decrypting_video_decoder.cc
diff --git a/media/filters/decrypting_video_decoder.cc b/media/filters/decrypting_video_decoder.cc
new file mode 100644
index 0000000000000000000000000000000000000000..cc2a7efa4d0d46b34475836aff007bc715192c00
--- /dev/null
+++ b/media/filters/decrypting_video_decoder.cc
@@ -0,0 +1,299 @@
+// Copyright (c) 2012 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 "media/filters/decrypting_video_decoder.h"
+
+#include "base/bind.h"
+#include "base/callback_helpers.h"
+#include "base/location.h"
+#include "base/message_loop_proxy.h"
+#include "media/base/decoder_buffer.h"
+#include "media/base/decryptor.h"
+#include "media/base/demuxer_stream.h"
+#include "media/base/pipeline.h"
+#include "media/base/video_decoder_config.h"
+#include "media/base/video_frame.h"
+
+namespace media {
+
+DecryptingVideoDecoder::DecryptingVideoDecoder(
+    const MessageLoopFactoryCB& message_loop_factory_cb,
+    Decryptor* decryptor)
+    : message_loop_factory_cb_(message_loop_factory_cb),
+      message_loop_(NULL),
+      state_(kUninitialized),
+      decryptor_(decryptor) {
+}
+
+void DecryptingVideoDecoder::Initialize(
+    const scoped_refptr<DemuxerStream>& stream,
+    const PipelineStatusCB& status_cb,
+    const StatisticsCB& statistics_cb) {
+  DCHECK(!message_loop_);
+  message_loop_ = base::ResetAndReturn(&message_loop_factory_cb_).Run();
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &DecryptingVideoDecoder::DoInitialize, this,
+      stream, status_cb, statistics_cb));
+}
+
+void DecryptingVideoDecoder::Read(const ReadCB& read_cb) {
+  // Complete operation asynchronously on different stack of execution as per
+  // the API contract of VideoDecoder::Read()
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &DecryptingVideoDecoder::DoRead, this, read_cb));
+}
+
+void DecryptingVideoDecoder::Reset(const base::Closure& closure) {
+  if (!message_loop_->BelongsToCurrentThread()) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &DecryptingVideoDecoder::Reset, this, closure));
+    return;
+  }
+
+  DCHECK_NE(state_, kUninitialized);
+  DCHECK(stop_cb_.is_null());  // No Reset() during pending Stop().
+  DCHECK(reset_cb_.is_null());
+  reset_cb_ = closure;
+
+  decryptor_->CancelDecryptAndDecodeVideo();
+
+  // Defer the reset if a read is pending.

[ddorwin, 2012/10/01 18:43:20]

Reply to conversation in other patch: Okay. Might be nice to explain why and/or
when it will happen.

[xhwang, 2012/10/01 20:09:26]

Done.

+  if (!read_cb_.is_null())
+    return;
+
+  DoReset();
+}
+
+void DecryptingVideoDecoder::Stop(const base::Closure& closure) {
+  if (!message_loop_->BelongsToCurrentThread()) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &DecryptingVideoDecoder::Stop, this, closure));
+    return;
+  }
+
+  DCHECK(stop_cb_.is_null());
+  stop_cb_ = closure;
+
+  decryptor_->StopVideoDecoder();
+
+  // Defer stopping if a read is pending.

[ddorwin, 2012/10/01 18:43:20]

Same

[xhwang, 2012/10/01 20:09:26]

Done.

+  if (!read_cb_.is_null())
+    return;
+
+  DoStop();
+}
+
+DecryptingVideoDecoder::~DecryptingVideoDecoder() {
+  DCHECK_EQ(state_, kUninitialized);
+}
+
+void DecryptingVideoDecoder::DoInitialize(
+    const scoped_refptr<DemuxerStream>& stream,
+    const PipelineStatusCB& status_cb,
+    const StatisticsCB& statistics_cb) {
+  DCHECK(stream);

[ddorwin, 2012/10/01 18:43:20]

DCHECK thread.

[xhwang, 2012/10/01 20:09:26]

Done.

+  const VideoDecoderConfig& config = stream->video_decoder_config();
+  if (!config.IsValidConfig()) {
+    DLOG(ERROR) << "Invalid video stream - " << config.AsHumanReadableString();

[ddorwin, 2012/10/01 18:43:20]

...stream config?

[xhwang, 2012/10/01 20:09:26]

Done.

[xhwang, 2012/10/01 20:09:26]

Done.

+    status_cb.Run(PIPELINE_ERROR_DECODE);
+    return;
+  }
+
+  // DecryptingVideoDecoder only accepts potentially encrypted stream.
+  if (!config.is_encrypted()) {
+    status_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
+    return;
+  }
+
+  DCHECK(!demuxer_stream_);
+  demuxer_stream_ = stream;
+  statistics_cb_ = statistics_cb;
+
+  DCHECK(decryptor_);
+  decryptor_->InitializeVideoDecoder(
+      config,
+      base::Bind(&DecryptingVideoDecoder::OnDecoderInitialized, this,
+                 status_cb));
+}
+
+void DecryptingVideoDecoder::OnDecoderInitialized(
+    const PipelineStatusCB& status_cb,

[ddorwin, 2012/10/01 18:43:20]

nit: Is there a reason the result comes AFTER the callback?

[xhwang, 2012/10/01 20:09:26]

This is because we need to bind the status_cb in the callback for
decryptor_->InitializeVideoDecoder(). See line 116.

+    bool success) {
+  if (!message_loop_->BelongsToCurrentThread()) {
+    message_loop_->PostTask(FROM_HERE, base::Bind(
+        &DecryptingVideoDecoder::OnDecoderInitialized, this,
+        status_cb, success));
+    return;
+  }
+
+  DCHECK(!status_cb.is_null());
+
+  if (!success) {
+    status_cb.Run(DECODER_ERROR_NOT_SUPPORTED);
+    return;
+  }
+
+  // Success!
+  state_ = kNormal;
+  status_cb.Run(PIPELINE_OK);
+}
+
+void DecryptingVideoDecoder::DoRead(const ReadCB& read_cb) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK(!read_cb.is_null());
+  CHECK_NE(state_, kUninitialized);
+  CHECK(read_cb_.is_null()) << "Overlapping decodes are not supported.";
+
+  // Return empty frames if decoding has finished.
+  if (state_ == kDecodeFinished) {
+    read_cb.Run(kOk, VideoFrame::CreateEmptyFrame());
+    return;
+  }
+
+  read_cb_ = read_cb;
+  ReadFromDemuxerStream();
+}
+
+void DecryptingVideoDecoder::ReadFromDemuxerStream() {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kNormal);
+  DCHECK(!read_cb_.is_null());
+
+  demuxer_stream_->Read(
+      base::Bind(&DecryptingVideoDecoder::DecryptAndDecodeBuffer, this));
+}
+
+void DecryptingVideoDecoder::DecryptAndDecodeBuffer(
+    DemuxerStream::Status status,
+    const scoped_refptr<DecoderBuffer>& buffer) {
+  // In theory, we don't need to force post the task here, because we do a

[ddorwin, 2012/10/01 18:43:20]

Isn't the old comment referring to the fact that this call is executed on the
stack of demuxer_stream_->Read()? If so, isn't that different from the callback
that DoDecryptAndDecodeBuffer calls?
I also worry about removing this TODO because when it is fixed, it will affect
all decoders.

[xhwang, 2012/10/01 20:09:26]

That's a long story :)

The original comment is copied from FFVD
(http://code.google.com/searchframe#OAMlx_jo-ck/src/media/filters/ffmpeg_video...).
The reason for this is that if FFmpegDemuxerStream::Read() execute the read
callback on the same stack (synchronously), we may end up with the following
re-entrancing call sequence:
ReadFromDemuxerStream() -> FFmpegDemuxerStream::Read() ->
DecryptOrDecodeBuffer() -> DoDecryptOrDecodeBuffer -> ReadFromDemuxerStream().

In our case, we have one more callback, the video_decode_cb_. So to produce the
above re-entrancing call sequence it has to be:
ReadFromDemuxerStream() -> FFmpegDemuxerStream::Read() ->
DecryptAndDecodeBuffer() -> DoDecryptAndDecodeBuffer() ->
Decryptor::DecryptAndDecodeVideo() -> DeliverFrame() -> DoDeliverFrame() ->
ReadFromDemuxerStream().
Note that this can happen because FFmpegDemuxerStream::Read() and
Decryptor::DecryptAndDecodeVideo() can both call their callbacks synchronously
(on the same call stack).

To prevent the above call sequence, we can force post either in
DecryptOrDecodeBuffer() (as we did in FFVD), or in DeliverFrame().

Other than that, we also could end up with the following call sequence:
DecryptingVideoDecoder::Reset() -> Decryptor::CancelDecryptAndDecodeVideo() ->
DeliverFrame() -> DoDeliverFrame() -> base::ResetAndReturn(&read_cb_).Run(kOk,
NULL),
because Decryptor::CancelDecryptAndDecodeVideo() can also call the
video_decode_cb_ synchronously. 
Since we also check "read_cb_" in Reset() (line 62), this will be an issue. It
can be fixed by adding more logic in Reset(). But since the long calling
sequence isn't good anyways, I'd like to just force post in DeliverFrame() to
prevent it.

Since we force post in DeliverFrame(), it's not necessary in theory to force
post in DecryptOrDecodeBuffer(). But for the same reason (to avoid super long
call sequence), I also force post in DecryptOrDecodeBuffer().

Question: Since the ProxyDecryptor will have it's own message loop, does it make
sense to enforce the ProxyDecryptor to always return callbacks asynchronously?
(If we do that we won't need any for post task in DVD, but may need to always
post Decrypt()/CancelDecrypt() calls to make sure callbacks are not returned in
the same callstack.)

Actually I wonder how expensive it is to have an extra post? Is this a big
performance concern or just some minor optimization?

+scherkus: WDYT?

+  // force task post in DeliverFrame(). Therefore, even if
+  // demuxer_stream_->Read() execute the read callback on the same execution
+  // stack we are still fine. But it looks like a force post task makes the
+  // logic more understandable and manageable, so why not?

[ddorwin, 2012/10/01 18:43:20]

I would rephrase the "why not?" questions here and below to statements. They
appear to be uncertain as to whether this is the right thing.

[xhwang, 2012/10/01 20:09:26]

Agreed. The right explanation is too long (see above) and I didn't have a good
way to summarize it. Will try again once we decided what to do (see Question
above).

+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &DecryptingVideoDecoder::DoDecryptAndDecodeBuffer, this,
+      status, buffer));
+}
+
+void DecryptingVideoDecoder::DoDecryptAndDecodeBuffer(
+    DemuxerStream::Status status,
+    const scoped_refptr<DecoderBuffer>& buffer) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kNormal);
+  DCHECK(!read_cb_.is_null());
+  DCHECK_EQ(!!buffer, status == DemuxerStream::kOk) << status;
+
+  if (!stop_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    if (!reset_cb_.is_null())
+      DoReset();

[xhwang, 2012/09/30 19:58:51]

This is to handle the case where Stop() is called during pending Reset(). But I
feel it's not really very useful as they both depend on the pending read.
Probably we should just specify in the API that Stop() shouldn't be called
during a pending Reset()?

[ddorwin, 2012/10/01 18:43:20]

Does it make sense to call both callbacks? Do callers expect this?
I'm fine with updating the API but we should see what others say.

[xhwang, 2012/10/01 20:09:26]

Yeah, I also prefer to updating the API to prevent this. This is kind of
unnecessary complexity. +scherkus, what do you think?

+
+    DoStop();
+    return;
+  }
+
+  if (!reset_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    DoReset();
+    return;
+  }
+
+  if (status == DemuxerStream::kAborted) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    return;
+  }
+
+  if (status == DemuxerStream::kConfigChanged) {
+    // TODO(xhwang): Add config change support.
+    base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
+    return;
+  }
+
+  DCHECK_EQ(status, DemuxerStream::kOk);
+  DCHECK(buffer);
+
+  decryptor_->DecryptAndDecodeVideo(
+      buffer, base::Bind(&DecryptingVideoDecoder::DeliverFrame, this,
+                         buffer->GetDataSize()));
+}
+
+void DecryptingVideoDecoder::DeliverFrame(
+    int buffer_size,
+    Decryptor::Status status,
+    const scoped_refptr<VideoFrame>& frame) {
+  // We need to force task post here because the VideoDecodeCB can be executed
+  // synchronously in Reset()/Stop(). Instead of using more complicated logic in

[ddorwin, 2012/10/01 18:43:20]

Oh, I didn't realize it was just for Reset and Stop. We're paying a cost in the
normal case for the rare case of a reset/stop.

[xhwang, 2012/10/01 20:09:26]

That's a valid point: paying a cost in the normal case for rare cases. This can
be avoided by enforcing video_decode_cb_ is always fired asychronously in
CancelDecryptAndDecodeVideo(). But I'd like to get consensus on this before
updating (see above comments about force posting).

+  // those function to fix it, why not force task post here to make everything
+  // simple and clear?
+  message_loop_->PostTask(FROM_HERE, base::Bind(
+      &DecryptingVideoDecoder::DoDeliverFrame, this,
+      buffer_size, status, frame));
+}
+
+void DecryptingVideoDecoder::DoDeliverFrame(
+    int buffer_size,
+    Decryptor::Status status,
+    const scoped_refptr<VideoFrame>& frame) {
+  DCHECK(message_loop_->BelongsToCurrentThread());
+  DCHECK_EQ(state_, kNormal);
+  DCHECK(!read_cb_.is_null());
+
+  if (!stop_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    if (!reset_cb_.is_null())
+      DoReset();
+
+    DoStop();
+    return;
+  }
+
+  if (!reset_cb_.is_null()) {
+    base::ResetAndReturn(&read_cb_).Run(kOk, NULL);
+    DoReset();
+    return;
+  }
+
+  if (status == Decryptor::kNoKey || status == Decryptor::kError) {
+    DCHECK(!frame);
+    state_ = kDecodeFinished;
+    base::ResetAndReturn(&read_cb_).Run(kDecodeError, NULL);
+    return;
+  }
+
+  // The buffer has been accepted by the decoder, let's report statistics.
+  if (buffer_size) {
+    PipelineStatistics statistics;
+    statistics.video_bytes_decoded = buffer_size;
+    statistics_cb_.Run(statistics);
+  }
+
+  if (status == Decryptor::kNeedMoreData) {
+    DCHECK(!frame);
+    ReadFromDemuxerStream();
+    return;
+  }
+
+  DCHECK_EQ(status, Decryptor::kSuccess);
+  DCHECK(frame);
+  if (frame->IsEndOfStream())
+    state_ = kDecodeFinished;
+
+  base::ResetAndReturn(&read_cb_).Run(kOk, frame);
+}
+
+void DecryptingVideoDecoder::DoReset() {
+  DCHECK(read_cb_.is_null());
+  DCHECK_NE(state_, kUninitialized);
+  state_ = kNormal;
+  base::ResetAndReturn(&reset_cb_).Run();
+}
+
+void DecryptingVideoDecoder::DoStop() {
+  DCHECK(read_cb_.is_null());
+  state_ = kUninitialized;
+  base::ResetAndReturn(&stop_cb_).Run();
+}
+
+}  // namespace media