Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions .github/skills/pr-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,13 +56,17 @@ Before acting on any review comment or suggestion, classify it by importance:
|-------|----------|--------|
| **P1 — Must Fix** | Bug, security issue, broken behavior, API contract violation, CI failure | Fix immediately |
| **P2 — Should Fix** | Correctness risk, meaningful maintainability improvement, clear code smell with real impact | Fix with brief justification |
| **P3 — Optional** | Style preference, minor naming nitpick, debatable design choice, "could be cleaner" | **Do NOT auto-fix** — surface to user for decision |
| **P3 — Conditional** | Style preference, minor naming, "could be cleaner" | Fix if it aligns with best practices **and** the change is safe + trivial; defer if complex or has any potential functional impact |
| **P4 — Reject** | Contradicts project conventions, introduces unnecessary complexity, or is factually wrong | Reject with explanation |

**Rules:**
- **Do not implement P3 suggestions automatically.** Note them in the final summary and let the user decide.
- For **P3**, apply this two-question test before touching the code:
1. **Best practice?** — Does the change follow language/framework conventions (e.g. prefer imports over FQNs, use existing imports, standard patterns)?
2. **Safe & trivial?** — Is the diff mechanical with zero risk of behavioral change and low effort?
- Both **yes** → fix it silently, mark thread resolved.
- Either **no** → do NOT modify code; record in the summary table and let the user decide.
- **Do not add comments to code** unless the comment explains non-obvious logic that is truly necessary. Never add comments just to acknowledge a review suggestion was applied.
- When in doubt about importance, prefer the lower severity (P3/P4) and defer to the user rather than making the change.
- When the two-question test is ambiguous, prefer deferring rather than guessing.

## Procedure

Expand All @@ -80,7 +84,7 @@ Before acting on any review comment or suggestion, classify it by importance:
3. **Address review comments** — apply the Review Restraint Policy to each comment:
- P1/P2: implement the fix, then **immediately resolve the thread** using the GraphQL mutation above
- Outdated threads: resolve them regardless of priority (no action needed, just mark resolved)
- P3: record in summary, do **not** modify code, ask user
- P3: apply the two-question test — fix + resolve if both answers are yes; otherwise record in summary and defer to user
- P4: record rejection reason in summary
4. **Commit & push** — single commit covering all non-workflow fixes (workflow fixes are pushed incrementally during step 2)
5. **Final verification** — once all fixes are applied, confirm every check is green: `GH_PAGER= gh pr checks <PR>`
Expand All @@ -94,8 +98,9 @@ Before acting on any review comment or suggestion, classify it by importance:
```markdown
| # | Source (comment / CI) | Issue description | Priority | Action taken | Reason if not fixed |
|---|-----------------------|-------------------|----------|--------------|---------------------|
| 1 | Reviewer @xxx | e.g. rename foo() | P3 | Not fixed | Style preference, no functional impact — deferred to user |
| 2 | CI: lint | e.g. null-check | P1 | Fixed | — |
| 1 | Reviewer @xxx | use import instead of FQN | P3 | Fixed | Best practice + trivial mechanical change |
| 2 | Reviewer @xxx | rename internal var foo→bar | P3 | Not fixed | Non-standard opinion, no best-practice backing — deferred to user |
| 3 | CI: lint | null-check missing | P1 | Fixed | — |
```

All P3/P4 items that were **not** fixed must appear in this table with a clear reason, so the user can make an informed decision.
82 changes: 79 additions & 3 deletions library/src/main/java/org/wysaid/camera/Camera1Provider.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import android.hardware.Camera;
import android.util.Log;

import java.util.List;

import org.wysaid.common.Common;

