jwplayer / jwplayer-react-native

MIT License
32 stars 9 forks source link

[BUG] Trying to exit fullscreen on Android doesn't exit fullscreen or crashes #52

Closed dominictobias closed 2 months ago

dominictobias commented 5 months ago

Describe the bug On Android when going fullscreen you can't exit it.

To Reproduce Simple component I'm not doing anything unusual just config, not using jwPlayerRef yet.

interface VideoPlayerProps {
  playlist: JwPlaylistItem[]
  autoStart?: boolean
  height: number
  onPlay?: (player: JWPlayer) => void
  onPause?: (player: JWPlayer) => void
  onComplete?: (player: JWPlayer) => void
}

export function VideoPlayer({
  playlist,
  autoStart = false,
  height,
  onPlay,
  onPause,
  onComplete,
}: VideoPlayerProps) {
  const jwPlayerRef = useRef<JWPlayer>(null)

  // JWPlayer isn't reactive to playlist changes so don't render until we have one.
  if (!playlist.length) {
    return null
  }

  const config: JwConfig = {
    license:
      Platform.OS === 'android'
        ? process.env.EXPO_PUBLIC_LICENSE_KEY_ANDROID || ''
        : process.env.EXPO_PUBLIC_LICENSE_KEY_IOS || '',
    autostart: autoStart,
    playlist,
  }

  const onVideoPlay = () => {
    onPlay?.(jwPlayerRef.current!)
  }
  const onVideoPause = () => {
    onPause?.(jwPlayerRef.current!)
  }
  const onVideoComplete = () => {
    onComplete?.(jwPlayerRef.current!)
  }

  return (
    <View>
      <JWPlayer
        ref={jwPlayerRef}
        style={{ height }}
        config={config}
        onPlay={onVideoPlay}
        onPause={onVideoPause}
        onComplete={onVideoComplete}
      />
    </View>
  )
}
  1. Open a video
  2. Press fullscreen
  3. Try to exit - either from the X or [ ] - either just slightly changes window (but still fullscreen) or crashes the app

// Press Fullscreen, then close X (still fullscreen), then Fullscreen [ ] = crash https://github.com/jwplayer/jwplayer-react-native/assets/160451465/508fea32-0e50-4fa9-8161-efebd8900388

// Open with Fullscreen and try to close with Fullscreen [ ]. After a couple of attempts of it only slightly changing it crashes https://github.com/jwplayer/jwplayer-react-native/assets/160451465/d63e1c35-318f-4e59-bb02-cfe7e36abbfa

Desktop (please complete the following information):

System:
  OS: macOS 14.5
  CPU: (11) arm64 Apple M3 Pro
  Memory: 52.09 MB / 36.00 GB
  Shell:
    version: "5.9"
    path: /bin/zsh
Binaries:
  Node:
    version: 20.11.1
    path: ~/.nvm/versions/node/v20.11.1/bin/node
  Yarn:
    version: 1.22.21
    path: ~/.nvm/versions/node/v20.11.1/bin/yarn
  npm:
    version: 10.2.4
    path: ~/.nvm/versions/node/v20.11.1/bin/npm
  Watchman: Not Found
Managers:
  CocoaPods:
    version: 1.15.2
    path: /Users/dominic.tobias/.rbenv/shims/pod
SDKs:
  iOS SDK:
    Platforms:
      - DriverKit 23.4
      - iOS 17.4
      - macOS 14.4
      - tvOS 17.4
      - visionOS 1.1
      - watchOS 10.4
  Android SDK:
    API Levels:
      - "34"
    Build Tools:
      - 33.0.1
      - 34.0.0
      - 35.0.0
    System Images:
      - android-34 | Google APIs ARM 64 v8a
      - android-34 | Google Play ARM 64 v8a
    Android NDK: 22.1.7171670
IDEs:
  Android Studio: 2023.2 AI-232.10227.8.2321.11479570
  Xcode:
    version: 15.3/15E204a
    path: /usr/bin/xcodebuild
