Bug 134890 – WebVideoFullscreenInterfaceAVKit should only call the UI from main thread.

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Jeremy Jones]

WebVideoFullscreenInterfaceAVKit should only call the UI from main thread.

[Jeremy Jones]

Created attachment 234872 [details]
Patch

[Eric Carlson]

Comment on attachment 234872 [details]
Patch

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

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:592
> +        if (m_playerController)
> +            playerController().delegate = m_videoFullscreenModel;

Why use both m_playerController and playerController()?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> +        playerController.contentDuration = duration;
> +        playerController.maxTime = duration;
> +        playerController.contentDurationWithinEndTimes = duration;
> +        playerController.loadedTimeRanges = @[@0, @(duration)];
> +        
> +        // FIXME: we take this as an indication that playback is ready.
> +        playerController.canPlay = YES;
> +        playerController.canPause = YES;
> +        playerController.canTogglePlayback = YES;
> +        playerController.hasEnabledAudio = YES;
> +        playerController.canSeek = YES;
> +        playerController.minTime = 0;
> +        playerController.status = AVPlayerControllerStatusReadyToPlay;

In setWebVideoFullscreenModel you test to ensure that m_playerController isn't NULL. Why is it unnecessary here?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:637
> +        NSTimeInterval anchorTimeStamp = ![playerController() rate] ? NAN : anchorTime;
> +        AVValueTiming *timing = [getAVValueTimingClass() valueTimingWithAnchorValue:currentTime
> +            anchorTimeStamp:anchorTimeStamp rate:0];
> +        playerController().timing = timing;

Ditto. Also, why does this code use playerController() when above you use playerController.?

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:648
> +        playerController().rate = isPlaying ? playbackRate : 0.;

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:660
> +        playerController().hasEnabledVideo = hasVideo;
> +        playerController().contentDimensions = CGSizeMake(width, height);

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:682
> +        playerController().seekableTimeRanges = seekableRanges;

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:707
> +        playerController().audioMediaSelectionOptions = webOptions;
> +        if (selectedIndex < webOptions.count)
> +            playerController().currentAudioMediaSelectionOption = webOptions[(size_t)selectedIndex];

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:721
> +        playerController().legibleMediaSelectionOptions = webOptions;
> +        if (selectedIndex < webOptions.count)
> +            playerController().currentLegibleMediaSelectionOption = webOptions[(size_t)selectedIndex];

Ditto.

> Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:740
> +        playerController().externalPlaybackAirPlayDeviceLocalizedName = localizedDeviceName;
> +        playerController().externalPlaybackType = externalPlaybackType;
> +        playerController().externalPlaybackActive = enabled;

Ditto.

[Jeremy Jones]

<rdar://problem/16917699>

[Jeremy Jones]

(In reply to comment #2)
> (From update of attachment 234872 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=234872&action=review
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:592
> > +        if (m_playerController)
> > +            playerController().delegate = m_videoFullscreenModel;
> 
> Why use both m_playerController and playerController()?
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:623
> > +        playerController.contentDuration = duration;
> > +        playerController.maxTime = duration;
> > +        playerController.contentDurationWithinEndTimes = duration;
> > +        playerController.loadedTimeRanges = @[@0, @(duration)];
> > +        
> > +        // FIXME: we take this as an indication that playback is ready.
> > +        playerController.canPlay = YES;
> > +        playerController.canPause = YES;
> > +        playerController.canTogglePlayback = YES;
> > +        playerController.hasEnabledAudio = YES;
> > +        playerController.canSeek = YES;
> > +        playerController.minTime = 0;
> > +        playerController.status = AVPlayerControllerStatusReadyToPlay;
> 
> In setWebVideoFullscreenModel you test to ensure that m_playerController isn't NULL. Why is it unnecessary here?
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:637
> > +        NSTimeInterval anchorTimeStamp = ![playerController() rate] ? NAN : anchorTime;
> > +        AVValueTiming *timing = [getAVValueTimingClass() valueTimingWithAnchorValue:currentTime
> > +            anchorTimeStamp:anchorTimeStamp rate:0];
> > +        playerController().timing = timing;
> 
> Ditto. Also, why does this code use playerController() when above you use playerController.?
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:648
> > +        playerController().rate = isPlaying ? playbackRate : 0.;
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:660
> > +        playerController().hasEnabledVideo = hasVideo;
> > +        playerController().contentDimensions = CGSizeMake(width, height);
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:682
> > +        playerController().seekableTimeRanges = seekableRanges;
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:707
> > +        playerController().audioMediaSelectionOptions = webOptions;
> > +        if (selectedIndex < webOptions.count)
> > +            playerController().currentAudioMediaSelectionOption = webOptions[(size_t)selectedIndex];
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:721
> > +        playerController().legibleMediaSelectionOptions = webOptions;
> > +        if (selectedIndex < webOptions.count)
> > +            playerController().currentLegibleMediaSelectionOption = webOptions[(size_t)selectedIndex];
> 
> Ditto.
> 
> > Source/WebCore/platform/ios/WebVideoFullscreenInterfaceAVKit.mm:740
> > +        playerController().externalPlaybackAirPlayDeviceLocalizedName = localizedDeviceName;
> > +        playerController().externalPlaybackType = externalPlaybackType;
> > +        playerController().externalPlaybackActive = enabled;
> 
> Ditto.

So long as playerController() is called before m_playerController is used, this are fine.

Filed https://bugs.webkit.org/show_bug.cgi?id=134895 for consistent use of playerController() and m_playerController.

[WebKit Commit Bot]

Comment on attachment 234872 [details]
Patch

Clearing flags on attachment: 234872

Committed r171089: <http://trac.webkit.org/changeset/171089>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.