Rewrite orchestrator (#992)

* Add Orchestrator.trim()

* Create new orchestrator

* Prepare PR

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
     }
 }
 

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
+        // Zoom requests can be high frequency (e.g. linked to touch events), let's trim the oldest.
-        // we remove the task before scheduling to avoid stack overflows in orchestrator.
+        getOrchestrator().trim("zoom", ALLOWED_ZOOM_OPS);
-        getOrchestrator().remove("zoom");
         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
+        // EV requests can be high frequency (e.g. linked to touch events), let's trim the oldest.
-        // we remove the task before scheduling to avoid stack overflows in orchestrator.
+        getOrchestrator().trim("exposure correction", ALLOWED_EV_OPS);
-        getOrchestrator().remove("exposure correction");
         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() {

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
+        // Zoom requests can be high frequency (e.g. linked to touch events), let's trim the oldest.
-        // we remove the task before scheduling to avoid stack overflows in orchestrator.
+        getOrchestrator().trim("zoom", ALLOWED_ZOOM_OPS);
-        getOrchestrator().remove("zoom");
         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
+        // EV requests can be high frequency (e.g. linked to touch events), let's trim the oldest.
-        // we remove the task before scheduling to avoid stack overflows in orchestrator.
+        getOrchestrator().trim("exposure correction", ALLOWED_EV_OPS);
-        getOrchestrator().remove("exposure correction");
         mExposureCorrectionTask = getOrchestrator().scheduleStateful(
                 "exposure correction",
                 CameraState.ENGINE,

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;

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<T> {
         public final String name;
-        public final Task<?> task;
+        public final TaskCompletionSource<T> source = new TaskCompletionSource<>();
+        public final Callable<Task<T>> scheduler;
+        public final boolean dispatchExceptions;
+        public final long startTime;
 
-        private Token(@NonNull String name, @NonNull Task<?> task) {
+        private Job(@NonNull String name, @NonNull Callable<Task<T>> scheduler, boolean dispatchExceptions, long startTime) {
             this.name = name;
-            this.task = task;
+            this.scheduler = scheduler;
-        }
+            this.dispatchExceptions = dispatchExceptions;
-
+            this.startTime = startTime;
-        @Override
-        public boolean equals(@Nullable Object obj) {
-            return obj instanceof Token && ((Token) obj).name.equals(name);
         }
     }
 
     protected final Callback mCallback;
-    protected final ArrayDeque<Token> mJobs = new ArrayDeque<>();
+    protected final ArrayDeque<Job<?>> mJobs = new ArrayDeque<>();
-    protected final Object mLock = new Object();
+    protected boolean mJobRunning = false;
-    private final Map<String, Runnable> mDelayedJobs = new HashMap<>();
+    protected final Object mJobsLock = new Object();
 
     public CameraOrchestrator(@NonNull Callback callback) {
         mCallback = callback;
-        ensureToken();
     }
 
     @NonNull
     public Task<Void> schedule(@NonNull String name,
                                boolean dispatchExceptions,
-                               @NonNull final Runnable job) {
+                               @NonNull Runnable job) {
-        return schedule(name, dispatchExceptions, new Callable<Task<Void>>() {
+        return scheduleDelayed(name, dispatchExceptions, 0L, job);
+    }
+
+    @NonNull
+    public Task<Void> scheduleDelayed(@NonNull String name,
+                                      boolean dispatchExceptions,
+                                      long minDelay,
+                                      @NonNull final Runnable job) {
+        return scheduleInternal(name, dispatchExceptions, minDelay, new Callable<Task<Void>>() {
             @Override
             public Task<Void> call() {
                 job.run();
@@ -78,98 +86,144 @@ public class CameraOrchestrator {
         });
     }
 
-    @SuppressWarnings("unchecked")
     @NonNull
-    public <T> Task<T> schedule(@NonNull final String name,
+    public <T> Task<T> schedule(@NonNull String name,
-                                final boolean dispatchExceptions,
+                                boolean dispatchExceptions,
-                                @NonNull final Callable<Task<T>> job) {
+                                @NonNull Callable<Task<T>> scheduler) {
+        return scheduleInternal(name, dispatchExceptions, 0L, scheduler);
+    }
+
+    @NonNull
+    private <T> Task<T> scheduleInternal(@NonNull String name,
+                                         boolean dispatchExceptions,
+                                         long minDelay,
+                                         @NonNull Callable<Task<T>> scheduler) {
         LOG.i(name.toUpperCase(), "- Scheduling.");
-        final TaskCompletionSource<T> source = new TaskCompletionSource<>();
+        Job<T> job = new Job<>(name, scheduler, dispatchExceptions,
-        final WorkerHandler handler = mCallback.getJobWorker(name);
+                System.currentTimeMillis() + minDelay);
-        synchronized (mLock) {
+        synchronized (mJobsLock) {
-            applyCompletionListener(mJobs.getLast().task, handler,
+            mJobs.addLast(job);
-                    new OnCompleteListener() {
+            sync(minDelay);
-                @Override
+        }
-                public void onComplete(@NonNull Task task) {
+        return job.source.getTask();
-                    synchronized (mLock) {
+    }
-                        mJobs.removeFirst();
+
-                        ensureToken();
+    private void sync(long after) {
-                    }
+        // Jumping on the message handler even if after = 0L should avoid StackOverflow errors.
-                    try {
+        mCallback.getJobWorker("_sync").post(after, new Runnable() {
-                        LOG.i(name.toUpperCase(), "- Executing.");
+            @SuppressWarnings("StatementWithEmptyBody")
-                        Task<T> inner = job.call();
+            @Override
-                        applyCompletionListener(inner, handler, new OnCompleteListener<T>() {
+            public void run() {
-                            @Override
+                synchronized (mJobsLock) {
-                            public void onComplete(@NonNull Task<T> task) {
+                    if (!mJobRunning) {
-                                Exception e = task.getException();
+                        long now = System.currentTimeMillis();
-                                if (e != null) {
+                        Job<?> job = null;
-                                    LOG.w(name.toUpperCase(), "- Finished with ERROR.", e);
+                        for (Job<?> candidate : mJobs) {
-                                    if (dispatchExceptions) {
+                            if (candidate.startTime <= now) {
-                                        mCallback.handleJobException(name, e);
+                                job = candidate;
-                                    }
+                                break;
-                                    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());
-                                }
                             }
-                        });
+                        }
-                    } catch (Exception e) {
+                        if (job != null) execute(job);
-                        LOG.i(name.toUpperCase(), "- Finished.", e);
+                    } else {
-                        if (dispatchExceptions) mCallback.handleJobException(name, e);
+                        // Do nothing, job will be picked in executed().
-                        source.trySetException(e);
                     }
                 }
-            });
+            }
-            mJobs.addLast(new Token(name, source.getTask()));
+        });
-        }
-        return source.getTask();
     }
 
-    public void scheduleDelayed(@NonNull final String name,
+    @GuardedBy("mJobsLock")
-                                long minDelay,
+    private <T> void execute(@NonNull final Job<T> job) {
-                                @NonNull final Runnable runnable) {
+        if (mJobRunning) {
-        Runnable wrapper = new Runnable() {
+            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);
+                try {
-                synchronized (mLock) {
+                    LOG.i(job.name.toUpperCase(), "- Executing.");
-                    if (mDelayedJobs.containsValue(this)) {
+                    Task<T> task = job.scheduler.call();
-                        mDelayedJobs.remove(name);
+                    onComplete(task, worker, new OnCompleteListener<T>() {
+                        @Override
+                        public void onComplete(@NonNull Task<T> 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 <T> void executed(Job<T> 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) {
+        trim(name, 0);
-            if (mDelayedJobs.get(name) != null) {
+    }
-                //noinspection ConstantConditions
+
-                mCallback.getJobWorker(name).remove(mDelayedJobs.get(name));
+    public void trim(@NonNull String name, int allowed) {
-                mDelayedJobs.remove(name);
+        synchronized (mJobsLock) {
+            List<Job<?>> 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) {
+        synchronized (mJobsLock) {
-            List<String> all = new ArrayList<>();
+            Set<String> all = new HashSet<>();
-            //noinspection CollectionAddAllCanBeReplacedWithConstructor
+            for (Job<?> job : mJobs) {
-            all.addAll(mDelayedJobs.keySet());
+                all.add(job.name);
-            for (Token token : mJobs) {
-                all.add(token.name);
             }
             for (String job : all) {
                 remove(job);
@@ -177,17 +231,9 @@ public class CameraOrchestrator {
         }
     }
 
-    private void ensureToken() {
+    private static <T> void onComplete(@NonNull final Task<T> task,
-        synchronized (mLock) {
+                                       @NonNull WorkerHandler handler,
-            if (mJobs.isEmpty()) {
+                                       @NonNull final OnCompleteListener<T> listener) {
-                mJobs.add(new Token("BASE", Tasks.forResult(null)));
-            }
-        }
-    }
-
-    private static <T> void applyCompletionListener(@NonNull final Task<T> task,
-                                                    @NonNull WorkerHandler handler,
-                                                    @NonNull final OnCompleteListener<T> listener) {
         if (task.isComplete()) {
             handler.run(new Runnable() {
                 @Override

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) {
+        synchronized (mJobsLock) {
-            for (Token token : mJobs) {
+            for (Job<?> job : mJobs) {
-                if ((token.name.contains(" >> ") || token.name.contains(" << "))
+                if ((job.name.contains(" >> ") || job.name.contains(" << "))
-                        && !token.task.isComplete()) {
+                        && !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)) {