Bug 220964 - [GPUP][MSE] A video element does not fire “canplaythrough” event if SourceBuffer.abort() is called
Summary: [GPUP][MSE] A video element does not fire “canplaythrough” event if SourceBuf...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-01-25 21:10 PST by Peng Liu
Modified: 2021-04-09 14:23 PDT (History)
11 users (show)

See Also:


Attachments
Patch (8.49 KB, patch)
2021-01-25 21:32 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (9.40 KB, patch)
2021-01-25 22:09 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Fix a test timeout on mac-wk1 (10.04 KB, patch)
2021-01-26 12:00 PST, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (17.70 KB, patch)
2021-01-26 20:03 PST, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (31.74 KB, patch)
2021-01-27 21:24 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (33.46 KB, patch)
2021-01-27 22:42 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (33.48 KB, patch)
2021-01-27 22:44 PST, Peng Liu
jer.noble: review+
Details | Formatted Diff | Diff
Revise the patch based on Jer and Darin's comments (33.48 KB, patch)
2021-01-28 12:22 PST, Peng Liu
no flags Details | Formatted Diff | Diff
Revise the patch based Darin's comment (33.49 KB, patch)
2021-01-28 13:13 PST, Peng Liu
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-01-25 21:10:27 PST
[GPUP][MSE] A video element does not fire “canplaythrough” event if SourceBuffer.abort() is called
Comment 1 Peng Liu 2021-01-25 21:14:48 PST
<rdar://problem/72391066>
Comment 2 Peng Liu 2021-01-25 21:32:50 PST
Created attachment 418370 [details]
Patch
Comment 3 Peng Liu 2021-01-25 22:09:50 PST
Created attachment 418372 [details]
Patch
Comment 4 Peng Liu 2021-01-26 12:00:32 PST
Created attachment 418459 [details]
Fix a test timeout on mac-wk1
Comment 5 Darin Adler 2021-01-26 13:28:28 PST
Comment on attachment 418459 [details]
Fix a test timeout on mac-wk1

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:309
> +    while (!m_initializationSegmentIsHandledSemaphore->waitFor(100_ms)) {

Where does the 100ms duty cycle come from? Normally with a magic number like this we like a comment explaining how we chose the number. Often we use a named constant to give us a place to write that comment.
Comment 6 Peng Liu 2021-01-26 14:56:01 PST
Comment on attachment 418459 [details]
Fix a test timeout on mac-wk1

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

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferParserAVFObjC.mm:309
>> +    while (!m_initializationSegmentIsHandledSemaphore->waitFor(100_ms)) {
> 
> Where does the 100ms duty cycle come from? Normally with a magic number like this we like a comment explaining how we chose the number. Often we use a named constant to give us a place to write that comment.

To be honest, 100ms is an arbitrary value. I chose it because it is not too short, so it is good for efficiency. And it is not too long so that SourceBufferPrivateAVFObjC::abort() won't take a long time to complete.
Comment 7 Darin Adler 2021-01-26 14:59:53 PST
(In reply to Peng Liu from comment #6)
> I chose it because it is not too
> short, so it is good for efficiency. And it is not too long so that
> SourceBufferPrivateAVFObjC::abort() won't take a long time to complete.

This is the comment. Maybe tweak the wording.
Comment 8 Peng Liu 2021-01-26 15:03:22 PST
(In reply to Darin Adler from comment #7)
> (In reply to Peng Liu from comment #6)
> > I chose it because it is not too
> > short, so it is good for efficiency. And it is not too long so that
> > SourceBufferPrivateAVFObjC::abort() won't take a long time to complete.
> 
> This is the comment. Maybe tweak the wording.

Got it, thanks!
Comment 9 Peng Liu 2021-01-26 19:50:51 PST
After a discussion with Jer, we decide to refactor the current implementation of SourceBufferPrivateAVFObjC, SourceBufferParserAVFObjC and SourceBufferParserWebM to get rid of the semaphore (initializationSegmentIsHandledSemaphore).
Comment 10 Peng Liu 2021-01-26 20:03:15 PST
Created attachment 418498 [details]
WIP patch
Comment 11 Peng Liu 2021-01-27 21:24:56 PST
Created attachment 418615 [details]
WIP patch
Comment 12 Peng Liu 2021-01-27 22:42:12 PST
Created attachment 418622 [details]
Patch
Comment 13 Peng Liu 2021-01-27 22:44:02 PST
Created attachment 418623 [details]
Patch
Comment 14 Jer Noble 2021-01-28 10:53:43 PST
Comment on attachment 418623 [details]
Patch

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

r=me, with nit:

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:406
> +            for (auto trackIdMediaSamplePair : m_mediaSamples) {

This...

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:415
> +            m_mediaSamples.clear();

...and this could probably be merged together into something more like:

auto samples = WTFMove(m_mediaSamples);
for (auto trackIdMediaSamplePair : samples) {

And I know this isn't an issue for this particular code, but insulates m_mediaSamples from being modified while iterating in the future.
Comment 15 Darin Adler 2021-01-28 11:01:12 PST
Comment on attachment 418623 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:406
>> +            for (auto trackIdMediaSamplePair : m_mediaSamples) {
> 
> This...

Writing auto& instead of auto should make the code slight more efficient with no other repercussions..

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:408
> +                auto trackId = trackIdMediaSamplePair.first;
> +                auto mediaSample = trackIdMediaSamplePair.second;

Same here for the same reasons. Unless there is some risk of m_mediaSamples being mutated while iterating. And if so, then a great mitigation is what Jer offered.
Comment 16 Peng Liu 2021-01-28 12:22:47 PST
Created attachment 418661 [details]
Revise the patch based on Jer and Darin's comments
Comment 17 Darin Adler 2021-01-28 12:57:35 PST
Comment on attachment 418661 [details]
Revise the patch based on Jer and Darin's comments

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

> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:406
> +            auto mediaSamples = WTFMove(m_mediaSamples);

I know that Jer offered this sample code, and I didn’t comment then. But I since realized that this is not the correct idiom. The correct idiom is:

    auto mediaSamples = std::exchange(m_mediaSamples, { });

Using WTFMove allows the code to be optimized by moving, but it can also be copying as long as the source object, m_mediaSamples, is still safely destructible. Using std::exchange explicitly sets m_mediaSamples to an empty value, and that is the semantic we want here. Any time we are relying on actually looking at the value of something after moving from it (as opposed to just overwriting it or destroying it) we should use the std::exchange idiom. It should be roughly the same efficiency.
Comment 18 Peng Liu 2021-01-28 13:06:43 PST
Comment on attachment 418661 [details]
Revise the patch based on Jer and Darin's comments

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

>> Source/WebCore/platform/graphics/avfoundation/objc/SourceBufferPrivateAVFObjC.mm:406
>> +            auto mediaSamples = WTFMove(m_mediaSamples);
> 
> I know that Jer offered this sample code, and I didn’t comment then. But I since realized that this is not the correct idiom. The correct idiom is:
> 
>     auto mediaSamples = std::exchange(m_mediaSamples, { });
> 
> Using WTFMove allows the code to be optimized by moving, but it can also be copying as long as the source object, m_mediaSamples, is still safely destructible. Using std::exchange explicitly sets m_mediaSamples to an empty value, and that is the semantic we want here. Any time we are relying on actually looking at the value of something after moving from it (as opposed to just overwriting it or destroying it) we should use the std::exchange idiom. It should be roughly the same efficiency.

Thanks for the detailed explanation!
Comment 19 Peng Liu 2021-01-28 13:13:28 PST
Created attachment 418664 [details]
Revise the patch based Darin's comment
Comment 20 EWS 2021-01-28 16:19:38 PST
Committed r272039: <https://trac.webkit.org/changeset/272039>

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