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..731eb5d24ecda24ead065de092acc08da05d4218 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -59,6 +59,7 @@ DecoderStream<StreamType>::DecoderStream(
       decoding_eos_(false),
       pending_decode_requests_(0),
       duration_tracker_(8),
+      sequence_token_(0),
       weak_factory_(this) {}
 
 template <DemuxerStream::Type StreamType>
@@ -80,6 +81,7 @@ DecoderStream<StreamType>::~DecoderStream() {
     task_runner_->PostTask(FROM_HERE, base::ResetAndReturn(&reset_cb_));
 
   stream_ = NULL;
+  pending_buffers_.clear();
   decoder_.reset();
   decrypting_demuxer_stream_.reset();
 }
@@ -319,12 +321,14 @@ void DecoderStream<StreamType>::Decode(
   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>
@@ -333,10 +337,15 @@ void DecoderStream<StreamType>::FlushDecoder() {
 }
 
 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_;
@@ -363,6 +372,24 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
 
   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();
@@ -408,6 +435,13 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
          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;
@@ -418,6 +452,8 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   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().
@@ -432,7 +468,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 +480,13 @@ void DecoderStream<StreamType>::ReadFromDemuxerStream() {
   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()));
@@ -458,10 +501,25 @@ void DecoderStream<StreamType>::OnBufferReady(
                                      : "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);
@@ -475,6 +533,9 @@ void DecoderStream<StreamType>::OnBufferReady(
     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();