Improve input handling for WaveAudioHandler.

media/audio/simple_sources.cc

 // Copyright (c) 2012 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.
 // MSVC++ requires this to be set before any other includes to get M_PI.
 #define _USE_MATH_DEFINES
 #include <cmath>
 
 #include "media/audio/simple_sources.h"
 
 #include <algorithm>
@@ skipping 31 matching lines @@
   size_t read_bytes = wav_file.Read(0, reinterpret_cast<char*>(wav_file_data),
                                     wav_file_length);
   if (read_bytes != wav_file_length) {
     LOG(ERROR) << "Failed to read all bytes of " << wav_filename.value();
     return nullptr;
   }
   *file_length = wav_file_length;
   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;
 static const int kBeepFrequency = 400;
 
 // Intervals between two automatic beeps.
 static const int kAutomaticBeepIntervalInMs = 500;
 
 // Automatic beep will be triggered every |kAutomaticBeepIntervalInMs| unless
 // users explicitly call BeepOnce(), which will disable the automatic beep.
@@ skipping 99 matching lines @@
     return;
 
   // Read the file, and put its data in a scoped_ptr so it gets deleted later.
   size_t file_length = 0;
   wav_file_data_ = ReadWavFile(path_to_wav_file, &file_length);
   if (!wav_file_data_) {
     load_failed_ = true;
     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
   // of it at a time and not the whole thing (like 10 ms at a time).
   AudioParameters file_audio_slice(
       AudioParameters::AUDIO_PCM_LOW_LATENCY,
       GuessChannelLayout(wav_audio_handler_->num_channels()),
       wav_audio_handler_->sample_rate(), wav_audio_handler_->bits_per_sample(),
       params_.frames_per_buffer());
 
@@ skipping 101 matching lines @@
 }
 
 void BeepingSource::OnError(AudioOutputStream* stream) {
 }
 
 void BeepingSource::BeepOnce() {
   g_beep_context.Pointer()->SetBeepOnce(true);
 }
 
 }  // namespace media