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

[Android] with 'fullscreen' property playback controls are missing #2864

Closed
bulkinav opened this issue Sep 21, 2022 · 21 comments
Closed

[Android] with 'fullscreen' property playback controls are missing #2864

bulkinav opened this issue Sep 21, 2022 · 21 comments

Comments

@bulkinav
Copy link

bulkinav commented Sep 21, 2022

Bug

Tested 6.0.0-alpha4 and see the following issue:

On Android with 'fullscreen' property playback controls are missing. Tapping on the screen also does not cause the playback to appear.

If don't use 'fullscreen' the controls are presented, BUT the player runs not in fullscreen mode (this is understandable by the "Switch to fullscreen mode" button). After clicking the "Switch to fullscreen mode" button, the fullscreen appears.

https://github.com/react-native-video/react-native-video/blob/master/API.md#controls - it says here that the 'fullscreen' property is applicable only for iOS. But as you can see, and for Android too.

Platform

Android (using Pixel 5a with Android 13)

Environment info

React native info output:

System:
    OS: macOS 12.6
    CPU: (6) x64 Intel(R) Core(TM) i5-8500 CPU @ 3.00GHz
    Memory: 31.04 MB / 24.00 GB
    Shell: 3.2.57 - /bin/bash
  Binaries:
    Node: 18.8.0 - /usr/local/bin/node
    Yarn: 1.22.19 - /usr/local/bin/yarn
    npm: 8.5.5 - /usr/local/bin/npm
    Watchman: 2022.08.29.00 - /usr/local/bin/watchman
  Managers:
    CocoaPods: 1.11.3 - /usr/local/bin/pod
  SDKs:
    iOS SDK:
      Platforms: DriverKit 21.4, iOS 16.0, macOS 12.3, tvOS 16.0, watchOS 9.0
    Android SDK:
      API Levels: 28, 29, 30, 31
      Build Tools: 30.0.2, 30.0.3
      System Images: android-22 | Google APIs Intel x86 Atom_64, android-28 | Android TV Intel x86 Atom, android-30 | Intel x86 Atom_64, android-30 | Google TV Intel x86 Atom, android-30 | Google APIs Intel x86 Atom
      Android NDK: Not Found
  IDEs:
    Android Studio: Dolphin 2021.3.1 Dolphin 2021.3.1
    Xcode: 14.0/14A309 - /usr/bin/xcodebuild
  Languages:
    Java: 1.8.0_312 - /usr/bin/javac
  npmPackages:
    @react-native-community/cli: Not Found
    react: 18.2.0 => 18.2.0
    react-native: 0.67.4 => 0.67.4
    react-native-macos: Not Found
  npmGlobalPackages:
    *react-native*: Not Found

Library version: 6.0.0-alpha4

Steps To Reproduce

  1. Open the app
  2. Tap on the video tile -> video player runs in fullscreen mode, playback controls are missing

Expected behaviour

Playback controls are presented

Reproducible sample code

        <View style={styles.container}>
            <Video
                source={{ uri: uri }}
                onProgress={handleProgress}
                onPlaybackRateChange={handlePlaybackRateChange}
                onEnd={handleEnd}
                controls={isNeedControls}
                pictureInPicture
                fullscreen
                fullscreenOrientation="landscape"
                style={StyleSheet.absoluteFill}
            />
        </View>
@freeboub
Copy link
Collaborator

Ok,I will clarify doc! I confirm it is available on Android.
Regarding to your issue, that you want to do is not clear to me.
I understand you want to use in the same time controls and Fullscreen props ? If that is the case it is not expecting to work like this...

  • either you use default controls ( you will have a Fullscreen button on Android player and leave Fullscreen button will be displayed once in Fullscreen)
  • either you use Fullscreen and you implement your own controller UI
    Please clarify your expectations

@freeboub freeboub added Platform: Android triage needed Help needed to confirm the issue labels Sep 22, 2022
@bulkinav
Copy link
Author

@freeboub thanks for answer.

In my app the video should play only in fullscreen mode. That is, I do not use the "mini" player placed on the user screen (as described here: #2763 (comment))

So, yes, I shoud use 'fullscreen' property. But shoud I use 'controls=true'? If no, how will the native playback controls be displayed?

I also want to note that there was no such problem with 6.0.0-alpha 2.

@freeboub
Copy link
Collaborator

@bulkinav I can confirm that last change broke this behavior. I didn't expect someone do this...
I will have a look in coming days.

@freeboub freeboub added bug and removed triage needed Help needed to confirm the issue labels Sep 23, 2022
@freeboub
Copy link
Collaborator

ping @wood1986 if you have more time than me :)

@wood1986
Copy link
Contributor

@bulkinav Is it possible to record a video? I do not understand the issue

@freeboub
Copy link
Collaborator

@wood1986 the issue happen when Fullscreen and controls are both true.
Fullscreen expect to force player in Fullscreen
Controls expect to show the video
But when both are set, it expected to go in Fullscreen, but without the Fullscreen button and without the panelView you added

@wood1986
Copy link
Contributor

Ok I will follow up this weekend.

@wood1986
Copy link
Contributor

wood1986 commented Oct 2, 2022

@freeboub I do not quite understand.

But when both are set, it expected to go in Fullscreen, but without the Fullscreen button and without the panelView you added

When both set to true, this means native control will be used. native will handle the full screen button onPress and enter/exit the Dialog.

