Bug 177016 - MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoData should enqueue data if still useful
Summary: MediaPlayerPrivateMediaStreamAVFObjC::requestNotificationWhenReadyForVideoDat...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-09-15 11:21 PDT by youenn fablet
Modified: 2017-09-15 12:55 PDT (History)
5 users (show)

See Also:


Attachments
Patch (1.93 KB, patch)
2017-09-15 11:22 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-09-15 11:21:02 PDT
If it is dead, no need to do it.
Comment 1 youenn fablet 2017-09-15 11:21:14 PDT
rdar://problem/33803695
Comment 2 youenn fablet 2017-09-15 11:22:49 PDT
Created attachment 320933 [details]
Patch
Comment 3 Jer Noble 2017-09-15 11:32:50 PDT
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.
Comment 4 youenn fablet 2017-09-15 12:01:56 PDT
> 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?
Comment 5 Jer Noble 2017-09-15 12:18:01 PDT
(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!).
Comment 6 Eric Carlson 2017-09-15 12:22:36 PDT
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.
Comment 7 Jer Noble 2017-09-15 12:23:56 PDT
(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. :)
Comment 8 Alexey Proskuryakov 2017-09-15 12:27:54 PDT
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?
Comment 9 youenn fablet 2017-09-15 12:32:27 PDT
I think there is a plan to try removing the WebThread.
Comment 10 youenn fablet 2017-09-15 12:36:07 PDT
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.
Comment 11 Geoffrey Garen 2017-09-15 12:46:19 PDT
Comment on attachment 320933 [details]
Patch

Have we converged to an r+ then?
Comment 12 Jer Noble 2017-09-15 12:48:16 PDT
(In reply to Geoffrey Garen from comment #11)
> Comment on attachment 320933 [details]
> Patch
> 
> Have we converged to an r+ then?

Yep.
Comment 13 WebKit Commit Bot 2017-09-15 12:55:26 PDT
Comment on attachment 320933 [details]
Patch

Clearing flags on attachment: 320933

Committed r222105: <http://trac.webkit.org/changeset/222105>
Comment 14 WebKit Commit Bot 2017-09-15 12:55:27 PDT
All reviewed patches have been landed.  Closing bug.