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

Eric Carlson
Reported 2017-04-03 10:02:38 PDT
Video doesn't render in fullscreen on iOS
Attachments
Proposed patch. (9.05 KB, patch)
2017-04-03 11:10 PDT, Eric Carlson 		youennf: review+
DetailsFormatted DiffDiff
Patch for landing. (9.71 KB, patch)
2017-04-05 10:54 PDT, Eric Carlson 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Eric Carlson
Comment 1 2017-04-03 11:10:57 PDT
Created attachment 306086 [details] Proposed patch.
Sam Weinig
Comment 2 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?
Radar WebKit Bug Importer
Comment 3 2017-04-03 11:30:35 PDT
<rdar://problem/31407253>
youenn fablet
Comment 4 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?
youenn fablet
Comment 5 2017-04-03 12:24:59 PDT
Any chance for some kind of test?
Eric Carlson
Comment 6 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.
Eric Carlson
Comment 7 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.
Eric Carlson
Comment 8 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.
Eric Carlson
Comment 9 2017-04-05 10:54:12 PDT
Created attachment 306298 [details] Patch for landing.
WebKit Commit Bot
Comment 10 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>
Note You need to log in before you can comment on or make changes to this bug.