Fix MSE garbage collection for disabled media tracks

media/filters/chunk_demuxer.cc

 // Copyright (c) 2012 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/chunk_demuxer.h"
 
 #include <algorithm>
 #include <limits>
 #include <utility>
 
@@ skipping 101 matching lines @@
 
   return true;
 }
 
 void ChunkDemuxerStream::Remove(TimeDelta start, TimeDelta end,
                                 TimeDelta duration) {
   base::AutoLock auto_lock(lock_);
   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(
     DecodeTimestamp media_time,
     base::MemoryPressureListener::MemoryPressureLevel memory_pressure_level,
     bool force_instant_gc) {
   base::AutoLock auto_lock(lock_);
   return stream_->OnMemoryPressure(media_time, memory_pressure_level,
                                    force_instant_gc);
 }
@@ skipping 643 matching lines @@
 }
 
 bool ChunkDemuxer::EvictCodedFrames(const std::string& id,
                                     base::TimeDelta currentMediaTime,
                                     size_t newDataSize) {
   DVLOG(1) << __func__ << "(" << id << ")"
            << " media_time=" << currentMediaTime.InSecondsF()
            << " 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,
                               const uint8_t* data,
                               size_t length,
                               TimeDelta append_window_start,
                               TimeDelta append_window_end,
                               TimeDelta* timestamp_offset) {
   DVLOG(1) << "AppendData(" << id << ", " << length << ")";
 
@@ skipping 538 matching lines @@
 }
 
 void ChunkDemuxer::ShutdownAllStreams() {
   for (auto itr = source_state_map_.begin(); itr != source_state_map_.end();
        ++itr) {
     itr->second->Shutdown();
   }
 }
 
 }  // namespace media