media: Fix PipelineDecoderInfo reporting in PipelineImpl

Currently we compare the whole PipelineDecoderInfo to decide where
there's a change to report. But we only store the |decoder_name| for the
next comparison, so info like |is_decrypting_demuxer_stream| is not
updated. Then we'll keep reporting PipelineDecoderInfo even if there's
no new change.

This CL fixes this issue by storeing the whole PipelineDecoderInfo
instead of just the |decoder_name|. Additional logging is also added for
the convenience in debugging.

Bug: 1018372
Test: Adds new unit test to cover this
Change-Id: I6d309075b694f6a7cde6a4662c2200e2606652f3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1917600Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Ted Meyer <tmathmeyer@chromium.org>
Commit-Queue: Xiaohan Wang <xhwang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#715571}

media/base/pipeline_impl.cc

@@ -699,8 +699,7 @@ void PipelineImpl::RendererWrapper::OnStatisticsUpdate(
 
   if (!stats.audio_decoder_info.decoder_name.empty() &&
       stats.audio_decoder_info != shared_state_.statistics.audio_decoder_info) {
-    shared_state_.statistics.audio_decoder_info.decoder_name =
-        stats.audio_decoder_info.decoder_name;
+    shared_state_.statistics.audio_decoder_info = stats.audio_decoder_info;
     main_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(&PipelineImpl::OnAudioDecoderChange,
                                   weak_pipeline_, stats.audio_decoder_info));
@@ -708,8 +707,7 @@ void PipelineImpl::RendererWrapper::OnStatisticsUpdate(
 
   if (!stats.video_decoder_info.decoder_name.empty() &&
       stats.video_decoder_info != shared_state_.statistics.video_decoder_info) {
-    shared_state_.statistics.video_decoder_info.decoder_name =
-        stats.video_decoder_info.decoder_name;
+    shared_state_.statistics.video_decoder_info = stats.video_decoder_info;
     main_task_runner_->PostTask(
         FROM_HERE, base::BindOnce(&PipelineImpl::OnVideoDecoderChange,
                                   weak_pipeline_, stats.video_decoder_info));
@@ -1476,7 +1474,7 @@ void PipelineImpl::OnVideoAverageKeyframeDistanceUpdate() {
 }
 
 void PipelineImpl::OnAudioDecoderChange(const PipelineDecoderInfo& info) {
-  DVLOG(2) << __func__;
+  DVLOG(2) << __func__ << ": info=" << info;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());
 
@@ -1485,7 +1483,7 @@ void PipelineImpl::OnAudioDecoderChange(const PipelineDecoderInfo& info) {
 }
 
 void PipelineImpl::OnVideoDecoderChange(const PipelineDecoderInfo& info) {
-  DVLOG(2) << __func__;
+  DVLOG(2) << __func__ << ": info=" << info;
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(IsRunning());
 

media/base/pipeline_impl_unittest.cc

@@ -669,6 +669,45 @@ TEST_F(PipelineImplTest, BufferedTimeRangesCanChangeAfterStop) {
   RunBufferedTimeRangesTest(base::TimeDelta::FromSeconds(5));
 }
 
+TEST_F(PipelineImplTest, OnStatisticsUpdate) {
+  CreateAudioAndVideoStream();
+  SetDemuxerExpectations();
+  StartPipelineAndExpect(PIPELINE_OK);
+
+  PipelineStatistics stats;
+  stats.audio_decoder_info.decoder_name = "TestAudioDecoderName";
+  stats.audio_decoder_info.is_platform_decoder = false;
+  EXPECT_CALL(callbacks_, OnAudioDecoderChange(_));
+  renderer_client_->OnStatisticsUpdate(stats);
+  base::RunLoop().RunUntilIdle();
+
+  // VideoDecoderInfo changed and we expect OnVideoDecoderChange() to be called.
+  stats.video_decoder_info.decoder_name = "TestVideoDecoderName";
+  stats.video_decoder_info.is_platform_decoder = true;
+  EXPECT_CALL(callbacks_, OnVideoDecoderChange(_));
+  renderer_client_->OnStatisticsUpdate(stats);
+  base::RunLoop().RunUntilIdle();
+
+  // OnStatisticsUpdate() with the same |stats| should not cause new
+  // PipelineClient calls.
+  renderer_client_->OnStatisticsUpdate(stats);
+  base::RunLoop().RunUntilIdle();
+
+  // AudioDecoderInfo changed and we expect OnAudioDecoderChange() to be called.
+  stats.audio_decoder_info.is_platform_decoder = true;
+  EXPECT_CALL(callbacks_, OnAudioDecoderChange(_));
+  renderer_client_->OnStatisticsUpdate(stats);
+  base::RunLoop().RunUntilIdle();
+
+  // Both info changed.
+  stats.audio_decoder_info.decoder_name = "NewTestAudioDecoderName";
+  stats.video_decoder_info.is_decrypting_demuxer_stream = true;
+  EXPECT_CALL(callbacks_, OnAudioDecoderChange(_));
+  EXPECT_CALL(callbacks_, OnVideoDecoderChange(_));
+  renderer_client_->OnStatisticsUpdate(stats);
+  base::RunLoop().RunUntilIdle();
+}
+
 TEST_F(PipelineImplTest, EndedCallback) {
   CreateAudioAndVideoStream();
   SetDemuxerExpectations();

media/base/pipeline_status.cc

@@ -85,4 +85,11 @@ bool operator!=(const PipelineDecoderInfo& first,
   return !(first == second);
 }
 
+std::ostream& operator<<(std::ostream& out, const PipelineDecoderInfo& info) {
+  return out << "{decoder_name:" << info.decoder_name << ","
+             << "is_platform_decoder:" << info.is_platform_decoder << ","
+             << "is_decrypting_demuxer_stream:"
+             << info.is_decrypting_demuxer_stream << "}";
+}
+
 }  // namespace media

media/base/pipeline_status.h

@@ -81,6 +81,8 @@ MEDIA_EXPORT bool operator==(const PipelineDecoderInfo& first,
                              const PipelineDecoderInfo& second);
 MEDIA_EXPORT bool operator!=(const PipelineDecoderInfo& first,
                              const PipelineDecoderInfo& second);
+MEDIA_EXPORT std::ostream& operator<<(std::ostream& out,
+                                      const PipelineDecoderInfo& info);
 
 struct MEDIA_EXPORT PipelineStatistics {
   PipelineStatistics();