Bug 170404 - [MediaStream] Video doesn't render in fullscreen on iOS
Summary: [MediaStream] Video doesn't render in fullscreen on iOS
Status: ASSIGNED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-03 10:02 PDT by Eric Carlson
Modified: 2017-04-05 11:34 PDT (History)
4 users (show)

See Also:

Attachments
Proposed patch. (9.05 KB, patch)
2017-04-03 11:10 PDT, Eric Carlson 		youennf: review+
Details | Formatted Diff | Diff
Patch for landing. (9.71 KB, patch)
2017-04-05 10:54 PDT, Eric Carlson 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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>