Fix MSE garbage collection for disabled media tracks

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 9fb533e2efd2bca1659048d52bbdb5ee147e5765..93a001fbed4ccf8f0ddc116f60d873e3ed5e9305 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -119,10 +119,25 @@ void ChunkDemuxerStream::Remove(TimeDelta start, TimeDelta end,
   stream_->Remove(start, end, duration);
 }
 
-bool ChunkDemuxerStream::EvictCodedFrames(DecodeTimestamp media_time,
+bool ChunkDemuxerStream::EvictCodedFrames(base::TimeDelta media_time,
                                           size_t newDataSize) {
   base::AutoLock auto_lock(lock_);
-  return stream_->GarbageCollectIfNeeded(media_time, newDataSize);
+
+  // If the stream is disabled, then the renderer is not reading from it and
+  // thus the read position might be stale. MSE GC algorithm uses the read
+  // position to determine when to stop removing data from the front of buffered
+  // ranges, so do a Seek in order to update the read position and allow the GC
+  // to collect unnecessary data that is earlier than the GOP containing
+  // |media_time|.
+  if (!is_enabled_)
+    stream_->Seek(media_time);

[wolenetz, 2017/04/20 18:30:18]

Interesting. I think this will work. The comment does expose a bit of the
internals of SourceBufferStream's GC implementation, though I'm unclear of a
better way, except perhaps telling the stream itself if it is enabled or
disabled and letting the GC heuristic internals in SourceBufferStream handle the
rest internally. WDYT?

[wolenetz, 2017/04/20 19:01:09]

Hmm. Actually I like the way you did this better, since the consequence of not
explicitly seeking the stream here might be harder to understand from
ChunkDemuxerStream perspective:

Since the SourceBufferStream needs to seek away from the "current read position"
if that position is stale, to avoid unnecessary retaining of the stale "current
GOP", it's better to explicitly expose the Seek intent here rather than suffer
side-effects of hidden seeks that could risk maintainability worse than this
comment's minor layer violation.

So, LGTM!

Thanks :)

[servolk, 2017/04/20 20:12:32]

Thanks for diving deep into this with me, Matt :) I have considered other
options yesterday, but eventually came to the same conclusion as you - calling
Seek here seems to be the cleanest way to ensure all the corner cases are
properly handled. Glad we agree on that!

+
+  // Note: The direct conversion from PTS to DTS is safe here, since we don't
+  // need to know currentTime precisely for GC. GC only needs to know which GOP
+  // currentTime points to.
+  DecodeTimestamp media_time_dts =
+      DecodeTimestamp::FromPresentationTime(media_time);
+  return stream_->GarbageCollectIfNeeded(media_time_dts, newDataSize);
 }
 
 void ChunkDemuxerStream::OnMemoryPressure(
@@ -786,19 +801,13 @@ bool ChunkDemuxer::EvictCodedFrames(const std::string& id,
            << " newDataSize=" << newDataSize;
   base::AutoLock auto_lock(lock_);
 
-  // Note: The direct conversion from PTS to DTS is safe here, since we don't
-  // need to know currentTime precisely for GC. GC only needs to know which GOP
-  // currentTime points to.
-  DecodeTimestamp media_time_dts =
-      DecodeTimestamp::FromPresentationTime(currentMediaTime);
-
   DCHECK(!id.empty());
   auto itr = source_state_map_.find(id);
   if (itr == source_state_map_.end()) {
     LOG(WARNING) << __func__ << " stream " << id << " not found";
     return false;
   }
-  return itr->second->EvictCodedFrames(media_time_dts, newDataSize);
+  return itr->second->EvictCodedFrames(currentMediaTime, newDataSize);
 }
 
 bool ChunkDemuxer::AppendData(const std::string& id,