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

[video_player] Avoid blocking the main thread loading video count #4714

Merged
merged 4 commits into from
Feb 1, 2022

Conversation

jmagman
Copy link
Member

@jmagman jmagman commented Jan 28, 2022

#4639 introduced playing mp3 files with video_player. The method that checks if the player is ready needs to handle zero-sized audio files, so it called [AVAsset tracksWithMediaType:AVMediaTypeVideo] on the main thread. However, if the tracks aren't loaded, that API will synchronously load them. Docs say: "Becomes callable without blocking when the key @"tracks" has been loaded". This caused the UI to become unresponsive until the video loads.

Instead, check that the tracks are loaded, and if not, load them, then re-schedule the call to make sure the player is ready.

Unfortunately the loadTracksWithMediaType:completionHandler: we really want only became available on iOS 15.

Added more tests for controlling both audio and video media types.

Fixes flutter/flutter#97356
Blocked by test audio file PR flutter/assets-for-api-docs#177

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

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

FLTVideoPlayerPlugin *videoPlayerPlugin =
(FLTVideoPlayerPlugin *)[[FLTVideoPlayerPlugin alloc] initWithRegistrar:registrar];

NSDictionary<NSString *, id> *audioInitialization = [self testPlugin:videoPlayerPlugin uri:@"https://cdn.pixabay.com/audio/2021/09/06/audio_bacd4d6020.mp3"];
Copy link
Member Author

Choose a reason for hiding this comment

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

I will update this to https://flutter.github.io/assets-for-api-docs/assets/videos/rooster.mp3 once flutter/assets-for-api-docs#177 merges. Will also update the duration integer assert.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why a separate repo instead of landing the mp3 here and using a raw GitHub URL?

Copy link
Member Author

Choose a reason for hiding this comment

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

Checking binaries into git repos is generally frowned upon since it never gets removed from the git history. I think assets-for-api-doc is the side-repo other flutter org repos use to localize the sin to one place.

'https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4',

'https://flutter.github.io/assets-for-api-docs/assets/videos/bee.mp4',

We can host them somewhere else, but we should host the videos and images all together.

Copy link
Member Author

Choose a reason for hiding this comment

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

Comment on lines 342 to 343
[asset loadValuesAsynchronouslyForKeys:@[ @"tracks" ] completionHandler:^{
if ([asset statusOfValueForKey:@"tracks" error:nil] != AVKeyValueStatusLoaded) {
Copy link
Member Author

Choose a reason for hiding this comment

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

See similar logic at

if ([asset statusOfValueForKey:@"tracks" error:nil] == AVKeyValueStatusLoaded) {
NSArray *tracks = [asset tracksWithMediaType:AVMediaTypeVideo];

[asset loadValuesAsynchronouslyForKeys:@[ @"tracks" ] completionHandler:assetCompletionHandler];

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

Thanks @jmagman ! I think we should do more checks with threads in the future, contributors (including me) can accidently break the thread model or cause the main thread blocked easily.

@jmagman jmagman added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Feb 1, 2022
@fluttergithubbot fluttergithubbot merged commit a5b97ea into flutter:main Feb 1, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Feb 1, 2022
@jmagman jmagman deleted the block-vp branch February 1, 2022 18:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
p: video_player platform-ios waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[video_player IOS] VideoPlayerController.network is blocking the UI while loading
5 participants