media: Add kError to DemuxerStream::Status

Some DemuxerStream could hit non-recoverable fatal errors. For example,
DecryptingDemuxerStream cannot decrypt a buffer because the CDM was
crashed.

This CL adds a new kError status to DemuxerStream::Status to indicate
such conditions.

BUG=730766
TEST=New unittests added.

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: Ia07e14662d7da852f712076dcc94aed1900aaba3
Reviewed-on: https://chromium-review.googlesource.com/598702
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Yuri Wiitala <miu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491960}

diff --git a/media/base/demuxer_stream.h b/media/base/demuxer_stream.h
index ad6f215..a88c1ca4 100644
--- a/media/base/demuxer_stream.h
+++ b/media/base/demuxer_stream.h

@@ -51,11 +51,13 @@
   //                  when this status is returned.
   //                  This will only be returned if SupportsConfigChanges()
   //                  returns 'true' for this DemuxerStream.
+  // kError : Unexpected fatal error happened. Playback should fail.
   enum Status {
     kOk,
     kAborted,
     kConfigChanged,
-    kStatusMax = kConfigChanged,
+    kError,
+    kStatusMax = kError,
   };
 
   // Request a buffer to returned via the provided callback.

diff --git a/media/base/fake_demuxer_stream.cc b/media/base/fake_demuxer_stream.cc
index a7883db5..fdbeee6 100644
--- a/media/base/fake_demuxer_stream.cc
+++ b/media/base/fake_demuxer_stream.cc

@@ -140,6 +140,13 @@
     base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
 }
 
+void FakeDemuxerStream::Error() {
+  read_to_hold_ = -1;
+
+  if (!read_cb_.is_null())
+    base::ResetAndReturn(&read_cb_).Run(kError, nullptr);
+}
+
 void FakeDemuxerStream::SeekToStart() {
   Reset();
   Initialize();

diff --git a/media/base/fake_demuxer_stream.h b/media/base/fake_demuxer_stream.h
index f081fa3..0360f29d 100644
--- a/media/base/fake_demuxer_stream.h
+++ b/media/base/fake_demuxer_stream.h

@@ -59,6 +59,10 @@
   // always clears |hold_next_read_|.
   void Reset();
 
+  // Satisfies the pending read (if any) with kError and NULL. This call
+  // always clears |hold_next_read_|.
+  void Error();
+
   // Reset() this demuxer stream and set the reading position to the start of
   // the stream.
   void SeekToStart();

diff --git a/media/base/fake_demuxer_stream_unittest.cc b/media/base/fake_demuxer_stream_unittest.cc
index def5f9ca..85d09f9 100644
--- a/media/base/fake_demuxer_stream_unittest.cc
+++ b/media/base/fake_demuxer_stream_unittest.cc

@@ -42,13 +42,7 @@
       num_buffers_received_++;
   }
 
