Bug 212073

Summary: MediaPlayerPrivateMediaStreamAVFObjC should enqueue samples in a background thread
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: eric.carlson, ews-watchlist, glenn, jacob_uphoff, jer.noble, philipj, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 194802    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch for landing
none
Patch for relanding none

Description youenn fablet 2020-05-19 07:48:32 PDT 
MediaPlayerPrivateMediaStreamAVFObjC should enqueue samples in a background thread
Comment 1 youenn fablet 2020-05-26 04:56:09 PDT 
Created attachment 400239 [details]
Patch
Comment 2 youenn fablet 2020-05-26 04:56:30 PDT 
<rdar://problem/62450764>
Comment 3 youenn fablet 2020-05-28 01:46:10 PDT 
Created attachment 400437 [details]
Patch
Comment 4 Eric Carlson 2020-05-28 09:09:44 PDT 
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 5 youenn fablet 2020-05-29 01:33:47 PDT 
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.
Comment 6 youenn fablet 2020-05-29 03:32:02 PDT 
Created attachment 400565 [details]
Patch for landing
Comment 7 EWS 2020-05-29 05:15:10 PDT 
Committed r262289: <https://trac.webkit.org/changeset/262289>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400565 [details].
Comment 8 Jacob Uphoff 2020-05-29 14:06:09 PDT 
Reverted r262289 for reason:

This commit caused a test to crash internally

Committed r262316: <https://trac.webkit.org/changeset/262316>
Comment 9 youenn fablet 2020-06-02 02:30:24 PDT 
Created attachment 400799 [details]
Patch for relanding
Comment 10 EWS 2020-06-02 04:24:24 PDT 
Committed r262410: <https://trac.webkit.org/changeset/262410>

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