Bug 177016 – MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData should enqueue data if still useful

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[youenn fablet]

If it is dead, no need to do it.

[youenn fablet]

rdar://problem/33803695

[youenn fablet]

Created attachment 320933 [details]
Patch

[Jer Noble]

Comment on attachment 320933 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        (WebCore::MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData): exciting early in block to prevent enqueueing.

Nit: "exiting".

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401
> +    auto weakThis = createWeakPtr();
>      [m_sampleBufferDisplayLayer requestMediaDataWhenReadyOnQueue:dispatch_get_main_queue() usingBlock:^ {
> +        if (!weakThis)
> +            return;
> +

Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are.

[youenn fablet]

> Note that this won't necessarily be correct on iOS WK1, as the
> "dispatch_get_main_queue()" won't necessarily be the WebThread, in which
> case, you'll assert in the weakThis getter. This isn't a problem in the MSE
> code, since that's not enabled on iOS, but it may be a problem here, since
> media streams are.

I am not familiar with WebThread.
I guess that if we are not in the WebThread, we could hop to it, through callOnMainThread probably?.

Or is there a way to dispatch it to the WebThread directly?

[Jer Noble]

(In reply to youenn fablet from comment #4)
> > Note that this won't necessarily be correct on iOS WK1, as the
> > "dispatch_get_main_queue()" won't necessarily be the WebThread, in which
> > case, you'll assert in the weakThis getter. This isn't a problem in the MSE
> > code, since that's not enabled on iOS, but it may be a problem here, since
> > media streams are.
> 
> I am not familiar with WebThread.

The basic gist is that, to get smooth scrolling on iOS in WK1, all the WebCore "main thread" functions run off the process's main thread, an on a named thread called "WebThread".

> I guess that if we are not in the WebThread, we could hop to it, through
> callOnMainThread probably?.

Yes. 

> Or is there a way to dispatch it to the WebThread directly?

No, there's not (but that would be great!).

[Eric Carlson]

Comment on attachment 320933 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401
>> +
> 
> Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are.

We don't support media streams in WK1 so this should be OK.

[Jer Noble]

(In reply to Eric Carlson from comment #6)
> Comment on attachment 320933 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=320933&action=review
> 
> >> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:401
> >> +
> > 
> > Note that this won't necessarily be correct on iOS WK1, as the "dispatch_get_main_queue()" won't necessarily be the WebThread, in which case, you'll assert in the weakThis getter. This isn't a problem in the MSE code, since that's not enabled on iOS, but it may be a problem here, since media streams are.
> 
> We don't support media streams in WK1 so this should be OK.

Oh, ok, never mind then. :)

[Alexey Proskuryakov]

Comment on attachment 320933 [details]
Patch

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:400
> +        if (!weakThis)
> +            return;

What if the object gets destroyed after this check, but before the rest of the code in the block runs?

[youenn fablet]

I think there is a plan to try removing the WebThread.

[youenn fablet]

Comment on attachment 320933 [details]
Patch

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:400
>> +            return;
> 
> What if the object gets destroyed after this check, but before the rest of the code in the block runs?

As I read it, the object destruction and this code block should both happen on the same thread.

[Geoffrey Garen]

Comment on attachment 320933 [details]
Patch

Have we converged to an r+ then?

[Jer Noble]

(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 320933 [details]
> Patch
> 
> Have we converged to an r+ then?

Yep.

[WebKit Commit Bot]

Comment on attachment 320933 [details]
Patch

Clearing flags on attachment: 320933

Committed r222105: <http://trac.webkit.org/changeset/222105>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.