Bug 166620 - [MSE][GStreamer] Avoid QUOTA_EXCEEDED_ERR when seeking to a buffered range just before the buffered one
Summary: [MSE][GStreamer] Avoid QUOTA_EXCEEDED_ERR when seeking to a buffered range ju...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Enrique Ocaña
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-12-30 05:06 PST by Enrique Ocaña
Modified: 2019-06-07 03:55 PDT (History)
8 users (show)

See Also:


Attachments
Patch (2.31 KB, patch)
2016-12-30 05:11 PST, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (17.37 KB, patch)
2019-06-06 10:18 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff
Patch (17.46 KB, patch)
2019-06-07 02:54 PDT, Enrique Ocaña
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Enrique Ocaña 2016-12-30 05:06:06 PST
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.
Comment 1 Enrique Ocaña 2016-12-30 05:11:59 PST
Created attachment 297856 [details]
Patch
Comment 2 Jer Noble 2017-02-03 08:10:02 PST
Comment on attachment 297856 [details]
Patch

No test?
Comment 3 Enrique Ocaña 2017-02-03 08:28:25 PST
(In reply to comment #2)
> Comment on attachment 297856 [details]
> Patch
> 
> No test?

Good point. I'll try to manage to craft one.
Comment 4 Andrew Gall 2019-03-12 05:29:08 PDT
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.
Comment 5 Enrique Ocaña 2019-03-12 06:10:40 PDT
(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.
Comment 6 Andrew Gall 2019-03-12 06:38:56 PDT
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.
Comment 7 Xabier Rodríguez Calvar 2019-03-12 06:53:03 PDT
Comment on attachment 297856 [details]
Patch

Jer is right, this needs a test.
Comment 8 Jer Noble 2019-03-12 07:49:35 PDT
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().
Comment 9 Enrique Ocaña 2019-06-06 10:18:37 PDT
Created attachment 371506 [details]
Patch
Comment 10 Enrique Ocaña 2019-06-06 10:31:29 PDT
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.
Comment 11 Xabier Rodríguez Calvar 2019-06-07 00:10:56 PDT
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 <iivlev@productengine.com>

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"
Comment 12 Enrique Ocaña 2019-06-07 02:54:50 PDT
Created attachment 371579 [details]
Patch
Comment 13 Enrique Ocaña 2019-06-07 02:57:21 PDT
(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.
Comment 14 WebKit Commit Bot 2019-06-07 03:54:52 PDT
Comment on attachment 371579 [details]
Patch

Clearing flags on attachment: 371579

Committed r246195: <https://trac.webkit.org/changeset/246195>
Comment 15 WebKit Commit Bot 2019-06-07 03:54:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Radar WebKit Bug Importer 2019-06-07 03:55:19 PDT
<rdar://problem/51517397>