-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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(android): replace deprecated ExoPlayer2 with AndroidX media3 #3337
feat(android): replace deprecated ExoPlayer2 with AndroidX media3 #3337
Conversation
android/src/main/java/com/brentvatne/receiver/AudioBecomingNoisyReceiver.java
Outdated
Show resolved
Hide resolved
Thank you for the PR, it looks easy to merge ! Just 2 small comments to check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@YangJonghun please do rebase. On master there are important changes like move versions to gradle.properties
. Thanks for your PR 🙏
Conflicts: android/build.gradle android/src/main/java/com/brentvatne/common/react/VideoEventEmitter.java
@@ -1,22 +0,0 @@ | |||
package com.brentvatne; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please restore this file.
It will be used soon !
Thank you
@@ -1,6 +1,6 @@ | |||
package com.brentvatne.common.API | |||
|
|||
import com.brentvatne.ReactBridgeUtils | |||
import com.brentvatne.common.toolbox.ReactBridgeUtils |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look like you move the file, but not re add it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I just connect ReactBridgeUtils.kt and remove old file(ReactBridgeUtils.java)
these files are already existed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you, good catch then !
@@ -266,7 +264,7 @@ public void onCues(List<Cue> cues) { | |||
subtitleLayout.setCues(cues); | |||
} | |||
|
|||
// ExoPlayer.VideoListener implementation | |||
// SimpleExoPlayer.VideoListener implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// SimpleExoPlayer.VideoListener implementation | |
// ExoPlayer.VideoListener implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. I think we don't need comments anymore. because ComponentListener implement only one interface (It was required in v5)
If you don't want to remove it, please revert a29d85d commit
one last point, and the code is Ok for me (I will need to test it also) |
@YangJonghun It's me or native controls don't work ? |
I confirm that controls are broken. LOG {"error":{"errorStackTrace":"android.view.InflateException: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Error inflating class com.google.android.exoplayer2.ui.DefaultTimeBar\nCaused by: android.view.InflateException: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Error inflating class com.google.android.exoplayer2.ui.DefaultTimeBar\nCaused by: java.lang.ClassNotFoundException: com.google.android.exoplayer2.ui.DefaultTimeBar\n\tat java.lang.Class.classForName(Native Method)\n\tat java.lang.Class.forName(Class.java:454)\n\tat android.view.LayoutInflater.createView(LayoutInflater.java:819)\n\tat android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:1010)\n\tat android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:965)\n\tat android.view.LayoutInflater.rInflate(LayoutInflater.java:1127)\n\tat android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:1088)\n\tat android.view.LayoutInflater.rInflate(LayoutInflater.java:1130)\n\tat android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:1088)\n\tat android.view.LayoutInflater.inflate(LayoutInflater.java:686)\n\tat android.view.LayoutInflater.inflate(LayoutInflater.java:538)\n\tat android.view.LayoutInflater.inflate(LayoutInflater.java:485)\n\tat androidx.media3.ui.PlayerControlView.(PlayerControlView.java:419)\n\tat androidx.media3.ui.PlayerControlView.(PlayerControlView.java:355)\n\tat androidx.media3.ui.PlayerControlView.(PlayerControlView.java:351)\n\tat androidx.media3.ui.PlayerControlView.(PlayerControlView.java:347)\n\tat com.brentvatne.exoplayer.ReactExoplayerView.initializePlayerControl(ReactExoplayerView.java:394)\n\tat com.brentvatne.exoplayer.ReactExoplayerView.finishPlayerInitialization(ReactExoplayerView.java:717)\n\tat com.brentvatne.exoplayer.ReactExoplayerView.initializePlayerSource(ReactExoplayerView.java:712)\n\tat com.brentvatne.exoplayer.ReactExoplayerView.lambda$initializePlayer$4$com-brentvatne-exoplayer-ReactExoplayerView(ReactExoplayerView.java:573)\n\tat com.brentvatne.exoplayer.ReactExoplayerView$$ExternalSyntheticLambda12.run(Unknown Source:6)\n\tat android.os.Handler.handleCallback(Handler.java:942)\n\tat android.os.Handler.dispatchMessage(Handler.java:99)\n\tat android.os.Looper.loopOnce(Looper.java:201)\n\tat android.os.Looper.loop(Looper.java:288)\n\tat android.app.ActivityThread.main(ActivityThread.java:7898)\n\tat java.lang.reflect.Method.invoke(Native Method)\n\tat com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)\n\tat com.android.internal.os.ZygoteInit.main(ZygoteInit.java:936)\nCaused by: java.lang.ClassNotFoundException: Didn't find class "com.google.android.exoplayer2.ui.DefaultTimeBar" on path: DexPathList[[zip file "/data/app/~~vUs5ytyVyQva3WXiYoOUEg==/com.videoplayer-rwz9E_zQ5RLHJaxAM-DRQg==/base.apk"],nativeLibraryDirectories=[/data/app/~~vUs5ytyVyQva3WXiYoOUEg==/com.videoplayer-rwz9E_zQ5RLHJaxAM-DRQg==/lib/arm64, /data/app/~~vUs5ytyVyQva3WXiYoOUEg==/com.videoplayer-rwz9E_zQ5RLHJaxAM-DRQg==/base.apk!/lib/arm64-v8a, /system/lib64, /system_ext/lib64]]\n\tat dalvik.system.BaseDexClassLoader.findClass(BaseDexClassLoader.java:259)\n\tat java.lang.ClassLoader.loadClass(ClassLoader.java:379)\n\tat java.lang.ClassLoader.loadClass(ClassLoader.java:312)\n\t... 29 more\n","errorCode":"1001","errorException":"android.view.InflateException: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Error inflating class com.google.android.exoplayer2.ui.DefaultTimeBar","errorString":"android.view.InflateException: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Binary XML file line #63 in com.videoplayer:layout/exo_player_control_view: Error inflating class com.google.android.exoplayer2.ui.DefaultTimeBar"}} |
@YangJonghun in file: ./android/src/main/res/layout/exo_player_control_view.xml |
@KrzysztofMoch @freeboub FYI) https://developer.android.com/reference/androidx/media3/ui/LegacyPlayerControlView |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like all is working 🚀 Thanks for your PR!
@KrzysztofMoch @freeboub And if possible, it would be nice if you could comment on why this PR was held. |
PR is hold because we would like to bump minor version number for migration from
I don't mind if you do migration based on this branch but in this case kotlin migration will be also held to moment where we will merge this PR. Lets see what @freeboub think about it Thanks for you PR and Sorry for the trouble 🙏 |
@YangJonghun First Thank you for the proposal ! Regarding to the hold state, It should not be too long. We clearly should move to media3 soon. It would be great to go out of alpha state before merging this PR. Keep in mind that there is another PR which introduce more kotlin : #3204 Regarding to RxKotlin, I am not really fan of it (I guess this is the same philosophy than RxJs). but I have no experience with coroutines. You can create a discussion if you want advices or way to progress, but please don't do a PR which moves all the code in kotlin, it will be too hard to review ! |
@freeboub However, I don't think there is much risk in just migrating to Kotlin, and if you think there is, I could create a PR for each file or exclude large files like ReactExoPlayer and migrate them first. But I respect your opinion, so I won't create a PR if you still don't want to. I'm not sure if this is the right place for this, but I'm a fan of this library and have been using it for almost 4 years, but I think the update rate is slow, which implies that the alpha version is already unstable, and I think the alpha version needs to change faster and be used and exposed to more users to avoid more risks. I hope you don't take this as a blame, as I trust and appreciate your contributions and hard work very much. |
@YangJonghun I agree it should not be risky to switch to kotlin, but who really knows :) Moreover I think this repo is very important for react native ecosystem That said, I don't take your comment as a blame, but you highlight the necessity of more contributors, and I fully agree with you ! So Yes you can update code to kotlin, I will review PRs and merge them with pleasure ! |
media3 1.2.0 has been released yesterday 😂 |
It is a joke, 1.1.1 looks good for now ! |
@YangJonghun We discussed internally, and we agree to merge this PR. |
Can't we support the recent release v1.2.0, or any point release before v6 or future beta versions? I hope, the most challenging part is already achieved, isn't it? |
@asharamseervi yes we can use it, I don't think this is a major issue, but it will force us to use the last android sdk, where react native core team doesn't support it officially. |
Update the changelog
feat(android): replace deprecated ExoPlayer2 with AndroidX media3
Describe the changes
AndroidX
dependenciesokhttp
dependencygetExtOrDefault
ReactBridgeUtils
FYI)
v6.0.0-alpha.9
code