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

feat: implement enterPictureInPictureOnLeave prop for both platform(Android, iOS) #3385

Open
wants to merge 83 commits into
base: master
Choose a base branch
from

Conversation

YangJonghun
Copy link
Collaborator

@YangJonghun YangJonghun commented Nov 26, 2023

Update the changelog

feat(android): implement pictureInPicture prop and onPictureInPictureStatusChanged event

Describe the changes

  • (Breaking Change) Remove pictureInPicture boolean prop (iOS)

  • Add enterPictureInPictureOnLeave boolean prop (Android, iOS)

  • Add enterPictureInPicture method (Android, iOS)

  • Add exitPictureInPicture method (Android, iOS)

  • Add onPictureInPictureStatusChanged event listener (Android)

  • Resolve issue React Native Video Picture In Picture for Android #2621

memo)
As I know Activity's onUserLeaveHint is acceptable lifecycle method for enter PIP. but it's impossible to detect that method within this library. so, I implement enterPicturInPicture within onHostPause.
If someone doesn't like this approach, please leave comment in this PR, I'll give you intent style code.

@freeboub
Copy link
Collaborator

freeboub commented Nov 26, 2023

Thank you for the PR, can you open a PR in draft, it will allow to comment on it.

public void onHostPause() {
isInBackground = true;
if (pictureInPictureEnabled) {
enterPictureInPictureMode();
return;
}
You cannot do like this, when another application will go over the app, it will automatically switch to Pip ...

@freeboub
Copy link
Collaborator

I don't understand why you use a dedicated fragment : ReactExoplayerFragment can you explain why you had to use it please ? (point to doc or other sample is enough)

@freeboub
Copy link
Collaborator

can you enable pip in sample to be able to test easily please

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Nov 26, 2023

can you enable pip in sample to be able to test easily please

      <Video
        style={{width: '100%', height: 300, backgroundColor: 'black'}}
        resizeMode={ResizeMode.CONTAIN}
        source={{
          uri: 'https://demo.unified-streaming.com/k8s/features/stable/video/tears-of-steel/tears-of-steel-en.ism/.mpd',
        }}
        selectedTextTrack={{
          type: SelectedTrackType.INDEX,
          value: 0,
        }}
        fullscreen={false}
        controls
        pictureInPicture
        playWhenInactive
        playInBackground={false}
        debug={{enable: true, thread: true}}
        onPictureInPictureStatusChanged={({isActive}) => {
          console.log(isActive, 'isActive');
        }}
      />

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Nov 26, 2023

@freeboub

I don't understand why you use a dedicated fragment : ReactExoplayerFragment can you explain why you had to use it please ? (point to doc or other sample is enough)

To implement PIP, we need to use the onUserLeaveHint and onPictureInPictureModeChanged methods of Activity. However, this requires forcing the user to modify their native code.
To avoid this, I tricky replaced onUserLeaveHint with onPause in the Fragment and onPictureInPictureModeChanged with Fragment's onPictureInPictureModeChanged in the Fragment.

@freeboub
Copy link
Collaborator

can you enable pip in sample to be able to test easily please

      <Video
        style={{width: '100%', height: 300, backgroundColor: 'black'}}
        resizeMode={ResizeMode.CONTAIN}
        source={{
          uri: 'https://demo.unified-streaming.com/k8s/features/stable/video/tears-of-steel/tears-of-steel-en.ism/.mpd',
        }}
        selectedTextTrack={{
          type: SelectedTrackType.INDEX,
          value: 0,
        }}
        fullscreen={false}
        controls
        pictureInPicture
        playWhenInactive
        playInBackground={false}
        debug={{enable: true, thread: true}}
        onPictureInPictureStatusChanged={({isActive}) => {
          console.log(isActive, 'isActive');
        }}
      />

Ok, but will it work without native controls ?

@YangJonghun
Copy link
Collaborator Author

can you enable pip in sample to be able to test easily please

      <Video
        style={{width: '100%', height: 300, backgroundColor: 'black'}}
        resizeMode={ResizeMode.CONTAIN}
        source={{
          uri: 'https://demo.unified-streaming.com/k8s/features/stable/video/tears-of-steel/tears-of-steel-en.ism/.mpd',
        }}
        selectedTextTrack={{
          type: SelectedTrackType.INDEX,
          value: 0,
        }}
        fullscreen={false}
        controls
        pictureInPicture
        playWhenInactive
        playInBackground={false}
        debug={{enable: true, thread: true}}
        onPictureInPictureStatusChanged={({isActive}) => {
          console.log(isActive, 'isActive');
        }}
      />

Ok, but will it work without native controls ?

Yes, it's same like iOS and there are no restrictions.

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Nov 26, 2023

Thank you for the PR, can you open a PR in draft, it will allow to comment on it.

public void onHostPause() { isInBackground = true; if (pictureInPictureEnabled) { enterPictureInPictureMode(); return; } You cannot do like this, when another application will go over the app, it will automatically switch to Pip ...

I'm not sure, but as I know onUserLeaveHint is also called when another app is turned on.
Actually I wanted to detect and implement the home key event, but the API was deprecated and I replaced it with onPause.

@YangJonghun
Copy link
Collaborator Author

@nick-mccomb-easygo
Unfortunately, I've been so busy lately that I haven't been able to keep up with this work, but I'm going to finish it.
You're welcome to implement it if you'd like.

@YangJonghun YangJonghun marked this pull request as ready for review January 19, 2024 17:40
@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Jan 19, 2024

@freeboub @KrzysztofMoch

Hi. I'd like to let you know this PR is ready for review 🙌
please review when you possible. :)

FYI,
this PR depends on PR #3489
If doesn't merge, PIP's action button doesn't work properly

I think this code is too mush complicate to understand.
but I couldn't rewrite FullScreenPlayerView code in this PR.
It would be nice if you(or someone) could refactor it later.

Copy link
Member

@KrzysztofMoch KrzysztofMoch left a comment

Choose a reason for hiding this comment

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

LGTM 👍, thank you for the PR!
Tested it on Android 14, and 10 (simulators) and it worked for me. Will test it on device later.
Lets @freeboub review and for me we can merge

@freeboub
Copy link
Collaborator

I just fix conflict. The only comments I have are in the discussion

@KrzysztofMoch
Copy link
Member

I have created 6.7.0-rc.0 release that contain this PR - as we will get feedback and do some changes I will push more RCs. For now let's see how it will go 🙌

@KrzysztofMoch
Copy link
Member

I have released next rc version 6.8.0-rc.0 - if there will be no reported issues, this PR should be merged 🙌

@tungmin97
Copy link

Hi @YangJonghun, I'm encountering the same issue as @tiplefbxx mentioned where my app would always crash upon return from PIP.

For a little bit more context, I have a separated Video component outside my navigator stack, and in my testing unload the video component below when having a floating player eliminates this behavior.

Your app just crashed. See the error below.
java.lang.IndexOutOfBoundsException: Index: 1, Size: 1
  java.util.ArrayList.get(ArrayList.java:437)
  com.brentvatne.exoplayer.ReactExoplayerView.setIsInPictureInPicture(ReactExoplayerView.java:2305)
  com.brentvatne.exoplayer.ReactExoplayerFragment.onPictureInPictureModeChanged(ReactExoplayerFragment.kt:35)
  androidx.fragment.app.Fragment.performPictureInPictureModeChanged(Fragment.java:3221)
  androidx.fragment.app.FragmentManager.dispatchPictureInPictureModeChanged(FragmentManager.java:3017)
  androidx.fragment.app.FragmentManager.lambda$new$3$androidx-fragment-app-FragmentManager(FragmentManager.java:468)
  androidx.fragment.app.FragmentManager$$ExternalSyntheticLambda3.accept(Unknown Source:4)
  androidx.activity.ComponentActivity.onPictureInPictureModeChanged(ComponentActivity.java:1097)
  android.app.Activity.dispatchPictureInPictureModeChanged(Unknown Source:23)
  android.app.ActivityThread.handleWindowingModeChangeIfNeeded(Unknown Source:40)
  android.app.ActivityThread.performActivityConfigurationChanged(Unknown Source:12)
  android.app.ActivityThread.performConfigurationChangedForActivity(Unknown Source:28)
  android.app.ActivityThread.handleActivityConfigurationChanged(Unknown Source:273)
  android.app.ActivityThread$ActivityClientRecord$1.onConfigurationChanged(Unknown Source:14)
  android.view.ViewRootImpl.performConfigurationChange(Unknown Source:81)
  android.view.ViewRootImpl.handleResized(Unknown Source:112)
  android.view.ViewRootImpl.-$$Nest$mhandleResized(Unknown Source:0)
  android.view.ViewRootImpl$ViewRootHandler.handleMessageImpl(Unknown Source:658)
  android.view.ViewRootImpl$ViewRootHandler.handleMessage(Unknown Source:15)
  android.os.Handler.dispatchMessage(Unknown Source:19)
  android.os.Looper.loopOnce(Unknown Source:182)
  android.os.Looper.loop(Unknown Source:82)
  android.app.ActivityThread.main(Unknown Source:123)
  java.lang.reflect.Method.invoke(Native Method)
  com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(Unknown Source:11)
  com.android.internal.os.ZygoteInit.main(Unknown Source:312)

@YangJonghun
Copy link
Collaborator Author

@tungmin97
Thanks for reporting me. but it's hard to me to reproduce what you're describing🥲
If you can, please provide me any small reproduction code🙏

@tungmin97
Copy link

@YangJonghun here is the crash example. I've found some issues only happen on android.

1 - We can still swipe home to enter pip even after the video outside navigation is paused, the other has enterPictureInPictureOnLeave set to false.

2 - Has both video playing, pause then play the floated video, swipe home or press the button to enter PIP mode, notice the source is set to the other video with enterPictureInPictureOnLeave set to false.

3 - Resume the app while in PIP will always crash the application

@YangJonghun
Copy link
Collaborator Author

@tungmin97
Sorry for late reply.
Multiplayer issue is a difficult problem to solve, so fix will be delayed.
I'm working on this PR personally, so I can't guarantee any due date, but I'll get to it as soon as I can. 🙏

FYI. @KrzysztofMoch @freeboub

@YangJonghun
Copy link
Collaborator Author

YangJonghun commented Dec 3, 2024

@l2hyunwoo May I request your code review?🙏

@KrzysztofMoch
Copy link
Member

Hey @YangJonghun what is status of PR? Together with @freeboub we are okay to merge this PR - so let me know if you are also okay to merge it

- remove code that uses Fragment, which is a tricky implementation
@YangJonghun
Copy link
Collaborator Author

@KrzysztofMoch @freeboub
I apologise for the delay in responding.
I've been testing in my spare time and this PR has the following issues

  • It may behave differently than intended when multiple Video components exist at the same time
  • Fullscreen behavior has changed significantly from when this PR was originally written, so it doesn't work correctly on Android. (Fullscreen controller will be exposed on pip)
  • I've removed tricky implementation with Fragment and replaced it with androidx. The current implementation is recommended stable implementation for Android, but it's been implemented recently and lacks testing.
  • Web support is not written because it was added while the PR was not merged. It's not a difficult task, but I don't have enough time these days to work on it🥲

I'm fine with this PR being merged, but there are things that need to be fixed, so it would be nice to let the developers know that this is an experimental feature.


val onPictureInPictureModeChanged: (info: PictureInPictureModeChangedInfo) -> Unit = { info: PictureInPictureModeChangedInfo ->
view.setIsInPictureInPicture(info.isInPictureInPictureMode)
if (!info.isInPictureInPictureMode && context.findActivity().lifecycle.currentState == Lifecycle.State.CREATED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (!info.isInPictureInPictureMode && context.findActivity().lifecycle.currentState == Lifecycle.State.CREATED) {
if (!info.isInPictureInPictureMode && activity.lifecycle.currentState == Lifecycle.State.CREATED) {

How about this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't it possible Activity is different between when bind eventListeners and when onPictureInPictureModeChanged is called?🤔

Copy link
Collaborator Author

@YangJonghun YangJonghun Dec 15, 2024

Choose a reason for hiding this comment

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

It makes more sense to fix it as you said! We need to only handle Activity where the Video is visible

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

5e244e8
I've fixed it in above commit :)

@KrzysztofMoch
Copy link
Member

Sure! We can go with what we have, I will open tickets for those issues

@KrzysztofMoch
Copy link
Member

So @YangJonghun, once you are ready please let me know 🙌

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.