Bug 220964 – [GPUP][MSE] A video element does not fire “canplaythrough” event if SourceBuffer.abort() is called

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

[Peng Liu]

[GPUP][MSE] A video element does not fire “canplaythrough” event if SourceBuffer.abort() is called

[Peng Liu]

<rdar://problem/72391066>

[Peng Liu]

Created attachment 418370 [details]
Patch

[Peng Liu]

Created attachment 418372 [details]
Patch

[Peng Liu]

Created attachment 418459 [details]
Fix a test timeout on mac-wk1

[Darin Adler]

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.

[Peng Liu]

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.

[Darin Adler]

(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.

[Peng Liu]

(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!

[Peng Liu]

After a discussion with Jer, we decide to refactor the current implementation of SourceBufferPrivateAVFObjC, SourceBufferParserAVFObjC and SourceBufferParserWebM to get rid of the semaphore (initializationSegmentIsHandledSemaphore).

[Peng Liu]

Created attachment 418498 [details]
WIP patch

[Peng Liu]

Created attachment 418615 [details]
WIP patch

[Peng Liu]

Created attachment 418622 [details]
Patch

[Peng Liu]

Created attachment 418623 [details]
Patch

[Jer Noble]

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.

[Darin Adler]

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.

[Peng Liu]

Created attachment 418661 [details]
Revise the patch based on Jer and Darin's comments

[Darin Adler]

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.

[Peng Liu]

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!

[Peng Liu]

Created attachment 418664 [details]
Revise the patch based Darin's comment

[EWS]

Committed r272039: <https://trac.webkit.org/changeset/272039>

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