From 104d54b7bd571747225289a9de063f41dd9da29c Mon Sep 17 00:00:00 2001 From: Mattia Iavarone Date: Wed, 4 Sep 2019 18:13:36 +0200 Subject: [PATCH] Fix various bugs --- .../cameraview/engine/Camera2Engine.java | 31 +++++++++---- .../cameraview/engine/action/BaseAction.java | 13 ++++-- .../cameraview/engine/lock/UnlockAction.java | 25 ----------- .../engine/meter/ExposureMeter.java | 11 ++++- .../engine/meter/ExposureReset.java | 44 ++++++++++++++----- .../cameraview/engine/meter/FocusMeter.java | 8 +++- .../cameraview/engine/meter/FocusReset.java | 12 ++--- .../engine/meter/MeterResetAction.java | 8 ++-- .../engine/meter/WhiteBalanceReset.java | 5 ++- 9 files changed, 95 insertions(+), 62 deletions(-) delete mode 100644 cameraview/src/main/java/com/otaliastudios/cameraview/engine/lock/UnlockAction.java diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java index 46baa3da..82788f5d 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java @@ -45,8 +45,8 @@ import com.otaliastudios.cameraview.controls.WhiteBalance; import com.otaliastudios.cameraview.engine.action.Action; import com.otaliastudios.cameraview.engine.action.ActionHolder; import com.otaliastudios.cameraview.engine.action.Actions; +import com.otaliastudios.cameraview.engine.action.BaseAction; import com.otaliastudios.cameraview.engine.action.CompletionCallback; -import com.otaliastudios.cameraview.engine.lock.UnlockAction; import com.otaliastudios.cameraview.engine.mappers.Camera2Mapper; import com.otaliastudios.cameraview.engine.meter.MeterAction; import com.otaliastudios.cameraview.engine.meter.MeterResetAction; @@ -243,8 +243,8 @@ public class Camera2Engine extends CameraEngine implements ImageReader.OnImageAv } } - private final CameraCaptureSession.CaptureCallback mRepeatingRequestCallback = - new CameraCaptureSession.CaptureCallback() { + private final CameraCaptureSession.CaptureCallback mRepeatingRequestCallback + = new CameraCaptureSession.CaptureCallback() { @Override public void onCaptureStarted(@NonNull CameraCaptureSession session, @NonNull CaptureRequest request, @@ -701,11 +701,13 @@ public class Camera2Engine extends CameraEngine implements ImageReader.OnImageAv boolean fullPicture = mPictureRecorder instanceof Full2PictureRecorder; super.onPictureResult(result, error); if (fullPicture && mPictureCaptureStopsPreview) { - // See comments in Full2PictureRecorder. applyRepeatingRequestBuilder(); } - boolean unlock = (fullPicture && getPictureMetering()) || - (!fullPicture && getPictureSnapshotMetering()); + + // Some picture recorders might lock metering, and we usually run a metering sequence + // before running the recorders. So, run an unlock/reset sequence if needed. + boolean unlock = (fullPicture && getPictureMetering()) + || (!fullPicture && getPictureSnapshotMetering()); if (unlock) { unlockAndResetMetering(); } @@ -848,6 +850,7 @@ public class Camera2Engine extends CameraEngine implements ImageReader.OnImageAv builder.set(CaptureRequest.CONTROL_AE_REGIONS, oldBuilder.get(CaptureRequest.CONTROL_AE_REGIONS)); builder.set(CaptureRequest.CONTROL_AWB_REGIONS, oldBuilder.get(CaptureRequest.CONTROL_AWB_REGIONS)); builder.set(CaptureRequest.CONTROL_AF_MODE, oldBuilder.get(CaptureRequest.CONTROL_AF_MODE)); + // Do NOT copy exposure or focus triggers! } } @@ -1273,10 +1276,20 @@ public class Camera2Engine extends CameraEngine implements ImageReader.OnImageAv private void unlockAndResetMetering() { if (getEngineState() == STATE_STARTED) { - applyDefaultFocus(mRepeatingRequestBuilder); Actions.sequence( - new UnlockAction(), - new MeterResetAction(true) + new BaseAction() { + @Override + protected void onStart(@NonNull ActionHolder holder) { + super.onStart(holder); + applyDefaultFocus(holder.getBuilder(this)); + holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_LOCK, false); + holder.getBuilder(this).set(CaptureRequest.CONTROL_AWB_LOCK, false); + holder.applyBuilder(this); + setState(STATE_COMPLETED); + // TODO should wait results? + } + }, + new MeterResetAction() ).start(Camera2Engine.this); } } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/action/BaseAction.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/action/BaseAction.java index f744d38a..0c3d29e2 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/action/BaseAction.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/action/BaseAction.java @@ -67,7 +67,7 @@ public abstract class BaseAction implements Action { * holder stream anymore. It will soon be marked as completed. * @param holder holder */ - @SuppressWarnings({"WeakerAccess", "unused"}) + @SuppressWarnings("unused") protected void onAbort(@NonNull ActionHolder holder) { // Overrideable } @@ -100,6 +100,7 @@ public abstract class BaseAction implements Action { } if (state == STATE_COMPLETED) { holder.removeAction(this); + onCompleted(holder); } } } @@ -108,11 +109,18 @@ public abstract class BaseAction implements Action { * Whether this action has reached the completed state. * @return true if completed */ - @SuppressWarnings("WeakerAccess") public boolean isCompleted() { return state == STATE_COMPLETED; } + /** + * Called when this action has completed (possibly aborted). + * @param holder holder + */ + protected void onCompleted(@NonNull ActionHolder holder) { + // Overrideable + } + /** * Returns the holder. * @return the holder @@ -130,7 +138,6 @@ public abstract class BaseAction implements Action { * @param key type * @return value or fallback */ - @SuppressWarnings("WeakerAccess") @NonNull protected T readCharacteristic(@NonNull CameraCharacteristics.Key key, @NonNull T fallback) { diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/lock/UnlockAction.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/lock/UnlockAction.java deleted file mode 100644 index 28941b06..00000000 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/lock/UnlockAction.java +++ /dev/null @@ -1,25 +0,0 @@ -package com.otaliastudios.cameraview.engine.lock; - -import android.hardware.camera2.CaptureRequest; -import android.os.Build; - -import androidx.annotation.NonNull; -import androidx.annotation.RequiresApi; - -import com.otaliastudios.cameraview.engine.action.ActionHolder; -import com.otaliastudios.cameraview.engine.action.BaseAction; - -@RequiresApi(Build.VERSION_CODES.LOLLIPOP) -public class UnlockAction extends BaseAction { - - @Override - protected void onStart(@NonNull ActionHolder holder) { - super.onStart(holder); - holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_LOCK, false); - holder.getBuilder(this).set(CaptureRequest.CONTROL_AWB_LOCK, false); - holder.applyBuilder(this); - setState(STATE_COMPLETED); - // TODO focus is managed by the engine - // TODO should wait results? - } -} diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureMeter.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureMeter.java index 37b626d7..cfd0826f 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureMeter.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureMeter.java @@ -24,12 +24,11 @@ public class ExposureMeter extends BaseMeter { private static final int STATE_WAITING_PRECAPTURE = 0; private static final int STATE_WAITING_PRECAPTURE_END = 1; + @SuppressWarnings("WeakerAccess") public ExposureMeter(@NonNull List areas, boolean skipIfPossible) { super(areas, skipIfPossible); } - // TODO set trigger to null? - @Override protected boolean checkIsSupported(@NonNull ActionHolder holder) { // In our case, this means checking if we support the AE precapture trigger. @@ -76,6 +75,14 @@ public class ExposureMeter extends BaseMeter { setState(STATE_WAITING_PRECAPTURE); } + @Override + protected void onCompleted(@NonNull ActionHolder holder) { + super.onCompleted(holder); + // Remove (but not apply) the risky parameter so it is not included in new requests. + // Documentation about this key says that this should be allowed. + holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER, null); + } + @Override public void onCaptureCompleted(@NonNull ActionHolder holder, @NonNull CaptureRequest request, @NonNull TotalCaptureResult result) { diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureReset.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureReset.java index 4997e01d..04ace0b9 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureReset.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/ExposureReset.java @@ -2,6 +2,8 @@ package com.otaliastudios.cameraview.engine.meter; import android.hardware.camera2.CameraCharacteristics; import android.hardware.camera2.CaptureRequest; +import android.hardware.camera2.CaptureResult; +import android.hardware.camera2.TotalCaptureResult; import android.hardware.camera2.params.MeteringRectangle; import android.os.Build; @@ -18,33 +20,53 @@ public class ExposureReset extends BaseReset { private static final String TAG = ExposureReset.class.getSimpleName(); private static final CameraLogger LOG = CameraLogger.create(TAG); - public ExposureReset(boolean resetArea) { - super(resetArea); + private static final int STATE_WAITING_LOCK = 0; + + @SuppressWarnings("WeakerAccess") + public ExposureReset() { + super(true); } @Override protected void onStarted(@NonNull ActionHolder holder, @Nullable MeteringRectangle area) { - boolean changed = false; int maxRegions = readCharacteristic(CameraCharacteristics.CONTROL_MAX_REGIONS_AE, 0); if (area != null && maxRegions > 0) { holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_REGIONS, new MeteringRectangle[]{area}); - changed = true; } // NOTE: precapture might not be supported, in which case I think it will be ignored. - Integer trigger = holder.getBuilder(this).get(CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER); - LOG.w("onStarted:", "current precapture trigger is", trigger); - if (trigger == null || trigger == CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_START) { - LOG.w("onStarted:", "canceling precapture."); + Integer trigger = holder.getLastResult(this).get(CaptureResult.CONTROL_AE_PRECAPTURE_TRIGGER); + LOG.i("onStarted:", "last precapture trigger is", trigger); + if (trigger != null && trigger == CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_START) { + LOG.i("onStarted:", "canceling precapture."); int newTrigger = Build.VERSION.SDK_INT >= 23 ? CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_CANCEL : CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_IDLE; holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER, newTrigger); - changed = true; } - if (changed) holder.applyBuilder(this); - setState(STATE_COMPLETED); + // Documentation about CONTROL_AE_PRECAPTURE_TRIGGER says that, if it was started but not + // followed by a CAPTURE_INTENT_STILL_PICTURE request, the internal AE routine might remain + // locked unless we unlock manually. + // This is often the case for us, since the snapshot picture recorder does not use the intent + // and anyway we use the precapture sequence for touch metering as well. + // To reset, docs suggest the use of CONTROL_AE_PRECAPTURE_TRIGGER_CANCEL, which we do above, + // or the technique used below: locking then unlocking. This proved to be the ONLY method + // to unlock reliably, unlike the cancel trigger (which we'll run anyway). + holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_LOCK, true); + holder.applyBuilder(this); + setState(STATE_WAITING_LOCK); + } + + @Override + public void onCaptureCompleted(@NonNull ActionHolder holder, @NonNull CaptureRequest request, + @NonNull TotalCaptureResult result) { + super.onCaptureCompleted(holder, request, result); + if (getState() == STATE_WAITING_LOCK) { + holder.getBuilder(this).set(CaptureRequest.CONTROL_AE_LOCK, false); + holder.applyBuilder(this); + setState(STATE_COMPLETED); + } } } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusMeter.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusMeter.java index a3e72e7c..0356a20b 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusMeter.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusMeter.java @@ -62,7 +62,13 @@ public class FocusMeter extends BaseMeter { holder.applyBuilder(this); } - // TODO Set trigger to null? + @Override + protected void onCompleted(@NonNull ActionHolder holder) { + super.onCompleted(holder); + // Remove (but not apply) the risky parameter so it is not included in new requests. + // Documentation about this key says that this should be allowed. + holder.getBuilder(this).set(CaptureRequest.CONTROL_AF_TRIGGER, null); + } @Override public void onCaptureCompleted(@NonNull ActionHolder holder, @NonNull CaptureRequest request, diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusReset.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusReset.java index 318dee05..7554a86c 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusReset.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/FocusReset.java @@ -2,6 +2,7 @@ package com.otaliastudios.cameraview.engine.meter; import android.hardware.camera2.CameraCharacteristics; import android.hardware.camera2.CaptureRequest; +import android.hardware.camera2.CaptureResult; import android.hardware.camera2.params.MeteringRectangle; import android.os.Build; @@ -18,8 +19,9 @@ public class FocusReset extends BaseReset { private static final String TAG = FocusReset.class.getSimpleName(); private static final CameraLogger LOG = CameraLogger.create(TAG); - public FocusReset(boolean resetArea) { - super(resetArea); + @SuppressWarnings("WeakerAccess") + public FocusReset() { + super(true); } @Override @@ -33,9 +35,9 @@ public class FocusReset extends BaseReset { } // NOTE: trigger might not be supported, in which case I think it will be ignored. - Integer trigger = holder.getBuilder(this).get(CaptureRequest.CONTROL_AF_TRIGGER); - LOG.w("onStarted:", "current focus trigger is", trigger); - if (trigger == null || trigger == CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_START) { + Integer trigger = holder.getLastResult(this).get(CaptureResult.CONTROL_AF_TRIGGER); + LOG.w("onStarted:", "last focus trigger is", trigger); + if (trigger != null && trigger == CaptureRequest.CONTROL_AE_PRECAPTURE_TRIGGER_START) { holder.getBuilder(this).set(CaptureRequest.CONTROL_AF_TRIGGER, CaptureRequest.CONTROL_AF_TRIGGER_CANCEL); changed = true; diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/MeterResetAction.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/MeterResetAction.java index 91dae114..e2da92cf 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/MeterResetAction.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/MeterResetAction.java @@ -18,11 +18,11 @@ public class MeterResetAction extends ActionWrapper { private final BaseAction action; - public MeterResetAction(boolean resetAreas) { + public MeterResetAction() { this.action = Actions.together( - new ExposureReset(resetAreas), - new FocusReset(resetAreas), - new WhiteBalanceReset(resetAreas) + new ExposureReset(), + new FocusReset(), + new WhiteBalanceReset() ); } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/WhiteBalanceReset.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/WhiteBalanceReset.java index c65c399d..5d24fd55 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/WhiteBalanceReset.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/meter/WhiteBalanceReset.java @@ -22,8 +22,9 @@ public class WhiteBalanceReset extends BaseReset { private static final String TAG = WhiteBalanceReset.class.getSimpleName(); private static final CameraLogger LOG = CameraLogger.create(TAG); - public WhiteBalanceReset(boolean resetArea) { - super(resetArea); + @SuppressWarnings("WeakerAccess") + public WhiteBalanceReset() { + super(true); } @Override