MSE: Optimize frame processor appends to streams

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 8fb25c34e6c3f7fbc9c1ecd55f64f803badcc911..0333e5bb88d566ebb3489c900f17a1fb647b23c8 100644
--- a/media/filters/frame_processor.cc
+++ b/media/filters/frame_processor.cc
@@ -67,6 +67,14 @@ class MseTrackBuffer {
   // monotonically increasing.
   void SetHighestPresentationTimestampIfIncreased(base::TimeDelta timestamp);
 
+  // Adds |frame_| to the end of |processed_frames_|.

[acolwell GONE FROM CHROMIUM, 2014/07/09 00:11:47]

nit: s/frame_/frame/

[wolenetz, 2014/07/09 18:23:29]

Done.

+  void EnqueueProcessedFrame(const scoped_refptr<StreamParserBuffer>& frame);
+
+  // Appends |processed_frames_|, if not empty, to |stream_| and clears
+  // |processed_frames_|. Returns false if append failed, true otherwise.
+  // |processed_frames_| is cleared in both cases.
+  bool FlushProcessedFrames();
+
  private:
   // The decode timestamp of the last coded frame appended in the current coded
   // frame group. Initially kNoTimestamp(), meaning "unset".
@@ -91,6 +99,11 @@ class MseTrackBuffer {
   // by |this|.
   ChunkDemuxerStream* const stream_;
 
+  // Queue of processed frames that have not yet been appended to |stream_|.
+  // EnqueueProcessedFrame() adds to this queue, and FlushProcessedFrames()
+  // clears it.
+  StreamParser::BufferQueue processed_frames_;
+
   DISALLOW_COPY_AND_ASSIGN(MseTrackBuffer);
 };
 
@@ -124,6 +137,25 @@ void MseTrackBuffer::SetHighestPresentationTimestampIfIncreased(
   }
 }
 
+void MseTrackBuffer::EnqueueProcessedFrame(
+    const scoped_refptr<StreamParserBuffer>& frame) {
+  processed_frames_.push_back(frame);
+}
+
+bool MseTrackBuffer::FlushProcessedFrames() {
+  if (processed_frames_.empty())
+    return true;
+
+  bool result = stream_->Append(processed_frames_);
+  processed_frames_.clear();
+  if (!result) {
+    DVLOG(3) << __FUNCTION__

[damienv1, 2014/07/09 01:59:24]

nit: DVLOG_IF

[wolenetz, 2014/07/09 18:23:29]

Done.

+             << "(): Failure appending processed frames to stream";
+  }
+
+  return result;
+}
+
 FrameProcessor::FrameProcessor(const UpdateDurationCB& update_duration_cb)
     : sequence_mode_(false),
       group_start_timestamp_(kNoTimestamp()),
@@ -180,10 +212,14 @@ bool FrameProcessor::ProcessFrames(
        frames_itr != frames.end(); ++frames_itr) {
     if (!ProcessFrame(*frames_itr, append_window_start, append_window_end,
                       timestamp_offset, new_media_segment)) {
+      FlushProcessedFrames();
       return false;
     }
   }
 
+  if (!FlushProcessedFrames())
+    return false;
+
   // 2. - 4. Are handled by the WebMediaPlayer / Pipeline / Media Element.
 
   // Step 5:
@@ -278,6 +314,19 @@ void FrameProcessor::NotifyNewMediaSegmentStarting(
   }
 }
 
+bool FrameProcessor::FlushProcessedFrames() {
+  DVLOG(2) << __FUNCTION__ << "()";
+
+  bool result = true;
+  for (TrackBufferMap::iterator itr = track_buffers_.begin();
+       itr != track_buffers_.end();
+       ++itr) {
+    result &= itr->second->FlushProcessedFrames();

[damienv1, 2014/07/09 01:56:56]

Notation might be confusing here.
I would rather do sth like:

if (!itr->second->FlushProcessedFrames())
  result = false;

Had you written: result = result && itr->second->FlushProcessedFrames();
then FlushProcessedFrames would not have been called for each stream (if
flushing fails for the first stream).

[wolenetz, 2014/07/09 18:23:29]

Done. Note, as you very probably know, C++ &= and |= are not short-circuiting.
There are also no ||= or &&= in C++ (which would probably short-circuit if they
did exist). We use &= and |= in many places -- when masking or setting up flags,
for instance. However, I'll make this explicit as you suggest to prevent
confusion here.

+  }
+
+  return result;
+}
+
 bool FrameProcessor::HandlePartialAppendWindowTrimming(
     base::TimeDelta append_window_start,
     base::TimeDelta append_window_end,
@@ -602,6 +651,11 @@ bool FrameProcessor::ProcessFrame(
     // If it is the first in a new media segment or following a discontinuity,
     // notify all the track buffers' streams that a new segment is beginning.
     if (*new_media_segment) {
+      // First, complete the append to track buffer streams of previous media
+      // segment's frames, if any.
+      if (!FlushProcessedFrames())
+        return false;
+
       *new_media_segment = false;
       NotifyNewMediaSegmentStarting(decode_timestamp);
     }
@@ -610,16 +664,12 @@ bool FrameProcessor::ProcessFrame(
              << "PTS=" << presentation_timestamp.InSecondsF()
              << ", DTS=" << decode_timestamp.InSecondsF();
 
-    // Steps 13-18:
-    // TODO(wolenetz): Collect and emit more than one buffer at a time, if
-    // possible. Also refactor SourceBufferStream to conform to spec GC timing.
+    // Steps 13-18: Note, we optimize by appending groups of contiguous
+    // processed frames for each track buffer at end of ProcessFrames() or prior
+    // to NotifyNewMediaSegmentStarting().
+    // TODO(wolenetz): Refactor SourceBufferStream to conform to spec GC timing.
     // See http://crbug.com/371197.
-    StreamParser::BufferQueue buffer_to_append;
-    buffer_to_append.push_back(frame);
-    if (!track_buffer->stream()->Append(buffer_to_append)) {
-      DVLOG(3) << __FUNCTION__ << ": Failure appending frame to stream";
-      return false;
-    }
+    track_buffer->EnqueueProcessedFrame(frame);
 
     // 19. Set last decode timestamp for track buffer to decode timestamp.
     track_buffer->set_last_decode_timestamp(decode_timestamp);