Summary: | [iOS] Fullscreen video playback gets stuck after interacting with the playback controls | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Peng Liu <peng.liu6> | ||||||||||
Component: | Media | Assignee: | Peng Liu <peng.liu6> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | eric.carlson, ews-watchlist, glenn, hi, jer.noble, philipj, sergio, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=227106 | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 227106 | ||||||||||||
Attachments: |
|
Description
Peng Liu
2021-06-15 14:37:54 PDT
Created attachment 431486 [details]
Patch
Created attachment 431487 [details]
Patch
Comment on attachment 431486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431486&action=review > Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105 > + virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { } Could we maybe have an `OptionSet` of enum flags? > Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:-55 > -static const float StalledPlaybackRate = 0.00000001f; Do we still need to report this value (or something like it) to AVKit? Or will AVKit be fine with a playback rate of 1 (or 2 or 0.5 or whatever) when we're still loading? > Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm:130 > + if (!isStalled) I wonder if instead of not modifying `m_playerController` we should instead have a `stalled` property on `WebAVPlayerController` and have `setRate:` ignore/defer modifications if `stalled`. This way, if AVKit calls `setRate:` then we will wait to update the rate until we're no longer stalled (e.g. inside `setStalled:` you could send the `rate` back to the `model`). Though I think all of this assumes my comment on PlaybackSessionModelMediaElement.mm:55 is accurate. Created attachment 431494 [details]
Fix build failures on macOS
Comment on attachment 431486 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=431486&action=review >> Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105 >> + virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { } > > Could we maybe have an `OptionSet` of enum flags? Yes. Like an `OptionSet` of ``` enum class PlaybackState { Playing = 1 << 0, Stalled = 1 << 1, }; ``` >> Source/WebCore/platform/cocoa/PlaybackSessionModelMediaElement.mm:-55 >> -static const float StalledPlaybackRate = 0.00000001f; > > Do we still need to report this value (or something like it) to AVKit? Or will AVKit be fine with a playback rate of 1 (or 2 or 0.5 or whatever) when we're still loading? With this patch, I cannot reproduce bug 211797. So looks like AVKit does not need this value anymore. >> Source/WebCore/platform/ios/PlaybackSessionInterfaceAVKit.mm:130 >> + if (!isStalled) > > I wonder if instead of not modifying `m_playerController` we should instead have a `stalled` property on `WebAVPlayerController` and have `setRate:` ignore/defer modifications if `stalled`. This way, if AVKit calls `setRate:` then we will wait to update the rate until we're no longer stalled (e.g. inside `setStalled:` you could send the `rate` back to the `model`). Though I think all of this assumes my comment on PlaybackSessionModelMediaElement.mm:55 is accurate. Hmm, this approach sounds safer. However, if `m_playerController` does not report the rate back to the WebContent process when its `stalled` property is YES, its rate might be overwritten by a value comes from the WebContent process. That could happen when a user changes playback rate in fullscreen immediately after scrubbing the video. Comment on attachment 431494 [details] Fix build failures on macOS View in context: https://bugs.webkit.org/attachment.cgi?id=431494&action=review > Source/WebCore/platform/cocoa/PlaybackSessionModel.h:105 > + virtual void rateChanged(bool /* isPlaying */, bool /* isStalled */, double /* playbackRate */, double /* defaultPlaybackRate */) { } I agree that an enums and an OptionSet would be nicer than bools Created attachment 431727 [details]
Revise the patch based on Devin and Eric's comments
Committed r279043 (238963@main): <https://commits.webkit.org/238963@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 431727 [details]. *** Bug 227106 has been marked as a duplicate of this bug. *** |