[video_player] #60048 ios picture in picture #3500

vanlooverenkoen · 2023-03-20T20:52:57Z

Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.

List which issues are fixed by this PR. You must list at least one issue.

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

Fixes flutter/flutter#60048

Migration from flutter/plugin to flutter/packages is done with this pr

old pr: flutter/plugins#6284

No migration needed this is an addon

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

Pre-launch Checklist

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

vanlooverenkoen · 2023-03-20T20:56:18Z

@hellohuanlin & @stuartmorgan I did the migration from flutter/plugins#6284 to here

hellohuanlin

carrying over the stamp from previous PR

vanlooverenkoen · 2023-04-03T10:31:45Z

@tarrinneal can I assist you with the review?

tarrinneal

Initial pass. A few changes to be made. Good work on this!

packages/video_player/video_player/CHANGELOG.md

packages/video_player/video_player/README.md

packages/video_player/video_player_avfoundation/CHANGELOG.md

packages/video_player/video_player_platform_interface/lib/video_player_platform_interface.dart

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

packages/video_player/video_player/README.md

packages/video_player/video_player_platform_interface/CHANGELOG.md

# Conflicts: # packages/video_player/video_player/CHANGELOG.md # packages/video_player/video_player/lib/video_player.dart # packages/video_player/video_player/pubspec.yaml # packages/video_player/video_player_android/example/lib/mini_controller.dart # packages/video_player/video_player_avfoundation/CHANGELOG.md # packages/video_player/video_player_avfoundation/example/lib/mini_controller.dart # packages/video_player/video_player_avfoundation/lib/src/avfoundation_video_player.dart # packages/video_player/video_player_avfoundation/pubspec.yaml

…tsPictureInPicture

vanlooverenkoen · 2023-04-17T16:18:14Z

@tarrinneal I updated my pull request fully retested it to make sure it does what I expect.

ErickCReis · 2023-05-09T14:42:33Z

packages/video_player/video_player/lib/video_player.dart

