Skip to content
Prev Previous commit
Next Next commit
change few names and comments
  • Loading branch information
misos1 committed Nov 22, 2024
commit a1e0361b3814259ac5afcf752e320c45d64196d0
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +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;
// The target playback speed requested by the plugin client.
@property(nonatomic, readonly) NSNumber *targetPlaybackSpeed;
@property(nonatomic) BOOL isLooping;
@property(nonatomic, readonly) BOOL isInitialized;
// The updater that drives callbacks to the engine to indicate that a new frame is ready.
Expand Down Expand Up @@ -399,11 +399,11 @@ - (void)updatePlayingState {
return;
}
if (_isPlaying) {
// Calling play is the same as setting the rate to 1.0 (or to defaultRate depending on ios
// Calling play is the same as setting the rate to 1.0 (or to defaultRate depending on iOS
// version) so last set playback speed must be set here if any instead.
// https://github.com/flutter/flutter/issues/71264
// https://github.com/flutter/flutter/issues/73643
if (_playbackSpeed) {
if (_targetPlaybackSpeed) {
[self updateRate];
} else {
[_player play];
Expand All @@ -416,15 +416,16 @@ - (void)updatePlayingState {
_displayLink.running = _isPlaying || self.waitingForFrame;
}

/// Synchronizes the player's playback rate with targetPlaybackSpeed, constrained by the playback rate capabilities of the player.
- (void)updateRate {
// See https://developer.apple.com/library/archive/qa/qa1772/_index.html for an explanation of
// these checks.
// If status is not AVPlayerItemStatusReadyToPlay then both canPlayFastForward
// and canPlaySlowForward are always false and it is unknown whether video can
// 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;
float speed = _targetPlaybackSpeed.floatValue;
BOOL readyToPlay = _player.currentItem.status == AVPlayerItemStatusReadyToPlay;
if (speed > 2.0 && !_player.currentItem.canPlayFastForward) {
if (!readyToPlay) {
Copy link
Contributor

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

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

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;
Expand Down Expand Up @@ -553,7 +554,7 @@ - (void)setVolume:(double)volume {
}

- (void)setPlaybackSpeed:(double)speed {
_playbackSpeed = @(speed);
_targetPlaybackSpeed = @(speed);
[self updatePlayingState];
}

Expand Down