[video_player_avfoundation] fix playback speed resetting #7657

misos1 · 2024-09-17T19:34:00Z

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 mention defaultRate):

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 because play may do something more than just setting rate 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 (through play) then immediately to the value set by setPlaybackSpeed. One of them or both caused change to playbackLikelyToKeepUp which triggered observeValueForKeyPath with playbackLikelyToKeepUpContext which in turn called again updatePlayingState where was rate 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 when rate is assigned but not really changed it does not trigger anything.

In issues below seekTo can trigger playbackLikelyToKeepUp change which will call updatePlayingState and reset rate to 1.0 through play. When the speed is set in dart before initialization then the dart side will set it right after calling play but even some time after initialization there is still a flood of events calling updatePlayingState and it is likely that some of them will call it after setPlaybackSpeed. Actually even when setPlaybackSpeed was not called on dart side it (dart side) will always change speed after play to 1.0 so it ignores this new defaultRate feature.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

stuartmorgan · 2024-10-08T19:46:38Z

Sorry, this fell off my review queue. @hellohuanlin can you take a look as well?

hellohuanlin · 2024-10-08T21:08:44Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+  // status changes to AVPlayerItemStatusReadyToPlay.
+  float speed = _playbackSpeed.floatValue;
+  if (speed > 2.0 && !_player.currentItem.canPlayFastForward) {
+    if (_player.currentItem.status != AVPlayerItemStatusReadyToPlay) {


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?

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

stuartmorgan

The logic and tests all LGTM, just small comments on naming and style guide conformance.

stuartmorgan · 2024-11-06T19:58:17Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -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


stuartmorgan · 2024-11-06T20:00:26Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+  // 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;


BOOL since this is Obj-C code.

stuartmorgan · 2024-11-06T20:03:02Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

@@ -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;


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

cbracken · 2024-11-20T22:15:25Z

@misos1 are you still working on this patch?

misos1 · 2024-11-21T17:10:51Z

@misos1 are you still working on this patch?

Yes.

stuartmorgan

LGTM!

sha3rawi33 · 2024-11-26T22:40:06Z

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.

hellohuanlin

Sorry I realized that I wrote the comments a long time ago but didn't actually hit the submit button!

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

hellohuanlin · 2024-10-15T19:20:07Z

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m

+  float speed = _playbackSpeed.floatValue;
+  bool readyToPlay = _player.currentItem.status == AVPlayerItemStatusReadyToPlay;
+  if (speed > 2.0 && !_player.currentItem.canPlayFastForward) {
+    if (!readyToPlay) {


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?

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.

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

stuartmorgan · 2024-12-06T16:16:33Z

This will just need conflicts resolved against the code location refactoring, and then it can be landed!

misos1 · 2024-12-11T20:20:54Z

I wanted to synchronize my branch with upstream, but this time there was no "sync" button. Instead, there was an option about discarding some commits. When I clicked it, the PR closed without any confirmation or warning. Can this be reopened?

hellohuanlin · 2024-12-11T21:13:06Z

I can't re-open it either. You may have to create a new PR.

stuartmorgan · 2024-12-11T21:20:52Z

This happened once before with another PR; I was never able to figure out what caused GitHub to put it into this state, or why :(

misos1 · 2024-12-12T18:16:45Z

Nice, after I pushed new changes it was possible to reopen.

Now I am planning to check that problem with defaultRate on ios 16 (as I now have more info about it), so please wait.

misos1 · 2024-12-18T18:50:32Z

PR #8274 states that rate is set to defaultRate on seeking (#8274 (comment)) and references a link to documentation about defaultRate, but the provided link does not say anything like that and I cannot reproduce such behavior either (and it also sounds really unlikely). More likely is that what was observed by the author is what is explained in the description of this PR, seeking triggers re-buffering which triggers observeValueForKeyPath with playbackLikelyToKeepUpContext which calls updatePlayingState which calls AVPlayer.play which sets rate to defaultRate as is stated in the mentioned link.

sha3rawi33 · 2024-12-19T04:44:43Z

PR #8274 states that rate is set to defaultRate on seeking (#8274 (comment)) and references a link to documentation about defaultRate, but the provided link does not say anything like that and I cannot reproduce such behavior either (and it also sounds really unlikely). More likely is that what was observed by the author is what is explained in the description of this PR, seeking triggers re-buffering which triggers observeValueForKeyPath with playbackLikelyToKeepUpContext which calls updatePlayingState which calls AVPlayer.play which sets rate to defaultRate as is stated in the mentioned link.

Yeah, that's exactly what is causing the issue to arise; and setting defaultRate to the preferred playback rate prevents it from happening. I tested this on my apps on production and all went well. (Also native tests pass)
Also please note that it is mentioned in the link I provided in my PR. See the attached image.
it says:
The next time you call the play method, the player restores the rate to the value of defaultRate.
Which happens on seeking or rebuffering causing the issue.

misos1 · 2024-12-19T17:22:07Z

Also please note that it is mentioned in the link I provided in my PR. See the attached image.
it says:
The next time you call the play method, the player restores the rate to the value of defaultRate.
Which happens on seeking or rebuffering causing the issue.

Where does that documentation say that play is called (or rate is set to defaultRate) internally (meaning not through FVPVideoPlayer) on seeking or rebuffering? Where in that link is that mentioned?

github-actions bot added platform-ios platform-macos p: video_player labels Sep 17, 2024

misos1 force-pushed the speed_reset branch from c5acab1 to aa17e65 Compare September 17, 2024 20:04

fix playback speed resetting

a75e906

misos1 force-pushed the speed_reset branch from aa17e65 to a75e906 Compare September 18, 2024 15:35

misos1 changed the title ~~[video_player_avfoundation, video_player] fix playback speed resetting~~ [video_player_avfoundation] fix playback speed resetting Sep 18, 2024

misos1 marked this pull request as ready for review September 18, 2024 16:14

misos1 requested a review from hellohuanlin as a code owner September 18, 2024 16:14

stuartmorgan reviewed Sep 19, 2024

View reviewed changes

...on/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayerPlugin.m Outdated Show resolved Hide resolved

hellohuanlin reviewed Oct 8, 2024

View reviewed changes

misos1 added 2 commits October 13, 2024 20:09

fetch status before canPlay calls

9de9700

Merge branch 'main' into speed_reset

1916de5

stuartmorgan requested changes Nov 6, 2024

View reviewed changes

stuartmorgan added the triage-ios Should be looked at in iOS triage label Nov 8, 2024

misos1 added 6 commits November 22, 2024 16:48

change few names and comments

a1e0361

update version

ba86187

update version

0c74a20

Merge branch 'main' into speed_reset

7755c4f

change comment

e9716fc

Merge branch 'main' into speed_reset

785d53a

misos1 requested a review from stuartmorgan November 22, 2024 18:47

stuartmorgan approved these changes Nov 25, 2024

View reviewed changes

misos1 requested a review from hellohuanlin November 25, 2024 16:06

hellohuanlin reviewed Dec 3, 2024

View reviewed changes

misos1 requested a review from hellohuanlin December 4, 2024 19:26

hellohuanlin approved these changes Dec 5, 2024

View reviewed changes

misos1 closed this Dec 11, 2024

misos1 force-pushed the speed_reset branch from 785d53a to 8238e1c Compare December 11, 2024 20:11

stuartmorgan mentioned this pull request Dec 11, 2024

[video_player_avfoundation] fix video playback speed resetting on iOS 16+ #8274

Open

Merge branch 'main' into speed_reset

39a189d

misos1 reopened this Dec 12, 2024

Merge branch 'main' into speed_reset

f9121b2

Merge branch 'main' into speed_reset

5c105a1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player_avfoundation] fix playback speed resetting #7657

[video_player_avfoundation] fix playback speed resetting #7657

misos1 commented Sep 17, 2024 •

edited

Loading

stuartmorgan commented Oct 8, 2024

hellohuanlin Oct 8, 2024

misos1 Oct 13, 2024

stuartmorgan left a comment

stuartmorgan Nov 6, 2024

stuartmorgan Nov 6, 2024

stuartmorgan Nov 6, 2024

cbracken commented Nov 20, 2024

misos1 commented Nov 21, 2024

stuartmorgan left a comment

sha3rawi33 commented Nov 26, 2024

hellohuanlin left a comment

hellohuanlin Oct 15, 2024

misos1 Dec 4, 2024

stuartmorgan Dec 5, 2024

stuartmorgan commented Dec 6, 2024

misos1 commented Dec 11, 2024

hellohuanlin commented Dec 11, 2024

stuartmorgan commented Dec 11, 2024

misos1 commented Dec 12, 2024 •

edited

Loading

misos1 commented Dec 18, 2024

sha3rawi33 commented Dec 19, 2024

misos1 commented Dec 19, 2024

[video_player_avfoundation] fix playback speed resetting #7657

Are you sure you want to change the base?

[video_player_avfoundation] fix playback speed resetting #7657

Conversation

misos1 commented Sep 17, 2024 • edited Loading

Pre-launch Checklist

stuartmorgan commented Oct 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbracken commented Nov 20, 2024

misos1 commented Nov 21, 2024

stuartmorgan left a comment

Choose a reason for hiding this comment

sha3rawi33 commented Nov 26, 2024

hellohuanlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Dec 6, 2024

misos1 commented Dec 11, 2024

hellohuanlin commented Dec 11, 2024

stuartmorgan commented Dec 11, 2024

misos1 commented Dec 12, 2024 • edited Loading

misos1 commented Dec 18, 2024

sha3rawi33 commented Dec 19, 2024

misos1 commented Dec 19, 2024

misos1 commented Sep 17, 2024 •

edited

Loading

misos1 commented Dec 12, 2024 •

edited

Loading