| Summary: | [GPUP][MSE] QuotaExceededError Exception not thrown even if the sum of totalTrackBufferSize and appendBuffer size exceeds maximumBufferSize | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||
| Component: | Media | Assignee: | Jean-Yves Avenard [:jya] <jean-yves.avenard> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | calvaris, eric.carlson, ews-watchlist, glenn, jean-yves.avenard, jer.noble, peng.liu6, philipj, sergio, webkit-bug-importer | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | Other | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| See Also: |
https://bugs.webkit.org/show_bug.cgi?id=225630 https://bugs.webkit.org/show_bug.cgi?id=226711 |
||||||||
| Attachments: |
|
||||||||
|
Description
Peng Liu
2021-05-20 11:50:59 PDT
Created attachment 429986 [details]
Patch
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 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. (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. 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 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 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 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 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 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 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 on attachment 430570 [details]
Patch
unrelated failure
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]. |