-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[video_player_avfoundation] fix playback speed resetting #7657
base: main
Are you sure you want to change the base?
Conversation
...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic and tests all LGTM, just small comments on naming and style guide conformance.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Show resolved
Hide resolved
@misos1 are you still working on this patch? |
Yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
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. |
Calling play is the same as setting the rate to 1.0 (or to
defaultRate
depending 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 becauseplay
may do something more than just settingrate
as 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
rate
to 1.0 (throughplay
) then immediately to the value set bysetPlaybackSpeed
. One of them or both caused change toplaybackLikelyToKeepUp
which triggeredobserveValueForKeyPath
withplaybackLikelyToKeepUpContext
which in turn called againupdatePlayingState
where wasrate
again 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 whenrate
is assigned but not really changed it does not trigger anything.In issues below
seekTo
can triggerplaybackLikelyToKeepUp
change which will callupdatePlayingState
and resetrate
to 1.0 throughplay
. When the speed is set in dart before initialization then the dart side will set it right after callingplay
but even some time after initialization there is still a flood of events callingupdatePlayingState
and it is likely that some of them will call it aftersetPlaybackSpeed
. Actually even whensetPlaybackSpeed
was not called on dart side it (dart side) will always change speed after play to 1.0 so it ignores this newdefaultRate
feature.Pre-launch Checklist
dart format
.)[shared_preferences]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.md
to 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.