Attempt decoder fallback if first decode fails

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 <utility>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
@@ skipping 41 matching lines @@
       state_(STATE_UNINITIALIZED),
       stream_(NULL),
       decoder_selector_(new DecoderSelector<StreamType>(task_runner,
                                                         std::move(decoders),
                                                         media_log)),
       decoded_frames_since_fallback_(0),
       active_splice_(false),
       decoding_eos_(false),
       pending_decode_requests_(0),
       duration_tracker_(8),
+      sequence_token_(0),
       weak_factory_(this) {}
 
 template <DemuxerStream::Type StreamType>
 DecoderStream<StreamType>::~DecoderStream() {
   FUNCTION_DVLOG(2);
   DCHECK(task_runner_->BelongsToCurrentThread());
 
   decoder_selector_.reset();
 
   if (!init_cb_.is_null()) {
     task_runner_->PostTask(FROM_HERE,
                            base::Bind(base::ResetAndReturn(&init_cb_), false));
   }
   if (!read_cb_.is_null()) {
     task_runner_->PostTask(FROM_HERE, base::Bind(
         base::ResetAndReturn(&read_cb_), ABORTED, scoped_refptr<Output>()));
   }
   if (!reset_cb_.is_null())
     task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
   stream_ = NULL;
+  pending_buffers_.clear();
   decoder_.reset();
   decrypting_demuxer_stream_.reset();
 }
 
 template <DemuxerStream::Type StreamType>
 std::string DecoderStream<StreamType>::GetStreamTypeString() {
   return DecoderStreamTraits<StreamType>::ToString();
 }
 
 template <DemuxerStream::Type StreamType>
@@ skipping 219 matching lines @@
   TRACE_EVENT_ASYNC_BEGIN2(
       "media", GetTraceString<StreamType>(), this, "key frame",
       !buffer->end_of_stream() && buffer->is_key_frame(), "timestamp (ms)",
       !buffer->end_of_stream() ? buffer->timestamp().InMilliseconds() : 0);
 
   if (buffer->end_of_stream())
     decoding_eos_ = true;
   else if (buffer->duration() != kNoTimestamp())
     duration_tracker_.AddSample(buffer->duration());
 
+  if (!previous_decoder_ && !decoded_frames_since_fallback_ && !decoding_eos_)
+    pending_buffers_.push_back(buffer);

[sandersd (OOO until July 31), 2016/04/14 00:38:04]

Why don't we queue EOS buffers?

[tguilbert, 2016/04/14 22:08:35]

In order to reinitialize the decoders on a config change, we flush the decoder
by sending it an EOS. The first buffer that would be queued in that case is an
EOS. I will make sure to only reject EOS if we are flushing the decoder, and
will add a comment.

+
   ++pending_decode_requests_;
   decoder_->Decode(buffer,
                    base::Bind(&DecoderStream<StreamType>::OnDecodeDone,
-                              weak_factory_.GetWeakPtr(),
-                              buffer_size,
-                              buffer->end_of_stream()));
+                              weak_factory_.GetWeakPtr(), sequence_token_,

[sandersd (OOO until July 31), 2016/04/14 00:38:04]

If OnDecodeDone() should never be called when the sequence token is old, it may
be simpler to create a second WeakPtrFactory just for this Bind(). Then you can
call Invalidate() to prevent any more callbacks from coming. (You can still vend
new weak pointers from the same factory.)

[tguilbert, 2016/04/14 22:08:35]

I did not know this was something that could be done. This is great, thank you
:)

+                              buffer_size, buffer->end_of_stream()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::FlushDecoder() {
   Decode(DecoderBuffer::CreateEOSBuffer());
 }
 
 template <DemuxerStream::Type StreamType>