/**
Expand Down Expand Up @@ -103,7 +105,7 @@ public void focusAtPoint(float x, float y, float radius, AutoFocusCallback callb

@Override
public boolean setFlashMode(FlashMode mode) {
Camera.Parameters params = CameraInstance.getInstance().getParams();
Camera.Parameters params = getParams();
if (params == null) return false;

String camera1Mode = ICameraProvider.flashModeToCamera1(mode);
Expand All @@ -124,7 +126,7 @@ public boolean setFlashMode(FlashMode mode) {

@Override
public FlashMode getFlashMode() {
Camera.Parameters params = CameraInstance.getInstance().getParams();
Camera.Parameters params = getParams();
if (params == null) return null;
return ICameraProvider.camera1ToFlashMode(params.getFlashMode());
}
Expand All @@ -139,6 +141,80 @@ public void setFocusMode(String focusMode) {
CameraInstance.getInstance().setFocusMode(focusMode);
}

// ========== Private helpers ==========

/**
* Shorthand for {@link CameraInstance#getParams()}.
* Returns {@code null} when the camera is closed.
*/
private Camera.Parameters getParams() {
return CameraInstance.getInstance().getParams();
}

/**
* Returns camera parameters only when the camera is open <em>and</em> zoom is supported;
* returns {@code null} otherwise. Use this as the single guard in all zoom methods.
*/
private Camera.Parameters getZoomParams() {
Camera.Parameters params = getParams();
return (params != null && params.isZoomSupported()) ? params : null;
}

// ========== Zoom ==========

@Override
public boolean isZoomSupported() {
return getZoomParams() != null;
}

@Override
public float getMinZoomRatio() {
return 1.0f; // Camera1 always starts at 1.0x (zoom index 0)
}

@Override
public float getMaxZoomRatio() {
Camera.Parameters params = getZoomParams();
if (params == null) return 1.0f;
List<Integer> ratios = params.getZoomRatios();
if (ratios == null || ratios.isEmpty()) return 1.0f;
int maxIndex = Math.min(params.getMaxZoom(), ratios.size() - 1);
return ratios.get(maxIndex) / 100.0f;
}

@Override
public void setZoomRatio(float ratio) {
Camera.Parameters params = getZoomParams();
if (params == null) return;

if (!Float.isFinite(ratio) || ratio <= 0f) {
Log.e(LOG_TAG, "Camera1: invalid zoom ratio: " + ratio);
return;
}

List<Integer> zoomRatios = params.getZoomRatios();
if (zoomRatios == null || zoomRatios.isEmpty()) return;
int targetHundredths = Math.round(ratio * 100.0f);

// Find the closest discrete zoom index for the requested ratio.
int bestIndex = 0;
long bestDiff = Long.MAX_VALUE;
for (int i = 0; i < zoomRatios.size(); i++) {
long diff = Math.abs((long) zoomRatios.get(i) - targetHundredths);
if (diff < bestDiff) {
bestDiff = diff;
bestIndex = i;
}
}

Comment thread
wysaid marked this conversation as resolved.
try {
params.setZoom(bestIndex);
CameraInstance.getInstance().setParams(params);
} catch (Exception e) {
Log.e(LOG_TAG, "Camera1: setZoomRatio failed: " + e.toString());
}
}

// ========== Capture ==========

