Merge pull request #231 from JLLeitschuh/fix/imageConverterWrongThread

Fixes ImageConverter running in the wrong thread

Author: JLLeitschuh

ui/src/main/java/edu/wpi/grip/ui/preview/BlobsSocketPreviewView.java

@@ -87,10 +87,10 @@ private void convertImage() {
                 }
             }
 
-            final Image image = this.imageConverter.convert(input);
+            final Mat inputToConvert = input;
             final int numBlobs = blobsReport.getBlobs().size();
-
             platform.runAsSoonAsPossible(() -> {
+                final Image image = this.imageConverter.convert(inputToConvert);
                 this.imageView.setImage(image);
                 this.infoLabel.setText("Found " + numBlobs + " blobs");
             });

ui/src/main/java/edu/wpi/grip/ui/preview/ContoursSocketPreviewView.java

@@ -84,9 +84,10 @@ private void render() {
                 }
             }
 
-            final Image image = this.imageConverter.convert(tmp);
             final long finalNumContours = numContours;
+            final Mat convertInput = tmp;
             platform.runAsSoonAsPossible(() -> {
+                final Image image = this.imageConverter.convert(convertInput);
                 this.imageView.setImage(image);
                 this.infoLabel.setText("Found " + finalNumContours + " contours");
             });

ui/src/main/java/edu/wpi/grip/ui/preview/ImageSocketPreviewView.java

@@ -44,8 +44,8 @@ public void onSocketChanged(SocketChangedEvent event) {
     private void convertImage() {
         synchronized (this) {
             this.getSocket().getValue().ifPresent(mat -> {
-                final Image image = this.imageConverter.convert(mat);
                 platform.runAsSoonAsPossible(() -> {
+                    final Image image = this.imageConverter.convert(mat);
                     this.imageView.setImage(image);
                 });
 

ui/src/main/java/edu/wpi/grip/ui/preview/LinesSocketPreviewView.java

@@ -94,11 +94,10 @@ private void convertImage() {
                     circle(input, endPoint, 2, Scalar.WHITE, 2, LINE_8, 0);
                 }
             }
-
-            final Image image = this.imageConverter.convert(input);
+            final Mat convertInput = input;
             final int numLines = lines.size();
-
             platform.runAsSoonAsPossible(() -> {
+                final Image image = this.imageConverter.convert(convertInput);
                 this.imageView.setImage(image);
                 this.infoLabel.setText("Found " + numLines + " lines");
             });

ui/src/main/java/edu/wpi/grip/ui/util/ImageConverter.java

@@ -1,6 +1,7 @@
 package edu.wpi.grip.ui.util;
 
 import com.google.common.primitives.UnsignedBytes;
+import javafx.application.Platform;
 import javafx.scene.image.Image;
 import javafx.scene.image.PixelFormat;
 import javafx.scene.image.WritableImage;
@@ -31,6 +32,16 @@ public final class ImageConverter {
      * @return A JavaFX image, or null for empty
      */
     public Image convert(Mat mat) {
+        /*
+         * IMPORTANT!
+         * The {@link ImageConverter#image} is a component that may be actively part of the UI
+         * If we are changing it while it is being rendered by the UI thread this could cause
+         * a problem in the UI thread.
+         */
+        if(!Platform.isFxApplicationThread()) {
+            throw new IllegalStateException("This modifies an FX object. This must be run in the UI Thread");
+        }
+
         final int width = mat.cols();
         final int height = mat.rows();
         final int channels = mat.channels();

ui/src/test/java/edu/wpi/grip/ui/util/ImageConverterTest.java

@@ -3,10 +3,13 @@
 import edu.wpi.grip.core.util.ImageLoadingUtility;
 import edu.wpi.grip.util.Files;
 import edu.wpi.grip.util.ImageWithData;
+import javafx.scene.Scene;
 import javafx.scene.image.Image;
+import javafx.scene.layout.Pane;
+import javafx.stage.Stage;
 import org.bytedeco.javacpp.opencv_core.Mat;
-import org.junit.Before;
 import org.junit.Test;
+import org.testfx.framework.junit.ApplicationTest;
 
 import java.net.URLDecoder;
 import java.nio.file.Paths;
@@ -16,24 +19,34 @@
 import static org.junit.Assert.assertEquals;
 
 
-public class ImageConverterTest {
+public class ImageConverterTest extends ApplicationTest {
     private static final ImageWithData
             gompeiImage = Files.gompeiJpegFile,
             imageFile = Files.imageFile;
 
     private ImageConverter converter;
 
-    @Before
-    public void setUp() {
+    @Override
+    public void start(Stage stage) {
         converter = new ImageConverter();
+        stage.setScene(new Scene(new Pane()));
+        stage.show();
     }
 
     @Test
     public void testConvertImage() throws Exception {
         Mat mat = new Mat();
         ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(gompeiImage.file.toURI()).toString()), mat);
-        Image javaFXImage = converter.convert(mat);
-        assertSameImage(gompeiImage, javaFXImage);
+        interact(() -> {
+            Image javaFXImage = converter.convert(mat);
+            assertSameImage(gompeiImage, javaFXImage);
+        });
+
+    }
+
+    @Test(expected = IllegalStateException.class)
+    public void testConvertInWrongThreadThrowsIllegalState() {
+        converter.convert(new Mat());
     }
 
     @Test
@@ -42,9 +55,11 @@ public void testConvertImageSwitch() throws Exception {
         Mat imageMat = new Mat();
         ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(gompeiImage.file.toURI()).toString()), gompeiMat);
         ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(imageFile.file.toURI()).toString()), imageMat);
-        converter.convert(gompeiMat);
-        Image javaFXImage = converter.convert(imageMat);
-        assertSameImage(imageFile, javaFXImage);
+        interact(() -> {
+            converter.convert(gompeiMat);
+            Image javaFXImage = converter.convert(imageMat);
+            assertSameImage(imageFile, javaFXImage);
+        });
     }
 
     @Test
@@ -54,8 +69,10 @@ public void testConvertSingleChanelImage() throws Exception {
         final Mat desaturatedMat = new Mat();
         cvtColor(gompeiMat, desaturatedMat, COLOR_BGR2GRAY);
 
-        Image javaFXImage = converter.convert(desaturatedMat);
-        assertSameImage(gompeiImage, javaFXImage);
+        interact(() -> {
+            Image javaFXImage = converter.convert(desaturatedMat);
+            assertSameImage(gompeiImage, javaFXImage);
+        });
     }
 
     private void assertSameImage(ImageWithData imageWithData, Image javaFXImage) {