RESOLVED FIXED 166620
[MSE][GStreamer] Avoid QUOTA_EXCEEDED_ERR when seeking to a buffered range just before the buffered one
https://bugs.webkit.org/show_bug.cgi?id=166620
Summary [MSE][GStreamer] Avoid QUOTA_EXCEEDED_ERR when seeking to a buffered range ju...
Enrique Ocaña
Reported 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.
Attachments
Patch (2.31 KB, patch)
2016-12-30 05:11 PST, Enrique Ocaña
no flags
Patch (17.37 KB, patch)
2019-06-06 10:18 PDT, Enrique Ocaña
no flags
Patch (17.46 KB, patch)
2019-06-07 02:54 PDT, Enrique Ocaña
no flags
Enrique Ocaña
Comment 1 2016-12-30 05:11:59 PST
Jer Noble
Comment 2 2017-02-03 08:10:02 PST
Comment on attachment 297856 [details] Patch No test?
Enrique Ocaña
Comment 3 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.
Andrew Gall
Comment 4 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.
Enrique Ocaña
Comment 5 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.
Andrew Gall
Comment 6 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.
Xabier Rodríguez Calvar
Comment 7 2019-03-12 06:53:03 PDT
Comment on attachment 297856 [details] Patch Jer is right, this needs a test.
Jer Noble
Comment 8 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().
Enrique Ocaña
Comment 9 2019-06-06 10:18:37 PDT
Enrique Ocaña
Comment 10 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.
Xabier Rodríguez Calvar
Comment 11 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"
Enrique Ocaña
Comment 12 2019-06-07 02:54:50 PDT
Enrique Ocaña
Comment 13 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.
WebKit Commit Bot
Comment 14 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>
WebKit Commit Bot
Comment 15 2019-06-07 03:54:54 PDT
All reviewed patches have been landed. Closing bug.
Radar WebKit Bug Importer
Comment 16 2019-06-07 03:55:19 PDT
Note You need to log in before you can comment on or make changes to this bug.