-  enum ReadResult {
-    OK,
-    ABORTED,
-    CONFIG_CHANGED,
-    EOS,
-    PENDING
-  };
+  enum ReadResult { OK, ABORTED, CONFIG_CHANGED, READ_ERROR, EOS, PENDING };
 
   void EnterNormalReadState() {
     stream_.reset(
@@ -87,6 +81,12 @@
         EXPECT_FALSE(buffer_.get());
         break;
 
+      case READ_ERROR:
+        EXPECT_FALSE(read_pending_);
+        EXPECT_EQ(DemuxerStream::kError, status_);
+        EXPECT_FALSE(buffer_.get());
+        break;
+
       case EOS:
         EXPECT_FALSE(read_pending_);
         EXPECT_EQ(DemuxerStream::kOk, status_);
@@ -137,6 +137,16 @@
       ExpectReadResult(ABORTED);
   }
 
+  void Error() {
+    bool had_read_pending = read_pending_;
+    stream_->Error();
+    base::RunLoop().RunUntilIdle();
+
+    EXPECT_FALSE(read_pending_);
+    if (had_read_pending)
+      ExpectReadResult(READ_ERROR);
+  }
+
   void ReadAllBuffers(int num_configs, int num_buffers_in_one_config) {
     DCHECK_EQ(0, num_buffers_received_);
     for (int i = 0; i < num_configs; ++i) {
@@ -256,6 +266,35 @@
   ReadAndExpect(EOS);
 }
 
+TEST_F(FakeDemuxerStreamTest, Error_Normal) {
+  EnterNormalReadState();
+  Error();
+  ReadAndExpect(OK);
+}
+
+TEST_F(FakeDemuxerStreamTest, Error_AfterHoldRead) {
+  EnterNormalReadState();
+  stream_->HoldNextRead();
+  Error();
+  ReadAndExpect(OK);
+}
+
+TEST_F(FakeDemuxerStreamTest, Error_BeforeConfigChanged) {
+  EnterNormalReadState();
+  stream_->HoldNextConfigChangeRead();
+  ReadUntilPending();
+  Error();
+  ReadAndExpect(CONFIG_CHANGED);
+}
+
+TEST_F(FakeDemuxerStreamTest, Error_BeforeEOS) {
+  EnterBeforeEOSState();
+  stream_->HoldNextRead();
+  ReadAndExpect(PENDING);
+  Error();
+  ReadAndExpect(EOS);
+}
+
 TEST_F(FakeDemuxerStreamTest, NoConfigChanges) {
   stream_.reset(
       new FakeDemuxerStream(1, kNumBuffersInOneConfig, false));

diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index 368469c..ab12038 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc

@@ -595,14 +595,25 @@
         pending_buffers_.clear();
         break;
       case DemuxerStream::kAborted:
-        // |this| will read from the demuxer stream again in OnDecoderSelected()
-        // and receive a kAborted then.
+      case DemuxerStream::kError:
+        // Will read from the demuxer stream again in OnDecoderSelected().
         pending_buffers_.clear();
         break;
     }
     return;
   }
 
+  if (status == DemuxerStream::kError) {
+    FUNCTION_DVLOG(1) << ": Demuxer stream read error!";
+    state_ = STATE_ERROR;
+    MEDIA_LOG(ERROR, media_log_)
+        << GetStreamTypeString() << " demuxer stream read error!";
+    pending_buffers_.clear();
+    ready_outputs_.clear();
+    if (!read_cb_.is_null())
+      SatisfyRead(DECODE_ERROR, nullptr);
+  }
+
   // Decoding has been stopped.
   if (state_ == STATE_ERROR) {
     DCHECK(read_cb_.is_null());

diff --git a/media/filters/decrypting_audio_decoder_unittest.cc b/media/filters/decrypting_audio_decoder_unittest.cc
index fb34c76..b8bca93b1b 100644
--- a/media/filters/decrypting_audio_decoder_unittest.cc
+++ b/media/filters/decrypting_audio_decoder_unittest.cc

@@ -202,6 +202,7 @@
     decoder_->Decode(encrypted_buffer_,
                      base::Bind(&DecryptingAudioDecoderTest::DecodeDone,
                                 base::Unretained(this)));
+
     base::RunLoop().RunUntilIdle();
   }
 

diff --git a/media/filters/decrypting_demuxer_stream.cc b/media/filters/decrypting_demuxer_stream.cc
index 1afb497..f5dfcd4 100644
--- a/media/filters/decrypting_demuxer_stream.cc
+++ b/media/filters/decrypting_demuxer_stream.cc

@@ -174,7 +174,7 @@
 void DecryptingDemuxerStream::DecryptBuffer(
     DemuxerStream::Status status,
     const scoped_refptr<DecoderBuffer>& buffer) {
-  DVLOG(3) << __func__;
+  DVLOG(3) << __func__ << ": status = " << status;
   DCHECK(task_runner_->BelongsToCurrentThread());
   DCHECK_EQ(state_, kPendingDemuxerRead) << state_;
   DCHECK(!read_cb_.is_null());
@@ -204,10 +204,13 @@
     return;
   }
 
-  if (status == kAborted) {
-    DVLOG(2) << "DoDecryptBuffer() - kAborted.";
+  if (status == kAborted || status == kError) {
+    if (status == kError) {
+      MEDIA_LOG(ERROR, media_log_)
+          << GetDisplayName() << ": demuxer stream read error.";
+    }
     state_ = kIdle;
-    base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
+    base::ResetAndReturn(&read_cb_).Run(status, nullptr);
     return;
   }
 
@@ -285,7 +288,7 @@
     MEDIA_LOG(ERROR, media_log_) << GetDisplayName() << ": decrypt error";
     pending_buffer_to_decrypt_ = NULL;
     state_ = kIdle;
-    base::ResetAndReturn(&read_cb_).Run(kAborted, NULL);
+    base::ResetAndReturn(&read_cb_).Run(kError, nullptr);
     return;
   }
 
@@ -330,8 +333,8 @@
   }
 
   if (state_ == kWaitingForKey) {
-    MEDIA_LOG(INFO, media_log_) << GetDisplayName()
-                                << ": key added, resuming decrypt";
+    MEDIA_LOG(INFO, media_log_)
+        << GetDisplayName() << ": key was added, resuming decrypt";
     state_ = kPendingDecrypt;
     DecryptPendingBuffer();
   }

