Improve input handling for WaveAudioHandler.

media/audio/simple_sources.cc

Index: media/audio/simple_sources.cc
diff --git a/media/audio/simple_sources.cc b/media/audio/simple_sources.cc
index 877f9fd5bdac20866823b14574e1c46e03c7e23e..347b21305fe0fa0a3bdf7bc78fdacc6a3a030724 100644
--- a/media/audio/simple_sources.cc
+++ b/media/audio/simple_sources.cc
@@ -49,17 +49,6 @@ static scoped_ptr<uint8[]> ReadWavFile(const base::FilePath& wav_filename,
   return scoped_ptr<uint8[]>(wav_file_data);
 }
 
-// Opens |wav_filename|, reads it and loads it as a wav file. This function will
-// bluntly trigger CHECKs if we can't read the file or if it's malformed.
-static scoped_ptr<WavAudioHandler> CreateWavAudioHandler(
-    const base::FilePath& wav_filename, const uint8* wav_file_data,
-    size_t wav_file_length, const AudioParameters& expected_params) {
-  base::StringPiece wav_data(reinterpret_cast<const char*>(wav_file_data),
-                             wav_file_length);
-  scoped_ptr<WavAudioHandler> wav_audio_handler(new WavAudioHandler(wav_data));
-  return wav_audio_handler.Pass();
-}
-
 // These values are based on experiments for local-to-local
 // PeerConnection to demonstrate audio/video synchronization.
 static const int kBeepDurationMilliseconds = 20;
@@ -179,8 +168,14 @@ void FileSource::LoadWavFile(const base::FilePath& path_to_wav_file) {
     return;
   }
 
-  wav_audio_handler_ = CreateWavAudioHandler(
-      path_to_wav_file, wav_file_data_.get(), file_length, params_);
+  // Attempt to create a handler with this data. If the data is invalid, return.
+  wav_audio_handler_ = WavAudioHandler::Create(base::StringPiece(
+      reinterpret_cast<const char*>(wav_file_data_.get()), file_length));

[tommi (sloooow) - chröme, 2015/11/19 09:44:06]

hmm... I guess shared_ptr could have helped here. As is, it's not exactly
obvious why the wav_file_data_ member variable exists in the first place (it's
only used in this scope and just used to pass the data to this constructor).
Alternatively it might make sense at some point to pass ownership of the data to
the WavAudioHandler (and then possibly change the type to scoped_ptr<uint8[]>). 
I don't know if any caller needs to also hold on to this data.

No action needed in this CL but if you agree with me, you could add a TODO to
WavAudioHandler for either myself or you to do that.

[slan, 2015/11/19 18:57:16]

Seems reasonable to remove it as a member. Done. Refactored the reading function
and added some test coverage.

[slan, 2015/11/19 19:05:26]

Whoops, just read your comment more thoroughly. I don't see the copy in the ctor
as a huge deal, but I am willing to pass ownership if you'd like. StringPiece
makes it awfully nice to parse the headers out, though, so I might prefer we
keep that data type.

+  if (!wav_audio_handler_) {
+    LOG(ERROR) << "WAV data could be read but is not valid";
+    load_failed_ = true;
+    return;
+  }
 
   // Hook us up so we pull in data from the file into the converter. We need to
   // modify the wav file's audio parameters since we'll be reading small slices