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

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Follow up bug 225630.

[Radar WebKit Bug Importer]

<rdar://problem/78579904>

[Peng Liu]

Created attachment 429986 [details]
Patch

[Jean-Yves Avenard [:jya]]

Comment on attachment 429986 [details]
Patch

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

> Source/WebKit/ChangeLog:9
> +        `SourceBufferPrivateRemote::totalTrackBufferSizeInBytes()`. We have to use

Why not return the total track buffer size from the previous append buffer operation?
It's known then and won't change after that.

The total calculations can be calculated at the end of some operations (appendBuffer, remove). The remove blocking the main thread while a sync call is occurring to a different process.

[Peng Liu]

Comment on attachment 429986 [details]
Patch

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

>> Source/WebKit/ChangeLog:9
>> +        `SourceBufferPrivateRemote::totalTrackBufferSizeInBytes()`. We have to use
> 
> Why not return the total track buffer size from the previous append buffer operation?
> It's known then and won't change after that.
> 
> The total calculations can be calculated at the end of some operations (appendBuffer, remove). The remove blocking the main thread while a sync call is occurring to a different process.

`SourceBuffer::appendBuffer()` calls `SourceBuffer::appendBufferInternal()`, which in turn calls `SourceBufferPrivate::evictCodedFrames()`. It may change the return value of `SourceBufferPrivate::totalTrackBufferSizeInBytes()`. And all those operations happen in the same run loop.

Currently, we implement `SourceBufferPrivateRemote::evictCodedFrames()` with a synchronous IPC message. We can reuse this synchronous IPC message to "fetch" the latest `totalTrackBufferSizeInBytes` from the GPU Process. Because currently only `SourceBuffer::appendBufferInternal()` calls `SourceBufferPrivate::totalTrackBufferSizeInBytes()`, that will work.

I did not choose this approach because we may use `SourceBufferPrivate::totalTrackBufferSizeInBytes()` in other places in the future. As you mentioned, we have to make sure the calculation is done correctly in all operations that may change `totalTrackBufferSizeInBytes`, and we need to make sure there isn’t any timing issue.

[Jean-Yves Avenard [:jya]]