/**
Expand Down Expand Up @@ -190,7 +266,7 @@ public void takePicture(ShutterCallback shutterCallback, PictureDataCallback pic
int jpegRotation = computeJpegRotation(camera1Facing);

// Set rotation
Camera.Parameters params = CameraInstance.getInstance().getParams();
Camera.Parameters params = getParams();
if (params != null) {
try {
params.setRotation(jpegRotation);
Expand Down
63 changes: 63 additions & 0 deletions library/src/main/java/org/wysaid/camera/CameraXProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
import android.util.Size;
import android.view.Surface;

import androidx.camera.core.ZoomState;

import androidx.camera.camera2.interop.Camera2Interop;
import androidx.camera.camera2.interop.ExperimentalCamera2Interop;

Expand Down Expand Up @@ -340,6 +342,67 @@ public void setFocusMode(String focusMode) {
Log.i(LOG_TAG, "CameraX: setFocusMode ignored (CameraX manages focus automatically): " + focusMode);
}

// ========== Zoom ==========

@Override
public boolean isZoomSupported() {
if (mCamera == null) return false;
ZoomState state = mCamera.getCameraInfo().getZoomState().getValue();
return state != null && state.getMaxZoomRatio() > state.getMinZoomRatio();
}

@Override
public float getMinZoomRatio() {
if (mCamera == null) return 1.0f;
ZoomState state = mCamera.getCameraInfo().getZoomState().getValue();
return state != null ? state.getMinZoomRatio() : 1.0f;
}

@Override
public float getMaxZoomRatio() {
if (mCamera == null) return 1.0f;
ZoomState state = mCamera.getCameraInfo().getZoomState().getValue();
return state != null ? state.getMaxZoomRatio() : 1.0f;
}

@Override
public void setZoomRatio(float ratio) {
if (mCamera == null) {
Log.w(LOG_TAG, "CameraX: setZoomRatio called but camera not bound.");
return;
}

ZoomState zoomState = mCamera.getCameraInfo().getZoomState().getValue();
if (zoomState == null) {
Log.w(LOG_TAG, "CameraX: setZoomRatio called but ZoomState not yet available.");
return;
}

float minZoom = zoomState.getMinZoomRatio();
float maxZoom = zoomState.getMaxZoomRatio();
float clampedRatio = Math.max(minZoom, Math.min(ratio, maxZoom));

if (clampedRatio != ratio) {
Log.w(LOG_TAG, "CameraX: Requested zoom ratio " + ratio
+ " is out of range [" + minZoom + ", " + maxZoom + "], clamped to "
+ clampedRatio + ".");
}

final ListenableFuture<Void> future =
mCamera.getCameraControl().setZoomRatio(clampedRatio);
future.addListener(() -> {
try {
future.get();
} catch (ExecutionException e) {
Throwable cause = e.getCause();
Log.e(LOG_TAG, "CameraX: setZoomRatio failed: " + (cause != null ? cause : e));
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
Log.e(LOG_TAG, "CameraX: setZoomRatio interrupted.", e);
}
}, ContextCompat.getMainExecutor(mContext));
}

// ========== Capture ==========

@Override
Expand Down
50 changes: 50 additions & 0 deletions library/src/main/java/org/wysaid/camera/ICameraProvider.java
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,56 @@ default void startPreview(SurfaceTexture texture, PreviewSizeReadyCallback callb
*/
void setFocusMode(String focusMode);

// ========== Zoom ==========

/**
* Whether the camera hardware supports zoom.
*
* <p>CameraX always returns {@code true} (handled by the framework).
* Camera1 returns the result of {@link android.hardware.Camera.Parameters#isZoomSupported()}.
*
* @return {@code true} if zoom is supported, {@code false} otherwise.
*/
default boolean isZoomSupported() {
return false;
}

/**
* Get the minimum zoom ratio supported by the camera hardware.
* Always 1.0 (no zoom, native field of view).
*
* @return minimum zoom ratio, typically 1.0f.
*/
default float getMinZoomRatio() {
return 1.0f;
}

/**
* Get the maximum zoom ratio supported by the camera hardware.
*
* @return maximum zoom ratio (e.g. 10.0f); 1.0f when zoom is unsupported.
*/
default float getMaxZoomRatio() {
return 1.0f;
}

/**
* Set the zoom ratio.
*
* <p>CameraX applies the ratio asynchronously via {@code CameraControl.setZoomRatio()}.
* Camera1 maps the float ratio to the closest supported integer zoom index
* ({@link android.hardware.Camera.Parameters#setZoom(int)}) — slight rounding may occur
* because Camera1 only exposes discrete steps.
*
* <p>Values are clamped to [{@link #getMinZoomRatio()}, {@link #getMaxZoomRatio()}].
* Calling this method when {@link #isZoomSupported()} returns {@code false} is a no-op.
*
* @param ratio The desired zoom ratio. 1.0f means no zoom.
*/
default void setZoomRatio(float ratio) {
// Default no-op; implementations override.
}

// ========== Capture ==========

/**
Expand Down
Loading