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/1145188
Reviewed-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}diff --git a/media/filters/BUILD.gn b/media/filters/BUILD.gn
index 30eb8ec..72ac8bd 100644
--- a/media/filters/BUILD.gn
+++ b/media/filters/BUILD.gn
@@ -249,6 +249,7 @@
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",
diff --git a/media/filters/audio_buffer_stream_unittest.cc b/media/filters/audio_buffer_stream_unittest.cc
new file mode 100644
index 0000000..f674a49
--- /dev/null
+++ b/media/filters/audio_buffer_stream_unittest.cc
@@ -0,0 +1,154 @@
+// 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
diff --git a/media/filters/decoder_stream.cc b/media/filters/decoder_stream.cc
index 09d6173..be0c71f 100644
--- a/media/filters/decoder_stream.cc
+++ b/media/filters/decoder_stream.cc
@@ -688,7 +688,6 @@
pending_buffers_.clear();
const DecoderConfig& config = traits_->GetDecoderConfig(stream_);
- traits_->OnConfigChanged(config);
MEDIA_LOG(INFO, media_log_)
<< GetStreamTypeString()
diff --git a/media/filters/decoder_stream_traits.cc b/media/filters/decoder_stream_traits.cc
index 707919d..df6f5f2 100644
--- a/media/filters/decoder_stream_traits.cc
+++ b/media/filters/decoder_stream_traits.cc
@@ -66,6 +66,11 @@
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);
diff --git a/media/filters/decoder_stream_traits.h b/media/filters/decoder_stream_traits.h
index ec3a2d4..62c1732 100644
--- a/media/filters/decoder_stream_traits.h
+++ b/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 @@
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 @@
// device changes.
ChannelLayout initial_hw_layout_;
PipelineStatistics stats_;
+ AudioDecoderConfig config_;
};
template <>
@@ -103,7 +104,6 @@
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_;
