Guard against midstream audio configuration changes.

In WebAudio this can lead to oob reads.  In HTML5 this will cause
garbled audio or oob reads depending on the sample format.  The
guards in place just ensure channel count nor sample format change
between demux and decode.

Video already has similar guards in place via the checks done in
VideoFrame::IsValidConfig during buffer allocation.

BUG=166565
TEST=unit tests pass under ASAN.


Review URL: https://chromiumcodereview.appspot.com/12224114

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@182027 0039d316-1c4b-4281-b951-d872f2087c98

7 files changed, 78 insertions, 19 deletions

diff --git a/media/filters/audio_file_reader.cc b/media/filters/audio_file_reader.cc
index 58c4501..32627b0 100644
--- a/media/filters/audio_file_reader.cc
+++ b/media/filters/audio_file_reader.cc
@@ -15,21 +15,16 @@ namespace media {
 AudioFileReader::AudioFileReader(FFmpegURLProtocol* protocol)
     : codec_context_(NULL),
       stream_index_(0),
-      protocol_(protocol) {
+      protocol_(protocol),
+      channels_(0),
+      sample_rate_(0),
+      av_sample_format_(0) {
 }
 
 AudioFileReader::~AudioFileReader() {
   Close();
 }
 
-int AudioFileReader::channels() const {
-  return codec_context_->channels;
-}
-
-int AudioFileReader::sample_rate() const {
-  return codec_context_->sample_rate;
-}
-
 base::TimeDelta AudioFileReader::duration() const {
   const AVRational av_time_base = {1, AV_TIME_BASE};
 
@@ -110,6 +105,11 @@ bool AudioFileReader::Open() {
     return false;
   }
 
+  // Store initial values to guard against midstream configuration changes.
+  channels_ = codec_context_->channels;
+  sample_rate_ = codec_context_->sample_rate;
+  av_sample_format_ = codec_context_->sample_fmt;
+
   return true;
 }
 
@@ -179,6 +179,22 @@ int AudioFileReader::Read(AudioBus* audio_bus) {
         break;
       }
 
+      if (av_frame->sample_rate != sample_rate_ ||
+          av_frame->channels != channels_ ||
+          av_frame->format != av_sample_format_) {
+        DLOG(ERROR) << "Unsupported midstream configuration change!"
+                    << " Sample Rate: " << av_frame->sample_rate << " vs "
+                    << sample_rate_
+                    << ", Channels: " << av_frame->channels << " vs "
+                    << channels_
+                    << ", Sample Format: " << av_frame->format << " vs "
+                    << av_sample_format_;
+
+        // This is an unrecoverable error, so bail out.
+        continue_decoding = false;
+        break;
+      }
+
       // Truncate, if necessary, if the destination isn't big enough.
       if (current_frame + frames_read > audio_bus->frames())
         frames_read = audio_bus->frames() - current_frame;
diff --git a/media/filters/audio_file_reader.h b/media/filters/audio_file_reader.h
index c9996f39..e345dc0 100644
--- a/media/filters/audio_file_reader.h
+++ b/media/filters/audio_file_reader.h
@@ -43,8 +43,8 @@ class MEDIA_EXPORT AudioFileReader {
   int Read(AudioBus* audio_bus);
 
   // These methods can be called once Open() has been called.
-  int channels() const;
-  int sample_rate() const;
+  int channels() const { return channels_; }
+  int sample_rate() const { return sample_rate_; }
 
   // Please note that duration() and number_of_frames() attempt to be accurate,
   // but are only estimates.  For some encoded formats, the actual duration
@@ -58,6 +58,11 @@ class MEDIA_EXPORT AudioFileReader {
   AVCodecContext* codec_context_;
   int stream_index_;
   FFmpegURLProtocol* protocol_;
+  int channels_;
+  int sample_rate_;
+
+  // AVSampleFormat initially requested; not Chrome's SampleFormat.
+  int av_sample_format_;
 
   DISALLOW_COPY_AND_ASSIGN(AudioFileReader);
 };
diff --git a/media/filters/audio_file_reader_unittest.cc b/media/filters/audio_file_reader_unittest.cc
index 72c2603..64c58e0 100644
--- a/media/filters/audio_file_reader_unittest.cc
+++ b/media/filters/audio_file_reader_unittest.cc
@@ -73,11 +73,19 @@ class AudioFileReaderTest : public testing::Test {
     ReadAndVerify(hash, trimmed_frames);
   }
 
-  void RunFailingTest(const char* fn) {
+  void RunTestFailingDemux(const char* fn) {
     Initialize(fn);
     EXPECT_FALSE(reader_->Open());
   }
 
+  void RunTestFailingDecode(const char* fn) {
+    Initialize(fn);
+    EXPECT_TRUE(reader_->Open());
+    scoped_ptr<AudioBus> decoded_audio_data = AudioBus::Create(
+        reader_->channels(), reader_->number_of_frames());
+    EXPECT_EQ(reader_->Read(decoded_audio_data.get()), 0);
+  }
+
  protected:
   scoped_refptr<DecoderBuffer> data_;
   scoped_ptr<InMemoryUrlProtocol> protocol_;
@@ -91,7 +99,7 @@ TEST_F(AudioFileReaderTest, WithoutOpen) {
 }
 
 TEST_F(AudioFileReaderTest, InvalidFile) {
-  RunFailingTest("ten_byte_file");
+  RunTestFailingDemux("ten_byte_file");
 }
 
 TEST_F(AudioFileReaderTest, WithVideo) {
@@ -134,10 +142,14 @@ TEST_F(AudioFileReaderTest, AAC) {
   RunTest("sfx.m4a", NULL, 1, 44100,
           base::TimeDelta::FromMicroseconds(312001), 13759, 13312);
 }
+
+TEST_F(AudioFileReaderTest, MidStreamConfigChangesFail) {
+  RunTestFailingDecode("midstream_config_change.mp3");
+}
 #endif
 
 TEST_F(AudioFileReaderTest, VorbisInvalidChannelLayout) {
-  RunFailingTest("9ch.ogg");
+  RunTestFailingDemux("9ch.ogg");
 }
 
 TEST_F(AudioFileReaderTest, WaveValidFourChannelLayout) {
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index 1f091c3..8162bbf 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -218,7 +218,7 @@ class ChunkDemuxerStream : public DemuxerStream {
   // Append() belong to a media segment that starts at |start_timestamp|.
   void OnNewMediaSegment(TimeDelta start_timestamp);
 
-  // Called when mid-stream config updates occur.
+  // Called when midstream config updates occur.
   // Returns true if the new config is accepted.
   // Returns false if the new config should trigger an error.
   bool UpdateAudioConfig(const AudioDecoderConfig& config);
diff --git a/media/filters/ffmpeg_audio_decoder.cc b/media/filters/ffmpeg_audio_decoder.cc
index be142d5..6180063 100644
--- a/media/filters/ffmpeg_audio_decoder.cc
+++ b/media/filters/ffmpeg_audio_decoder.cc
@@ -43,7 +43,9 @@ FFmpegAudioDecoder::FFmpegAudioDecoder(
       codec_context_(NULL),
       bits_per_channel_(0),
       channel_layout_(CHANNEL_LAYOUT_NONE),
+      channels_(0),
       samples_per_second_(0),
+      av_sample_format_(0),
       bytes_per_frame_(0),
       last_input_timestamp_(kNoTimestamp()),
       output_bytes_to_drop_(0),
@@ -303,6 +305,11 @@ bool FFmpegAudioDecoder::ConfigureDecoder() {
   output_timestamp_helper_.reset(new AudioTimestampHelper(
       config.bytes_per_frame(), config.samples_per_second()));
   bytes_per_frame_ = config.bytes_per_frame();
+
+  // Store initial values to guard against midstream configuration changes.
+  channels_ = codec_context_->channels;
+  av_sample_format_ = codec_context_->sample_fmt;
+
   return true;
 }
 
@@ -387,10 +394,16 @@ void FFmpegAudioDecoder::RunDecodeLoop(
 
     int decoded_audio_size = 0;
     if (frame_decoded) {
-      int output_sample_rate = av_frame_->sample_rate;
-      if (output_sample_rate != samples_per_second_) {
-        DLOG(ERROR) << "Output sample rate (" << output_sample_rate
-                    << ") doesn't match expected rate " << samples_per_second_;
+      if (av_frame_->sample_rate != samples_per_second_ ||
+          av_frame_->channels != channels_ ||
+          av_frame_->format != av_sample_format_) {
+        DLOG(ERROR) << "Unsupported midstream configuration change!"
+                    << " Sample Rate: " << av_frame_->sample_rate << " vs "
+                    << samples_per_second_
+                    << ", Channels: " << av_frame_->channels << " vs "
+                    << channels_
+                    << ", Sample Format: " << av_frame_->format << " vs "
+                    << av_sample_format_;
 
         // This is an unrecoverable error, so bail out.
         QueuedAudioBuffer queue_entry = { kDecodeError, NULL };
diff --git a/media/filters/ffmpeg_audio_decoder.h b/media/filters/ffmpeg_audio_decoder.h
index d2ba8c5..99fef1b 100644
--- a/media/filters/ffmpeg_audio_decoder.h
+++ b/media/filters/ffmpeg_audio_decoder.h
@@ -65,8 +65,12 @@ class MEDIA_EXPORT FFmpegAudioDecoder : public AudioDecoder {
   // Decoded audio format.
   int bits_per_channel_;
   ChannelLayout channel_layout_;
+  int channels_;
   int samples_per_second_;
 
+  // AVSampleFormat initially requested; not Chrome's SampleFormat.
+  int av_sample_format_;
+
   // Used for computing output timestamps.
   scoped_ptr<AudioTimestampHelper> output_timestamp_helper_;
   int bytes_per_frame_;
diff --git a/media/filters/pipeline_integration_test.cc b/media/filters/pipeline_integration_test.cc
index 3e2eb19..e96da57 100644
--- a/media/filters/pipeline_integration_test.cc
+++ b/media/filters/pipeline_integration_test.cc
@@ -660,6 +660,15 @@ TEST_F(PipelineIntegrationTest,
   EXPECT_EQ(PIPELINE_ERROR_DECODE, WaitUntilEndedOrError());
   source.Abort();
 }
+
+// Verify files which change configuration midstream fail gracefully.
+TEST_F(PipelineIntegrationTest, MidStreamConfigChangesFail) {
+  ASSERT_TRUE(Start(
+      GetTestDataFilePath("midstream_config_change.mp3"), PIPELINE_OK));
+  Play();
+  ASSERT_EQ(WaitUntilEndedOrError(), PIPELINE_ERROR_DECODE);
+}
+
 #endif
 
 TEST_F(PipelineIntegrationTest, BasicPlayback_16x9AspectRatio) {