Languages:
  Java:
    version: 17.0.4.1
    path: /Users/dominic.tobias/.sdkman/candidates/java/current/bin/javac
  Ruby:
    version: 3.3.0
    path: /Users/dominic.tobias/.rbenv/shims/ruby
npmPackages:
  "@react-native-community/cli": Not Found
  "react": "18.2.0"
  "react-native": "0.74.2"
  react-native-macos: Not Found
npmGlobalPackages:
  "*react-native*": Not Found
Android:
  hermesEnabled: true
  newArchEnabled: false
iOS:
  hermesEnabled: true
  newArchEnabled: false

Additional context Tested on Samsung S22 with latest software updates (Android 14 + latest everything)

dominictobias commented 5 months ago
06-17 10:53:15.308 27653 27653 E AndroidRuntime: FATAL EXCEPTION: main
06-17 10:53:15.308 27653 27653 E AndroidRuntime: Process: com.coindesk.mobile, PID: 27653
06-17 10:53:15.308 27653 27653 E AndroidRuntime: java.lang.NullPointerException: Attempt to invoke virtual method 'android.view.ViewParent com.jwplayer.rnjwplayer.RNJWPlayer.getParent()' on a null object reference
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.jwplayer.rnjwplayer.RNJWPlayerView$fullscreenHandler.onFullscreenRequested(RNJWPlayerView.java:457)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.longtailvideo.jwplayer.h.b.b(SourceFile:54)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.longtailvideo.jwplayer.h.b.$r8$lambda$CMv2bsmwcKFSBKGpgvM5DPziXNs(Unknown Source:0)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.longtailvideo.jwplayer.h.b$$ExternalSyntheticLambda0.run(Unknown Source:4)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at android.os.Handler.handleCallback(Handler.java:958)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at android.os.Handler.dispatchMessage(Handler.java:99)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at android.os.Looper.loopOnce(Looper.java:230)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at android.os.Looper.loop(Looper.java:319)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at android.app.ActivityThread.main(ActivityThread.java:8919)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at java.lang.reflect.Method.invoke(Native Method)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:578)
06-17 10:53:15.308 27653 27653 E AndroidRuntime:    at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1103)
06-17 10:53:15.317  1738 28090 I DropBoxManagerService: add tag=data_app_crash isTagEnabled=true flags=0x2
06-17 10:53:15.317  1738  1758 D Debug   : low && ship && 3rdparty app crash, do not dump
06-17 10:53:15.318  1738 28091 W ActivityManager: crash : com.coindesk.mobile,10355

Looking at the logs it looks like a null pointer exception caused in this function: https://github.com/jwplayer/jwplayer-react-native/blob/8493364b5b46d5962faf3143536ea89c0a4ff094/android/src/main/java/com/jwplayer/rnjwplayer/RNJWPlayerView.java#L457

Seems like mPlayerView doesn't exist at that point?

Perhaps because I tried to exit fullscreen but nothing really happened, so this was executed:

// Remove the player view from the root ViewGroup.
mRootView.removeView(mPlayerView);

And then I try and press the fullscreen button again, only this time the mPlayerView has already been removed? Just a guess

Jmilham21 commented 4 months ago

@dominictobias I think the full-screen handler here needs a refactor. It's a pretty dated implementation. I'll make sure to follow up if we can get a quick fix in place.

Out of curiosity, what is your preference for full-screen? Do you expect the player to go into full-screen portrait or landscape? Would you like to have a method for overriding the behavior? I'm considering how best to refactor this piece, and any input would be appreciated.

dominictobias commented 4 months ago

what is your preference for full-screen? Do you expect the player to go into full-screen portrait or landscape? Would you like to have a method for overriding the behavior? I'm considering how best to refactor this piece, and any input would be appreciated.

It would be great to control that as we will have "normal" landscape videos which we might want to open fullscreen in horizontal mode and the user can rotate the screen. But we will also have Insta/TikTok/YT style Shorts which are portrait, and clicking these should definitely open in portrait fullscreen

