Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds video-quality presets and output-dimension tracking to the frame recorder, wraps native library loading in a try-catch with device diagnostic logging, expands an invalid-image-size log to include dimensions, and adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@library/src/main/java/org/wysaid/nativePort/CGEFrameRecorder.java`:
- Around line 93-95: The no-arg-default overload startRecording(int fps, String
filename) now delegates to VideoQuality.HIGH (10Mbps) which changes prior
behavior (native default 1,650,000 bps); either restore the original default
bitrate or explicitly warn downstream callers: update startRecording(int,String)
to delegate to the same bitrate used previously (i.e., call startRecording(fps,
filename, VideoQuality matching 1_650_000 bps) or create a
VideoQuality.CONSERVATIVE/DEFAULT_1650000 and use it), or if you intentionally
keep HIGH, emit a clear runtime warning/log message in
startRecording(int,String) telling callers the default bitrate changed and
recommending use of startRecording(int,String,VideoQuality) to control file
size.
There was a problem hiding this comment.
Pull request overview
This PR updates the Android GPU image/video library to improve debuggability during native library loading and to introduce an adaptive-bitrate, quality-preset API for video recording.
Changes:
- Add device-context logging when native library loading fails (and wrap load sequence in a try/catch).
- Introduce
VideoQualitypresets and adaptive bitrate calculation inCGEFrameRecorder, updating defaultstartRecording(...)behavior. - Improve an error log in native bitmap result generation to include invalid dimensions.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
library/src/main/jni/interface/cgeImageHandlerAndroid.cpp |
Improves invalid-size error logging by including width/height. |
library/src/main/java/org/wysaid/nativePort/NativeLibraryLoader.java |
Wraps System.loadLibrary(...) calls with logging on UnsatisfiedLinkError, including device/ABI info. |
library/src/main/java/org/wysaid/nativePort/CGEFrameRecorder.java |
Adds adaptive bitrate calculation and VideoQuality-based recording overloads; changes default recording bitrate selection. |
.gitignore |
Ignores tasks/ directory. |
| public boolean startRecording(int fps, String filename, VideoQuality quality) { | ||
| // Use default 1080p if dimensions not set | ||
| int width = (mOutputWidth > 0) ? mOutputWidth : 1920; | ||
| int height = (mOutputHeight > 0) ? mOutputHeight : 1080; | ||
|
|
||
| int bitrate = calculateBitrate(width, height, quality); | ||
| return startRecording(fps, bitrate, filename); |
There was a problem hiding this comment.
The fallback output size used for bitrate calculation defaults to 1920x1080 when init() hasn’t populated mOutputWidth/Height. This is inconsistent with CGEFrameRenderer’s documented default recording resolution (640x480) and can unexpectedly select a much higher bitrate (e.g., 10 Mbps) for callers that haven’t initialized or that rely on defaults. Consider defaulting the bitrate calculation to the same default resolution as the native/renderer side (or failing fast if init() hasn’t been called), so the default startRecording() behavior remains predictable.
| public boolean startRecording(int fps, String filename, VideoQuality quality) { | ||
| // Use default 1080p if dimensions not set | ||
| int width = (mOutputWidth > 0) ? mOutputWidth : 1920; | ||
| int height = (mOutputHeight > 0) ? mOutputHeight : 1080; | ||
|
|
||
| int bitrate = calculateBitrate(width, height, quality); | ||
| return startRecording(fps, bitrate, filename); |
There was a problem hiding this comment.
Public methods accept a VideoQuality parameter but don’t handle null; passing null will trigger a NullPointerException in calculateBitrate()’s switch. If this is part of the public API surface, consider validating quality (e.g., defaulting to HIGH) to avoid crashing callers.
| int width = (mOutputWidth > 0) ? mOutputWidth : 1920; | ||
| int height = (mOutputHeight > 0) ? mOutputHeight : 1080; |
There was a problem hiding this comment.
getRecommendedBitrate() also assumes mOutputWidth/Height are set and otherwise falls back to 1920x1080. If the renderer/recorder defaults are lower (e.g., 640x480 per CGEFrameRenderer’s docs), this will overestimate the recommended bitrate unless init() has been called. Consider aligning the fallback with the actual default recording resolution or returning a value derived from native state when available.
| int width = (mOutputWidth > 0) ? mOutputWidth : 1920; | |
| int height = (mOutputHeight > 0) ? mOutputHeight : 1080; | |
| int width = (mOutputWidth > 0) ? mOutputWidth : 640; | |
| int height = (mOutputHeight > 0) ? mOutputHeight : 480; |
Address code review feedback: the no-arg startRecording(int, String) now maintains the original 1.65 Mbps bitrate instead of defaulting to HIGH quality (10 Mbps). This preserves backward compatibility while keeping the new quality-based API available. - Mark legacy method as @deprecated - Add clear JavaDoc guiding users to the quality-based overload - Prevents breaking existing code that relies on the old bitrate Resolves: #561 (review comment)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@library/src/main/java/org/wysaid/nativePort/CGEFrameRecorder.java`:
- Around line 151-155: The getRecommendedBitrate method silently falls back to
1920x1080 when mOutputWidth/mOutputHeight are uninitialized; update
getRecommendedBitrate to detect this fallback and emit a warning via the
existing logger (or System.err) before calling calculateBitrate, or add an
overload getRecommendedBitrate(int width, int height, VideoQuality quality) that
accepts explicit dimensions and delegate to calculateBitrate; reference
getRecommendedBitrate, mOutputWidth, mOutputHeight, and calculateBitrate so the
change is applied where the defaulting occurs and callers can opt into the
explicit-dimensions overload.
- Around line 110-117: The startRecording(int fps, String filename, VideoQuality
quality) method silently falls back to 1920x1080 when mOutputWidth/mOutputHeight
are zero; update startRecording to detect this fallback and log a warning (using
the existing logger or process logger used in this class) indicating that
default 1920x1080 is being assumed and advising the caller to call init() or use
the overload that sets dimensions; reference mOutputWidth, mOutputHeight,
startRecording and calculateBitrate so the change is applied where the default
width/height are chosen and before calculateBitrate is called.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
library/src/main/java/org/wysaid/nativePort/NativeLibraryLoader.java (1)
29-29:Build.SUPPORTED_ABIS[0]could throw if the array is empty.While extremely unlikely on real Android devices, defensive access would be safer in a diagnostic code path.
Proposed defensive access
- Build.MANUFACTURER, Build.MODEL, Build.VERSION.RELEASE, Build.SUPPORTED_ABIS[0] + Build.MANUFACTURER, Build.MODEL, Build.VERSION.RELEASE, + Build.SUPPORTED_ABIS.length > 0 ? Build.SUPPORTED_ABIS[0] : "unknown"
…arnings, fix array access - Deprecate startRecording(fps, filename) with warning about bitrate change (1.65Mbps -> ~10Mbps) - Add warnings when dimensions fallback to 1920x1080 in startRecording() and getRecommendedBitrate() - Add defensive check for Build.SUPPORTED_ABIS array to prevent potential IndexOutOfBoundsException Addresses CodeRabbit review comments on PR #561
There was a problem hiding this comment.
🧹 Nitpick comments (2)
library/src/main/java/org/wysaid/nativePort/CGEFrameRecorder.java (2)
139-142: Consider validating explicitwidth/heightparameters.If a caller passes zero or negative dimensions,
calculateBitratewill silently bucket them into the lowest tier (VGA/2 Mbps base). Since this overload exists specifically for explicit control, a quick guard would prevent silent misconfiguration.Proposed validation
public boolean startRecording(int fps, String filename, int width, int height, VideoQuality quality) { + if (width <= 0 || height <= 0) { + Log.w(Common.LOG_TAG, "startRecording: Invalid dimensions " + width + "x" + height + ", using 1920x1080 fallback."); + width = 1920; + height = 1080; + } int bitrate = calculateBitrate(width, height, quality); return startRecording(fps, bitrate, filename); }
116-124: Duplicated dimension-fallback logic across two methods.The "check
mOutputWidth/mOutputHeight, default to 1080p, log warning" pattern appears in bothstartRecordingandgetRecommendedBitrate. A small private helper (e.g.,getEffectiveDimensions()) could DRY this up, but with only two call sites and slightly different log messages, this is a minor nit.Also applies to: 163-171
- CGEFrameRecorder: Add adaptive bitrate calculation based on resolution - VideoQuality enum (LOW/MEDIUM/HIGH/ULTRA) - calculateBitrate() method for resolution-aware bitrate - New API: startRecording(fps, filename, quality) - Improves 1080p quality from 1.65Mbps to 10Mbps - Maintains backward compatibility with original startRecording(fps, filename) - NativeLibraryLoader: Add error logging with device info - Wrap library loading in try-catch to log device details on failure - Helps debugging on specific devices/Android versions - cgeImageHandlerAndroid.cpp: Improve error message formatting - Log actual image dimensions on validation errors Build verified: all tests passing
- Update Gradle version from 8.11.1 to 8.13 - Update Android Gradle plugin from 8.10.1 to 8.13.2 - Update compileSdkVersion to 35 for Android 15 support with 16KB page sizes
Add adaptive bitrate for video recording, improve error logging, and simplify native library loading.
Summary by CodeRabbit
New Features
Bug Fixes
Chores