-
-
Notifications
You must be signed in to change notification settings - Fork 265
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
Adds option to change the decoder used by the player #893
Adds option to change the decoder used by the player #893
Conversation
…ation for tracks that are not video
I did a quick test on both the emulator and the real device and I got mixed results. This feature works as expected but the actual device might not support decoding the track with the specified decoder but that is more of an ExoPlayer issue than ours. I was thinking of maybe forcing the decoding even if the decoder doesn't support the capabilities(resolutions, profiles) but this turns out to be the default behavior of ExpoPlayer. Here is a quick demo video on the emulator. Kapture.2022-11-20.at.09.19.52.mp4 |
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.
Nice work! I tweaked some small stuff myself already, but still left some other comments for you. Looking forward to merging this once these are addressed.
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/PlayerViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/jellyfin/mobile/player/source/JellyfinMediaSource.kt
Outdated
Show resolved
Hide resolved
Thanks, @Maxr1998 for taking the time to review the code. I have made the requested changes. Please let me know if I need to change anything. |
Thanks, I left some more comments in the threads above. The rest is good now. One last request, I usually prefer line comments to start with an uppercase letter, so I'd appreciate if you could fix that as well. I apologize for being a bit nitpicky, I just prefer consistency 😅 |
…case, added setter for startTimeMs
I understand @Maxr1998, I would have done the same if I were you. Since I am very new to this repository I will be making a lot of rookie mistakes so feel free to correct me. It will take me some time to get familiar with the coding style and app architecture. |
It's ok, I just have relatively specific standards. Some of these style requirements should probably be documented somewhere. |
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.
Thanks a lot for your contribution!
Changes
This PR adds an option to change between Hardware and Software decoding. When this option is changed the current player is recreated with the new decoder. We are using the
mediaCodecSelector
to control which decoder we want to select for the particular codec. The propertyMediaCodecInfo.hardwareAccelerated
should be taken with a grain of salt.Issues
Closes #760