android: Integrate ExoPlayer Java components#8094
android: Integrate ExoPlayer Java components#8094osagie98 wants to merge 19 commits intoyoutube:mainfrom
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Java-side implementation for ExoPlayer. The overall structure is sound, using a handler to manage ExoPlayer on the main thread and providing a bridge for native code. However, I've identified several high-severity issues related to thread safety, resource management, and performance that need to be addressed. Specifically, there's a critical issue in ExoPlayerCodecUtil that could lead to a crash, concurrency problems in ExoPlayerBridge, and a performance bottleneck in ExoPlayerSampleStream due to frequent object allocations. I've also included some medium-severity suggestions to improve code robustness and logging practices.
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerCodecUtil.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
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerSampleStream.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerManager.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaSource.java
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerSampleStream.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaSource.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerSampleStream.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaSource.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerSampleStream.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerManager.java
Outdated
Show resolved
Hide resolved
| exoplayerHandler.postDelayed(this::updatePlaybackPos, 150); | ||
| } | ||
|
|
||
| public void destroy() { |
There was a problem hiding this comment.
nit: to make the function name more consistent with other media components, I feel release can be a better name.
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
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
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
Show resolved
Hide resolved
| private static final int MAX_BUFFER_DURATION_MS = 5000; // 5 seconds. | ||
| // Based on the requirement to begin playback if at least 500 milliseconds of media has been | ||
| // appended. | ||
| private static final int MIN_BUFFER_DURATION_MS = 450; |
There was a problem hiding this comment.
The comment says it needs at least 500ms of data, but MIN_BUFFER_DURATION_MS is 450, which is confusing.
There was a problem hiding this comment.
I added a comment to clarify
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
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/ExoPlayerSampleStream.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerManager.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
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/ExoPlayerMediaPeriod.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the Java-side classes for ExoPlayer-based rendering. The changes include adding new classes for managing the ExoPlayer instance, handling media sources, and streaming samples, along with updating the build configuration. The new classes are well-structured and follow common ExoPlayer integration patterns.
My review has identified a few issues:
- A critical logic error in
ExoPlayerMediaSource.canAcceptMoreData()that could lead to aNullPointerExceptionor incorrect buffer management. - A high-severity bug in
ExoPlayerBridge.getCurrentPositionUsec()where the playback rate is not considered for position extrapolation. - A high-severity race condition in several
ExoPlayerBridgemethods that could lead to a crash during player release. - A minor typo in a log message.
I've provided suggestions to fix these issues. Addressing them will improve the robustness and correctness of the implementation.
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaSource.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
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerManager.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerManager.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerBridge.java
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the Java-side implementation for ExoPlayer, including bridges for communication with native code, DRM handling, and media source management. The changes are extensive, adding several new classes to support the ExoPlayer pipeline. The code is generally well-structured, using appropriate Android and ExoPlayer patterns. I've identified a few areas for improvement, mainly concerning exception handling correctness and documentation clarity. Specifically, there's an issue with exception types in the DRM bridge and several instances of incorrect Javadoc comments that should be addressed to improve maintainability.
| return mPendingProvisionRequestResponse.get( | ||
| PROVISION_REQUEST_TIMEOUT_MS, TimeUnit.MILLISECONDS); | ||
| } catch (Exception e) { | ||
| throw new RuntimeException(e); |
There was a problem hiding this comment.
The executeProvisionRequest method is declared to throw MediaDrmCallbackException, but it currently catches a generic Exception and wraps it in a RuntimeException. This violates the method's contract. You should wrap the caught exception in a MediaDrmCallbackException to conform to the interface.
| throw new RuntimeException(e); | |
| throw new MediaDrmCallbackException(e); |
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerDrmBridge.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaPeriod.java
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerMediaSource.java
Outdated
Show resolved
Hide resolved
cobalt/android/apk/app/src/main/java/dev/cobalt/media/ExoPlayerSampleStream.java
Show resolved
Hide resolved
|
/generate-commit-message |
🤖 Gemini Suggested Commit Message💡 Pro Tips for a Better Commit Message:
|
android: Integrate ExoPlayer Java components
This change introduces the core Java classes for the ExoPlayer
rendering implementation on Android. These classes manage the ExoPlayer
instance, facilitate communication between native and Java code,
handle Digital Rights Management (DRM) sessions, and provide the
mechanism for streaming media samples to ExoPlayer.
This is a foundational step to leverage ExoPlayer for media playback,
aiming to improve media capabilities, performance, and compatibility
on Android devices.
Bug: 191693650