diff --git a/media/filters/decrypting_demuxer_stream_unittest.cc b/media/filters/decrypting_demuxer_stream_unittest.cc
index 8e0e457fc..84e992a 100644
--- a/media/filters/decrypting_demuxer_stream_unittest.cc
+++ b/media/filters/decrypting_demuxer_stream_unittest.cc

@@ -17,12 +17,15 @@
 #include "media/base/gmock_callback_support.h"
 #include "media/base/media_util.h"
 #include "media/base/mock_filters.h"
+#include "media/base/mock_media_log.h"
 #include "media/base/test_helpers.h"
 #include "media/filters/decrypting_demuxer_stream.h"
 #include "testing/gmock/include/gmock/gmock.h"
 
 using ::testing::_;
+using ::testing::HasSubstr;
 using ::testing::IsNull;
+using ::testing::InSequence;
 using ::testing::Return;
 using ::testing::SaveArg;
 using ::testing::StrictMock;
@@ -217,11 +220,13 @@
   }
 
   void EnterWaitingForKeyState() {
+    InSequence s;
     EXPECT_CALL(*input_audio_stream_, Read(_))
         .WillRepeatedly(ReturnBuffer(encrypted_buffer_));
     EXPECT_CALL(*decryptor_, Decrypt(_, encrypted_buffer_, _))
         .WillRepeatedly(RunCallback<2>(Decryptor::kNoKey,
                                        scoped_refptr<DecoderBuffer>()));
+    EXPECT_MEDIA_LOG(HasSubstr("DecryptingDemuxerStream: no key for key ID"));
     EXPECT_CALL(*this, OnWaitingForDecryptionKey());
     demuxer_stream_->Read(base::Bind(&DecryptingDemuxerStreamTest::BufferReady,
                                      base::Unretained(this)));
@@ -254,7 +259,7 @@
   MOCK_METHOD0(OnWaitingForDecryptionKey, void(void));
 
   base::MessageLoop message_loop_;
-  MediaLog media_log_;
+  StrictMock<MockMediaLog> media_log_;
   std::unique_ptr<DecryptingDemuxerStream> demuxer_stream_;
   std::unique_ptr<StrictMock<MockCdmContext>> cdm_context_;
   std::unique_ptr<StrictMock<MockDecryptor>> decryptor_;
@@ -308,6 +313,7 @@
   AudioDecoderConfig input_config(kCodecVorbis, kSampleFormatPlanarF32,
                                   CHANNEL_LAYOUT_STEREO, 44100,
                                   EmptyExtraData(), AesCtrEncryptionScheme());
+  EXPECT_MEDIA_LOG(HasSubstr("DecryptingDemuxerStream: no decryptor"));
   InitializeAudioAndExpectStatus(input_config, DECODER_ERROR_NOT_SUPPORTED);
 }
 
