Bug 127762 – Refactor WebVideoFullscreenController separating AVKit from MediaElement.

• Component: New Bugs
• Status: RESOLVED FIXED

[Jeremy Jones]

Refactor WebVideoFullscreenController separating AVKit from MediaElement.

[Jeremy Jones]

Created attachment 222405 [details]
Patch

[Eric Carlson]

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.

[Jer Noble]

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.

[Jer Noble]

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.

[Jer Noble]

(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.

[Jeremy Jones]

Created attachment 222537 [details]
Patch

[Jer Noble]

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.

[Jeremy Jones]

Created attachment 222689 [details]
Patch

[Jer Noble]

Comment on attachment 222689 [details]
Patch

LGTM, r=me.

[WebKit Commit Bot]

Comment on attachment 222689 [details]
Patch

Clearing flags on attachment: 222689

Committed r163097: <http://trac.webkit.org/changeset/163097>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

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?

[Joseph Pecoraro]

I think just a page with a video triggers this. I was able to reproduce. I will investigate.

[Eric Carlson]

Joe submitted a fix in https://bugs.webkit.org/show_bug.cgi?id=131878