Summary: | WebVideoFullscreenInterfaceAVKit should only call the UI from main thread. | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||
Component: | Media | Assignee: | Jeremy Jones <jeremyj-wk> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | commit-queue, eric.carlson, glenn, jer.noble, philipj, sergio | ||||
Priority: | P2 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | iPhone / iPad | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Jeremy Jones
2014-07-14 12:02:33 PDT
Created attachment 234872 [details]
Patch
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. (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 on attachment 234872 [details] Patch Clearing flags on attachment: 234872 Committed r171089: <http://trac.webkit.org/changeset/171089> All reviewed patches have been landed. Closing bug. |