Bug 225630 – [MSE] QuotaExceededError Exception not thrown even if the sum of totalTrackBufferSize and appendBuffer size exceeds maximumBufferSize.

• Component: Media
• Status: RESOLVED FIXED

[Toshio Ogasawara]

When appendBuffer is executed, the exception of QuotaExceededError is not thrown even if the sum of totalTrackBufferSize and appendBuffer size exceeds maximumBufferSize.
It is also possible to send large size data exceeding the maximumBufferSize to the platform layer.
When appendBuffer is called, if it exceeds maximumBufferSize, it is better to throw QuotaExceededError exception.

[Toshio Ogasawara]

Created attachment 428237 [details]
patch

[EWS]

Committed r277345 (237603@main): <https://commits.webkit.org/237603@main>

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

[Radar WebKit Bug Importer]

<rdar://problem/77872373>

[Peng Liu]

Comment on attachment 428237 [details]
patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:494
> +    if (m_private->bufferFull() || m_private->totalTrackBufferSizeInBytes() + m_pendingAppendData.capacity() + size >= maximumBufferSize()) {

Looks like this won't work when "Media in GPU Process" is enabled. `SourceBufferPrivateRemote` does not provide a correct implementation of totalTrackBufferSizeInBytes().

[Jean-Yves Avenard [:jya]]

Also this doesn't seem to take into account that amount of data that can be evicted if any. So it will always read low.

[Peng Liu]

(In reply to Peng Liu from comment #4)
> Comment on attachment 428237 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=428237&action=review
> 
> > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:494
> > +    if (m_private->bufferFull() || m_private->totalTrackBufferSizeInBytes() + m_pendingAppendData.capacity() + size >= maximumBufferSize()) {
> 
> Looks like this won't work when "Media in GPU Process" is enabled.
> `SourceBufferPrivateRemote` does not provide a correct implementation of
> totalTrackBufferSizeInBytes().

Filed bug 226034  to track this.

[Toshio Ogasawara]

(In reply to Peng Liu from comment #6)
> (In reply to Peng Liu from comment #4)
> > Comment on attachment 428237 [details]
> > patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=428237&action=review
> > 
> > > Source/WebCore/Modules/mediasource/SourceBuffer.cpp:494
> > > +    if (m_private->bufferFull() || m_private->totalTrackBufferSizeInBytes() + m_pendingAppendData.capacity() + size >= maximumBufferSize()) {
> > 
> > Looks like this won't work when "Media in GPU Process" is enabled.
> > `SourceBufferPrivateRemote` does not provide a correct implementation of
> > totalTrackBufferSizeInBytes().
> 
> Filed bug 226034  to track this.

I understand.

[Jean-Yves Avenard [:jya]]

(In reply to Toshio Ogasawara from comment #0)
> When appendBuffer is executed, the exception of QuotaExceededError is not
> thrown even if the sum of totalTrackBufferSize and appendBuffer size exceeds
> maximumBufferSize.
> It is also possible to send large size data exceeding the maximumBufferSize
> to the platform layer.
> When appendBuffer is called, if it exceeds maximumBufferSize, it is better
> to throw QuotaExceededError exception.

On further thought, this assessment was incorrect.

If the size of the source buffer + pending data capacity + new size is greater than the maximum buffer size; then SourceBufferPrivate::bufferFull() would return true and the QuotaExceededError would have been thrown.

It sounds like in your case, you have a SourceBufferPrivate::evictCodedFrames that doesn't properly update the value returned by SourceBufferPrivate::bufferFull. This is where the fix is needed if any.

[Jean-Yves Avenard [:jya]]

To add.

The issue here is a spec issue.

We are to reject a buffer only once we know that the source buffer is full.
The first appendBuffer should always complete.

https://w3c.github.io/media-source/#sourcebuffer-buffer-full-flag
"The buffer full flag keeps track of whether appendBuffer() is allowed to accept more bytes. It is set to false when the SourceBuffer object is created and gets updated as data is appended and removed."

"buffer full flag" only gets modified to true in the 3.5.1 Segment Parser Loop algorithm, step 6.3
https://w3c.github.io/media-source/#sourcebuffer-segment-parser-loop
"If this SourceBuffer is full and cannot accept more media data, then set the buffer full flag to true."

On the 2nd call to the appendBuffer, in the Prepare Append algorithm, step 3.5.4.6:
https://w3c.github.io/media-source/#sourcebuffer-prepare-append

"If the buffer full flag equals true, then throw a QuotaExceededError exception and abort these steps."

This change as such, made the WebKit implementation non-compliant. 

However, I can understand why this change would be desired.

I will have a follow-up bug to rework how we handle source buffer size and checking capacity. It will make it easier to toggle between the two behaviours if needed (one before and after this fix)

[Toshio Ogasawara]

(In reply to Jean-Yves Avenard [:jya] from comment #9)
> To add.
> 
> The issue here is a spec issue.
> 
> We are to reject a buffer only once we know that the source buffer is full.
> The first appendBuffer should always complete.
> 
> https://w3c.github.io/media-source/#sourcebuffer-buffer-full-flag
> "The buffer full flag keeps track of whether appendBuffer() is allowed to
> accept more bytes. It is set to false when the SourceBuffer object is
> created and gets updated as data is appended and removed."
> 
> "buffer full flag" only gets modified to true in the 3.5.1 Segment Parser
> Loop algorithm, step 6.3
> https://w3c.github.io/media-source/#sourcebuffer-segment-parser-loop
> "If this SourceBuffer is full and cannot accept more media data, then set
> the buffer full flag to true."
> 
> On the 2nd call to the appendBuffer, in the Prepare Append algorithm, step
> 3.5.4.6:
> https://w3c.github.io/media-source/#sourcebuffer-prepare-append
> 
> "If the buffer full flag equals true, then throw a QuotaExceededError
> exception and abort these steps."
> 
> This change as such, made the WebKit implementation non-compliant. 
> 
> However, I can understand why this change would be desired.
> 
> I will have a follow-up bug to rework how we handle source buffer size and
> checking capacity. It will make it easier to toggle between the two
> behaviours if needed (one before and after this fix)

Thank you for your comment.
I will consider.

[Toshio Ogasawara]

https://bugs.webkit.org/show_bug.cgi?id=226711

Thank you for follow-up.