This is an automated email from the ASF dual-hosted git repository.
desruisseaux pushed a commit to branch geoapi-4.0
in repository https://gitbox.apache.org/repos/asf/sis.git
commit 7bf11d171b0ac83212aa966d0c147a231a993eab
Author: Martin Desruisseaux <martin.desruisseaux@geomatys.com>
AuthorDate: Wed Aug 5 12:12:37 2020 +0200
Add a safety check against integer overflow, trace information for debugging and link
to JIRA task.
---
.../apache/sis/gui/coverage/CoverageCanvas.java | 57 +++++++++++++++++++---
.../sis/gui/coverage/ImagePropertyExplorer.java | 2 +-
.../org/apache/sis/gui/coverage/RenderingData.java | 11 +++--
.../sis/internal/coverage/CompoundTransform.java | 3 ++
4 files changed, 62 insertions(+), 11 deletions(-)
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
index 42ce921..1cd0ad1 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/CoverageCanvas.java
@@ -63,6 +63,7 @@ import org.apache.sis.internal.system.Modules;
import org.apache.sis.util.logging.Logging;
import org.apache.sis.measure.Units;
import org.apache.sis.storage.Resource;
+import org.apache.sis.util.Debug;
/**
@@ -79,6 +80,21 @@ import org.apache.sis.storage.Resource;
@DefaultProperty("coverage")
public class CoverageCanvas extends MapCanvasAWT {
/**
+ * An arbitrary safety margin (in number of pixels) for avoiding integer overflow situation.
+ * This margin shall be larger than any reasonable widget width or height, and much smaller
+ * than {@link Integer#MAX_VALUE}.
+ */
+ private static final int OVERFLOW_SAFETY_MARGIN = 10_000_000;
+
+ /**
+ * Whether to print debug information to {@link System#out}. We use {@code stdout} instead
than logging
+ * because the log messages are intercepted and rerouted to the "logging" tab in the
explorer widget.
+ * This field should always be {@code false} except during debugging.
+ */
+ @Debug
+ private static final boolean TRACE = false;
+
+ /**
* The data shown in this canvas. Note that setting this property to a non-null value
may not
* modify the canvas content immediately. Instead, a background process will request
the tiles.
*
@@ -397,6 +413,7 @@ public class CoverageCanvas extends MapCanvasAWT {
*
* <p>All arguments can be {@code null} for clearing the canvas.</p>
*/
+ @SuppressWarnings("UseOfSystemOutOrSystemErr")
private void setRawImage(final RenderedImage image, final GridGeometry domain, final
List<SampleDimension> ranges) {
resampledImage = null;
derivedImages.clear();
@@ -407,6 +424,7 @@ public class CoverageCanvas extends MapCanvasAWT {
}
setObjectiveBounds(bounds);
requestRepaint(); // Cause `Worker` class to be executed.
+ if (TRACE) System.out.format("setRawImage: %s%n", image);
}
/**
@@ -455,13 +473,15 @@ public class CoverageCanvas extends MapCanvasAWT {
/**
* Value of {@link CoverageCanvas#getObjectiveToDisplay()} at the time this worker
has been initialized.
* This is the conversion from {@link #objectiveCRS} to the canvas display CRS.
- * Can be thought as a conversion from "real world" units to pixel units.
+ * Can be thought as a conversion from "real world" units to pixel units
+ * and depends on the zoom and translation events that happened before rendering.
*/
private final LinearTransform objectiveToDisplay;
/**
* Value of {@link CoverageCanvas#getDisplayBounds()} at the time this worker has
been initialized.
* This is the size and location of the display device, in pixel units.
+ * This value is usually constant when the widget is not resized.
*/
private final Envelope2D displayBounds;
@@ -486,6 +506,11 @@ public class CoverageCanvas extends MapCanvasAWT {
/**
* Conversion from {@link #prefetchedImage} pixel coordinates to display coordinates.
+ * This transform usually contains only a translation, because we do not recompute
a new {@link #prefetchedImage}
+ * when the only change is a translation. But this transform may also contain a rotation
or scale factor during
+ * a short time if the rendering happens while {@link #prefetchedImage} is in need
to be recomputed.
+ *
+ * @see RenderingData#displayToObjective
*/
private AffineTransform resampledToDisplay;
@@ -531,7 +556,7 @@ public class CoverageCanvas extends MapCanvasAWT {
* to skip some steps for example if the required source image is already resampled.
*/
@Override
- @SuppressWarnings("PointlessBitwiseExpression")
+ @SuppressWarnings({"PointlessBitwiseExpression", "UseOfSystemOutOrSystemErr"})
protected void render() throws TransformException {
final Long id = LogHandler.loadingStart(originator);
try {
@@ -540,18 +565,33 @@ public class CoverageCanvas extends MapCanvasAWT {
* has been rendered, ignoring translations. Translations do not require
new resampling operations
* because we can manage translations by changing `RenderedImage` coordinates.
*/
- boolean resamplingChanged = (resampledImage == null);
- if (!resamplingChanged) {
+ boolean isValid = (resampledImage != null);
+ if (isValid) {
resampledToDisplay = data.getTransform(objectiveToDisplay);
- resamplingChanged = (resampledToDisplay.getType() &
- ~(AffineTransform.TYPE_IDENTITY | AffineTransform.TYPE_TRANSLATION))
!= 0;
+ isValid = (resampledToDisplay.getType() &
+ ~(AffineTransform.TYPE_IDENTITY | AffineTransform.TYPE_TRANSLATION))
== 0;
+ /*
+ * If user pans the image close to integer range limit, create a new
resampled image shifted to
+ * new location (i.e. force `resampleAndConvert(…)` to be invoked again).
The intent is to move
+ * away from integer overflow situation.
+ */
+ if (isValid) {
+ isValid = Math.max(Math.abs(resampledToDisplay.getTranslateX()),
+ Math.abs(resampledToDisplay.getTranslateY()))
+ < Integer.MAX_VALUE - OVERFLOW_SAFETY_MARGIN;
+ if (TRACE && !isValid) {
+ System.out.println("New resample for avoiding overflow caused
by translation.");
+ }
+ }
}
- if (resamplingChanged) {
+ if (!isValid) {
if (recoloredImage == null) {
recoloredImage = data.recolor();
+ if (TRACE) System.out.format("Recolor by application of %s.%n", data.selectedDerivative.name());
}
resampledImage = data.resampleAndConvert(recoloredImage, objectiveCRS,
objectiveToDisplay);
resampledToDisplay = data.getTransform(objectiveToDisplay);
+ if (TRACE) System.out.format("Resampled image: %s%n", resampledImage);
}
prefetchedImage = data.prefetch(resampledImage, resampledToDisplay, displayBounds);
} finally {
@@ -582,6 +622,7 @@ public class CoverageCanvas extends MapCanvasAWT {
* Invoked after a paint event for caching rendering data.
* If the resampled image changed, all previously cached images are discarded.
*/
+ @SuppressWarnings("UseOfSystemOutOrSystemErr")
private void cacheRenderingData(final Worker worker) {
data = worker.data;
derivedImages.put(data.selectedDerivative, worker.recoloredImage);
@@ -592,6 +633,8 @@ public class CoverageCanvas extends MapCanvasAWT {
*/
if (imageProperty != null) {
imageProperty.setImage(resampledImage, worker.getVisibleImageBounds());
+ if (TRACE) System.out.format("Update image property view with visible area %s.%n",
+ imageProperty.getVisibleImageBounds(resampledImage));
}
if (statusBar != null) {
final Object value = resampledImage.getProperty(PlanarImage.POSITIONAL_ACCURACY_KEY);
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/ImagePropertyExplorer.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/ImagePropertyExplorer.java
index 899705b..327e7af 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/ImagePropertyExplorer.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/ImagePropertyExplorer.java
@@ -555,7 +555,7 @@ public class ImagePropertyExplorer extends Widget {
* Returns the pixel coordinates of the region shown on screen,
* or {@code null} if none or does not apply to the currently selected image.
*/
- private Rectangle getVisibleImageBounds(final RenderedImage selected) {
+ final Rectangle getVisibleImageBounds(final RenderedImage selected) {
return Boolean.TRUE.equals(imageUseBoundsCS.get(selected)) ? visibleImageBounds :
null;
}
diff --git a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/RenderingData.java
b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/RenderingData.java
index 2d136f1..ee1e100 100644
--- a/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/RenderingData.java
+++ b/application/sis-javafx/src/main/java/org/apache/sis/gui/coverage/RenderingData.java
@@ -152,17 +152,23 @@ final class RenderingData implements Cloneable {
* active at the time resampled images have been computed. The concatenation of this
transform with the actual
* "objective to display" transform at the time the rendered image is drawn should be
a translation.
* May be {@code null} if not yet computed.
+ *
+ * @see #getTransform(LinearTransform)
*/
private AffineTransform displayToObjective;
/**
* Key of the currently selected alternative in {@link CoverageCanvas#derivedImages}
map.
+ *
+ * @see #recolor()
*/
Stretching selectedDerivative;
/**
* Statistics on pixel values of current {@link #data}, or {@code null} if none or not
yet computed.
* There is one {@link Statistics} instance per band.
+ *
+ * @see #recolor()
*/
private Statistics[] statistics;
@@ -299,9 +305,8 @@ final class RenderingData implements Cloneable {
* will be very large, potentially larger than 32 bit integer capacity (calculation
done below clamps
* the result to 32 bit integer range). This is okay since only visible tiles will
be created.
*
- * TODO: if user pans the image close to integer range limit, we should create a
new resampled image
- * shifted to new location (i.e. clear `CoverageCanvas.resampledImage` for
forcing this method
- * to be invoked again). The intent is to move away from integer overflow situation.
+ * NOTE: if user pans image close to integer range limit, a new resampled image will
need to be computed
+ * for shifting away from integer overflow risk situation. This check is done
by the caller.
*/
final LinearTransform inverse = objectiveToDisplay.inverse();
displayToObjective = AffineTransforms2D.castOrCopy(inverse);
diff --git a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/CompoundTransform.java
b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/CompoundTransform.java
index bb989a1..3037ce5 100644
--- a/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/CompoundTransform.java
+++ b/core/sis-feature/src/main/java/org/apache/sis/internal/coverage/CompoundTransform.java
@@ -33,6 +33,9 @@ import org.apache.sis.util.ArraysExt;
/**
* A transform composed of an arbitrary amount of juxtaposed transforms.
*
+ * This implementation is sufficient for {@code sis-feature} purposes, but incomplete for
{@code sis-referencing}
+ * purposes. See <a href="https://issues.apache.org/jira/browse/SIS-498">SIS-498</a>.
+ *
* @author Martin Desruisseaux (Geomatys)
* @version 1.1
* @since 1.1
|