MSE: Make WebMClusterParser hold back buffers at or beyond buffer missing duration

media/formats/webm/webm_cluster_parser.h

Index: media/formats/webm/webm_cluster_parser.h
diff --git a/media/formats/webm/webm_cluster_parser.h b/media/formats/webm/webm_cluster_parser.h
index add21f23f91d432a20efd47cc32bbaa1e8d8bfc1..f86fc6e27ee4e77eb555f3be43d06b5d269188d7 100644
--- a/media/formats/webm/webm_cluster_parser.h
+++ b/media/formats/webm/webm_cluster_parser.h
@@ -23,6 +23,8 @@ namespace media {
 class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
  public:
   typedef StreamParser::TrackId TrackId;
+  typedef std::deque<scoped_refptr<StreamParserBuffer> > BufferQueue;
+  typedef std::map<TrackId, const BufferQueue> TextBufferQueueMap;
 
   // Arbitrarily-chosen numbers to estimate the duration of a buffer if none is
   // set and there is not enough information to get a better estimate.
@@ -37,13 +39,37 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
   // Helper class that manages per-track state.
   class Track {
    public:
-    Track(int track_num, bool is_video, base::TimeDelta default_duration);
+    Track(int track_num,
+          bool is_video,
+          base::TimeDelta default_duration,
+          const LogCB& log_cb);
     ~Track();
 
     int track_num() const { return track_num_; }
-    const std::deque<scoped_refptr<StreamParserBuffer> >& buffers() const {
-      return buffers_;
-    }
+
+    // If a buffer is currently held aside pending duration calculation, returns
+    // its decode timestamp. Otherwise, returns kInfiniteDuration(). Across all
+    // tracks being parsed by a WebMClusterParser, the minimum value of their

[acolwell GONE FROM CHROMIUM, 2014/04/23 22:36:39]

nit: It seems like text from about here and below is TMI and could easily get
out of sync w/ the actual code.

[wolenetz, 2014/04/25 20:04:21]

Done.

+    // GetReadyUpperBound() determines the exclusive maximum buffer decode
+    // timestamp limit for buffers ready for emission. This mechanism ensures
+    // any buffers held aside for duration calculation also prevent emission of
+    // buffers from other tracks with same or greater decode timestamp. See also
+    // WebMClusterParser::UpdateReadyBuffers().
+    base::TimeDelta GetReadyUpperBound();
+
+    // Extracts and returns |ready_buffers_|, which are the initial portion of

[acolwell GONE FROM CHROMIUM, 2014/04/23 22:36:39]

nit: ditto. I think a smaller comment here would be more helpful and avoid this
text getting out of sync w/ the code.

[wolenetz, 2014/04/25 20:04:21]

Done.

+    // |buffers_|, if any, excluding all buffers with timestamp at or after
+    // |before_timestamp|. Removes all ready buffers from |buffers_|. If
+    // |before_timestamp| is kInfiniteDuration(), then |ready_buffers| is filled
+    // with all of |buffers_|. Note, QueueBuffer() ensures that |buffers_| is in
+    // non-decreasing timestamp order.
+    // The returned BufferQueue& is cleared by ClearReadyBuffers() and Reset(),
+    // and it must be empty prior to the next ExtractReadyBuffers() call.
+    // ready_buffers() does not do any extraction, rather, it just returns
+    // the current |ready_buffers_| even if not yet extracted since last clear.
+    const BufferQueue& ExtractReadyBuffers(
+        const base::TimeDelta before_timestamp);
+    const BufferQueue& ready_buffers() const { return ready_buffers_; }
 
     // If |last_added_buffer_missing_duration_| is set, updates its duration
     // relative to |buffer|'s timestamp, and adds it to |buffers_| and unsets
@@ -59,12 +85,14 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
     // emit all buffers in a media segment before signaling end of segment.)
     void ApplyDurationEstimateIfNeeded();
 
-    // Clears all buffer state, except a possibly held-aside buffer that is
+    // Clears |ready_buffers_| (use ExtractReadyBuffers() to fill it again).
+    // Leaves as-is |buffers_| and any possibly held-aside buffer that is
     // missing duration.
-    void ClearBuffersButKeepLastIfMissingDuration();
+    void ClearReadyBuffers();
 
     // Clears all buffer state, including any possibly held-aside buffer that
-    // was missing duration.
+    // was missing duration, and all contents of |buffers_| and
+    // |ready_buffers_|.
     void Reset();
 
     // Helper function used to inspect block data to determine if the
@@ -87,27 +115,28 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
     base::TimeDelta GetDurationEstimate();
 
     int track_num_;
-    std::deque<scoped_refptr<StreamParserBuffer> > buffers_;
+    BufferQueue buffers_;
     bool is_video_;
     scoped_refptr<StreamParserBuffer> last_added_buffer_missing_duration_;
 
+    // Extracted buffers from |buffers_|, maintained by ExtractReadyBuffers(),
+    // and cleared by Reset() and ClearReadyBuffers().

[acolwell GONE FROM CHROMIUM, 2014/04/23 22:36:39]

nit: This comment doesn't really explain the difference between what is held in
buffers_ vs what is held in ready_buffers_. What does it mean to be "ready"?

[wolenetz, 2014/04/25 20:04:21]

Done.

+    BufferQueue ready_buffers_;
+
     // If kNoTimestamp(), then |estimated_next_frame_duration_| will be used.
     base::TimeDelta default_duration_;
+
     // If kNoTimestamp(), then a default value will be used. This estimate is
     // the maximum duration seen or derived so far for this track, and is valid
     // only if |default_duration_| is kNoTimestamp().
-    //
-    // TODO(wolenetz): Add unittests for duration estimation and default
-    // duration handling. http://crbug.com/361786 .
     base::TimeDelta estimated_next_frame_duration_;
+
+    LogCB log_cb_;
   };
 
   typedef std::map<int, Track> TextTrackMap;
 
  public:
-  typedef std::deque<scoped_refptr<StreamParserBuffer> > BufferQueue;
-  typedef std::map<TrackId, const BufferQueue> TextBufferQueueMap;
-
   WebMClusterParser(int64 timecode_scale,
                     int audio_track_num,
                     base::TimeDelta audio_default_duration,
@@ -132,17 +161,32 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
 
   base::TimeDelta cluster_start_time() const { return cluster_start_time_; }
 
-  // Get the buffers resulting from Parse().
+  // Get the current ready buffers resulting from Parse().
   // If the parse reached the end of cluster and the last buffer was held aside
   // due to missing duration, the buffer is given an estimated duration and
   // included in the result.
+  // Otherwise, if there are is a buffer held aside due to missing duration for
+  // any of the tracks, no buffers with same or greater (decode) timestamp will
+  // be included in the buffers.
+  // The returned deques are cleared by Parse() or Reset() and updated by the
+  // next calls to Get{Audio,Video}Buffers().
+  // If no Parse() or Reset() has occurred since the last call to Get{Audio,
+  // Video,Text}Buffers(), then the previous BufferQueue& is returned again
+  // without any recalculation.
   const BufferQueue& GetAudioBuffers();
   const BufferQueue& GetVideoBuffers();
 
   // Constructs and returns a subset of |text_track_map_| containing only
-  // tracks with non-empty buffer queues produced by the last Parse().
+  // tracks with non-empty buffer queues produced by the last Parse() and
+  // filtered to exclude any buffers that have (decode) timestamp same or
+  // greater than the lowest (decode) timestamp across all tracks of any buffer
+  // held aside due to missing duration (unless the end of cluster has been
+  // reached).
   // The returned map is cleared by Parse() or Reset() and updated by the next
   // call to GetTextBuffers().
+  // If no Parse() or Reset() has occurred since the last call to
+  // GetTextBuffers(), then the previous TextBufferQueueMap& is returned again
+  // without any recalculation.
   const TextBufferQueueMap& GetTextBuffers();
 
   // Returns true if the last Parse() call stopped at the end of a cluster.
@@ -166,6 +210,22 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
   // Resets the Track objects associated with each text track.
   void ResetTextTracks();
 
+  // Clears the the ready buffers associated with each text track.
+  void ClearTextTrackReadyBuffers();
+
+  // Helper method for Get{Audio,Video,Text}Buffers() that recomputes
+  // |ready_buffer_upper_bound_| and calls ExtractReadyBuffers() on each track.
+  // If |cluster_ended_| is true, first applies duration estimate if needed for
+  // |audio_| and |video_| and sets |ready_buffer_upper_bound_| to
+  // kInfiniteDuration(). Otherwise, sets |ready_buffer_upper_bound_| to the
+  // minimum upper bound across |audio_| and |video_|. (Text tracks can have no
+  // buffers missing duration, so they are not involved in calculating the upper
+  // bound.)
+  // Parse() or Reset() must be called between calls to UpdateReadyBuffers() to
+  // clear each track's ready buffers and to reset |ready_buffer_upper_bound_|
+  // to kNoTimestamp().
+  void UpdateReadyBuffers();
+
   // Search for the indicated track_num among the text tracks.  Returns NULL
   // if that track num is not a text track.
   Track* FindTextTrack(int track_num);
@@ -197,10 +257,18 @@ class MEDIA_EXPORT WebMClusterParser : public WebMParserClient {
   TextTrackMap text_track_map_;
 
   // Subset of |text_track_map_| maintained by GetTextBuffers(), and cleared by
-  // ResetTextTracks(). Callers of GetTextBuffers() get a const-ref to this
-  // member.
+  // ClearTextTrackReadyBuffers(). Callers of GetTextBuffers() get a const-ref
+  // to this member.
   TextBufferQueueMap text_buffers_map_;
 
+  // Limits the range of buffers returned by Get{Audio,Video,Text}Buffers() to
+  // this exclusive upper bound. Set to kNoTimestamp(), meaning not yet
+  // calculated, by Reset() and Parse(). If kNoTimestamp(), then
+  // Get{Audio,Video,Text}Buffers() will calculate it to be the minimum (decode)
+  // timestamp across all tracks' |last_buffer_missing_duration_|, or
+  // kInfiniteDuration() if no buffers are currently missing duration.
+  base::TimeDelta ready_buffer_upper_bound_;
+
   LogCB log_cb_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(WebMClusterParser);