Bug 134890 - WebVideoFullscreenInterfaceAVKit should only call the UI from main thread.
Summary: WebVideoFullscreenInterfaceAVKit should only call the UI from main thread.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-07-14 12:02 PDT by Jeremy Jones
Modified: 2014-07-14 15:32 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.49 KB, patch)
2014-07-14 12:19 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2014-07-14 12:02:33 PDT
WebVideoFullscreenInterfaceAVKit should only call the UI from main thread.
Comment 1 Jeremy Jones 2014-07-14 12:19:45 PDT
Created attachment 234872 [details]
Patch
Comment 2 Eric Carlson 2014-07-14 12:46:56 PDT
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.
Comment 3 Jeremy Jones 2014-07-14 12:59:31 PDT
<rdar://problem/16917699>
Comment 4 Jeremy Jones 2014-07-14 13:05:06 PDT
(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.
Comment 5 WebKit Commit Bot 2014-07-14 15:32:28 PDT
Comment on attachment 234872 [details]
Patch

Clearing flags on attachment: 234872

Committed r171089: <http://trac.webkit.org/changeset/171089>
Comment 6 WebKit Commit Bot 2014-07-14 15:32:30 PDT
All reviewed patches have been landed.  Closing bug.