Add support for partial append window end trimming.

media/filters/frame_processor.cc

 // 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/filters/frame_processor.h"
 
 #include <cstdlib>
 
 #include "base/stl_util.h"
 #include "media/base/buffers.h"
@@ skipping 314 matching lines @@
 
   return result;
 }
 
 bool FrameProcessor::HandlePartialAppendWindowTrimming(
     base::TimeDelta append_window_start,
     base::TimeDelta append_window_end,
     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;
   }
 
   // If the buffer is entirely before |append_window_start|, save it as preroll
   // for the first buffer which overlaps |append_window_start|.
   if (buffer->timestamp() < append_window_start &&
       frame_end_timestamp <= append_window_start) {
     audio_preroll_buffer_ = buffer;
     return false;
   }
 
   // There's nothing to be done if we have no preroll and the buffer starts on
   // the append window start.
   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 (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.
+    // 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(
     const scoped_refptr<StreamParserBuffer>& frame,
     base::TimeDelta append_window_start,
     base::TimeDelta append_window_end,
     base::TimeDelta* timestamp_offset,
     bool* new_media_segment) {
   // Implements the loop within step 1 of the coded frame processing algorithm
   // for a single input frame per April 1, 2014 MSE spec editor's draft:
@@ skipping 146 matching lines @@
 
         // 7.6. Jump to the Loop Top step above to restart processing of the
         //      current coded frame.
         DVLOG(3) << __FUNCTION__ << ": Discontinuity: reprocessing frame";
         continue;
       }
     }
 
     // 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
     //      the need random access point flag to true, drop the coded frame, and
     //      jump to the top of the loop to start processing the next coded
     //      frame.
     // Note: We keep the result of partial discard of a buffer that overlaps
     //      |append_window_start| and does not end after |append_window_end|.
     // 11. If frame end timestamp is greater than appendWindowEnd, then set the
     //     need random access point flag to true, drop the coded frame, and jump
     //     to the top of the loop to start processing the next coded frame.
     frame->set_timestamp(presentation_timestamp);
     frame->SetDecodeTimestamp(decode_timestamp);
     if (track_buffer->stream()->supports_partial_append_window_trimming() &&
         HandlePartialAppendWindowTrimming(append_window_start,
                                           append_window_end,
                                           frame)) {
       // If |frame| was shortened a discontinuity may exist, so treat the next

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

s/was shortened/was front-trimmed/

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

Done.

       // frames appended as if they were the beginning of a new media segment.
       if (frame->timestamp() != presentation_timestamp && !sequence_mode_)
         *new_media_segment = true;
 
       // |frame| has been partially trimmed or had preroll added.  Though
       // |frame|'s duration may have changed, do not update |frame_duration|
       // here, so |track_buffer|'s last frame duration update uses original
       // 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 ||
         frame_end_timestamp > append_window_end) {
       track_buffer->set_needs_random_access_point(true);
       DVLOG(3) << "Dropping frame that is outside append window.";
       return true;
     }
 
     // Note: This step is relocated, versus April 1 spec, to allow append window
@@ skipping 74 matching lines @@
     DCHECK(group_end_timestamp_ >= base::TimeDelta());
 
     return true;
   }
 
   NOTREACHED();
   return false;
 }
 
 }  // namespace media