Fix MediaPlayerRenderer error propagation

The detection of HLS by the demuxer is propagated through the pipeline
via a special error code. This error code is saved by RendererWrapper,
and never reset. RW does not propagate errors when it is already has an
error status. This means that HLS errors are never fully reported to the
pipeline.

This CL fixes the issue by reseting RendererWrapper's status on stop.
This makes the comments above |status_| accurate again.

The CL also fixes a bug in MediaPlayerRenderer, where
MEDIA_ERROR_FORMAT errors are dropped, and MEDIA_ERROR_INVALID_CODE
errors are propagated.

Test: manual tests, added UT
Bug: 812465
Change-Id: I281d90104c8f9cb879e90c6d31a99167991c06ab
Reviewed-on: https://chromium-review.googlesource.com/920881
Commit-Queue: Thomas Guilbert <tguilbert@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537407}

content/browser/media/android/media_player_renderer.cc

@@ -273,10 +273,12 @@ void MediaPlayerRenderer::OnSeekComplete(int player_id,
 
 void MediaPlayerRenderer::OnError(int player_id, int error) {
   // Some errors are forwarded to the MediaPlayerListener, but are of no
-  // importance to us. Ignore these errors, which are reported as error 0 by
-  // MediaPlayerListener.
-  if (!error)
+  // importance to us. Ignore these errors, which are reported as
+  // MEDIA_ERROR_INVALID_CODE by MediaPlayerListener.
+  if (error ==
+      media::MediaPlayerAndroid::MediaErrorType::MEDIA_ERROR_INVALID_CODE) {
     return;
+  }
 
   LOG(ERROR) << __func__ << " Error: " << error;
   has_error_ = true;

media/base/pipeline_impl.cc

@@ -297,6 +297,10 @@ void PipelineImpl::RendererWrapper::Stop(const base::Closure& stop_cb) {
 
   SetState(kStopped);
 
+  // Reset the status. Otherwise, if we encountered an error, new erros will
+  // never be propagated. See https://crbug.com/812465.
+  status_ = PIPELINE_OK;
+
   // Post the stop callback to enqueue it after the tasks that may have been
   // posted by Demuxer and Renderer during stopping. Note that in theory the
   // tasks posted by Demuxer/Renderer may post even more tasks that will get

media/base/pipeline_impl_unittest.cc

@@ -943,6 +943,43 @@ TEST_F(PipelineImplTest, GetMediaTimeAfterSeek) {
   EXPECT_EQ(kMediaTime, pipeline_->GetMediaTime());
 }
 
+// This test makes sure that, after receiving an error, stopping and starting
+// the pipeline clears all internal error state, and allows errors to be
+// propagated again.
+TEST_F(PipelineImplTest, RendererErrorsReset) {
+  // Basic setup
+  CreateAudioStream();
+  MockDemuxerStreamVector streams;
+  streams.push_back(audio_stream());
+  SetDemuxerExpectations(&streams);
+  SetRendererExpectations();
+  StartPipelineAndExpect(PIPELINE_OK);
+
+  // Trigger two errors. The second error will be ignored.
+  EXPECT_CALL(callbacks_, OnError(PIPELINE_ERROR_READ)).Times(1);
+  renderer_client_->OnError(PIPELINE_ERROR_READ);
+  renderer_client_->OnError(PIPELINE_ERROR_READ);
+
+  base::RunLoop().RunUntilIdle();
+
+  // Stopping the demuxer should clear internal state.
+  EXPECT_CALL(*demuxer_, Stop());
+  pipeline_->Stop();
+
+  base::RunLoop().RunUntilIdle();
+
+  CreateRenderer();
+  SetDemuxerExpectations(&streams);
+  SetRendererExpectations();
+  StartPipelineAndExpect(PIPELINE_OK);
+
+  // New errors should propagate and not be ignored.
+  EXPECT_CALL(callbacks_, OnError(PIPELINE_ERROR_READ)).Times(1);
+  renderer_client_->OnError(PIPELINE_ERROR_READ);
+
+  base::RunLoop().RunUntilIdle();
+}
+
 class PipelineTeardownTest : public PipelineImplTest {
  public:
   enum TeardownState {