Improve input handling for WaveAudioHandler.

media/audio/sounds/wav_audio_handler.cc

 // Copyright 2013 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 "media/audio/sounds/wav_audio_handler.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/logging.h"
@@ skipping 37 matching lines @@
   result = base::ByteSwap(result);
 #endif
   return result;
 }
 
 }  // namespace
 
 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.";
     return false;
   }
   if (AtEnd(cursor)) {
     bus->Zero();
     return true;
   }
   const int bytes_per_frame = num_channels_ * bits_per_sample_ / 8;
   const int remaining_frames = (data_.size() - cursor) / bytes_per_frame;
   const int frames = std::min(bus->frames(), remaining_frames);
 
   bus->FromInterleaved(data_.data() + cursor, frames, bits_per_sample_ / 8);
   *bytes_written = frames * bytes_per_frame;
   bus->ZeroFramesPartial(frames, bus->frames() - frames);
   return true;
 }
 
 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) {
   if (data.size() < kChunkHeaderSize)
     return data.size();
   uint32 chunk_length = ReadInt<uint32>(data, 4);
   if (data.starts_with(kSubchunk1Id)) {
     if (!ParseFmtChunk(data.substr(kChunkHeaderSize, chunk_length)))
       return -1;
   } else if (data.starts_with(kSubchunk2Id)) {
@@ skipping 15 matching lines @@
   sample_rate_ = ReadInt<uint32>(data, kSampleRateOffset);
   bits_per_sample_ = ReadInt<uint16>(data, kBitsPerSampleOffset);
   return true;
 }
 
 bool WavAudioHandler::ParseDataChunk(const base::StringPiece& data) {
   data_ = 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