@@ -340,7 +346,8 @@
   EXPECT_CALL(*decryptor_, Decrypt(_, encrypted_buffer_, _))
       .WillRepeatedly(RunCallback<2>(Decryptor::kError,
                                      scoped_refptr<DecoderBuffer>()));
-  ReadAndExpectBufferReadyWith(DemuxerStream::kAborted, NULL);
+  EXPECT_MEDIA_LOG(HasSubstr("DecryptingDemuxerStream: decrypt error"));
+  ReadAndExpectBufferReadyWith(DemuxerStream::kError, nullptr);
 }
 
 // Test the case where the input is an end-of-stream buffer.
@@ -362,6 +369,8 @@
   Initialize();
   EnterWaitingForKeyState();
 
+  EXPECT_MEDIA_LOG(
+      HasSubstr("DecryptingDemuxerStream: key was added, resuming decrypt"));
   EXPECT_CALL(*decryptor_, Decrypt(_, encrypted_buffer_, _))
       .WillRepeatedly(RunCallback<2>(Decryptor::kSuccess, decrypted_buffer_));
   EXPECT_CALL(*this, BufferReady(DemuxerStream::kOk, decrypted_buffer_));
@@ -375,6 +384,9 @@
   Initialize();
   EnterPendingDecryptState();
 
+  EXPECT_MEDIA_LOG(HasSubstr("DecryptingDemuxerStream: no key for key ID"));
+  EXPECT_MEDIA_LOG(
+      HasSubstr("DecryptingDemuxerStream: key was added, resuming decrypt"));
   EXPECT_CALL(*decryptor_, Decrypt(_, encrypted_buffer_, _))
       .WillRepeatedly(RunCallback<2>(Decryptor::kSuccess, decrypted_buffer_));
   EXPECT_CALL(*this, BufferReady(DemuxerStream::kOk, decrypted_buffer_));

diff --git a/media/filters/video_frame_stream_unittest.cc b/media/filters/video_frame_stream_unittest.cc
index 8e68439..b53d42d 100644
--- a/media/filters/video_frame_stream_unittest.cc
+++ b/media/filters/video_frame_stream_unittest.cc

@@ -15,6 +15,7 @@
 #include "media/base/fake_demuxer_stream.h"
 #include "media/base/gmock_callback_support.h"
 #include "media/base/mock_filters.h"
+#include "media/base/mock_media_log.h"
 #include "media/base/test_helpers.h"
 #include "media/base/timestamp_constants.h"
 #include "media/filters/decoder_stream.h"
@@ -24,6 +25,8 @@
 using ::testing::_;
 using ::testing::AnyNumber;
 using ::testing::Assign;
+using ::testing::HasSubstr;
+using ::testing::InSequence;
 using ::testing::Invoke;
 using ::testing::InvokeWithoutArgs;
 using ::testing::NiceMock;
@@ -97,6 +100,11 @@
       EXPECT_CALL(*cdm_context_, GetDecryptor())
           .WillRepeatedly(Return(decryptor_.get()));
     }
+
+    // Covering most MediaLog messages for now.
+    // TODO(wolenetz/xhwang): Fix tests to have better MediaLog checking.
+    EXPECT_MEDIA_LOG(HasSubstr("video")).Times(AnyNumber());
+    EXPECT_MEDIA_LOG(HasSubstr("decryptor")).Times(AnyNumber());
   }
 
   ~VideoFrameStreamTest() {
@@ -322,6 +330,7 @@
 
       case DECRYPTOR_NO_KEY:
         if (GetParam().is_encrypted && GetParam().has_decryptor) {
+          EXPECT_MEDIA_LOG(HasSubstr("no key for key ID"));
           EXPECT_CALL(*this, OnWaitingForDecryptionKey());
           has_no_key_ = true;
         }
@@ -403,7 +412,7 @@
 
   base::MessageLoop message_loop_;
 
-  MediaLog media_log_;
+  StrictMock<MockMediaLog> media_log_;
   std::unique_ptr<VideoFrameStream> video_frame_stream_;
   std::unique_ptr<FakeDemuxerStream> demuxer_stream_;
   std::unique_ptr<StrictMock<MockCdmContext>> cdm_context_;
@@ -617,6 +626,25 @@
       frame_read_->metadata()->IsTrue(VideoFrameMetadata::END_OF_STREAM));
 }
 
