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.

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
merged 14 commits into from
Jan 15, 2025
Merged
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 2.6.3

* Fixes playback speed resetting.

## 2.6.2

* Updates Pigeon for non-nullable collection type support.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,8 @@ - (void)testSeekToleranceWhenSeekingToEnd {
// Change playback speed.
[videoPlayerPlugin setPlaybackSpeed:2 forPlayer:textureId.integerValue error:&error];
XCTAssertNil(error);
[videoPlayerPlugin playPlayer:textureId.integerValue error:&error];
XCTAssertNil(error);
XCTAssertEqual(avPlayer.rate, 2);
XCTAssertEqual(avPlayer.timeControlStatus, AVPlayerTimeControlStatusWaitingToPlayAtSpecifiedRate);

Expand Down Expand Up @@ -790,6 +792,41 @@ - (void)testPublishesInRegistration {
XCTAssertTrue([publishedValue isKindOfClass:[FVPVideoPlayerPlugin class]]);
}

- (void)testUpdatePlayingStateShouldNotResetRate {
NSObject<FlutterPluginRegistrar> *registrar =
[GetPluginRegistry() registrarForPlugin:@"testUpdatePlayingStateShouldNotResetRate"];

FVPVideoPlayerPlugin *videoPlayerPlugin = [[FVPVideoPlayerPlugin alloc]
initWithAVFactory:[[StubFVPAVFactory alloc] initWithPlayer:nil output:nil]
displayLinkFactory:nil
registrar:registrar];

FlutterError *error;
[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);
FVPCreationOptions *create = [FVPCreationOptions
makeWithAsset:nil
uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4"
packageName:nil
formatHint:nil
httpHeaders:@{}];
NSNumber *textureId = [videoPlayerPlugin createWithOptions:create error:&error];
FVPVideoPlayer *player = videoPlayerPlugin.playersByTextureId[textureId];

XCTestExpectation *initializedExpectation = [self expectationWithDescription:@"initialized"];
[player onListenWithArguments:nil
eventSink:^(NSDictionary<NSString *, id> *event) {
if ([event[@"event"] isEqualToString:@"initialized"]) {
[initializedExpectation fulfill];
}
}];
[self waitForExpectationsWithTimeout:10 handler:nil];

[videoPlayerPlugin setPlaybackSpeed:2 forPlayer:textureId.integerValue error:&error];
[videoPlayerPlugin playPlayer:textureId.integerValue error:&error];
XCTAssertEqual(player.player.rate, 2);
}

#if TARGET_OS_IOS
- (void)validateTransformFixForOrientation:(UIImageOrientation)orientation {
AVAssetTrack *track = [[FakeAVAssetTrack alloc] initWithOrientation:orientation];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

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.

@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 @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: 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) {
[self updateRate];
} else {
[_player play];
}
} else {
[_player pause];
}
Expand All @@ -406,6 +416,30 @@ - (void)updatePlayingState {
_displayLink.running = _isPlaying || self.waitingForFrame;
}

- (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;
Copy link
Contributor

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 (!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
Contributor

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 (!readyToPlay) {
return;
}
speed = 1.0;
}
_player.rate = speed;
}

- (void)setupEventSinkIfReadyToPlay {
if (_eventSink && !_isInitialized) {
AVPlayerItem *currentItem = self.player.currentItem;
Expand Down Expand Up @@ -519,27 +553,8 @@ - (void)setVolume:(double)volume {
}

- (void)setPlaybackSpeed:(double)speed {
// See https://developer.apple.com/library/archive/qa/qa1772/_index.html for an explanation of
// these checks.
if (speed > 2.0 && !_player.currentItem.canPlayFastForward) {
if (_eventSink != nil) {
_eventSink([FlutterError errorWithCode:@"VideoError"
message:@"Video cannot be fast-forwarded beyond 2.0x"
details:nil]);
}
return;
}

if (speed < 1.0 && !_player.currentItem.canPlaySlowForward) {
if (_eventSink != nil) {
_eventSink([FlutterError errorWithCode:@"VideoError"
message:@"Video cannot be slow-forwarded"
details:nil]);
}
return;
}

_player.rate = speed;
_playbackSpeed = @(speed);
[self updatePlayingState];
}

- (CVPixelBufferRef)copyPixelBuffer {
Expand Down
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