Add support for partial append window end trimming.

media/filters/frame_processor.cc

Index: media/filters/frame_processor.cc
diff --git a/media/filters/frame_processor.cc b/media/filters/frame_processor.cc
index a10ed5017ed50814a2269dd0fde06d8fa3189618..67c2b0ae3673bf826bb29903b6062a82b1621254 100644
--- a/media/filters/frame_processor.cc
+++ b/media/filters/frame_processor.cc
@@ -332,18 +332,15 @@ bool FrameProcessor::HandlePartialAppendWindowTrimming(
     const scoped_refptr<StreamParserBuffer>& buffer) {
   DCHECK(buffer->duration() > base::TimeDelta());
   DCHECK_EQ(DemuxerStream::AUDIO, buffer->type());
+  DCHECK(buffer->IsKeyframe());
 
   const base::TimeDelta frame_end_timestamp =
       buffer->timestamp() + buffer->duration();
 
-  // Ignore any buffers which start after |append_window_start| or end after
-  // |append_window_end|.  For simplicity, even those that start before
-  // |append_window_start|.
-  if (buffer->timestamp() > append_window_start ||
-      frame_end_timestamp > append_window_end) {
-    // TODO(dalecurtis): Partial append window trimming could also be done
-    // around |append_window_end|, but is not necessary since splice frames
-    // cover overlaps there.
+  // Ignore any buffers entirely after or inside of the append window.
+  if (buffer->timestamp() > append_window_end ||
+      (buffer->timestamp() > append_window_start &&

[wolenetz, 2014/07/25 22:33:03]

Is there any allowance we need to make here for applying a previously saved
audio_preroll_buffer_ to a buffer that starts slightly after
append_window_start? ISTM we need to allow variance within +/-
|sample_duration_| here rather than always assuming there is no partial append
window processing needed here if buffer->timestamp() is strictly >
|append_window_start_|.
Also, this looks like it needs some unit test coverage; please add.

[DaleCurtis, 2014/07/29 01:35:10]

We don't attach preroll for disjoint buffers.  If a buffer started after
append_window_start there'd be a gap between any buffer which was before it. 
Otherwise we'd have already processed that buffer for partial discard and
attached any preroll to it.

That said, we were missing cases where the preroll was within range for use
around the append window start.  I've reflowed the code for legibility in this
regard.

+       frame_end_timestamp <= append_window_end)) {
     return false;
   }
 
@@ -360,44 +357,63 @@ bool FrameProcessor::HandlePartialAppendWindowTrimming(
   if (buffer->timestamp() == append_window_start && !audio_preroll_buffer_)
     return false;
 
+  bool trimmed_buffer = false;
+
   // See if a partial discard can be done around |append_window_start|.
-  DCHECK(buffer->timestamp() <= append_window_start);
-  DCHECK(buffer->IsKeyframe());
-  DVLOG(1) << "Truncating buffer which overlaps append window start."
-           << " presentation_timestamp " << buffer->timestamp().InSecondsF()
-           << " append_window_start " << append_window_start.InSecondsF();
-
-  // If this isn't the first buffer discarded by the append window, try to use
-  // the last buffer discarded for preroll.  This ensures that the partially
-  // trimmed buffer can be correctly decoded.
-  if (audio_preroll_buffer_) {
-    // We only want to use the preroll buffer if it directly precedes (less than
-    // one sample apart) the current buffer.
-    const int64 delta = std::abs((audio_preroll_buffer_->timestamp() +
-                                  audio_preroll_buffer_->duration() -
-                                  buffer->timestamp()).InMicroseconds());
-    if (delta < sample_duration_.InMicroseconds()) {
-      buffer->SetPrerollBuffer(audio_preroll_buffer_);
-    } else {
-      // TODO(dalecurtis): Add a MEDIA_LOG() for when this is dropped unused.
+  if (buffer->timestamp() <= append_window_start) {

[wolenetz, 2014/07/25 22:33:03]

ditto

[DaleCurtis, 2014/07/29 01:35:10]

Acknowledged.

+    DVLOG(1) << "Truncating buffer which overlaps append window start."
+             << " presentation_timestamp " << buffer->timestamp().InSecondsF()
+             << " frame_end_timestamp " << frame_end_timestamp.InSecondsF()
+             << " append_window_start " << append_window_start.InSecondsF();
+
+    // If this isn't the first buffer discarded by the append window, try to use
+    // the last buffer discarded for preroll.  This ensures that the partially
+    // trimmed buffer can be correctly decoded.
+    if (audio_preroll_buffer_) {
+      // We only want to use the preroll buffer if it directly precedes (less
+      // than one sample apart) the current buffer.
+      const int64 delta = std::abs((audio_preroll_buffer_->timestamp() +
+                                    audio_preroll_buffer_->duration() -
+                                    buffer->timestamp()).InMicroseconds());
+      if (delta < sample_duration_.InMicroseconds()) {
+        buffer->SetPrerollBuffer(audio_preroll_buffer_);
+      } else {
+        // TODO(dalecurtis): Add a MEDIA_LOG() for when this is dropped unused.
+      }
+      audio_preroll_buffer_ = NULL;
     }
-    audio_preroll_buffer_ = NULL;
+
+    // Decrease the duration appropriately.  We only need to shorten the buffer
+    // if it overlaps |append_window_start|.
+    if (buffer->timestamp() < append_window_start) {
+      buffer->set_discard_padding(std::make_pair(
+          append_window_start - buffer->timestamp(), base::TimeDelta()));
+      buffer->set_duration(frame_end_timestamp - append_window_start);
+    }
+
+    // Adjust the timestamp of this buffer forward to |append_window_start|. The
+    // timestamps are always set, even if |buffer|'s timestamp is already set to
+    // |append_window_start| to ensure the preroll buffer is setup correctly.
+    buffer->set_timestamp(append_window_start);
+    buffer->SetDecodeTimestamp(append_window_start);
+    trimmed_buffer = true;
   }
 
-  // Decrease the duration appropriately.  We only need to shorten the buffer if
-  // it overlaps |append_window_start|.
-  if (buffer->timestamp() < append_window_start) {
-    buffer->set_discard_padding(std::make_pair(
-        append_window_start - buffer->timestamp(), base::TimeDelta()));
-    buffer->set_duration(frame_end_timestamp - append_window_start);
+  // See if a partial discard can be done around |append_window_end|.
+  if (buffer->timestamp() < append_window_end &&
+      frame_end_timestamp > append_window_end) {
+    DVLOG(1) << "Truncating buffer which overlaps append window end."
+             << " presentation_timestamp " << buffer->timestamp().InSecondsF()
+             << " frame_end_timestamp " << frame_end_timestamp.InSecondsF()
+             << " append_window_end " << append_window_end.InSecondsF();
+    buffer->set_discard_padding(
+        std::make_pair(buffer->discard_padding().first,
+                       frame_end_timestamp - append_window_end));
+    buffer->set_duration(append_window_end - buffer->timestamp());
+    trimmed_buffer = true;
   }
 
-  // Adjust the timestamp of this buffer forward to |append_window_start|.  The
-  // timestamps are always set, even if |buffer|'s timestamp is already set to
-  // |append_window_start|, to ensure the preroll buffer is setup correctly.
-  buffer->set_timestamp(append_window_start);
-  buffer->SetDecodeTimestamp(append_window_start);
-  return true;
+  return trimmed_buffer;
 }
 
 bool FrameProcessor::ProcessFrame(
@@ -564,7 +580,7 @@ bool FrameProcessor::ProcessFrame(
 
     // 9. Let frame end timestamp equal the sum of presentation timestamp and
     //    frame duration.
-    const base::TimeDelta frame_end_timestamp =
+    base::TimeDelta frame_end_timestamp =
         presentation_timestamp + frame_duration;
 
     // 10.  If presentation timestamp is less than appendWindowStart, then set
@@ -593,9 +609,7 @@ bool FrameProcessor::ProcessFrame(
       // frame duration and reduces spurious discontinuity detection.
       decode_timestamp = frame->GetDecodeTimestamp();
       presentation_timestamp = frame->timestamp();
-
-      // The end timestamp of the frame should be unchanged.
-      DCHECK(frame_end_timestamp == presentation_timestamp + frame->duration());
+      frame_end_timestamp = frame->timestamp() + frame->duration();

[wolenetz, 2014/07/25 22:33:04]

nit: Though we currently require all paths that reach here to be due to
keyframed buffer streams, it would be good to either comment, or even better
require in code that track_buffer->set_needs_random_access_point(true) is called
if we reach this point in this code block. See the next code block, below, that
would do this except |presentation_timestamp| and |frame_end_timestamp| are
clamped to the append window in this code path here.

[DaleCurtis, 2014/07/29 01:35:10]

Are you saying we should call that if we trim the buffer in any way?  Or always?

[wolenetz, 2014/07/30 22:04:31]

I see. If just preroll added, we don't need to call
set_needs_random_access_point(true) on track_buffer. But if trimmed in any way,
we should (though for keyframed streams it I think is just overhead).

[wolenetz, 2014/07/30 23:12:34]

I think my earlier reply to this comment was missed.

[DaleCurtis, 2014/07/31 00:00:32]

Whoops, I didn't see this, though I'm not sure I follow. Can you elaborate or
why this should be called here? It's required via DCHECK that all frames trimmed
are keyframes. Partial append window trimming all happens post decode, so it
would never change the frame type anyways.

To me it seems you only want to call set_needs_random_access_point() if you are
completely dropping frames. 

     }
 
     if (presentation_timestamp < append_window_start ||