Bug 218228 – [MSE] Infinite loop in sample eviction when duration is NaN

• Component: Media
• Status: RESOLVED FIXED

[Philippe Normand]

When playing live streams the MediaSource DOM duration attribute has no meaning and would thus be set as +inf. When seeks are triggered to positions prior to the current playback position the SourceBuffer might attempt to free some space in order to keep the amount of memory used under control. It proceeds in 2 steps:
    
1. Attempt to free space represented by buffered range from media start up until current playback position - 30 seconds.
2. If step 1 didn't free enough memory, attempt to release memory represented by buffered ranges starting from current playback position + 30 seconds until media duration.
    
Step 2 here is not taking into account the case where MediaSource.duration is actually invalid, and thus enters an infinite loop.

[Philippe Normand]

Created attachment 412413 [details]
Patch

[Philippe Normand]

I'm not sure why the new test would trigger a new failure in another test.

[Philippe Normand]

Ha might be related with the QuotaExceeded support, I'll check it.

[Philippe Normand]

Created attachment 412436 [details]
Patch

[Xabier Rodríguez Calvar]

Comment on attachment 412436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412436&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
> +    if (!rangeEnd.hasDoubleValue()) {

I don't think this test is ok. In the case you are dealing with, I guess rangeEnd should be positiveInfinite so you should check for isPositiveInfinite. Bonus points to assert on validity, which I guess should be true.

[Philippe Normand]

Comment on attachment 412436 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412436&action=review

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
>> +    if (!rangeEnd.hasDoubleValue()) {
> 
> I don't think this test is ok. In the case you are dealing with, I guess rangeEnd should be positiveInfinite so you should check for isPositiveInfinite. Bonus points to assert on validity, which I guess should be true.

checking isPositiveInfinite && !rangeEnd.isIndefinite() makes sense. I'm not sure about adding an assert though, by default the duration is invalidTime(), how can we be sure that ever changes in this part of the code?

[Philippe Normand]

Created attachment 412531 [details]
Patch

[Philippe Normand]

Created attachment 412535 [details]
Patch

[Darin Adler]

Comment on attachment 412535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
> +    if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) {

Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log?

[Darin Adler]

Comment on attachment 412535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review

>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
>> +    if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) {
> 
> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log?

Does the test cover all three of these, and negative infinity too?

[Philippe Normand]

Comment on attachment 412535 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412535&action=review

>>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
>>> +    if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) {
>> 
>> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log?
> 
> Does the test cover all three of these, and negative infinity too?

NaN is what's mentioned in the spec: https://www.w3.org/TR/media-source/#dom-mediasource-duration 
So perhaps there's no need to check infinite cases here.

[Philippe Normand]

Created attachment 412646 [details]
Patch

[Jer Noble]

Comment on attachment 412646 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=412646&action=review

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1000
> +    if (rangeEnd.isInvalid() || rangeEnd.isIndefinite()) {
> +        rangeEnd = buffered.maximumBufferedTime();

I'm not sure this covers all the possible cases; it sounds like MediaTime needs an "isFinite()" that returns false for invalid, indefinite, and infinite values.  (The Cocoa ports, for example, treat "Indefinite" differently than "Positive Infinite")

[Jer Noble]

(In reply to Philippe Normand from comment #11)
> Comment on attachment 412535 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412535&action=review
> 
> >>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
> >>> +    if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) {
> >> 
> >> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log?
> > 
> > Does the test cover all three of these, and negative infinity too?
> 
> NaN is what's mentioned in the spec:
> https://www.w3.org/TR/media-source/#dom-mediasource-duration 
> So perhaps there's no need to check infinite cases here.

It's initially set to NaN, but it's also a read-write unrestricted double, so the page can set the duration to positive infinite.

[Philippe Normand]

(In reply to Jer Noble from comment #13)
> Comment on attachment 412646 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=412646&action=review
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:1000
> > +    if (rangeEnd.isInvalid() || rangeEnd.isIndefinite()) {
> > +        rangeEnd = buffered.maximumBufferedTime();
> 
> I'm not sure this covers all the possible cases; it sounds like MediaTime
> needs an "isFinite()" that returns false for invalid, indefinite, and
> infinite values.  (The Cocoa ports, for example, treat "Indefinite"
> differently than "Positive Infinite")

Sounds good. I guess you meant to r- instead of clearing the r flag :)

(In reply to Jer Noble from comment #14)
> (In reply to Philippe Normand from comment #11)
> > Comment on attachment 412535 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=412535&action=review
> > 
> > >>> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:999
> > >>> +    if (rangeEnd.isInvalid() || rangeEnd.isPositiveInfinite() || rangeEnd.isIndefinite()) {
> > >> 
> > >> Seems like the MediaTime class interface is really letting us down here with a confusing interface where each of these three conditions has to be checked separately. Are these the correct three things to check? Why do we need to check positive infinity, but not negative infinity? Why do we call this "NaN" in the log?
> > > 
> > > Does the test cover all three of these, and negative infinity too?
> > 
> > NaN is what's mentioned in the spec:
> > https://www.w3.org/TR/media-source/#dom-mediasource-duration 
> > So perhaps there's no need to check infinite cases here.
> 
> It's initially set to NaN, but it's also a read-write unrestricted double,
> so the page can set the duration to positive infinite.

So a test for Infinite time is needed too?

[Jer Noble]

(In reply to Philippe Normand from comment #15)
> 
> Sounds good. I guess you meant to r- instead of clearing the r flag :)

Whoops! I did. I initially had it set to r+ until I noticed the unrestricted double thing, and mistakenly set it to empty rather than back to r-. Apologies!

> So a test for Infinite time is needed too?

Yes, I think so. Either explicitly in a big if-or statement, or by adding an "isFinite()" method to MediaTime that just checks all the right flags.

[Philippe Normand]

Created attachment 412730 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/70989434>

[Philippe Normand]

Committed r270106: <https://trac.webkit.org/changeset/270106>