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

[video_player] ios picture in picture #6284

Closed
wants to merge 83 commits into from

Conversation

vanlooverenkoen
Copy link
Contributor

Added support for picture in picture video playback

video-player-pip-demo-ios.MP4

This feature is only available on iOS because android has a different implementation. simple_pip_mode can be used for Android

flutter/flutter#60048

No migration needed this is an addon

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.

@vanlooverenkoen
Copy link
Contributor Author

platform-android should be removed. This is iOS only.

Copy link
Contributor

@hellohuanlin hellohuanlin left a comment

Choose a reason for hiding this comment

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

left a few questions. not a complete review yet (will need to look into apple's api doc)

@vanlooverenkoen
Copy link
Contributor Author

Thanks already for your fast reviews!!! This is the first objective-c code I have ever written!! I will make the changes tomorrow!!

@stuartmorgan-g
Copy link
Contributor

As a high-level note: as discussed in your previous PR, we require test coverage. There's currently no testing of the native part of this change, which is the most important part.

Do you have a plan in place to add native tests, so that this doesn't end up being eventually closed as incomplete like that previous PR?

We should hold off on detailed review until that has been addressed.

@vanlooverenkoen
Copy link
Contributor Author

vanlooverenkoen commented Aug 18, 2022

@stuartmorgan indeed I get that, in the meantime is there somebody at Flutter that can help me with that?
Updating this plugin gave me some confidence in objective-c. So I will take a look at it after our deadline. For now we did manual testing. And everything seems to be working as expected.

Just to make sure that I a working on the correct thing. I should write the tests in video_player_avfoundation/example/ios/RunnerTest & video_player_avfoundation/example/ios/RunnerUITests
Or is there another location where I should write them?

@stuartmorgan-g
Copy link
Contributor

@stuartmorgan indeed I get that, in the meantime is there somebody at Flutter that can help me with that?

If you have specific questions, please ask them and someone can answer.

Just to make sure that I a working on the correct thing. I should write the tests in video_player_avfoundation/example/ios/RunnerTest & video_player_avfoundation/example/ios/RunnerUITests

Yes, for a case like this where integration tests can't inspect the results meaningfully, native tests would be needed.

@stuartmorgan-g
Copy link
Contributor

Also, you need to follow https://github.com/flutter/flutter/wiki/Contributing-to-Plugins-and-Packages#changing-federated-plugins so that the PR can actually run tests.

@stuartmorgan-g stuartmorgan-g marked this pull request as draft August 18, 2022 13:41
@vanlooverenkoen
Copy link
Contributor Author

@stuartmorgan thanks for the info. I will start with that! I will let you know when I need something else.

@vanlooverenkoen
Copy link
Contributor Author

@stuartmorgan I created a UI test for opening & closing the pip. also for checking if pip is supported. That covers all 3 methods. isSupported, prepare, start/stop
Is there something else you would expect to be tested there?

@vanlooverenkoen
Copy link
Contributor Author

@stuartmorgan & @jmagman updated the gif with the latest demo UI & way smaller size, comparable to demo_ipod.gif

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

The new tests are failing in CI running on iPhone simulators that do not support PiP.

@@ -634,4 +724,23 @@ - (void)setMixWithOthers:(FLTMixWithOthersMessage *)input
}
}

- (void)preparePictureInPicture:(FLTPreparePictureInPictureMessage *)input
error:(FlutterError **)error {
FLTVideoPlayer *player = self.playersByTextureId[input.textureId];
Copy link
Member

Choose a reason for hiding this comment

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

Something like:

- (BOOL)doesInfoPlistSupportPictureInPicture:(FlutterError **)error {
  NSArray *backgroundModes = [NSBundle.mainBundle objectForInfoDictionaryKey:@"UIBackgroundModes"];
  if (![backgroundModes isKindOfClass:[NSArray class]] ||
      ![backgroundModes containsObject:@"audio"]) {
    *error = [FlutterError errorWithCode:@"video_player"
                                 message:@"missing audio UIBackgroundModes audio in Info.plist"
                                 details:nil];
    return NO;
  }
  return YES;
}

- (void)startPictureInPicture:(FLTStartPictureInPictureMessage *)input
                        error:(FlutterError **)error {
  if (![self doesInfoPlistSupportPictureInPicture:error]) {
    return;
  }
  FLTVideoPlayer *player = self.playersByTextureId[input.textureId];
  [player startOrStopPictureInPicture:YES];
}

- (void)stopPictureInPicture:(FLTStopPictureInPictureMessage *)input error:(FlutterError **)error {
  if (![self doesInfoPlistSupportPictureInPicture:error]) {
    return;
  }
  FLTVideoPlayer *player = self.playersByTextureId[input.textureId];
  [player startOrStopPictureInPicture:NO];
}

Copy link
Member

@jmagman jmagman left a comment

Choose a reason for hiding this comment

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

Sorry, didn't mean to approve this yet based on the failing tests 🙂

@vanlooverenkoen vanlooverenkoen requested review from jmagman and removed request for stuartmorgan-g January 19, 2023 08:47
@stuartmorgan-g
Copy link
Contributor

Checkin from triage: Is this just waiting for another review from @jmagman ?

On the CI failures, you probably just need to add a path override for video_player_platform_implementation in video_player/video_player/example/pubspec.yaml. The tool currently misses it (it's a filed issue in the tooling).

@vanlooverenkoen
Copy link
Contributor Author

@stuartmorgan I updated the video_player_platform_interface but for some reason I get an analyze error that I don't see in my local env. The const is already there. But still analyzer is complaining about the const

https://github.com/flutter/plugins/pull/6284/files#diff-764b559024be361f427ff2d3965f9058491e3311178d0c6165ad10e72e29d45fR328

@stuartmorgan-g
Copy link
Contributor

for some reason I get an analyze error that I don't see in my local env.

It's master-only; you are likely either using stable, or just not the latest version of master.

The const is already there. But still analyzer is complaining about the const

It's line 326, not 328. See #7074 for the solution to the remaining issue.

@vanlooverenkoen
Copy link
Contributor Author

Alright I see, I did not know master was always used. I will fix that one

@stuartmorgan-g
Copy link
Contributor

Alright I see, I did not know master was always used. I will fix that one

We test both master and stable; that's what the CHANNEL:master and CHANNEL:stable in the CI task titles correspond to.

@vanlooverenkoen
Copy link
Contributor Author

Makes sense, will that not result in a breaking ci for stable?

@stuartmorgan-g
Copy link
Contributor

Makes sense, will that not result in a breaking ci for stable?

No; unnecessary ignores don't cause analyzer failures.

@vanlooverenkoen
Copy link
Contributor Author

Hmm I don't get it, what should I be doing to get everything green? Because now stable fails

@stuartmorgan-g
Copy link
Contributor

stuartmorgan-g commented Feb 10, 2023

Please replicate the pattern from the PR I linked to above (including exactly duplicating the comment).

@vanlooverenkoen
Copy link
Contributor Author

Aah, alright din't know I had to really ignore it.

@stuartmorgan-g
Copy link
Contributor

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!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants