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 51511c8f69ad9dff37773d5d147c54b993211786..2022b46871285e5664a79849081ce00ddbcfa378 100644
--- a/media/filters/chunk_demuxer.cc
+++ b/media/filters/chunk_demuxer.cc
@@ -131,6 +131,12 @@ class SourceState {
   // ChunkDemuxerStreams managed by this object.
   void Remove(TimeDelta start, TimeDelta end, TimeDelta duration);
 
+  // If the buffer is full, attempts to try to free up space, as specified in
+  // the "Coded Frame Eviction Algorithm" in the Media Source Extensions Spec.
+  // Returns false iff buffer is still full after running eviction.
+  // https://w3c.github.io/media-source/#sourcebuffer-coded-frame-eviction
+  bool EvictCodedFrames(DecodeTimestamp media_time, size_t newDataSize);
+
   // Returns true if currently parsing a media segment, or false otherwise.
   bool parsing_media_segment() const { return parsing_media_segment_; }
 
@@ -369,6 +375,36 @@ void SourceState::Remove(TimeDelta start, TimeDelta end, TimeDelta duration) {
   }
 }
 
+bool SourceState::EvictCodedFrames(DecodeTimestamp media_time,

[wolenetz, 2015/08/20 23:23:37]

Please extract the heuristic for determining newAudioSize and newVideoSize into
a method, and add unit tests for that method.

[servolk, 2015/08/21 01:32:08]

I've extracted the heuristic into a new method in SourceState class. But that's
an internal helper class for the ChunkDemuxer and currently there are no direct
tests for it. So for now I've added a new ChunkDemuxer test for
EvictCodedFrames, that uses muxed data and implicitly tests this function as
well.

+                                   size_t newDataSize) {
+  bool success = true;
+
+  size_t newAudioSize = 0;
+  size_t newVideoSize = 0;
+  size_t videoBufferedSize = video_ ? video_->GetBufferedSize() : 0;
+  if (audio_ && video_ && videoBufferedSize > 0) {

[wolenetz, 2015/08/20 23:23:37]

nit: please add a comment describing this heuristic, perhaps mentioning that
other heuristics that incrementally build audio/video/text removal ranges that
are aligned in time could be a better solution, since muxed contents are muxed
in interleaved DTS sequence across the streams.

[servolk, 2015/08/21 01:32:08]

Done.

+    newVideoSize = newDataSize * audio_->GetBufferedSize() / videoBufferedSize;

[wolenetz, 2015/08/20 23:23:37]

This seems incorrect.
[estimated headroom for new video] should be = [total new media size] *
[proportion of video of all buffered size]

example:
100MB total currently buffered, 80MB video + 20MB audio
total new media size = 10MB
estimated new video size = 10MB * (80MB / 100MB) = 8MB
estimated new audio size = 10MB * (20MB / 100MB) = 2MB

However, current patch set would instead calculate:
estimated new video size = 10MB * (20MB / 80MB) = 2.5MB
estimated new audio size = 10MB - 2.5MB = 7.5MB

[servolk, 2015/08/21 01:32:08]

Oh, yes, you are right, not sure what I was thinking, I've changed the formula
to newDataSize * V / (A+V) now.

+    newAudioSize = newDataSize - newVideoSize;
+  } else if (video_) {
+    newVideoSize = newDataSize;
+  } else if (audio_) {
+    newAudioSize = newDataSize;
+  }
+

[wolenetz, 2015/08/20 23:23:37]

Please add some DVLOGs describing newDataSize, newAudioSize, newVideoSize, and
before and after video_,audio_ ->GetBufferedSize()

[servolk, 2015/08/21 01:32:08]

Done.

+  if (audio_)
+    success = audio_->EvictCodedFrames(media_time, newAudioSize) && success;
+
+  if (video_)
+    success = video_->EvictCodedFrames(media_time, newVideoSize) && success;
+
+  for (TextStreamMap::iterator itr = text_stream_map_.begin();
+       itr != text_stream_map_.end(); ++itr) {
+    success = itr->second->EvictCodedFrames(media_time, 0) && 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
@@ -879,6 +915,12 @@ void ChunkDemuxerStream::Remove(TimeDelta start, TimeDelta end,
   stream_->Remove(start, end, duration);
 }
 
+bool ChunkDemuxerStream::EvictCodedFrames(DecodeTimestamp media_time,
+                                          size_t newDataSize) {
+  base::AutoLock auto_lock(lock_);
+  return stream_->GarbageCollectIfNeeded(media_time, newDataSize);
+}
+
 void ChunkDemuxerStream::OnSetDuration(TimeDelta duration) {
   base::AutoLock auto_lock(lock_);
   stream_->OnSetDuration(duration);
@@ -915,6 +957,10 @@ TimeDelta ChunkDemuxerStream::GetBufferedDuration() const {
   return stream_->GetBufferedDuration();
 }
 
+size_t ChunkDemuxerStream::GetBufferedSize() const {
+  return stream_->GetBufferedSize();
+}
+
 void ChunkDemuxerStream::OnNewMediaSegment(DecodeTimestamp start_timestamp) {
   DVLOG(2) << "ChunkDemuxerStream::OnNewMediaSegment("
            << start_timestamp.InSecondsF() << ")";
@@ -1321,6 +1367,29 @@ Ranges<TimeDelta> ChunkDemuxer::GetBufferedRanges(const std::string& id) const {
   return itr->second->GetBufferedRanges(duration_, state_ == ENDED);
 }
 
+bool ChunkDemuxer::EvictCodedFrames(const std::string& id,
+                                    base::TimeDelta currentMediaTime,
+                                    size_t newDataSize) {
+  DVLOG(1) << __FUNCTION__ << "(" << 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());
+  SourceStateMap::const_iterator itr = source_state_map_.find(id);
+  if (itr == source_state_map_.end()) {
+    LOG(WARNING) << __FUNCTION__ << " stream " << id << " not found";
+    return false;
+  }
+  return itr->second->EvictCodedFrames(media_time_dts, newDataSize);
+}
+
 void ChunkDemuxer::AppendData(
     const std::string& id,
     const uint8* data,