Bug 226034 - [GPUP][MSE] QuotaExceededError Exception not thrown even if the sum of totalTrackBufferSize and appendBuffer size exceeds maximumBufferSize
Summary: [GPUP][MSE] QuotaExceededError Exception not thrown even if the sum of totalT...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jean-Yves Avenard [:jya]
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-20 11:50 PDT by Peng Liu
Modified: 2021-06-07 00:55 PDT (History)
10 users (show)

See Also:


Attachments
Patch (11.74 KB, patch)
2021-05-27 22:35 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (19.83 KB, patch)
2021-06-04 05:11 PDT, Jean-Yves Avenard [:jya]
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2021-05-20 11:50:59 PDT
Follow up bug 225630.
Comment 1 Radar WebKit Bug Importer 2021-05-27 11:51:17 PDT
<rdar://problem/78579904>
Comment 2 Peng Liu 2021-05-27 22:35:52 PDT
Created attachment 429986 [details]
Patch
Comment 3 Jean-Yves Avenard [:jya] 2021-05-27 23:06:26 PDT
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.
Comment 4 Peng Liu 2021-05-28 13:32:59 PDT
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.
Comment 5 Jean-Yves Avenard [:jya] 2021-06-04 02:20:10 PDT
(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.
Comment 6 Jean-Yves Avenard [:jya] 2021-06-04 05:11:15 PDT
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
Comment 7 Eric Carlson 2021-06-04 09:28:23 PDT
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!
Comment 8 Peng Liu 2021-06-04 09:37:33 PDT
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?
Comment 9 Jean-Yves Avenard [:jya] 2021-06-04 16:27:36 PDT
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
Comment 10 Peng Liu 2021-06-04 17:40:58 PDT
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.
Comment 11 Jean-Yves Avenard [:jya] 2021-06-04 18:15:29 PDT
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
Comment 12 Jean-Yves Avenard [:jya] 2021-06-04 18:15:30 PDT
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
Comment 13 Jean-Yves Avenard [:jya] 2021-06-06 16:39:07 PDT
Comment on attachment 430570 [details]
Patch

unrelated failure
Comment 14 EWS 2021-06-06 17:01:57 PDT
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].