ImageDetections: Wait some time prior to fetching

This reduces network load and should speed up other threads (since we aren't writing to disk or performing other calculations). Signed-off-by: Taylor Smock <[email protected]>
JOSM · Sep 15, 2021 · 21fc175 · 21fc175
1 parent eb57b88
commit 21fc175
Show file tree

Hide file tree

Showing 7 changed files with 137 additions and 91 deletions.
diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/MapillaryMainDialog.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/MapillaryMainDialog.java
@@ -5,16 +5,6 @@
 import static org.openstreetmap.josm.tools.I18n.tr;
 import static org.openstreetmap.josm.tools.I18n.trc;
 
-import javax.annotation.Nonnull;
-import javax.imageio.ImageIO;
-import javax.swing.AbstractButton;
-import javax.swing.Action;
-import javax.swing.BoxLayout;
-import javax.swing.JButton;
-import javax.swing.JComponent;
-import javax.swing.JPanel;
-import javax.swing.JToggleButton;
-import javax.swing.SwingUtilities;
 import java.awt.BorderLayout;
 import java.awt.Color;
 import java.awt.Component;
@@ -40,6 +30,17 @@
 import java.util.function.Supplier;
 import java.util.stream.Stream;
 
+import javax.annotation.Nonnull;
+import javax.imageio.ImageIO;
+import javax.swing.AbstractButton;
+import javax.swing.Action;
+import javax.swing.BoxLayout;
+import javax.swing.JButton;
+import javax.swing.JComponent;
+import javax.swing.JPanel;
+import javax.swing.JToggleButton;
+import javax.swing.SwingUtilities;
+
 import org.openstreetmap.josm.actions.JosmAction;
 import org.openstreetmap.josm.data.cache.BufferedImageCacheEntry;
 import org.openstreetmap.josm.data.cache.CacheEntry;
@@ -98,7 +99,6 @@ public final class MapillaryMainDialog extends ToggleDialog
     private final PlayAction playAction = new PlayAction();
     private final PauseAction pauseAction = new PauseAction();
     private final StopAction stopAction = new StopAction();
-    private ImageDetection.ImageDetectionForkJoinTask futureDetections;
 
     /**
      * Buttons mode.
@@ -491,19 +491,13 @@ public synchronized void updateImage() {
                 };
 
                 ForkJoinPool pool = MapillaryUtils.getForkJoinPool();
-                if (this.futureDetections != null && !this.futureDetections.isDone()
-                    && MapillaryImageUtils.getKey(image) != this.futureDetections.key) {
-                    this.futureDetections.cancel(false);
-                }
-                this.futureDetections = ImageDetection.getDetections(MapillaryImageUtils.getKey(this.image),
-                    (key, detections) -> {
-                        INode tImage = this.image;
-                        if (tImage != null && key.equals(MapillaryImageUtils.getKey(tImage))) {
-                            this.updateDetections(imageFullCache, tImage, detections);
-                        }
-                    });
-                List<ImageDetection<?>> detections = ImageDetection.getDetections(MapillaryImageUtils.getKey(image),
-                    false);
+                ImageDetection.getDetectionsLaterOptional(MapillaryImageUtils.getKey(this.image), (key, detections) -> {
+                    INode tImage = this.image;
+                    if (tImage != null && key.equals(MapillaryImageUtils.getKey(tImage))) {
+                        this.updateDetections(imageFullCache, tImage, detections);
+                    }
+                }, 5000);
+                List<ImageDetection<?>> detections = ImageDetection.getDetections(MapillaryImageUtils.getKey(image));
                 if (imageFullCache.get() != null) {
                     setDisplayImage(() -> getImage.apply(imageFullCache.get()), detections,
                         MapillaryImageUtils.IS_PANORAMIC.test(currentImage));
@@ -807,7 +801,7 @@ private void realLoadingFinished(final CacheEntry data) {
                     Logging.error(e);
                 }
                 return null;
-            }, ImageDetection.getDetections(MapillaryImageUtils.getKey(mai), false),
+            }, ImageDetection.getDetections(MapillaryImageUtils.getKey(mai)),
                 MapillaryImageUtils.IS_PANORAMIC.test(mai));
             if (mai != null) {
                 ImageDetection.getDetections(MapillaryImageUtils.getKey(mai),

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/dialog/MapillaryFilterDialog.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/dialog/MapillaryFilterDialog.java
@@ -1,6 +1,44 @@
 // License: GPL. For details, see LICENSE file.
 package org.openstreetmap.josm.plugins.mapillary.gui.dialog;
 
+import static org.openstreetmap.josm.tools.I18n.tr;
+
+import java.awt.Component;
+import java.awt.Dimension;
+import java.awt.FlowLayout;
+import java.awt.GridBagConstraints;
+import java.awt.GridBagLayout;
+import java.awt.event.ActionEvent;
+import java.awt.event.ItemEvent;
+import java.awt.event.KeyEvent;
+import java.time.Instant;
+import java.time.LocalDate;
+import java.time.LocalDateTime;
+import java.time.ZoneOffset;
+import java.time.ZonedDateTime;
+import java.time.temporal.ChronoUnit;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.function.Consumer;
+import java.util.function.Predicate;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+import java.util.stream.LongStream;
+import java.util.stream.Stream;
+
+import javax.swing.AbstractAction;
+import javax.swing.JButton;
+import javax.swing.JCheckBox;
+import javax.swing.JComboBox;
+import javax.swing.JComponent;
+import javax.swing.JDialog;
+import javax.swing.JLabel;
+import javax.swing.JOptionPane;
+import javax.swing.JPanel;
+import javax.swing.JSeparator;
+import javax.swing.JSpinner;
+import javax.swing.SpinnerNumberModel;
+
 import org.openstreetmap.josm.actions.ExpertToggleAction;
 import org.openstreetmap.josm.actions.ExpertToggleAction.ExpertModeChangeListener;
 import org.openstreetmap.josm.data.imagery.vectortile.mapbox.MVTTile;
@@ -31,43 +69,6 @@
 import org.openstreetmap.josm.tools.ListenerList;
 import org.openstreetmap.josm.tools.Shortcut;
 
-import javax.swing.AbstractAction;
-import javax.swing.JButton;
-import javax.swing.JCheckBox;
-import javax.swing.JComboBox;
-import javax.swing.JComponent;
-import javax.swing.JDialog;
-import javax.swing.JLabel;
-import javax.swing.JOptionPane;
-import javax.swing.JPanel;
-import javax.swing.JSeparator;
-import javax.swing.JSpinner;
-import javax.swing.SpinnerNumberModel;
-import java.awt.Component;
-import java.awt.Dimension;
-import java.awt.FlowLayout;
-import java.awt.GridBagConstraints;
-import java.awt.GridBagLayout;
-import java.awt.event.ActionEvent;
-import java.awt.event.ItemEvent;
-import java.awt.event.KeyEvent;
-import java.time.Instant;
-import java.time.LocalDate;
-import java.time.LocalDateTime;
-import java.time.ZoneOffset;
-import java.time.ZonedDateTime;
-import java.time.temporal.ChronoUnit;
-import java.util.Arrays;
-import java.util.Collection;
-import java.util.function.Consumer;
-import java.util.function.Predicate;
-import java.util.regex.Pattern;
-import java.util.stream.Collectors;
-import java.util.stream.LongStream;
-import java.util.stream.Stream;
-
-import static org.openstreetmap.josm.tools.I18n.tr;
-
 /**
  * ToggleDialog that lets you filter the images that are being shown.
  *
@@ -480,9 +481,8 @@ public boolean test(INode img) {
                 if (!this.downloadedIsSelected) {
                     return true;
                 }
-                if (this.onlySignsIsSelected
-                    && (ImageDetection.getDetections(MapillaryImageUtils.getKey(img), false).isEmpty()
-                        || !checkSigns(ImageDetection.getDetections(MapillaryImageUtils.getKey(img), false)))) {
+                if (this.onlySignsIsSelected && (ImageDetection.getDetections(MapillaryImageUtils.getKey(img)).isEmpty()
+                    || !checkSigns(ImageDetection.getDetections(MapillaryImageUtils.getKey(img))))) {
                     return true;
                 }
                 if (!OrganizationRecord.NULL_RECORD.equals(this.organization)

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/imageinfo/ImageInfoPanel.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/imageinfo/ImageInfoPanel.java
@@ -246,14 +246,15 @@ private void selectedImageChanged(@Nullable INode oldImage, @Nullable INode newI
             imgKeyValue.setText(newImageKey);
 
             // First, set the detections number that we currently have
-            List<ImageDetection<?>> detections = ImageDetection.getDetections(MapillaryImageUtils.getKey(newImage),
-                false);
+            List<ImageDetection<?>> detections = ImageDetection.getDetections(MapillaryImageUtils.getKey(newImage));
             numDetectionsLabel
                 .setText(tr("{0} detections", detections.stream().filter(ImageDetection::isTrafficSign).count()));
             // Then, set the detections number that we get
-            ImageDetection.getDetections(MapillaryImageUtils.getKey(newImage),
-                (key, detectionList) -> GuiHelper.runInEDT(() -> numDetectionsLabel.setText(
-                    tr("{0} detections", detectionList.stream().filter(ImageDetection::isTrafficSign).count()))));
+            ImageDetection
+                .getDetectionsLaterOptional(MapillaryImageUtils.getKey(newImage),
+                    (key, detectionList) -> GuiHelper.runInEDT(() -> numDetectionsLabel.setText(
+                        tr("{0} detections", detectionList.stream().filter(ImageDetection::isTrafficSign).count()))),
+                    1000);
             copyImgKeyAction.setContents(new StringSelection(newImageKey));
             addMapillaryTagAction.setTag(new Tag("mapillary", newImageKey));
         } else {

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/layer/PointObjectLayer.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/gui/layer/PointObjectLayer.java
@@ -607,9 +607,9 @@ private void selectedImageChanged(
         Collection<VectorPrimitive> currentSelection = data.getSelected();
         final VectorNode newImage = event.getSelection().stream().filter(VectorNode.class::isInstance)
             .map(VectorNode.class::cast).findFirst().orElse(null);
-        if (newImage != null && !ImageDetection.getDetections(newImage.getId(), false).isEmpty()) {
+        if (newImage != null && !ImageDetection.getDetections(newImage.getId()).isEmpty()) {
             final long key = newImage.getId();
-            final Collection<IPrimitive> nodes = ImageDetection.getDetections(key, false).parallelStream()
+            final Collection<IPrimitive> nodes = ImageDetection.getDetections(key).parallelStream()
                 .map(detection -> data.getPrimitiveById(detection.getKey(), OsmPrimitiveType.NODE))
                 .collect(Collectors.toList());
 

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/model/ImageDetection.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/model/ImageDetection.java
@@ -8,18 +8,25 @@
 import java.net.URL;
 import java.nio.charset.StandardCharsets;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
+import java.util.EnumSet;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
+import java.util.Timer;
+import java.util.TimerTask;
 import java.util.concurrent.ForkJoinPool;
 import java.util.concurrent.ForkJoinTask;
 import java.util.function.BiConsumer;
 
+import javax.annotation.Nullable;
 import javax.json.Json;
 import javax.json.JsonObject;
 import javax.json.JsonReader;
 
+import org.apache.commons.jcs3.access.CacheAccess;
 import org.openstreetmap.josm.data.cache.JCSCacheManager;
 import org.openstreetmap.josm.data.osm.IPrimitive;
 import org.openstreetmap.josm.data.vector.VectorDataSet;
@@ -39,12 +46,20 @@
 import org.openstreetmap.josm.tools.Logging;
 import org.openstreetmap.josm.tools.Pair;
 
-import org.apache.commons.jcs3.access.CacheAccess;
-
 /**
  * A store for ImageDetection information
  */
 public class ImageDetection<T extends Shape> extends SpecialImageArea<Long, T> {
+
+    public enum Options {
+        /** Wait for the fetch to complete. Implies {@link #FETCH} */
+        WAIT,
+        /** Fetch the detections */
+        FETCH
+    }
+
+    private static final Timer DETECTION_TIMER = new Timer();
+    private static TimerTask currentTask;
     private static final String DETECTIONS = "detections";
 
     /**
@@ -76,22 +91,43 @@ public static ImageDetectionForkJoinTask getDetections(long key,
             .submit(new ImageDetectionForkJoinTask(key, listener));
     }
 
+    /**
+     * Get detections at some later time
+     *
+     * @param key The key to use
+     * @param listener The listener to notify
+     * @param timeout The time to wait prior to getting the detections (milliseconds)
+     */
+    public static void getDetectionsLaterOptional(final long key,
+        final BiConsumer<Long, List<ImageDetection<?>>> listener, long timeout) {
+
+        synchronized (DETECTION_TIMER) {
+            if (currentTask != null) {
+                currentTask.cancel();
+            }
+            currentTask = new ImageDetectionWaitTask(new ImageDetectionForkJoinTask(key, listener));
+            DETECTION_TIMER.schedule(currentTask, timeout);
+        }
+    }
+
     /**
      * Get the detections for an image key
      *
      * @param key The image key
-     * @param wait {@code true} to wait for the detections
+     * @param options The option(s) to use. May be {@code null}
      * @return The image detections
      */
-    public static List<ImageDetection<?>> getDetections(long key, boolean wait) {
-        if (wait) {
+    public static List<ImageDetection<?>> getDetections(final long key, @Nullable final Options... options) {
+        final EnumSet<Options> optionSet = EnumSet.noneOf(Options.class);
+        Optional.ofNullable(options).map(Arrays::asList).ifPresent(optionSet::addAll);
+        if (optionSet.contains(Options.WAIT)) {
             final ImageDetectionForkJoinTask task = new ImageDetectionForkJoinTask(key, null);
             task.fork();
             return task.join();
         } else {
             if (key != 0 && DETECTION_CACHE.get(key) != null) {
                 return Collections.unmodifiableList(new ArrayList<>(DETECTION_CACHE.get(key)));
-            } else if (key != 0) {
+            } else if (key != 0 && optionSet.contains(Options.FETCH)) {
                 final ImageDetectionForkJoinTask task = new ImageDetectionForkJoinTask(key, null);
                 ForkJoinPool.commonPool().execute(task);
             }
@@ -164,8 +200,8 @@ public boolean isTrafficSign() {
     public Color getColor() {
         if (MainApplication.getLayerManager().getLayersOfType(PointObjectLayer.class).parallelStream()
             .map(PointObjectLayer::getData).map(VectorDataSet::getSelected).flatMap(Collection::stream)
-            .mapToLong(IPrimitive::getId).mapToObj(id -> ImageDetection.getDetections(id, false))
-            .flatMap(Collection::stream).filter(Objects::nonNull).anyMatch(id -> id.getKey().equals(this.getKey()))) {
+            .mapToLong(IPrimitive::getId).mapToObj(ImageDetection::getDetections).flatMap(Collection::stream)
+            .filter(Objects::nonNull).anyMatch(id -> id.getKey().equals(this.getKey()))) {
             return isRejected() || Boolean.TRUE.equals(MapillaryProperties.SMART_EDIT.get()) ? Color.RED : Color.CYAN;
         }
         if (this.isTrafficSign())
@@ -209,6 +245,22 @@ public int hashCode() {
         return Objects.hash(super.hashCode(), this.approvalType, this.originalValue, this.rejected, this.value);
     }
 
+    /**
+     * Class for delayed get of image detections
+     */
+    private static class ImageDetectionWaitTask extends TimerTask {
+        private final ImageDetectionForkJoinTask task;
+
+        public ImageDetectionWaitTask(final ImageDetectionForkJoinTask task) {
+            this.task = task;
+        }
+
+        @Override
+        public void run() {
+            MapillaryUtils.getForkJoinPool().execute(this.task);
+        }
+    }
+
     /**
      * A ForkJoinTask that can be cancelled
      */

diff --git a/src/main/java/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryURL.java b/src/main/java/org/openstreetmap/josm/plugins/mapillary/utils/MapillaryURL.java
@@ -17,12 +17,12 @@
 import java.util.stream.LongStream;
 import java.util.stream.Stream;
 
-import org.openstreetmap.josm.data.Bounds;
-import org.openstreetmap.josm.tools.Logging;
-
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 
+import org.openstreetmap.josm.data.Bounds;
+import org.openstreetmap.josm.tools.Logging;
+
 public final class MapillaryURL {
     /**
      * Mapillary v4 API

diff --git a/test/unit/org/openstreetmap/josm/plugins/mapillary/model/ImageDetectionTest.java b/test/unit/org/openstreetmap/josm/plugins/mapillary/model/ImageDetectionTest.java
@@ -9,15 +9,14 @@
 import java.util.List;
 import java.util.stream.Stream;
 
-import org.junit.jupiter.api.extension.RegisterExtension;
-import org.openstreetmap.josm.plugins.mapillary.data.mapillary.ObjectDetections;
-import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryCaches;
-import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryURLWireMock;
-
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.openstreetmap.josm.plugins.mapillary.data.mapillary.ObjectDetections;
+import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryCaches;
+import org.openstreetmap.josm.plugins.mapillary.testutils.annotations.MapillaryURLWireMock;
 import org.openstreetmap.josm.testutils.JOSMTestRules;
 
 @MapillaryURLWireMock
@@ -55,14 +54,14 @@ static Stream<Arguments> getDetectionsArguments() {
     @ParameterizedTest
     @MethodSource("getDetectionsArguments")
     void testGetDetectionsWait(long id, int expectedDetections) {
-        List<ImageDetection<?>> detections = ImageDetection.getDetections(id, true);
+        List<ImageDetection<?>> detections = ImageDetection.getDetections(id, ImageDetection.Options.WAIT);
         assertEquals(expectedDetections, detections.size());
     }
 
     @ParameterizedTest
     @MethodSource("getDetectionsArguments")
-    void testGetDetectionsNoWait(long id, int expectedDetections) {
-        List<ImageDetection<?>> detections = ImageDetection.getDetections(id, false);
+    void testGetDetectionsNoWait(long id) {
+        List<ImageDetection<?>> detections = ImageDetection.getDetections(id, ImageDetection.Options.FETCH);
         assertTrue(detections.isEmpty());
     }
 }