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

Support for displaying controls in Android Exoplayer #1414

Merged
merged 9 commits into from
Feb 11, 2019

Conversation

IbrahimSulai
Copy link
Contributor

Added support for displaying player controls in Android Exoplayer component using PlayerControlView
Utilised the existing boolean prop 'controls' to show/hide the native controls for the player.

Sample Usage

<Video
  source={
    {uri: 'https://www.w3schools.com/html/mov_bbb.mp4'}
  }
  controls={true}
/>

@kcalmes
Copy link

kcalmes commented Jan 8, 2019

When will this pull request be accepted?

@IbrahimSulai
Copy link
Contributor Author

@cobarx
could you help to review and merge.

@IbrahimSulai
Copy link
Contributor Author

@n1ru4l @cobarx - Could you please help to review this pull request.

Copy link
Contributor

@cobarx cobarx left a comment

Choose a reason for hiding this comment

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

Hi @IbrahimSulai, sorry for the delay in looking at this, I've been sick the last week so haven't gotten to put any time into the project.

Here are some minor changes that I saw by glancing over the PR. I still need to run the code and see how it works.

Thanks for putting this together! I had no idea that ExoPlayer included built-in controls so this makes it so much easier to implement this feature. Nice work!

Go ahead and make the changes, I will test this out in the next day or two.

README.md Outdated Show resolved Hide resolved
@IbrahimSulai
Copy link
Contributor Author

@cobarx - Thanks for your valuable review comments. I have resolved the same. Please review the changes.

I hope you are doing good regarding your health. Take care.

@AndriiUhryn
Copy link

Is there any news about this PR? @cobarx

@mgezkaya
Copy link

mgezkaya commented Jan 22, 2019

Hey @cobarx I have tried this PR on Android in my local and it's really works. It has some issues that needs to be solved although but I think it's still better than nothing. I hope you can merge it when you are available. And @IbrahimSulai thanks for your attention but can you apply a change for fullScreen on control, too? Because i couldn't see and i really need it. Thank you both.

@FelipeSSantos1
Copy link

Please @cobarx merge it!!!!! 🎉🎉🎉 thank you @IbrahimSulai

@cobarx
Copy link
Contributor

cobarx commented Jan 23, 2019

Ok I tested this and am seeing a weird behavior where when I press pause, the play button doesn't appear there is just a blank space. Any idea what's going on here? Here is a screenshot:
no play

Also, if I start the video with controls={true} then change it to false, the controls don't disappear. I looked at the code and this should be possible, can you update it to handle this?

@IbrahimSulai
Copy link
Contributor Author

@cobarx - I will look into the above UI issue and also will handle the controls props.

@IbrahimSulai
Copy link
Contributor Author

Ok I tested this and am seeing a weird behavior where when I press pause, the play button doesn't appear there is just a blank space. Any idea what's going on here? Here is a screenshot:
no play

Also, if I start the video with controls={true} then change it to false, the controls don't disappear. I looked at the code and this should be possible, can you update it to handle this?

@cobarx - I have fixed the UI player control UI issue. Also handled the controls prop state. Could you please review it.

@mgezkaya
Copy link

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@IbrahimSulai
Copy link
Contributor Author

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@mgezkaya , Sure. Once @cobarx merge this, will add the full screen feature as a separate PR. Thanks.

@mgezkaya
Copy link

@IbrahimSulai Can you apply a button for fullscreen if you can, please?

@mgezkaya , Sure. Once @cobarx merge this, will add the full screen feature as a separate PR. Thanks.

Thanks for return

@xstable
Copy link

xstable commented Jan 28, 2019

@IbrahimSulai didn't work for me on Android 9.
Play-Button still disappear after pressing Pause.

But if I put the App into background, and get it back to foreground, it autoplays the video, and "Pause" button is visible.
If I start the Video with "paused", I see the Play-Button. And If I press on it, the "pause"Button disappear.

Here is my patch-file: https://hastebin.com/jugowesova.diff

@IbrahimSulai
Copy link
Contributor Author

IbrahimSulai commented Jan 28, 2019

@IbrahimSulai didn't work for me on Android 9.
Play-Button still disappear after pressing Pause.

But if I put the App into background, and get it back to foreground, it autoplays the video, and "Pause" button is visible.

Here is my patch-file: https://hastebin.com/ximupiwexe.diff

@xstable
I tested in Android version 9 and lower versions (Models: Google Pixel, OnePlus5T, and Nexus), the player works as expected. Can you share more details (like Device Model) on the issue you are facing?

I’m not sure about the patch package you are using, so can you also please try to use the changes AS IS from the PR instead of trying via any patch/patch packages.

PFB, the recording of the player.
exoplayercontrols

@xstable
Copy link

xstable commented Jan 29, 2019

@IbrahimSulai The Patch is my own one, I've created with patch-package. It contains the diff between default "react-native-video" and my changes.

My Device is: Xiaomi Mi A1 & Samsung S4

Maybe I've missed something, or have an Issue in my code, so here I've create a package of my changed source-code from android-explayer so that you can have a look inside.
android-exoplayer.tar.gz

@IbrahimSulai
Copy link
Contributor Author

@xstable
I have tested the app in Samsung S4, it is working as expected. Do you have a specific scenario where this is happening since I am not able to reproduce this.

Could you please share your mail id. So, that we can communicate in person in the below slack channel to solve this.

https://ibrahim-6ic2441.slack.com/messages/CFS1PQ3HQ

@IbrahimSulai
Copy link
Contributor Author

