Bug 218228

Summary: [MSE] Infinite loop in sample eviction when duration is NaN
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: MediaAssignee: Philippe Normand <pnormand>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, calvaris, cdumez, cmarcelo, darin, eocanha, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=218381
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch darin: review+

Description Philippe Normand 2020-10-27 05:20:22 PDT
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.
Comment 1 Philippe Normand 2020-10-27 05:25:20 PDT
Created attachment 412413 [details]
Patch
Comment 2 Philippe Normand 2020-10-27 09:33:29 PDT
I'm not sure why the new test would trigger a new failure in another test.
Comment 3 Philippe Normand 2020-10-27 09:34:27 PDT
Ha might be related with the QuotaExceeded support, I'll check it.
Comment 4 Philippe Normand 2020-10-27 10:02:11 PDT
Created attachment 412436 [details]
Patch
Comment 5 Xabier Rodríguez Calvar 2020-10-28 05:17:38 PDT
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.
Comment 6 Philippe Normand 2020-10-28 08:47:51 PDT
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?
Comment 7 Philippe Normand 2020-10-28 09:10:39 PDT
Created attachment 412531 [details]
Patch
Comment 8 Philippe Normand 2020-10-28 09:27:42 PDT
Created attachment 412535 [details]
Patch
Comment 9 Darin Adler 2020-10-28 12:45:48 PDT
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?
Comment 10 Darin Adler 2020-10-28 12:46:16 PDT
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?
Comment 11 Philippe Normand 2020-10-29 07:35:02 PDT
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.
Comment 12 Philippe Normand 2020-10-29 07:47:03 PDT
Created attachment 412646 [details]
Patch
Comment 13 Jer Noble 2020-10-29 09:28:27 PDT
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")
Comment 14 Jer Noble 2020-10-29 09:31:55 PDT
(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.
Comment 15 Philippe Normand 2020-10-29 10:00:24 PDT
(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?
Comment 16 Jer Noble 2020-10-29 11:04:13 PDT
(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.
Comment 17 Philippe Normand 2020-10-30 04:37:42 PDT
Created attachment 412730 [details]
Patch
Comment 18 Radar WebKit Bug Importer 2020-11-03 04:21:20 PST
<rdar://problem/70989434>
Comment 19 Philippe Normand 2020-11-20 04:20:02 PST
Committed r270106: <https://trac.webkit.org/changeset/270106>