Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement picture in picture for iOS #1325

Merged
merged 3 commits into from
Feb 19, 2019
Merged

Conversation

aalzanki
Copy link
Contributor

@aalzanki aalzanki commented Nov 12, 2018

Test Plan:

  • Run on ipad
  • test out onIsPictureInPictureSupported, onIsPictureInPictureActive, restoreUserInterfaceForPictureInPictureStop, startPictureInPicture, stopPictureInPicture.

@cobarx
Copy link
Contributor

cobarx commented Nov 21, 2018

This is a very cool feature, thanks for submitting and providing all the documentation!

I'd like to see if we can condense the interface so that it's as simple as possible.

onIsPictureInPictureSupported - React Native only supports iOS >= 9.0 so I don't think we need this.
onIsPictureInPictureActive - This gets called whenever the state changes, so it should be onPictureInPictureChanged or onPictureInPictureStatusChanged. The payload should be similar to other events: isActive or isPictureInPictureActive
restoreUserInterfaceForPictureInPictureStop - Given that this is mandatory to be called, how about just calling this automatically when we exit picture in picture
startPictureInPicture / stopPictureInPicture - I'm trying to use props wherever possible and avoid functions unless absolutely necessary. I'd suggest changing this to a boolean prop like pictureInPicture.

@aalzanki
Copy link
Contributor Author

Ops, didn't realize my debug commits were showing up here. This is not ready to be reviewed again. There are some issues. I'll resolve them when I get a chance.

In terms of restoreUserInterfaceForPictureInPictureStop, I don't think we can just always call it when PIP stops. I think the idea behind restoreUserInterfaceForPictureInPictureStopWithCompletionHandler is that when the user moves away from the screen they were on when they started the PIP, and then taps the restore button from the PIP controls (left most button in this picture) the video wouldn't just disappear, but rather, it'd give your app a chance to navigate to the screen that the video is on, and then the little PIP will animate back into the video player's area. Defaulting to always calling restoreUserInterfaceForPictureInPictureStop means that the video will always immediately disappear, instead of animating back into place.

@aalzanki
Copy link
Contributor Author

@cobarx I made 4 of the 5 changes you mentioned. Please let me know what you think about the restoration piece. I tried to mirror Apple's restoration function with: onRestoreUserInterfaceForPictureInPictureStop and restoreUserInterfaceForPictureInPictureStopCompleted.

Test Plan:
 - Run on ipad
 - test out onIsPictureInPictureSupported, onIsPictureInPictureActive, restoreUserInterfaceForPictureInPictureStop, startPictureInPicture, stopPictureInPicture
@aalzanki
Copy link
Contributor Author

@cobarx any feedback about the updated code?

@cobarx
Copy link
Contributor

cobarx commented Feb 19, 2019

Sorry for the delay in getting to review this. This is working great, I'm going to merge it.

Two things I noticed:
Do we need the true/false argument to restoreUserInterfaceForPictureInPictureStopCompleted? If not, you could just pass true automatically and remove the argument from the README / JS function. If it is needed, please document when it's appropriate to use true vs. false.

Also, I'm not seeing events when I put the video in fullscreen and then engage PiP from there. If possible, can you add a PR to capture those using the same framework you've done here.

@cobarx
Copy link
Contributor

cobarx commented Feb 19, 2019

Btw this works great. Thanks so much for putting it together, great feature.

It would be wonderful to be able to support this on ExoPlayer as well.

@cobarx cobarx merged commit d5fe47f into TheWidlarzGroup:master Feb 19, 2019
@danielmarino24i
Copy link
Contributor

danielmarino24i commented Mar 4, 2019

I think this PR breaks compatibility with tvOS, as PiP is only on AVKit (ios)

Probably is can be fixed with a solution like this (probably some condition has to be added also on AVPictureInPictureControllerDelegate delegate's methods ) or __TVOS_PROHIBITED:

#if __has_include(<react-native-video/RCTVideoCache.h>)
@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate, DVAssetLoaderDelegatesDelegate, AVAssetResourceLoaderDelegate>
#elif TARGET_OS_TV
@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate>
#else
@interface RCTVideo : UIView <RCTVideoPlayerViewControllerDelegate, AVPictureInPictureControllerDelegate>
#endif

@danielmarino24i
Copy link
Contributor

danielmarino24i commented Mar 4, 2019

Also it fails if initial value is set to true (it should be UI/JS triggered) of it is set as initial value, trigger the function on onLoad or sth like this

@FullstackJack
Copy link
Contributor

@cobarx As noted by @danielmarino24i, this created a regression on tvOS. We'll need to get a PR up to hotfix it.

@FullstackJack
Copy link
Contributor

I've spun up a work branch to complete this work for Android, we'll see how it goes.

retyui added a commit to retyui/flow-typed that referenced this pull request May 1, 2019
AndrewSouthpaw pushed a commit to flow-typed/flow-typed that referenced this pull request May 3, 2019
beauner69 pushed a commit to beauner69/react-native-video that referenced this pull request Oct 10, 2019
Implement picture in picture for iOS

(rebased from commit d5fe47f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants