From 0001ab7a54fc4e4a8bc578cc894e5a7290d3f6ed Mon Sep 17 00:00:00 2001 From: Mattia Iavarone Date: Tue, 10 Nov 2020 12:11:13 +0100 Subject: [PATCH] Rewrite orchestrator (#992) * Add Orchestrator.trim() * Create new orchestrator * Prepare PR --- cameraview/build.gradle.kts | 6 +- .../cameraview/engine/Camera1Engine.java | 12 +- .../cameraview/engine/Camera2Engine.java | 10 +- .../cameraview/engine/CameraBaseEngine.java | 3 + .../orchestrator/CameraOrchestrator.java | 246 +++++++++++------- .../orchestrator/CameraStateOrchestrator.java | 10 +- 6 files changed, 166 insertions(+), 121 deletions(-) diff --git a/cameraview/build.gradle.kts b/cameraview/build.gradle.kts index 360d81df..3f24f095 100644 --- a/cameraview/build.gradle.kts +++ b/cameraview/build.gradle.kts @@ -51,15 +51,15 @@ publisher { project.group = "com.otaliastudios" project.url = "https://github.com/natario1/CameraView" project.addLicense(License.APACHE_2_0) - release.setSources(Release.SOURCES_AUTO) - release.setDocs(Release.DOCS_AUTO) bintray { + release.setSources(Release.SOURCES_AUTO) + release.setDocs(Release.DOCS_AUTO) auth.user = "BINTRAY_USER" auth.key = "BINTRAY_KEY" auth.repo = "BINTRAY_REPO" } directory { - directory = "build/local" + directory = file(repositories.mavenLocal().url).absolutePath } } diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera1Engine.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera1Engine.java index 922da7b0..d8b498cf 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera1Engine.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera1Engine.java @@ -623,9 +623,8 @@ public class Camera1Engine extends CameraBaseEngine implements public void setZoom(final float zoom, @Nullable final PointF[] points, final boolean notify) { final float old = mZoomValue; mZoomValue = zoom; - // Zoom requests can be high frequency (e.g. linked to touch events), so - // we remove the task before scheduling to avoid stack overflows in orchestrator. - getOrchestrator().remove("zoom"); + // Zoom requests can be high frequency (e.g. linked to touch events), let's trim the oldest. + getOrchestrator().trim("zoom", ALLOWED_ZOOM_OPS); mZoomTask = getOrchestrator().scheduleStateful("zoom", CameraState.ENGINE, new Runnable() { @@ -658,9 +657,8 @@ public class Camera1Engine extends CameraBaseEngine implements @Nullable final PointF[] points, final boolean notify) { final float old = mExposureCorrectionValue; mExposureCorrectionValue = EVvalue; - // EV requests can be high frequency (e.g. linked to touch events), so - // we remove the task before scheduling to avoid stack overflows in orchestrator. - getOrchestrator().remove("exposure correction"); + // EV requests can be high frequency (e.g. linked to touch events), let's trim the oldest. + getOrchestrator().trim("exposure correction", ALLOWED_EV_OPS); mExposureCorrectionTask = getOrchestrator().scheduleStateful( "exposure correction", CameraState.ENGINE, @@ -888,7 +886,7 @@ public class Camera1Engine extends CameraBaseEngine implements // The auto focus callback is not guaranteed to be called, but we really want it // to be. So we remove the old runnable if still present and post a new one. getOrchestrator().remove(JOB_FOCUS_END); - getOrchestrator().scheduleDelayed(JOB_FOCUS_END, AUTOFOCUS_END_DELAY_MILLIS, + getOrchestrator().scheduleDelayed(JOB_FOCUS_END, true, AUTOFOCUS_END_DELAY_MILLIS, new Runnable() { @Override public void run() { 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 2fa7f385..505d690f 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/Camera2Engine.java @@ -1246,9 +1246,8 @@ public class Camera2Engine extends CameraBaseEngine implements public void setZoom(final float zoom, final @Nullable PointF[] points, final boolean notify) { final float old = mZoomValue; mZoomValue = zoom; - // Zoom requests can be high frequency (e.g. linked to touch events), so - // we remove the task before scheduling to avoid stack overflows in orchestrator. - getOrchestrator().remove("zoom"); + // Zoom requests can be high frequency (e.g. linked to touch events), let's trim the oldest. + getOrchestrator().trim("zoom", ALLOWED_ZOOM_OPS); mZoomTask = getOrchestrator().scheduleStateful( "zoom", CameraState.ENGINE, @@ -1305,9 +1304,8 @@ public class Camera2Engine extends CameraBaseEngine implements final boolean notify) { final float old = mExposureCorrectionValue; mExposureCorrectionValue = EVvalue; - // EV requests can be high frequency (e.g. linked to touch events), so - // we remove the task before scheduling to avoid stack overflows in orchestrator. - getOrchestrator().remove("exposure correction"); + // EV requests can be high frequency (e.g. linked to touch events), let's trim the oldest. + getOrchestrator().trim("exposure correction", ALLOWED_EV_OPS); mExposureCorrectionTask = getOrchestrator().scheduleStateful( "exposure correction", CameraState.ENGINE, diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/CameraBaseEngine.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/CameraBaseEngine.java index a751f4b0..07e3c231 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/CameraBaseEngine.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/CameraBaseEngine.java @@ -50,6 +50,9 @@ import java.util.List; */ public abstract class CameraBaseEngine extends CameraEngine { + protected final static int ALLOWED_ZOOM_OPS = 20; + protected final static int ALLOWED_EV_OPS = 20; + @SuppressWarnings("WeakerAccess") protected CameraPreview mPreview; @SuppressWarnings("WeakerAccess") protected CameraOptions mCameraOptions; @SuppressWarnings("WeakerAccess") protected PictureRecorder mPictureRecorder; diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraOrchestrator.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraOrchestrator.java index d703eabf..3339dccc 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraOrchestrator.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraOrchestrator.java @@ -1,7 +1,7 @@ package com.otaliastudios.cameraview.engine.orchestrator; +import androidx.annotation.GuardedBy; import androidx.annotation.NonNull; -import androidx.annotation.Nullable; import com.google.android.gms.tasks.OnCompleteListener; import com.google.android.gms.tasks.Task; @@ -12,9 +12,10 @@ import com.otaliastudios.cameraview.internal.WorkerHandler; import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.HashMap; +import java.util.Collections; +import java.util.HashSet; import java.util.List; -import java.util.Map; +import java.util.Set; import java.util.concurrent.Callable; import java.util.concurrent.CancellationException; @@ -40,36 +41,43 @@ public class CameraOrchestrator { void handleJobException(@NonNull String job, @NonNull Exception exception); } - protected static class Token { + protected static class Job { public final String name; - public final Task task; + public final TaskCompletionSource source = new TaskCompletionSource<>(); + public final Callable> scheduler; + public final boolean dispatchExceptions; + public final long startTime; - private Token(@NonNull String name, @NonNull Task task) { + private Job(@NonNull String name, @NonNull Callable> scheduler, boolean dispatchExceptions, long startTime) { this.name = name; - this.task = task; - } - - @Override - public boolean equals(@Nullable Object obj) { - return obj instanceof Token && ((Token) obj).name.equals(name); + this.scheduler = scheduler; + this.dispatchExceptions = dispatchExceptions; + this.startTime = startTime; } } protected final Callback mCallback; - protected final ArrayDeque mJobs = new ArrayDeque<>(); - protected final Object mLock = new Object(); - private final Map mDelayedJobs = new HashMap<>(); + protected final ArrayDeque> mJobs = new ArrayDeque<>(); + protected boolean mJobRunning = false; + protected final Object mJobsLock = new Object(); public CameraOrchestrator(@NonNull Callback callback) { mCallback = callback; - ensureToken(); } @NonNull public Task schedule(@NonNull String name, boolean dispatchExceptions, - @NonNull final Runnable job) { - return schedule(name, dispatchExceptions, new Callable>() { + @NonNull Runnable job) { + return scheduleDelayed(name, dispatchExceptions, 0L, job); + } + + @NonNull + public Task scheduleDelayed(@NonNull String name, + boolean dispatchExceptions, + long minDelay, + @NonNull final Runnable job) { + return scheduleInternal(name, dispatchExceptions, minDelay, new Callable>() { @Override public Task call() { job.run(); @@ -78,98 +86,144 @@ public class CameraOrchestrator { }); } - @SuppressWarnings("unchecked") @NonNull - public Task schedule(@NonNull final String name, - final boolean dispatchExceptions, - @NonNull final Callable> job) { + public Task schedule(@NonNull String name, + boolean dispatchExceptions, + @NonNull Callable> scheduler) { + return scheduleInternal(name, dispatchExceptions, 0L, scheduler); + } + + @NonNull + private Task scheduleInternal(@NonNull String name, + boolean dispatchExceptions, + long minDelay, + @NonNull Callable> scheduler) { LOG.i(name.toUpperCase(), "- Scheduling."); - final TaskCompletionSource source = new TaskCompletionSource<>(); - final WorkerHandler handler = mCallback.getJobWorker(name); - synchronized (mLock) { - applyCompletionListener(mJobs.getLast().task, handler, - new OnCompleteListener() { - @Override - public void onComplete(@NonNull Task task) { - synchronized (mLock) { - mJobs.removeFirst(); - ensureToken(); - } - try { - LOG.i(name.toUpperCase(), "- Executing."); - Task inner = job.call(); - applyCompletionListener(inner, handler, new OnCompleteListener() { - @Override - public void onComplete(@NonNull Task task) { - Exception e = task.getException(); - if (e != null) { - LOG.w(name.toUpperCase(), "- Finished with ERROR.", e); - if (dispatchExceptions) { - mCallback.handleJobException(name, e); - } - source.trySetException(e); - } else if (task.isCanceled()) { - LOG.i(name.toUpperCase(), "- Finished because ABORTED."); - source.trySetException(new CancellationException()); - } else { - LOG.i(name.toUpperCase(), "- Finished."); - source.trySetResult(task.getResult()); - } + Job job = new Job<>(name, scheduler, dispatchExceptions, + System.currentTimeMillis() + minDelay); + synchronized (mJobsLock) { + mJobs.addLast(job); + sync(minDelay); + } + return job.source.getTask(); + } + + private void sync(long after) { + // Jumping on the message handler even if after = 0L should avoid StackOverflow errors. + mCallback.getJobWorker("_sync").post(after, new Runnable() { + @SuppressWarnings("StatementWithEmptyBody") + @Override + public void run() { + synchronized (mJobsLock) { + if (!mJobRunning) { + long now = System.currentTimeMillis(); + Job job = null; + for (Job candidate : mJobs) { + if (candidate.startTime <= now) { + job = candidate; + break; } - }); - } catch (Exception e) { - LOG.i(name.toUpperCase(), "- Finished.", e); - if (dispatchExceptions) mCallback.handleJobException(name, e); - source.trySetException(e); + } + if (job != null) execute(job); + } else { + // Do nothing, job will be picked in executed(). } } - }); - mJobs.addLast(new Token(name, source.getTask())); - } - return source.getTask(); + } + }); } - public void scheduleDelayed(@NonNull final String name, - long minDelay, - @NonNull final Runnable runnable) { - Runnable wrapper = new Runnable() { + @GuardedBy("mJobsLock") + private void execute(@NonNull final Job job) { + if (mJobRunning) { + throw new IllegalStateException("mJobRunning is already true! job=" + job.name); + } + mJobRunning = true; + final WorkerHandler worker = mCallback.getJobWorker(job.name); + worker.run(new Runnable() { @Override public void run() { - schedule(name, true, runnable); - synchronized (mLock) { - if (mDelayedJobs.containsValue(this)) { - mDelayedJobs.remove(name); + try { + LOG.i(job.name.toUpperCase(), "- Executing."); + Task task = job.scheduler.call(); + onComplete(task, worker, new OnCompleteListener() { + @Override + public void onComplete(@NonNull Task task) { + Exception e = task.getException(); + if (e != null) { + LOG.w(job.name.toUpperCase(), "- Finished with ERROR.", e); + if (job.dispatchExceptions) { + mCallback.handleJobException(job.name, e); + } + job.source.trySetException(e); + } else if (task.isCanceled()) { + LOG.i(job.name.toUpperCase(), "- Finished because ABORTED."); + job.source.trySetException(new CancellationException()); + } else { + LOG.i(job.name.toUpperCase(), "- Finished."); + job.source.trySetResult(task.getResult()); + } + synchronized (mJobsLock) { + executed(job); + } + } + }); + } catch (Exception e) { + LOG.i(job.name.toUpperCase(), "- Finished with ERROR.", e); + if (job.dispatchExceptions) { + mCallback.handleJobException(job.name, e); + } + job.source.trySetException(e); + synchronized (mJobsLock) { + executed(job); } } } - }; - synchronized (mLock) { - mDelayedJobs.put(name, wrapper); - mCallback.getJobWorker(name).post(minDelay, wrapper); + }); + } + + @GuardedBy("mJobsLock") + private void executed(Job job) { + if (!mJobRunning) { + throw new IllegalStateException("mJobRunning was not true after completing job=" + job.name); } + mJobRunning = false; + mJobs.remove(job); + sync(0L); } public void remove(@NonNull String name) { - synchronized (mLock) { - if (mDelayedJobs.get(name) != null) { - //noinspection ConstantConditions - mCallback.getJobWorker(name).remove(mDelayedJobs.get(name)); - mDelayedJobs.remove(name); + trim(name, 0); + } + + public void trim(@NonNull String name, int allowed) { + synchronized (mJobsLock) { + List> scheduled = new ArrayList<>(); + for (Job job : mJobs) { + if (job.name.equals(name)) { + scheduled.add(job); + } + } + LOG.v("trim: name=", name, "scheduled=", scheduled.size(), "allowed=", allowed); + int existing = Math.max(scheduled.size() - allowed, 0); + if (existing > 0) { + // To remove the oldest ones first, we must reverse the list. + // Note that we will potentially remove a job that is being executed: we don't + // have a mechanism to cancel the ongoing execution, but it shouldn't be a problem. + Collections.reverse(scheduled); + scheduled = scheduled.subList(0, existing); + for (Job job : scheduled) { + mJobs.remove(job); + } } - Token token = new Token(name, Tasks.forResult(null)); - //noinspection StatementWithEmptyBody - while (mJobs.remove(token)) { /* do nothing */ } - ensureToken(); } } public void reset() { - synchronized (mLock) { - List all = new ArrayList<>(); - //noinspection CollectionAddAllCanBeReplacedWithConstructor - all.addAll(mDelayedJobs.keySet()); - for (Token token : mJobs) { - all.add(token.name); + synchronized (mJobsLock) { + Set all = new HashSet<>(); + for (Job job : mJobs) { + all.add(job.name); } for (String job : all) { remove(job); @@ -177,17 +231,9 @@ public class CameraOrchestrator { } } - private void ensureToken() { - synchronized (mLock) { - if (mJobs.isEmpty()) { - mJobs.add(new Token("BASE", Tasks.forResult(null))); - } - } - } - - private static void applyCompletionListener(@NonNull final Task task, - @NonNull WorkerHandler handler, - @NonNull final OnCompleteListener listener) { + private static void onComplete(@NonNull final Task task, + @NonNull WorkerHandler handler, + @NonNull final OnCompleteListener listener) { if (task.isComplete()) { handler.run(new Runnable() { @Override diff --git a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraStateOrchestrator.java b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraStateOrchestrator.java index 0c3d5aff..f55a6622 100644 --- a/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraStateOrchestrator.java +++ b/cameraview/src/main/java/com/otaliastudios/cameraview/engine/orchestrator/CameraStateOrchestrator.java @@ -35,10 +35,10 @@ public class CameraStateOrchestrator extends CameraOrchestrator { } public boolean hasPendingStateChange() { - synchronized (mLock) { - for (Token token : mJobs) { - if ((token.name.contains(" >> ") || token.name.contains(" << ")) - && !token.task.isComplete()) { + synchronized (mJobsLock) { + for (Job job : mJobs) { + if ((job.name.contains(" >> ") || job.name.contains(" << ")) + && !job.source.getTask().isComplete()) { return true; } } @@ -107,7 +107,7 @@ public class CameraStateOrchestrator extends CameraOrchestrator { @NonNull final CameraState atLeast, long delay, @NonNull final Runnable job) { - scheduleDelayed(name, delay, new Runnable() { + scheduleDelayed(name, true, delay, new Runnable() { @Override public void run() { if (getCurrentState().isAtLeast(atLeast)) {