Bug 225800 – [MSE] MediaSample that need to be removed with SourceBufferPrivate::evictCodedFrames() may not be removed.

• Component: Media
• Status: RESOLVED FIXED

[Toshio Ogasawara]

Repeating appendBuffer and seek may result in a buffer full.
When buffer full occurs in appnedBuffer, MediaSample after the CurrnetTime + 30s needs to be removed by SourceBufferPrivate::evictCodedFrames(), but it may not be removed depending on the buffered status.

[Toshio Ogasawara]

Created attachment 428607 [details]
patch

This patch removes MediaSample after CurrentTime + 30s that need to be removed with SourceBufferPrivate::evictCodedFrames().

[Jer Noble]

Comment on attachment 428607 [details]
patch

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

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:703
> -    while (rangeStart > minimumRangeStart) {
> +    while (rangeEnd > minimumRangeStart) {

Comparing `rangeEnd` to `mininimumRangeStart` feels wrong here.  Perhaps we just need to restructure this code to correctly set `minimumRangeStart` to the beginning of the next buffered range, if that time is > `minimumRangeStart`.  Then this while loop can look identical to the while loop above.

[Toshio Ogasawara]

>Comparing `rangeEnd` to `mininimumRangeStart` feels wrong here. 

I think it is correct to compare with rangeEnd.

At the end of this while loop, there is a process to subtract thirtySeconds from rangeStart and rangeEnd.
If this process is repeated, rangeStart may become less than minimumRangeStart before rangeEnd.
If rangeStart is less than minimumRangeStart and rangeEnd is greater than minimumRangeStart, this while loop will not be processed.
Therefore, removable MediaSamples in the range from minimumRangeStart to rangeEnd are not removed.

[Toshio Ogasawara]

>Then this while loop can look identical to the while loop above.

In the first while loop of evictCodedFrame, there is a process to add thirtySeconds to rangeStart and rangeEnd.
Even if this process is repeated, rangeStart will not be larger than maximumRangeEnd before rangeEnd.
Therefore, the removable MediaSamples in the range from rangeStart to maximumRangeEnd is removed.

I think it is correct to compare the end of the range(rangeEnd) when processing in descending order, and the start of the range(startRange) when processing in ascending order.

[Radar WebKit Bug Importer]

<rdar://problem/78296352>

[Jer Noble]

Comment on attachment 428607 [details]
patch

> >Comparing `rangeEnd` to `mininimumRangeStart` feels wrong here. 
> 
> I think it is correct to compare with rangeEnd.

No, it's definitely not. You can see why it's wrong here:

-            rangeEnd = buffered.start(endTimeRange);
+            if (endTimeRange == notFound)
+                rangeStart = buffered.end(startTimeRange);
+            else
+                rangeEnd = buffered.start(endTimeRange);

If that comparison was correct, this change would be unnecessary. This just adds a complicated calculation to an already overcomplicated while loop.

The correct fix would look more like this:

@@ -689,7 +689,7 @@ void SourceBufferPrivate::evictCodedFrames(uint64_t newDataSize, uint64_t pendin
         return;
     }
 
-    MediaTime minimumRangeStart = currentTime + thirtySeconds;
+    MediaTime minimumRangeStart = std::min(currentTime + thirtySeconds, buffered.end(currentTimeRange));
 
     rangeEnd = duration;
     if (!rangeEnd.isFinite()) {

This sets up minimumRangeStart correctly to be located at the end of the current range, or 30s after currentTime, whichever is lower.

@@ -701,16 +701,6 @@ void SourceBufferPrivate::evictCodedFrames(uint64_t newDataSize, uint64_t pendin
 
     rangeStart = rangeEnd - thirtySeconds;
     while (rangeStart > minimumRangeStart) {
-        // Do not evict data from the time range that contains currentTime.
-        uint64_t startTimeRange = buffered.find(rangeStart);
-        if (currentTimeRange != notFound && startTimeRange == currentTimeRange) {
-            uint64_t endTimeRange = buffered.find(rangeEnd);
-            if (currentTimeRange != notFound && endTimeRange == currentTimeRange)
-                break;
-
-            rangeEnd = buffered.start(endTimeRange);
-        }
-

And now we don't need to do a complicated recalculation at every step through the loop. It just walks back in 30s increments until it reaches minimumRangeStart.

[Jer Noble]

(In reply to Jer Noble from comment #6)
> -    MediaTime minimumRangeStart = currentTime + thirtySeconds;
> +    MediaTime minimumRangeStart = std::min(currentTime + thirtySeconds,
> buffered.end(currentTimeRange));

Whoops, this should be max, not min.

(In reply to Toshio Ogasawara from comment #4)
> I think it is correct to compare the end of the range(rangeEnd) when
> processing in descending order, and the start of the range(startRange) when
> processing in ascending order.

I take back my earlier comment, you're right about this comparison, because rangeStart isn't actually used inside the loop without doing `max(minimumRangeStart, rangeStart)` first.

Still, the entire first section of the while() loop can be removed to solve this problem, rather than adding more calculations.

[Toshio Ogasawara]

(In reply to Jer Noble from comment #7)
> (In reply to Jer Noble from comment #6)
> > -    MediaTime minimumRangeStart = currentTime + thirtySeconds;
> > +    MediaTime minimumRangeStart = std::min(currentTime + thirtySeconds,
> > buffered.end(currentTimeRange));
> 
> Whoops, this should be max, not min.
> 
> (In reply to Toshio Ogasawara from comment #4)
> > I think it is correct to compare the end of the range(rangeEnd) when
> > processing in descending order, and the start of the range(startRange) when
> > processing in ascending order.
> 
> I take back my earlier comment, you're right about this comparison, because
> rangeStart isn't actually used inside the loop without doing
> `max(minimumRangeStart, rangeStart)` first.
> 
> Still, the entire first section of the while() loop can be removed to solve
> this problem, rather than adding more calculations.

I was able to confirm with your fix that minimumRangeStart is set to be correctly placed at the end of the current range, or 30 seconds after the currentTime, whichever is higher.
Now we don't need to do complicated recalculation.
Thank you.


> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:703
> -    while (rangeStart > minimumRangeStart) {
> +    while (rangeEnd > minimumRangeStart) {
I will add this fix to take into account the following conditions.

When "rangeStart < minimumRangeStart, rangeEnd > minimumRangeStart", remove between minimumRangeStart and rangeEnd.

[Toshio Ogasawara]

Created attachment 429743 [details]
patch

Fixed patch.

[Jer Noble]

Comment on attachment 429743 [details]
patch

r=me. Thanks for continuing to work on this with me!

[Toshio Ogasawara]

(In reply to Jer Noble from comment #10)
> Comment on attachment 429743 [details]
> patch
> 
> r=me. Thanks for continuing to work on this with me!

Thank you for the review.

[EWS]

Committed r278147 (238191@main): <https://commits.webkit.org/238191@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 429743 [details].