Skip to content

[video_player_avfoundation] fix playback speed resetting #7657

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.

Sign up for GitHub

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

Merged
auto-submit merged 14 commits into from
Jan 15, 2025
Merged

[video_player_avfoundation] fix playback speed resetting #7657

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a75e906
fix playback speed resetting
misos1 Sep 17, 2024
9de9700
fetch status before canPlay calls
misos1 Oct 13, 2024
1916de5
Merge branch 'main' into speed_reset
misos1 Oct 13, 2024
a1e0361
change few names and comments
misos1 Nov 22, 2024
ba86187
update version
misos1 Nov 22, 2024
0c74a20
update version
misos1 Nov 22, 2024
7755c4f
Merge branch 'main' into speed_reset
misos1 Nov 22, 2024
e9716fc
change comment
misos1 Nov 22, 2024
785d53a
Merge branch 'main' into speed_reset
misos1 Nov 22, 2024
39a189d
Merge branch 'main' into speed_reset
misos1 Dec 12, 2024
f9121b2
Merge branch 'main' into speed_reset
misos1 Dec 18, 2024
5c105a1
Merge branch 'main' into speed_reset
misos1 Dec 19, 2024
45f49c3
Re-bump version
stuartmorgan-g Jan 14, 2025
fed2d90
Merge branch 'main' into speed_reset
stuartmorgan-g Jan 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fetch status before canPlay calls
  • Loading branch information
@misos1
misos1 committed Oct 13, 2024
commit 9de97006a0c17e6a50a88ed75c486d81d9c6ee33
2 changes: 1 addition & 1 deletion packages/video_player/video_player_avfoundation/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
## 2.6.2
## 2.6.3

* Fixes playback speed resetting.
* Updates minimum supported SDK version to Flutter 3.19/Dart 3.3.
Expand Down
5 changes: 3 additions & 2 deletions ...darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -424,14 +424,15 @@ - (void)updateRate {
// 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;
Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Nov 6, 2024

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.

if (speed > 2.0 && !_player.currentItem.canPlayFastForward) {
if (_player.currentItem.status != AVPlayerItemStatusReadyToPlay) {
if (!readyToPlay) {
Copy link
Contributor

@hellohuanlin hellohuanlin Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@misos1 misos1 Dec 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

return;
}
speed = 2.0;
}
if (speed < 1.0 && !_player.currentItem.canPlaySlowForward) {
if (_player.currentItem.status != AVPlayerItemStatusReadyToPlay) {
if (!readyToPlay) {
return;
}
speed = 1.0;
Expand Down
2 changes: 1 addition & 1 deletion packages/video_player/video_player_avfoundation/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ name: video_player_avfoundation
description: iOS and macOS implementation of the video_player plugin.
repository: https://github.com/flutter/packages/tree/main/packages/video_player/video_player_avfoundation
issue_tracker: https://github.com/flutter/flutter/issues?q=is%3Aissue+is%3Aopen+label%3A%22p%3A+video_player%22
version: 2.6.2
version: 2.6.3

environment:
sdk: ^3.3.0
Expand Down