(In reply to Peng Liu from comment #4)

> I did not choose this approach because we may use
> `SourceBufferPrivate::totalTrackBufferSizeInBytes()` in other places in the
> future. As you mentioned, we have to make sure the calculation is done
> correctly in all operations that may change `totalTrackBufferSizeInBytes`,
> and we need to make sure there isn’t any timing issue.

There can't be timing issue because by spec we can only have a single source buffer operation going at a given time.

That is, you can't call remove or appendBuffer or modify anything related to the source buffer while updating attribute is true.

So caching the value of the source buffer size at the end of these operations is safe.

[Jean-Yves Avenard [:jya]]

Created attachment 430570 [details]
Patch

Cache the size of the remote source buffer, we do so without adding an extra sync dispatch, instead amending existing calls to include totalTrackBufferSizeInBytes

[Eric Carlson]

Comment on attachment 430570 [details]
Patch

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

> Source/WebCore/Modules/mediasource/SourceBuffer.cpp:494
> -    m_private->evictCodedFrames(size, m_pendingAppendData.capacity(), maximumBufferSize(), m_source->currentTime(), m_source->duration(), m_source->isEnded());
> +    m_private->evictCodedFrames(size, m_pendingAppendData.size(), maximumBufferSize(), m_source->currentTime(), m_source->duration(), m_source->isEnded());
>  
>      // 5. If the buffer full flag equals true, then throw a QuotaExceededError exception and abort these step.
> -    if (m_private->bufferFull() || m_private->totalTrackBufferSizeInBytes() + m_pendingAppendData.capacity() + size >= maximumBufferSize()) {
> +    if (m_private->bufferFull() || m_private->totalTrackBufferSizeInBytes() + m_pendingAppendData.size() + size >= maximumBufferSize()) {

Good catch!

[Peng Liu]

Comment on attachment 430570 [details]
Patch

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

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

Maybe we need to modify the test to cover that?

> Source/WebCore/platform/graphics/SourceBufferPrivate.h:91
> +    WEBCORE_EXPORT virtual uint64_t totalTrackBufferSizeInBytes() const;

Hmm, my major concern was that `totalTrackBufferSizeInBytes()` is a public method, so `SourceBuffer` class may call it in other places (in the future), where `totalTrackBufferSizeInBytes()` may return a stale value when `SourceBufferPrivate` is modifying track buffers in another process. It seems not an issue for now though.
Is it a good idea to add a comment for `totalTrackBufferSizeInBytes()` about that?

[Jean-Yves Avenard [:jya]]

Comment on attachment 430570 [details]
Patch

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

>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:91
>> +    WEBCORE_EXPORT virtual uint64_t totalTrackBufferSizeInBytes() const;
> 
> Hmm, my major concern was that `totalTrackBufferSizeInBytes()` is a public method, so `SourceBuffer` class may call it in other places (in the future), where `totalTrackBufferSizeInBytes()` may return a stale value when `SourceBufferPrivate` is modifying track buffers in another process. It seems not an issue for now though.
> Is it a good idea to add a comment for `totalTrackBufferSizeInBytes()` about that?

As it's used, and following these changes ; the value read in the web content process will never be stale.
Every method that could lead to the source buffer size to change causes the cache value to be updated. So even future use of this method will return an up to  date value

[Peng Liu]

Comment on attachment 430570 [details]
Patch

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

>>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:91
>>> +    WEBCORE_EXPORT virtual uint64_t totalTrackBufferSizeInBytes() const;
>> 
>> Hmm, my major concern was that `totalTrackBufferSizeInBytes()` is a public method, so `SourceBuffer` class may call it in other places (in the future), where `totalTrackBufferSizeInBytes()` may return a stale value when `SourceBufferPrivate` is modifying track buffers in another process. It seems not an issue for now though.
>> Is it a good idea to add a comment for `totalTrackBufferSizeInBytes()` about that?
> 
> As it's used, and following these changes ; the value read in the web content process will never be stale.
> Every method that could lead to the source buffer size to change causes the cache value to be updated. So even future use of this method will return an up to  date value

But the values are sent back to the WebContent process asynchronously, for example, in the `RemoveCodedFrames` message. If the `SourceBuffer` calls `totalTrackBufferSizeInBytes()` before the response coming back, the cached value will be stale. It is kind like "eventual consistency". Probably it is good enough.

[Jean-Yves Avenard [:jya]]

Comment on attachment 430570 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:91
>>>> +    WEBCORE_EXPORT virtual uint64_t totalTrackBufferSizeInBytes() const;
>>> 
>>> Hmm, my major concern was that `totalTrackBufferSizeInBytes()` is a public method, so `SourceBuffer` class may call it in other places (in the future), where `totalTrackBufferSizeInBytes()` may return a stale value when `SourceBufferPrivate` is modifying track buffers in another process. It seems not an issue for now though.
>>> Is it a good idea to add a comment for `totalTrackBufferSizeInBytes()` about that?
>> 
>> As it's used, and following these changes ; the value read in the web content process will never be stale.
>> Every method that could lead to the source buffer size to change causes the cache value to be updated. So even future use of this method will return an up to  date value
> 
> But the values are sent back to the WebContent process asynchronously, for example, in the `RemoveCodedFrames` message. If the `SourceBuffer` calls `totalTrackBufferSizeInBytes()` before the response coming back, the cached value will be stale. It is kind like "eventual consistency". Probably it is good enough.

But RemoveCodedFrames is asynchronous to start with. So checking the size of the source buffer before the operation has been completed is nonsensical.
It is only useful once we know the operation has completed,

That RemoveCodedFrames happened to be done synchronously before the GPU process existed was an implementation details. 
Just like reading a JS value outside a stable state is undefined

[Jean-Yves Avenard [:jya]]

Comment on attachment 430570 [details]
Patch

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

>>>> Source/WebCore/platform/graphics/SourceBufferPrivate.h:91
>>>> +    WEBCORE_EXPORT virtual uint64_t totalTrackBufferSizeInBytes() const;
>>> 
>>> Hmm, my major concern was that `totalTrackBufferSizeInBytes()` is a public method, so `SourceBuffer` class may call it in other places (in the future), where `totalTrackBufferSizeInBytes()` may return a stale value when `SourceBufferPrivate` is modifying track buffers in another process. It seems not an issue for now though.
>>> Is it a good idea to add a comment for `totalTrackBufferSizeInBytes()` about that?
>> 
>> As it's used, and following these changes ; the value read in the web content process will never be stale.
>> Every method that could lead to the source buffer size to change causes the cache value to be updated. So even future use of this method will return an up to  date value
> 
> But the values are sent back to the WebContent process asynchronously, for example, in the `RemoveCodedFrames` message. If the `SourceBuffer` calls `totalTrackBufferSizeInBytes()` before the response coming back, the cached value will be stale. It is kind like "eventual consistency". Probably it is good enough.

But RemoveCodedFrames is asynchronous to start with. So checking the size of the source buffer before the operation has been completed is nonsensical.
It is only useful once we know the operation has completed,

That RemoveCodedFrames happened to be done synchronously before the GPU process existed was an implementation details. 
Just like reading a JS value outside a stable state is undefined

[Jean-Yves Avenard [:jya]]

Comment on attachment 430570 [details]
Patch

unrelated failure

[EWS]

Committed r278539 (238537@main): <https://commits.webkit.org/238537@main>

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