TMI-40: Fixed subsampling offset bug (and removed the slow, stepwise reading + simplified the code, at the cost of higher memory consumption).

imageio/imageio-jpeg/src/main/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReader.java

@@ -447,75 +447,46 @@ public class JPEGImageReader extends ImageReaderBase {
         Rectangle dstRegion = new Rectangle();
         computeRegions(param, origWidth, origHeight, image, srcRegion, dstRegion);
 
-        // We're ready to go
+        // Need to undo the subsampling offset translations, as they are applied again in delegate.readRaster
-        processImageStarted(imageIndex);
+        int gridX = param.getSubsamplingXOffset();
+        int gridY = param.getSubsamplingYOffset();
+        srcRegion.translate(-gridX, -gridY);
+        srcRegion.width += gridX;
+        srcRegion.height += gridY;
 
-        // Unfortunately looping is slower than reading all at once, but
+        // Unfortunately, reading the image in steps, is increasingly slower
-        // that requires 2 x memory or more, so a few steps is an ok compromise I guess
+        // for each iteration, so we'll read all at once.
         try {
-            final int step = Math.max(1024, srcRegion.height / 10); // TODO: Using a multiple of 8 is probably a good idea for JPEG
+            param.setSourceRegion(srcRegion);
-            final int srcMaxY = srcRegion.y + srcRegion.height;
+            Raster raster = delegate.readRaster(imageIndex, param); // non-converted
-            int destY = dstRegion.y;
 
-            for (int y = srcRegion.y; y < srcMaxY; y += step) {
+            // Apply source color conversion from implicit color space
-                int scan = Math.min(step, srcMaxY - y);
+            if (csType == JPEGColorSpace.YCbCr || csType == JPEGColorSpace.YCbCrA) {
+                YCbCrConverter.convertYCbCr2RGB(raster);
+            }
+            else if (csType == JPEGColorSpace.YCCK) {
+                YCbCrConverter.convertYCCK2CMYK(raster);
+            }
+            else if (csType == JPEGColorSpace.CMYK) {
+                invertCMYK(raster);
+            }
+            // ...else assume the raster is already converted
 
-                // Let the progress delegator handle progress, using corrected range
+            WritableRaster dest = destination.createWritableChild(dstRegion.x, dstRegion.y, raster.getWidth(), raster.getHeight(), 0, 0, param.getDestinationBands());
-                progressDelegator.updateProgressRange(100f * (y + scan) / srcRegion.height);
 
-                // Make sure subsampling is within bounds
+            // Apply further color conversion for explicit color space, or just copy the pixels into place
-                if (scan <= param.getSubsamplingYOffset()) {
+            if (convert != null) {
-                    param.setSourceSubsampling(param.getSourceXSubsampling(), param.getSourceYSubsampling(), param.getSubsamplingXOffset(), scan - 1);
+                convert.filter(raster, dest);
-                }
+            }
-
+            else {
-                Rectangle subRegion = new Rectangle(srcRegion.x, y, srcRegion.width, scan);
+                dest.setRect(0, 0, raster);
-                param.setSourceRegion(subRegion);
-                Raster raster = delegate.readRaster(imageIndex, param); // non-converted
-
-                // Apply source color conversion from implicit color space
-                if (csType == JPEGColorSpace.YCbCr || csType == JPEGColorSpace.YCbCrA) {
-                    YCbCrConverter.convertYCbCr2RGB(raster);
-                }
-                else if (csType == JPEGColorSpace.YCCK) {
-                    YCbCrConverter.convertYCCK2CMYK(raster);
-                }
-                else if (csType == JPEGColorSpace.CMYK) {
-                    invertCMYK(raster);
-                }
-                // ...else assume the raster is already converted
-
-                int destHeight = Math.min(raster.getHeight(), dstRegion.height - destY); // Avoid off-by-one
-                Raster src = raster.createChild(0, 0, raster.getWidth(), destHeight, 0, 0, param.getSourceBands());
-                WritableRaster dest = destination.createWritableChild(dstRegion.x, destY, raster.getWidth(), destHeight, 0, 0, param.getDestinationBands());
-
-                // Apply further color conversion for explicit color space, or just copy the pixels into place
-                if (convert != null) {
-                    convert.filter(src, dest);
-//                    WritableRaster filtered = convert.filter(src, null);
-//                    new AffineTransformOp(AffineTransform.getRotateInstance(2 * Math.PI, filtered.getWidth() / 2.0, filtered.getHeight() / 2.0), null).filter(filtered, dest);
-                }
-                else {
-                    dest.setRect(0, 0, src);
-                }
-
-                destY += raster.getHeight();
-
-                if (abortRequested()) {
-                    processReadAborted();
-                    break;
-                }
             }
         }
         finally {
-            // Restore normal read progress processing
-            progressDelegator.resetProgressRange();
-
             // NOTE: Would be cleaner to clone the param, unfortunately it can't be done easily...
             param.setSourceRegion(origSourceRegion);
         }
 
-        processImageComplete();
-
         return image;
     }
 
@@ -1232,54 +1203,25 @@ public class JPEGImageReader extends ImageReaderBase {
     }
 
     private class ProgressDelegator extends ProgressListenerBase implements IIOReadUpdateListener, IIOReadWarningListener {
-        float readProgressStart = -1;
-        float readProgressStop = -1;
-
-        void resetProgressRange() {
-            readProgressStart = -1;
-            readProgressStop = -1;
-        }
-
-        private boolean isProgressRangeCorrected() {
-            return readProgressStart == -1 && readProgressStop == -1;
-        }
-
-        void updateProgressRange(float limit) {
-            Validate.isTrue(limit >= 0, limit, "Negative range limit");
-
-            readProgressStart = readProgressStop != -1 ? readProgressStop : 0;
-            readProgressStop = limit;
-        }
 
         @Override
         public void imageComplete(ImageReader source) {
-            if (isProgressRangeCorrected()) {
                 processImageComplete();
-            }
         }
 
         @Override
         public void imageProgress(ImageReader source, float percentageDone) {
-            if (isProgressRangeCorrected()) {
                 processImageProgress(percentageDone);
-            }
-            else {
-                processImageProgress(readProgressStart + (percentageDone * (readProgressStop - readProgressStart) / 100f));
-            }
         }
 
         @Override
         public void imageStarted(ImageReader source, int imageIndex) {
-            if (isProgressRangeCorrected()) {
                 processImageStarted(imageIndex);
-            }
         }
 
         @Override
         public void readAborted(ImageReader source) {
-            if (isProgressRangeCorrected()) {
                 processReadAborted();
-            }
         }
 
         @Override

imageio/imageio-jpeg/src/test/java/com/twelvemonkeys/imageio/plugins/jpeg/JPEGImageReaderTest.java

@@ -163,7 +163,7 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTestCase<JPEGImageRe
 
         ImageReadParam param = reader.getDefaultReadParam();
         param.setSourceRegion(new Rectangle(800, 800, 64, 8));
-        param.setSourceSubsampling(8, 8, 1, 1);
+        param.setSourceSubsampling(8, 8, 2, 2);
 
         BufferedImage image = reader.read(0, param);
         assertNotNull(image);
@@ -180,7 +180,7 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTestCase<JPEGImageRe
 
     private static void assertJPEGPixelsEqual(byte[] expected, byte[] actual, int actualOffset) {
         for (int i = 0; i < expected.length; i++) {
-            assertEquals(expected[i], actual[i + actualOffset], 5);
+            assertEquals(String.format("Difference in pixel %d", i), expected[i], actual[i + actualOffset], 5);
         }
     }
 
@@ -947,7 +947,6 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTestCase<JPEGImageRe
         assertNotNull(image);
     }
 
-    @Ignore
     @Test
     public void testReadSubsamplingNotSkippingLines1028() throws IOException {
         JPEGImageReader reader = createReader();
@@ -1012,7 +1011,6 @@ public class JPEGImageReaderTest extends ImageReaderAbstractTestCase<JPEGImageRe
         assertNotNull(image);
     }
 
-    @Ignore
     @Test
     public void testReadSubsamplingNotSkippingLines1025() throws IOException {
         JPEGImageReader reader = createReader();