Bug 212073 – MediaPlayerPrivateMediaStreamAVFObjC should enqueue samples in a background thread

• Status: RESOLVED
• Resolution: FIXED
• Component: WebRTC

[youenn fablet]

MediaPlayerPrivateMediaStreamAVFObjC should enqueue samples in a background thread

[youenn fablet]

Created attachment 400239 [details]
Patch

[youenn fablet]

<rdar://problem/62450764>

[youenn fablet]

Created attachment 400437 [details]
Patch

[Eric Carlson]

Comment on attachment 400437 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:294
> +// Called on a background thread.
>  void LocalSampleBufferDisplayLayer::enqueueSample(MediaSample& sample)
>  {

Is this comment in the wrong place?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:250
> +    processNewVideoSample(sample, sample.videoRotation() != m_videoRotation || sample.videoMirrored() != m_videoMirrored);
> +    enqueueVideoSample(sample);

processNewVideoSample does its work on the main thread, but enqueueVideoSample updates the layer and enqueues the sample immediately. What will happen if AVF displays the sample before processNewVideoSample runs?

[youenn fablet]

Comment on attachment 400437 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/LocalSampleBufferDisplayLayer.mm:294
>>  {
> 
> Is this comment in the wrong place?

This follows the practice of audioSamplesAvailable, I'll remove it.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:250
>> +    enqueueVideoSample(sample);
> 
> processNewVideoSample does its work on the main thread, but enqueueVideoSample updates the layer and enqueues the sample immediately. What will happen if AVF displays the sample before processNewVideoSample runs?

I don't think this will cause much issues.
processNewVideoSample does things like updating the ready state or keeping a ref to the sample in case of canvas capture.
Updating the ready state might trigger starting the video player, which might then happen after receiving a few samples.

It also updates the display mode which might change the visibility of the layer.
It might be that some samples will be rendered before the layer visibility is updated.

[youenn fablet]

Created attachment 400565 [details]
Patch for landing

[EWS]

Committed r262289: <https://trac.webkit.org/changeset/262289>

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

[Jacob Uphoff]

Reverted r262289 for reason:

This commit caused a test to crash internally

Committed r262316: <https://trac.webkit.org/changeset/262316>

[youenn fablet]

Created attachment 400799 [details]
Patch for relanding

[EWS]

Committed r262410: <https://trac.webkit.org/changeset/262410>

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