-void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
+void DecoderStream<StreamType>::OnDecodeDone(int sequence_token,
+                                             int buffer_size,
                                              bool end_of_stream,
                                              DecodeStatus status) {
   FUNCTION_DVLOG(2) << ": " << status;
+  // Ignore stale calls from a previous decoder.
+  if (sequence_token_ != sequence_token)
+    return;
+
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
          state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
       << state_;
   DCHECK_GT(pending_decode_requests_, 0);
 
   --pending_decode_requests_;
 
   TRACE_EVENT_ASYNC_END0("media", GetTraceString<StreamType>(), this);
 
   if (end_of_stream) {
     DCHECK(!pending_decode_requests_);
     decoding_eos_ = false;
   }
 
   if (state_ == STATE_ERROR) {
     DCHECK(read_cb_.is_null());
     return;
   }
 
   // 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 DecodeStatus::DECODE_ERROR:
+      if (!decoded_frames_since_fallback_) {
+        pending_decode_requests_ = 0;
+        // Note: increment |sequence_token_| here rather than in
+        // OnDecoderSelected. This covers the case where parallel decode
+        // requests were sent to |decoder_|, and responses are received before
+        // a new decoder is selected. This prevents |pending_decode_request_|
+        // from going below 0.
+        ++sequence_token_;
+        state_ = STATE_REINITIALIZING_DECODER;
+        decoder_selector_->SelectDecoder(
+            stream_, nullptr,
+            base::Bind(&DecoderStream<StreamType>::OnDecoderSelected,
+                       weak_factory_.GetWeakPtr()),
+            base::Bind(&DecoderStream<StreamType>::OnDecodeOutputReady,
+                       weak_factory_.GetWeakPtr()),
+            waiting_for_decryption_key_cb_);
+        return;
+      }
       state_ = STATE_ERROR;
       MEDIA_LOG(ERROR, media_log_) << GetStreamTypeString() << " decode error";
       ready_outputs_.clear();
       if (!read_cb_.is_null())
         SatisfyRead(DECODE_ERROR, NULL);
       return;
 
     case DecodeStatus::ABORTED:
       // Decoder can return DecodeStatus::ABORTED during Reset() or during
       // destruction.
@@ skipping 25 matching lines @@
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::OnDecodeOutputReady(
     const scoped_refptr<Output>& output) {
   FUNCTION_DVLOG(2) << ": " << output->timestamp().InMilliseconds() << " ms";
   DCHECK(output.get());
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
          state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
       << state_;
 
+  // Note: Checking that we have decoded more frames than the max number of
+  // parallel requests ensures we don't delete the buffers before the fallback
+  // decoder has had a chance to use them.
+  if (!pending_buffers_.empty() &&

[DaleCurtis, 2016/04/14 00:42:53]

Multiline if needs {}

[tguilbert, 2016/04/14 22:08:35]

CL format pushed it to multiline :(...

Done.

+      decoded_frames_since_fallback_ > GetMaxDecodeRequests())
+    pending_buffers_.clear();
+
   if (state_ == STATE_ERROR) {
     DCHECK(read_cb_.is_null());
     return;
   }
 
   // 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;
 
+  ++decoded_frames_since_fallback_;
+
   if (!read_cb_.is_null()) {
     // If |ready_outputs_| was non-empty, the read would have already been
     // satisifed by Read().
     DCHECK(ready_outputs_.empty());
     SatisfyRead(OK, output);
     return;
   }
 
   // Store decoded output.
   ready_outputs_.push_back(output);
 
   // Destruct any previous decoder once we've decoded enough frames to ensure
   // that it's no longer in use.
   if (previous_decoder_ &&
-      ++decoded_frames_since_fallback_ > limits::kMaxVideoFrames) {
+      decoded_frames_since_fallback_ > limits::kMaxVideoFrames) {
     previous_decoder_.reset();
   }
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::ReadFromDemuxerStream() {
   FUNCTION_DVLOG(2);
   DCHECK_EQ(state_, STATE_NORMAL);
   DCHECK(CanDecodeMore());
   DCHECK(reset_cb_.is_null());
 
+  if (!pending_buffers_.empty() && previous_decoder_) {

[sandersd (OOO until July 31), 2016/04/14 00:38:04]

It seems that there really needs to be two lists of pending buffers; one is what
has been fed in so far, the other is what has yet to be sent to the current
decoder. Then when switching decoders the second list can just be reset to a
copy of the first, and there are no strange conditions to check.

It may be easiest to do that by splitting Decode() into two methods: a wrapper
that records to that list (Decode()), and an internal method that actually
dispatches (DoDecode() or DecodeInternal()). Then you can call the internal one
here and don't need to worry about the pending list being walked on.

[tguilbert, 2016/04/14 22:08:35]

Good idea. Really simplifies the logic in many places, and adds support for
multiple consecutive failures.

+    scoped_refptr<DecoderBuffer> buffer = pending_buffers_.front();
+    pending_buffers_.pop_front();
+    Decode(buffer);
+    return;
+  }
+
   state_ = STATE_PENDING_DEMUXER_READ;
   stream_->Read(base::Bind(&DecoderStream<StreamType>::OnBufferReady,
                            weak_factory_.GetWeakPtr()));
 }
 
 template <DemuxerStream::Type StreamType>
 void DecoderStream<StreamType>::OnBufferReady(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
   FUNCTION_DVLOG(2) << ": " << status << ", "
                     << (buffer.get() ? buffer->AsHumanReadableString()
                                      : "NULL");
 
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
+  DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR ||
+         STATE_REINITIALIZING_DECODER)
       << state_;
   DCHECK_EQ(buffer.get() != NULL, status == DemuxerStream::kOk) << status;
 
+  // If parallel decode requests are supported, multiple read requests might
+  // have been sent to the demuxer. The buffers might arrive while the decoder
+  // is reinitializing after falling back on first decode error.
+  if (state_ == STATE_REINITIALIZING_DECODER &&
+      !decoded_frames_since_fallback_) {
+    // Save valid buffers to be consumed by the new decoder

[DaleCurtis, 2016/04/14 00:42:53]

You'll need to set STATE_ERROR in the non-kOk case and adapt
OnDecoderReinitialized to handle it I think.

[tguilbert, 2016/04/14 22:08:35]

Talked a bit offline with Dan. Created
https://bugs.chromium.org/p/chromium/issues/detail?id=603713

+    // TODO(tguilbert): Question for CR: what is the proper way to error out?
+    if (status == DemuxerStream::kOk)
+      pending_buffers_.push_back(buffer);
+    else
+      pending_buffers_.clear();
+    return;
+  }
+
   // Decoding has been stopped (e.g due to an error).
   if (state_ != STATE_PENDING_DEMUXER_READ) {
     DCHECK(state_ == STATE_ERROR);
     DCHECK(read_cb_.is_null());
     return;
   }
 
   state_ = STATE_NORMAL;
 
   if (status == DemuxerStream::kConfigChanged) {
     FUNCTION_DVLOG(2) << ": " << "ConfigChanged";
     DCHECK(stream_->SupportsConfigChanges());
 
+    // Pending buffers might not match the reinitiliazed decoder's new config

[DaleCurtis, 2016/04/14 00:42:53]

DCHECK(pending_buffers_.empty()); ? I think this can't happen with your above
code. Also misspelled reinitialized.

[tguilbert, 2016/04/14 22:08:35]

This is when we receive a normal config change before having outputted a frame,
which can still happen I think. This is not when we receive a config change
while reinitializing. 

+    pending_buffers_.clear();
+
     if (!config_change_observer_cb_.is_null())
       config_change_observer_cb_.Run();
 
     state_ = STATE_FLUSHING_DECODER;
     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_));
       // Reinitialization will continue after Reset() is done.
@@ skipping 131 matching lines @@
   }
 
   // The resetting process will be continued in OnDecoderReinitialized().
   ReinitializeDecoder();
 }
 
 template class DecoderStream<DemuxerStream::VIDEO>;
 template class DecoderStream<DemuxerStream::AUDIO>;
 
 }  // namespace media