Add callback in VideoDecoder and AudioDecoder to return decoded frames.

media/filters/decoder_stream.cc

Index: media/filters/decoder_stream.cc
diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index 4f712be2f2227883e37710fd9ae5a1b6b9bd9751..852a6b18b640ce6b7db71b2eb7213cb70fb12f1f 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -80,6 +80,8 @@ void DecoderStream<StreamType>::Initialize(DemuxerStream* stream,
   decoder_selector_->SelectDecoder(
       stream, low_delay,
       base::Bind(&DecoderStream<StreamType>::OnDecoderSelected,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
                  weak_factory_.GetWeakPtr()));
 }
 
@@ -113,19 +115,7 @@ void DecoderStream<StreamType>::Read(const ReadCB& read_cb) {
     ready_outputs_.pop_front();
   }
 
-  // Decoder may be in reinitializing state as result of the previous Read().
-  if (state_ == STATE_REINITIALIZING_DECODER)
-    return;
-
-  if (!CanDecodeMore())
-    return;
-
-  if (state_ == STATE_FLUSHING_DECODER) {
-    FlushDecoder();
-    return;
-  }
-
-  if (state_ != STATE_PENDING_DEMUXER_READ)
+  if (state_ == STATE_NORMAL && CanDecodeMore())
     ReadFromDemuxerStream();
 }
 
