Skip to content

Commit 6878138

Browse files
committed
Fixes ImageConverter running in the wrong thread
If an image changed dimention in the preview pane it's UI counterpart could be changed in the wrong thread. This could cause random ArrayOutOfBounds Exceptions without any context as to what was causing the problem. This forces the ImageConverter.convert method to be run in the UI thread or it will throw an exception. Possibly related: #225
1 parent 54daac9 commit 6878138

6 files changed

Lines changed: 46 additions & 18 deletions

File tree

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,10 +87,10 @@ private void convertImage() {
8787
}
8888
}
8989

90-
final Image image = this.imageConverter.convert(input);
90+
final Mat inputToConvert = input;
9191
final int numBlobs = blobsReport.getBlobs().size();
92-
9392
platform.runAsSoonAsPossible(() -> {
93+
final Image image = this.imageConverter.convert(inputToConvert);
9494
this.imageView.setImage(image);
9595
this.infoLabel.setText("Found " + numBlobs + " blobs");
9696
});

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,9 +84,10 @@ private void render() {
8484
}
8585
}
8686

87-
final Image image = this.imageConverter.convert(tmp);
8887
final long finalNumContours = numContours;
88+
final Mat convertInput = tmp;
8989
platform.runAsSoonAsPossible(() -> {
90+
final Image image = this.imageConverter.convert(convertInput);
9091
this.imageView.setImage(image);
9192
this.infoLabel.setText("Found " + finalNumContours + " contours");
9293
});

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ public void onSocketChanged(SocketChangedEvent event) {
4444
private void convertImage() {
4545
synchronized (this) {
4646
this.getSocket().getValue().ifPresent(mat -> {
47-
final Image image = this.imageConverter.convert(mat);
4847
platform.runAsSoonAsPossible(() -> {
48+
final Image image = this.imageConverter.convert(mat);
4949
this.imageView.setImage(image);
5050
});
5151

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -94,11 +94,10 @@ private void convertImage() {
9494
circle(input, endPoint, 2, Scalar.WHITE, 2, LINE_8, 0);
9595
}
9696
}
97-
98-
final Image image = this.imageConverter.convert(input);
97+
final Mat convertInput = input;
9998
final int numLines = lines.size();
100-
10199
platform.runAsSoonAsPossible(() -> {
100+
final Image image = this.imageConverter.convert(convertInput);
102101
this.imageView.setImage(image);
103102
this.infoLabel.setText("Found " + numLines + " lines");
104103
});

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
package edu.wpi.grip.ui.util;
22

33
import com.google.common.primitives.UnsignedBytes;
4+
import javafx.application.Platform;
45
import javafx.scene.image.Image;
56
import javafx.scene.image.PixelFormat;
67
import javafx.scene.image.WritableImage;
@@ -31,6 +32,16 @@ public final class ImageConverter {
3132
* @return A JavaFX image, or null for empty
3233
*/
3334
public Image convert(Mat mat) {
35+
/*
36+
* IMPORTANT!
37+
* The {@link ImageConverter#image} is a component that may be actively part of the UI
38+
* If we are changing it while it is being rendered by the UI thread this could cause
39+
* a problem in the UI thread.
40+
*/
41+
if(!Platform.isFxApplicationThread()) {
42+
throw new IllegalStateException("This modifies an FX object. This must be run in the UI Thread");
43+
}
44+
3445
final int width = mat.cols();
3546
final int height = mat.rows();
3647
final int channels = mat.channels();

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

Lines changed: 28 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,13 @@
33
import edu.wpi.grip.core.util.ImageLoadingUtility;
44
import edu.wpi.grip.util.Files;
55
import edu.wpi.grip.util.ImageWithData;
6+
import javafx.scene.Scene;
67
import javafx.scene.image.Image;
8+
import javafx.scene.layout.Pane;
9+
import javafx.stage.Stage;
710
import org.bytedeco.javacpp.opencv_core.Mat;
8-
import org.junit.Before;
911
import org.junit.Test;
12+
import org.testfx.framework.junit.ApplicationTest;
1013

1114
import java.net.URLDecoder;
1215
import java.nio.file.Paths;
@@ -16,24 +19,34 @@
1619
import static org.junit.Assert.assertEquals;
1720

1821

19-
public class ImageConverterTest {
22+
public class ImageConverterTest extends ApplicationTest {
2023
private static final ImageWithData
2124
gompeiImage = Files.gompeiJpegFile,
2225
imageFile = Files.imageFile;
2326

2427
private ImageConverter converter;
2528

26-
@Before
27-
public void setUp() {
29+
@Override
30+
public void start(Stage stage) {
2831
converter = new ImageConverter();
32+
stage.setScene(new Scene(new Pane()));
33+
stage.show();
2934
}
3035

3136
@Test
3237
public void testConvertImage() throws Exception {
3338
Mat mat = new Mat();
3439
ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(gompeiImage.file.toURI()).toString()), mat);
35-
Image javaFXImage = converter.convert(mat);
36-
assertSameImage(gompeiImage, javaFXImage);
40+
interact(() -> {
41+
Image javaFXImage = converter.convert(mat);
42+
assertSameImage(gompeiImage, javaFXImage);
43+
});
44+
45+
}
46+
47+
@Test(expected = IllegalStateException.class)
48+
public void testConvertInWrongThreadThrowsIllegalState() {
49+
converter.convert(new Mat());
3750
}
3851

3952
@Test
@@ -42,9 +55,11 @@ public void testConvertImageSwitch() throws Exception {
4255
Mat imageMat = new Mat();
4356
ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(gompeiImage.file.toURI()).toString()), gompeiMat);
4457
ImageLoadingUtility.loadImage(URLDecoder.decode(Paths.get(imageFile.file.toURI()).toString()), imageMat);
45-
converter.convert(gompeiMat);
46-
Image javaFXImage = converter.convert(imageMat);
47-
assertSameImage(imageFile, javaFXImage);
58+
interact(() -> {
59+
converter.convert(gompeiMat);
60+
Image javaFXImage = converter.convert(imageMat);
61+
assertSameImage(imageFile, javaFXImage);
62+
});
4863
}
4964

5065
@Test
@@ -54,8 +69,10 @@ public void testConvertSingleChanelImage() throws Exception {
5469
final Mat desaturatedMat = new Mat();
5570
cvtColor(gompeiMat, desaturatedMat, COLOR_BGR2GRAY);
5671

57-
Image javaFXImage = converter.convert(desaturatedMat);
58-
assertSameImage(gompeiImage, javaFXImage);
72+
interact(() -> {
73+
Image javaFXImage = converter.convert(desaturatedMat);
74+
assertSameImage(gompeiImage, javaFXImage);
75+
});
5976
}
6077

6178
private void assertSameImage(ImageWithData imageWithData, Image javaFXImage) {

0 commit comments

Comments
 (0)