Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

[video_player] Make video player avfoundation seekTo async #7164

Closed
gabrielgarciagava wants to merge 2 commits into from
Closed

[video_player] Make video player avfoundation seekTo async #7164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
07e6f10
Upgrading pigeon version to ^8.0.0
gabrielgarciagava Feb 12, 2023
8eede17
Change seekTo method to async so it only completes when [AVPlayer see…
gabrielgarciagava Feb 13, 2023
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
3 changes: 2 additions & 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,5 +1,6 @@
## NEXT
## 2.3.9

* Makes seekTo async and only complete when AVPlayer.seekTo completes.
* Updates minimum Flutter version to 3.0.

## 2.3.8
Expand Down
5 changes: 1 addition & 4 deletions ...es/video_player/video_player_avfoundation/example/integration_test/video_player_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,7 @@ void main() {

await controller.seekTo(const Duration(seconds: 3));

// TODO(stuartmorgan): Switch to _controller.position once seekTo is
// fixed on the native side to wait for completion, so this is testing
// the native code rather than the MiniController position cache.
expect(controller.value.position, const Duration(seconds: 3));
expect(await controller.position, const Duration(seconds: 3));
});

testWidgets('can be paused', (WidgetTester tester) async {
Expand Down
25 changes: 23 additions & 2 deletions packages/video_player/video_player_avfoundation/example/ios/RunnerTests/VideoPlayerTests.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
@interface FLTVideoPlayer : NSObject <FlutterStreamHandler>
@property(readonly, nonatomic) AVPlayer *player;
@property(readonly, nonatomic) AVPlayerLayer *playerLayer;
@property(readonly, nonatomic) int64_t position;
@end

@interface FLTVideoPlayerPlugin (Test) <FLTAVFoundationVideoPlayerApi>
Expand Down Expand Up @@ -106,10 +107,30 @@ - (void)testSeekToInvokesTextureFrameAvailableOnTextureRegistry {
OCMStub([partialRegistrar textures]).andReturn(mockTextureRegistry);
FLTVideoPlayerPlugin *videoPlayerPlugin =
(FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:partialRegistrar];
FLTPositionMessage *message = [FLTPositionMessage makeWithTextureId:@101 position:@0];

FlutterError *error;
[videoPlayerPlugin seekTo:message error:&error];
[videoPlayerPlugin initialize:&error];
XCTAssertNil(error);
FLTCreateMessage *create = [FLTCreateMessage
makeWithAsset:nil
uri:@"https://flutter.github.io/assets-for-api-docs/assets/videos/hls/bee.m3u8"
packageName:nil
formatHint:nil
httpHeaders:@{}];
FLTTextureMessage *textureMessage = [videoPlayerPlugin create:create error:&error];
NSNumber *textureId = textureMessage.textureId;

XCTestExpectation *initializedExpectation = [self expectationWithDescription:@"seekTo completes"];
FLTPositionMessage *message = [FLTPositionMessage makeWithTextureId:textureId position:@1234];
[videoPlayerPlugin seekTo:message
completion:^(FlutterError *_Nullable error) {
[initializedExpectation fulfill];
}];
[self waitForExpectationsWithTimeout:30.0 handler:nil];
OCMVerify([mockTextureRegistry textureFrameAvailable:message.textureId.intValue]);

FLTVideoPlayer *player = videoPlayerPlugin.playersByTextureId[textureId];
XCTAssertEqual([player position], 1234);
}

- (void)testDeregistersFromPlayer {
Expand Down
22 changes: 13 additions & 9 deletions packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,11 @@ - (int64_t)duration {
return FLTCMTimeToMillis([[[_player currentItem] asset] duration]);
}

- (void)seekTo:(int)location {
// TODO(stuartmorgan): Update this to use completionHandler: to only return
// once the seek operation is complete once the Pigeon API is updated to a
// version that handles async calls.
- (void)seekTo:(int)location completionHandler:(void (^)(BOOL))completionHandler {
[_player seekToTime:CMTimeMake(location, 1000)
toleranceBefore:kCMTimeZero
toleranceAfter:kCMTimeZero];
toleranceBefore:kCMTimeZero
toleranceAfter:kCMTimeZero
completionHandler:completionHandler];
}

- (void)setIsLooping:(BOOL)isLooping {
Expand Down Expand Up @@ -636,10 +634,16 @@ - (FLTPositionMessage *)position:(FLTTextureMessage *)input error:(FlutterError
return result;
}

- (void)seekTo:(FLTPositionMessage *)input error:(FlutterError **)error {
- (void)seekTo:(FLTPositionMessage *)input
completion:(void (^)(FlutterError *_Nullable))completion {
FLTVideoPlayer *player = self.playersByTextureId[input.textureId];
[player seekTo:input.position.intValue];
[self.registry textureFrameAvailable:input.textureId.intValue];
[player seekTo:input.position.intValue
completionHandler:^(BOOL finished) {
dispatch_async(dispatch_get_main_queue(), ^{
[self.registry textureFrameAvailable:input.textureId.intValue];
completion(nil);
Copy link
Contributor

@hellohuanlin hellohuanlin Feb 13, 2023

Choose a reason for hiding this comment

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

may wanna pass finished to dart side?

Copy link
Author

@gabrielgarciagava gabrielgarciagava Feb 13, 2023

Choose a reason for hiding this comment

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

It is possible, however that would require an interface change, right?

Copy link
Contributor

@hellohuanlin hellohuanlin Feb 13, 2023

Choose a reason for hiding this comment

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

isn't changing from sync to async already an interface change? I think the question here is, if it's not finished, what do we expect UI to behave?

Copy link
Author

@gabrielgarciagava gabrielgarciagava Feb 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It is not an interface change because the method channel calls are always async by its nature, right?

According to the documentation of AVPlayer, finished as true means that the seekTo completed properly, while finished as false means that seekTo was aborted because of another seekTo request. So in both cases it means the seekTo is not happening anymore. I agree it can be an useful information though, but I tried to go with a slightly simpler improvement as my first contribution :D

I can implement the interface change if needed, of course. Let me know.

Copy link
Author

@gabrielgarciagava gabrielgarciagava Feb 13, 2023

Choose a reason for hiding this comment

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

When I say that it will require an interface change, I'm saying I will need to touch all packages, the app facing, the platform interface, and the platform implementations. While the current PR only require a change in the avfoundation one.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 13, 2023

Choose a reason for hiding this comment

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

In theory we could pass it to the Dart side (changing the Pigeon interface definition) and then drop it here, which would still be an implementation-package-local change. I don't think there's much value in doing that since we have no plans to use that information, but @hellohuanlin if you feel strongly about adding it, it would be easy enough to do.

This PR definitely should not become a breaking change to the public seek API.

Copy link
Contributor

@hellohuanlin hellohuanlin Feb 13, 2023

Choose a reason for hiding this comment

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

gotcha. i don't have an strong opinion. I just wanted to make sure the UI can revert back to the original position if finished is false. Sounds like we don't need this flag to achieve this.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 13, 2023

Choose a reason for hiding this comment

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

In general, the position handling in video_player is not great right now; it can get out of sync with native for short periods quite easily, and is primary polling based. It needs an overhaul, and there are issues tracking that, but it's way out of scope here.

Copy link
Author

@gabrielgarciagava gabrielgarciagava Feb 13, 2023

Choose a reason for hiding this comment

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

In this case, if I may ask, what are the following steps I need to take now?
I see that "submit-queue" is still failling, but I could not find out what that means.

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Feb 13, 2023

Choose a reason for hiding this comment

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

submit-queue means the tree is closed, and isn't related to the PR; you don't need to worry about it.

});
}];
}

- (void)pause:(FLTTextureMessage *)input error:(FlutterError **)error {
Expand Down
6 changes: 4 additions & 2 deletions packages/video_player/video_player_avfoundation/ios/Classes/messages.g.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
// Copyright 2013 The Flutter Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
// Autogenerated from Pigeon (v2.0.1), do not edit directly.
// Autogenerated from Pigeon (v8.0.0), do not edit directly.
// See also: https://pub.dev/packages/pigeon

#import <Foundation/Foundation.h>

@protocol FlutterBinaryMessenger;
@protocol FlutterMessageCodec;
@class FlutterError;
Expand Down Expand Up @@ -97,7 +99,7 @@ NSObject<FlutterMessageCodec> *FLTAVFoundationVideoPlayerApiGetCodec(void);
/// @return `nil` only when `error != nil`.
- (nullable FLTPositionMessage *)position:(FLTTextureMessage *)msg
error:(FlutterError *_Nullable *_Nonnull)error;
- (void)seekTo:(FLTPositionMessage *)msg error:(FlutterError *_Nullable *_Nonnull)error;
- (void)seekTo:(FLTPositionMessage *)msg completion:(void (^)(FlutterError *_Nullable))completion;
- (void)pause:(FLTTextureMessage *)msg error:(FlutterError *_Nullable *_Nonnull)error;
- (void)setMixWithOthers:(FLTMixWithOthersMessage *)msg
error:(FlutterError *_Nullable *_Nonnull)error;
Expand Down
Loading