@IbrahimSulai The Patch is my own one, I've created with patch-package. It contains the diff between default "react-native-video" and my changes.

My Device is: Xiaomi Mi A1 & Samsung S4

Maybe I've missed something, or have an Issue in my code, so here I've create a package of my changed source-code from android-explayer so that you can have a look inside.
android-exoplayer.tar.gz

I can able to reproduce the issue with your patch file. I debugged the issue, the player states are working exactly fine. But, there is a UI glitch with the play/pause button. I am working on it to find the root cause for the same, will get back with the solution ASAP.

@IbrahimSulai
Copy link
Contributor Author

@cobarx @n1ru4l

I have fixed the issue observed in the test harness.

Changed the execution order of initializePlayerControl method in order to align with the player state. Since the createView is executed before the "setControls" props method gets called. so the values of the playerControl are not aligned with the player state values.

Could you please review and merge the same.

@cobarx
Copy link
Contributor

cobarx commented Feb 11, 2019

Very nice, this is working wonderfully for me in the test harness. I'm merging now. There are going to be a lot of happy people using this!

Adding the controls exposes a lot of new functionality that you could develop further (if you have the time and interest). If you want to do another PR, it would be great to have onPause & onPlay events so that the app can be notified that the play/pause state changed due a click within the controls. It might also be good to implement some kind of auto-hide behavior so the controls disappear after a certain amount of time, configurable by a prop.

Keep up the good work!

@cobarx cobarx merged commit e0fd69e into TheWidlarzGroup:master Feb 11, 2019
@FelipeSSantos1
Copy link

Thank you guys! I was wanting this merge!

@IbrahimSulai
Copy link
Contributor Author

IbrahimSulai commented Feb 11, 2019

Very nice, this is working wonderfully for me in the test harness. I'm merging now. There are going to be a lot of happy people using this!

Adding the controls exposes a lot of new functionality that you could develop further (if you have the time and interest). If you want to do another PR, it would be great to have onPause & onPlay events so that the app can be notified that the play/pause state changed due a click within the controls. It might also be good to implement some kind of auto-hide behavior so the controls disappear after a certain amount of time, configurable by a prop.

Keep up the good work!

Thank you @cobarx and others in the community who supported for this feature to get merged.
Sure, I will be focusing on the other key features related to this.

@AndriiUhryn
Copy link

@IbrahimSulai Is there an ability to make video fullScreen on Android?

@IbrahimSulai
Copy link
Contributor Author

@IbrahimSulai Is there an ability to make video fullScreen on Android?

Will add the full-screen feature in upcoming PR.

@xstable
Copy link

xstable commented Feb 26, 2019

@cobarx
Isn't that wrong... now after you've merged @IbrahimSulai commit... and as we can activate video-controls by a property now ?

As you don't have to create your own controls, but you are "able to do".
BTW: Because react-native-video-player is depending on react-native-video ^2.0.0 I would remove the suggestion to use it in the docs.

@xstable
Copy link

xstable commented Feb 28, 2019

Because I hardly needed the fullscreen-functionality, I forked and upgraded react-native-video-player so that it now support react-native-video 4.4.0. Feel free to give it a try.

But I still looking forward to @IbrahimSulai Solution to have the whole functionality inside of react-native-video. So you can take my solution as temporary workaround, till @IbrahimSulai is done with it.

@shaunsantacruz
Copy link

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

@IbrahimSulai
Copy link
Contributor Author

IbrahimSulai commented Mar 15, 2019

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

Currently working on the full-screen functionality for the Android Exoplayer. Once completed, will check on this. Thanks

@datzun
Copy link

datzun commented May 1, 2019

Thank you so much for adding controls! Would it be possible to make them more accessible for dpad/keyboard users? My app is being rejected from the play store due to this.

Currently working on the full-screen functionality for the Android Exoplayer. Once completed, will check on this. Thanks

@IbrahimSulai thanks for adding control support. Any updates on fullscreen functionality?

AndrewSouthpaw pushed a commit to flow-typed/flow-typed that referenced this pull request May 3, 2019
@shmagdy
Copy link

shmagdy commented Jun 17, 2019

@IbrahimSulai
thanks for adding control support. Any updates on fullscreen functionality?

@jonathangreco
Copy link

@IbrahimSulai hey is the fullscreen functionality coming ? So much waiting about this one :)

@IbrahimSulai
Copy link
Contributor Author

IbrahimSulai commented Jul 26, 2019

@jonathangreco
There is a limitation in the android native bridge to change the size of the view. So we can't directly implement the full-screen functionality.

I am planning to control the full-screen functionality in the react-native layer based on the platform. Will let you know once it's done. Thanks

@jonathangreco
Copy link

@IbrahimSulai Hi thx for those news, Iknow that the fullscreen is a very complicated aspect on react native for videos, because on iOS it does not fully work either. I really hope that you work will do the job for all cases.

Keep up the good work and hope this will be released this year :)

@IbrahimSulai
Copy link
Contributor Author

@jonathangreco - Thanks for your support and encouragement.

I will add this feature ASAP.

@IbrahimSulai
Copy link
Contributor Author

@jonathangreco @datzun @shaunsantacruz @sherif-magdy @xstable @AndriiUhryn @mgezkaya

Sorry for the delay. No more wait. Here is the full-screen functionality.

Opened a PR (#1730) for full-screen support in Android Exoplayer.

beauner69 pushed a commit to beauner69/react-native-video that referenced this pull request Oct 10, 2019
Support for displaying controls in Android Exoplayer

(rebased from commit e0fd69e)
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.