Remove pending demuxer read state, prefer flag, it's not stable.

The pending demuxer read state is overwritten in too many cases to
be reliable, instead always use the flag instead of trying to have
it both ways.

The main issue this fixes is where there are parallel decodes and a
demuxer read outstanding, one decode completes with an error, and
the new decoder is initialized before the demuxer read completes.

Previously this would stomp the pending read state and we'd try to
issue a new demuxer read when the decoder initialization completed,
blowing up the demuxer in the process.

BUG=597605
TEST=new unittest

Review-Url: https://codereview.chromium.org/2110463003
Cr-Commit-Position: refs/heads/master@{#402667}

media/filters/decoder_stream.cc

@@ -397,7 +397,7 @@ void DecoderStream<StreamType>::OnDecodeDone(int buffer_size,
                                              DecodeStatus status) {
   FUNCTION_DVLOG(2) << ": " << status;
   DCHECK(state_ == STATE_NORMAL || state_ == STATE_FLUSHING_DECODER ||
-         state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
+         state_ == STATE_ERROR)
       << state_;
   DCHECK_GT(pending_decode_requests_, 0);
 
@@ -481,7 +481,7 @@ void DecoderStream<StreamType>::OnDecodeOutputReady(
   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_ == STATE_ERROR)
       << state_;
 
   if (state_ == STATE_ERROR) {
@@ -539,12 +539,11 @@ void DecoderStream<StreamType>::ReadFromDemuxerStream() {
     return;
   }
 
-  // Set a flag in addition to the state, because the state can be overwritten
-  // when encountering an error. See crbug.com/597605.
-  DCHECK(!pending_demuxer_read_);
-  pending_demuxer_read_ = true;
+  // We may get here when a read is already pending, ignore this.
+  if (pending_demuxer_read_)
+    return;
 
-  state_ = STATE_PENDING_DEMUXER_READ;
+  pending_demuxer_read_ = true;
   stream_->Read(base::Bind(&DecoderStream<StreamType>::OnBufferReady,
                            weak_factory_.GetWeakPtr()));
 }
@@ -560,17 +559,13 @@ void DecoderStream<StreamType>::OnBufferReady(
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK(pending_demuxer_read_);
   if (decoded_frames_since_fallback_) {
-    DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR)
-        << state_;
+    DCHECK(pending_demuxer_read_ || state_ == STATE_ERROR) << state_;
   } else {
-    DCHECK(state_ == STATE_PENDING_DEMUXER_READ || state_ == STATE_ERROR ||
-           state_ == STATE_REINITIALIZING_DECODER || state_ == STATE_NORMAL)
+    DCHECK(state_ == STATE_ERROR || state_ == STATE_REINITIALIZING_DECODER ||
+           state_ == STATE_NORMAL)
         << state_;
   }
   DCHECK_EQ(buffer.get() != NULL, status == DemuxerStream::kOk) << status;
-
-  // Unset the flag. STATE_PENDING_DEMUXER_READ might have been overwritten.
-  // See crbug.com/597605.
   pending_demuxer_read_ = false;
 
   // If parallel decode requests are supported, multiple read requests might

media/filters/decoder_stream.h

@@ -141,7 +141,6 @@ class MEDIA_EXPORT DecoderStream {
     STATE_INITIALIZING,
     STATE_NORMAL,  // Includes idle, pending decoder decode/reset.
     STATE_FLUSHING_DECODER,
-    STATE_PENDING_DEMUXER_READ,
     STATE_REINITIALIZING_DECODER,
     STATE_END_OF_STREAM,  // End of stream reached; returns EOS on all reads.
     STATE_ERROR,
@@ -257,8 +256,8 @@ class MEDIA_EXPORT DecoderStream {
   // crbug.com/603713
   bool received_config_change_during_reinit_;
 
-  // Used to track read requests in case the STATE_PENDIND_DEMUXER_READ get
-  // overwritten by an error.
+  // Used to track read requests; not rolled into |state_| since that is
+  // overwritten in many cases.
   bool pending_demuxer_read_;
 
   // NOTE: Weak pointers must be invalidated before all other member variables.

media/filters/video_frame_stream_unittest.cc

@@ -810,6 +810,42 @@ TEST_P(VideoFrameStreamTest, FallbackDecoder_EndOfStreamReachedBeforeFallback) {
       frame_read_->metadata()->IsTrue(VideoFrameMetadata::END_OF_STREAM));
 }
 
+TEST_P(VideoFrameStreamTest, FallbackDecoder_DoesReinitializeStompPendingRead) {
+  // Test only the case where there is no decoding delay and parallel decoding.
+  if (GetParam().decoding_delay != 0 || GetParam().parallel_decoding <= 1)
+    return;
+
+  Initialize();
+  decoder1_->HoldDecode();
+
+  // Queue one read, defer the second.
+  frame_read_ = nullptr;
+  pending_read_ = true;
+  video_frame_stream_->Read(
+      base::Bind(&VideoFrameStreamTest::FrameReady, base::Unretained(this)));
+  demuxer_stream_->HoldNextRead();
+
+  // Force an error to occur on the first decode, but ensure it isn't propagated
+  // until after the next read has been started.
+  decoder1_->SimulateError();
+  decoder2_->HoldDecode();
+
+  // Complete the fallback to the second decoder with the read still pending.
+  base::RunLoop().RunUntilIdle();
+
+  // Can't check |decoder1_| right now, it might have been destroyed already.
+  // Verify that there was nothing decoded until we kicked the decoder.
+  EXPECT_EQ(decoder2_->total_bytes_decoded(), 0);
+  decoder2_->SatisfyDecode();
+  const int first_decoded_bytes = decoder2_->total_bytes_decoded();
+  ASSERT_GT(first_decoded_bytes, 0);
+
+  // Satisfy the previously pending read and ensure it is decoded.
+  demuxer_stream_->SatisfyRead();
+  base::RunLoop().RunUntilIdle();
+  ASSERT_GT(decoder2_->total_bytes_decoded(), first_decoded_bytes);
+}
+
 TEST_P(VideoFrameStreamTest,
        FallbackDecoder_SelectedOnInitialDecodeError_Twice) {
   Initialize();