Jmilham21 commented 4 months ago

Thanks @dominictobias. I am starting work on this refactor this week and will update you as we progress.

My goal right now is to get closer to what existed before a change in the Android SDK altered the experience and be crash-free. You should be able to dictate what type of full-screen is favored, as was the past experience. As always, if you need a non-standard use case, you can fork this repository and create your own full-screen handler class to extend your needs.

Jmilham21 commented 4 months ago

@dominictobias, the issue turned out to be that you were using the player in a Modal (or so it seemed).

For now, I took the simple(r) path of allowing you (the developer) to set a prop for playerInModal, where the player will be set up with a different full-screen handler that's better suited for this scenario. This full-screen handler doesn't necessarily play nice with the standard configurations, so this will be the simplest way to resolve this.

Once approved and released, when using the player in a Modal (or dialog), you must set the prop playerInModal to true.

dominictobias commented 4 months ago

@dominictobias, the issue turned out to be that you were using the player in a Modal (or so it seemed).

For now, I took the simple(r) path of allowing you (the developer) to set a prop for playerInModal, where the player will be set up with a different full-screen handler that's better suited for this scenario. This full-screen handler doesn't necessarily play nice with the standard configurations, so this will be the simplest way to resolve this.

Once approved and released, when using the player in a Modal (or dialog), you must set the prop playerInModal to true.

You are right it is in a <Modal>. Great! Look forward to testing

Tactica-Zone commented 4 months ago

Hello, is there a fix for this already.

We currently have an app in production and we are facing the same issue.

Although our player isn't embedded in a modal, however the app crashes with the same error when you attempt to exitfullscreen

Jmilham21 commented 3 months ago

Hey @Tactica-Zone , could you share your implementation? Are you able to recreate this inside a sample app (or ideally the Example app in the repository) you can share with us?

Jmilham21 commented 3 months ago

@dominictobias We have merged a PR that we hope will resolve this. Can you test the master branch in your app to see if it resolves your crash? I can push a build to NPM, but I would like to ensure it fixes your issue first.

dominictobias commented 3 months ago

@Jmilham21 Trying out master the behavior is still quite strange. It only crashes some of the time, other times it just minimizes the app.

Another issue I noticed is that the video becomes dimmer in fullscreen.

Full screen X button doesn't work correct, pressing [ ] button closes app: https://github.com/user-attachments/assets/d7591bfd-836f-4d93-ad99-ecebbbeaa531

Same thing but this time pressing [ ] button crashes the app, screen dimming is more noticeable in this vid: https://github.com/user-attachments/assets/f286fd17-27f9-455a-a797-d0f078a09a69

If only the [ ] button is pressed, the behavior is also odd - the first 1-2 clicks it doesn't do much, next it crashes: https://github.com/user-attachments/assets/bdd38367-8d41-424f-a5eb-f43daf4493b7

On IOS side same code works nicely: https://github.com/user-attachments/assets/484161a3-ef62-4b16-95bf-8fd3e82cc274

"@jwplayer/jwplayer-react-native": "https://github.com/jwplayer/jwplayer-react-native#master",

VideoModal.ts

import {
  Modal,
  Pressable,
  StyleSheet,
  View,
  useWindowDimensions,
} from 'react-native'

import { VideoItemResponse } from '@api/types/external/jwVideo'

import { VideoPlayer } from './VideoPlayer'

interface VideoModalProps {
  visible: boolean
  video: VideoItemResponse
  onRequestClose: () => void
}

export function VideoModal({
  visible,
  video,
  onRequestClose,
}: VideoModalProps) {
  const { width } = useWindowDimensions()
  const videoHeight = (width / 8) * 5 // JW is 8/5

  return (
    <Modal
      animationType="slide"
      transparent
      visible={visible}
      onRequestClose={onRequestClose}
    >
      <View style={styles.modalContainer}>
        <Pressable style={styles.bgPress} onPress={onRequestClose} />
        <View pointerEvents="box-none">
          <VideoPlayer
            height={videoHeight}
            autoStart={true}
            playlist={video.playlist}
            onComplete={() => onRequestClose()}
          />
        </View>
      </View>
    </Modal>
  )
}

