Fix MSE GC, make it less aggressive, more spec-compliant

media/filters/chunk_demuxer.cc

Index: media/filters/chunk_demuxer.cc
diff --git a/media/filters/chunk_demuxer.cc b/media/filters/chunk_demuxer.cc
index d641ded72c00e74a3fd48b9805c1e5f319cf9efa..f7102c710ce1f2eb75980ebf09107a0a7972c758 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -133,6 +133,14 @@ class SourceState {
   // ChunkDemuxerStreams managed by this object.
   void Remove(TimeDelta start, TimeDelta end, TimeDelta duration);
 
+  // Attempts to perform garbage collection in SourceBuffers associated with
+  // this ChunkDemuxer. Returns true if garbage collection was successful.
+  // |media_time| parameter should indicate the current playback position,

[wolenetz, 2015/06/04 00:38:56]

nit: s/position, if/position. If/

[servolk, 2015/06/04 03:03:01]

Done.

+  // if it's a valid time value, the garbage collection algorithm will make sure
+  // to preserve unconsumed data buffers in the range between last read position
+  // and the current playback position.
+  bool EvictFrames(DecodeTimestamp media_time);
+
   // Returns true if currently parsing a media segment, or false otherwise.
   bool parsing_media_segment() const { return parsing_media_segment_; }
 
@@ -374,6 +382,23 @@ void SourceState::Remove(TimeDelta start, TimeDelta end, TimeDelta duration) {
   }
 }
 
+bool SourceState::EvictFrames(DecodeTimestamp media_time) {
+  bool success = true;
+
+  if(audio_)

[wolenetz, 2015/06/04 00:38:56]

nit: spacing

[servolk, 2015/06/04 03:03:01]

Done.

+    success = audio_->EvictFrames(media_time) && success;
+
+  if (video_)
+    success = video_->EvictFrames(media_time) && success;
+
+  for (TextStreamMap::iterator itr = text_stream_map_.begin();
+       itr != text_stream_map_.end(); ++itr) {
+    success = itr->second->EvictFrames(media_time) && success;
+  }
+
+  return success;
+}
+
 Ranges<TimeDelta> SourceState::GetBufferedRanges(TimeDelta duration,
                                                  bool ended) const {
   // TODO(acolwell): When we start allowing disabled tracks we'll need to update
@@ -883,6 +908,11 @@ void ChunkDemuxerStream::Remove(TimeDelta start, TimeDelta end,
   stream_->Remove(start, end, duration);
 }
 
+bool ChunkDemuxerStream::EvictFrames(DecodeTimestamp media_time) {
+  base::AutoLock auto_lock(lock_);
+  return stream_->GarbageCollectIfNeeded(media_time);
+}
+
 void ChunkDemuxerStream::OnSetDuration(TimeDelta duration) {
   base::AutoLock auto_lock(lock_);
   stream_->OnSetDuration(duration);
@@ -1106,6 +1136,7 @@ ChunkDemuxer::ChunkDemuxer(
     const base::Closure& open_cb,
     const EncryptedMediaInitDataCB& encrypted_media_init_data_cb,
     const LogCB& log_cb,
+    const GetMediaTimeCB& get_media_time_cb,
     const scoped_refptr<MediaLog>& media_log,
     bool splice_frames_enabled)
     : state_(WAITING_FOR_INIT),
@@ -1115,6 +1146,7 @@ ChunkDemuxer::ChunkDemuxer(
       encrypted_media_init_data_cb_(encrypted_media_init_data_cb),
       enable_text_(false),
       log_cb_(log_cb),
+      get_media_time_cb_(get_media_time_cb),
       media_log_(media_log),
       duration_(kNoTimestamp()),
       user_specified_duration_(-1),
@@ -1317,6 +1349,23 @@ Ranges<TimeDelta> ChunkDemuxer::GetBufferedRanges(const std::string& id) const {
   return itr->second->GetBufferedRanges(duration_, state_ == ENDED);
 }
 
+bool ChunkDemuxer::EvictFrames() {
+  DecodeTimestamp current_media_time;
+  if (!get_media_time_cb_.is_null()) {
+    current_media_time =
+        DecodeTimestamp::FromPresentationTime(get_media_time_cb_.Run());

[wolenetz, 2015/06/04 00:38:57]

I'm not convinced the conversion from PTS to DTS is good for out-of-order
decodes and the scenario this CL is trying to fix. Are you also trying to fix
this scenario with out-of-order decode streams (like h264)?

[servolk, 2015/06/04 03:03:01]

The reason we are converting to DTS here is because buffered ranges are defined
in terms of DTS (SourceBufferRange::GetStartTimestamp/GetEndTimestamp return DTS
and most other processing in SBS seems to be in terms of DTS), but media
renderer returns current playback position as PTS. In order to figure out which
buffered range PTS belongs to, we need to compare it with range start/end DTS
values, so we need a DTS. I understand that PTS to DTS conversion doesn't make
actual adjustment for out-of-order decode streams, but I think in this case
that's fine, since we only need to figure out which buffered range playback
position belongs to. And as far as I understand single GOP always belongs
completely to some SourceBufferRange (i.e. a single GOP is never split between
two different SourceBufferRange objects). So I believe this should work
correctly both for regular in-order streams and out-of-order H.264 (or HEVC)
streams.

[wolenetz, 2015/06/04 20:06:50]

Ok. Thanks for considering this detail. As FYI, the incorrect usage of DTS
instead of PTS in older parts of Chromium MSE impl such as SourceBufferStream is
a known issue and tracked at least bug 373039 (plus bug 398130 and bug 402502).
Hence my original question since I didn't want this CL to regress behavior
further.

[servolk, 2015/06/04 20:22:46]

Acknowledged.

+  } else {
+    current_media_time = kNoDecodeTimestamp();
+  }
+
+  bool success = true;
+  for (SourceStateMap::iterator itr = source_state_map_.begin();
+       itr != source_state_map_.end(); ++itr) {
+    success = itr->second->EvictFrames(current_media_time) && success;
+  }
+  return success;
+}
+
 void ChunkDemuxer::AppendData(
     const std::string& id,
     const uint8* data,