Add callback in VideoDecoder and AudioDecoder to return decoded frames.

media/filters/decoder_stream.cc

 // Copyright 2014 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/decoder_stream.h"
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/debug/trace_event.h"
 #include "base/location.h"
@@ skipping 62 matching lines @@
   statistics_cb_ = statistics_cb;
   init_cb_ = init_cb;
   stream_ = stream;
   low_delay_ = low_delay;
 
   state_ = STATE_INITIALIZING;
   // TODO(xhwang): DecoderSelector only needs a config to select a decoder.
   decoder_selector_->SelectDecoder(
       stream, low_delay,
       base::Bind(&DecoderStream<StreamType>::OnDecoderSelected,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
                  weak_factory_.GetWeakPtr()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::Read(const ReadCB& read_cb) {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
          state_ == STATE_ERROR || state_ == STATE_REINITIALIZING_DECODER ||
          state_ == STATE_PENDING_DEMUXER_READ)
@@ skipping 13 matching lines @@
                                       scoped_refptr<Output>()));
     return;
   }
 
   if (!ready_outputs_.empty()) {
     task_runner_->PostTask(FROM_HERE, base::Bind(
         base::ResetAndReturn(&read_cb_), OK, ready_outputs_.front()));
     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();
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::Reset(const base::Closure& closure) {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(state_ != STATE_UNINITIALIZED && state_ != STATE_STOPPED) << state_;
   DCHECK(reset_cb_.is_null());
   DCHECK(stop_cb_.is_null());
@@ skipping 70 matching lines @@
   state_ = STATE_STOPPED;
   stream_ = NULL;
   decoder_.reset();
   decrypting_demuxer_stream_.reset();
   task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&stop_cb_));
 }
 
 template <DemuxerStream::Type StreamType>
 bool DecoderStream<StreamType>::CanReadWithoutStalling() const {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return decoder_->CanReadWithoutStalling();

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

!ready_outputs_.empty() || decoder_->CanReadWithoutStalling() ?

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

Done.

 }
 
 template <>
 bool DecoderStream<DemuxerStream::AUDIO>::CanReadWithoutStalling() const {
   DCHECK(task_runner_->BelongsToCurrentThread());
   return true;
 }
 
 template <DemuxerStream::Type StreamType>
 bool DecoderStream<StreamType>::CanDecodeMore() const {
@@ skipping 63 matching lines @@
   DCHECK(CanDecodeMore());
   DCHECK(reset_cb_.is_null());
   DCHECK(stop_cb_.is_null());
   DCHECK(buffer);
 
   int buffer_size = buffer->end_of_stream() ? 0 : buffer->data_size();
 
   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_;
 
   TRACE_EVENT_ASYNC_END0("media", GetTraceString<StreamType>(), this);
 
   if (state_ == STATE_ERROR) {
     DCHECK(read_cb_.is_null());
     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;
+
+  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;
+  }
+}
+
+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_;
+
+  if (state_ == STATE_ERROR) {
+    DCHECK(read_cb_.is_null());
     return;
   }
 
-  if (status == Decoder::kDecryptError) {
-    state_ = STATE_ERROR;
-    ready_outputs_.clear();
-    if (!read_cb_.is_null())
-      SatisfyRead(DECRYPT_ERROR, NULL);
-    return;
-  }
-
-  if (status == Decoder::kAborted) {
-    if (!read_cb_.is_null())
-      SatisfyRead(ABORTED, NULL);
-    return;
-  }
-
-  // Any successful decode counts!
-  if (buffer_size > 0) {
-    StreamTraits::ReportStatistics(statistics_cb_, buffer_size);
-  }
-
   // 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;
 
-  // Decoder flushed. Reinitialize the decoder.
-  if (state_ == STATE_FLUSHING_DECODER &&
-      status == Decoder::kOk && output->end_of_stream()) {
-    ReinitializeDecoder();
+  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;
   }
 
-  if (status == Decoder::kNotEnoughData) {
-    if (state_ == STATE_NORMAL)
-      ReadFromDemuxerStream();
-    else if (state_ == STATE_FLUSHING_DECODER)
-      FlushDecoder();
-    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()) {
     scoped_refptr<Output> read_result = ready_outputs_.front();
     ready_outputs_.pop_front();
     SatisfyRead(OK, output);
   }
 }
 
 template <DemuxerStream::Type StreamType>
@@ skipping 52 matching lines @@
 
   if (!reset_cb_.is_null()) {
     // If we are using DecryptingDemuxerStream, we already called DDS::Reset()
     // which will continue the resetting process in it's callback.
     if (!decrypting_demuxer_stream_)
       Reset(base::ResetAndReturn(&reset_cb_));
     return;
   }
 
   if (status == DemuxerStream::kAborted) {
     SatisfyRead(DEMUXER_READ_ABORTED, NULL);

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

hmm, actually, shall we check whether read_cb_.is_null() before calling
SatisfyRead()?

     return;
   }
 
   if (!splice_observer_cb_.is_null() && !buffer->end_of_stream()) {
     const bool has_splice_ts = buffer->splice_timestamp() != kNoTimestamp();
     if (active_splice_ || has_splice_ts) {
       splice_observer_cb_.Run(buffer->splice_timestamp());
       active_splice_ = has_splice_ts;
     }
   }
@@ skipping 13 matching lines @@
   DCHECK_EQ(state_, STATE_FLUSHING_DECODER) << state_;
   DCHECK_EQ(pending_decode_requests_, 0);
 
   DCHECK(StreamTraits::GetDecoderConfig(*stream_).IsValidConfig());
   state_ = STATE_REINITIALIZING_DECODER;
   DecoderStreamTraits<StreamType>::Initialize(
       decoder_.get(),
       StreamTraits::GetDecoderConfig(*stream_),
       low_delay_,
       base::Bind(&DecoderStream<StreamType>::OnDecoderReinitialized,
+                 weak_factory_.GetWeakPtr()),
+      base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
                  weak_factory_.GetWeakPtr()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::OnDecoderReinitialized(PipelineStatus status) {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, STATE_REINITIALIZING_DECODER) << state_;
   DCHECK(stop_cb_.is_null());
 
@@ skipping 68 matching lines @@
   decrypting_demuxer_stream_.reset();
   // Post |stop_cb_| because pending |read_cb_| and/or |reset_cb_| are also
   // posted in Stop().
   task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&stop_cb_));
 }
 
 template class DecoderStream<DemuxerStream::VIDEO>;
 template class DecoderStream<DemuxerStream::AUDIO>;
 
 }  // namespace media