-
Notifications
You must be signed in to change notification settings - Fork 284
Fixed audio track and subtitle track issues in Android #547
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
base: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @naylinndev, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses compatibility issues and introduces enhancements by upgrading the underlying VLC player library on Android to version 4.0.0. It involves a comprehensive update of the platform-specific code, particularly for Android and iOS, to integrate with the new VLC APIs and align with the latest Pigeon messaging conventions. The changes aim to resolve existing track-related problems and ensure the player's functionality remains robust with modern dependencies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request updates the Android VLC player dependency to version 4.0.0 and adapts the codebase to the new API, fixing audio and subtitle track issues. The changes are extensive, especially in the native Android and iOS code, to align with the updated Pigeon-generated interfaces. While the effort to adapt to the new API is commendable, I've identified several critical issues related to null safety and potential crashes in both the Android (Java) and iOS (Swift) implementations. These issues stem from missing null checks that were present in the previous version, and force unwrapping of optional values. I've also found some minor issues like copy-paste errors in log messages and code formatting inconsistencies. Addressing the critical issues is essential to ensure the stability of the plugin.
...lc_player/android/src/main/java/software/solid/fluttervlcplayer/FlutterVlcPlayerBuilder.java
Show resolved
Hide resolved
...lc_player/android/src/main/java/software/solid/fluttervlcplayer/FlutterVlcPlayerBuilder.java
Show resolved
Hide resolved
...lc_player/android/src/main/java/software/solid/fluttervlcplayer/FlutterVlcPlayerBuilder.java
Show resolved
Hide resolved
| try { | ||
|
|
||
| if ( index >= 0) { | ||
| // Select the track by its ID | ||
| mediaPlayer.selectTrack("audio/"+index); | ||
| } else if (index == -1) { | ||
| // Unselect all audio tracks | ||
| mediaPlayer.unselectTrackType(IMedia.Track.Type.Audio); | ||
| } | ||
| } catch (Exception e) { | ||
| Log.e(TAG, "setAudioTrack failed: " + e.getMessage()); | ||
| } |
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.
The code formatting in this method is inconsistent with standard Java conventions, which affects readability. The empty line and indentation should be fixed.
try {
if (index >= 0) {
// Select the track by its ID
mediaPlayer.selectTrack("audio/" + index);
} else if (index == -1) {
// Unselect all audio tracks
mediaPlayer.unselectTrackType(IMedia.Track.Type.Audio);
}
} catch (Exception e) {
Log.e(TAG, "setAudioTrack failed: " + e.getMessage());
}| DataSourceType (int type) | ||
| { | ||
| this.mType = type; | ||
| } |
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.
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.
I think it's a reasonable comment - can we fix it?
flutter_vlc_player/android/src/main/java/software/solid/fluttervlcplayer/VLCTextureView.java
Show resolved
Hide resolved
| HwAcc (int type) | ||
| { | ||
| this.mType = type; | ||
| } |
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.
flutter_vlc_player/android/src/main/java/software/solid/fluttervlcplayer/FlutterVlcPlayer.java
Show resolved
Hide resolved
| enum HWAccellerationType: Int | ||
| { |
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.
Should we rool-back those formatting changes to avoid tracking it in the diff? Any reason we want to re-format?
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.
When I tried using the master branch, I couldn’t run it on iOS.
Then I pulled the code directly from tag 7.4.4, which contains the HWAccellerationType code.
I mean there were no changes for iOS—I just pulled the code from tag 7.4.4 to make the example app run.
illia-romanenko
left a comment
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 for your PR! Please review comments by Gemini in formatting other cases and resolve them as I think the package in general will benefit from it. Looking forward to merge it.
|
@naylinndev By the way, may I ask why we needed to change the minimum SDK version? Thank you! 🙏 |
Fixed audio track and subtitle track issues in Android after updated Android VLC player V4.0.0