Skip to content

Conversation

@vanlooverenkoen
Copy link
Contributor

@vanlooverenkoen vanlooverenkoen commented Mar 20, 2023

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

demo_pip_iphone

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

  • 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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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

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

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.

carrying over the stamp from previous PR

@camsim99 camsim99 removed their request for review March 27, 2023 15:33
@vanlooverenkoen
Copy link
Contributor Author

@tarrinneal can I assist you with the review?

Copy link
Contributor

@tarrinneal tarrinneal left a comment

Choose a reason for hiding this comment

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

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

# 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
@vanlooverenkoen
Copy link
Contributor Author

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

@reidbaker reidbaker requested a review from camsim99 May 4, 2023 18:44
@tarrinneal
Copy link
Contributor

@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
Copy link
Contributor Author

@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

@abdalazeezalyosfi

This comment was marked as off-topic.

@philipgiuliani
Copy link

Hi @vanlooverenkoen, we are using your fork already for a while in production and it works very well. Our released version was still from the end of last year. We've just updated the library and noticed a strange animation happening when setting the pip overlay rect. Also when changing page the video remains visible for a while. See the video below:

TEST.Simulator.Screen.Recording.-.iPhone.16.-.2025-02-20.at.15.39.09.mp4

@Nols1000
Copy link

Hi @philipgiuliani ,
this is probably because of me merging the new main branch and trying to implment the change requests. If you use stuff in production please be sure to pin the commit, or maintain your own branch, as features in development might break. I'll try to get to working on it again, but it might be a bit before my weekends are available again.

@philipgiuliani
Copy link

philipgiuliani commented Feb 20, 2025

Thanks for the quick response and checking it! We pinned the commit, but we had to update the package because the old video_player cannot build on Android with Flutter 3.29.

@philipgiuliani
Copy link

It seems to work perfectly fine by adding the following code back in the initWithPlayerItem function. This code was removed in the process of the refactoring at the beginning of January.

  _playerLayer = [AVPlayerLayer playerLayerWithPlayer:_player];
  // picture-in-picture shows a placeholder where the original video was playing.
  // This is a native overlay that does not scroll with the rest of the Flutter UI.
  // That is why we need to set the opacity of the overlay.
  // Setting it to 0 would result in the picture-in-picture not working.
  // Setting it to 1 would result in the picture-in-picture overlay always showing over other
  // widget. Setting it to 0.001 makes the placeholder invisible, but still allows the
  // picture-in-picture.
  _playerLayer.opacity = 0.001;
  [self.flutterViewLayer addSublayer:_playerLayer];

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Apologies, doing another review pass had fallen off my radar since the review state was never switched back to review request. I've done an initial pass over the current state.

sdk: flutter
path_provider: ^2.0.0
video_player: ^2.7.0
video_player_platform_interface: ^6.2.3
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this addition necessary?

### Picture-in-Picture

#### iOS
To enable picture-in-picture functionality, you need to add the **Background Modes** capabilities for **Audio, AirPlay, and Picture in Picture** as described in [Configuring your app for media playback > Configure the background modes](https://developer.apple.com/documentation/AVFoundation/configuring-your-app-for-media-playback#Configure-the-background-modes). Resulting in a new string entry `audio` in the array value of the entry `UIBackgroundModes` in your `Info.plist` file, which is located in `<project root>/ios/Runner/Info.plist`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The second sentence here is a fragment. It should probably say something like "Adding this mode will result in ..."

### Picture-in-Picture

#### iOS
To enable picture-in-picture functionality, you need to add the **Background Modes** capabilities for **Audio, AirPlay, and Picture in Picture** as described in [Configuring your app for media playback > Configure the background modes](https://developer.apple.com/documentation/AVFoundation/configuring-your-app-for-media-playback#Configure-the-background-modes). Resulting in a new string entry `audio` in the array value of the entry `UIBackgroundModes` in your `Info.plist` file, which is located in `<project root>/ios/Runner/Info.plist`:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything from "which is located in" on can be removed; documenting basic iOS project structure isn't necessary here.

> Failing to add the `audio` **Background Modes** capability will result in a silent failure to start picture-in-picture playback.
Example:
![The example app running in iOS with picture-in-picture enabled](https://github.com/flutter/plugins/blob/main/packages/video_player/video_player/doc/demo_pip_iphone.gif?raw=true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

READMEs should never have references to non-pinned URLs, as it means old versions' READMEs can change or break at any time. E.g., renaming the plugin in the future would break all previous READMEs.

This will need to be added in a follow-up PR, using a hash instead of main.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file should be listed in a .pubignore.

),
);
},
child: const Text('Set picture-in-picture overlay rect'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this something that clients have to call manually? Shouldn't the controller be determining this information from the widget as needed?

}

/// Sets the location of the video player view in order to animate the picture-in-picture view.
Future<void> setPictureInPictureOverlaySettings({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Per my comment in the example code, it's not clear to me why this needs to be public API.

);
}

/// Starts picture-in-picture mode.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The methods that require isPictureInPictureSupported to be true in order to call them should all be commented accordingly.

XCTAssertNil(error);
XCTAssertEqual(avPlayer.volume, 0.1f);

// Set picture-in-picture
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needs to be resolved.

XCTAssertTrue([playButton waitForExistenceWithTimeout:30.0]);
[playButton tap];

if ([AVPictureInPictureController isPictureInPictureSupported]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This still needs to be resolved.

@Piinks
Copy link
Contributor

Piinks commented Jun 4, 2025

Hey @vanlooverenkoen did you want to continue working on this change and address the above feedback?

decisivearmor added a commit to decisivearmor/packages that referenced this pull request Jun 25, 2025
- Set minimal opacity (0.001) to AVPlayerLayer to ensure PiP works
- Add comprehensive debugging for layer state and PiP controller
- Check and set default bounds if layer has empty bounds
- Add isPictureInPicturePossible checks before starting PiP

Based on flutter#3500 discussions about PiP requirements.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <noreply@anthropic.com>
@stuartmorgan-g
Copy link
Collaborator

From triage: @Nols1000 are you still planning on continuing work on this PR?

@Nols1000
Copy link

Nols1000 commented Jul 20, 2025

@stuartmorgan-g If there is someone else looking to finish it please do. It seems that I am not experienced enough with the flutter packages versioning. I've not found the time to work on it on multiple occasions, because of different priorities. If it's still open once I get to it, I'll give it another try.

@akaboshinit
Copy link

I’d like to take on the remaining work to complete this change. 

@stuartmorgan-g
Copy link
Collaborator

@akaboshinit The next step would be to either coordinate getting commit access to this branch, or creating a fork of the branch and opening a new PR using that branch (referencing this PR for additional context) to continue the work.

@akaboshinit
Copy link

Thank you. I'm currently working on it locally and will make a PR once it works.

@akaboshinit
Copy link

To share the progress, I am changing the current PR and direction, and instead of creating a new texture, I am thinking of making the video_player texture directly into a pip.

trim.DDE25789-FE28-4840-88CD-68E95E2213CF.MOV

@abdalazeezalyosfi
Copy link

Hi @akaboshinit
any update please keep us updated

@Azzeccagarbugli
Copy link

@akaboshinit it would be very much appreciated if you share any updated, thanks a lot 🥹

@stuartmorgan-g
Copy link
Collaborator

Thank you. I'm currently working on it locally and will make a PR once it works.

Given that the plan is to open a new PR based on this one, I'm going to go ahead and close this PR. @akaboshinit , or anyone else interested in picking this up, please feel free to open a new PR forked from this once you have something ready for review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ios][video_player] pip - picture in picture