Improve input handling for WaveAudioHandler.

media/audio/sounds/wav_audio_handler.cc

Index: media/audio/sounds/wav_audio_handler.cc
diff --git a/media/audio/sounds/wav_audio_handler.cc b/media/audio/sounds/wav_audio_handler.cc
index c9808394aea0c12c2906b64b2e18f7c9d7769b99..9f073149af6e309913f684f9edf6f7419a69883f 100644
--- a/media/audio/sounds/wav_audio_handler.cc
+++ b/media/audio/sounds/wav_audio_handler.cc
@@ -55,34 +55,58 @@ T ReadInt(const base::StringPiece& data, size_t offset) {
 namespace media {
 
 WavAudioHandler::WavAudioHandler(const base::StringPiece& wav_data)
-    : num_channels_(0), sample_rate_(0), bits_per_sample_(0), total_frames_(0) {
-  CHECK_LE(kWavFileHeaderSize, wav_data.size()) << "wav data is too small";
-  CHECK(wav_data.starts_with(kChunkId) &&
-        memcmp(wav_data.data() + 8, kFormat, 4) == 0)
-      << "incorrect wav header";
+    : is_valid_(false),
+      num_channels_(0),
+      sample_rate_(0),
+      bits_per_sample_(0),
+      total_frames_(0) {
+  if (wav_data.size() < kWavFileHeaderSize) {
+    LOG(ERROR) << "wav_data is too small";
+    return;
+  }
+  if (!wav_data.starts_with(kChunkId) ||
+      memcmp(wav_data.data() + 8, kFormat, 4) != 0) {
+    LOG(ERROR) << "incorrect wav header";
+    return;
+  }
 
   uint32 total_length = std::min(ReadInt<uint32>(wav_data, 4),
                                  static_cast<uint32>(wav_data.size()));
   uint32 offset = kWavFileHeaderSize;
   while (offset < total_length) {
     const int length = ParseSubChunk(wav_data.substr(offset));
-    CHECK_LE(0, length) << "can't parse wav sub-chunk";
+    if (length < 0) {
+      LOG(ERROR) << "can't parse wav sub-chunk";
+      ResetAll();
+      return;
+    }
     offset += length;
   }
 
+  if (num_channels_ == 0u || bits_per_sample_ == 0u || sample_rate_ == 0u) {

[tommi (sloooow) - chröme, 2015/11/17 20:48:39]

nit: could this expression be is_valid() or do we really need is_valid_?

[slan, 2015/11/17 21:50:16]

Done.

+    LOG(ERROR) << "Format is invalid. "
+               << "num_channels: " << num_channels_ << " "
+               << "sample_rate: " << sample_rate_ << " "
+               << "bits_per_sample: " << bits_per_sample_;
+    ResetAll();
+    return;
+  }
+
+  // The audio chunk has parsed successfully.
+  is_valid_ = true;
   total_frames_ = data_.size() * 8 / num_channels_ / bits_per_sample_;
 }
 
 WavAudioHandler::~WavAudioHandler() {}
 
 bool WavAudioHandler::AtEnd(size_t cursor) const {
-  return data_.size() <= cursor;
+  return is_valid_ ? data_.size() <= cursor : true;

[tommi (sloooow) - chröme, 2015/11/17 20:48:39]

nit: could also do
return !is_valid() || data_.size() <= cursor;

Actually, is it expected or should it be, that AtEnd() is called if the handler
is not valid?  Maybe there should just be a DCHECK(is_valid()); at the top of
each function?

[slan, 2015/11/17 21:50:16]

In practice, this object should not be used if is_valid() returns false. Adding
IsValid() seemed defensive and has precedent (AudioParameters), where I don't
see many cases of an object with methods that can't be touched after the ctor.

Alternatively, what do you think about a factory method that returns a
scoped_ptr<WavAudioHandler> iff it parses successfully (and null ptr otherwise)?
Thoughts?

[tommi (sloooow) - chröme, 2015/11/18 11:52:58]

That sounds like a good idea to me and would simplify error handling quite a bit
in this class. We might also be able to make some members const. I don't see any
thread checks, so I think that would be a very good thing actually.

 }
 
 bool WavAudioHandler::CopyTo(AudioBus* bus,
                              size_t cursor,
                              size_t* bytes_written) const {
-  if (!bus)
+  if (!is_valid_ || !bus)
     return false;
   if (bus->channels() != num_channels_) {
     DVLOG(1) << "Number of channels mismatch.";
@@ -103,8 +127,9 @@ bool WavAudioHandler::CopyTo(AudioBus* bus,
 }
 
 base::TimeDelta WavAudioHandler::GetDuration() const {
-  return base::TimeDelta::FromSecondsD(total_frames_ /
-                                       static_cast<double>(sample_rate_));
+  return is_valid_ ? base::TimeDelta::FromSecondsD(
+                         total_frames_ / static_cast<double>(sample_rate_))
+                   : base::TimeDelta();
 }
 
 int WavAudioHandler::ParseSubChunk(const base::StringPiece& data) {
@@ -140,4 +165,13 @@ bool WavAudioHandler::ParseDataChunk(const base::StringPiece& data) {
   return true;
 }
 
+void WavAudioHandler::ResetAll() {
+  DCHECK(!is_valid_);
+  data_ = base::StringPiece();

[tommi (sloooow) - chröme, 2015/11/17 20:48:39]

another potential implementation for is_valid();

return !data_.empty();

[slan, 2015/11/17 21:50:16]

I like the idea you left above. Theoretically, we could have an emppty WAV bit,
no?

+  num_channels_ = 0u;
+  sample_rate_ = 0u;
+  bits_per_sample_ = 0u;
+  total_frames_ = 0u;
+}
+
 }  // namespace media