Fix MSE GC, make it less aggressive, more spec-compliant (Blink CL)

public/platform/WebSourceBuffer.h

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 35 matching lines @@
 
     virtual ~WebSourceBuffer() { }
 
     // This will only be called once and only with a non-null pointer to a
     // client whose ownership is not transferred to this WebSourceBuffer.
     virtual void setClient(WebSourceBufferClient*) = 0;
 
     virtual bool setMode(AppendMode) = 0;
     virtual WebTimeRanges buffered() = 0;
 
+    // Run coded frame eviction/garbage collection algorithm.
+    // |currentPlaybackTime| is HTMLMediaElement::currentTime. The algorithm

[philipj_slow, 2015/07/02 09:48:22]

Playback position in HTMLMediaElement is unfortunately a bit of a mess. Can you
check which of these two concepts actually makes sense in this context?
https://html.spec.whatwg.org/#current-playback-position
https://html.spec.whatwg.org/#official-playback-position

We don't have them accessible by their per-spec names in Blink, but if it's the
real playback position that's needed here and not one that compensates for
seeking, then HTMLMediaElement::currentTime() isn't the right thing to use,
instead one has to go to WebMediaPlayer::currentTime() directly.

I'd also name the variable to match, probably currentPlaybackPosition(). If you
want to expose HTMLMediaElement::currentPlaybackPosition() in the process,
that'd be nice.

[servolk, 2015/07/07 23:01:52]

Actually we do want the position to reflect the pending seek position as soon as
seeking starts, so I believe HTMLMediaElement::currentTime is the best choice
here. Think about it - we are going to use this value to decide which data to
keep during MSE GC, and if a seek operation has been started, then we should try
to keep data appended at the new playback position/seek target, rather than the
old playback position. This should fix crbug.com/440173 (see the comments
there).

[philipj_slow, 2015/07/08 12:47:44]

OK, so it sounds like the MSE spec actually says the wrong thing here? If you
agree, can you file a spec bug to use "official playback position" instead of
"current playback position"?

[servolk, 2015/07/08 19:03:47]

Well, AFAIK MSE spec actually does not explicitly describe the garbage
collection algorithm and leaves it up to implementation, so it also doesn't
require that some specific 'current playback position' or 'official playback
position' should be used here. The 'current playback position' is mentioned
multiple times in MSE spec section 2.4.4
https://w3c.github.io/media-source/#buffer-monitoring, and we want our GC
algorithm to preserve data around the 'current playback position', instead of
internal demuxer read position. So I don't think there's any problems with MSE
spec.
Btw, as far as I can see the definitions of 'official' and 'current' positions
in your links don't explicitly specify that the two are different during
seeking. The definitions just say that the 'current' position is some position
on the media timeline and the 'official' one is an approximation of 'current'
which "is kept stable while scripts are running". But I'm not sure what does
'kept stable' means in this context. Does that mean that the 'official' position
is kept at the old playback time until pending seek completes? That's not
mentioned in the definition.

[philipj_slow, 2015/07/09 09:49:23]

One instance of "current playback position" is linked to
http://www.w3.org/TR/html5/embedded-content-0.html#current-playback-position,
which is the same as the WHATWG spec in this case.

The definition of "official playback position" just says that it's initially
zero, there are other parts of the spec where it's set to various values,
notably "Any time the user agent provides a stable state, the official playback
position must be set to the current playback position."

However, you'll notice some discrepancies around seeking when comparing what the
spec says and how it's implemented. Per the spec, setting currentTime doesn't
synchronously set it to the new value, the seeking algorithm returns early and
only later would make the change visible in currentTime. Blink, however, has an
if (m_seeking) return m_lastSeekTime bit in the currentTime setter. So it's a
bit of a mess, and I've filed a spec bug:
https://www.w3.org/Bugs/Public/show_bug.cgi?id=28929

In the meantime, I guess we just need to decide how this should work, implement
that with a sprinkling of TODOs, and keep track of the spec bugs for the
mismatch. Out of "current playback position", "official playback position",
"default playback start position", a non-spec'd "seek position" and the
combinations of those that is currentTime, what's the right time to use?

[servolk, 2015/07/10 01:11:27]

I believe HTMLMediaElement::currentTime is exactly what we want. It is current
playback position for normal playback case, but it changes to the new seek
position as soon as seeking starts and that's what we want to protect during GC
(evictCodedFrames). After all this is sorted out, will there be some property on
HTMLMediaElement with the same semantics as the current implementation of
currentTime?

[philipj_slow, 2015/07/10 08:40:27]

If there isn't an internal concept for it, then simply having the MSE spec be
defined in terms of currentTime would also work.

wolenetz@, what do you think, and should I file an MSE spec bug to sort this
out?

[wolenetz, 2015/08/12 22:42:03]

Hmm. We need the current playback position, and if there is a seek, it needs to
be the seek position as our implementation currently synchronously updates it
(and as a reply on the WHATWG spec bug mentioned, seems sane.) However, as
servolk@ mentioned, the MSE spec leaves selection of ranges for removal as part
of GC up to implementations, and doesn't mention anything about
current/official/seek playback position.

In many other places in the MSE spec, we refer to current playback position
(e.g. SourceBuffer monitoring). The intent to me appears to be similar: it
should be like our current Blink HTMLMediaElement::currentTIme, which is updated
synchronously on seek start. Since MSE extends the HTMLMediaElement, getting the
latter's spec updated seems to me to be the right thing (especially since
implementations already synchronously update currentTime). I'm less sure there
needs to be any mention of this in the MSE spec.

+    // will try to preserve data around current playback position.
+    // Returns false if buffer is still full after eviction.
+    virtual bool evictCodedFrames(double currentPlaybackTime) = 0;
+
     // Appends data and runs the segment parser loop algorithm.
     // The algorithm may update |*timestampOffset| if |timestampOffset| is not null.
     virtual void append(const unsigned char* data, unsigned length, double* timestampOffset) = 0;
 
     virtual void abort() = 0;
     virtual void remove(double start, double end) = 0;
     virtual bool setTimestampOffset(double) = 0;
 
     // Set presentation timestamp for the start of append window.
     virtual void setAppendWindowStart(double) = 0;
 
     // Set presentation timestamp for the end of append window.
     virtual void setAppendWindowEnd(double) = 0;
 
     // After this method is called, this WebSourceBuffer should never use the
     // client pointer passed to setClient().
     virtual void removedFromMediaSource() = 0;
 };
 
 } // namespace blink
 
 #endif