[video_player_avfoundation] fix playback speed resetting #7657
Conversation
misos1
changed the title
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
Outdated
Show resolved
Hide resolved
|
Sorry, this fell off my review queue. @hellohuanlin can you take a look as well? |
| // status changes to AVPlayerItemStatusReadyToPlay. | ||
| float speed = _playbackSpeed.floatValue; | ||
| if (speed > 2.0 && !_player.currentItem.canPlayFastForward) { | ||
| if (_player.currentItem.status != AVPlayerItemStatusReadyToPlay) { |
There was a problem hiding this comment.
what if we call updateRate with speed < 2.0? should we also check the status here? Should this status check be moved to top of the fucntion?
There was a problem hiding this comment.
That check is to confirm whether canPlay... returned false because it cannot be played at speed>2 instead of because status was not ready to play. There is no need to check status for speeds for which is not needed canPlay.... But as I am thinking about it now maybe status should be fetched before canPlay... to avoid a false positive when status becomes ready right after canPlay....
| @@ -397,7 +399,15 @@ - (void)updatePlayingState { | |||
| return; | |||
| } | |||
| if (_isPlaying) { | |||
| [_player play]; | |||
| // Calling play is the same as setting the rate to 1.0 (or to defaultRate depending on ios | |||
| // be played at these speeds, updatePlayingState will be called again when | ||
| // status changes to AVPlayerItemStatusReadyToPlay. | ||
| float speed = _playbackSpeed.floatValue; | ||
| bool readyToPlay = _player.currentItem.status == AVPlayerItemStatusReadyToPlay; |
There was a problem hiding this comment.
BOOL since this is Obj-C code.
| @@ -82,6 +82,8 @@ @interface FVPVideoPlayer () | |||
| @property(nonatomic) CGAffineTransform preferredTransform; | |||
| @property(nonatomic, readonly) BOOL disposed; | |||
| @property(nonatomic, readonly) BOOL isPlaying; | |||
| // Playback speed when video is playing. | |||
| @property(nonatomic, readonly) NSNumber *playbackSpeed; | |||
There was a problem hiding this comment.
It would be more accurate to call this targetPlaybackSpeed (and document it as something like "The target playback speed requested by the plugin client."), since it's entirely possible for the playback speed to not be this value while playing.
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
Outdated
Show resolved
Hide resolved
|
@misos1 are you still working on this patch? |
Yes. |
|
I'm still getting the same unexpected behavior when I play a video. The playback speed resets to 1.0x when I seek anywhere in the video. |
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
Outdated
Show resolved
Hide resolved
| float speed = _playbackSpeed.floatValue; | ||
| bool readyToPlay = _player.currentItem.status == AVPlayerItemStatusReadyToPlay; | ||
| if (speed > 2.0 && !_player.currentItem.canPlayFastForward) { | ||
| if (!readyToPlay) { |
There was a problem hiding this comment.
I'm still confused why this check can't be moved to the top of the function? What if readyToPlay is false, and we set the speed to a value between 1.0 and 2.0? Why are we allowing setting the speed for this value but not setting the speed for clamped value?
There was a problem hiding this comment.
What if readyToPlay is false, and we set the speed to a value between 1.0 and 2.0?
Everything is fine then, the rate can be set anytime.
Why are we allowing setting the speed for this value but not setting the speed for clamped value?
To avoid setting it to a value which was not requested and seems each change to rate triggers re-buffering and playbackLikelyToKeepUp as is stated in description.
I'm still confused why this check can't be moved to the top of the function?
I chose to do it strictly only when needed. So you probably argue to do it always due to simplicity maybe. But I believe setting up the rate before ReadyToPlay may avoid some re-buffering as mentioned above.
There was a problem hiding this comment.
Why are we allowing setting the speed for this value but not setting the speed for clamped value?
The problem isn't that we can't set the speed while buffering, it's that we don't know whether we need to clamp while buffering. But for 1.0-2.0 it's irrelevant because clamping wouldn't apply anyway.
(There is an unfortunate side-effect of this PR in that setting at unsupported value now silently clamps instead of throwing an exception, adding another potential way for native and Dart to get out of sync about state since Dart will think it's successfully set the out-of-range value, but we can address that edge case later with more event channel communication.)
flutter/packages@f73cb00...e8f1f63 2025-01-20 mchudy@users.noreply.github.com [in_app_purchase] Update Play Billing library to 7.1.1 (flutter/packages#8218) 2025-01-20 engine-flutter-autoroll@skia.org Roll Flutter from 5517cc9 to b9e86a5 (22 revisions) (flutter/packages#8458) 2025-01-18 stuartmorgan@google.com [pigeon] Update analyzer and formatter (flutter/packages#8456) 2025-01-17 49699333+dependabot[bot]@users.noreply.github.com [dependabot]: Bump the gradle-plugin group across 3 directories with 1 update (flutter/packages#8328) 2025-01-17 magder@google.com [local_auth_darwin] Handle when FaceID hardware is available but permissions have been denied for the app (flutter/packages#8348) 2025-01-16 engine-flutter-autoroll@skia.org Roll Flutter from 40c2b86 to 5517cc9 (28 revisions) (flutter/packages#8441) 2025-01-15 louisehsu@google.com [in_app_purchase_storekit] expose `jsonRepresentation` for Transactions (flutter/packages#8430) 2025-01-15 engine-flutter-autoroll@skia.org Roll Flutter (stable) from 17025dd to 68415ad (4 revisions) (flutter/packages#8434) 2025-01-15 30872003+misos1@users.noreply.github.com [video_player_avfoundation] fix playback speed resetting (flutter/packages#7657) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC flutter-ecosystem@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Calling play is the same as setting the rate to 1.0 (or to
defaultRatedepending on ios version, documentation in this first link is clearly not updated because it does not mentiondefaultRate):https://developer.apple.com/documentation/avfoundation/avplayer/1386726-play?language=objc
https://developer.apple.com/documentation/avfoundation/avplayer/3929373-defaultrate?language=objc
The second link contains a note about not starting playback by setting the rate to 1.0. I assume this is because of the introduction of
defaultRate(which can be different than 1.0) and not becauseplaymay do something more than just settingrateas that wording is explicit about setting rate to 1.0, it says nothing about any other value.This is also why flutter/plugins#4331 did not work well. It was setting
rateto 1.0 (throughplay) then immediately to the value set bysetPlaybackSpeed. One of them or both caused change toplaybackLikelyToKeepUpwhich triggeredobserveValueForKeyPathwithplaybackLikelyToKeepUpContextwhich in turn called againupdatePlayingStatewhere wasrateagain changed first to 1.0 then to another value and so on. In this PR the rate is changed only once and then to the same value, seems whenrateis assigned but not really changed it does not trigger anything.In issues below
seekTocan triggerplaybackLikelyToKeepUpchange which will callupdatePlayingStateand resetrateto 1.0 throughplay. When the speed is set in dart before initialization then the dart side will set it right after callingplaybut even some time after initialization there is still a flood of events callingupdatePlayingStateand it is likely that some of them will call it aftersetPlaybackSpeed. Actually even whensetPlaybackSpeedwas not called on dart side it (dart side) will always change speed after play to 1.0 so it ignores this newdefaultRatefeature.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.