[MSE][GStreamer] Avoid QUOTA_EXCEEDED_ERR when seeking to a buffered range just before the buffered one

• Component: Media
• Status: RESOLVED FIXED

[Enrique Ocaña]

SourceBuffer::evictCodedFrames() sometimes finishes without evicting any buffered range in cases where eviction would actually be possible. This triggers a QUOTA_EXCEEDED_ERR on GStreamer platforms.

One of such cases is when seeking to an unbuffered range just before the buffered one. For example, if range [120, 300] was buffered, seek to 115 would fail. EvictCodedFrames() will try to evict ranges from the begining of the media timeline until "currentTime - 30 seconds". Then, only if there are buffered ranges *containing* the currentTime (except if it's the last range), will the algorithm try to evict future content from the end of the media timeline back to the current time until "currentTime + 30". However, as the seek target position isn't buffered in the example above, this second part of the algorithm will never run and the [120, 300] range will never be evicted.

A proposal to fix this would be to remove the "only if there are buffered ranges *containing* the currentTime" condition to enter into the second part of the algorithm.

[Enrique Ocaña]

Created attachment 297856 [details]
Patch

[Jer Noble]

Comment on attachment 297856 [details]
Patch

No test?

[Enrique Ocaña]

(In reply to comment #2)
> Comment on attachment 297856 [details]
> Patch
> 
> No test?

Good point. I'll try to manage to craft one.

[Andrew Gall]

It's a shame this fix was never merged. I see this bug still exists in the latest WebKit. In the coming weeks I will hopefully have time to submit a fix and test for this issue. Although if anyone knows a practical reason why a fix and test were never made I would greatly appreciated to know in advance.

[Enrique Ocaña]

(In reply to Andrew Gall from comment #4)
> It's a shame this fix was never merged. I see this bug still exists in the
> latest WebKit. In the coming weeks I will hopefully have time to submit a
> fix and test for this issue. Although if anyone knows a practical reason why
> a fix and test were never made I would greatly appreciated to know in
> advance.

It's been just lack of time and getting the task lost in my stack of tasks until I eventually forgot about it. I'm really sorry about it.

I usually work by analyzing logs of real use cases (eg: youtube.com/tv videos). Translating one of those real use cases to actual layout tests isn't trivial and I ended up procrastinating the task.

[Andrew Gall]

No worries, I understand. Thanks for the update. I'm unfamiliar with the WebKit test suite so I guess I'll see how I go creating a test when I have some time to do it.

[Xabier Rodríguez Calvar]

Comment on attachment 297856 [details]
Patch

Jer is right, this needs a test.

[Jer Noble]

Okay, so it looks like there's already a document().settings().maximumSourceBufferSize(), which can be set from Internals. And each MockMediaSample has a sizeInBytes() of the sizeof(MockSampleBox).  So it should be possible to write a layout test that sets the maximumSourceBufferSize() to some known value that's N * MockMediaSample sizeInBytes().

[Enrique Ocaña]

Created attachment 371506 [details]
Patch

[Enrique Ocaña]

Finally I gathered time to write the missing layout test. Thank you for insisting about writing one, because in the process I found that the original patch needed some more conditions to deal with the case when there's no currentTimeRange.

I noticed that in practice memory was exhausted earlier than what theory predicts when accumulating sizeInBytes() of the samples. In the end, I preferred not to go through the rabbit hole of finding where that discrepancy comes from, so I just chose some empirical size values for the number of samples I wanted to enqueue and stuck to what I wanted the test to prove.

[Xabier Rodríguez Calvar]

Comment on attachment 371506 [details]
Patch

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

> Source/WebCore/ChangeLog:16
> +        This patch is authored by iivlev <[e-mail]>

Given that in comment 10 you said that you had to change the conditions of the patch, I think the fair thing here would be to say that "This patch is based on one by..."

> LayoutTests/media/media-source/media-source-append-before-last-range-no-quota-exceeded.html:20
> +      for (var i = 0 ; i < timeRanges.length ; i++) {
> +       if (i)
> +        bufferedRanges += ', ';
> +       bufferedRanges += timeRanges.start(i) + '...' + timeRanges.end(i);
> +      }

This indentation here looks a bit "short"

[Enrique Ocaña]

Created attachment 371579 [details]
Patch

[Enrique Ocaña]

(In reply to Xabier Rodríguez Calvar from comment #11)
> Comment on attachment 371506 [details]

> Given that in comment 10 you said that you had to change the conditions of
> the patch, I think the fair thing here would be to say that "This patch is
> based on one by..."

Done.

> This indentation here looks a bit "short"

Thanks for noticing this. The whole indentation was inconsistent. I normalized it to 4 spaces.

[WebKit Commit Bot]

Comment on attachment 371579 [details]
Patch

Clearing flags on attachment: 371579

Committed r246195: <https://trac.webkit.org/changeset/246195>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51517397>