[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

stuartmorgan · 2024-11-11T14:19:23Z

Please see https://github.com/flutter/flutter/blob/master/docs/contributing/issue_hygiene/README.md#do-not-add-me-too-or-same-or-is-there-an-update-comments-to-issues-or-prs

Anyone interested in completing this PR is welcome to do so.

Azzeccagarbugli · 2024-11-28T15:52:55Z

Hello @vanlooverenkoen, I'm trying your branch to use the PiP mode with the video_player package in this way:

video_player:
      git: 
        url: https://github.com/vanlooverenkoen/flutter-packages.git
        path: packages/video_player/video_player
        ref: "feature/#60048-ios-picture-in-picture"

After doing a whole flutter clean and flutter pub cache repair, I still get the following error:

Error (Xcode): ../../.pub-cache/git/flutter-packages-b250e9c7c8cc79bb4a4d82e597d92e01e50f2c95/packages/video_player/video_player/lib/video_player.dart:635:14: Error: Type 'PictureInPictureOverlaySettings' not found.

Could not build the application for the simulator.
Error launching application on iPhone 16 Pro.

Flutter doctor

[✓] Flutter (Channel stable, 3.24.5, on macOS 15.1.1 24B91 darwin-arm64, locale en-US)
[✓] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
[✓] Xcode - develop for iOS and macOS (Xcode 16.1)
[✗] Chrome - develop for the web (Cannot find Chrome executable at /Applications/Google Chrome.app/Contents/MacOS/Google Chrome)
    ! Cannot find Chrome. Try setting CHROME_EXECUTABLE to a Chrome executable.
[✓] Android Studio (version 2024.2)
[✓] VS Code (version 1.95.3)
[✓] Connected device (3 available)
[✓] Network resources

! Doctor found issues in 1 category.

Do you know how I could fix this?

Thanks a lot in advance ✨

I got the package to work by overide the depency in the depency_overrides section in the pubspec.yaml, like this:

dependency_overrides:
  video_player_android:
    git: 
      url: https://github.com/vanlooverenkoen/flutter-packages.git
      path: packages/video_player/video_player_android
      ref: "feature/#60048-ios-picture-in-picture"
  video_player_avfoundation:
    git: 
      url: https://github.com/vanlooverenkoen/flutter-packages.git
      path: packages/video_player/video_player_avfoundation
      ref: "feature/#60048-ios-picture-in-picture"
  video_player_platform_interface:
    git: 
      url: https://github.com/vanlooverenkoen/flutter-packages.git
      path: packages/video_player/video_player_platform_interface
      ref: "feature/#60048-ios-picture-in-picture"

And in the dependecy section I used this:

video_player:
      git: 
        url: https://github.com/vanlooverenkoen/flutter-packages.git
        path: packages/video_player/video_player
        ref: "feature/#60048-ios-picture-in-picture"

For now this is the only good workround that I found to solve the PiP issue 🫡

kalismeras61 · 2024-12-17T13:17:30Z

@vanlooverenkoen do you have a plan update this PR with latest videoplayer changes?

This reverts commit cd7fede.

This reverts commit ddc7139.

…cture-in-picture

…eInPictureActive

Nols1000 · 2025-01-03T03:17:32Z

Hi @vanlooverenkoen and @stuartmorgan, I've picked up the old branch and merged all changes from main. I've also done some refactoring and regenerating the pigeon files.
Most tests are passing except the test, that checks the "is supported" functionality. It seems that pigeon tries to cast the int into a bool? but fails (very similar to flutter/flutter#90512).
I've also not changed the setUpPictureInPictureController yet. I'm a bit confused on what the todo is there.
You all can check out my branch at Nols1000/packages feature/#60048-ios-picture-in-picture.
How would you like to proceed? Do we want to open a new PR? Can I open a PR to your branch @vanlooverenkoen?

Thanks for all the hard work and kindest regards,
Nils

vanlooverenkoen · 2025-01-03T08:40:56Z

Thanks @Nols1000!!!

It has been quite a hectic year for me! I don't have an active project where I use the video_player package which makes it even harder to stay on top of this pull request.

So thanks @Nols1000 for updating & fixing the merge conflicts. I have some fixes on my branch I did a couple weeks ago (not tested & not committed), but new merge conflicts are already added again.

@stuartmorgan I really want to get this merged, but what do you think is the best way forward?

Merging the changes from @Nols1000 in this PR?
- I added @Nols1000 to my repository.
Creating a new merge request @Nols1000 created?

I calculated all the hours I have spend on this PR and it is already 41,5hours sinds august 2022. So it would be ashame if all that time was for nothing. So thanks again @Nols1000 🙏

Nols1000 · 2025-01-03T11:24:11Z

Hi @vanlooverenkoen,
Thanks for reaching out. I've pushed the changes to you branch.

stuartmorgan · 2025-01-03T11:34:14Z

@stuartmorgan I really want to get this merged, but what do you think is the best way forward?

Merging the changes from @Nols1000 in this PR?

I added @Nols1000 to my repository.

Creating a new merge request @Nols1000 created?

Either one is fine; usually when people pick up unfinished PRs they aren't able to push to them so make a new PR, but since that's not an issue here continuing with the same PR shouldn't have any issues.

stuartmorgan

A couple of quick notes on the updated version until I have time for a full review.

stuartmorgan · 2025-01-03T11:38:59Z

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m

-#import "./include/video_player_avfoundation/AVAssetTrackUtils.h"
+#import "AVAssetTrackUtils.h"
+#import "FVPDisplayLink.h"
+#import "messages.g.h"


Local imports need to use project-relative paths; fixing that should fix the build_all CI failures.

stuartmorgan · 2025-01-03T11:39:21Z

...undation/darwin/video_player_avfoundation/Sources/video_player_avfoundation/FVPVideoPlayer.m

@@ -4,10 +4,14 @@

 #import "./include/video_player_avfoundation/FVPVideoPlayer.h"
 #import "./include/video_player_avfoundation/FVPVideoPlayer_Test.h"
+#import "./include/video_player_avfoundation/AVAssetTrackUtils.h"


This change is incorrect; please see the style guide rules for header import ordering.

stuartmorgan · 2025-01-03T11:44:14Z

...ayer/video_player_android/android/src/main/java/io/flutter/plugins/videoplayer/Messages.java

@@ -1,7 +1,7 @@
 // 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 (v22.5.0), do not edit directly.
+// Autogenerated from Pigeon (v22.7.2), do not edit directly.


Why was video_player_android Pigeon code regenerated for this PR?

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

Nols1000 added 11 commits January 2, 2025 21:54

Revert "fix: add all the correct dependency overrides"

01aaf8d

This reverts commit cd7fede.

Revert "camera override video_player plugin"

ee6db47

This reverts commit ddc7139.

Update the documentation on configuring the required ios capabilities

2e3196b

Merge remote-tracking branch 'origin/main' into feature/#60048-ios-pi…

29c1d3b

…cture-in-picture

[video_player_android] regenerate pigeons

6d8fb1e

[video_player_avfoundation] regenerate pigeons

114f5b8

Revert changes to the camera_android_camerax example

5861e1d

[video_player] fix todo making the picture-in-picture message const

114f9ac

[video_player] add tests to ensure equals implementation for isPictur…

3c1b4e6

…eInPictureActive

[video_player_avfoundation] refactor picture in picture extension

3985a77

[video_player_avfoundation] refactor code

1649be4

Nols1000 requested a review from matanlurey as a code owner January 3, 2025 11:22

github-actions bot removed the p: camera label Jan 3, 2025

[video_player] format code

bc56139

stuartmorgan requested changes Jan 3, 2025

View reviewed changes

Nols1000 added 4 commits January 3, 2025 13:16

[video_player_avfoundation] reorder imports

e1348c8

[video_player] update version

7e70da0

[video_player_android] Revert all changes made to this package

4abe6f1

[video_player] upgrade dependencies

2006494

github-actions bot removed the platform-android label Jan 3, 2025

Nols1000 added 2 commits January 3, 2025 15:15

[video_player] fix dependencies for example

58dabf4

[video_player_android] add overwrite to fix analysis error

881e80e

github-actions bot added the platform-android label Jan 3, 2025

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

stuartmorgan commented Nov 11, 2024

Azzeccagarbugli commented Nov 28, 2024 •

edited

Loading

kalismeras61 commented Dec 17, 2024

Nols1000 commented Jan 3, 2025 •

edited

Loading

vanlooverenkoen commented Jan 3, 2025 •

edited

Loading

Nols1000 commented Jan 3, 2025

stuartmorgan commented Jan 3, 2025

stuartmorgan left a comment

stuartmorgan Jan 3, 2025

stuartmorgan Jan 3, 2025

stuartmorgan Jan 3, 2025

[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

ErickCReis May 9, 2023

Choose a reason for hiding this comment

vanlooverenkoen Jun 23, 2023

Choose a reason for hiding this comment

stuartmorgan Aug 28, 2023

Choose a reason for hiding this comment

stuartmorgan May 31, 2024

Choose a reason for hiding this comment

tarrinneal commented May 10, 2023

vanlooverenkoen commented May 10, 2023

stuartmorgan commented Nov 11, 2024

Azzeccagarbugli commented Nov 28, 2024 • edited Loading

kalismeras61 commented Dec 17, 2024

Nols1000 commented Jan 3, 2025 • edited Loading

vanlooverenkoen commented Jan 3, 2025 • edited Loading

Nols1000 commented Jan 3, 2025

stuartmorgan commented Jan 3, 2025

stuartmorgan left a comment

Choose a reason for hiding this comment

stuartmorgan Jan 3, 2025

Choose a reason for hiding this comment

stuartmorgan Jan 3, 2025

Choose a reason for hiding this comment

stuartmorgan Jan 3, 2025

Choose a reason for hiding this comment

vanlooverenkoen commented Mar 20, 2023 •

edited by stuartmorgan

Loading

Azzeccagarbugli commented Nov 28, 2024 •

edited

Loading

Nols1000 commented Jan 3, 2025 •

edited

Loading

vanlooverenkoen commented Jan 3, 2025 •

edited

Loading