android: Implement ExoPlayer based SbPlayer#5899
android: Implement ExoPlayer based SbPlayer#5899osagie98 wants to merge 93 commits intoyoutube:mainfrom
Conversation
osagie98
requested review from
borongc,
jasonzhangxx,
johnedocampo,
sideb0ard and
xiaomings
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @osagie98, 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!
Summary of Changes
Hello! Gemini here, providing a summary for this draft pull request titled "Chrobalt ExoPlayer - not ready for review".
This PR appears to be a work-in-progress effort to integrate the AndroidX Media3 ExoPlayer library into the Cobalt platform on Android. The goal is to replace the existing media player implementation with one based on ExoPlayer, leveraging its capabilities for media playback.
The changes involve adding the necessary ExoPlayer dependencies, introducing new Java and C++ components to bridge Cobalt's Starboard player API with the ExoPlayer Java API, and updating the build configuration to include these new components and dependencies. It also updates some Google Play Services dependencies and refines supported codecs.
Highlights
- ExoPlayer Integration: The core change is the introduction of ExoPlayer as the media playback engine for Cobalt on Android, replacing the previous implementation.
- New Java Bridge Components: Several new Java classes (
ExoPlayerBridge,CobaltMediaSource,CobaltSampleStream,ExoPlayerFormatCreator) have been added to handle the interaction between native Cobalt code and the ExoPlayer library. - New C++ Starboard Player Implementation: New C++ files (
exoplayer.cc,exoplayer_bridge.cc,player_impl.cc) implement the Starboard player API (SbPlayer*functions) by communicating with the new JavaExoPlayerBridgevia JNI. - Dependency Updates: New dependencies for
androidx.media3components (common, container, database, datasource, decoder, exoplayer, extractor, session) have been added, and some Google Play Services dependencies have been updated. - Codec Support Refinement: The supported audio codecs are explicitly limited to Opus and AAC, and supported video codecs to H264 and VP9 in the media support checks.
- Sample Writing Change: The maximum number of samples written per
SbPlayerWriteSamplescall has been changed from 256 to 1, aligning with the new sample-by-sample handling in the ExoPlayer integration.
Changelog
Click here to see the changelog
- DEPS
- Added new CIPD dependencies for androidx_media3 libraries (common, container, database, datasource, decoder, exoplayer, extractor, session) at version 1.2.0.cr1 (lines 2180-2266).
- Corrected path typo for src/third_party/android_deps/cipd/libs/com_google_android_gms_play_services_ads_identifier to src/third_party/android_deps/libs/... (line 2686).
- Updated version for com_google_android_gms_play_services_base from 18.0.1.cr1 to 18.5.0.cr1 (line 2734).
- Updated version for com_google_android_gms_play_services_basement from 18.1.0.cr1 to 18.4.0.cr1 (line 2745).
- Updated version for com_google_android_gms_play_services_tasks from 18.0.2.cr1 to 18.2.0.cr1 (line 2888).
- cobalt/android/BUILD.gn
- Added dependencies on androidx_media3_media3_common_java, androidx_media3_media3_datasource_java, androidx_media3_media3_decoder_java, and androidx_media3_media3_exoplayer_java to cobalt_main_java (lines 135-140).
- Added new Java source files CobaltMediaSource.java, CobaltSampleStream.java, ExoPlayerBridge.java, and ExoPlayerFormatCreator.java to cobalt_main_java sources (lines 194-197).
- cobalt/android/apk/apk_sources.gni
- Added new Java source files CobaltMediaSource.java, CobaltSampleStream.java, ExoPlayerBridge.java, and ExoPlayerFormatCreator.java to apk_sources list (lines 43-46).
- cobalt/android/apk/app/src/main/java/dev/cobalt/coat/StarboardBridge.java
- Imported dev.cobalt.media.ExoPlayerBridge (line 37).
- Added private member exoPlayerBridge (line 69).
- Initialized exoPlayerBridge in the constructor (line 127).
- Added public method getExoPlayerBridge() (lines 568-570).
- cobalt/android/apk/app/src/main/java/dev/cobalt/media/CobaltMediaSource.java
- New file: Implements a custom MediaSource for ExoPlayer to receive media samples from Cobalt.
- cobalt/android/apk/app/src/main/java/dev/cobalt/media/CobaltSampleStream.java
- New file: Implements a custom SampleStream for ExoPlayer to manage and provide media samples from a SampleQueue.
- cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
- New file: Provides a JNI bridge between native C++ and the Java ExoPlayer instance, managing player lifecycle, state, and sample delivery.
- cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerFormatCreator.java
- New file: Helper class to create ExoPlayer Format objects from Cobalt media stream information.
- cobalt/android/apk/app/src/main/java/dev/cobalt/media/MediaCodecBridge.java
- Added import for androidx.media3.exoplayer.ExoPlayer (line 45).
- Added private ExoPlayer player member (line 57).
- starboard/android/shared/BUILD.gn
- Removed several source files related to the previous player implementation (lines 352-365).
- Added new source files for the ExoPlayer-based implementation (exoplayer.cc, exoplayer.h, exoplayer_bridge.cc, exoplayer_bridge.h, player_impl.cc) (lines 368-373).
- starboard/android/shared/exoplayer/exoplayer.cc
- New file: Implements the C++ side of the ExoPlayer integration, mapping Starboard player API calls to the Java bridge.
- starboard/android/shared/exoplayer/exoplayer.h
- New file: Header for the C++ ExoPlayer class.
- starboard/android/shared/exoplayer/exoplayer_bridge.cc
- New file: Implements the C++ JNI bridge to the Java ExoPlayerBridge class.
- starboard/android/shared/exoplayer/exoplayer_bridge.h
- New file: Header for the C++ ExoPlayerBridge class.
- starboard/android/shared/exoplayer/player_impl.cc
- New file: Implements the Starboard player API functions using the new ExoPlayer C++ class.
- starboard/android/shared/media_decoder.h
- Added a blank line (line 22).
- starboard/android/shared/media_is_audio_supported.cc
- Added checks to return false for audio codecs other than Opus and AAC (lines 40-43).
- starboard/android/shared/media_is_video_supported.cc
- Added checks to return false for video codecs other than H264 and VP9 (lines 46-49).
- starboard/android/shared/player_get_maximum_number_of_samples_per_write.cc
- Changed the return value from 256 to 1 (line 19).
- third_party/android_deps/BUILD.gn
- Added android_aar_prebuilt targets for androidx_media3 libraries (common, database, datasource, decoder, exoplayer, extractor, session) (lines 720-813).
- Corrected path for google_play_services_ads_identifier_java aar_path (line 1062).
- Updated aar_path for google_play_services_base_java to version 18.5.0 (line 1111).
- Updated aar_path for google_play_services_basement_java to version 18.4.0 (line 1126).
- Updated aar_path for google_play_services_tasks_java to version 18.2.0 (line 1235).
- Added android_aar_prebuilt target for androidx_media3_media3_container_java (lines 1783-1799).
- third_party/android_deps/additional_readme_paths.json
- Added paths for androidx_media3 libraries (lines 10-17).
- third_party/android_deps/build.gradle
- Added compile dependencies for androidx.media3 libraries (common, database, datasource, decoder, exoplayer, extractor, session) (lines 199-205).
- third_party/android_deps/buildSrc/src/main/groovy/BuildConfigGenerator.groovy
- Added androidx_media3 libraries to the list of dependencies to process (lines 105-116).
- Added debug logging for excluded dependencies (line 408).
- Added a check to isInDifferentRepo to return false for media3 dependencies (lines 588-590).
- third_party/android_deps/buildSrc/src/main/groovy/ChromiumDepGraph.groovy
- Added PROPERTY_OVERRIDES for androidx_media3 libraries with Apache 2.0 license (lines 27-47).
- Updated licenseUrl for org_hamcrest_hamcrest (line 197).
- Updated copyright year for org_jsoup_jsoup (line 6).
- third_party/android_deps/libs/androidx_media3_media3_common/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_common/3pp/fetch.py
- New file: Python script to fetch the media3-common AAR.
- third_party/android_deps/libs/androidx_media3_media3_common/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_common/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_common/README.chromium
- New file: Chromium README for media3-common.
- third_party/android_deps/libs/androidx_media3_media3_common/androidx_media3_media3_common.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_common/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_container/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_container/3pp/fetch.py
- New file: Python script to fetch the media3-container AAR.
- third_party/android_deps/libs/androidx_media3_media3_container/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_container/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_container/README.chromium
- New file: Chromium README for media3-container.
- third_party/android_deps/libs/androidx_media3_media3_container/androidx_media3_media3_container.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_container/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_database/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_database/3pp/fetch.py
- New file: Python script to fetch the media3-database AAR.
- third_party/android_deps/libs/androidx_media3_media3_database/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_database/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_database/README.chromium
- New file: Chromium README for media3-database.
- third_party/android_deps/libs/androidx_media3_media3_database/androidx_media3_media3_database.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_database/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_datasource/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_datasource/3pp/fetch.py
- New file: Python script to fetch the media3-datasource AAR.
- third_party/android_deps/libs/androidx_media3_media3_datasource/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_datasource/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_datasource/README.chromium
- New file: Chromium README for media3-datasource.
- third_party/android_deps/libs/androidx_media3_media3_datasource/androidx_media3_media3_datasource.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_datasource/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_decoder/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_decoder/3pp/fetch.py
- New file: Python script to fetch the media3-decoder AAR.
- third_party/android_deps/libs/androidx_media3_media3_decoder/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_decoder/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_decoder/README.chromium
- New file: Chromium README for media3-decoder.
- third_party/android_deps/libs/androidx_media3_media3_decoder/androidx_media3_media3_decoder.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_decoder/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/3pp/fetch.py
- New file: Python script to fetch the media3-exoplayer AAR.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/README.chromium
- New file: Chromium README for media3-exoplayer.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/androidx_media3_media3_exoplayer.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_exoplayer/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_extractor/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_extractor/3pp/fetch.py
- New file: Python script to fetch the media3-extractor AAR.
- third_party/android_deps/libs/androidx_media3_media3_extractor/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_extractor/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_extractor/README.chromium
- New file: Chromium README for media3-extractor.
- third_party/android_deps/libs/androidx_media3_media3_extractor/androidx_media3_media3_extractor.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_extractor/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/androidx_media3_media3_session/3pp/3pp.pb
- New file: CIPD 3pp definition.
- third_party/android_deps/libs/androidx_media3_media3_session/3pp/fetch.py
- New file: Python script to fetch the media3-session AAR.
- third_party/android_deps/libs/androidx_media3_media3_session/LICENSE
- New file: Apache 2.0 License text.
- third_party/android_deps/libs/androidx_media3_media3_session/OWNERS
- New file: Points to parent OWNERS file.
- third_party/android_deps/libs/androidx_media3_media3_session/README.chromium
- New file: Chromium README for media3-session.
- third_party/android_deps/libs/androidx_media3_media3_session/androidx_media3_media3_session.info
- New file: Generated AAR info file.
- third_party/android_deps/libs/androidx_media3_media3_session/cipd.yaml
- New file: CIPD package definition.
- third_party/android_deps/libs/com_google_android_gms_play_services_ads_identifier/3pp/fetch.py
- Updated fetch script to use direct urllib.request instead of fetch_common (lines 6-78).
- third_party/android_deps/libs/com_google_android_gms_play_services_ads_identifier/README.chromium
- Removed 'Shipped: yes' line (line 9).
- Removed description line (line 12).
- third_party/android_deps/libs/com_google_android_gms_play_services_base/README.chromium
- Updated version from 18.0.1 to 18.5.0 (line 4).
- third_party/android_deps/libs/com_google_android_gms_play_services_base/cipd.yaml
- Updated version tag and file name from 18.0.1.cr1 to 18.5.0.cr1 (lines 6, 10).
- third_party/android_deps/libs/com_google_android_gms_play_services_basement/README.chromium
- Updated version from 18.1.0 to 18.4.0 (line 4).
- third_party/android_deps/libs/com_google_android_gms_play_services_basement/cipd.yaml
- Updated version tag and file name from 18.1.0.cr1 to 18.4.0.cr1 (lines 6, 10).
- third_party/android_deps/libs/com_google_android_gms_play_services_tasks/README.chromium
- Updated version from 18.0.2 to 18.2.0 (line 4).
- third_party/android_deps/libs/com_google_android_gms_play_services_tasks/cipd.yaml
- Updated version tag and file name from 18.0.2.cr1 to 18.2.0.cr1 (lines 6, 10).
- third_party/android_deps/libs/com_google_code_findbugs_jsr305/3pp/fetch.py
- Added version override '3.0.2' (line 19).
- third_party/android_deps/libs/com_google_errorprone_error_prone_annotations/3pp/fetch.py
- Added version override '2.14.0' (line 19).
- third_party/android_deps/libs/com_google_guava_failureaccess/3pp/fetch.py
- Added version override '1.0.1' (line 19).
- third_party/android_deps/libs/com_google_guava_guava_android/3pp/fetch.py
- Added version override '31.1-android' (line 19).
- third_party/android_deps/libs/com_google_j2objc_j2objc_annotations/3pp/fetch.py
- Added version override '1.3' (line 19).
- third_party/android_deps/libs/org_checkerframework_checker_qual/3pp/fetch.py
- Added version override '3.25.0' (line 19).
- third_party/android_deps/libs/org_hamcrest_hamcrest/LICENSE
- Updated license header to BSD 3-Clause License and copyright year (lines 1-3).
- third_party/android_deps/libs/org_jsoup_jsoup/LICENSE
- Updated copyright year from 2023 to 2025 (line 3).
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
From native land to Java's shore,
A bridge is built, and something more.
With ExoPlayer's might,
Media takes flight,
Playing video, sound, and lore.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces a significant change by integrating ExoPlayer (Media3) as the media playback backend for Android. This involves substantial new Java and C++ code, along with updates to dependencies and build configurations.
Given the "[draft]" status in the title, it's understandable that some parts might be incomplete or have placeholders. However, there are a few critical issues identified that would prevent basic functionality like seeking and potentially cause crashes with Opus audio. There are also some high-severity concerns regarding thread management and preroll logic.
Overall, this is a large and complex change. The core structure for ExoPlayer integration seems to be taking shape. Addressing the identified critical and high-severity issues will be essential for progressing this work.
Summary of Findings
- Critical: Seek Functionality Incomplete: The C++ bridge code (
ExoPlayerBridge::DoSeek) has the JNI call to Java'sseekmethod commented out, which will prevent seeking from working. - Critical: Opus Audio CSD Handling: In
ExoPlayerFormatCreator.java, there's an issue with handling Opus audio specific configuration data (CSD). If CSD is present and parsed, it's not correctly passed to ExoPlayer'sFormat.Builder, and error handling for parsing failure usesassert(false)which can crash the app. - High: ExoPlayer Thread Join on Destruction: In
ExoPlayerBridge.java, theexoplayerThread.join()call indestroyExoPlayer()is commented out. This could lead to race conditions or incomplete cleanup during player destruction. - High: Preroll Flag Logic: The
audioPrerolledandvideoPrerolledflags inExoPlayerBridge.javaare updated inwriteSamplebased on a method that always returns true, which seems incorrect. TheonPrerolledcallback should likely be the sole source for updating these flags. - Low: Unused Imports in CobaltMediaSource.java: Several imports in
CobaltMediaSource.javaare unused. This was not commented on due to review settings. - Low: Commented-out Code: Various files contain commented-out code blocks (e.g.,
pendingSamplesinCobaltMediaSource.java,maybeThrowErrorinCobaltSampleStream.java). These should be reviewed and removed if no longer needed. This was not commented on due to review settings. - Low: TODO Comments: Several TODO comments exist across the new files (e.g., dynamic format selection, clearing stream on release, variable playback rate support). These should be tracked. This was not commented on due to review settings.
- Low: Unused Field in MediaCodecBridge.java: The
playerfield added toMediaCodecBridge.javais not used. This was not commented on due to review settings. - Low: Logging Verbosity: The new Java classes include extensive
Log.icalls, which are useful for development but might need to be reviewed for release builds. This was not commented on due to review settings. - Low:
Shipped: yesin README for GMS Ads Identifier: TheREADME.chromiumforcom_google_android_gms_play_services_ads_identifierhadShipped: yesremoved. This might be an oversight. This was not commented on due to review settings.
Merge Readiness
This pull request is currently a draft and introduces a major architectural change by integrating ExoPlayer. While the foundational work is evident, there are critical issues, particularly with seek functionality and Opus CSD handling, that need to be addressed. Additionally, high-severity issues related to thread management on destruction and preroll logic should be resolved.
Given the draft status and these findings, the PR is not ready for merging. I recommend addressing the critical and high-severity issues before further review or consideration for merging. As an AI, I am not authorized to approve pull requests; this feedback is intended to help improve the code quality.
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerFormatCreator.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
Outdated
Show resolved
Hide resolved
osagie98
force-pushed
the
import-exoplayer
branch
3 times, most recently
from
e10c317 to
6974e02
Compare
osagie98
force-pushed
the
import-exoplayer
branch
from
d349bcb to
6ae8cb8
Compare
|
/generate-commit-message |
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
osagie98
changed the title
Also adds a non DRM constructor for the ExoPlayerBridge C++ class.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This PR introduces a new ExoPlayer-based media playback pipeline, which is a significant and foundational change. The implementation is comprehensive, covering player lifecycle, DRM, and data handling through custom MediaSource components. The code is well-structured, with clear separation of concerns between the native C++ bridge and the Java ExoPlayer implementation.
I've identified several critical logic errors in the ExoPlayer listener implementations that would prevent player state updates from being correctly propagated. Additionally, there's a potential performance issue with a blocking call in the DRM initialization path that could lead to slow startup or ANRs. I've also included several medium-severity comments regarding code style, unused imports, and unused parameters to improve maintainability.
Overall, this is a great step towards integrating ExoPlayer. Addressing the identified issues will be crucial for ensuring the stability and correctness of the new playback pipeline.
| public void onPlaybackStateChanged(@Player.State int playbackState) { | ||
| if (mPlayer == null || mIsReleased) { | ||
| switch (playbackState) { | ||
| case Player.STATE_BUFFERING: | ||
| case Player.STATE_IDLE: | ||
| return; | ||
| case Player.STATE_READY: | ||
| ExoPlayerBridgeJni.get().onReady(mNativeExoPlayerBridge); | ||
| break; | ||
| case Player.STATE_ENDED: | ||
| ExoPlayerBridgeJni.get().onEnded(mNativeExoPlayerBridge); | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in onPlaybackStateChanged is inverted. The JNI callbacks should only be triggered when the player is active, but the current if (mPlayer == null || mIsReleased) condition means they are only called when the player is released or null. This is a critical bug that will prevent player state changes from being communicated to the native layer. The condition should be checked at the beginning of the method to guard against calls after release.
| public void onPlaybackStateChanged(@Player.State int playbackState) { | |
| if (mPlayer == null || mIsReleased) { | |
| switch (playbackState) { | |
| case Player.STATE_BUFFERING: | |
| case Player.STATE_IDLE: | |
| return; | |
| case Player.STATE_READY: | |
| ExoPlayerBridgeJni.get().onReady(mNativeExoPlayerBridge); | |
| break; | |
| case Player.STATE_ENDED: | |
| ExoPlayerBridgeJni.get().onEnded(mNativeExoPlayerBridge); | |
| break; | |
| } | |
| } | |
| } | |
| public void onPlaybackStateChanged(@Player.State int playbackState) { | |
| if (mPlayer == null || mIsReleased) { | |
| return; | |
| } | |
| switch (playbackState) { | |
| case Player.STATE_READY: | |
| ExoPlayerBridgeJni.get().onReady(mNativeExoPlayerBridge); | |
| break; | |
| case Player.STATE_ENDED: | |
| ExoPlayerBridgeJni.get().onEnded(mNativeExoPlayerBridge); | |
| break; | |
| default: | |
| // No-op for STATE_IDLE and STATE_BUFFERING. | |
| break; | |
| } | |
| } |
| public void onTracksChanged(Tracks tracks) { | ||
| if (mPlayer == null || mIsReleased) { | ||
| ExoPlayerBridgeJni.get().onInitialized(mNativeExoPlayerBridge); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in onTracksChanged is inverted. onInitialized should only be called when the player is active. The current condition if (mPlayer == null || mIsReleased) means it's only called when the player is released or null. This is a critical bug.
| public void onTracksChanged(Tracks tracks) { | |
| if (mPlayer == null || mIsReleased) { | |
| ExoPlayerBridgeJni.get().onInitialized(mNativeExoPlayerBridge); | |
| } | |
| } | |
| public void onTracksChanged(Tracks tracks) { | |
| if (mPlayer == null || mIsReleased) { | |
| return; | |
| } | |
| ExoPlayerBridgeJni.get().onInitialized(mNativeExoPlayerBridge); | |
| } |
| public synchronized void onIsPlayingChanged(boolean isPlaying) { | ||
| updatePositionAnchor(); | ||
| mIsProgressing = isPlaying; | ||
| if (mPlayer == null || mIsReleased) { | ||
| ExoPlayerBridgeJni.get().onIsPlayingChanged(mNativeExoPlayerBridge, isPlaying); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in onIsPlayingChanged is inverted. The JNI callback should only be triggered when the player is active. The current condition if (mPlayer == null || mIsReleased) means it's only called when the player is released or null. This is a critical bug.
| public synchronized void onIsPlayingChanged(boolean isPlaying) { | |
| updatePositionAnchor(); | |
| mIsProgressing = isPlaying; | |
| if (mPlayer == null || mIsReleased) { | |
| ExoPlayerBridgeJni.get().onIsPlayingChanged(mNativeExoPlayerBridge, isPlaying); | |
| } | |
| } | |
| public synchronized void onIsPlayingChanged(boolean isPlaying) { | |
| updatePositionAnchor(); | |
| mIsProgressing = isPlaying; | |
| if (mPlayer == null || mIsReleased) { | |
| return; | |
| } | |
| ExoPlayerBridgeJni.get().onIsPlayingChanged(mNativeExoPlayerBridge, isPlaying); | |
| } |
| public void onDroppedVideoFrames( | ||
| @NonNull EventTime eventTime, int droppedFrames, long elapsedMs) { | ||
| if (mPlayer == null || mIsReleased) { | ||
| ExoPlayerBridgeJni.get() | ||
| .onDroppedVideoFrames(mNativeExoPlayerBridge, droppedFrames); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic in onDroppedVideoFrames is inverted. The JNI callback should only be triggered when the player is active. The current condition if (mPlayer == null || mIsReleased) means it's only called when the player is released or null. This is a critical bug.
public void onDroppedVideoFrames(
@NonNull EventTime eventTime, int droppedFrames, long elapsedMs) {
if (mPlayer == null || mIsReleased) {
return;
}
ExoPlayerBridgeJni.get()
.onDroppedVideoFrames(mNativeExoPlayerBridge, droppedFrames);
}| std::vector<uint8_t> DrmSystemExoPlayer::GetInitializationData() { | ||
| std::unique_lock<std::mutex> lock(mutex_); | ||
| bool received_init_data = init_data_available_cv_.wait_for( | ||
| lock, std::chrono::microseconds(kWaitForInitializedTimeoutUsec), | ||
| [this] { return init_data_available_; }); | ||
|
|
||
| if (!received_init_data) { | ||
| return std::vector<uint8_t>(); | ||
| } | ||
|
|
||
| return initialization_data_; | ||
| } |
There was a problem hiding this comment.
This method uses wait_for with a 5-second timeout, which can block the player worker thread during initialization. This could lead to slow startup times or even Application Not Responding (ANR) errors if the DRM initialization data is not provided promptly by GenerateSessionUpdateRequest. Consider using a non-blocking approach or ensuring that the data is always available before this method is called to avoid blocking the main player thread.
| import androidx.media3.common.DrmInitData.SchemeData; | ||
| import androidx.media3.common.Format; | ||
| import androidx.media3.common.MimeTypes; | ||
| import androidx.media3.common.util.UnstableApi; |
There was a problem hiding this comment.
The import androidx.media3.common.util.UnstableApi is unused and can be removed.
| import androidx.media3.exoplayer.source.TrackGroupArray; | ||
| import androidx.media3.exoplayer.trackselection.ExoTrackSelection; | ||
| import androidx.media3.exoplayer.upstream.Allocator; | ||
| import androidx.media3.extractor.TrackOutput.CryptoData; |
There was a problem hiding this comment.
The import androidx.media3.extractor.TrackOutput.CryptoData is unused and can be removed.
| import androidx.media3.exoplayer.upstream.Allocator; | ||
| import androidx.media3.extractor.TrackOutput.CryptoData; | ||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; |
There was a problem hiding this comment.
The import java.nio.ByteBuffer is unused and can be removed.
| import androidx.media3.exoplayer.source.SinglePeriodTimeline; | ||
| import androidx.media3.exoplayer.upstream.Allocator; | ||
| import java.io.IOException; | ||
| import java.nio.ByteBuffer; |
There was a problem hiding this comment.
The import java.nio.ByteBuffer is unused and can be removed.
| import static dev.cobalt.media.Log.TAG; | ||
|
|
||
| import android.content.Context; | ||
| import android.content.Context; |
There was a problem hiding this comment.
The import android.content.Context is duplicated and can be removed.
3 participants
Introduce a new media playback pipeline utilizing the Android ExoPlayer
library. This implementation provides an alternative SbPlayer handler
that forwards encoded media data and playback commands to the Java-side
ExoPlayer.
This change is foundational for integrating ExoPlayer, with initial
support for basic playback. Key areas such as tunnel mode seek/pause
and DRM playback are noted as future work. The new pipeline is enabled
via the kEnableExoPlayer feature flag.
Unimplemented features:
Bug: 191693650