feat: Implement CameraX support with runtime camera API selection#565
Merged
feat: Implement CameraX support with runtime camera API selection#565
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 a pluggable camera abstraction (ICameraProvider) with Camera1 and CameraX implementations and factory; migrates view and demo code to provider-based preview/capture/flash/focus flows; adds scoped-storage media saving, API-aware permissions, CameraX deps, UI selector, and adb target resolution in scripts. Changes
Sequence DiagramsequenceDiagram
participant Activity as Activity
participant Factory as CameraProviderFactory
participant Provider as ICameraProvider
participant Surface as CameraGLSurfaceView
participant Backend as Camera Backend
Activity->>Factory: create(CameraAPI, Context)
Factory->>Provider: instantiate Camera1Provider / CameraXProvider
Activity->>Surface: setCameraProvider(provider)
Activity->>Provider: openCamera(facing, callback)
Provider->>Backend: initialize backend (Camera1 or CameraX)
Backend-->>Provider: onCameraOpened
Provider-->>Activity: callback.onCameraOpened()
Activity->>Surface: startPreview(SurfaceTexture)
Surface->>Provider: startPreview(texture)
Provider->>Backend: bind preview & start streaming
Backend-->>Provider: onPreviewStarted
Activity->>Provider: focusAtPoint(x,y,radius,callback)
Provider->>Backend: perform autofocus
Backend-->>Provider: autofocus complete
Provider-->>Activity: callback.onAutoFocus()
Activity->>Provider: takePicture(shutter, pictureCallback)
Provider->>Backend: capture image
Backend-->>Provider: JPEG bytes (+rotation)
Provider->>Provider: convert JPEG→Bitmap, apply rotation/mirror
Provider-->>Activity: pictureCallback.onPictureData(bitmap, rotation)
Activity->>Provider: closeCamera()
Provider->>Backend: release/unbind resources
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
This was referenced Feb 21, 2026
Closed
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java (2)
532-538:⚠️ Potential issue | 🟡 MinorSame NPE risk as
FaceTrackingDemoActivity:getCameraProvider()may return null.🛡️ Proposed defensive check
public void onPause() { super.onPause(); - mCameraView.getCameraProvider().closeCamera(); + ICameraProvider provider = mCameraView.getCameraProvider(); + if (provider != null) { + provider.closeCamera(); + } Log.i(LOG_TAG, "activity onPause..."); mCameraView.release(null); mCameraView.onPause(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java` around lines 532 - 538, In CameraDemoActivity.onPause, guard the call to mCameraView.getCameraProvider().closeCamera() against a null CameraProvider to avoid the same NPE as in FaceTrackingDemoActivity: retrieve the provider into a local (e.g., provider = mCameraView.getCameraProvider()), check provider != null before calling provider.closeCamera(), and then proceed to call mCameraView.release(null) and mCameraView.onPause() as before so release/cleanup still runs even if the provider was null.
114-134:⚠️ Potential issue | 🔴 CriticalMissing null check:
createVideoOutputPath()result is passed directly tostartRecording.If
createVideoOutputPath()returnsnull(e.g., MediaStore insert failed, permission issue),recordFilenamewill benullandmCameraView.startRecording(null, ...)will likely crash in native code or produce undefined behavior.🐛 Proposed fix: guard against null output path
if (!mCameraView.isRecording()) { btn.setText("Recording"); Log.i(LOG_TAG, "Start recording..."); recordFilename = createVideoOutputPath(); + if (recordFilename == null) { + showText("Failed to create output path!"); + isValid = true; + return; + } mCameraView.startRecording(recordFilename, new CameraRecordGLSurfaceView.StartRecordingCallback() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java` around lines 114 - 134, The code passes the result of createVideoOutputPath() directly to mCameraView.startRecording, which can be null; change the flow in the button handling so you first assign recordFilename = createVideoOutputPath(), then check if recordFilename == null and handle the error (e.g., showText("Failed to create output path"), Log.e(LOG_TAG, ...), set isValid = false and do not call mCameraView.startRecording). Only call mCameraView.startRecording(recordFilename, new CameraRecordGLSurfaceView.StartRecordingCallback() { ... }) when recordFilename is non-null so startRecording never receives null.cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java (1)
229-232:⚠️ Potential issue | 🟠 MajorPre-existing bug: string comparison uses
==instead of.equals().Line 229 compares
mDemo.activityNamewith a string literal using==. SinceactivityNameis set fromnew DemoClassDescription("FaceTrackingDemoActivity", ...), this comparison will always befalsedue to reference inequality, so the guard never fires. This is a pre-existing issue but should be fixed while you're touching this area.🐛 Proposed fix
- if (mDemo.activityName == "FaceTrackingDemoActivity") { + if ("FaceTrackingDemoActivity".equals(mDemo.activityName)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java` around lines 229 - 232, The comparison in MainActivity using mDemo.activityName == "FaceTrackingDemoActivity" is incorrect for String content and should be changed to use String.equals; replace the identity check with a content check (e.g., "FaceTrackingDemoActivity".equals(mDemo.activityName) or mDemo.activityName != null && mDemo.activityName.equals("FaceTrackingDemoActivity")) so the guard before calling MsgUtil.toastMsg(...) and returning will correctly trigger for that demo.library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java (1)
253-259:⚠️ Potential issue | 🔴 Critical
onSwitchCameraunconditionally applies Camera1-specific rotation, breaking CameraX after camera switch.After switching cameras,
setSrcRotation(Math.PI / 2.0)is always applied regardless of the provider. For CameraX (whereneedsManualRotation()returnsfalse), this overrides the correct0.0frotation set duringonSurfaceCreated, causing the preview to appear rotated 90° after a camera switch.🐛 Proposed fix: respect provider's rotation needs
`@Override` protected void onSwitchCamera() { super.onSwitchCamera(); if(mFrameRecorder != null) { - mFrameRecorder.setSrcRotation((float) (Math.PI / 2.0)); + if (getCameraProvider().needsManualRotation()) { + mFrameRecorder.setSrcRotation((float) (Math.PI / 2.0)); + } else { + mFrameRecorder.setSrcRotation(0.0f); + } mFrameRecorder.setRenderFlipScale(1.0f, -1.0f); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java` around lines 253 - 259, onSwitchCamera currently forces Camera1-specific rotation by calling mFrameRecorder.setSrcRotation(Math.PI/2) unconditionally; change it to respect the provider's rotation requirements by checking needsManualRotation() (or equivalent provider flag) before applying the 90° rotation so CameraX keeps its 0.0f rotation set in onSurfaceCreated. In practice, update CameraGLSurfaceViewWithTexture.onSwitchCamera to only call mFrameRecorder.setSrcRotation((float)(Math.PI/2.0)) and setRenderFlipScale when needsManualRotation() returns true (otherwise leave source rotation as-is or reset to 0.0f), using the existing mFrameRecorder reference so the preview orientation remains correct after a camera switch.
🧹 Nitpick comments (7)
tasks.sh (1)
129-130: Redundant device check afterresolveAdbTarget.Line 129 calls
resolveAdbTarget(without failing on error, unlikerunAndroidApp), then Line 130 has a compound condition that re-invokesadb devices. WhenresolveAdbTargetreturns 1 (no devices),ADB_TARGET_ARGSis empty and the fallbackadb devicescheck will also find nothing — so the behavior is correct, but the second half of the condition is redundant in most cases.This is minor and doesn't affect correctness, but a simpler approach would be to track the return value:
♻️ Suggested simplification
- resolveAdbTarget - if [[ -n "$GRADLEW_RUN_TASK" ]] && [[ ${`#ADB_TARGET_ARGS`[@]} -gt 0 || $(. "$ADB_COMMAND" devices | grep -v 'List' | grep -vE '^$' | grep $'\tdevice$' | wc -l | tr -d ' ') -ne 0 ]]; then + if [[ -n "$GRADLEW_RUN_TASK" ]] && resolveAdbTarget; then🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tasks.sh` around lines 129 - 130, The compound condition after calling resolveAdbTarget redundantly rechecks connected devices via adb when resolveAdbTarget already determines availability; modify the logic to capture resolveAdbTarget's return status (e.g., check its exit code or set a flag) and use that to decide the if-condition instead of re-running the adb devices pipeline—update the block that references ADB_TARGET_ARGS and ADB_COMMAND to rely on the resolveAdbTarget result (or a boolean like adb_target_resolved) so you can remove the second adb devices check while preserving the existing behavior used by runAndroidApp.library/src/main/java/org/wysaid/camera/Camera1Provider.java (1)
178-184:resumePreviewAfterCapturebypassesCameraInstancestate management.Calling
cameraDevice.startPreview()directly skipsCameraInstance's internal preview state tracking (itsisPreviewing()flag, etc.). This could leaveCameraInstance's state inconsistent — e.g.,isPreviewing()returnsfalsewhile the preview is actually running.Consider delegating through
CameraInstanceto maintain consistent state:♻️ Suggested change
`@Override` public void resumePreviewAfterCapture() { - Camera cameraDevice = CameraInstance.getInstance().getCameraDevice(); - if (cameraDevice != null) { - cameraDevice.startPreview(); + CameraInstance instance = CameraInstance.getInstance(); + if (instance.isCameraOpened()) { + // Re-use the existing preview surface to restart preview + Camera cameraDevice = instance.getCameraDevice(); + if (cameraDevice != null) { + cameraDevice.startPreview(); + } } }Or better yet, if
CameraInstancehas a method to restart preview that also updates its internal state, use that instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/camera/Camera1Provider.java` around lines 178 - 184, resumePreviewAfterCapture currently calls CameraInstance.getInstance().getCameraDevice().startPreview() directly which bypasses CameraInstance's internal preview state tracking; change this to delegate preview restarting through CameraInstance (e.g., call a CameraInstance method such as startPreview() or a restartPreview()/resumePreview() helper) so that CameraInstance's internal flags (isPreviewing(), etc.) are updated consistently rather than manipulating the Camera object directly.library/src/main/java/org/wysaid/myUtils/PermissionUtil.java (1)
58-79: Consider requesting only the missing permissions.Line 73 passes the full
permissionsarray torequestPermissions, including permissions that are already granted. WhileActivityCompat.requestPermissionshandles already-granted permissions gracefully, passing only the missing subset is cleaner and avoids unnecessary system dialog flicker on some devices.♻️ Suggested change
if (toastText != null) { Toast.makeText(activity, toastText.toString(), Toast.LENGTH_LONG).show(); - ActivityCompat.requestPermissions(activity, permissions, REQUEST_PERMISSION); + // Build array of only the missing permissions + java.util.List<String> missing = new java.util.ArrayList<>(); + for (String p : permissions) { + if (ActivityCompat.checkSelfPermission(activity, p) != PackageManager.PERMISSION_GRANTED) { + missing.add(p); + } + } + ActivityCompat.requestPermissions(activity, + missing.toArray(new String[0]), REQUEST_PERMISSION); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/myUtils/PermissionUtil.java` around lines 58 - 79, In verifyStoragePermissions, only the missing permissions from getRequiredPermissions() should be requested and shown in the toast: iterate permissions, collect those with ActivityCompat.checkSelfPermission(...) != PackageManager.PERMISSION_GRANTED into a List/array (use the existing toastText logic to build the message from that collection), and then call ActivityCompat.requestPermissions(activity, missingPermissionsArray, REQUEST_PERMISSION) instead of passing the full permissions array; keep the try/catch and logging as-is and ensure you handle the case where no permissions are missing (no toast/request).library/src/main/java/org/wysaid/camera/ICameraProvider.java (1)
73-82:openCamerareturnsbooleansynchronously, but CameraX binding is async.The
openCameramethod returnsbooleanfor success/failure, which works for Camera1's synchronous open. However, CameraX binds the camera asynchronously viaProcessCameraProvider, so the return value may not accurately reflect the final outcome. TheCameraOpenCallbackhandles the async case, but callers relying on the boolean return may get misleading results from a CameraX backend.Consider documenting that the return value for async backends indicates whether the request was submitted successfully, not whether the camera is actually open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/camera/ICameraProvider.java` around lines 73 - 82, The openCamera(boolean) return value is misleading for async backends like CameraX (which binds via ProcessCameraProvider); update the JavaDoc for the interface method openCamera(CameraFacing, CameraOpenCallback) and the default openCamera(CameraOpenCallback) to state that the boolean only indicates whether the open request was submitted/accepted (true) or immediately failed (false), not whether the camera is fully opened, and ensure implementations of openCamera (especially the CameraX backend) return true when the bind request is initiated and rely on CameraOpenCallback for final success/failure.library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java (1)
433-452: Inefficient: writes entire JPEG to disk just to read EXIF orientation.
shouldRotateByExifwrites the full JPEG byte array to a temp file, then reads it back withExifInterface. Since API 24,ExifInterfaceaccepts anInputStream, which avoids the disk round-trip entirely.♻️ Suggested: use ExifInterface(InputStream)
private Boolean shouldRotateByExif(byte[] jpegData) { - String cacheDir = getContext().getExternalCacheDir() != null ? getContext().getExternalCacheDir().getAbsolutePath() : null; - if (cacheDir == null) { - return null; - } - - String tmpFilename = cacheDir + "/picture_cache000.jpg"; - try (FileOutputStream fileout = new FileOutputStream(tmpFilename); - BufferedOutputStream bufferOutStream = new BufferedOutputStream(fileout)) { - bufferOutStream.write(jpegData); - bufferOutStream.flush(); - - ExifInterface exifInterface = new ExifInterface(tmpFilename); + try { + java.io.ByteArrayInputStream bis = new java.io.ByteArrayInputStream(jpegData); + ExifInterface exifInterface = new ExifInterface(bis); int orientation = exifInterface.getAttributeInt(ExifInterface.TAG_ORIENTATION, ExifInterface.ORIENTATION_NORMAL); return orientation == ExifInterface.ORIENTATION_ROTATE_90; - } catch (IOException e) { + } catch (Exception e) { Log.e(LOG_TAG, "Err when reading exif from cache image: " + e.toString()); return null; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java` around lines 433 - 452, The shouldRotateByExif method currently writes jpegData to a temp file then reads it back; instead, replace the file round-trip by creating a ByteArrayInputStream from jpegData and passing that InputStream into the ExifInterface(InputStream) constructor inside a try-with-resources block, read TAG_ORIENTATION as before and return the same boolean; remove the FileOutputStream/BufferedOutputStream and temp filename logic, keep the null check for getExternalCacheDir if needed, and ensure IOException handling/logging remains around the ExifInterface/InputStream usage.cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java (1)
73-221: Significant code duplication withCameraDemoActivity.
needsScopedStorageWrite(),createVideoOutputPath(), andpublishVideoToGallery()/publishRecordedVideo()are essentially the same logic duplicated between this file andCameraDemoActivity.java. Consider extracting a sharedMediaStoreHelperutility class (or adding these toFileUtil) to avoid maintaining parallel implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java` around lines 73 - 221, Duplicate MediaStore/file helper logic exists in TestCaseActivity (needsScopedStorageWrite, createVideoOutputPath, publishVideoToGallery) and CameraDemoActivity (publishRecordedVideo); extract these into a single shared utility (e.g., MediaStoreHelper or add to FileUtil) and have both activities call it. Move needsScopedStorageWrite(), createVideoOutputPath(String, Uri[], ParcelFileDescriptor[]), publishVideoToGallery(Uri, ParcelFileDescriptor, String) (and publishRecordedVideo equivalent) into the helper, preserve existing method signatures and behavior for API 29+ vs legacy paths, update both activities to delegate to the new helper methods, and delete the duplicated implementations from each activity. Ensure the helper has access to a Context (Context parameter or constructor) so it can call getContentResolver(), MediaScannerConnection, and logging.library/src/main/java/org/wysaid/view/CameraGLSurfaceView.java (1)
83-122:setFlashModeshould besynchronizedfor consistency withsetFlashLightMode.
setFlashLightModecarriessynchronized(guarding against concurrent flash operations on the same view object). The newsetFlashModethat replaces it does not, creating a subtle inconsistency. WhileCamera1Provider.setFlashModeusesCameraInstance's per-method synchronization at a lower level, the view-level guard onmIsCameraBackForwardis still a non-atomic read–check–act under this inconsistency.♻️ Suggested fix
- public boolean setFlashMode(ICameraProvider.FlashMode mode) { + public synchronized boolean setFlashMode(ICameraProvider.FlashMode mode) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceView.java` around lines 83 - 122, The method setFlashMode(ICameraProvider.FlashMode) is not synchronized while the deprecated setFlashLightMode(String) is, creating a race on the view-level check of mIsCameraBackForward; make setFlashMode synchronized (add the synchronized modifier to its declaration) so it uses the same view-level lock as setFlashLightMode, preserving atomicity of the read-check-act sequence before delegating to getCameraProvider().setFlashMode(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java`:
- Around line 380-447: The ParcelFileDescriptor mRecordingPfd opened in
createVideoOutputPath can leak if the Activity is destroyed before
publishRecordedVideo runs; add cleanup in the Activity's onDestroy to check if
mRecordingPfd is non-null, close it inside a try/catch (catch
IOException/Exception), set mRecordingPfd to null, and clear any related state
(e.g., leave mRecordingMediaUri/mRecordingPublicPath as appropriate or delete
the pending MediaStore entry) to ensure no FD leaks; reference
createVideoOutputPath, mRecordingPfd, mRecordingMediaUri, publishRecordedVideo,
and onDestroy when applying the fix.
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/FaceTrackingDemoActivity.java`:
- Around line 64-70: In onPause, guard against a null CameraProvider by checking
mCameraView and mCameraView.getCameraProvider() before calling closeCamera();
update FaceTrackingDemoActivity.onPause to only call
mCameraView.getCameraProvider().closeCamera() when getCameraProvider() != null
(and ensure mCameraView itself is non-null) so release(null) and
mCameraView.onPause() still run safely if the provider is absent.
- Around line 26-39: The FaceTrackingDemoActivity camera provider initialization
(the block using EXTRA_CAMERA_API, CAMERA_API_CAMERA1, CameraProviderFactory,
ICameraProvider and mCameraView) is dead because MainActivity currently prevents
launching this activity; either re-enable FaceTrackingDemoActivity in
MainActivity (remove or adjust the early return that checks the activity name
and the "Error: Please checkout the branch 'face_features'..." logic) and ensure
MainActivity passes EXTRA_CAMERA_API when starting activities so the existing
setup runs, or remove the unreachable camera provider setup code from
FaceTrackingDemoActivity (delete the block that reads EXTRA_CAMERA_API and
creates/attaches the CameraProvider and calling mCameraView.setCameraProvider).
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java`:
- Around line 244-258: MainActivity currently only attaches EXTRA_CAMERA_API to
intents when mDemo.activityName equals "CameraDemoActivity", so
FaceTrackingDemoActivity never receives the camera API selection; update the
intent-building logic in MainActivity (where getSharedPreferences(PREFS_NAME,
MODE_PRIVATE) and intent.putExtra(EXTRA_CAMERA_API, api) are used) to also
include EXTRA_CAMERA_API for FaceTrackingDemoActivity (or all camera-using
activities) by expanding the condition to check for "FaceTrackingDemoActivity"
in addition to "CameraDemoActivity" before reading PREFS_KEY_CAMERA_API/
CAMERA_API_CAMERA1 and calling intent.putExtra.
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java`:
- Around line 96-145: createVideoOutputPath currently assigns a
ParcelFileDescriptor into outPfd[0] and returns a proc path but can leak that
PFD if the caller (e.g., generateVideoWithFilter) throws or the flow aborts
before publishVideoToGallery runs; ensure the PFD is always closed on error and
when no longer needed: either (A) modify createVideoOutputPath to not expose
outPfd until the caller explicitly accepts it or to auto-close the PFD on any
exceptional exit (close pfd in the catch and on any early-return paths), and (B)
wrap the entire recording/generation flow that calls createVideoOutputPath and
generateVideoWithFilter in a try-finally that checks outPfd[0] and calls
ParcelFileDescriptor.close() (and nulls out outPfd[0]) in the finally block;
reference createVideoOutputPath, outPfd, publishVideoToGallery, and
generateVideoWithFilter when making the changes.
In `@cgeDemo/src/main/res/values-v35/styles.xml`:
- Around line 4-7: Update the comment in styles.xml to note that
R.attr#windowOptOutEdgeToEdgeEnforcement is deprecated and ineffective when
targeting Android 16 (API 36) so the app cannot opt out of edge-to-edge on
future targets; add a clear TODO referencing windowOptOutEdgeToEdgeEnforcement
and outline next steps (e.g., implement window insets handling and migrate
ActionBar layout) so the necessary migration work is tracked for when
compileSdkVersion/targetSdkVersion moves to 36.
- Around line 8-10: The v35 styles override replaces the entire AppTheme;
instead introduce an AppTheme.Base that contains all shared attributes (e.g.,
colorPrimary, colorAccent, actionBar and font overrides) and make both variants
derive from it: update your base values/styles.xml to declare <style
name="AppTheme.Base"> with the existing AppTheme attributes, change the base
AppTheme to inherit from `@style/AppTheme.Base`, and modify values-v35/styles.xml
so its AppTheme uses parent="@style/AppTheme.Base" and only contains the
v35-specific <item name="android:windowOptOutEdgeToEdgeEnforcement">true</item>;
this preserves all original customisations while applying the API‑35 specific
item.
In `@library/src/main/java/org/wysaid/camera/Camera1Provider.java`:
- Around line 155-175: The hardcoded 90° rotation in Camera1Provider
(params.setRotation(90) and the 90 passed to pictureCallback in
cameraDevice.takePicture) is wrong for front-facing and landscape captures;
replace it by computing the JPEG rotation using Camera.CameraInfo
(Camera.getCameraInfo(cameraId, info)), the camera's info.orientation and
info.facing, and the device display rotation
(Activity.getWindowManager().getDefaultDisplay().getRotation()) following the
Camera1 formula (accounting for front-facing mirror adjustment), then call
params.setRotation(computedRotation) and pass computedRotation to
pictureCallback.onPictureTaken instead of the fixed 90; ensure you locate
cameraId via the same logic used by CameraInstance/getFacing to pick the correct
CameraInfo.
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 349-371: The ImageCapture callback passed to
mImageCapture.takePicture currently uses ContextCompat.getMainExecutor(mContext)
so imageProxyToJpeg runs on the main thread; move the JPEG conversion off the UI
by invoking takePicture with a background executor (e.g., a single-thread
ExecutorService or existing camera/background executor) and perform
imageProxyToJpeg(image) and pictureCallback.onPictureTaken(...) on that
background thread, ensuring you still call image.close() after conversion and
post any UI updates back to the main executor if needed; update the
ImageCapture.OnImageCapturedCallback registration and ensure proper shutdown or
reuse of the executor.
- Around line 496-552: imageProxyToYuvImage produces wrong NV21 when plane
rowStride != width; change the copy logic to read per-row bytes using each
plane's rowStride and pixelStride instead of lump-copying buffers. For the Y
plane (planes[0]) iterate height rows and copy exactly width bytes per row from
planes[0].getBuffer() using planes[0].getRowStride(); for chroma
(planes[1]/planes[2]) use chromaRowStride = planes[1].getRowStride() and
chromaPixelStride = planes[1].getPixelStride() and build the interleaved VU
output by iterating height/2 rows and width/2 columns, reading U/V bytes via
(row*chromaRowStride + col*chromaPixelStride) from planes[1]/planes[2] and
writing V then U into the NV21 uv area so the output Y size is width*height and
UV size is width*height/2 as expected by YuvImage.
- Around line 392-403: Replace the deprecated
Preview.Builder.setTargetResolution usage by configuring the Preview.Builder
with setResolutionSelector using a ResolutionSelector built with a
ResolutionStrategy that matches the desired size (use
ResolutionSelector.Builder().setResolutionStrategy(...) and a ResolutionStrategy
that prefers the Size(mPreferredPreviewWidth, mPreferredPreviewHeight)); remove
setTargetResolution. For Camera2 interop, add the opt-in annotation
`@OptIn`(markerClass = ExperimentalCamera2Interop.class) on the enclosing method
or class that constructs the Camera2Interop.Extender<Preview> (the code
referencing Camera2Interop.Extender and setCaptureRequestOption) so the
experimental API is allowed. Ensure you keep the existing
CaptureRequest.CONTROL_AE_TARGET_FPS_RANGE Range<>(30,60) option passed to
extender.setCaptureRequestOption after switching to setResolutionSelector.
- Around line 253-259: onSwitchCamera() currently forces
mFrameRecorder.setSrcRotation(Math.PI/2) which is Camera1-specific; update
onSwitchCamera() to call needsManualRotation() first and only call
mFrameRecorder.setSrcRotation((float)(Math.PI/2.0)) when needsManualRotation()
returns true, while still calling mFrameRecorder.setRenderFlipScale(1.0f, -1.0f)
unconditionally if mFrameRecorder != null; locate the change in the
onSwitchCamera() method and mirror the same needsManualRotation() check used in
onSurfaceCreated().
In `@library/src/main/java/org/wysaid/myUtils/FileUtil.java`:
- Around line 62-99: Currently a previously-resolved inferior path can remain
cached and prevent re-resolution after FileUtil.init(context) is called; update
the init(...) method in FileUtil to invalidate the cached storagePath by setting
storagePath = null when a non-null Context is provided (or always clear it),
then optionally call getPath(context) to eagerly re-resolve and cache the
preferred app-specific path; refer to the getPath(Context) and init(...) symbols
to locate and change the initializer logic.
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceView.java`:
- Around line 238-241: The deprecated CameraGLSurfaceView.focusAtPoint currently
forwards a lambda that passes null as the Camera to Camera.AutoFocusCallback;
change the forwarding so the callback receives the actual Camera instance from
the provider (i.e., in focusAtPoint wrap the callback as success ->
focusCallback.onAutoFocus(success, <current Camera instance from
getCameraProvider()>)) so callers that dereference the Camera won't NPE, and if
the provider cannot expose a Camera instance reliably, update the deprecated
method's Javadoc on focusAtPoint to explicitly state that the legacy Camera
argument may be null and direct callers to use the new
ICameraProvider.AutoFocusCallback variant (and show how to resume preview with
resumePreviewAfterCapture()).
- Around line 54-74: Mark the field mCameraProvider as volatile to ensure
cross-thread visibility between setCameraProvider (main thread) and
getCameraProvider (GL thread); additionally change getCameraProvider to use a
simple synchronized block or double-checked locking around the lazy-init so only
one Camera1Provider is constructed when mCameraProvider is null (referencing
mCameraProvider, setCameraProvider, getCameraProvider, and Camera1Provider).
- Around line 62-64: setCameraProvider currently just assigns mCameraProvider
and doesn't forward an already-configured preview size; update
setCameraProvider(ICameraProvider provider) to, after setting mCameraProvider,
call the provider's preview-size setter with the view-held values (e.g.,
provider.setPreferredPreviewSize(mRecordWidth, mRecordHeight) or the equivalent
method defined on ICameraProvider) so that a prior presetRecordingSize() is
applied to custom providers (matching how Camera1Provider gets the size).
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java`:
- Around line 323-402: The takePicture callback passed to provider.takePicture
is doing heavy work (BitmapFactory.decodeByteArray, Bitmap.createScaledBitmap,
Canvas/Matrix drawing, CGENativeLibrary.filterImage_MultipleEffectsWriteBack) on
the provider's callback (main) thread; move all bitmap decoding,
scaling/rotation (logic in shouldRotateForCapture, the Canvas/Matrix branches,
and filterImage_MultipleEffectsWriteBack) onto a background thread or
ExecutorService and only post the final result back to the main thread to call
photoCallback.takePictureOK(bmp2) and provider.resumePreviewAfterCapture();
ensure any bmp.recycle() still runs on the background thread and guard lifecycle
(check provider/state) before posting results.
---
Outside diff comments:
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java`:
- Around line 532-538: In CameraDemoActivity.onPause, guard the call to
mCameraView.getCameraProvider().closeCamera() against a null CameraProvider to
avoid the same NPE as in FaceTrackingDemoActivity: retrieve the provider into a
local (e.g., provider = mCameraView.getCameraProvider()), check provider != null
before calling provider.closeCamera(), and then proceed to call
mCameraView.release(null) and mCameraView.onPause() as before so release/cleanup
still runs even if the provider was null.
- Around line 114-134: The code passes the result of createVideoOutputPath()
directly to mCameraView.startRecording, which can be null; change the flow in
the button handling so you first assign recordFilename =
createVideoOutputPath(), then check if recordFilename == null and handle the
error (e.g., showText("Failed to create output path"), Log.e(LOG_TAG, ...), set
isValid = false and do not call mCameraView.startRecording). Only call
mCameraView.startRecording(recordFilename, new
CameraRecordGLSurfaceView.StartRecordingCallback() { ... }) when recordFilename
is non-null so startRecording never receives null.
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java`:
- Around line 229-232: The comparison in MainActivity using mDemo.activityName
== "FaceTrackingDemoActivity" is incorrect for String content and should be
changed to use String.equals; replace the identity check with a content check
(e.g., "FaceTrackingDemoActivity".equals(mDemo.activityName) or
mDemo.activityName != null &&
mDemo.activityName.equals("FaceTrackingDemoActivity")) so the guard before
calling MsgUtil.toastMsg(...) and returning will correctly trigger for that
demo.
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java`:
- Around line 253-259: onSwitchCamera currently forces Camera1-specific rotation
by calling mFrameRecorder.setSrcRotation(Math.PI/2) unconditionally; change it
to respect the provider's rotation requirements by checking
needsManualRotation() (or equivalent provider flag) before applying the 90°
rotation so CameraX keeps its 0.0f rotation set in onSurfaceCreated. In
practice, update CameraGLSurfaceViewWithTexture.onSwitchCamera to only call
mFrameRecorder.setSrcRotation((float)(Math.PI/2.0)) and setRenderFlipScale when
needsManualRotation() returns true (otherwise leave source rotation as-is or
reset to 0.0f), using the existing mFrameRecorder reference so the preview
orientation remains correct after a camera switch.
---
Nitpick comments:
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java`:
- Around line 73-221: Duplicate MediaStore/file helper logic exists in
TestCaseActivity (needsScopedStorageWrite, createVideoOutputPath,
publishVideoToGallery) and CameraDemoActivity (publishRecordedVideo); extract
these into a single shared utility (e.g., MediaStoreHelper or add to FileUtil)
and have both activities call it. Move needsScopedStorageWrite(),
createVideoOutputPath(String, Uri[], ParcelFileDescriptor[]),
publishVideoToGallery(Uri, ParcelFileDescriptor, String) (and
publishRecordedVideo equivalent) into the helper, preserve existing method
signatures and behavior for API 29+ vs legacy paths, update both activities to
delegate to the new helper methods, and delete the duplicated implementations
from each activity. Ensure the helper has access to a Context (Context parameter
or constructor) so it can call getContentResolver(), MediaScannerConnection, and
logging.
In `@library/src/main/java/org/wysaid/camera/Camera1Provider.java`:
- Around line 178-184: resumePreviewAfterCapture currently calls
CameraInstance.getInstance().getCameraDevice().startPreview() directly which
bypasses CameraInstance's internal preview state tracking; change this to
delegate preview restarting through CameraInstance (e.g., call a CameraInstance
method such as startPreview() or a restartPreview()/resumePreview() helper) so
that CameraInstance's internal flags (isPreviewing(), etc.) are updated
consistently rather than manipulating the Camera object directly.
In `@library/src/main/java/org/wysaid/camera/ICameraProvider.java`:
- Around line 73-82: The openCamera(boolean) return value is misleading for
async backends like CameraX (which binds via ProcessCameraProvider); update the
JavaDoc for the interface method openCamera(CameraFacing, CameraOpenCallback)
and the default openCamera(CameraOpenCallback) to state that the boolean only
indicates whether the open request was submitted/accepted (true) or immediately
failed (false), not whether the camera is fully opened, and ensure
implementations of openCamera (especially the CameraX backend) return true when
the bind request is initiated and rely on CameraOpenCallback for final
success/failure.
In `@library/src/main/java/org/wysaid/myUtils/PermissionUtil.java`:
- Around line 58-79: In verifyStoragePermissions, only the missing permissions
from getRequiredPermissions() should be requested and shown in the toast:
iterate permissions, collect those with ActivityCompat.checkSelfPermission(...)
!= PackageManager.PERMISSION_GRANTED into a List/array (use the existing
toastText logic to build the message from that collection), and then call
ActivityCompat.requestPermissions(activity, missingPermissionsArray,
REQUEST_PERMISSION) instead of passing the full permissions array; keep the
try/catch and logging as-is and ensure you handle the case where no permissions
are missing (no toast/request).
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceView.java`:
- Around line 83-122: The method setFlashMode(ICameraProvider.FlashMode) is not
synchronized while the deprecated setFlashLightMode(String) is, creating a race
on the view-level check of mIsCameraBackForward; make setFlashMode synchronized
(add the synchronized modifier to its declaration) so it uses the same
view-level lock as setFlashLightMode, preserving atomicity of the read-check-act
sequence before delegating to getCameraProvider().setFlashMode(...).
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java`:
- Around line 433-452: The shouldRotateByExif method currently writes jpegData
to a temp file then reads it back; instead, replace the file round-trip by
creating a ByteArrayInputStream from jpegData and passing that InputStream into
the ExifInterface(InputStream) constructor inside a try-with-resources block,
read TAG_ORIENTATION as before and return the same boolean; remove the
FileOutputStream/BufferedOutputStream and temp filename logic, keep the null
check for getExternalCacheDir if needed, and ensure IOException handling/logging
remains around the ExifInterface/InputStream usage.
In `@tasks.sh`:
- Around line 129-130: The compound condition after calling resolveAdbTarget
redundantly rechecks connected devices via adb when resolveAdbTarget already
determines availability; modify the logic to capture resolveAdbTarget's return
status (e.g., check its exit code or set a flag) and use that to decide the
if-condition instead of re-running the adb devices pipeline—update the block
that references ADB_TARGET_ARGS and ADB_COMMAND to rely on the resolveAdbTarget
result (or a boolean like adb_target_resolved) so you can remove the second adb
devices check while preserving the existing behavior used by runAndroidApp.
Comment on lines
+96
to
+145
| private String createVideoOutputPath(String displayName, | ||
| Uri[] outUri, | ||
| ParcelFileDescriptor[] outPfd) { | ||
| if (needsScopedStorageWrite()) { | ||
| ContentValues values = new ContentValues(); | ||
| values.put(MediaStore.Video.Media.DISPLAY_NAME, displayName); | ||
| values.put(MediaStore.Video.Media.MIME_TYPE, "video/mp4"); | ||
| values.put(MediaStore.Video.Media.RELATIVE_PATH, | ||
| Environment.DIRECTORY_MOVIES + "/libCGE"); | ||
| values.put(MediaStore.Video.Media.IS_PENDING, 1); | ||
|
|
||
| Uri collection = MediaStore.Video.Media.getContentUri( | ||
| MediaStore.VOLUME_EXTERNAL_PRIMARY); | ||
| Uri uri = getContentResolver().insert(collection, values); | ||
| if (uri == null) { | ||
| Log.e(LOG_TAG, "createVideoOutputPath: MediaStore insert failed"); | ||
| return null; | ||
| } | ||
| outUri[0] = uri; | ||
|
|
||
| try { | ||
| ParcelFileDescriptor pfd = getContentResolver().openFileDescriptor(uri, "rw"); | ||
| if (pfd == null) { | ||
| getContentResolver().delete(uri, null, null); | ||
| return null; | ||
| } | ||
| outPfd[0] = pfd; | ||
| String path = "/proc/self/fd/" + pfd.getFd(); | ||
| Log.i(LOG_TAG, "createVideoOutputPath: " + path + " -> " + displayName); | ||
| return path; | ||
| } catch (Exception e) { | ||
| Log.e(LOG_TAG, "createVideoOutputPath: " + e); | ||
| getContentResolver().delete(uri, null, null); | ||
| outUri[0] = null; | ||
| return null; | ||
| } | ||
|
|
||
| } else { | ||
| outUri[0] = null; | ||
| outPfd[0] = null; | ||
| File dir = new File( | ||
| Environment.getExternalStoragePublicDirectory(Environment.DIRECTORY_MOVIES), | ||
| "libCGE"); | ||
| if (!dir.exists() && !dir.mkdirs()) { | ||
| Log.e(LOG_TAG, "createVideoOutputPath: mkdirs failed: " + dir); | ||
| return null; | ||
| } | ||
| return new File(dir, displayName).getAbsolutePath(); | ||
| } | ||
| } |
There was a problem hiding this comment.
createVideoOutputPath leaks ParcelFileDescriptor if caller doesn't explicitly close it.
The outPfd[0] is returned to the caller via the output array, but neither this method nor publishVideoToGallery guarantees cleanup if the caller forgets to call publishVideoToGallery (e.g., if generateVideoWithFilter throws or the thread is interrupted). Consider wrapping the entire recording flow in a try-finally that closes the PFD.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java` around lines
96 - 145, createVideoOutputPath currently assigns a ParcelFileDescriptor into
outPfd[0] and returns a proc path but can leak that PFD if the caller (e.g.,
generateVideoWithFilter) throws or the flow aborts before publishVideoToGallery
runs; ensure the PFD is always closed on error and when no longer needed: either
(A) modify createVideoOutputPath to not expose outPfd until the caller
explicitly accepts it or to auto-close the PFD on any exceptional exit (close
pfd in the catch and on any early-return paths), and (B) wrap the entire
recording/generation flow that calls createVideoOutputPath and
generateVideoWithFilter in a try-finally that checks outPfd[0] and calls
ParcelFileDescriptor.close() (and nulls out outPfd[0]) in the finally block;
reference createVideoOutputPath, outPfd, publishVideoToGallery, and
generateVideoWithFilter when making the changes.
library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Implements a runtime-selectable camera backend abstraction to add CameraX support while retaining the existing Camera1 path, and updates the demo app/storage handling for newer Android versions.
Changes:
- Introduces
ICameraProviderplusCamera1Provider/CameraXProviderand a factory for runtime backend selection. - Refactors camera GL views and demo activities to operate through
ICameraProvider(including rotation/preview-size handling). - Updates demo storage/permissions and build tooling (ADB target selection, scoped storage/gallery visibility, Android 15 edge-to-edge opt-out).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
tasks.sh |
Adds ADB target auto-selection and threads target args through run/install commands. |
library/src/main/java/org/wysaid/view/CameraGLSurfaceView.java |
Adds ICameraProvider plumbing, provider-based flash/focus/switch lifecycle. |
library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java |
Migrates preview + capture flow to provider API; adjusts rotation handling for CameraX. |
library/src/main/java/org/wysaid/camera/ICameraProvider.java |
Defines provider abstraction and legacy conversion helpers. |
library/src/main/java/org/wysaid/camera/Camera1Provider.java |
Wraps existing CameraInstance behind ICameraProvider. |
library/src/main/java/org/wysaid/camera/CameraXProvider.java |
Adds CameraX preview/capture implementation with SurfaceTexture-backed SurfaceProvider. |
library/src/main/java/org/wysaid/camera/CameraProviderFactory.java |
Adds factory for selecting Camera1 vs CameraX providers. |
library/src/main/java/org/wysaid/myUtils/PermissionUtil.java |
Updates permission requests for Android 10+ / 13+ media permissions. |
library/src/main/java/org/wysaid/myUtils/FileUtil.java |
Switches default storage path preference to app-specific external storage; adds init(Context). |
library/build.gradle |
Adds CameraX dependencies. |
cgeDemo/src/main/res/values-v35/styles.xml |
Opts demo out of Android 15 forced edge-to-edge enforcement. |
cgeDemo/src/main/res/layout/activity_main.xml |
Adds Camera API selector UI. |
cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java |
Persists Camera API selection and passes it to camera activities. |
cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java |
Uses provider selection + scoped-storage-friendly recording output/publishing. |
cgeDemo/src/main/java/org/wysaid/cgeDemo/FaceTrackingDemoActivity.java |
Uses provider selection and closes via provider on pause. |
cgeDemo/src/main/java/org/wysaid/cgeDemo/TestCaseActivity.java |
Saves images/videos via MediaStore (API 29+) and scans legacy paths otherwise. |
cgeDemo/src/main/AndroidManifest.xml |
Updates storage permission declarations for API 33+ and adds legacy flag. |
.github/instructions/code-conventions.instructions.md |
Minor wording cleanup to conventions doc. |
library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java
Show resolved
Hide resolved
cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java
Outdated
Show resolved
Hide resolved
cgeDemo/src/main/java/org/wysaid/cgeDemo/FaceTrackingDemoActivity.java
Outdated
Show resolved
Hide resolved
cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
library/src/main/java/org/wysaid/camera/CameraXProvider.java (1)
112-113: Restore thread interrupt status when catchingInterruptedException.Swallowing
InterruptedExceptionwithout callingThread.currentThread().interrupt()discards the interrupt signal.♻️ Proposed fix
- } catch (ExecutionException | InterruptedException e) { - Log.e(LOG_TAG, "CameraX: Failed to get ProcessCameraProvider: " + e.toString()); - } + } catch (ExecutionException e) { + Log.e(LOG_TAG, "CameraX: Failed to get ProcessCameraProvider: " + e.toString()); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + Log.e(LOG_TAG, "CameraX: Interrupted while getting ProcessCameraProvider: " + e.toString()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java` around lines 112 - 113, The catch block in CameraXProvider currently swallows InterruptedException in the combined catch (ExecutionException | InterruptedException e) and only logs it; update the error handling so that if e is an InterruptedException you restore the thread interrupt status by calling Thread.currentThread().interrupt() after logging (keep logging via Log.e(LOG_TAG, ...)); to locate the change, modify the catch handling for ExecutionException | InterruptedException in the CameraXProvider class so InterruptedException is detected and Thread.currentThread().interrupt() is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 243-246: Replace use of SurfaceOrientedMeteringPointFactory with
DisplayOrientedMeteringPointFactory when creating metering points from
view/display coordinates: in CameraXProvider.java change the factory creation
that currently uses new SurfaceOrientedMeteringPointFactory(1.0f, 1.0f) to use
DisplayOrientedMeteringPointFactory linked to a Display (or the Preview) so
display rotation is accounted for before calling factory.createPoint(x, y). If
you only have an Application context (mContext) obtain the Display from the
calling Activity's WindowManager or pass mPreview into the factory (3-arg
overload) so CameraX can apply the correct conversion; update any related
imports and ensure createPoint(x,y) now uses the
DisplayOrientedMeteringPointFactory instance.
- Around line 434-452: Replace the deprecated
ImageCapture.Builder.setTargetResolution(Size) usage by creating a
ResolutionSelector with a ResolutionStrategy matching the Preview migration and
apply it to the ImageCapture.Builder; specifically, inside the block that builds
mImageCapture (where ImageCapture.Builder captureBuilder, mPictureWidth,
mPictureHeight and mFlashMode are used), instantiate a ResolutionSelector via
ResolutionSelector.of(ResolutionStrategy.Builder... setResolution(new
Size(mPictureWidth, mPictureHeight)) or equivalent) and call
captureBuilder.setResolutionSelector(resolutionSelector) before
captureBuilder.build(), keeping the existing flash-mode switch logic unchanged;
add imports for androidx.camera.core.resolutionselector.ResolutionSelector and
androidx.camera.core.resolutionselector.ResolutionStrategy and mirror the same
strategy options used for Preview.Builder migration.
---
Duplicate comments:
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 350-372: The JPEG conversion is running on the main executor
inside mImageCapture.takePicture's OnImageCapturedCallback.onCaptureSuccess —
move the heavy work off the main thread: in onCaptureSuccess(ImageProxy image)
submit imageProxyToJpeg(image) to a background executor (or
AsyncTask/Coroutine/Executors.newSingleThreadExecutor), perform YUV→NV21→JPEG
conversion there, ensure image.close() is called after conversion (or in a
finally block), then post the resulting jpegData and rotation back to the main
thread to call pictureCallback.onPictureTaken; update references:
mImageCapture.takePicture,
ImageCapture.OnImageCapturedCallback.onCaptureSuccess, imageProxyToJpeg,
ContextCompat.getMainExecutor(mContext), and pictureCallback to implement this
off-main-thread conversion and safe resource closing.
- Around line 394-397: Replace the deprecated
Preview.Builder.setTargetResolution usage by creating a ResolutionSelector that
targets the preferred preview Size and passing it to
Preview.Builder.setResolutionSelector; specifically, build a ResolutionSelector
configured for new Size(mPreferredPreviewWidth, mPreferredPreviewHeight) (using
ResolutionSelector.Builder and appropriate resolution filter/fallback rules) and
call previewBuilder.setResolutionSelector(theResolutionSelector) instead of
previewBuilder.setTargetResolution(...), keeping the rest of the Preview setup
unchanged.
- Around line 497-553: The imageProxyToYuvImage method incorrectly copies Y and
interleaves UV using buffer.remaining() and ignores per-row padding; fix by
reading Y and UV data row-by-row using each plane's rowStride and pixelStride:
for Y, use planes[0].getRowStride() and copy width bytes per row into nv21 (skip
padding at end of each source row); for chroma, compute chromaWidth=width/2 and
chromaHeight=height/2, use planes[1].getRowStride() (uvRowStride) and
planes[1].getPixelStride() (pixelStride) to read U and V rows and interleave V
then U into nv21 at the correct uvIndex (ySize), iterating chroma rows and
columns and handling pixelStride==1 or 2 by reading source offsets
(row*uvRowStride + col*pixelStride) and writing destination sequentially; keep
using imageProxyToYuvImage, planes, yBuffer/uBuffer/vBuffer, getRowStride(), and
getPixelStride() to locate and implement these changes.
---
Nitpick comments:
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 112-113: The catch block in CameraXProvider currently swallows
InterruptedException in the combined catch (ExecutionException |
InterruptedException e) and only logs it; update the error handling so that if e
is an InterruptedException you restore the thread interrupt status by calling
Thread.currentThread().interrupt() after logging (keep logging via
Log.e(LOG_TAG, ...)); to locate the change, modify the catch handling for
ExecutionException | InterruptedException in the CameraXProvider class so
InterruptedException is detected and Thread.currentThread().interrupt() is
invoked.
Comment on lines
+243
to
+246
| androidx.camera.core.MeteringPointFactory factory = | ||
| new androidx.camera.core.SurfaceOrientedMeteringPointFactory( | ||
| 1.0f, 1.0f); | ||
| androidx.camera.core.MeteringPoint point = factory.createPoint(x, y); |
There was a problem hiding this comment.
SurfaceOrientedMeteringPointFactory produces incorrect metering coordinates in portrait orientation — use DisplayOrientedMeteringPointFactory.
SurfaceOrientedMeteringPointFactory simply normalizes coordinates by dividing by (width, height); it is suitable only for apps that already have coordinates pre-converted into surface-oriented (sensor) space. The x, y values here are display/view coordinates from the caller. This factory aligns with the sensor coordinate system, which is why focus works correctly in landscape (view coords match sensor coords), but produces wrong results in portrait mode.
The official CameraX documentation recommends DisplayOrientedMeteringPointFactory when coordinates come from a SurfaceView or display, as it properly accounts for display rotation and camera orientation.
🐛 Proposed fix
- androidx.camera.core.MeteringPointFactory factory =
- new androidx.camera.core.SurfaceOrientedMeteringPointFactory(
- 1.0f, 1.0f);
+ android.view.Display display = ((android.app.Activity) mContext).getWindowManager().getDefaultDisplay();
+ androidx.camera.core.MeteringPointFactory factory =
+ new androidx.camera.core.DisplayOrientedMeteringPointFactory(
+ display, mCamera.getCameraInfo(), 1.0f, 1.0f);
androidx.camera.core.MeteringPoint point = factory.createPoint(x, y);Note: if mContext is the Application context (as stored in the constructor), getWindowManager() won't be available directly. The Display should be obtained from the calling Activity's WindowManager, or stored as a field when a suitable context is available. Alternatively, mPreview can be passed to the 3-arg SurfaceOrientedMeteringPointFactory(1.0f, 1.0f, mPreview) overload so CameraX can apply the correct aspect-ratio conversion, though DisplayOrientedMeteringPointFactory remains the more accurate choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java` around lines
243 - 246, Replace use of SurfaceOrientedMeteringPointFactory with
DisplayOrientedMeteringPointFactory when creating metering points from
view/display coordinates: in CameraXProvider.java change the factory creation
that currently uses new SurfaceOrientedMeteringPointFactory(1.0f, 1.0f) to use
DisplayOrientedMeteringPointFactory linked to a Display (or the Preview) so
display rotation is accounted for before calling factory.createPoint(x, y). If
you only have an Application context (mContext) obtain the Display from the
calling Activity's WindowManager or pass mPreview into the factory (3-arg
overload) so CameraX can apply the correct conversion; update any related
imports and ensure createPoint(x,y) now uses the
DisplayOrientedMeteringPointFactory instance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
library/src/main/java/org/wysaid/myUtils/FileUtil.java (1)
65-101: Thread safety:storagePathandsAppContextare accessed without synchronization.
storagePath(public static, Line 21) andsAppContext(private static, Line 23) can be read/written from different threads (e.g.,init()on the main thread,getPath()from a background thread). This is pre-existing but exacerbated by the newinit()method that nulls outstoragePath. Consider at minimum making both fieldsvolatile, or synchronizing the accessor methods.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/myUtils/FileUtil.java` around lines 65 - 101, The static fields storagePath and sAppContext are accessed from multiple threads (e.g., init() and getPath()) without synchronization; make them thread-safe by declaring both storagePath and sAppContext as volatile or by synchronizing accessors (e.g., synchronize init(), getPath(), and any setters) so reads/writes are atomic and visible across threads; update references in init(), getPath(), and getPathInPackage() to rely on the chosen approach (volatile or synchronized) to prevent races when nulling or initializing storagePath.library/src/main/java/org/wysaid/camera/CameraXProvider.java (1)
86-90:mCaptureExecutoris never shut down — potential thread leak.If
CameraXProviderinstances are created and discarded (e.g., when switching providers at runtime), the single-thread executor is never shut down and its thread will linger. Consider shutting it down incloseCamera()or providing an explicitrelease()method.♻️ Proposed fix
`@Override` public void closeCamera() { if (Looper.myLooper() != Looper.getMainLooper()) { throw new IllegalStateException( "CameraXProvider.closeCamera() must be called on the main thread."); } mIsPreviewing = false; if (mCameraProvider != null) { mCameraProvider.unbindAll(); mCamera = null; } + mCaptureExecutor.shutdownNow(); }Alternatively, lazily create the executor and track its lifecycle separately if
closeCamera()may be called before capture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java` around lines 86 - 90, CameraXProvider currently creates mCaptureExecutor with Executors.newSingleThreadExecutor() but never shuts it down, causing thread leaks; modify the class to properly shut down mCaptureExecutor (call shutdown() and optionally awaitTermination or shutdownNow()) when the provider is disposed—either by adding executor shutdown logic to the existing closeCamera() method or by introducing a public release() method that closes camera resources and then shuts down mCaptureExecutor; alternatively (optional) lazily instantiate mCaptureExecutor when first needed and null-check it before shutting down to avoid shutting an executor that was never created.library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java (1)
155-203:resumePreviewdual-path (sync + async) size handling looks correct but subtly fragile.The
doStartPreviewRunnable both registers an asyncPreviewSizeReadyCallback(Line 171) and immediately readsgetPreviewWidth()/Height()(Lines 183-187). For Camera1 (synchronous), the immediate read works. For CameraX, the immediate read returns 0 (sizes aren't known until theSurfaceProvidercallback fires), and the async callback handles it later viaqueueEvent.This works because the CameraX callback at Line 176 guards with
pw > 0 && ph > 0, and the immediate path at Line 185 also guards. But if Camera1 ever becomes async or a third provider is added, this dual-read pattern could cause double-resize. Consider documenting this contract more explicitly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java` around lines 155 - 203, The dual-path size handling in resumePreview (doStartPreview Runnable) can call mFrameRecorder.srcResize twice: once from the async provider.startPreview callback and once from the immediate provider.getPreviewWidth()/getPreviewHeight() path; to fix, introduce a small guard that tracks the last applied preview size (e.g., a pair of lastPreviewWidth/lastPreviewHeight fields) and only call mFrameRecorder.srcResize(ph, pw) when pw/ph are > 0 and differ from the tracked values, updating the tracked values after resizing; apply this check in both the startPreview callback and the immediate-read block so duplicate/resizing races are avoided while preserving current behavior (referencing resumePreview, doStartPreview, provider.startPreview callback, provider.getPreviewWidth/getPreviewHeight, and mFrameRecorder.srcResize).cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java (1)
537-547:closeCamera()called twice duringonPause.Line 542 calls
provider.closeCamera()explicitly, then Line 546 callsmCameraView.onPause(), which internally callsgetCameraProvider().closeCamera()again (inCameraGLSurfaceView.onPause(), Line 319). The double call is harmless (CameraX'sunbindAllis idempotent, Camera1 guards internally), but it's redundant. Consider removing the explicit close here and relying on the view'sonPauselifecycle.♻️ Proposed simplification
`@Override` public void onPause() { super.onPause(); - ICameraProvider provider = mCameraView.getCameraProvider(); - if (provider != null) { - provider.closeCamera(); - } Log.i(LOG_TAG, "activity onPause..."); mCameraView.release(null); mCameraView.onPause(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java` around lines 537 - 547, The onPause implementation calls provider.closeCamera() explicitly and then calls mCameraView.onPause(), which itself invokes getCameraProvider().closeCamera() (in CameraGLSurfaceView.onPause()), creating a redundant double-close; remove the explicit provider.closeCamera() call from CameraDemoActivity.onPause() and rely on mCameraView.onPause() to close the camera, keeping the existing mCameraView.release(null) and mCameraView.onPause() calls and their order intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java`:
- Around line 439-458: shouldRotateByExif currently returns true only for
ExifInterface.ORIENTATION_ROTATE_90 which misses ORIENTATION_ROTATE_270 and
causes incorrect skipping of rotation; update shouldRotateByExif to consider
both 90° and 270° rotations (i.e., return true when orientation equals
ExifInterface.ORIENTATION_ROTATE_90 or ExifInterface.ORIENTATION_ROTATE_270)
while preserving the existing null-on-error behavior and file handling in the
shouldRotateByExif method.
- Around line 344-352: In the scaling branch inside
CameraGLSurfaceViewWithTexture (the block that computes scaling and calls
Bitmap.createScaledBitmap with variable bmp), avoid leaking the original bitmap
by storing a reference to the pre-scaled bitmap, creating the scaled bitmap,
then if the new bitmap is a different instance and the old bitmap is not already
recycled call recycle() on the old bitmap before reassigning bmp and updating
width/height; this ensures you only recycle when necessary (oldBmp != bmp &&
!oldBmp.isRecycled()) to avoid recycling the returned bitmap or an
already-recycled instance.
---
Duplicate comments:
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java`:
- Around line 533-535: onDestroy currently doesn't clean up mRecordingPfd
causing a ParcelFileDescriptor leak; update CameraDemoActivity.onDestroy to
check if mRecordingPfd != null (and/or if isRecording flag indicates an active
recording), close the ParcelFileDescriptor inside a try/catch (handle
IOException), set mRecordingPfd to null, and also invoke any
stopRecording/cleanup method used by this class (e.g., stopRecording or
releaseRecordingResources) to ensure all recording-related resources are
released and logged on error.
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/MainActivity.java`:
- Around line 244-255: The code only attaches EXTRA_CAMERA_API for
"CameraDemoActivity", so when face-tracking support lands the camera API won't
be forwarded; update the intent-building block (where intent is created and
EXTRA_CAMERA_API is currently added) to also pass the preference for
FaceTrackingDemoActivity (or any other explicitly camera-backed demo) — for
example check mDemo.activityName for "CameraDemoActivity" OR
"FaceTrackingDemoActivity" (or inspect cls.getSimpleName()/cls equality) and
then read PREFS_KEY_CAMERA_API from PREFS_NAME and putExtra(EXTRA_CAMERA_API,
api) on the intent before startActivity; keep the same prefs keys and
EXTRA_CAMERA_API symbol.
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 250-253: The current use of SurfaceOrientedMeteringPointFactory
with two floats is normalizing against sensor coordinates and mis-maps
view/display coordinates in portrait; replace it so CameraX applies display
rotation/aspect conversion by either creating a
DisplayOrientedMeteringPointFactory and use its createPoint(x, y), or call the
3-arg SurfaceOrientedMeteringPointFactory overload passing the preview view
(mPreview) along with the floats before invoking factory.createPoint(x, y);
update the instantiation in CameraXProvider.java where
SurfaceOrientedMeteringPointFactory is created so the factory receives the
display-aware context.
In `@library/src/main/java/org/wysaid/myUtils/FileUtil.java`:
- Around line 32-39: The init() method now invalidates the cached storagePath to
force re-resolution with the updated context; ensure sAppContext is set from the
provided Context and storagePath is set to null so subsequent calls to getPath()
re-resolve using the new context (verify references to init(), sAppContext,
storagePath, and getPath() are consistent and that no other code assumes
storagePath remains valid across init()).
---
Nitpick comments:
In `@cgeDemo/src/main/java/org/wysaid/cgeDemo/CameraDemoActivity.java`:
- Around line 537-547: The onPause implementation calls provider.closeCamera()
explicitly and then calls mCameraView.onPause(), which itself invokes
getCameraProvider().closeCamera() (in CameraGLSurfaceView.onPause()), creating a
redundant double-close; remove the explicit provider.closeCamera() call from
CameraDemoActivity.onPause() and rely on mCameraView.onPause() to close the
camera, keeping the existing mCameraView.release(null) and mCameraView.onPause()
calls and their order intact.
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 86-90: CameraXProvider currently creates mCaptureExecutor with
Executors.newSingleThreadExecutor() but never shuts it down, causing thread
leaks; modify the class to properly shut down mCaptureExecutor (call shutdown()
and optionally awaitTermination or shutdownNow()) when the provider is
disposed—either by adding executor shutdown logic to the existing closeCamera()
method or by introducing a public release() method that closes camera resources
and then shuts down mCaptureExecutor; alternatively (optional) lazily
instantiate mCaptureExecutor when first needed and null-check it before shutting
down to avoid shutting an executor that was never created.
In `@library/src/main/java/org/wysaid/myUtils/FileUtil.java`:
- Around line 65-101: The static fields storagePath and sAppContext are accessed
from multiple threads (e.g., init() and getPath()) without synchronization; make
them thread-safe by declaring both storagePath and sAppContext as volatile or by
synchronizing accessors (e.g., synchronize init(), getPath(), and any setters)
so reads/writes are atomic and visible across threads; update references in
init(), getPath(), and getPathInPackage() to rely on the chosen approach
(volatile or synchronized) to prevent races when nulling or initializing
storagePath.
In `@library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java`:
- Around line 155-203: The dual-path size handling in resumePreview
(doStartPreview Runnable) can call mFrameRecorder.srcResize twice: once from the
async provider.startPreview callback and once from the immediate
provider.getPreviewWidth()/getPreviewHeight() path; to fix, introduce a small
guard that tracks the last applied preview size (e.g., a pair of
lastPreviewWidth/lastPreviewHeight fields) and only call
mFrameRecorder.srcResize(ph, pw) when pw/ph are > 0 and differ from the tracked
values, updating the tracked values after resizing; apply this check in both the
startPreview callback and the immediate-read block so duplicate/resizing races
are avoided while preserving current behavior (referencing resumePreview,
doStartPreview, provider.startPreview callback,
provider.getPreviewWidth/getPreviewHeight, and mFrameRecorder.srcResize).
library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java
Show resolved
Hide resolved
library/src/main/java/org/wysaid/view/CameraGLSurfaceViewWithTexture.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 148-159: closeCamera() and stopPreview() leave mPreview and
mImageCapture non-null which lets takePicture() call
mImageCapture.takePicture(...) on an unbound use case; extract the duplicated
teardown into a private releaseCameraInternal() helper and call it from both
closeCamera() and stopPreview(), and inside that helper set mIsPreviewing =
false, call mCameraProvider.unbindAll() if mCameraProvider != null, set mCamera
= null, and also null out mPreview and mImageCapture so the takePicture() guard
(mImageCapture == null) works correctly.
- Around line 85-86: mCaptureExecutor is never shut down causing a thread leak
and can lead to RejectedExecutionException when mImageCapture is used after
executor shutdown; modify closeCamera() and stopPreview() to call
mCaptureExecutor.shutdownNow() (or shutdown()) and set mImageCapture = null, and
change the field to be recreated lazily (e.g., initialize mCaptureExecutor when
opening the camera or before using it in takePicture()); ensure takePicture()
still guards for mImageCapture == null and that any code referencing
mCaptureExecutor fetches/creates the executor instance rather than relying on
the old final field.
- Around line 329-334: bindCamera() ignores mPictureSizeBigger and always uses
FALLBACK_RULE_CLOSEST_HIGHER_THEN_LOWER; update bindCamera() to choose the
fallback rule based on the flag set by setPictureSize(int width, int height,
boolean isBigger): when mPictureSizeBigger is true keep using
FALLBACK_RULE_CLOSEST_HIGHER_THEN_LOWER, and when false use the opposite
fallback rule (e.g. FALLBACK_RULE_CLOSEST_LOWER_THEN_HIGHER or whatever constant
represents preferring smaller sizes) when selecting the picture size/resolution
for ImageCapture/Preview so the isBigger flag actually takes effect on next
bind.
---
Duplicate comments:
In `@cgeDemo/src/main/res/values-v35/styles.xml`:
- Around line 11-13: The v35 styles file defines AppTheme which replaces the
base AppTheme on API 35+, dropping shared customizations; create a new base
theme named AppTheme.Base in the default styles (move or copy all common items
like colorPrimary, colorAccent, actionBar/font overrides into AppTheme.Base),
then change the default AppTheme to inherit from AppTheme.Base and change the
v35 AppTheme to inherit from AppTheme.Base and only include the API-35-specific
item (android:windowOptOutEdgeToEdgeEnforcement) so the base customizations are
preserved on all API levels.
In `@library/src/main/java/org/wysaid/camera/CameraXProvider.java`:
- Around line 253-255: The metering point factory is created with
SurfaceOrientedMeteringPointFactory(1.0f, 1.0f) which ignores preview/display
rotation and breaks tap-to-focus in portrait; change the creation to use the
three-argument Preview-aware overload by passing mPreview (i.e. new
SurfaceOrientedMeteringPointFactory(1.0f, 1.0f, mPreview)) so CameraX computes
correct aspect/orientation mapping, or alternatively replace it with
DisplayOrientedMeteringPointFactory if you prefer handling display rotation
explicitly; update the code that constructs the factory
(SurfaceOrientedMeteringPointFactory) to use mPreview which is already
null-checked.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/skills/pr-review/SKILL.md:
- Around line 54-59: The fenced code block containing the markdown table (the
triple-backtick block shown around "| # | Source (comment / CI) | Issue
description | Priority | Action taken | Reason if not fixed |") is missing a
language identifier which triggers MD040; fix it by adding a language specifier
(e.g., "markdown" or "text") immediately after the opening backticks so the
block becomes ```markdown (or ```text), leaving the table content unchanged.
- Add ICameraProvider interface to abstract camera operations - Implement Camera1Provider wrapping legacy Camera API - Implement CameraXProvider using androidx.camera (CameraX) - Create CameraProviderFactory for runtime API selection - Update view layer (CameraGLSurfaceView, CameraGLSurfaceViewWithTexture) to support provider - Add RadioGroup selector in MainActivity for user to choose Camera1 vs CameraX at runtime - Persist user selection via SharedPreferences - Pass selection via Intent extras to camera activities (CameraDemoActivity, FaceTrackingDemoActivity) - Add CameraX dependencies to library/build.gradle - Update CameraInstance constructor to public for backward compatibility - Support both Camera1 and CameraX backends seamlessly without compile-time gating - Mark Camera1-specific APIs as deprecated in favor of provider interface
…ments Android 15 (API 35) enforces edge-to-edge mode for apps compiled with compileSdkVersion >= 35, causing the system status bar to overlap app content and obscuring the Camera1/CameraX radio buttons on the main screen. Add values-v35/styles.xml that sets windowOptOutEdgeToEdgeEnforcement=true to restore traditional window insets behaviour on API 35+ devices without requiring a full insets-aware layout rewrite.
- Update cgeDemo AndroidManifest: Mark WRITE_EXTERNAL_STORAGE and READ_EXTERNAL_STORAGE with maxSdkVersion limits (API 28/32 respectively) to prevent invalid requests on high-API devices. - Add READ_MEDIA_IMAGES and READ_MEDIA_VIDEO fine-grained permissions for Android 13+ (API 33+). - Refactor PermissionUtil.verifyStoragePermissions() to dynamically build permission list based on SDK_INT: * API 33+: Use READ_MEDIA_* and drop no-op WRITE_EXTERNAL_STORAGE * API 29-32: Only READ_EXTERNAL_STORAGE (WRITE is a no-op) * API 28: All legacy permissions - This resolves the issue where Toast was shown but permission dialog did not appear on Android 13+ devices.
ProcessCameraProvider.unbindAll() is a main-thread-only CameraX API. The original switchCamera() dispatched camera lifecycle calls via queueEvent(), which runs on the GL thread, causing: IllegalStateException: Not in application's main thread Changes: - CameraXProvider: add main-thread assertions to closeCamera() and stopPreview() with clear error messages and JavaDoc - CameraGLSurfaceView: remove queueEvent() wrapper from switchCamera() and stopPreview() both are public APIs that callers invoke from the main thread (button click / Activity lifecycle), so no dispatch is needed - No GL-thread synchronization required: RENDERMODE_WHEN_DIRTY ensures the GL thread is naturally idle during the camera switch gap, and mSurfaceTexture is never recreated during the switch
Remove 5 generic/standard rules that are either: - Enforced by compiler (JNI native declarations) - Standard JNI practices (local ref management) - Common best practices (param validation, shader error handling) - Available elsewhere (.clang-format file) Keep only project-specific constraints with direct behavioral impact.
- Refactor all test cases to use MediaStore with IS_PENDING for gallery visibility - Test Case 1: Use createVideoOutputPath() and publishVideoToGallery() for video output - Test Cases 2 & 3: Use new saveBitmapToGallery() for image output to public Pictures directory - Replace deprecated sendBroadcast(ACTION_MEDIA_SCANNER_SCAN_FILE) with proper MediaStore notification - Fix hardcoded /sdcard/libCGE paths in toast messages - Add FileUtil.init(this) for context-aware path resolution
- opt-in ExperimentalCamera2Interop to suppress lint UnsafeOptInUsageError - MainActivity: fix String identity comparison (== -> .equals()) - CameraDemoActivity: add null guard for createVideoOutputPath() result; guard getCameraProvider() NPE in onPause - FileUtil: invalidate storagePath cache in init() to allow re-resolution - PermissionUtil: only pass missing permissions to requestPermissions(), not the full array - CameraGLSurfaceView: make mCameraProvider volatile; synchronize getCameraProvider(), setCameraProvider(), and setFlashMode(); forward existing preview size to newly set provider - CameraGLSurfaceView: fix deprecated focusAtPoint to pass actual Camera instance instead of null to legacy AutoFocusCallback - CameraGLSurfaceViewWithTexture: onSwitchCamera now checks needsManualRotation() before applying PI/2 rotation so CameraX preserves its own orientation handling - Camera1Provider: replace hardcoded 90-degree JPEG rotation with CameraInfo-derived rotation (handles front camera correctly) - CameraXProvider: replace deprecated setTargetResolution() with ResolutionSelector/ResolutionStrategy for Preview and ImageCapture - CameraXProvider: move takePicture image conversion to background executor to avoid blocking the main thread - CameraXProvider: fix imageProxyToYuvImage to correctly copy Y plane row-by-row using rowStride and build NV21 chroma using per-pixel and per-row strides, fixing corrupted output when rowStride != width - Pass EXTRA_CAMERA_API to all activities in MainActivity, not just CameraDemoActivity, so FaceTrackingDemoActivity honours the user's camera API selection - Restore thread interrupt status in CameraXProvider.openCamera() when catching InterruptedException to avoid silently discarding the signal - Add TODO comment in values-v35/styles.xml noting that windowOptOutEdgeToEdgeEnforcement is deprecated in API 36 and will require proper insets handling when targeting Android 16
wysaid
force-pushed
the
feature/camera-x
branch
from
cf107b9 to
fca1df9
Compare
On Android API 29+ (Scoped Storage), createVideoOutputPath() passes a
/proc/self/fd/N path to FFmpeg. avio_open() internally calls open() on
that symlink a second time, which fails on API 34 emulators (EACCES),
causing "could not open file" and recording failure.
Fix: when the filename matches /proc/self/fd/N, dup() the fd and create
a custom AVIOContext backed by write()/lseek64() callbacks instead of
calling avio_open(). The custom context is flushed and freed manually in
cleanup() to avoid double-close via FFmpeg's file protocol.
Fixes: "CGEFrameRecorder::startRecording - start recording failed!" on
Android API 34 emulator with scoped storage.
Owner
Author
|
@Auggie review |
- Make CameraX dependencies compileOnly; load CameraXProvider via reflection so AAR consumers are not forced to pull in CameraX at runtime. Add CameraProviderFactory.isCameraXAvailable() for runtime probing. - Fix SurfaceOrientedMeteringPointFactory: use 3-arg constructor with mPreview so focus coordinates are correctly mapped regardless of sensor orientation. - Fix two Bitmap memory leaks in CameraGLSurfaceViewWithTexture: createScaledBitmap now recycles the original, and the front-camera non-rotate path recycles bmp after drawing to bmp2. - closeCamera()/stopPreview() now auto-post to main thread instead of throwing IllegalStateException when called from background threads. - Null out mPreview/mImageCapture on close; shutdown mCaptureExecutor. - Honor mPictureSizeBigger flag in capture ResolutionStrategy fallback rule. - Close mRecordingPfd in CameraDemoActivity.onDestroy() to prevent fd leak. - Remove redundant closeCamera() call in CameraDemoActivity.onPause() (CameraGLSurfaceView.onPause() already handles it). - Remove unused imports across 4 files.
- CameraXProvider: lock AE target FPS range to (30, 30) instead of (30, 60) - CameraInstance: select the highest supported rate at or below the requested previewRate (defaults to 30fps via DEFAULT_PREVIEW_RATE) instead of always picking the device maximum; fall back to fpsMax only when no rate at or below the target is available
onSwitchCamera() and resumePreview() both mutate FrameRecorder (an OpenGL-backed native object) and must run on the GL thread that owns the context. The previous code called them directly from the main/camera thread, which is undefined behaviour and could cause crashes or misrendering. - Wrap onSwitchCamera() in queueEvent() so rotation/flip changes happen on the GL thread - Wrap the cameraReady callback's resumePreview() in queueEvent() for the same reason - Camera lifecycle calls (closeCamera / openCamera) intentionally remain on the main thread, as required by CameraX
- Add 'Resolving Review Threads' section with GraphQL mutation and thread-listing query templates - Update step 3 of the procedure: resolve the corresponding thread immediately after applying a P1/P2 fix, and resolve outdated threads unconditionally
shouldRotateByExif previously only returned true for ORIENTATION_ROTATE_90. Some Camera1 devices report ORIENTATION_ROTATE_270 for the same physical orientation; the function now returns true in both cases so the bitmap transform is applied correctly on such devices. Also add markdown language identifier to the summary table code block in the pr-review skill (MD040 lint fix).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds CameraX support as an alternative camera backend, selectable at runtime, while preserving full Camera1 backward compatibility.
Changes
New: CameraX Backend
ICameraProvider— new interface abstracting camera operations (open, preview, photo capture, flash, zoom, facing)Camera1Provider— wraps existing Camera1 logic behindICameraProviderCameraXProvider— full CameraX implementation (preview, ImageCapture, flash, zoom, front/back)CameraProviderFactory— factory that selects Camera1 or CameraX at runtime; CameraX is loaded via reflection so the library has no hard runtime dependency onandroidx.camera.*CameraGLSurfaceView— refactored to delegate camera ops throughICameraProvider; CameraX path uses aSurfaceTexture-backedPreview.SurfaceProviderCameraGLSurfaceViewWithTexture— same refactorBug Fixes
styles.xml)TestCaseActivityviaMediaScannerConnectionSurfaceOrientedMeteringPointFactorynow uses the 3-arg constructor withmPreviewso that touch-to-focus maps correctly in any orientationCameraGLSurfaceViewWithTexture:createScaledBitmapnow recycles the original bitmap, and the front-camera non-rotate path recyclesbmpafter drawingcloseCamera()/stopPreview()auto-post to main thread instead of throwingIllegalStateExceptionmPreview/mImageCaptureon close; shut downmCaptureExecutormPictureSizeBiggerflag: captureResolutionStrategyfallback rule now respects theisBiggerparameterCameraDemoActivity.onDestroy()now closesmRecordingPfdcloseCamera()call inCameraDemoActivity.onPause()(already handled byCameraGLSurfaceView.onPause())CameraXProvider,CameraDemoActivity,FaceTrackingDemoActivityDependency Strategy
library/build.gradleare nowcompileOnly— they compile into the library but are not transitively forced onto AAR consumersimplementation 'androidx.camera:camera-*:1.4.1'in their ownbuild.gradle(demo app already does this)CameraProviderFactory.isCameraXAvailable()probes the classpath at runtime viaClass.forNamebefore instantiationIllegalStateExceptionis thrownOther
AndroidManifest.xml: update permission declarations for Android 13+tasks.sh: improvements to build task helpers.github/instructions/code-conventions.instructions.md: minor rule cleanupHow to Verify
adb shell dumpsys meminfo)ClassNotFoundExceptionBackward Compatibility
org.wysaid.nativePort.*is unmodifiedcompileOnly— AAR consumers that do not include CameraX experience no change; upgrading from master is seamless