Bug 226711

Summary: [MSE] Rework handling of SourceBuffer's buffer full.
Product: WebKit Reporter: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Component: MediaAssignee: Jean-Yves Avenard [:jya] <jean-yves.avenard>
Status: RESOLVED FIXED    
Severity: Normal CC: calvaris, eric.carlson, ews-watchlist, glenn, jer.noble, philipj, sergio, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=225630
https://bugs.webkit.org/show_bug.cgi?id=226034
Attachments:
Description Flags
Patch
none
Patch none

Description Jean-Yves Avenard [:jya] 2021-06-07 00:55:04 PDT
Follow-up from https://bugs.webkit.org/show_bug.cgi?id=225630#c9

bug 225630 changed the logic on how we are to reject an append only after one has already been added.

One downside of the change was also that it broke the class separation between SourceBuffer and SourceBufferPrivate ; it is preferable to have all the handling of buffer size in the SourceBufferPrivate which avoid ambiguities as discussed in bug https://bugs.webkit.org/show_bug.cgi?id=226034
Comment 1 Radar WebKit Bug Importer 2021-06-07 00:58:09 PDT
<rdar://problem/78937909>
Comment 2 Jean-Yves Avenard [:jya] 2021-06-07 02:06:34 PDT
Created attachment 430730 [details]
Patch
Comment 3 Jer Noble 2021-06-07 16:43:24 PDT
Comment on attachment 430730 [details]
Patch

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

> Source/WebCore/platform/graphics/SourceBufferPrivate.cpp:726
> +    if (WTF::sumOverflows<uint64_t>(totalTrackBufferSizeInBytes(), requiredSize))
> +        return true;
> +
> +    return totalTrackBufferSizeInBytes() + requiredSize >= maximumBufferSize;

Irrespective of the overall patch, this performs the addition twice. It would probably be more efficient to do something like:

uint64_t totalRequired = 0;
if (CheckedState::DidOverflow == CheckedSum(totalTrackBufferSizeInBytes(), requiredSize).safeGet(totalRequired);
    return true;
return totalRequired >= maximumBufferSize;
Comment 4 Jean-Yves Avenard [:jya] 2021-06-07 21:27:54 PDT
Created attachment 430806 [details]
Patch
Comment 5 Jean-Yves Avenard [:jya] 2021-06-07 23:43:52 PDT
Comment on attachment 430806 [details]
Patch

test failures unrelated to media's MSE
Comment 6 EWS 2021-06-07 23:44:15 PDT
jean-yves.avenard@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 430806 [details] from commit queue.
Comment 7 EWS 2021-06-08 00:41:04 PDT
jean-yves.avenard@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 430806 [details] from commit queue.
Comment 8 EWS 2021-06-08 03:52:11 PDT
jean-yves.avenard@apple.com does not have committer permissions according to https://raw.githubusercontent.com/WebKit/WebKit/main/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 430806 [details] from commit queue.
Comment 9 EWS 2021-06-08 05:29:58 PDT
Committed r278603 (238592@main): <https://commits.webkit.org/238592@main>

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