Bug 220964

<rdar://problem/72391066>

Created attachment 418370 [details]
Patch

Created attachment 418372 [details]
Patch

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

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

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

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

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

Created attachment 418498 [details]
WIP patch

Created attachment 418615 [details]
WIP patch

Created attachment 418622 [details]
Patch

Created attachment 418623 [details]
Patch

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

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

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

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

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

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