+TEST_P(VideoFrameStreamTest, Read_DemuxerStreamReadError) {
+  Initialize();
+  EnterPendingState(DEMUXER_READ_NORMAL);
+
+  InSequence s;
+
+  if (GetParam().is_encrypted && GetParam().has_decryptor) {
+    EXPECT_MEDIA_LOG(
+        HasSubstr("DecryptingDemuxerStream: demuxer stream read error"));
+  }
+  EXPECT_MEDIA_LOG(HasSubstr("video demuxer stream read error"));
+
+  demuxer_stream_->Error();
+  base::RunLoop().RunUntilIdle();
+
+  ASSERT_FALSE(pending_read_);
+  EXPECT_EQ(last_read_status_, VideoFrameStream::DECODE_ERROR);
+}
+
 // No Reset() before initialization is successfully completed.
 TEST_P(VideoFrameStreamTest, Reset_AfterInitialization) {
   Initialize();

diff --git a/media/remoting/demuxer_stream_adapter.cc b/media/remoting/demuxer_stream_adapter.cc
index bb5f525..f6d6e49 100644
--- a/media/remoting/demuxer_stream_adapter.cc
+++ b/media/remoting/demuxer_stream_adapter.cc

@@ -271,6 +271,10 @@
       DCHECK(!input);
       SendReadAck();
       return;
+    case DemuxerStream::kError:
+      // Currently kError can only happen because of DECRYPTION_ERROR.
+      OnFatalError(DECRYPTION_ERROR);
+      return;
     case DemuxerStream::kConfigChanged:
       // TODO(erickung): Notify controller of new decoder config, just in case
       // that will require remoting to be shutdown (due to known

diff --git a/media/remoting/proto_enum_utils.cc b/media/remoting/proto_enum_utils.cc
index 45e6ff7..95b3749b 100644
--- a/media/remoting/proto_enum_utils.cc
+++ b/media/remoting/proto_enum_utils.cc

@@ -577,6 +577,7 @@
     CASE_RETURN_OTHER(kOk);
     CASE_RETURN_OTHER(kAborted);
     CASE_RETURN_OTHER(kConfigChanged);
+    CASE_RETURN_OTHER(kError);
   }
   return base::nullopt;  // Not a 'default' to ensure compile-time checks.
 }
@@ -589,6 +590,7 @@
     CASE_RETURN_OTHER(kOk);
     CASE_RETURN_OTHER(kAborted);
     CASE_RETURN_OTHER(kConfigChanged);
+    CASE_RETURN_OTHER(kError);
   }
   return base::nullopt;  // Not a 'default' to ensure compile-time checks.
 }

diff --git a/media/remoting/rpc.proto b/media/remoting/rpc.proto
index 6eb91af..baceb6a 100644
--- a/media/remoting/rpc.proto
+++ b/media/remoting/rpc.proto

@@ -363,6 +363,7 @@
     kOk = 0;
     kAborted = 1;
     kConfigChanged = 2;
+    kError = 3;
   };
 
   optional Status status = 1;

diff --git a/media/remoting/stream_provider.cc b/media/remoting/stream_provider.cc
index 3bd99591..35b2ba22 100644
--- a/media/remoting/stream_provider.cc
+++ b/media/remoting/stream_provider.cc

@@ -336,6 +336,7 @@
       base::ResetAndReturn(&read_complete_callback_).Run(status, nullptr);
       return;
     case DemuxerStream::kAborted:
+    case DemuxerStream::kError:
       base::ResetAndReturn(&read_complete_callback_).Run(status, nullptr);
       return;
     case DemuxerStream::kOk: