Attempt decoder fallback if first decode fails

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 19ae054d64e7ab73d786a44d5050bca508ca7390..9d40eb0abc6a2da2b36fcdfd5baba8702c8d6390 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -59,7 +59,8 @@ DecoderStream<StreamType>::DecoderStream(
       decoding_eos_(false),
       pending_decode_requests_(0),
       duration_tracker_(8),
-      weak_factory_(this) {}
+      weak_factory_(this),
+      decode_weak_factory_(this) {}
 
 template <DemuxerStream::Type StreamType>
 DecoderStream<StreamType>::~DecoderStream() {
@@ -80,6 +81,8 @@ DecoderStream<StreamType>::~DecoderStream() {
     task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
   stream_ = NULL;
+  pending_buffers_.clear();

[sandersd (OOO until July 31), 2016/04/14 23:10:30]

You don't actually need to do this, the destructor will handle it for you.

[tguilbert, 2016/04/15 00:29:18]

Done.

+  fallback_buffers_.clear();
   decoder_.reset();
   decrypting_demuxer_stream_.reset();
 }
@@ -243,16 +246,33 @@ void DecoderStream<StreamType>::OnDecoderSelected(
                     << (selected_decoder ? selected_decoder->GetDisplayName()
                                          : "No decoder selected.");
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ == STATE_INITIALIZING || state_ == STATE_REINITIALIZING_DECODER)
+  DCHECK(state_ == STATE_INITIALIZING ||
+         state_ == STATE_REINITIALIZING_DECODER ||
+         state_ == STATE_CONFIG_CHANGE_RECEIVED_WHILE_REINITIALIZING_DECODER)
       << state_;
   if (state_ == STATE_INITIALIZING) {
     DCHECK(!init_cb_.is_null());
     DCHECK(read_cb_.is_null());
     DCHECK(reset_cb_.is_null());
-  } else {
+  } else if (state_ == STATE_REINITIALIZING_DECODER) {
     DCHECK(decoder_);
   }
 
+  // TODO(tguilbert): crbug.com/603713 support config changes on decoder reinit.
+  if (state_ == STATE_CONFIG_CHANGE_RECEIVED_WHILE_REINITIALIZING_DECODER) {
+    CompleteDecoderReinitialization(false);

[tguilbert, 2016/04/14 22:26:42]

Sorry, I didn't take into account what should be done with |selected_decoder|.
Looking into this now.

[tguilbert, 2016/04/15 00:29:18]

Done.

+    return;
+  }
+
+  // Push the pending buffers to the front of the fallback buffers
+  if (fallback_buffers_.empty()) {
+    fallback_buffers_.swap(pending_buffers_);
+  } else {
+    fallback_buffers_.insert(fallback_buffers_.begin(),
+                             pending_buffers_.begin(), pending_buffers_.end());
+    pending_buffers_.clear();
+  }
+
   previous_decoder_ = std::move(decoder_);
   decoded_frames_since_fallback_ = 0;
   decoder_ = std::move(selected_decoder);
@@ -319,12 +339,17 @@ void DecoderStream<StreamType>::Decode(
   else if (buffer->duration() != kNoTimestamp())
     duration_tracker_.AddSample(buffer->duration());
 
+  // Don't save the buffer if we have successfully decoded a frame or if it's
+  // the EOS buffer we send to flush the decoder.

[sandersd (OOO until July 31), 2016/04/14 23:10:30]

I still think this would be more clear if ReadFromDemuxerStream() called a
shared helper which would never push buffers into |pending_buffers_|. Right now
it's not trivial to understand the exact usage of the two lists, nor the
conditions under which they should be updated.

[tguilbert, 2016/04/15 00:29:18]

Done. After our offline discussion, I realized you probably meant FlushDecoder()
instead of ReadFromDemuxerStream() (which was the source of my confusion).

+  if (!decoded_frames_since_fallback_ &&
+      !(state_ == STATE_FLUSHING_DECODER && buffer->end_of_stream())) {
+    pending_buffers_.push_back(buffer);
+  }
+
   ++pending_decode_requests_;
-  decoder_->Decode(buffer,
-                   base::Bind(&DecoderStream<StreamType>::OnDecodeDone,
-                              weak_factory_.GetWeakPtr(),
-                              buffer_size,
-                              buffer->end_of_stream()));
+  decoder_->Decode(buffer, base::Bind(&DecoderStream<StreamType>::OnDecodeDone,
+                                      decode_weak_factory_.GetWeakPtr(),
+                                      buffer_size, buffer->end_of_stream()));
 }
 
 template <DemuxerStream::Type StreamType>
@@ -337,6 +362,7 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
                                              bool end_of_stream,
                                              DecodeStatus status) {
   FUNCTION_DVLOG(2) << ": " << status;
+
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
          state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
       << state_;
@@ -363,6 +389,21 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
 
   switch (status) {
     case DecodeStatus::DECODE_ERROR:
+      if (!decoded_frames_since_fallback_) {
+        pending_decode_requests_ = 0;
+        // Prevent all pending decode requests from being called back.

[sandersd (OOO until July 31), 2016/04/14 23:10:30]

Newline before comment.

[tguilbert, 2016/04/15 00:29:18]

Done.

+        decode_weak_factory_.InvalidateWeakPtrs();
+
+        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();
@@ -418,6 +459,11 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   if (!reset_cb_.is_null())
     return;
 
+  ++decoded_frames_since_fallback_;
+
+  if (!pending_buffers_.empty())

[sandersd (OOO until July 31), 2016/04/14 23:10:30]

No need for a condition, it can just be unconditionally cleared.

[tguilbert, 2016/04/15 00:29:18]

Oops! Artifact of the previous patchset.

+    pending_buffers_.clear();
+
   if (!read_cb_.is_null()) {
     // If |ready_outputs_| was non-empty, the read would have already been
     // satisifed by Read().
@@ -432,7 +478,7 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   // 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();
   }
 }
@@ -444,6 +490,13 @@ void DecoderStream<StreamType>::ReadFromDemuxerStream() {
   DCHECK(CanDecodeMore());
   DCHECK(reset_cb_.is_null());
 
+  if (!fallback_buffers_.empty()) {
+    scoped_refptr<DecoderBuffer> buffer = fallback_buffers_.front();
+    fallback_buffers_.pop_front();
+    Decode(buffer);
+    return;
+  }
+
   state_ = STATE_PENDING_DEMUXER_READ;
   stream_->Read(base::Bind(&DecoderStream<StreamType>::OnBufferReady,
                            weak_factory_.GetWeakPtr()));
@@ -458,10 +511,48 @@ void DecoderStream<StreamType>::OnBufferReady(
                                      : "NULL");
 
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
-      << state_;
+  if (decoded_frames_since_fallback_) {
+    DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
+        << state_;
+  } else {
+    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_) {
+    switch (status) {
+      case DemuxerStream::kOk:
+        // Save valid buffers to be consumed by the new decoder.
+        // |pending_buffers_| is copied to |fallback_buffers| in
+        // OnDecoderSelected().
+        pending_buffers_.push_back(buffer);
+        break;
+      case DemuxerStream::kConfigChanged:
+        // TODO(tguilbert): crbug.com/603713
+        // |decoder_| might have a stale config by the time it is reinitialized.
+        // Ideally, we would save the config from |stream_| and reinitialize the
+        // decoder by playing back the sequence of buffers and config changes.
+        state_ = STATE_CONFIG_CHANGE_RECEIVED_WHILE_REINITIALIZING_DECODER;
+        pending_buffers_.clear();
+        break;
+      case DemuxerStream::kAborted:
+        // |this| will read from the demuxer stream again in OnDecoderSelected()
+        // and receive a kAborted then.
+        pending_buffers_.clear();
+        break;
+      default:
+        NOTREACHED();
+        break;
+    }
+    return;
+  }
+
   // Decoding has been stopped (e.g due to an error).
   if (state_ != STATE_PENDING_DEMUXER_READ) {
     DCHECK(state_ == STATE_ERROR);
@@ -475,6 +566,9 @@ void DecoderStream<StreamType>::OnBufferReady(
     FUNCTION_DVLOG(2) << ": " << "ConfigChanged";
     DCHECK(stream_->SupportsConfigChanges());
 
+    // Pending buffers might not match the reinitialiazed decoder's new config
+    pending_buffers_.clear();
+
     if (!config_change_observer_cb_.is_null())
       config_change_observer_cb_.Run();