sis-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From desruisse...@apache.org
Subject [sis] 01/02: Add a safety check against integer overflow, trace information for debugging and link to JIRA task.
Date Wed, 05 Aug 2020 20:32:40 GMT
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


Mime
View raw message