Reset timestamp validator when config change is complete

When DecoderStream<DemuxerStream::AUDIO> encounters a config change it needs to
do two things, among others: flush the decoder and reset the audio timestamp
validator. It's important that it does the latter only after processing any
decoder output produced during the flushing.

This CL moves the responsibility of resetting the validator from
DecoderStream to DecoderStreamTraits<DemuxerStream::AUDIO>. It also adds
a test where the decoder's behavior of releasing internally buffered
samples upon flushing is mocked.

Bug: 865926
Test: New test AudioBufferStreamTest.FlushOnConfigChange doesn't fail assertions. No regressions in media_unittests.

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I8cbb01df3792ca9de5db8ab637285d1e64188b39
Reviewed-on: https://chromium-review.googlesource.com/1145188Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Commit-Queue: Wojciech Dzierżanowski <wdzierzanowski@opera.com>
Cr-Commit-Position: refs/heads/master@{#577123}

media/filters/BUILD.gn

@@ -249,6 +249,7 @@ static_library("test_support") {
 source_set("unit_tests") {
   testonly = true
   sources = [
+    "audio_buffer_stream_unittest.cc",
     "audio_clock_unittest.cc",
     "audio_decoder_selector_unittest.cc",
     "audio_renderer_algorithm_unittest.cc",

media/filters/audio_buffer_stream_unittest.cc

+// Copyright 2018 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <memory>
+#include <utility>
+
+#include "base/bind.h"
+#include "base/bind_helpers.h"
+#include "base/run_loop.h"
+#include "base/test/scoped_task_environment.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "media/base/gmock_callback_support.h"
+#include "media/base/media_log.h"
+#include "media/base/mock_filters.h"
+#include "media/filters/decoder_stream.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "testing/gmock/include/gmock/gmock.h"
+
+using testing::_;
+using testing::DoAll;
+using testing::Invoke;
+using testing::Return;
+using testing::SaveArg;
+
+namespace media {
+
+MATCHER(IsRegularDecoderBuffer, "") {
+  return !arg->end_of_stream();
+}
+
+MATCHER(IsEOSDecoderBuffer, "") {
+  return arg->end_of_stream();
+}
+
+static void OnAudioBufferStreamInitialized(base::OnceClosure closure,
+                                           bool success) {
+  ASSERT_TRUE(success);
+  std::move(closure).Run();
+}
+
+class AudioBufferStreamTest : public testing::Test {
+ public:
+  AudioBufferStreamTest()
+      : audio_buffer_stream_(
+            std::make_unique<AudioBufferStream::StreamTraits>(
+                &media_log_,
+                CHANNEL_LAYOUT_STEREO),
+            task_environment_.GetMainThreadTaskRunner(),
+            base::BindRepeating(&AudioBufferStreamTest::CreateMockAudioDecoder,
+                                base::Unretained(this)),
+            &media_log_) {
+    // Any valid config will do.
+    demuxer_stream_.set_audio_decoder_config(
+        {kCodecAAC, kSampleFormatS16, CHANNEL_LAYOUT_STEREO, 44100, {}, {}});
+    EXPECT_CALL(demuxer_stream_, SupportsConfigChanges())
+        .WillRepeatedly(Return(true));
+
+    base::RunLoop run_loop;
+    audio_buffer_stream_.Initialize(
+        &demuxer_stream_,
+        base::BindOnce(&OnAudioBufferStreamInitialized, run_loop.QuitClosure()),
+        nullptr, base::DoNothing(), base::DoNothing());
+    run_loop.Run();
+  }
+
+  MockDemuxerStream* demuxer_stream() { return &demuxer_stream_; }
+  MockAudioDecoder* decoder() { return decoder_; }
+
+  void ReadAudioBuffer(base::OnceClosure closure) {
+    audio_buffer_stream_.Read(
+        base::BindOnce(&AudioBufferStreamTest::OnAudioBufferReadDone,
+                       base::Unretained(this), std::move(closure)));
+  }
+
+  void ProduceDecoderOutput(scoped_refptr<DecoderBuffer> buffer,
+                            const AudioDecoder::DecodeCB& decode_cb) {
+    // Make sure successive AudioBuffers have increasing timestamps.
+    last_timestamp_ += base::TimeDelta::FromMilliseconds(27);
+    const auto& config = demuxer_stream_.audio_decoder_config();
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            decoder_output_cb_,
+            AudioBuffer::CreateEmptyBuffer(
+                config.channel_layout(), config.channels(),
+                config.samples_per_second(), 1221, last_timestamp_)));
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::BindOnce(decode_cb, DecodeStatus::OK));
+  }
+
+  void RunUntilIdle() { task_environment_.RunUntilIdle(); }
+
+ private:
+  std::vector<std::unique_ptr<AudioDecoder>> CreateMockAudioDecoder() {
+    auto decoder = std::make_unique<MockAudioDecoder>();
+    EXPECT_CALL(*decoder, Initialize(_, _, _, _, _))
+        .WillOnce(DoAll(SaveArg<3>(&decoder_output_cb_), RunCallback<2>(true)));
+    decoder_ = decoder.get();
+
+    std::vector<std::unique_ptr<AudioDecoder>> result;
+    result.push_back(std::move(decoder));
+    return result;
+  }
+
+  void OnAudioBufferReadDone(base::OnceClosure closure,
+                             AudioBufferStream::Status status,
+                             const scoped_refptr<AudioBuffer>& audio_buffer) {
+    std::move(closure).Run();
+  }
+
+  base::test::ScopedTaskEnvironment task_environment_;
+  MediaLog media_log_;
+  testing::NiceMock<MockDemuxerStream> demuxer_stream_{DemuxerStream::AUDIO};
+  AudioBufferStream audio_buffer_stream_;
+
+  MockAudioDecoder* decoder_ = nullptr;
+  AudioDecoder::OutputCB decoder_output_cb_;
+  base::TimeDelta last_timestamp_;
+
+  DISALLOW_COPY_AND_ASSIGN(AudioBufferStreamTest);
+};
+
+TEST_F(AudioBufferStreamTest, FlushOnConfigChange) {
+  MockAudioDecoder* first_decoder = decoder();
+  ASSERT_NE(first_decoder, nullptr);
+
+  // Make a regular DemuxerStream::Read().
+  EXPECT_CALL(*demuxer_stream(), Read(_))
+      .WillOnce(RunCallback<0>(DemuxerStream::kOk, new DecoderBuffer(12)));
+  EXPECT_CALL(*decoder(), Decode(IsRegularDecoderBuffer(), _))
+      .WillOnce(Invoke(this, &AudioBufferStreamTest::ProduceDecoderOutput));
+  base::RunLoop run_loop0;
+  ReadAudioBuffer(run_loop0.QuitClosure());
+  run_loop0.Run();
+
+  // Make a config-change DemuxerStream::Read().
+  // Expect the decoder to be flushed.  Upon flushing, the decoder releases
+  // internally buffered output.
+  EXPECT_CALL(*demuxer_stream(), Read(_))
+      .WillOnce(RunCallback<0>(DemuxerStream::kConfigChanged, nullptr));
+  EXPECT_CALL(*decoder(), Decode(IsEOSDecoderBuffer(), _))
+      .WillOnce(Invoke(this, &AudioBufferStreamTest::ProduceDecoderOutput));
+  base::RunLoop run_loop1;
+  ReadAudioBuffer(run_loop1.QuitClosure());
+  run_loop1.Run();
+
+  // Expect the decoder to be re-initialized when AudioBufferStream finishes
+  // processing the last decode.
+  EXPECT_CALL(*decoder(), Initialize(_, _, _, _, _));
+  RunUntilIdle();
+}
+
+}  // namespace media

media/filters/decoder_stream.cc

@@ -688,7 +688,6 @@ void DecoderStream<StreamType>::OnBufferReady(
     pending_buffers_.clear();
 
     const DecoderConfig& config = traits_->GetDecoderConfig(stream_);
-    traits_->OnConfigChanged(config);
 
     MEDIA_LOG(INFO, media_log_)
         << GetStreamTypeString()

media/filters/decoder_stream_traits.cc

@@ -66,6 +66,11 @@ void DecoderStreamTraits<DemuxerStream::AUDIO>::InitializeDecoder(
     const DecoderType::WaitingForDecryptionKeyCB&
         waiting_for_decryption_key_cb) {
   DCHECK(config.IsValidConfig());
+
+  if (config_.IsValidConfig() && !config_.Matches(config))
+    OnConfigChanged(config);
+  config_ = config;
+
   stats_.audio_decoder_name = decoder->GetDisplayName();
   decoder->Initialize(config, cdm_context, init_cb, output_cb,
                       waiting_for_decryption_key_cb);

media/filters/decoder_stream_traits.h

@@ -8,19 +8,18 @@
 #include "base/containers/flat_set.h"
 #include "base/time/time.h"
 #include "media/base/audio_decoder.h"
+#include "media/base/audio_decoder_config.h"
 #include "media/base/cdm_context.h"
 #include "media/base/channel_layout.h"
 #include "media/base/demuxer_stream.h"
 #include "media/base/moving_average.h"
 #include "media/base/pipeline_status.h"
 #include "media/base/video_decoder.h"
-#include "media/base/video_decoder_config.h"
 #include "media/filters/audio_timestamp_validator.h"
 
 namespace media {
 
 class AudioBuffer;
-class AudioDecoderConfig;
 class CdmContext;
 class DemuxerStream;
 class VideoDecoderConfig;
@@ -60,9 +59,10 @@ class MEDIA_EXPORT DecoderStreamTraits<DemuxerStream::AUDIO> {
   void OnDecode(const DecoderBuffer& buffer);
   PostDecodeAction OnDecodeDone(const scoped_refptr<OutputType>& buffer);
   void OnStreamReset(DemuxerStream* stream);
-  void OnConfigChanged(const DecoderConfigType& config);
 
  private:
+  void OnConfigChanged(const AudioDecoderConfig& config);
+
   // Validates encoded timestamps match decoded output duration. MEDIA_LOG warns
   // if timestamp gaps are detected. Sufficiently large gaps can lead to AV sync
   // drift.
@@ -72,6 +72,7 @@ class MEDIA_EXPORT DecoderStreamTraits<DemuxerStream::AUDIO> {
   // device changes.
   ChannelLayout initial_hw_layout_;
   PipelineStatistics stats_;
+  AudioDecoderConfig config_;
 };
 
 template <>
@@ -103,7 +104,6 @@ class MEDIA_EXPORT DecoderStreamTraits<DemuxerStream::VIDEO> {
   void OnDecode(const DecoderBuffer& buffer);
   PostDecodeAction OnDecodeDone(const scoped_refptr<OutputType>& buffer);
   void OnStreamReset(DemuxerStream* stream);
-  void OnConfigChanged(const DecoderConfigType& config) {}
 
  private:
   base::TimeDelta last_keyframe_timestamp_;