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.
Created attachment 297856 [details] Patch
Comment on attachment 297856 [details] Patch No test?
(In reply to comment #2) > Comment on attachment 297856 [details] > Patch > > No test? Good point. I'll try to manage to craft one.
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.
(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.
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 on attachment 297856 [details] Patch Jer is right, this needs a test.
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().
Created attachment 371506 [details] Patch
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 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"
Created attachment 371579 [details] Patch
(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 on attachment 371579 [details] Patch Clearing flags on attachment: 371579 Committed r246195: <https://trac.webkit.org/changeset/246195>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51517397>