const styles = StyleSheet.create({
  modalContainer: {
    flex: 1,
    position: 'relative',
    justifyContent: 'center',
    backgroundColor: 'rgba(0,0,0,0.5)',
  },
  bgPress: {
    position: 'absolute',
    width: '100%',
    height: '100%',
  },
})

VideoPlayer.ts

import JWPlayer, {
  JwConfig,
  JwPlaylistItem,
} from '@jwplayer/jwplayer-react-native'
import { useRef, useState } from 'react'
import { Platform, View } from 'react-native'

interface VideoPlayerProps {
  playlist: JwPlaylistItem[]
  autoStart?: boolean
  height: number
  onPlay?: (player: JWPlayer) => void
  onPause?: (player: JWPlayer) => void
  onComplete?: (player: JWPlayer) => void
}

export function VideoPlayer({
  playlist,
  autoStart = false,
  height,
  onPlay,
  onPause,
  onComplete,
}: VideoPlayerProps) {
  const jwPlayerRef = useRef<JWPlayer>(null)
  const [isFullScreen, setIsFullScreen] = useState(false)

  // JWPlayer isn't reactive to playlist changes so don't render until we have one.
  if (!playlist.length) {
    return null
  }

  const config: JwConfig = {
    license:
      Platform.OS === 'android'
        ? process.env.EXPO_PUBLIC_LICENSE_KEY_ANDROID || ''
        : process.env.EXPO_PUBLIC_LICENSE_KEY_IOS || '',
    autostart: autoStart,
    playlist,
  }

  const onVideoPlay = () => {
    onPlay?.(jwPlayerRef.current!)
  }
  const onVideoPause = () => {
    onPause?.(jwPlayerRef.current!)
  }
  const onVideoComplete = () => {
    onComplete?.(jwPlayerRef.current!)
  }

  return (
    <View>
      <JWPlayer
        ref={jwPlayerRef}
        style={isFullScreen ? undefined : { height }}
        config={config}
        onFullScreen={() => setIsFullScreen(true)}
        onFullScreenExit={() => setIsFullScreen(false)}
        onPlay={onVideoPlay}
        onPause={onVideoPause}
        onComplete={onVideoComplete}
      />
    </View>
  )
}
Jmilham21 commented 3 months ago

Thanks @dominictobias. I'll follow up when I can dig into this with your configuration. Thanks for the info.

Jmilham21 commented 3 months ago

@dominictobias do you have the prop playerInModal: true set? I see now that we didn't put playerInModal into the JwConfig type definition so we need to resolve this still. I don't see that in the snippets you sent over (thank you for this).

dominictobias commented 3 months ago

Sorry you are right I forgot to add it. It works great with playerInModal: true 👏 Yes would be good if it was added to types

dominictobias commented 2 months ago

Unfortunately the published version 1.0.1 is completely broken (videos are sometimes black, there are low level exceptions when going between them) while the master version mostly works (only occasional low level exception crashes when swiping too fast between videos) with or without playerInModal (at least in the custom modal I ended up making for other reasons)

Sadly doing "@jwplayer/jwplayer-react-native": "https://github.com/jwplayer/jwplayer-react-native#master", is breaking in our remote Expo build for some reason though. It would be great to get a published version.

Jmilham21 commented 2 months ago

@dominictobias I published our changes in version 1.0.2 to NPM. Please verify.

dominictobias commented 2 months ago

@dominictobias I published our changes in version 1.0.2 to NPM. Please verify.

The playerInModal is a JWPlayer prop right? It doesn't seem to exist in either JwConfig or JwPlayer in TS defs still. But 1.0.2 is working better thanks

Jmilham21 commented 2 months ago

Yes It's intended to be a Config or JwConfig prop. The Type Def was only made for Config and we need to also add it to JwConfig.

I'll make a new Issue for that and have it resolved in the next release, though I'll make this done.