[video_player] Make video player avfoundation seekTo async #7164

gabrielgarciagava · 2023-02-12T21:43:38Z

This PR updates the pigeon version that is being used by video_player_avfoundation so it is possible to use [AVPlayer seekTo:completionHandler:] method.

Potentially fixes https://github.com/flutter/flutter/issues/116056 ( Maybe I need some insights from you @stuartmorgan, specially related to this comment flutter/flutter#116056 (comment) )

This PR follows the step 1 of these instructions https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins

Pre-launch Checklist

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

Created a single commit for the pigeon upgrade so it is easier to read the second commit where changes to `message.dart` will be done

stuartmorgan-g

Thanks for the contribution! I've left a few comments, and you'll need to fix the unit tests that no longer compile (per the CI failure).

This won't fix the issue you referenced, which is about how position is tracked in the plugin. It doesn't actually have anything to do with the implementation of seekTo.

packages/video_player/video_player/pubspec.yaml

packages/video_player/video_player_avfoundation/example/ios/Flutter/AppFrameworkInfo.plist

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

gabrielgarciagava · 2023-02-13T08:34:05Z

Thanks for the contribution! I've left a few comments, and you'll need to fix the unit tests that no longer compile (per the CI failure).

This won't fix the issue you referenced, which is about how position is tracked in the plugin. It doesn't actually have anything to do with the implementation of seekTo.

Thank you for the feedback. Since this is my first contribution, I really appreaciate it.

I will fix all tests. Now, about the github issue. Is it really required to have a issues linked to the PR? If that is the case, I will create one since I could not find any existent one related to this.

…kTo:completionHandler:] completes

stuartmorgan-g · 2023-02-13T13:27:56Z

Now, about the github issue. Is it really required to have a issues linked to the PR?

Since it's addressing a to-do that didn't have an associated issue it doesn't need one.

gabrielgarciagava · 2023-02-13T13:44:26Z

All threads resolved.

Please check https://github.com/flutter/plugins/pull/7164/files#diff-ad57f5cf3efac84f7c3c41b77bd4b391d0a37e0f488309dfcf15b08423c71ae8 with care.

I updated that test to be able to properly seek to some part of a video.
I'm also testing that the player position is correct after the seek. So should the test be renamed?

hellohuanlin · 2023-02-13T18:19:38Z

packages/video_player/video_player_avfoundation/ios/Classes/FLTVideoPlayerPlugin.m

+      completionHandler:^(BOOL finished) {
+        dispatch_async(dispatch_get_main_queue(), ^{
+          [self.registry textureFrameAvailable:input.textureId.intValue];
+          completion(nil);


may wanna pass finished to dart side?

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

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?

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.

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.

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.

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.

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.

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.

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

stuartmorgan-g · 2023-02-22T15:49:07Z

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

gabrielgarciagava · 2023-02-22T17:32:31Z

No problem at all. I am completly in favor of any kind of long term improvements.

I will reopen it in the flutter/packages channel as soon as I find time to work on it.

Thanks for the feedback.

gabrielgarciagava requested review from hellohuanlin and tarrinneal as code owners February 12, 2023 21:43

github-actions bot added p: video_player platform-ios labels Feb 12, 2023

gabrielgarciagava changed the title ~~Make video player avfoundation seek to async~~ [video_player] Make video player avfoundation seek to async Feb 12, 2023

Upgrading pigeon version to ^8.0.0

07e6f10

Created a single commit for the pigeon upgrade so it is easier to read the second commit where changes to `message.dart` will be done

gabrielgarciagava force-pushed the make-video-player-avfoundation-seekTo-async branch from 6c8d1c7 to 3da6bfe Compare February 12, 2023 21:44

gabrielgarciagava changed the title ~~[video_player] Make video player avfoundation seek to async~~ [video_player] Make video player avfoundation seekTo async Feb 12, 2023

stuartmorgan-g suggested changes Feb 12, 2023

View reviewed changes

gabrielgarciagava force-pushed the make-video-player-avfoundation-seekTo-async branch 2 times, most recently from f62985d to b88b372 Compare February 13, 2023 13:19

Change seekTo method to async so it only completes when [AVPlayer see…

8eede17

…kTo:completionHandler:] completes

gabrielgarciagava force-pushed the make-video-player-avfoundation-seekTo-async branch from b88b372 to 8eede17 Compare February 13, 2023 13:25

gabrielgarciagava requested review from stuartmorgan-g and removed request for hellohuanlin and tarrinneal February 13, 2023 13:41

hellohuanlin reviewed Feb 13, 2023

View reviewed changes

stuartmorgan-g closed this Feb 22, 2023

gabrielgarciagava mentioned this pull request Feb 25, 2023

[video_player] Make video player avfoundation seek to async flutter/packages#3299

Merged

11 tasks

[video_player] Make video player avfoundation seekTo async #7164

[video_player] Make video player avfoundation seekTo async #7164

Uh oh!

Conversation

gabrielgarciagava commented Feb 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gabrielgarciagava commented Feb 13, 2023

Uh oh!

stuartmorgan-g commented Feb 13, 2023

Uh oh!

gabrielgarciagava commented Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabrielgarciagava Feb 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 22, 2023

Uh oh!

gabrielgarciagava commented Feb 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gabrielgarciagava commented Feb 12, 2023 •

edited

Loading

gabrielgarciagava commented Feb 13, 2023 •

edited

Loading

gabrielgarciagava Feb 13, 2023 •

edited

Loading