@@ -106,6 107,9 @@ class VideoPlayerValue {
 /// The current speed of the playback.
 final double playbackSpeed;

+ /// True if picture-in-picture is currently active.
+ final bool isPictureInPictureActive;


@vanlooverenkoen
Nice job, I'm in need of this feature.

I'm testing this pr in a project and I found a bug when updating this value. This property is not used in the bool operator ==(Object other) function, so it is not updated in the ValueNotifier. Can you fix this?

Damn! Good catch!

It doesn't look like you added a Dart unit test to cover that change (i.e., one which would have caught this bug).

It doesn't look like you added a Dart unit test to cover that change (i.e., one which would have caught this bug).

This doesn't appear to have been resolved.

tarrinneal · 2023-05-10T08:17:19Z

@tarrinneal I updated my pull request fully retested it to make sure it does what I expect.

Seems like you still have a few broken tests. I should be able to get this re-reviewed this week, if you can git them sorted out.

vanlooverenkoen · 2023-05-10T08:22:39Z

@tarrinneal the tests are failing on camera because of the dependency overrides. How should I handle this? Because the dependency override script doesn't update it in the camera package.

I will fix the merge conflicts today

vanlooverenkoen · 2024-04-23T08:49:38Z

@stuartmorgan Linux repo_checks will always fail on 50. federated safety check right? As long as I have not yet split the PR up. So this fail is allowed for now?

Dart changes are not allowed to other packages in video_player in the same PR as changes to public Dart code in video_player_platform_interface, as this can cause accidental breaking changes to be missed by automated checks. Please split the changes to these two packages into separate PRs.

vanlooverenkoen · 2024-04-23T10:50:57Z

@stuartmorgan what is the best way to fix check_podspecs & macos_platform_tests master?

Just wrap everything related to pip with this?

  if (@available(macOS 10.15, *)) {
// pip code
  }

stuartmorgan · 2024-05-07T15:46:15Z

@stuartmorgan Linux repo_checks will always fail on 50. federated safety check right? As long as I have not yet split the PR up. So this fail is allowed for now?

That's correct. The format failure in repo_checks is not expected though.

@stuartmorgan what is the best way to fix check_podspecs & macos_platform_tests master?

Just wrap everything related to pip with this?

It depends on what you mean by "just wrap"; you'll need reasonable else behavior (e.g., returning an error) for the unsupported OS case.

vanlooverenkoen · 2024-05-08T08:03:57Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

@@ -71,8 71,10 @@ - (FVPDisplayLink *)displayLinkWithRegistrar:(id<FlutterPluginRegistrar>)registr

 #pragma mark -

-@interface FVPVideoPlayer ()
+API_AVAILABLE(macos(10.15))


@stuartmorgan is this how I should mark the FVPVideoPlayer? Or how else should I fix AVPictureInPicutreControllerDelegate to only be available on macOS?

This is marking everything in the private interface as 10.15 ; you'll need to declare this in an extension:

API_AVAILABLE(macos(10.15)) @interface FVPVideoPlayer (AVPIPDelegate) <AVPictureInPictureControllerDelegate> @end @interface FVPVideoPlayer () ...

(This will, correctly, cause you to need to adjust other guards later in the code, such as the pictureInPictureController property and calls to it.)

…icture in picture

vanlooverenkoen · 2024-05-08T09:17:18Z

@stuartmorgan I have fixed all bugs and failing CI checks

stuartmorgan · 2024-05-31T17:56:43Z

packages/camera/camera/example/pubspec.yaml

@@ -30,3 30,14 @@ dev_dependencies:

 flutter:
 uses-material-design: true
+
+# FOR TESTING ONLY. DO NOT MERGE.


All of the camera* package overrides can be removed, since this won't affect the camera usages.

stuartmorgan · 2024-05-31T18:02:07Z

packages/video_player/video_player/README.md

+If you want to enable picture-in-picture make sure to enable the `audio` capability (in Xcode's UI it will say **Audio, AirPlay, and Picture in Picture**).
+Not setting this capability but calling `setPictureInPictureOverlayRectMessage` and `setPictureInPicture` will not start the picture-in-picture.
+
+```xml


There needs to be something in the text just before this that says exactly what file(s) this is showing.

stuartmorgan · 2024-05-31T18:05:19Z

packages/video_player/video_player/example/lib/main.dart

- VideoProgressIndicator(_controller, allowScrubbing: true),
+ if (_controller.value.isPictureInPictureActive) ...<Widget>[
+ Container(color: Colors.white),
+ // TODO(goderbauer): Make this const when this package requires Flutter 3.8 or later.


This is already the case.

stuartmorgan · 2024-05-31T18:06:43Z

packages/video_player/video_player/lib/video_player.dart

@@ -106,6 107,9 @@ class VideoPlayerValue {
 /// The current speed of the playback.
 final double playbackSpeed;

+ /// True if picture-in-picture is currently active.
+ final bool isPictureInPictureActive;


It doesn't look like you added a Dart unit test to cover that change (i.e., one which would have caught this bug).

This doesn't appear to have been resolved.

stuartmorgan · 2024-05-31T18:25:22Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

@@ -71,8 71,10 @@ - (FVPDisplayLink *)displayLinkWithRegistrar:(id<FlutterPluginRegistrar>)registr

 #pragma mark -

-@interface FVPVideoPlayer ()
+API_AVAILABLE(macos(10.15))


This is marking everything in the private interface as 10.15 ; you'll need to declare this in an extension:

API_AVAILABLE(macos(10.15)) @interface FVPVideoPlayer (AVPIPDelegate) <AVPictureInPictureControllerDelegate> @end @interface FVPVideoPlayer () ...

stuartmorgan · 2024-05-31T18:45:41Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

@@ -84,6 86,7 @@ @interface FVPVideoPlayer ()
 @property(nonatomic, readonly) BOOL isPlaying;
 @property(nonatomic) BOOL isLooping;
 @property(nonatomic, readonly) BOOL isInitialized;
+@property(nonatomic) BOOL isPictureInPictureStarted;


No is for the property, per https://google.github.io/styleguide/objcguide.html#objective-c-method-names

stuartmorgan · 2024-05-31T18:48:47Z

packages/video_player/video_player_avfoundation/darwin/Classes/FVPVideoPlayerPlugin.m

+ }
+}
+
+- (void)startOrStopPictureInPicture:(BOOL)shouldPictureInPictureStart {


It seems very confusing from an API standpoint that both

[self startOrStopPictureInPicture:foo];

(which is already hard to parse, since the bool argument isn't named by the signature in violation of standard Obj-C naming conventions)

and

self.isPictureInPictureStarted = foo;

exist, and do different things.

Why not just override the setter of the property to do what this method does, and eliminate this duplicate-sounding method?

stuartmorgan · 2024-05-31T18:51:35Z

packages/video_player/video_player_avfoundation/darwin/RunnerTests/VideoPlayerTests.m

@@ -664,6 664,30 @@ - (void)testSeekToleranceWhenSeekingToEnd {
 XCTAssertNil(error);
 XCTAssertEqual(avPlayer.volume, 0.1f);

+ // Set picture-in-picture


My earlier comment about this testing was never resolved.

stuartmorgan · 2024-05-31T18:52:13Z

packages/video_player/video_player_avfoundation/example/ios/RunnerUITests/VideoPlayerUITests.m

@@ -31,6 32,34 @@ - (void)testPlayVideo {
 XCTAssertTrue([playButton waitForExistenceWithTimeout:30.0]);
 [playButton tap];

+ if ([AVPictureInPictureController isPictureInPictureSupported]) {


This should be a separate test.

stuartmorgan · 2024-05-31T18:54:11Z

packages/video_player/video_player_avfoundation/pigeons/messages.dart

+ bool isPictureInPictureSupported();
+ @ObjCSelector('setPictureInPictureOverlaySettings:')
+ void setPictureInPictureOverlaySettings(
+ SetPictureInPictureOverlaySettingsMessage msg);


New code should not use the message object pattern (and in fact the existing code has since been updated not to use it).

stuartmorgan · 2024-07-09T19:25:33Z

@vanlooverenkoen Are you still planning on updating this PR?

vanlooverenkoen · 2024-07-10T07:29:18Z

@stuartmorgan Yes, for some reason I missed these comments again. Can we maybe plan a timeframe where I fix these comments and get a review right after, in order to prevent endless merge conflict fixes?

Because everytime I come back to this PR it often takes more time to figure out what changed on main and if it affected my PR?

Let me know if that is feasible, than I can add this PR again to my planning

stuartmorgan · 2024-07-10T10:06:09Z

@vanlooverenkoen I should generally be able to turn around an incremental review for this PR within about a week going forward. In cases where there's a month or more between when I do a review and when the PR is updated again though, I need to re-review the entire large PR almost from scratch because I no longer have enough context to do an incremental review, and finding a large block of time to re-review everything can take longer.

vanlooverenkoen · 2024-07-10T10:15:47Z

@stuartmorgan alright that is clear! Let me try to fit this in. Keep you posted

vanlooverenkoen added 5 commits March 20, 2023 19:47

#60048: Migration from flutter-plugins to flutter-packages

595d3df

Updated doc

4831f57

Merged current main into ios pip

976b630

#60048: Formatting

a7d5dd5

Fixed formatting

876dfe8

vanlooverenkoen requested review from hellohuanlin, camsim99 and tarrinneal as code owners March 20, 2023 20:52

github-actions bot added p: video_player platform-android platform-ios labels Mar 20, 2023

hellohuanlin approved these changes Mar 20, 2023

View reviewed changes

#60048: Updated the changelogs

a4ca9e7

camsim99 removed their request for review March 27, 2023 15:33

Merge branch 'main' into feature/#60048-ios-picture-in-picture

49f769a

tarrinneal requested changes Apr 14, 2023

View reviewed changes

vanlooverenkoen added 5 commits April 15, 2023 09:20

#60048: Updated comments related to docs & typo's

0764e27

Renamed setAutomaticallyStartPictureInPicture to setAutomaticallyStar…

65fdf65

…tsPictureInPicture

Updated starting to started to match stopped enum values

d4d9ff9

Removed some logic to pause

ff6d02f

vanlooverenkoen requested a review from tarrinneal April 17, 2023 16:17

tarrinneal requested a review from stuartmorgan April 17, 2023 18:44

reidbaker requested a review from camsim99 May 4, 2023 18:44

ErickCReis reviewed May 9, 2023

View reviewed changes

vanlooverenkoen and others added 5 commits April 23, 2024 10:19

changelog version to specific version

e9aba08

Update pubspec.yaml

30c0b7a

Update pubspec.yaml

c0ccbfc

fix: formatting of video_player

8ee1dcb

fix: compatability

58db59a

vanlooverenkoen added 3 commits April 23, 2024 11:08

fix: integervalue

16ae8cb

Merge branch 'main' into feature/#60048-ios-picture-in-picture

a15c3b7

fix: test on macOS

87b92c2

vanlooverenkoen added 5 commits April 24, 2024 08:56

Merge branch 'main' into feature/#60048-ios-picture-in-picture

5183028

Merge branch 'main' into feature/#60048-ios-picture-in-picture

002faa0

Merge branch 'main' into feature/#60048-ios-picture-in-picture

148cd55

Merge branch 'main' into feature/#60048-ios-picture-in-picture

d88cf33

Merge branch 'main' into feature/#60048-ios-picture-in-picture

45f96f5

vanlooverenkoen added 3 commits May 8, 2024 09:19

Merge branch 'main' into feature/#60048-ios-picture-in-picture

9459bd5

fix formatting

d9f883b

fix: macos checks

c9993dc

vanlooverenkoen commented May 8, 2024

View reviewed changes

vanlooverenkoen added 2 commits May 8, 2024 10:08

fix: comment

3e401c1

fix: check on macOS where there are no capabilities required to use p…

b250e9c

…icture in picture

stuartmorgan requested changes May 31, 2024

View reviewed changes

stuartmorgan added the triage-ios Should be looked at in iOS triage label Jun 3, 2024

jmagman removed the triage-ios Should be looked at in iOS triage label Jun 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[video_player] #60048 ios picture in picture #3500

[video_player] #60048 ios picture in picture #3500

vanlooverenkoen commented Mar 20, 2023 •

edited by stuartmorgan

Loading

vanlooverenkoen commented Mar 20, 2023

hellohuanlin left a comment

vanlooverenkoen commented Apr 3, 2023

tarrinneal left a comment

vanlooverenkoen commented Apr 17, 2023

ErickCReis May 9, 2023

vanlooverenkoen Jun 23, 2023

stuartmorgan Aug 28, 2023

stuartmorgan May 31, 2024

tarrinneal commented May 10, 2023

vanlooverenkoen commented May 10, 2023

vanlooverenkoen commented Apr 23, 2024

vanlooverenkoen commented Apr 23, 2024

stuartmorgan commented May 7, 2024

vanlooverenkoen May 8, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

vanlooverenkoen commented May 8, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan May 31, 2024

stuartmorgan commented Jul 9, 2024

vanlooverenkoen commented Jul 10, 2024

stuartmorgan commented Jul 10, 2024

vanlooverenkoen commented Jul 10, 2024

[video_player] #60048 ios picture in picture #3500

Are you sure you want to change the base?

[video_player] #60048 ios picture in picture #3500

Conversation

vanlooverenkoen commented Mar 20, 2023 • edited by stuartmorgan Loading

Pre-launch Checklist

vanlooverenkoen commented Mar 20, 2023

hellohuanlin left a comment

Choose a reason for hiding this comment

vanlooverenkoen commented Apr 3, 2023

tarrinneal left a comment

Choose a reason for hiding this comment

vanlooverenkoen commented Apr 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarrinneal commented May 10, 2023

vanlooverenkoen commented May 10, 2023

vanlooverenkoen commented Apr 23, 2024

vanlooverenkoen commented Apr 23, 2024

stuartmorgan commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vanlooverenkoen commented May 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartmorgan commented Jul 9, 2024

vanlooverenkoen commented Jul 10, 2024

stuartmorgan commented Jul 10, 2024

vanlooverenkoen commented Jul 10, 2024

vanlooverenkoen commented Mar 20, 2023 •

edited by stuartmorgan

Loading