Bug 170404

Summary: [MediaStream] Video doesn't render in fullscreen on iOS
Product: WebKit Reporter: Eric Carlson <eric.carlson>
Component: MediaAssignee: Eric Carlson <eric.carlson>
Status: ASSIGNED ---    
Severity: Normal CC: commit-queue, sam, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Proposed patch.
youennf: review+
Patch for landing. none

Description Eric Carlson 2017-04-03 10:02:38 PDT
Video doesn't render in fullscreen on iOS
Comment 1 Eric Carlson 2017-04-03 11:10:57 PDT
Created attachment 306086 [details]
Proposed patch.
Comment 2 Sam Weinig 2017-04-03 11:29:55 PDT
Comment on attachment 306086 [details]
Proposed patch.

Can this be tested?  If not, is there a bug tracking adding infrastructure to test this kind of thing?
Comment 3 Radar WebKit Bug Importer 2017-04-03 11:30:35 PDT
<rdar://problem/31407253>
Comment 4 youenn fablet 2017-04-03 12:24:31 PDT
Comment on attachment 306086 [details]
Proposed patch.

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

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:-56
> -#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)

PLATFORM(COCOA)?

> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:1112
> +    scheduleDeferredTask([this] {

Should we protect ┬┤this'?
If so, should the m_backgroundLayer check be done here?
Comment 5 youenn fablet 2017-04-03 12:24:59 PDT
Any chance for some kind of test?
Comment 6 Eric Carlson 2017-04-03 20:30:01 PDT
(In reply to Sam Weinig from comment #2)
> Comment on attachment 306086 [details]
> Proposed patch.
> 
> Can this be tested?  If not, is there a bug tracking adding infrastructure
> to test this kind of thing?

It can't be tested now, but I will file a bug and note it in the ChangeLog.
Comment 7 Eric Carlson 2017-04-03 20:30:16 PDT
Comment on attachment 306086 [details]
Proposed patch.

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

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:-56
>> -#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
> 
> PLATFORM(COCOA)?

Yep, good idea.

>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.mm:1112
>> +    scheduleDeferredTask([this] {
> 
> Should we protect ┬┤this'?
> If so, should the m_backgroundLayer check be done here?

scheduleDeferredTask creates a weak pointer, so that is done automatically. Good point about the m_backgroundLayer check, it should be done inside of the lambda.
Comment 8 Eric Carlson 2017-04-04 14:16:14 PDT
Comment on attachment 306086 [details]
Proposed patch.

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

>>> Source/WebCore/platform/graphics/avfoundation/objc/MediaPlayerPrivateMediaStreamAVFObjC.h:-56
>>> -#if PLATFORM(MAC) && ENABLE(VIDEO_PRESENTATION_MODE)
>> 
>> PLATFORM(COCOA)?
> 
> Yep, good idea.

Actually that won't work because not all versions of macOS support PiP.
Comment 9 Eric Carlson 2017-04-05 10:54:12 PDT
Created attachment 306298 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2017-04-05 11:34:27 PDT
Comment on attachment 306298 [details]
Patch for landing.

Clearing flags on attachment: 306298

Committed r214953: <http://trac.webkit.org/changeset/214953>