Introduce AudioDiscardHelper. Refactor audio decoders to use it.

media/base/audio_discard_helper.cc

Index: media/base/audio_discard_helper.cc
diff --git a/media/base/audio_discard_helper.cc b/media/base/audio_discard_helper.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b907fe8e4858d3a4b92fcaa6de5a3eeb17fa65d7
--- /dev/null
+++ b/media/base/audio_discard_helper.cc
@@ -0,0 +1,110 @@
+// Copyright 2014 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/base/audio_discard_helper.h"
+
+#include <cmath>
+
+#include "base/logging.h"
+#include "media/base/audio_buffer.h"
+#include "media/base/buffers.h"
+#include "media/base/decoder_buffer.h"
+
+namespace media {
+
+static void WarnOnNonMonotonicTimestamps(base::TimeDelta last_timestamp,
+                                         base::TimeDelta current_timestamp) {
+  if (last_timestamp == kNoTimestamp() || last_timestamp < current_timestamp)
+    return;
+
+  const base::TimeDelta diff = current_timestamp - last_timestamp;
+  DLOG(WARNING) << "Input timestamps are not monotonically increasing! "
+                << " ts " << current_timestamp.InMicroseconds() << " us"
+                << " diff " << diff.InMicroseconds() << " us";
+}
+
+AudioDiscardHelper::AudioDiscardHelper(int sample_rate)
+    : sample_rate_(sample_rate),
+      timestamp_helper_(sample_rate_),
+      discard_frames_(0),
+      last_input_timestamp_(kNoTimestamp()) {

[wolenetz, 2014/04/28 21:52:07]

nit: is any sanity checking needed on sample_rate_ (zero, negative, or too
large?)

[DaleCurtis, 2014/04/28 23:09:03]

As far as this code is concerned it just needs to be > 0.  Added a DCHECK().

+}
+
+AudioDiscardHelper::~AudioDiscardHelper() {
+}
+
+int AudioDiscardHelper::TimeDeltaToFrames(base::TimeDelta duration) const {
+  DCHECK(duration >= base::TimeDelta());
+  return std::ceil(duration.InSecondsF() * sample_rate_);

[wolenetz, 2014/04/28 21:52:07]

With ceil(), might we hit off-by-one stuff here? (Does this matter?)

[DaleCurtis, 2014/04/28 23:09:03]

Since this floating point value is calculated from two integers it's unlikely
we'll see floating point error here.  I don't think it matters in any case.

+}
+
+void AudioDiscardHelper::Reset(size_t initial_discard) {
+  discard_frames_ = initial_discard;
+  last_input_timestamp_ = kNoTimestamp();
+  timestamp_helper_.SetBaseTimestamp(kNoTimestamp());
+}
+
+bool AudioDiscardHelper::ProcessBuffers(
+    const scoped_refptr<DecoderBuffer>& encoded_buffer,
+    const scoped_refptr<AudioBuffer>& decoded_buffer) {
+  DCHECK(!encoded_buffer->end_of_stream());
+  DCHECK(encoded_buffer->timestamp() != kNoTimestamp());
+
+  // Issue a debug warning when we see non-monotonic timestamps.  Only a warning
+  // to allow chained OGG playback.
+  WarnOnNonMonotonicTimestamps(last_input_timestamp_,
+                               encoded_buffer->timestamp());
+  last_input_timestamp_ = encoded_buffer->timestamp();
+
+  // If this is the first buffer seen, setup the timestamp helper.
+  if (!initialized())
+    timestamp_helper_.SetBaseTimestamp(encoded_buffer->timestamp());
+  DCHECK(initialized());
+
+  if (!decoded_buffer || !decoded_buffer->frame_count())
+    return false;
+
+  if (discard_frames_ > 0) {
+    const size_t decoded_frames = decoded_buffer->frame_count();
+    const size_t frames_to_discard = std::min(discard_frames_, decoded_frames);
+    discard_frames_ -= frames_to_discard;
+
+    // If everything would be discarded, indicate a new buffer is required.
+    if (frames_to_discard == decoded_frames)
+      return false;
+
+    decoded_buffer->TrimStart(frames_to_discard);

[wolenetz, 2014/04/28 21:52:07]

TrimStart() will modify decoded_buffer's duration_ and potential later TrimEnd()
called from this method will use that modified duration_ prior to this method
explicitly setting the decoded_buffer's duration. ISTM like this could introduce
variant processing or at least be hard to maintain and keep bug-free (which
calculation of decoded buffer duration should be used, and when?) Note also that
TrimEnd() also adjusts the buffer's duration. TrimEnd() & TrimStart() directly
impact the buffer's frame_count(), but perhaps in ways different from this
method's expectation.

[DaleCurtis, 2014/04/28 22:03:51]

It's this codes expectation that it is the sole authority of decoded buffer
durations and timestamps.  Prior to setting the duration below, it does not
matter what the duration() or timestamp() are.

[wolenetz, 2014/04/28 22:37:56]

Regarding assumption of ability to set duration/timestamp externally, after
construction, of an AudioBuffer, it already looks like there is a partial plan
to disallow that
(https://code.google.com/p/chromium/codesearch#chromium/src/media/base/audio_b...).
Hence, maintainability of the assumption that AudioDiscardHelper "owns" the
maintenance externally of AudioBuffer duration and timestamp seems fragile
already. Perhaps adjust this comment in AudioBuffer?

[DaleCurtis, 2014/04/28 23:09:03]

I think that comment can be removed now.  I'll do so in the next patch set.

+  }
+
+  // TODO(dalecurtis): Applying the current buffer's discard padding doesn't
+  // make sense in the Vorbis case because there is a delay of one buffer before
+  // decoded buffers are returned. Fix and add support for more than just end
+  // trimming.

[wolenetz, 2014/04/28 21:52:07]

nit: reference bug?

[DaleCurtis, 2014/04/28 23:09:03]

Done.

+  if (encoded_buffer->discard_padding() > base::TimeDelta()) {
+    const size_t decoded_frames = decoded_buffer->frame_count();
+    const size_t end_frames_to_discard =
+        TimeDeltaToFrames(encoded_buffer->discard_padding());
+    if (end_frames_to_discard > decoded_frames) {

[wolenetz, 2014/04/28 21:52:07]

Is end discard aware of start discard? (If not, could a 'valid' stream contain
more trimming than actual start+end discard?)

[DaleCurtis, 2014/04/28 22:03:51]

It's expected that users of discard_padding() will ensure there's no overflow. 
Hence the error log.  End trimming only works in certain circumstances as you
can see in part II of this CL, so for now this error tracks the existing use
case.

+      DLOG(ERROR) << "Invalid file. Incorrect discard padding value.";

[wolenetz, 2014/04/28 21:52:07]

nit: If this helper will be used by MSE, not just ffmpeg, then s/file/stream?

[DaleCurtis, 2014/04/28 23:09:03]

Done.

+      return false;
+    }
+
+    // If everything would be discarded, indicate a new buffer is required.
+    if (end_frames_to_discard == decoded_frames)
+      return false;
+
+    decoded_buffer->TrimEnd(end_frames_to_discard);
+  } else {
+    DCHECK(encoded_buffer->discard_padding() == base::TimeDelta());
+  }
+
+  // Assign timestamp and duration to the buffer.
+  decoded_buffer->set_timestamp(timestamp_helper_.GetTimestamp());
+  decoded_buffer->set_duration(
+      timestamp_helper_.GetFrameDuration(decoded_buffer->frame_count()));
+  timestamp_helper_.AddFrames(decoded_buffer->frame_count());
+  return true;
+}
+
+}  // namespace media