Support start trimming post-decoding. Use it with FFmpegDemuxer.

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
index d868382ef42c6a639e832484bc4830ac0b4f6254..e28568549a025f823d0514c8ceb2f35893d130ca 100644
--- a/media/base/audio_discard_helper.cc
+++ b/media/base/audio_discard_helper.cc
@@ -24,8 +24,9 @@ static void WarnOnNonMonotonicTimestamps(base::TimeDelta last_timestamp,
                 << " diff " << diff.InMicroseconds() << " us";
 }
 
-AudioDiscardHelper::AudioDiscardHelper(int sample_rate)
+AudioDiscardHelper::AudioDiscardHelper(int sample_rate, size_t codec_delay)
     : sample_rate_(sample_rate),
+      codec_delay_(codec_delay),
       timestamp_helper_(sample_rate_),
       discard_frames_(0),
       last_input_timestamp_(kNoTimestamp()) {
@@ -44,6 +45,7 @@ void AudioDiscardHelper::Reset(size_t initial_discard) {
   discard_frames_ = initial_discard;
   last_input_timestamp_ = kNoTimestamp();
   timestamp_helper_.SetBaseTimestamp(kNoTimestamp());
+  delayed_discard_ = NULL;
 }
 
 bool AudioDiscardHelper::ProcessBuffers(
@@ -59,15 +61,29 @@ bool AudioDiscardHelper::ProcessBuffers(
   last_input_timestamp_ = encoded_buffer->timestamp();
 
   // If this is the first buffer seen, setup the timestamp helper.
-  if (!initialized()) {
+  const bool first_buffer = !initialized();
+  if (first_buffer) {
     // Clamp the base timestamp to zero.
     timestamp_helper_.SetBaseTimestamp(
         std::max(base::TimeDelta(), encoded_buffer->timestamp()));
   }
   DCHECK(initialized());
 
-  if (!decoded_buffer || !decoded_buffer->frame_count())
+  if (!decoded_buffer) {
+    // If there's a one buffer delay for decoding, we need to save it so it can
+    // be processed with the next decoder buffer.
+    if (first_buffer)
+      delayed_discard_ = encoded_buffer;
     return false;
+  }
+
+  const size_t original_frame_count = decoded_buffer->frame_count();
+
+  // If there's a one buffer delay for decoding, pick up the last encoded buffer
+  // for processing with the current decoded buffer.
+  scoped_refptr<DecoderBuffer> current_encoded_buffer = encoded_buffer;
+  if (delayed_discard_)
+    delayed_discard_.swap(current_encoded_buffer);

[acolwell GONE FROM CHROMIUM, 2014/05/01 01:08:34]

Why do you need the whole buffer? It looks like you only actually use the
discard_padding() everywhere below.

[DaleCurtis, 2014/05/01 01:26:32]

Good point, I'll only store that in the next patch set.

[DaleCurtis, 2014/05/01 19:21:27]

Done.

 
   if (discard_frames_ > 0) {
     const size_t decoded_frames = decoded_buffer->frame_count();
@@ -75,20 +91,74 @@ bool AudioDiscardHelper::ProcessBuffers(
     discard_frames_ -= frames_to_discard;
 
     // If everything would be discarded, indicate a new buffer is required.
-    if (frames_to_discard == decoded_frames)
+    if (frames_to_discard == decoded_frames) {
+      // For simplicity disallow cases where a buffer with discard padding is
+      // present.  Doing so allows us to avoid complexity around tracking
+      // discards across buffers.
+      DCHECK(current_encoded_buffer->discard_padding().first ==
+             base::TimeDelta());
+      DCHECK(current_encoded_buffer->discard_padding().second ==
+             base::TimeDelta());
       return false;
+    }
 
     decoded_buffer->TrimStart(frames_to_discard);
   }
 
-  // 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.  See http://crbug.com/360961.
-  if (encoded_buffer->discard_padding() > base::TimeDelta()) {
+  // Handle front discard padding.
+  if (current_encoded_buffer->discard_padding().first > base::TimeDelta()) {
+    const size_t decoded_frames = decoded_buffer->frame_count();
+    const size_t start_frames_to_discard =
+        TimeDeltaToFrames(current_encoded_buffer->discard_padding().first);

[acolwell GONE FROM CHROMIUM, 2014/05/01 01:08:34]

Why isn't discard padding in frames? You appear to convert it from frames to
time in the demuxer and here you are converting it back to frames.

[DaleCurtis, 2014/05/01 01:26:32]

It's in TimeDelta since the MSE case will be based the delta in append window,
you can see a similar approach in my gapless CL here:

https://codereview.chromium.org/227713008/diff/1/media/filters/legacy_frame_p...

If we use frames, I don't know how the frame processor will figure out the
conversion. I'm happy to change this all to frames if you see a way to make that
work.

[acolwell GONE FROM CHROMIUM, 2014/05/01 17:44:26]

Ok. I see. I don't have a good alternte proposal right now.

+
+    // Regardless of the timestamp on the encoded buffer, the corresponding
+    // decoded output will appear |codec_delay_| frames later.
+    size_t discard_start = codec_delay_;

[acolwell GONE FROM CHROMIUM, 2014/05/01 01:08:34]

Isn't this baking in an assumption that discard_padding() is only set for the
first buffer decoded?

[DaleCurtis, 2014/05/01 01:26:32]

No, this code should work for start discard padding on any buffer, hence the
calculation against codec_delay. Only in the FFmpeg case where codec delay is
munged with skip samples do we have to avoid later buffers having discard
padding.

+    if (codec_delay_ > 0) {

[acolwell GONE FROM CHROMIUM, 2014/05/01 17:44:26]

I think this should be renamed too because this name caused me to assume that
this path would be taken for Opus, but based on your comments and our offline
discussion, it isn't.

[DaleCurtis, 2014/05/01 19:21:27]

Done.

+      // If we have a |codec_delay_| and have already discarded frames from this
+      // buffer, the |discard_start| must be adjusted by the number of frames
+      // already discarded.
+      const size_t frames_discarded_so_far =
+          original_frame_count - decoded_buffer->frame_count();
+      CHECK_LE(frames_discarded_so_far, codec_delay_);
+      discard_start -= frames_discarded_so_far;
+    }
+
+    // For simplicity require the start of the discard to be within the current
+    // buffer.  Doing so allows us avoid complexity around tracking discards
+    // across buffers.
+    CHECK_LT(discard_start, decoded_frames);

[acolwell GONE FROM CHROMIUM, 2014/05/01 01:08:34]

Does this hold for Opus? I was under the impression that the codec delay could
span several buffers.

[DaleCurtis, 2014/05/01 01:26:32]

We may need to discuss this part more, since I'm a little fuzzy on OPUS, this
does seem to work in testing though. My understanding is that codec delay is
encoded into the OPUS packets, such that the packets representing the first
AudioDecoderConfig::codec_delay() of decoded frames are all junk.

My evidence for this is that the opus_multistream_decode_float() call is a 1-1
mapping. Once the end of stream buffer is received there's nothing left.  See
the comment here:

https://code.google.com/p/chromium/codesearch#chromium/src/media/filters/opus...

Because of this |codec_delay_| in AudioDiscardHelper is actually set to zero for
OPUS.

+
+    const size_t frames_to_discard =
+        std::min(start_frames_to_discard, decoded_frames - discard_start);
+
+    // Carry over any frames which need to be discarded from the front of the
+    // next buffer.
+    DCHECK(!discard_frames_);
+    discard_frames_ = start_frames_to_discard - frames_to_discard;
+
+    // If everything would be discarded, indicate a new buffer is required.
+    if (frames_to_discard == decoded_frames) {
+      // The buffer should not have been marked with end discard if the front
+      // discard removes everything.
+      DCHECK(current_encoded_buffer->discard_padding().second ==
+             base::TimeDelta());
+      return false;
+    }
+
+    decoded_buffer->TrimRange(discard_start, discard_start + frames_to_discard);
+  }
+
+  // Handle end discard padding.
+  if (current_encoded_buffer->discard_padding().second > base::TimeDelta()) {
+    // Limit end discarding to when there is no |codec_delay_|, otherwise it's
+    // non-trivial determining where to start discarding end frames.
+    CHECK(!codec_delay_);

[acolwell GONE FROM CHROMIUM, 2014/05/01 01:08:34]

Please add a TODO and file a bug for this since I believe this essentially
blocks this feature for Opus.

[DaleCurtis, 2014/05/01 01:26:32]

As far as I can tell this all works fine. See my comment above. 

+
     const size_t decoded_frames = decoded_buffer->frame_count();
     const size_t end_frames_to_discard =
-        TimeDeltaToFrames(encoded_buffer->discard_padding());
+        TimeDeltaToFrames(current_encoded_buffer->discard_padding().second);
+
     if (end_frames_to_discard > decoded_frames) {
       DLOG(ERROR) << "Encountered invalid discard padding value.";
       return false;
@@ -100,7 +170,8 @@ bool AudioDiscardHelper::ProcessBuffers(
 
     decoded_buffer->TrimEnd(end_frames_to_discard);
   } else {
-    DCHECK(encoded_buffer->discard_padding() == base::TimeDelta());
+    DCHECK(current_encoded_buffer->discard_padding().second ==
+           base::TimeDelta());
   }
 
   // Assign timestamp to the buffer.