What I can see the bug is when both set to true, it did not enter full screen mode after Video component is mounted (at the very beginning). As for this bug, I do not know how to fix it properly. Currently the behaviour of setFullscreen on android depends on the value of controls and this is necessary. The initialization order is setFullscreen is always called before setControls. I do not know how I can setControls first before setFullscreen.

I have a workaround for android. iOS is broken with this code both in v5 and v6 due to this #2808

<Video
  ref={videoRef}
  source={require('./playstation.mp4')}
  controls={true}
  resizeMode={'contain'}
  paused={true}
  style={[{flex: 1, height: '100%', width: '100%'}]}
  onLoad={() => {
    videoRef.current?.seek(0);
    videoRef.current?.presentFullscreenPlayer();
  }}
/>

I think fullscreen props is not reliable and we should deprecate it because when user tap the fullscreen icon via the native control, fullscreen props does not reflect the right state.

I think the workaround is the right code and we do not need to fix this issue and we only need to fix #2808. Then both iOS and android will have same behaviour with the "workaround" code.

@freeboub
Copy link
Collaborator

freeboub commented Oct 2, 2022

Thank you for the feedback!
Regarding to #2808 did you try the workaround indicated in the ticket ?

Unfortunnatly we cannot deprecate fullscreen as it is usefull when you implement you own controls.
I understand schedule of events issue.
On side, I am wondering if moving a lot of props inside the source parameter: see: #2693 can be a solution
If we move controls inside source it will solve the issue. I think it make sense as either you use controls either not, it doesn't make sens to activate deactivate controls on runtime.
Can you confirm it can be a solution ?

@wood1986
Copy link
Contributor

wood1986 commented Oct 2, 2022

it is usefull when you implement you own controls.

Wait. People still can keep track of the fullscreen via useState in these callbacks in onFullscreenPlayerDid{Present/Dismiss}. When their control has fullscreen button, they can use presentFullscrenPlayer(). I expect they still can implement their own controls.

Before investigating/introducing the startup config props, I want to know the possibility of deprecating fullscreen props after my latest point and to see if my workaround is the right way.

For now, I think #2808 is higher priority but the solution is based on a very old commit. And I will try to fix today. Then we can have the feature parity for both android and iOS.

Feel free to let me know if I am wrong

@freeboub
Copy link
Collaborator

freeboub commented Nov 3, 2022

@bulkinav @wood1986 I finnally found time to propose a fix here: #2911
@bulkinav can you check and confirm it solves your issue
@wood1986 I don't think deprecating fullscreen prop will solve anything, it will just bring complexity in application (OK, having both, keep complexity in the package :) ). please let me know if this change looks OK for you !

@bulkinav
Copy link
Author

bulkinav commented Nov 3, 2022

@freeboub thanks for your time. Unfortunately, I can still reproduce this problem.

Several behaviors are considered below:

  1. Use only 'controls' props: the player runs not in fullscreen mode with playback controls. After clicking the "Switch to fullscreen mode" button, the fullscreen appears. I thinks, this is correct behavior.

  2. Use only 'fullscreen' props: the player runs in fullscreen mode, but without playback controls. I thinks, this is correct behavior too.

  3. Use 'controls' and 'fullscreen' props: the player runs in fullscreen mode, but without playback controls. So, this is not normal, and this repeats point 2.

@freeboub
Copy link
Collaborator

freeboub commented Nov 3, 2022

3. Use '**controls**' and '**fullscreen**' pros: https://disk.yandex.ru/i/TFD6K3gprE-nHw - the player runs in fullscreen mode, but without playback controls. So, this is not normal, and this repeats point 2.

I cannot read correctly the video you provide... Anyway, when you say there is no control, you Don't see any control ? This patch should only remove the Fullscreen button in that case 🤔

@bulkinav
Copy link
Author

bulkinav commented Nov 3, 2022

@freeboub I changed example links.

This patch should only remove the Fullscreen button in that case

Yes, and I several times rebuild project / cleaned cache, but it didn't help.

@freeboub
Copy link
Collaborator

freeboub commented Nov 3, 2022

@bulkinav I reproduce the issue, I confirm this happens only on first playback start (and this should not be linked to @wood1986 change)
The fix I provided, fix an issue, but not this one ...

@freeboub
Copy link
Collaborator

freeboub commented Nov 3, 2022

@bulkinav It was harder than expected.
There was an issue in views creation linked @wood1986 change in fact.
I also review fullscreen button visibility.
Please let me know if it works for you.

@bulkinav
Copy link
Author

bulkinav commented Nov 5, 2022

@freeboub yes, I confirm - fullscreen playback now has controls.

Thanks for a time to fix this problem.

@freeboub
Copy link
Collaborator

freeboub commented Nov 7, 2022

@bulkinav I update the PR, can you please approve it ? I will generate a release soone

@bulkinav
Copy link
Author

bulkinav commented Nov 8, 2022

@freeboub I see you have removed this line:

if (player == null || exoPlayerView == null) return;

Yes, the PR also works as expected.

@freeboub
Copy link
Collaborator

freeboub commented Nov 8, 2022

@bulkinav I think these tests are messy,

for exemple:

        if (player == null || exoPlayerView == null) return;
        if (controls) {
            addPlayerControl();
            updateFullScreenButtonVisbility();

the test should be done in the subfunction, not in master function. For exemple depending on startup process (start playback is asynchronious) we can miss to apply control option correctly

@freeboub
Copy link
Collaborator

Has been merged, fix will be available on alpha.4

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

No branches or pull requests

3 participants