@@ -300,28 +290,26 @@ void DecoderStream<StreamType>::Decode(
   TRACE_EVENT_ASYNC_BEGIN0("media", GetTraceString<StreamType>(), this);
   ++pending_decode_requests_;
   decoder_->Decode(buffer,
-                   base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
+                   base::Bind(&DecoderStream<StreamType>::OnDecodeDone,
                               weak_factory_.GetWeakPtr(),
-                              buffer_size));
+                              buffer_size,
+                              buffer->end_of_stream()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::FlushDecoder() {
-  if (pending_decode_requests_ == 0)
-    Decode(DecoderBuffer::CreateEOSBuffer());
+  Decode(DecoderBuffer::CreateEOSBuffer());
 }
 
 template <DemuxerStream::Type StreamType>
-void DecoderStream<StreamType>::OnDecodeOutputReady(
-    int buffer_size,
-    typename Decoder::Status status,
-    const scoped_refptr<Output>& output) {
-  FUNCTION_DVLOG(2) << status << " " << output;
+void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
+                                             bool end_of_stream,
+                                             typename Decoder::Status status) {
+  FUNCTION_DVLOG(2) << status;
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
          state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
       << state_;
   DCHECK(stop_cb_.is_null());
-  DCHECK_EQ(status == Decoder::kOk, output != NULL);
   DCHECK_GT(pending_decode_requests_, 0);
 
   --pending_decode_requests_;
@@ -333,31 +321,60 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
     return;
   }
 
-  if (status == Decoder::kDecodeError) {
-    state_ = STATE_ERROR;
-    ready_outputs_.clear();
-    if (!read_cb_.is_null())
-      SatisfyRead(DECODE_ERROR, NULL);
+  // Drop decoding result if Reset() was called during decoding.
+  // The resetting process will be handled when the decoder is reset.
+  if (!reset_cb_.is_null())
     return;
-  }
 
-  if (status == Decoder::kDecryptError) {
-    state_ = STATE_ERROR;
-    ready_outputs_.clear();
-    if (!read_cb_.is_null())
-      SatisfyRead(DECRYPT_ERROR, NULL);
-    return;
+  switch (status) {
+    case Decoder::kDecodeError:
+      state_ = STATE_ERROR;
+      ready_outputs_.clear();
+      if (!read_cb_.is_null())
+        SatisfyRead(DECODE_ERROR, NULL);
+      break;
+
+    case Decoder::kDecryptError:
+      state_ = STATE_ERROR;
+      ready_outputs_.clear();
+      if (!read_cb_.is_null())
+        SatisfyRead(DECRYPT_ERROR, NULL);
+      break;

[xhwang, 2014/05/29 22:15:14]

nit: DECRYPT_ERROR is going to be obsolete. Feel free to handle these two cases
together and always return DECODE_ERROR. Or you can leave it as is and I'll
clean them up.

[Sergey Ulanov, 2014/06/03 00:08:11]

Done.

+
+    case Decoder::kAborted:

[xhwang, 2014/05/29 22:15:14]

IIUIC, This should only happen during Reset(), which is covered by l.326. Can we
DCHECK that this should not happen?

[Sergey Ulanov, 2014/06/03 00:08:11]

No. This came up with a previous CL as well:
https://codereview.chromium.org/239893002/diff/100001/media/filters/decoder_s...
(line 350). There are cases when we get into this state without Reset().

[xhwang, 2014/06/05 21:53:48]

hmm... Thanks for reminding me of that discussion! 

If you look at l.463 (OnBufferReady), if DemuxerStream::kAborted is received,
we'll abort the |read_cb_| immediately with AudioDecoder::kAborted. In other
words, the decoder only takes normal buffers or EOS as inputs. And the only case
it could return kAborted is because of Reset()/Stop().

Now I looked at the tests [Audio|Video]RendererImplTest.AbortPendingRead_*
again. They are testing something that should never happen. (They could happen
previously because decoder calls DemuxerStream::Read().) So I think these tests
are obsolete now and we may just remove them.

[Sergey Ulanov, 2014/06/06 22:49:39]

Done.

+      if (!read_cb_.is_null())
+        SatisfyRead(ABORTED, NULL);
+      break;
+
+    case Decoder::kOk:
+      // Any successful decode counts!
+      if (buffer_size > 0) {
+        StreamTraits::ReportStatistics(statistics_cb_, buffer_size);
+      }
+
+      if (state_ == STATE_NORMAL) {
+        if (CanDecodeMore() && !end_of_stream)
+          ReadFromDemuxerStream();
+      } else if (state_ == STATE_FLUSHING_DECODER) {
+        if (!pending_decode_requests_)
+          ReinitializeDecoder();

[xhwang, 2014/05/29 22:15:14]

Hmm, we should only ReinitializeDecoder() after the decoder is flushed (reset).
Why don't we do it on line 386 instead of here?

[Sergey Ulanov, 2014/06/03 00:08:11]

ReinitializeDecoder() should be called only after all Decode() requests have
been processes when the decoder supports parallel decoding. That's why we need
the !pending_decode_requests_ check above. That condition won't be true in
OnDecodeOutputReady(). When Decoder::Decode(EOS) is called the decoder
implementation must call output_cb for each output frame in the queue, before
decode_cb can be called - I've added comments about it in video_decoder.h .
I.e. the requirements for how EOS buffer should be handled are different from
regular buffers. For regular frames |output_cb| may be called before or after
|decode_cb| (and GPU decoder calls |output_cb| after |decode_cb|), but when
processing EOS buffer decoders must always call output_cb before decode_cb.

I think it would be better if we had VideoDecoder::Flush() instead of passing
EOS buffer to the decoder, but I don't think it has to be in this CL.

[xhwang, 2014/06/05 21:53:48]

Now I understand that comment. Thanks.
This makes sense. Previously we need multiple Decode(EOS) calls to flush the
decode. Now since we have OutputCB, we only need one call. Agreed that Flush()
will be cleaner.

+      }
+      break;
   }
+}
 
-  if (status == Decoder::kAborted) {
-    if (!read_cb_.is_null())
-      SatisfyRead(ABORTED, NULL);
-    return;
-  }
+template <DemuxerStream::Type StreamType>
+void DecoderStream<StreamType>::OnDecodeOutputReady(
+    const scoped_refptr<Output>& output) {
+  FUNCTION_DVLOG(2) << output;
+  DCHECK(output);
+  DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
+         state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
+      << state_;
 
-  // Any successful decode counts!
-  if (buffer_size > 0) {
-    StreamTraits::ReportStatistics(statistics_cb_, buffer_size);
+  if (state_ == STATE_ERROR) {
+    DCHECK(read_cb_.is_null());
+    return;
   }
 
   // Drop decoding result if Reset() was called during decoding.
@@ -365,29 +382,13 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   if (!reset_cb_.is_null())
     return;
 
-  // Decoder flushed. Reinitialize the decoder.
-  if (state_ == STATE_FLUSHING_DECODER &&
-      status == Decoder::kOk && output->end_of_stream()) {
-    ReinitializeDecoder();
-    return;
-  }
-
-  if (status == Decoder::kNotEnoughData) {
-    if (state_ == STATE_NORMAL)
-      ReadFromDemuxerStream();
-    else if (state_ == STATE_FLUSHING_DECODER)
-      FlushDecoder();
+  if (state_ == STATE_FLUSHING_DECODER && output->end_of_stream()) {
+    // ReinitializeDecoder() will be called from OnDecodeDone().

[xhwang, 2014/05/29 22:15:14]

See comment above about when to ReinitializeDecoder().

[Sergey Ulanov, 2014/06/03 00:08:11]

See my comment - it's better to keep it in OnDecodeDone()

     return;
   }
 
-  DCHECK(output);
-
   // Store decoded output.
   ready_outputs_.push_back(output);
-  scoped_refptr<Output> extra_output;
-  while ((extra_output = decoder_->GetDecodeOutput()) != NULL) {
-    ready_outputs_.push_back(extra_output);
-  }
 
   // Satisfy outstanding read request, if any.
   if (!read_cb_.is_null()) {
@@ -494,6 +495,8 @@ void DecoderStream<StreamType>::ReinitializeDecoder() {
       StreamTraits::GetDecoderConfig(*stream_),
       low_delay_,
       base::Bind(&DecoderStream<StreamType>::OnDecoderReinitialized,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
                  weak_factory_.GetWeakPtr()));
 }