Created attachment 400239 [details]
Patch

<rdar://problem/62450764>

Created attachment 400437 [details]
Patch

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?

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.

Created attachment 400565 [details]
Patch for landing

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

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

Reverted r262289 for reason:

This commit caused a test to crash internally

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

Created attachment 400799 [details]
Patch for relanding

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

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