Refactor WebVideoFullscreenController separating AVKit from MediaElement.
Created attachment 222405 [details] Patch
Comment on attachment 222405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222405&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:161 > UNUSED_PARAM(playerViewController); > UNUSED_PARAM(reason); > - > - WebThreadRun(^{ > - _mediaElement->pause(); > - _mediaElement->exitFullscreen(); > - }); > + self.delegate->requestExitFullScreen(); > return NO; Nit: it is probably worth having an ASSERT(self.delegate) here > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:167 > + self.delegate->play(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:173 > + self.delegate->pause(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:192 > + if (playing) > + self.delegate->play(); > + else > + self.delegate->pause(); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:202 > + self.delegate->seekToTime(time); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:209 > + :EventListener(EventListener::CPPEventListenerType) > + ,m_isListening{false} Nit: there should be a spaces between ':' & 'EventListener' and ',' & 'm_isListening' > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:231 > + if (m_mediaElement && !!m_borrowedVideoLayer) Nit: '!!' is unnecessary. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:251 > + if (m_videoFullscreenInterface) Nit: you can return early here if m_videoFullscreenInterface is NULL > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:273 > + Nit: added whitespace. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:280 > + || event->type() == eventNames().playEvent > + || event->type() == eventNames().ratechangeEvent) Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:342 > + :m_videoFullscreenModel(nullptr) Nit: you should have a space between ':' & 'm_videoFullscreenModel' > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:378 > + m_playerController.get().contentDuration = duration; > + m_playerController.get().maxTime = duration; > + m_playerController.get().contentDurationWithinEndTimes = duration; > + m_playerController.get().loadedTimeRanges = @[@0, @(duration)]; > + > + // FIXME: we take this as an indication that playback is ready. > + m_playerController.get().canPlay = YES; > + m_playerController.get().canPause = YES; > + m_playerController.get().canTogglePlayback = YES; > + m_playerController.get().hasEnabledAudio = YES; > + m_playerController.get().canSeek = YES; > + m_playerController.get().minTime = 0; > + m_playerController.get().status = AVPlayerControllerStatusReadyToPlay; Nit: you might as well have a local variable for m_playerController.get() since you use it so many times. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:385 > + AVValueTiming *timing = [classAVValueTiming valueTimingWithAnchorValue:currentTime > + anchorTimeStamp:anchorTimeStamp rate:0]; Nit: this indentation isn't helpful. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:404 > + [m_playerController.get().playerLayer removeFromSuperlayer]; > + [videoLayer removeFromSuperlayer]; > + m_playerController.get().playerLayer = (CALayer<AVPlayerLayer>*)videoLayer; Nit: you should either change this to take an AVPlayerLayer* or ASSERT that it is the correct class. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:432 > + enterFullscreen([](){}); Wow... > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:457 > + exitFullscreen([](){}); Ditto.
Comment on attachment 222405 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222405&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:46 > +// Changes here must be reflected in WebVideoFullscreenManagerProxy.messages.in I'd leave this comment out, or move it into the ChangeLog. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:47 > +class WebVideoFullscreenInterface { This class should go into its own header. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:61 > +// Changes here must be reflected in WebVideoFullscreenManager.messages.in > +class WebVideoFullscreenModel { Ditto about the comment and this class going into its own header. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:72 > +class WebVideoFullscreenModelMediaElement : public WebVideoFullscreenModel, public EventListener { This class (and implementation) can be pulled into their own files as well. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.h:95 > +class WebVideoFullscreenInterfaceAVKit : public WebVideoFullscreenInterface, public RefCounted<WebVideoFullscreenInterfaceAVKit> { Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:212 > + initAVPlayerLayer(); > + initAVValueTiming(); If this is really a MediaElement model, why does it have to init AVPlayerLayer and AVValueTiming? > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:293 > + ref(); > + WebThreadRun(^{ > + m_mediaElement->pause(); > + m_mediaElement->exitFullscreen(); > + deref(); > + }); The standard way to do this in WebCore is to use a RefPtr protector. I.e.: RefPtr<WebVideoFullscreenModelMediaElement> protect(this); WebThreadRun(^{ ... protect.clear(); }); That way, if the block exits early or is dealloced without being executed, the deref is called correctly. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:302 > +void WebVideoFullscreenModelMediaElement::play() { > + ref(); > + WebThreadRun(^{ > + m_mediaElement->play(); > + deref(); > + }); > +} Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:311 > +void WebVideoFullscreenModelMediaElement::pause() > +{ > + ref(); > + WebThreadRun(^{ > + m_mediaElement->pause(); > + deref(); > + }); > +} Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:320 > +void WebVideoFullscreenModelMediaElement::togglePlayState() > +{ > + ref(); > + WebThreadRun(^{ > + m_mediaElement->togglePlayState(); > + deref(); > + }); > +} Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:328 > + ref(); > + WebThreadRun(^{ > + m_mediaElement->setCurrentTime(time); > + deref(); > + }); Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:338 > + ref(); > + WebThreadRun(^{ > + setMediaElement(nullptr); > + deref(); > + }); > +} Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:354 > + m_playerController = [[[WebAVPlayerController alloc] init] autorelease]; This should be: m_playerController = adoptNS([[[WebAVPlayerController alloc] init]); > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:383 > + NSTimeInterval anchorTimeStamp = (![m_playerController.get() rate]) ? NAN : anchorTime; The parens are unnecessary here. > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:513 > + WebVideoFullscreenController* strongSelf = [self retain]; > + > + _interface->exitFullscreen([=](){ > + strongSelf->_model->setMediaElement(nullptr); > + strongSelf->_interface->setWebVideoFullscreenModel(nullptr); > + strongSelf->_model->setWebVideoFullscreenInterface(nullptr); > + strongSelf->_model = nullptr; > + strongSelf->_interface = nullptr; > + [strongSelf release]; Again, use a RetainPtr here (passing into the lambda by value) rather than manually retaining and releasing.
(In reply to comment #2) > (From update of attachment 222405 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=222405&action=review > > > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:432 > > + enterFullscreen([](){}); > > Wow... Such brace. Many bracket. Amaze.
Created attachment 222537 [details] Patch
Comment on attachment 222537 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222537&action=review > Source/WebCore/platform/ios/WebVideoFullscreenControllerAVKit.mm:84 > + WebVideoFullscreenController* strongSelf = [self retain]; > + This should use the RetainPtr<WebVideoFullscreenController> model. > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:186 > + else > + self.delegate->pause(); > + } Nit: Indentation. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:60 > +:EventListener(EventListener::CPPEventListenerType) > +,m_isListening{false} Nit: indentation > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:80 > + if (!!m_mediaElement && !!m_borrowedVideoLayer) { !! is not not required. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:136 > + ref(); > + WebThreadRun(^{ This should use RefPtr<WebVideoFullscreenModelMediaElement> strongSelf = this; > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:145 > + ref(); > + WebThreadRun(^{ Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:154 > + ref(); > + WebThreadRun(^{ Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:163 > + ref(); > + WebThreadRun(^{ Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:172 > + ref(); > + WebThreadRun(^{ Ditto. > Source/WebCore/platform/ios/WebVideoFullscreenModelMediaElement.mm:181 > + ref(); > + WebThreadRun(^{ Ditto.
Created attachment 222689 [details] Patch
Comment on attachment 222689 [details] Patch LGTM, r=me.
Comment on attachment 222689 [details] Patch Clearing flags on attachment: 222689 Committed r163097: <http://trac.webkit.org/changeset/163097>
All reviewed patches have been landed. Closing bug.
Comment on attachment 222689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=222689&action=review > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:128 > + self.playerControllerProxy = [[classAVPlayerController alloc] init]; I think this might be a leak. The property is a -retain property: @property(retain) AVPlayerController* playerControllerProxy; So assigning to it like this will retain the incoming object. And the -alloc/init is a +1 that is not balanced. I think this may need to be: self.playerControllerProxy = [[[classAVPlayerController alloc] init] autorelease]; Do you know how we can test this code patch to verify it is not leaking?
I think just a page with a video triggers this. I was able to reproduce. I will investigate.
Joe submitted a fix in https://bugs.webkit.org/show_bug.cgi?id=131878