You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2020/05/14 15:03:54 UTC
[GitHub] [commons-imaging] gwlucastrig opened a new pull request #72: IMAGING-251 support for TIFF floating-point formats
gwlucastrig opened a new pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72
This change adds the ability to obtain images from TIFF files that give data in the floating-point format. It partially addresses IMAGING-251. It also adds two example applications to show how to use the Commons Imaging API for TIFF images and adds one additional small sample TIFF data file that uses the floating-point format.
I saw that the project pom.xml specified PMD, so I run PMD and made minor PMD changes. In keeping with the project guidelines, PMD changes were done only for the files that I otherwise modified.
I ran all test cases successfully.
Additional changes to support numerical access for these files will be provided in a future patch.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629250783
Nice! Will start going through it tomorrow. Thanks!!!!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422086650
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
##########
@@ -285,4 +324,57 @@ public BufferedImage readImageData(final Rectangle subImage)
subImage.height);
}
+ @Override
+ public TiffRasterData readRasterData(Rectangle subImage)
+ throws ImageReadException, IOException {
+
+ int xRaster;
+ int yRaster;
+ int rasterWidth;
+ int rasterHeight;
+ if (subImage != null) {
+ xRaster = subImage.x;
+ yRaster = subImage.y;
+ rasterWidth = subImage.width;
+ rasterHeight = subImage.height;
+ } else {
+ xRaster = 0;
+ yRaster = 0;
+ rasterWidth = width;
+ rasterHeight = height;
+ }
+ float[] rasterData = new float[rasterWidth * rasterHeight];
+
+ // the legacy code is optimized to the reading of whole
+ // strips (except for the last strip in the image, which can
+ // be a partial). So create a working image with compatible
+ // dimensions and read that. Later on, the working image
+ // will be sub-imaged to the proper size.
+ // strip0 and strip1 give the indices of the strips containing
+ // the first and last rows of pixels in the subimage
+ final int strip0 = yRaster / rowsPerStrip;
Review comment:
Any chance—even if remote—that `rowsPerStrip` could be zero?
One of the reasons why 1.0-alpha1 took so long (couple years after the work started?) was largely due to CVE's created for sanselan.
One of the CVE, if I am not mistaken, was about a division by zero (or another sort of arithmetic error?), that caused an exception and some infinite loop or large memory allocation (allowing users to perform DDOS).
If it's never zero, then no need to worry. Otherwise, if there's any risk, better set a quick validation somewhere (here or constructor).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422125072
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
Review comment:
Originally, I was going to use something longer and more descriptive, but the length of the package line got a little too long. But I agree that a more descriptive name is better. We could use photometricinterpreters.floatingPoint, but the capitalization disagrees with the conventions of the original package. Do you prefer photometricinterpreters.floatingpoint?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30831964)
Coverage increased (+0.8%) to 75.883% when pulling **77b5936c414762d9a62823f6ead26561cdaf5d4e on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow closed pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow closed pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-623203829
Just checking in. Do you have all the information you need to process this pull request? Is there anything additional that I need to do?
Thanks.
Gary
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-617478052
This submission completes my work on the PR (pending review comments). Thank you for the opportunity to work on Commons Imaging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629740790
Approved. Rebasing and merging. Thanks @gwlucastrig !
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422139048
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForValue.java
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for a color assignment to a single value. This class
+ * will assign a color to a value only if it is an exact match for the input.
+ * This class will also support Float.NaN
+ */
+public class PaletteEntryForValue implements IPaletteEntry {
+
+ private final float value;
+ private final int iColor;
+ private final Color color;
+ private boolean isNull;
+
+ /**
+ * Constructs a palette entry for a single value.
+ * <p>
+ * This class will support color-assignments for Float.NaN.
+ *
+ * @param value the color value associated with this palette entry; a
+ * Float.NaN is allowed.
+ * @param color the color assigned to value
+ */
+ public PaletteEntryForValue(float value, Color color) {
+ if (color == null) {
+ throw new IllegalArgumentException("Null colors not allowed");
+ }
+ this.value = value;
+ this.color = color;
+ this.iColor = color.getRGB();
+ isNull = Float.isNaN(value);
+
+ }
+
+ @Override
+ public boolean isCovered(float f) {
+ if (isNull) {
+ return Float.isNaN(f);
+ }
+ return f == value;
+ }
+
+ @Override
+ public int getARGB(float f) {
+ if (isNull) {
+ if (Float.isNaN(f)) {
+ return iColor;
+ }
+ } else {
+ if (f == value) {
+ return iColor;
+ }
+ }
Review comment:
I agree that it would be cleaner to consolidate these blocks as you recommend.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig closed pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig closed pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-626341691
Thanks for the excellent review comments. I will do changes over the next few days.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629248366
All my tests completed successfully. I have some pretty nice pictures to show for it.
When accepted, I believe you will be able to close both JIRA item 251 and 102.
I am currently working on a tutorial showing how to use the Commons Imaging API to read elevation data. When it's done, I will be posting it on my wiki page at [Gridfour Wiki](https://github.com/gwlucastrig/gridfour/wiki). I will, of course, make sure it links to Commons Imaging.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422592759
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
##########
@@ -285,4 +324,57 @@ public BufferedImage readImageData(final Rectangle subImage)
subImage.height);
}
+ @Override
+ public TiffRasterData readRasterData(Rectangle subImage)
+ throws ImageReadException, IOException {
+
+ int xRaster;
+ int yRaster;
+ int rasterWidth;
+ int rasterHeight;
+ if (subImage != null) {
+ xRaster = subImage.x;
+ yRaster = subImage.y;
+ rasterWidth = subImage.width;
+ rasterHeight = subImage.height;
+ } else {
+ xRaster = 0;
+ yRaster = 0;
+ rasterWidth = width;
+ rasterHeight = height;
+ }
+ float[] rasterData = new float[rasterWidth * rasterHeight];
+
+ // the legacy code is optimized to the reading of whole
+ // strips (except for the last strip in the image, which can
+ // be a partial). So create a working image with compatible
+ // dimensions and read that. Later on, the working image
+ // will be sub-imaged to the proper size.
+ // strip0 and strip1 give the indices of the strips containing
+ // the first and last rows of pixels in the subimage
+ final int strip0 = yRaster / rowsPerStrip;
+ final int strip1 = (yRaster + rasterHeight - 1) / rowsPerStrip;
+
+ for (int strip = strip0; strip <= strip1; strip++) {
+ int yStrip = strip * rowsPerStrip;
+ int rowsRemaining = height - yStrip;
+ int rowsInThisStrip
+ = rowsRemaining > rowsPerStrip ? rowsPerStrip : rowsRemaining;
Review comment:
```
int rowsInThisStrip = Math.min(rowsRemaining, rowsPerStrip);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629638135
Brilliant! Then tomorrow I will finish the review. Final things I'm looking at are
- [x] test coverage of new code (I lost track of how much is covered here)
- [x] possible issues for security (those `N/0`, or loading values from the data without checking boundaries, etc)
- [ ] how much we broke backward compatibility—if any
If nothing is wrong, then I will just squash. Several ways of doing it I think; I normally do with `git rebase -i HEAD~N`, where `N` is the number of commits I want to squash. Doesn't mean we need to squash to a single one, but we can probably combine a few ones. And add a changelog, and merge :-)
Thanks for updating it so quickly!
Bruno
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-617144860
No hurry @gwlucastrig :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629638135
Brilliant! Then tomorrow I will finish the review. Final things I'm looking at are
- [x] test coverage of new code (I lost track of how much is covered here)
- [x] possible issues for security (those `N/0`, or loading values from the data without checking boundaries, etc)
- [x] how much we broke backward compatibility—if any
If nothing is wrong, then I will just squash. Several ways of doing it I think; I normally do with `git rebase -i HEAD~N`, where `N` is the number of commits I want to squash. Doesn't mean we need to squash to a single one, but we can probably combine a few ones. And add a changelog, and merge :-)
Thanks for updating it so quickly!
Bruno
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422129686
##########
File path: src/test/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloatTest.java
##########
@@ -0,0 +1,248 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.ImageWriteException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Provides a unit test for the TIFF photometric interpreter used for mapping
+ * floating-point values to a color palette.
+ */
+public class PhotometricInterpreterFloatTest {
+
+ private static PhotometricInterpreterFloat pInterp;
+ private static PhotometricInterpreterFloat bandedInterp;
+ private static ImageBuilder imageBuilder;
+ private static ImageBuilder bandedImageBuilder;
+
+ private static final Color orange = new Color(255, 136, 62);
+ private static final Color green = new Color(22, 155, 98);
+
+ public PhotometricInterpreterFloatTest() {
+ }
+
+ @BeforeAll
+ public static void setUpClass() throws ImageWriteException, ImageReadException, IOException {
+ // the setup is to assign color (grayscale) values to the
+ // pixels along the main diagonal at coordinates
+ // (0, 0), (1, 1), ... (256, 256).
+ // The floating point values at each pixel are just the
+ // index divided by 256.
+
+ List<IPaletteEntry> paletteList = new ArrayList<>();
+ List<IPaletteEntry> reverseList = new ArrayList<>();
+ for (int i = 0; i < 256; i += 32) {
+ int i1 = i + 31;
+ float f0 = (float) i / 256f;
+ float f1 = (float) (i + 32) / 256f;
+ int argb0 = 0xff000000 | (i << 8) | i;
+ int argb1 = 0xff000000 | (i1 << 8) | i;
+ Color c0 = new Color(argb0);
+ Color c1 = new Color(argb1);
+ PaletteEntryForRange entry = new PaletteEntryForRange(f0, f1, c0, c1);
+ paletteList.add(entry);
+ }
+ // The interpreter is supposed to sort entries. To test that,
+ // we copy them to a list in reverse order.
+ for (int i = paletteList.size() - 1; i >= 0; i--) {
+ IPaletteEntry entry = paletteList.get(i);
+ reverseList.add(entry);
+ }
+
+ pInterp = new PhotometricInterpreterFloat(reverseList);
+
+ // pre-populate the state data for the interpreter with
+ // some values so that we can test min/max access methods.
+ imageBuilder = new ImageBuilder(257, 257, false);
+ int[] samples = new int[1];
+ for (int i = 0; i <= 256; i++) {
+ float f = (float) i / 256f;
+ samples[0] = Float.floatToRawIntBits(f);
+ pInterp.interpretPixel(imageBuilder, samples, i, i);
+ }
+
+ // Now set up a palette than maps values in a range to a single color.
+ List<IPaletteEntry> bandedPaletteList = new ArrayList<>();
+ bandedPaletteList.add(new PaletteEntryForRange(0f, 0.33f, green));
+ bandedPaletteList.add(new PaletteEntryForRange(0.33f, 0.66f, Color.white));
+ bandedPaletteList.add(new PaletteEntryForRange(0.66f, 1.0f, orange));
+ bandedPaletteList.add(new PaletteEntryForValue(Float.NaN, Color.gray));
+ bandedPaletteList.add(new PaletteEntryForValue(-1, Color.gray));
+ bandedInterp = new PhotometricInterpreterFloat(bandedPaletteList);
+ bandedImageBuilder = new ImageBuilder(300, 200, false);
+ for (int j = 0; j < 300; j++) {
+ float f = (float) j / 299.0f;
+ samples[0] = Float.floatToRawIntBits(f);
+ for (int i = 0; i < 200; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, j, i);
+ }
+ }
+ samples[0] = Float.floatToRawIntBits(Float.NaN);
+ for (int i = 0; i < 200; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, 0, i);
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, 299, i);
+ }
+ samples[0] = Float.floatToRawIntBits(-1);
+ for (int i = 0; i < 300; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, i, 0);
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, i, 199);
+ }
+ }
+
+ /**
+ * Test of interpretPixel method, of class PhotometricInterpreterFloat.
+ */
+ @Test
+ public void testInterpretPixel() {
+ for (int i = 0; i < 256; i++) {
+ int lowTest = (i / 32) * 32;
+ int argb = imageBuilder.getRGB(i, i);
+ int b = argb & 0xff;
+ assertEquals(b, lowTest, "Invalid conversion for level " + i);
+ }
+
+ // nothing should match the i=256 case.
+ // The last entry in the palette has values
+ // in the range 224.0/256.0 <= value < 256.0/256.0. So when it
+ // was rendered, there was not palette entry that matched it,
+ // and the corresponding pixel was set to zero.
+ int argb = imageBuilder.getRGB(256, 256);
+ assertEquals(argb, 0, "Invalid upper-bound test");
+
+ // Now inspect the banded palette case
+ argb = bandedImageBuilder.getRGB(0, 0);
+ assertEquals(Color.gray.getRGB(), argb, "Invalid mapping of NaN");
+ argb = bandedImageBuilder.getRGB(50, 10);
+ assertEquals(green.getRGB(), argb, "Invalid mapping of green range");
+ argb = bandedImageBuilder.getRGB(150, 10);
+ assertEquals(Color.white.getRGB(), argb, "Invalid mapping of white range");
+ argb = bandedImageBuilder.getRGB(250, 10);
+ assertEquals(orange.getRGB(), argb, "Invalid mapping of orange range");
+ // Example code to write image to PNG file for visual inspection.
+ //try {
+ // BufferedImage bImage = bandedImageBuilder.getBufferedImage();
+ // ImageIO.write(bImage, "PNG", new File("C:/Users/Public/test1.png"));
+ //} catch (IOException ioex) {
+ // fail("IOException saving test image" + ioex.getMessage());
+ //}
Review comment:
It's not necessary. I left it in to help somebody who was trying to debug the code and wanted to inspect the result. But leaving it in the code is it's like leaving the scaffolding up after you've painted a house. I would follow your preference on this one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r426215117
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterData.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Provides a simple container for floating-point data. Some TIFF files are used
+ * to store floating-point data rather than images. This class is intended to
+ * support access to those TIFF files.
+ */
+public class TiffRasterData {
+
+
+ private final int width;
+ private final int height;
+ private final float[] data;
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ */
+ public TiffRasterData(int width, int height) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ int nCells = width * height;
+ data = new float[nCells];
+ this.width = width;
+ this.height = height;
+
+ }
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ * @param data the data to be stored in the raster.
+ */
+ public TiffRasterData(int width, int height, float[] data) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ if (data == null || data.length < width * height) {
+ throw new IllegalArgumentException(
+ "Specified data does not contain sufficient elements");
+ }
+ this.width = width;
+ this.height = height;
+ this.data = data;
+
+ }
+
+ /**
+ * Sets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @param value the value to be stored at the specified location;
+ * potentially a Float.NaN.
+ */
+ public void setValue(int x, int y, float value) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ data[y * width + x] = value;
+ }
+
+ /**
+ * Gets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @return the value stored at the specified location; potentially a
+ * Float.NaN.
+ */
+ public float getValue(int x, int y) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ return data[y * width + x];
+ }
+
+ /**
+ * Tabulates simple statistics for the raster and returns an instance
+ * containing general metadata.
+ *
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics() {
+ return new TiffRasterStatistics(this, Float.NaN);
+ }
+
+ /**
+ * Tabulates simple statistics for the raster excluding the specified value
+ * and returns an instance containing general metadata,
Review comment:
Fixed locally.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629593803
No blockers so far, another 9 files to go, and then review is done. Comments added now can either be fixed before merging, or afterwards.
@gwlucastrig do you mind if when merging I squash the commits when merging?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422074883
##########
File path: pom.xml
##########
@@ -231,6 +231,30 @@
<version>2.6</version>
<scope>test</scope>
</dependency>
+ <dependency>
+ <groupId>org.seleniumhq.selenium</groupId>
+ <artifactId>selenium-java</artifactId>
+ <scope>test</scope>
+ <version>2.44.0</version>
+ </dependency>
+ <dependency>
+ <groupId>com.opera</groupId>
+ <artifactId>operadriver</artifactId>
+ <scope>test</scope>
+ <version>1.5</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.seleniumhq.selenium</groupId>
+ <artifactId>selenium-remote-driver</artifactId>
+ </exclusion>
+ </exclusions>
+ </dependency>
Review comment:
@gwlucastrig what tests are using Selenium?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
##########
@@ -166,7 +166,7 @@ private void interpretStrip(
// this logic will handle all cases not conforming to the
// special case handled above
- try (final BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
+ try (BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
Review comment:
The less differences in the pull request that are not directly related, the easier it is to review the changes IMO. But if you changed only one or two places that should be fine, otherwise it's better to send the PMD changes in another PR.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-625762025
I had no idea the pom.xml changed. Netbeans is a pain. What a stupid
feature! I will put it back tonight.
On Fri, May 8, 2020, 6:49 AM Bruno P. Kinoshita <no...@github.com>
wrote:
> *@kinow* commented on this pull request.
> ------------------------------
>
> In pom.xml
> <https://github.com/apache/commons-imaging/pull/72#discussion_r422074883>:
>
> > + <artifactId>selenium-java</artifactId>
> + <scope>test</scope>
> + <version>2.44.0</version>
> + </dependency>
> + <dependency>
> + <groupId>com.opera</groupId>
> + <artifactId>operadriver</artifactId>
> + <scope>test</scope>
> + <version>1.5</version>
> + <exclusions>
> + <exclusion>
> + <groupId>org.seleniumhq.selenium</groupId>
> + <artifactId>selenium-remote-driver</artifactId>
> + </exclusion>
> + </exclusions>
> + </dependency>
>
> @gwlucastrig <https://github.com/gwlucastrig> what tests are using
> Selenium?
> ------------------------------
>
> In
> src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
> <https://github.com/apache/commons-imaging/pull/72#discussion_r422076450>:
>
> > @@ -166,7 +166,7 @@ private void interpretStrip(
> // this logic will handle all cases not conforming to the
> // special case handled above
>
> - try (final BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
> + try (BitInputStream bis = new BitInputStream(new ByteArrayInputStream(bytes), byteOrder)) {
>
> The less differences in the pull request that are not directly related,
> the easier it is to review the changes IMO. But if you changed only one or
> two places that should be fine, otherwise it's better to send the PMD
> changes in another PR.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/commons-imaging/pull/72#pullrequestreview-408154953>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AEWJDYKC67GLF2JCSZJNOXTRQPPSFANCNFSM4MAWGVRQ>
> .
>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-628694841
I believe that I have now integrated all the changes from the review comments with two exceptions.
The TiffRasterData.getData() method still exposes and internal array. However, I've added Javadoc explaining the rationale and use of the method.
The Collections.sort() methods in PhotometricInterpreterFloat are still there. When I made the recommended changes, animal-sniffer threw an exception. I then installed Java JDK 11 (picking 11 because that's what Travis uses) and the same thing happened. So I am leaving the older code in place.
My plan is to do a bit more testing over the next few days. I do not plan to make any further code changes unless I encounter an unexpected error.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629638135
Brilliant! Then tomorrow I will finish the review. Final things I'm looking at are
- [x] test coverage of new code (I lost track of how much is covered here)
- [ ] possible issues for security (those `N/0`, or loading values from the data without checking boundaries, etc)
- [ ] how much we broke backward compatibility—if any
If nothing is wrong, then I will just squash. Several ways of doing it I think; I normally do with `git rebase -i HEAD~N`, where `N` is the number of commits I want to squash. Doesn't mean we need to squash to a single one, but we can probably combine a few ones. And add a changelog, and merge :-)
Thanks for updating it so quickly!
Bruno
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30814781)
Coverage increased (+0.8%) to 75.883% when pulling **654ecb9ab8f9a22a085490f88966797e42a9a2d0 on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422568377
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/ImageDataReader.java
##########
@@ -14,6 +14,96 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
+ /*
+ * Implementation Notes:
+ *
+ * Additional implementation notes are given in:
+ * DataReaderStrips.java
+ * DataReaderTiled.java
+ *
+ * The TIFF Floating-Point Formats ----------------------------------
+ * In addition to providing images, TIFF files can supply data in the
+ * form of numerical values. As of March 2020 the Commons Imaging library
+ * was extended to support some floating-point data formats.
+ * Unfortunately, the floating-point format allows for a lot of different
+ * variations and only the most widely used of these are currently supported.
+ * At the time of implementation, only a small set of data products were
+ * available. Thus it is likely that developers will wish to extend this capability
+ * as additional test data become available. When implementing extensions
+ * to this logic, developers are reminder that image processing requires
+ * access to literally millions of pixels, so attention to performance
+ * is essential to a successful implementation (please see the notes in
+ * DataReaderStrips.java for more information).
+ * The TIFF floating-point implementation is very poorly documented.
+ * So these notes are included to provide clarification on at least
+ * some aspects of the format.
+ *
+ * The Predictor==3 Case
+ * TIFF specifies an extension for a predictor that is intended to
+ * improve data compression ratios for floating-point values. This
+ * predictor is specified using the TIFF predictor TAG with a value of 3
+ * (see TIFF Technical Note 3, April 8, 2005). Consider a 4-byte floating
+ * point value given in IEEE-754 format. Let f3 be the high-order byte,
+ * with f2 the next highest, followed by f1, and f0 for the
+ * low-order byte. This designation shoulod not be confused with the
+ * in-memory layout of the bytes (little-endian versus big-endian), but
+ * rather their numerical values. The sign bit and upper 7 bits of the exponent
+ * are given in the high-order byte, followed by the remaining sign bit
+ * and the mantissa in the lower.
+ * In many real-valued raster data sets, the sign and magnitude (exponent)
+ * of the values changes slowly which the contents of the mantissa vary in
+ * a semi-random manner, with the information entropy tending to increase
+ * in the lowest ordered bytes. Thus, the high-order bytes have more
+ * redundancy than the low-order bytes and can compress more efficiently.
+ * To exploit this, the TIFF format splits the bytes into groups based on their
+ * order-of-magnitude. This splitting process takes place on a ROW-BY-ROW
+ * basis (note the emphasis, this point is not clearly documented in the spec).
+ * . For example, for row length of 3 pixels -- A, B, and C -- the data
+ * for two rows would be given as shown below (again, ignoring endian issues):
+ * Original:
+ * A3 A2 A1 A0 B3 B2 B1 B0 C3 C2 C1 C0
+ * D3 D3 D1 D0 E3 E2 E2 E0 F3 F2 F1 F0
+ *
+ * Bytes split into groups by order-of-magnitude:
+ * A3 B3 C3 A2 B2 C2 A1 B1 C1 A0 B0 C0
+ * D3 E3 F3 D2 E2 F2 D1 E1 F1 D0 E0 F0
+ *
+ * To further improve the compression, the predictor takes the difference of
+ * each subsequent bytes. Again, the differences (deltas) are computed on
+ * a row-byte-row basis. For the most part, the differences combine
+ * bytes associated with the same order-of-magnitude, though there is
+ * a special transition at the end of each order-of-magnitude set (shown in
+ * parentheses):
+ *
+ * A3, B3-A3, C3-B3, (A2-C3), B2-A2, C2-B2, (A1-C2), etc.
+ * D3, E3-D3, F3-D3, (D2-F3), E3-D2, etc.
+ *
+ * Once the predictor transform is complete, the data is stored using
+ * conventional data compression techniques such as Deflate or LZW.
+ * In practice, floating point data does not compress especially well, but
+ * using the above technique, the TIFF process typically reduces the overall
+ * storage size by 20 to 30 percent (depending on the data).
+ * The TIFF Technical Note 3 specifies 3 data size formats for
+ * storing floating point values:
+ * 32 bits IEEE-754 single-precision standard
+ * 16 bits IEEE-754 half-precision standard
+ * 24 bits A non-standard representation
+ * At this time, we have not obtained data samples for the smaller
+ * representations used in combination with a predictor.
+ *
+ * Interleaved formats
+ * TIFF Technical Note 3 also provides sample code for interleaved
+ * data, such as a real-valued vector or a complex pair. At this time
+ * no samples of interleaved data were available. As a caveat, the specification
+ * that the document provides has disadvantages in terms of code complexity
+ * and performance. Because the interleaved evaluation is embedded inside
+ * the pixel row and column loops, it puts a lot of redundant conditional
+ * evaluations inside the double nested loops. It is recommended that when
+ * interleaved data is implemented, it should get their own block of code
+ * so as not to interfere with the more common non-interleaved floating-point
+ * processing.
+ */
Review comment:
Comments before the package declaration are normally only for the copyright. The class comment below will be included in the generated javadocs.
I think this whole block should be part of that too?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629638135
Brilliant! Then tomorrow I will finish the review. Final things I'm looking at are
- test coverage of new code (I lost track of how much is covered here)
- possible issues for security (those `N/0`, or loading values from the data without checking boundaries, etc)
- how much we broke backward compatibility—if any
If nothing is wrong, then I will just squash. Several ways of doing it I think; I normally do with `git rebase -i HEAD~N`, where `N` is the number of commits I want to squash. Doesn't mean we need to squash to a single one, but we can probably combine a few ones. And add a changelog, and merge :-)
Thanks for updating it so quickly!
Bruno
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422140919
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for colors associated with a range of values. The
+ * return value will be interpolated between the minimum and maximum value for
+ * this entry.
+ * <p>
+ * In keeping with the conventions of many Geographic Information Systems (GIS)
+ * and art applications, this instance "covered" values in the range v0 ≤ f
+ * < v1. Thus, a value that exactly matches the upper bound of the range is
+ * not considered "covered".
+ */
+public class PaletteEntryForRange implements IPaletteEntry {
+
+ private final float v0;
+ private final float v1;
+ private final float deltaV;
+ private final float r0;
+ private final float r1;
+ private final float g0;
+ private final float g1;
+ private final float b0;
+ private final float b1;
+ private final float a0;
+ private final float a1;
+ private final float deltaA;
+ private final float deltaR;
+ private final float deltaG;
+ private final float deltaB;
Review comment:
We could just get rid of the "delta" variables and change the places where it's used such as
int r = (int) (t * deltaR + r0 + 0.5);
To
int r = (int) (t * (r1-r0) + r0 + 0.5);
I don't think the first form actually saves any processing time. It's just the way I happened to code it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629637256
@kinow I would prefer that you squash the commits. I didn't know that there was a git command that allowed you to do that.
I renamed IPaletteEntry to simple PaletteEntry. I got a list of all Java files in the commons-imaging source code and saw that none of the others used the leading-I convention. So it makes sense to rename the class.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30775939)
Coverage increased (+0.8%) to 75.883% when pulling **ac1ef5220eb4b113b93402ea52ed70f15901565a on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-616916050
>Since I've already said that I've "finished" this Pull Request (pending your review), I won't burden you with more changes... But I may submit the change as a future PR, if you think it a good idea.
No burden at all (but thanks for considering that), we should use the momentum to improve as much as we can IMO. @gwlucastrig do you think it fits into this PR? If so, feel free to add a new commit.
Or, if you think this would be easier to review separately, feel free to create a new PR. Separate smaller pull requests would be easier to review, but only if you think it makes sense. Once a separate PR is done, then we can rebase this or other existing PR's to have the new changes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629740776
> how much we broke backward compatibility—if any
Nothing too drastic—i.e. API changes OK IMHO for next **alpha** release (this will change once we have 1.0 out)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422077764
##########
File path: src/test/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloatTest.java
##########
@@ -0,0 +1,248 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.ImageWriteException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Provides a unit test for the TIFF photometric interpreter used for mapping
+ * floating-point values to a color palette.
+ */
+public class PhotometricInterpreterFloatTest {
+
+ private static PhotometricInterpreterFloat pInterp;
+ private static PhotometricInterpreterFloat bandedInterp;
+ private static ImageBuilder imageBuilder;
+ private static ImageBuilder bandedImageBuilder;
+
+ private static final Color orange = new Color(255, 136, 62);
+ private static final Color green = new Color(22, 155, 98);
+
+ public PhotometricInterpreterFloatTest() {
+ }
+
+ @BeforeAll
+ public static void setUpClass() throws ImageWriteException, ImageReadException, IOException {
+ // the setup is to assign color (grayscale) values to the
+ // pixels along the main diagonal at coordinates
+ // (0, 0), (1, 1), ... (256, 256).
+ // The floating point values at each pixel are just the
+ // index divided by 256.
+
+ List<IPaletteEntry> paletteList = new ArrayList<>();
+ List<IPaletteEntry> reverseList = new ArrayList<>();
+ for (int i = 0; i < 256; i += 32) {
+ int i1 = i + 31;
+ float f0 = (float) i / 256f;
+ float f1 = (float) (i + 32) / 256f;
+ int argb0 = 0xff000000 | (i << 8) | i;
+ int argb1 = 0xff000000 | (i1 << 8) | i;
+ Color c0 = new Color(argb0);
+ Color c1 = new Color(argb1);
+ PaletteEntryForRange entry = new PaletteEntryForRange(f0, f1, c0, c1);
+ paletteList.add(entry);
+ }
+ // The interpreter is supposed to sort entries. To test that,
+ // we copy them to a list in reverse order.
+ for (int i = paletteList.size() - 1; i >= 0; i--) {
+ IPaletteEntry entry = paletteList.get(i);
+ reverseList.add(entry);
+ }
+
+ pInterp = new PhotometricInterpreterFloat(reverseList);
+
+ // pre-populate the state data for the interpreter with
+ // some values so that we can test min/max access methods.
+ imageBuilder = new ImageBuilder(257, 257, false);
+ int[] samples = new int[1];
+ for (int i = 0; i <= 256; i++) {
+ float f = (float) i / 256f;
+ samples[0] = Float.floatToRawIntBits(f);
+ pInterp.interpretPixel(imageBuilder, samples, i, i);
+ }
+
+ // Now set up a palette than maps values in a range to a single color.
+ List<IPaletteEntry> bandedPaletteList = new ArrayList<>();
+ bandedPaletteList.add(new PaletteEntryForRange(0f, 0.33f, green));
+ bandedPaletteList.add(new PaletteEntryForRange(0.33f, 0.66f, Color.white));
+ bandedPaletteList.add(new PaletteEntryForRange(0.66f, 1.0f, orange));
+ bandedPaletteList.add(new PaletteEntryForValue(Float.NaN, Color.gray));
+ bandedPaletteList.add(new PaletteEntryForValue(-1, Color.gray));
+ bandedInterp = new PhotometricInterpreterFloat(bandedPaletteList);
+ bandedImageBuilder = new ImageBuilder(300, 200, false);
+ for (int j = 0; j < 300; j++) {
+ float f = (float) j / 299.0f;
+ samples[0] = Float.floatToRawIntBits(f);
+ for (int i = 0; i < 200; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, j, i);
+ }
+ }
+ samples[0] = Float.floatToRawIntBits(Float.NaN);
+ for (int i = 0; i < 200; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, 0, i);
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, 299, i);
+ }
+ samples[0] = Float.floatToRawIntBits(-1);
+ for (int i = 0; i < 300; i++) {
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, i, 0);
+ bandedInterp.interpretPixel(bandedImageBuilder, samples, i, 199);
+ }
+ }
+
+ /**
+ * Test of interpretPixel method, of class PhotometricInterpreterFloat.
+ */
+ @Test
+ public void testInterpretPixel() {
+ for (int i = 0; i < 256; i++) {
+ int lowTest = (i / 32) * 32;
+ int argb = imageBuilder.getRGB(i, i);
+ int b = argb & 0xff;
+ assertEquals(b, lowTest, "Invalid conversion for level " + i);
+ }
+
+ // nothing should match the i=256 case.
+ // The last entry in the palette has values
+ // in the range 224.0/256.0 <= value < 256.0/256.0. So when it
+ // was rendered, there was not palette entry that matched it,
+ // and the corresponding pixel was set to zero.
+ int argb = imageBuilder.getRGB(256, 256);
+ assertEquals(argb, 0, "Invalid upper-bound test");
+
+ // Now inspect the banded palette case
+ argb = bandedImageBuilder.getRGB(0, 0);
+ assertEquals(Color.gray.getRGB(), argb, "Invalid mapping of NaN");
+ argb = bandedImageBuilder.getRGB(50, 10);
+ assertEquals(green.getRGB(), argb, "Invalid mapping of green range");
+ argb = bandedImageBuilder.getRGB(150, 10);
+ assertEquals(Color.white.getRGB(), argb, "Invalid mapping of white range");
+ argb = bandedImageBuilder.getRGB(250, 10);
+ assertEquals(orange.getRGB(), argb, "Invalid mapping of orange range");
+ // Example code to write image to PNG file for visual inspection.
+ //try {
+ // BufferedImage bImage = bandedImageBuilder.getBufferedImage();
+ // ImageIO.write(bImage, "PNG", new File("C:/Users/Public/test1.png"));
+ //} catch (IOException ioex) {
+ // fail("IOException saving test image" + ioex.getMessage());
+ //}
Review comment:
Do we need the example code in this test? Asking because we had a big review/refactoring during the 0.9 to 1.0 vote thread, and many of the reviews/changes requested were around things like commented code that wasn't working after some recent changes. But we can leave it here if that's useful for another developer maintaining this test.
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterData.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Provides a simple container for floating-point data. Some TIFF files are used
+ * to store floating-point data rather than images. This class is intended to
+ * support access to those TIFF files.
+ */
+public class TiffRasterData {
+
+
+ private final int width;
+ private final int height;
+ private final float[] data;
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ */
+ public TiffRasterData(int width, int height) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ int nCells = width * height;
+ data = new float[nCells];
+ this.width = width;
+ this.height = height;
+
+ }
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ * @param data the data to be stored in the raster.
+ */
+ public TiffRasterData(int width, int height, float[] data) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ if (data == null || data.length < width * height) {
+ throw new IllegalArgumentException(
+ "Specified data does not contain sufficient elements");
+ }
+ this.width = width;
+ this.height = height;
+ this.data = data;
+
+ }
+
+ /**
+ * Sets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @param value the value to be stored at the specified location;
+ * potentially a Float.NaN.
+ */
+ public void setValue(int x, int y, float value) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ data[y * width + x] = value;
+ }
+
+ /**
+ * Gets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @return the value stored at the specified location; potentially a
+ * Float.NaN.
+ */
+ public float getValue(int x, int y) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ return data[y * width + x];
+ }
+
+ /**
+ * Tabulates simple statistics for the raster and returns an instance
+ * containing general metadata.
+ *
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics() {
+ return new TiffRasterStatistics(this, Float.NaN);
+ }
+
+ /**
+ * Tabulates simple statistics for the raster excluding the specified value
+ * and returns an instance containing general metadata,
Review comment:
s/,/.
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterData.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Provides a simple container for floating-point data. Some TIFF files are used
+ * to store floating-point data rather than images. This class is intended to
+ * support access to those TIFF files.
+ */
+public class TiffRasterData {
+
+
+ private final int width;
+ private final int height;
+ private final float[] data;
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ */
+ public TiffRasterData(int width, int height) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ int nCells = width * height;
+ data = new float[nCells];
+ this.width = width;
+ this.height = height;
+
+ }
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ * @param data the data to be stored in the raster.
+ */
+ public TiffRasterData(int width, int height, float[] data) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ if (data == null || data.length < width * height) {
+ throw new IllegalArgumentException(
+ "Specified data does not contain sufficient elements");
+ }
+ this.width = width;
+ this.height = height;
+ this.data = data;
+
+ }
+
+ /**
+ * Sets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @param value the value to be stored at the specified location;
+ * potentially a Float.NaN.
+ */
+ public void setValue(int x, int y, float value) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ data[y * width + x] = value;
+ }
+
+ /**
+ * Gets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @return the value stored at the specified location; potentially a
+ * Float.NaN.
+ */
+ public float getValue(int x, int y) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ return data[y * width + x];
+ }
+
+ /**
+ * Tabulates simple statistics for the raster and returns an instance
+ * containing general metadata.
+ *
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics() {
+ return new TiffRasterStatistics(this, Float.NaN);
+ }
+
+ /**
+ * Tabulates simple statistics for the raster excluding the specified value
+ * and returns an instance containing general metadata,
+ *
+ * @param valueToExclude exclude samples with this specified value.
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics(float valueToExclude) {
+ return new TiffRasterStatistics(this, valueToExclude);
+ }
+
+ /**
+ * Gets the width (number of columns) of the raster.
+ *
+ * @return the width of the raster
+ */
+ public int getWidth() {
+ return width;
+ }
+
+ /**
+ * Gets the height (number of rows) of the raster.
+ *
+ * @return the height of the raster.
+ */
+ public int getHeight() {
+ return height;
+ }
+
+ /**
+ * Returns a reference to the data array stored in this instance. Note that
+ * value is <strong>not</strong> a safe copy and that modifying it would
+ * directly affect the content of the instance. While this design approach
+ * carries some risk in terms of data security, it was chosen for reasons of
+ * performance and memory conservation.
+ *
+ * @return a direct reference to the data array stored in this instance.
+ */
+ public float[] getData() {
+ return data;
Review comment:
Would returning a copy of the data be a problem? Is it the array of the complete image? We could offer this method, telling users to use it at their own risk too, while avoiding possible bugs (security or simple bugs due to changes in this array?)
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
##########
@@ -14,6 +14,12 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
+ /*
+ * Implementation notes:
+ * See ImageDataReader for notes on development
+ * with particular emphasis on run-time performance.
+ */
Review comment:
I think this comment could go above the class declaration, and maybe use the @link or @see, etc Javadocs annotations?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
##########
@@ -235,4 +286,59 @@ public BufferedImage readImageData(final Rectangle subImage)
subImage.height);
}
+ @Override
+ public TiffRasterData readRasterData(final Rectangle subImage)
+ throws ImageReadException, IOException {
+ final int bitsPerRow = tileWidth * bitsPerPixel;
+ final int bytesPerRow = (bitsPerRow + 7) / 8;
+ final int bytesPerTile = bytesPerRow * tileLength;
+ int xRaster;
+ int yRaster;
+ int rasterWidth;
+ int rasterHeight;
+ if (subImage != null) {
+ xRaster = subImage.x;
+ yRaster = subImage.y;
+ rasterWidth = subImage.width;
+ rasterHeight = subImage.height;
+ } else {
+ xRaster = 0;
+ yRaster = 0;
+ rasterWidth = width;
+ rasterHeight = height;
+ }
+ float[] rasterData = new float[rasterWidth * rasterHeight];
+
+ // tileWidth is the width of the tile
+ // tileLength is the height of the tile
+ final int col0 = xRaster / tileWidth;
+ final int col1 = (xRaster + rasterWidth - 1) / tileWidth;
Review comment:
Ditto the above here about the division by zero.
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderStrips.java
##########
@@ -285,4 +324,57 @@ public BufferedImage readImageData(final Rectangle subImage)
subImage.height);
}
+ @Override
+ public TiffRasterData readRasterData(Rectangle subImage)
+ throws ImageReadException, IOException {
+
+ int xRaster;
+ int yRaster;
+ int rasterWidth;
+ int rasterHeight;
+ if (subImage != null) {
+ xRaster = subImage.x;
+ yRaster = subImage.y;
+ rasterWidth = subImage.width;
+ rasterHeight = subImage.height;
+ } else {
+ xRaster = 0;
+ yRaster = 0;
+ rasterWidth = width;
+ rasterHeight = height;
+ }
+ float[] rasterData = new float[rasterWidth * rasterHeight];
+
+ // the legacy code is optimized to the reading of whole
+ // strips (except for the last strip in the image, which can
+ // be a partial). So create a working image with compatible
+ // dimensions and read that. Later on, the working image
+ // will be sub-imaged to the proper size.
+ // strip0 and strip1 give the indices of the strips containing
+ // the first and last rows of pixels in the subimage
+ final int strip0 = yRaster / rowsPerStrip;
Review comment:
Any change—even if remote—that `rowsPerStrip` could be zero?
One of the reasons why 1.0-alpha1 took so long (couple years after the work started?) was largely due to CVE's created for sanselan.
One of the CVE, if I am not mistaken, was about a division by zero (or another sort of arithmetic error?), that caused an exception and some infinite loop or large memory allocation (allowing users to perform DDOS).
If it's never zero, then no need to worry. Otherwise, if there's any risk, better set a quick validation somewhere (here or constructor).
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
Review comment:
I am really bad with names. But I wonder if we could find an alternative for `fp`. See its parent package as example, `photometricinterpreters`. It could be `pi` too, but `tiff.pi.fp` looks even more enigmatic.
@gwlucastrig could we use some different name, easier for users/devs to know what's that package for/about?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for colors associated with a range of values. The
+ * return value will be interpolated between the minimum and maximum value for
+ * this entry.
+ * <p>
+ * In keeping with the conventions of many Geographic Information Systems (GIS)
+ * and art applications, this instance "covered" values in the range v0 ≤ f
+ * < v1. Thus, a value that exactly matches the upper bound of the range is
+ * not considered "covered".
+ */
+public class PaletteEntryForRange implements IPaletteEntry {
+
+ private final float v0;
+ private final float v1;
+ private final float deltaV;
+ private final float r0;
+ private final float r1;
+ private final float g0;
+ private final float g1;
+ private final float b0;
+ private final float b1;
+ private final float a0;
+ private final float a1;
+ private final float deltaA;
+ private final float deltaR;
+ private final float deltaG;
+ private final float deltaB;
Review comment:
Probably nit-picking. I think the variables above are related to variable1, variable0, red1, red2, green1, green2, etc. But could be wrong.
In case these variables appear somewhere like debugger/profiler/log/stacktrace/etc, it would be better to have a better name I think. Is it something that could be changed @gwlucastrig ? (I acknowledge sometimes the best name is the simplest like these, so just asking if you have any suggestion that could make it easier for other contributors)
Maybe as we are talking about range, the v0 and v1 could be left, right. Or from, to. Or fromInclusive, toInclusive, etc. Something that is easy for another developer used to other "ranges" to associate with?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForValue.java
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for a color assignment to a single value. This class
+ * will assign a color to a value only if it is an exact match for the input.
+ * This class will also support Float.NaN
+ */
+public class PaletteEntryForValue implements IPaletteEntry {
+
+ private final float value;
+ private final int iColor;
+ private final Color color;
+ private boolean isNull;
+
+ /**
+ * Constructs a palette entry for a single value.
+ * <p>
+ * This class will support color-assignments for Float.NaN.
+ *
+ * @param value the color value associated with this palette entry; a
+ * Float.NaN is allowed.
+ * @param color the color assigned to value
+ */
+ public PaletteEntryForValue(float value, Color color) {
+ if (color == null) {
+ throw new IllegalArgumentException("Null colors not allowed");
+ }
+ this.value = value;
+ this.color = color;
+ this.iColor = color.getRGB();
+ isNull = Float.isNaN(value);
+
+ }
+
+ @Override
+ public boolean isCovered(float f) {
+ if (isNull) {
+ return Float.isNaN(f);
+ }
+ return f == value;
+ }
+
+ @Override
+ public int getARGB(float f) {
+ if (isNull) {
+ if (Float.isNaN(f)) {
+ return iColor;
+ }
+ } else {
+ if (f == value) {
+ return iColor;
+ }
+ }
+ return 0;
+ }
+
+ @Override
+ public Color getColor(float f) {
+ if (isNull) {
+ if (Float.isNaN(f)) {
+ return color;
+ }
+ } else {
+ if (f == value) {
+ return color;
+ }
+ }
Review comment:
Simplify expression above?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForValue.java
##########
@@ -0,0 +1,108 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for a color assignment to a single value. This class
+ * will assign a color to a value only if it is an exact match for the input.
+ * This class will also support Float.NaN
+ */
+public class PaletteEntryForValue implements IPaletteEntry {
+
+ private final float value;
+ private final int iColor;
+ private final Color color;
+ private boolean isNull;
+
+ /**
+ * Constructs a palette entry for a single value.
+ * <p>
+ * This class will support color-assignments for Float.NaN.
+ *
+ * @param value the color value associated with this palette entry; a
+ * Float.NaN is allowed.
+ * @param color the color assigned to value
+ */
+ public PaletteEntryForValue(float value, Color color) {
+ if (color == null) {
+ throw new IllegalArgumentException("Null colors not allowed");
+ }
+ this.value = value;
+ this.color = color;
+ this.iColor = color.getRGB();
+ isNull = Float.isNaN(value);
+
+ }
+
+ @Override
+ public boolean isCovered(float f) {
+ if (isNull) {
+ return Float.isNaN(f);
+ }
+ return f == value;
+ }
+
+ @Override
+ public int getARGB(float f) {
+ if (isNull) {
+ if (Float.isNaN(f)) {
+ return iColor;
+ }
+ } else {
+ if (f == value) {
+ return iColor;
+ }
+ }
Review comment:
```
if (isNull && Float.isNaN(f)) {
return iColor;
} else if (f == value) {
return iColor;
}
```
?
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterData.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Provides a simple container for floating-point data. Some TIFF files are used
+ * to store floating-point data rather than images. This class is intended to
+ * support access to those TIFF files.
+ */
+public class TiffRasterData {
+
+
+ private final int width;
+ private final int height;
Review comment:
Would it make sense call these two `columns` and `rows` instead? Asking because I noticed the other docs appear to treat them as such. Happy with either option :+1:
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629740020
>test coverage of new code (I lost track of how much is covered here)
Some of the new code is not covered by tests. But alas I think it's quite hard to find images for these tests. Later we may look into having some sort of image generator, or try to locate more test images.
https://coveralls.io/builds/30831964
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-617140956
I think the change would fit perfectly into the PR. I will try to get the work turned in tonight.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422592479
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterStatistics.java
##########
@@ -0,0 +1,144 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Collects and stores a set of simple statistics from the input raster.
+ */
+public class TiffRasterStatistics {
+
+ private final int nSample;
+ private final int nNull;
+ private final float minValue;
+ private final float maxValue;
+ private final float meanValue;
+ private final float excludedValue;
+
+ /**
+ * Constructs an instance of this class, tabulating results from the input
+ * raster data.
+ *
+ * @param raster the input data
+ * @param excludedValue an optional value to ignore; use Float.NaN if no
+ * value is to be ignored.
+ */
+ TiffRasterStatistics(TiffRasterData raster, float excludedValue) {
+ this.excludedValue = excludedValue;
+ float vMin = Float.POSITIVE_INFINITY;
+ float vMax = Float.NEGATIVE_INFINITY;
+ double vSum = 0;
+ int nS = 0;
+ int nN = 0;
+ float[] data = raster.getData();
+ for (int i = 0; i < data.length; i++) {
+ float test = data[i];
+ if (Float.isNaN(test)) {
+ nN++;
+ continue;
+ }
+ if (test == excludedValue) {
+ continue;
+ }
+
+ nS++;
+ vSum += test;
+ if (test < vMin) {
+ vMin = test;
+ }
+ if (test > vMax) {
+ vMax = test;
+ }
+ }
+
+ minValue = vMin;
+ maxValue = vMax;
+ nSample = nS;
+ nNull = nN;
+ if (nSample == 0) {
+ meanValue = 0;
+ } else {
+ meanValue = (float) (vSum / nSample);
+ }
+ }
+
+ /**
+ * Get the count of the number of non-null and non-excluded samples in the
+ * collection.
+ *
+ * @return the a positive number, potentially zero
+ */
+ public int getCountOfSamples() {
+ return nSample;
+ }
+
+ /**
+ * Get the count of the number of null samples in the collection.
+ *
+ * @return the a positive number, potentially zero
+ */
+ public int getCountOfNulls() {
+ return nNull;
+ }
+
+ /**
+ * Get the minimum value found in the source data
+ *
+ * @return the minimum value found in the source data
+ */
+ public float getMinValue() {
+ return minValue;
+ }
+
+ /**
+ * Get the maximum value found in the source data
+ *
+ * @return the maximum value found in the source data
+ */
+ public float getMaxValue() {
+ return maxValue;
+ }
+
+ /**
+ * Get the mean value for all sample values in the raster. Null-data values
+ * and excluded values are not considered.
+ *
+ * @return the meanValuethe mean value of the samples
Review comment:
"return the mean value of the samples"
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422128692
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/TiffRasterData.java
##########
@@ -0,0 +1,155 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff;
+
+/**
+ * Provides a simple container for floating-point data. Some TIFF files are used
+ * to store floating-point data rather than images. This class is intended to
+ * support access to those TIFF files.
+ */
+public class TiffRasterData {
+
+
+ private final int width;
+ private final int height;
+ private final float[] data;
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ */
+ public TiffRasterData(int width, int height) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ int nCells = width * height;
+ data = new float[nCells];
+ this.width = width;
+ this.height = height;
+
+ }
+
+ /**
+ * Construct an instance allocating memory for the specified dimensions.
+ *
+ * @param width a value of 1 or greater
+ * @param height a value of 1 or greater
+ * @param data the data to be stored in the raster.
+ */
+ public TiffRasterData(int width, int height, float[] data) {
+ if (width <= 0 || height <= 0) {
+ throw new IllegalArgumentException(
+ "Raster dimensions less than or equal to zero are not supported");
+ }
+ if (data == null || data.length < width * height) {
+ throw new IllegalArgumentException(
+ "Specified data does not contain sufficient elements");
+ }
+ this.width = width;
+ this.height = height;
+ this.data = data;
+
+ }
+
+ /**
+ * Sets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @param value the value to be stored at the specified location;
+ * potentially a Float.NaN.
+ */
+ public void setValue(int x, int y, float value) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ data[y * width + x] = value;
+ }
+
+ /**
+ * Gets the value stored at the specified raster coordinates.
+ *
+ * @param x integer coordinate in the columnar direction
+ * @param y integer coordinate in the row direction
+ * @return the value stored at the specified location; potentially a
+ * Float.NaN.
+ */
+ public float getValue(int x, int y) {
+ if (x < 0 || x >= width || y < 0 || y >= height) {
+ throw new IllegalArgumentException(
+ "Coordinates out of range (" + x + ", " + y + ")");
+ }
+ return data[y * width + x];
+ }
+
+ /**
+ * Tabulates simple statistics for the raster and returns an instance
+ * containing general metadata.
+ *
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics() {
+ return new TiffRasterStatistics(this, Float.NaN);
+ }
+
+ /**
+ * Tabulates simple statistics for the raster excluding the specified value
+ * and returns an instance containing general metadata,
+ *
+ * @param valueToExclude exclude samples with this specified value.
+ * @return a valid instance.
+ */
+ public TiffRasterStatistics getSimpleStatistics(float valueToExclude) {
+ return new TiffRasterStatistics(this, valueToExclude);
+ }
+
+ /**
+ * Gets the width (number of columns) of the raster.
+ *
+ * @return the width of the raster
+ */
+ public int getWidth() {
+ return width;
+ }
+
+ /**
+ * Gets the height (number of rows) of the raster.
+ *
+ * @return the height of the raster.
+ */
+ public int getHeight() {
+ return height;
+ }
+
+ /**
+ * Returns a reference to the data array stored in this instance. Note that
+ * value is <strong>not</strong> a safe copy and that modifying it would
+ * directly affect the content of the instance. While this design approach
+ * carries some risk in terms of data security, it was chosen for reasons of
+ * performance and memory conservation.
+ *
+ * @return a direct reference to the data array stored in this instance.
+ */
+ public float[] getData() {
+ return data;
Review comment:
To me, the right design choice here was not at all clear. I am still not entirely sure.
Returning a copy of the data would be safer and more in keeping with "secure coding" practices. The problem is that GeoTIFF images are often quite large. The US Geological Survey files I've been using are 10800x10800 pixels in size. So the memory they use is about 445 megabytes. To make a duplicate copy of such a large block of data seems like it could be a problem.
Also, I wrote the getData() to supplement the getData(int x, int y) for efficiency. The getData(int x, int y) call has to perform bounds checking. If an application has access to the whole array, then it can bypass that.
In practice, when an application is dealing with images that large, it should probably be reading them in in smaller pieces.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422593120
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/ImageDataReader.java
##########
@@ -232,4 +332,267 @@ protected void resetPredictor() {
throw new ImageReadException("Tiff: unknown/unsupported compression: " + compression);
}
}
+
+ /**
+ * Given a source file that specifies the floating-point data format, unpack
+ * the raw bytes obtained from the source file and organize them into an
+ * array of integers containing the bit-equivalent of IEEE-754 32-bit
+ * floats. Source files containing 64 bit doubles are downcast to floats.
+ * <p>
+ * This method supports either the tile format or the strip format of TIFF
+ * source files. The scan size indicates the number of columns to be
+ * extracted. For strips, the width and the scan size are always the full
+ * width of the image. For tiles, the scan size is the full width of the
+ * tile, but the width may be smaller in the cases where the tiles do not
+ * evenly divide the width (for example, a 256 pixel wide tile in a 257
+ * pixel wide image would result in two columns of tiles, the second column
+ * having only one column of pixels that were worth extracting.
+ *
+ * @param width the width of the data block to be extracted
+ * @param height the height of the data block to be extracted
+ * @param scansize the number of pixels in a single row of the block
+ * @param bytes the raw bytes
+ * @param predictor the predictor specified by the source, only predictor 3
+ * is supported.
+ * @param bitsPerSample the number of bits per sample, 32 or 64.
+ * @param byteOrder the byte order for the source data
+ * @return a valid array of integers in row major order, dimensions
+ * scan-size wide and height height.
+ * @throws ImageReadException in the event of an invalid format.
+ */
+ protected int[] unpackFloatingPointSamples(
+ int width,
+ int height,
+ int scansize,
+ byte[] bytes,
+ int predictor,
+ int bitsPerSample, ByteOrder byteOrder)
+ throws ImageReadException {
+ int bytesPerSample = bitsPerSample / 8;
+ int nBytes = bytesPerSample * scansize * height;
+ int length = bytes.length < nBytes ? nBytes / scansize : height;
+
+ int[] samples = new int[scansize * height];
+ // floating-point differencing is indicated by a predictor value of 3.
+ if (predictor == TiffTagConstants.PREDICTOR_VALUE_FLOATING_POINT_DIFFERENCING) {
+ // at this time, this class supports the 32-bit format. The
+ // main reason for this is that we have not located sample data
+ // that can be used for testing and analysis.
+ if (bitsPerSample != 32) {
+ throw new ImageReadException(
+ "Imaging does not yet support floating-point data"
+ + " with predictor type 3 for "
+ + bitsPerSample + " bits per sample");
+ }
+ int bytesInRow = scansize * 4;
+ for (int i = 0; i < length; i++) {
+ int aOffset = i * bytesInRow;
+ int bOffset = aOffset + scansize;
+ int cOffset = bOffset + scansize;
+ int dOffset = cOffset + scansize;
+ // in this loop, the source bytes give delta values.
+ // we adjust them to give true values. This operation is
+ // done on a row-by-row basis.
+ for (int j = 1; j < bytesInRow; j++) {
+ bytes[aOffset + j] += bytes[aOffset + j - 1];
+ }
+ // pack the bytes into the integer bit-equivalent of
+ // floating point values
+ int index = i * scansize;
+ for (int j = 0; j < width; j++) {
+ int a = bytes[aOffset + j];
+ int b = bytes[bOffset + j];
+ int c = bytes[cOffset + j];
+ int d = bytes[dOffset + j];
+ // Pack the 4 byte components into a single integer
+ // in the byte order used by the TIFF standard
+ samples[index++] = ((a & 0xff) << 24)
+ | ((b & 0xff) << 16)
+ | ((c & 0xff) << 8)
+ | (d & 0xff);
+ }
+ }
+ return samples;
+ } // end of predictor==3 case.
+
+ // simple packing case, 64 or 32 bits --------------------------
+ if (bitsPerSample == 64) {
+ int k = 0;
+ int index = 0;
+ for (int i = 0; i < length; i++) {
+ for (int j = 0; j < scansize; j++) {
+ long b0 = bytes[k++] & 0xffL;
+ long b1 = bytes[k++] & 0xffL;
+ long b2 = bytes[k++] & 0xffL;
+ long b3 = bytes[k++] & 0xffL;
+ long b4 = bytes[k++] & 0xffL;
+ long b5 = bytes[k++] & 0xffL;
+ long b6 = bytes[k++] & 0xffL;
+ long b7 = bytes[k++] & 0xffL;
+ long sbits;
+ if (byteOrder == ByteOrder.LITTLE_ENDIAN) {
+ sbits = (b7 << 56)
+ | (b6 << 48)
+ | (b5 << 40)
+ | (b4 << 32)
+ | (b3 << 24)
+ | (b2 << 16)
+ | (b1 << 8)
+ | b0;
+
+ } else {
+ sbits = (b0 << 56)
+ | (b1 << 48)
+ | (b2 << 40)
+ | (b3 << 32)
+ | (b4 << 24)
+ | (b5 << 16)
+ | (b6 << 8)
+ | b7;
+ }
+ // since the photometric interpreter does not
+ // currently support doubles, we need to replace this
+ // element with a float. This action is inefficient and
+ // should be improved.
+ float f = (float) Double.longBitsToDouble(sbits);
+ samples[index++] = Float.floatToRawIntBits(f);
+ }
+ }
+ } else if (bitsPerSample == 32) {
+ int k = 0;
+ int index = 0;
+ for (int i = 0; i < length; i++) {
+ for (int j = 0; j < scansize; j++) {
+ int b0 = bytes[k++] & 0xff;
+ int b1 = bytes[k++] & 0xff;
+ int b2 = bytes[k++] & 0xff;
+ int b3 = bytes[k++] & 0xff;
+ int sbits;
+ if (byteOrder == ByteOrder.LITTLE_ENDIAN) {
+ sbits
+ = (b3 << 24)
+ | (b2 << 16)
+ | (b1 << 8)
+ | b0;
+
+ } else {
+ sbits
+ = (b0 << 24)
+ | (b1 << 16)
+ | (b2 << 8)
+ | b3;
+ }
+ // since the photometric interpreter does not
+ // currently support doubles, we need to replace this
+ // element with a float. This action is inefficient and
+ // should be improved.
+ samples[index++] = sbits;
+ }
+ }
+ } else {
+ throw new ImageReadException(
+ "Imaging does not support floating-point samples with "
+ + bitsPerSample + " bits per sample");
+ }
+
+ return samples;
+ }
+
+ /**
+ *
+ * @param xBlock coordinate of block relative to source data
+ * @param yBlock coordinate of block relative to source data
+ * @param blockWidth width of block, in pixels
+ * @param blockHeight height of block in pixels
+ * @param blockData the data for the block
+ * @param xRaster coordinate of raster relative to source data
+ * @param yRaster coordinate of raster relative to source data
+ * @param rasterWidth width of the raster (salways smaller than source data)
Review comment:
s/salways/always
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/ImageDataReader.java
##########
@@ -232,4 +332,267 @@ protected void resetPredictor() {
throw new ImageReadException("Tiff: unknown/unsupported compression: " + compression);
}
}
+
+ /**
+ * Given a source file that specifies the floating-point data format, unpack
+ * the raw bytes obtained from the source file and organize them into an
+ * array of integers containing the bit-equivalent of IEEE-754 32-bit
+ * floats. Source files containing 64 bit doubles are downcast to floats.
+ * <p>
+ * This method supports either the tile format or the strip format of TIFF
+ * source files. The scan size indicates the number of columns to be
+ * extracted. For strips, the width and the scan size are always the full
+ * width of the image. For tiles, the scan size is the full width of the
+ * tile, but the width may be smaller in the cases where the tiles do not
+ * evenly divide the width (for example, a 256 pixel wide tile in a 257
+ * pixel wide image would result in two columns of tiles, the second column
+ * having only one column of pixels that were worth extracting.
+ *
+ * @param width the width of the data block to be extracted
+ * @param height the height of the data block to be extracted
+ * @param scansize the number of pixels in a single row of the block
+ * @param bytes the raw bytes
+ * @param predictor the predictor specified by the source, only predictor 3
+ * is supported.
+ * @param bitsPerSample the number of bits per sample, 32 or 64.
+ * @param byteOrder the byte order for the source data
+ * @return a valid array of integers in row major order, dimensions
+ * scan-size wide and height height.
+ * @throws ImageReadException in the event of an invalid format.
+ */
+ protected int[] unpackFloatingPointSamples(
+ int width,
+ int height,
+ int scansize,
+ byte[] bytes,
+ int predictor,
+ int bitsPerSample, ByteOrder byteOrder)
+ throws ImageReadException {
+ int bytesPerSample = bitsPerSample / 8;
+ int nBytes = bytesPerSample * scansize * height;
+ int length = bytes.length < nBytes ? nBytes / scansize : height;
+
+ int[] samples = new int[scansize * height];
+ // floating-point differencing is indicated by a predictor value of 3.
+ if (predictor == TiffTagConstants.PREDICTOR_VALUE_FLOATING_POINT_DIFFERENCING) {
+ // at this time, this class supports the 32-bit format. The
+ // main reason for this is that we have not located sample data
+ // that can be used for testing and analysis.
+ if (bitsPerSample != 32) {
+ throw new ImageReadException(
+ "Imaging does not yet support floating-point data"
+ + " with predictor type 3 for "
+ + bitsPerSample + " bits per sample");
+ }
+ int bytesInRow = scansize * 4;
+ for (int i = 0; i < length; i++) {
+ int aOffset = i * bytesInRow;
+ int bOffset = aOffset + scansize;
+ int cOffset = bOffset + scansize;
+ int dOffset = cOffset + scansize;
+ // in this loop, the source bytes give delta values.
+ // we adjust them to give true values. This operation is
+ // done on a row-by-row basis.
+ for (int j = 1; j < bytesInRow; j++) {
+ bytes[aOffset + j] += bytes[aOffset + j - 1];
+ }
+ // pack the bytes into the integer bit-equivalent of
+ // floating point values
+ int index = i * scansize;
+ for (int j = 0; j < width; j++) {
+ int a = bytes[aOffset + j];
+ int b = bytes[bOffset + j];
+ int c = bytes[cOffset + j];
+ int d = bytes[dOffset + j];
+ // Pack the 4 byte components into a single integer
+ // in the byte order used by the TIFF standard
+ samples[index++] = ((a & 0xff) << 24)
+ | ((b & 0xff) << 16)
+ | ((c & 0xff) << 8)
+ | (d & 0xff);
+ }
+ }
+ return samples;
+ } // end of predictor==3 case.
+
+ // simple packing case, 64 or 32 bits --------------------------
+ if (bitsPerSample == 64) {
+ int k = 0;
+ int index = 0;
+ for (int i = 0; i < length; i++) {
+ for (int j = 0; j < scansize; j++) {
+ long b0 = bytes[k++] & 0xffL;
+ long b1 = bytes[k++] & 0xffL;
+ long b2 = bytes[k++] & 0xffL;
+ long b3 = bytes[k++] & 0xffL;
+ long b4 = bytes[k++] & 0xffL;
+ long b5 = bytes[k++] & 0xffL;
+ long b6 = bytes[k++] & 0xffL;
+ long b7 = bytes[k++] & 0xffL;
+ long sbits;
+ if (byteOrder == ByteOrder.LITTLE_ENDIAN) {
+ sbits = (b7 << 56)
+ | (b6 << 48)
+ | (b5 << 40)
+ | (b4 << 32)
+ | (b3 << 24)
+ | (b2 << 16)
+ | (b1 << 8)
+ | b0;
+
+ } else {
+ sbits = (b0 << 56)
+ | (b1 << 48)
+ | (b2 << 40)
+ | (b3 << 32)
+ | (b4 << 24)
+ | (b5 << 16)
+ | (b6 << 8)
+ | b7;
+ }
+ // since the photometric interpreter does not
+ // currently support doubles, we need to replace this
+ // element with a float. This action is inefficient and
+ // should be improved.
+ float f = (float) Double.longBitsToDouble(sbits);
+ samples[index++] = Float.floatToRawIntBits(f);
+ }
+ }
+ } else if (bitsPerSample == 32) {
+ int k = 0;
+ int index = 0;
+ for (int i = 0; i < length; i++) {
+ for (int j = 0; j < scansize; j++) {
+ int b0 = bytes[k++] & 0xff;
+ int b1 = bytes[k++] & 0xff;
+ int b2 = bytes[k++] & 0xff;
+ int b3 = bytes[k++] & 0xff;
+ int sbits;
+ if (byteOrder == ByteOrder.LITTLE_ENDIAN) {
+ sbits
+ = (b3 << 24)
+ | (b2 << 16)
+ | (b1 << 8)
+ | b0;
+
+ } else {
+ sbits
+ = (b0 << 24)
+ | (b1 << 16)
+ | (b2 << 8)
+ | b3;
+ }
+ // since the photometric interpreter does not
+ // currently support doubles, we need to replace this
+ // element with a float. This action is inefficient and
+ // should be improved.
+ samples[index++] = sbits;
+ }
+ }
+ } else {
+ throw new ImageReadException(
+ "Imaging does not support floating-point samples with "
+ + bitsPerSample + " bits per sample");
+ }
+
+ return samples;
+ }
+
+ /**
+ *
+ * @param xBlock coordinate of block relative to source data
+ * @param yBlock coordinate of block relative to source data
+ * @param blockWidth width of block, in pixels
+ * @param blockHeight height of block in pixels
+ * @param blockData the data for the block
+ * @param xRaster coordinate of raster relative to source data
+ * @param yRaster coordinate of raster relative to source data
+ * @param rasterWidth width of the raster (salways smaller than source data)
+ * @param rasterHeight height of the raster (always smaller than source
+ * data)
+ * @param rasterData the raster data.
+ */
+ void transferBlockToRaster(int xBlock, int yBlock,
+ int blockWidth, int blockHeight, int blockData[],
+ int xRaster, int yRaster,
+ int rasterWidth, int rasterHeight, float[] rasterData) {
+
+ // xR0, yR0 are the coordinates within the raster (upper-left corner)
+ // xR1, yR1 are ONE PAST the coordinates of the lower-right corner
+ int xR0 = xBlock - xRaster; // xR0, yR0 coordinates relative to
+ int yR0 = yBlock - yRaster; // the raster
+ int xR1 = xR0 + blockWidth;
+ int yR1 = yR0 + blockHeight;
+ if (xR0 < 0) {
+ xR0 = 0;
+ }
+ if (yR0 < 0) {
+ yR0 = 0;
+ }
+ if (xR1 > rasterWidth) {
+ xR1 = rasterWidth;
+ }
+ if (yR1 > rasterHeight) {
+ yR1 = rasterHeight;
+ }
+
+ // Recall that the above logic may have adjusted xR0, xY0 so that
+ // they are not necessrily point to the source pixel at xRaster, yRaster
Review comment:
s/necessrily/necessrily
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/ImageDataReader.java
##########
@@ -14,6 +14,96 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
+ /*
+ * Implementation Notes:
+ *
+ * Additional implementation notes are given in:
+ * DataReaderStrips.java
+ * DataReaderTiled.java
+ *
+ * The TIFF Floating-Point Formats ----------------------------------
+ * In addition to providing images, TIFF files can supply data in the
+ * form of numerical values. As of March 2020 the Commons Imaging library
+ * was extended to support some floating-point data formats.
+ * Unfortunately, the floating-point format allows for a lot of different
+ * variations and only the most widely used of these are currently supported.
+ * At the time of implementation, only a small set of data products were
+ * available. Thus it is likely that developers will wish to extend this capability
+ * as additional test data become available. When implementing extensions
+ * to this logic, developers are reminder that image processing requires
+ * access to literally millions of pixels, so attention to performance
+ * is essential to a successful implementation (please see the notes in
+ * DataReaderStrips.java for more information).
+ * The TIFF floating-point implementation is very poorly documented.
+ * So these notes are included to provide clarification on at least
+ * some aspects of the format.
+ *
+ * The Predictor==3 Case
+ * TIFF specifies an extension for a predictor that is intended to
+ * improve data compression ratios for floating-point values. This
+ * predictor is specified using the TIFF predictor TAG with a value of 3
+ * (see TIFF Technical Note 3, April 8, 2005). Consider a 4-byte floating
+ * point value given in IEEE-754 format. Let f3 be the high-order byte,
+ * with f2 the next highest, followed by f1, and f0 for the
+ * low-order byte. This designation shoulod not be confused with the
Review comment:
s/shoulod/should
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloat.java
##########
@@ -0,0 +1,305 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.PhotometricInterpreter;
+
+/**
+ * Implements a custom photometric interpreter that can be supplied by
+ * applications in order to render Java images from real-valued TIFF data
+ * products. Most TIFF files include a specification for a "photometric
+ * interpreter" that implements logic for transforming the raw data in a TIFF
+ * file to a rendered image. But the TIFF standard does not include a
+ * specification for a photometric interpreter that can be used for rendering
+ * floating-point data. TIFF files are sometimes used to specify non-image data
+ * as a floating-point raster. This approach is particularly common in GeoTIFF
+ * files (TIFF files that contain tags for supporting geospatial reference
+ * metadata for Geographic Information Systems). Because of the limits of the
+ * stock photometric interpreters, most floating-point TIFF files to not produce
+ * useful images.
+ * <p>
+ * This class allows an Apache Commons implementation to construct and specify a
+ * custom photometric interpreter when reading from a TIFF file. Applications
+ * may supply their own palette that maps real-valued data to specified colors.
+ * <p>
+ * This class provides two constructors:
+ * <ol>
+ * <li>A simple constructor to support gray scales</li>
+ * <li>A constructor to support a color palette (with potential
+ * interpolation)</li>
+ * </ol>
+ * <p>
+ * To use this class, an application must access the TIFF file using the
+ * low-level, TIFF-specific API provided by the Apache Commons Imaging library.
+ *
+ */
+public class PhotometricInterpreterFloat extends PhotometricInterpreter {
+
+ ArrayList<IPaletteEntry> rangePaletteEntries = new ArrayList<>();
+ ArrayList<IPaletteEntry> singleValuePaletteEntries = new ArrayList<>();
+
+ float minFound = Float.POSITIVE_INFINITY;
+ float maxFound = Float.NEGATIVE_INFINITY;
+ int xMin;
+ int yMin;
+ int xMax;
+ int yMax;
+
+ double sumFound;
+ int nFound;
+
+ /**
+ * Constructs a photometric interpreter that will produce a gray scale
+ * linearly distributed across the RGB color space for values in the range
+ * valueBlack to valueWhite. Note that the two values may be given in either
+ * ascending order or descending order, but they must not be equal. Infinite
+ * values will not result in proper numerical computations.
+ *
+ * @param valueBlack the value associated with the dark side of the gray
+ * scale
+ * @param valueWhite the value associated with the light side of the gray
+ * scale
+ */
+ public PhotometricInterpreterFloat(
+ float valueBlack, float valueWhite) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
Review comment:
s/forma/format
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for colors associated with a range of values. The
+ * return value will be interpolated between the minimum and maximum value for
+ * this entry.
+ * <p>
+ * In keeping with the conventions of many Geographic Information Systems (GIS)
+ * and art applications, this instance "covered" values in the range v0 ≤ f
+ * < v1. Thus, a value that exactly matches the upper bound of the range is
+ * not considered "covered".
+ */
+public class PaletteEntryForRange implements IPaletteEntry {
+
+ private final float v0;
+ private final float v1;
+ private final float deltaV;
+ private final float r0;
+ private final float r1;
+ private final float g0;
+ private final float g1;
+ private final float b0;
+ private final float b1;
+ private final float a0;
+ private final float a1;
+ private final float deltaA;
+ private final float deltaR;
+ private final float deltaG;
+ private final float deltaB;
+
+ /**
+ * Constructs a palette entry for the range of values v0 ≤ f < v1. The
+ * return color value will be interpolated between the two specified colors.
+ *
+ * @param v0 the lower bounds (inclusive) of the covered range of values
+ * @param v1 the upper bounds (non-inclusive) of the covered range of value
+ * @param color0 the color assigned to value v0
+ * @param color1 the color assigned to value v1
+ */
+ public PaletteEntryForRange(float v0, float v1, Color color0, Color color1) {
+ this.v0 = v0;
+ this.v1 = v1;
+ deltaV = v1 - v0;
+ // check for range volation
+ if (deltaV <= 0 || Float.isNaN(deltaV)) {
+ throw new IllegalArgumentException("Specified values must be v0<v1");
+ }
+ if (color0 == null || color1 == null) {
+ throw new IllegalArgumentException("Null colors not allowed");
+ }
+ int argb0 = color0.getRGB();
+ a0 = (argb0 >> 24) & 0xff;
+ r0 = (argb0 >> 16) & 0xff;
+ g0 = (argb0 >> 8) & 0xff;
+ b0 = argb0 & 0xff;
+
+ int argb1 = color1.getRGB();
+ a1 = (argb1 >> 24) & 0xff;
+ r1 = (argb1 >> 16) & 0xff;
+ g1 = (argb1 >> 8) & 0xff;
+ b1 = argb1 & 0xff;
+
+ deltaA = a1 - a0;
+ deltaR = r1 - r0;
+ deltaG = g1 - g0;
+ deltaB = b1 - b0;
+ }
+
+ /**
+ * Constructs a palette entry for the range of values v0 ≤ f < v1. A
+ * single color will be returned for all values in range
+ *
+ * @param v0 the lower bounds (inclusive) of the covered range of values
+ * @param v1 the upper bounds (non-inclusive) of the covered range of value
+ * @param color the color assigned to value v0
+ */
+ public PaletteEntryForRange(float v0, float v1, Color color) {
+ this.v0 = v0;
+ this.v1 = v1;
+ deltaV = v1 - v0;
+ // check for range volation
Review comment:
s/volation/violation
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloat.java
##########
@@ -0,0 +1,305 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.PhotometricInterpreter;
+
+/**
+ * Implements a custom photometric interpreter that can be supplied by
+ * applications in order to render Java images from real-valued TIFF data
+ * products. Most TIFF files include a specification for a "photometric
+ * interpreter" that implements logic for transforming the raw data in a TIFF
+ * file to a rendered image. But the TIFF standard does not include a
+ * specification for a photometric interpreter that can be used for rendering
+ * floating-point data. TIFF files are sometimes used to specify non-image data
+ * as a floating-point raster. This approach is particularly common in GeoTIFF
+ * files (TIFF files that contain tags for supporting geospatial reference
+ * metadata for Geographic Information Systems). Because of the limits of the
+ * stock photometric interpreters, most floating-point TIFF files to not produce
+ * useful images.
+ * <p>
+ * This class allows an Apache Commons implementation to construct and specify a
+ * custom photometric interpreter when reading from a TIFF file. Applications
+ * may supply their own palette that maps real-valued data to specified colors.
+ * <p>
+ * This class provides two constructors:
+ * <ol>
+ * <li>A simple constructor to support gray scales</li>
+ * <li>A constructor to support a color palette (with potential
+ * interpolation)</li>
+ * </ol>
+ * <p>
+ * To use this class, an application must access the TIFF file using the
+ * low-level, TIFF-specific API provided by the Apache Commons Imaging library.
+ *
+ */
+public class PhotometricInterpreterFloat extends PhotometricInterpreter {
+
+ ArrayList<IPaletteEntry> rangePaletteEntries = new ArrayList<>();
+ ArrayList<IPaletteEntry> singleValuePaletteEntries = new ArrayList<>();
+
+ float minFound = Float.POSITIVE_INFINITY;
+ float maxFound = Float.NEGATIVE_INFINITY;
+ int xMin;
+ int yMin;
+ int xMax;
+ int yMax;
+
+ double sumFound;
+ int nFound;
+
+ /**
+ * Constructs a photometric interpreter that will produce a gray scale
+ * linearly distributed across the RGB color space for values in the range
+ * valueBlack to valueWhite. Note that the two values may be given in either
+ * ascending order or descending order, but they must not be equal. Infinite
+ * values will not result in proper numerical computations.
+ *
+ * @param valueBlack the value associated with the dark side of the gray
+ * scale
+ * @param valueWhite the value associated with the light side of the gray
+ * scale
+ */
+ public PhotometricInterpreterFloat(
+ float valueBlack, float valueWhite) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
+ 32 // pro format height value
+ );
+
+
+ if (valueWhite > valueBlack) {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueBlack, valueWhite, Color.black, Color.white);
+ rangePaletteEntries.add(entry);
+ } else {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueWhite, valueBlack, Color.white, Color.black);
+ rangePaletteEntries.add(entry);
+ }
+ }
+
+ /**
+ * Constructs a photometric interpreter that will use the specified palette
+ * to assign colors to floating-point values.
+ *
+ * @param paletteEntries a valid, non-empty list of palette entries
+ */
+ public PhotometricInterpreterFloat(List<IPaletteEntry> paletteEntries) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
Review comment:
s/forma/format
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PaletteEntryForRange.java
##########
@@ -0,0 +1,175 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+
+/**
+ * Provides a palette entry for colors associated with a range of values. The
+ * return value will be interpolated between the minimum and maximum value for
+ * this entry.
+ * <p>
+ * In keeping with the conventions of many Geographic Information Systems (GIS)
+ * and art applications, this instance "covered" values in the range v0 ≤ f
+ * < v1. Thus, a value that exactly matches the upper bound of the range is
+ * not considered "covered".
+ */
+public class PaletteEntryForRange implements IPaletteEntry {
+
+ private final float v0;
+ private final float v1;
+ private final float deltaV;
+ private final float r0;
+ private final float r1;
+ private final float g0;
+ private final float g1;
+ private final float b0;
+ private final float b1;
+ private final float a0;
+ private final float a1;
+ private final float deltaA;
+ private final float deltaR;
+ private final float deltaG;
+ private final float deltaB;
+
+ /**
+ * Constructs a palette entry for the range of values v0 ≤ f < v1. The
+ * return color value will be interpolated between the two specified colors.
+ *
+ * @param v0 the lower bounds (inclusive) of the covered range of values
+ * @param v1 the upper bounds (non-inclusive) of the covered range of value
+ * @param color0 the color assigned to value v0
+ * @param color1 the color assigned to value v1
+ */
+ public PaletteEntryForRange(float v0, float v1, Color color0, Color color1) {
+ this.v0 = v0;
+ this.v1 = v1;
+ deltaV = v1 - v0;
+ // check for range volation
Review comment:
s/volation/violation
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloat.java
##########
@@ -0,0 +1,305 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.PhotometricInterpreter;
+
+/**
+ * Implements a custom photometric interpreter that can be supplied by
+ * applications in order to render Java images from real-valued TIFF data
+ * products. Most TIFF files include a specification for a "photometric
+ * interpreter" that implements logic for transforming the raw data in a TIFF
+ * file to a rendered image. But the TIFF standard does not include a
+ * specification for a photometric interpreter that can be used for rendering
+ * floating-point data. TIFF files are sometimes used to specify non-image data
+ * as a floating-point raster. This approach is particularly common in GeoTIFF
+ * files (TIFF files that contain tags for supporting geospatial reference
+ * metadata for Geographic Information Systems). Because of the limits of the
+ * stock photometric interpreters, most floating-point TIFF files to not produce
+ * useful images.
+ * <p>
+ * This class allows an Apache Commons implementation to construct and specify a
+ * custom photometric interpreter when reading from a TIFF file. Applications
+ * may supply their own palette that maps real-valued data to specified colors.
+ * <p>
+ * This class provides two constructors:
+ * <ol>
+ * <li>A simple constructor to support gray scales</li>
+ * <li>A constructor to support a color palette (with potential
+ * interpolation)</li>
+ * </ol>
+ * <p>
+ * To use this class, an application must access the TIFF file using the
+ * low-level, TIFF-specific API provided by the Apache Commons Imaging library.
+ *
+ */
+public class PhotometricInterpreterFloat extends PhotometricInterpreter {
+
+ ArrayList<IPaletteEntry> rangePaletteEntries = new ArrayList<>();
+ ArrayList<IPaletteEntry> singleValuePaletteEntries = new ArrayList<>();
+
+ float minFound = Float.POSITIVE_INFINITY;
+ float maxFound = Float.NEGATIVE_INFINITY;
+ int xMin;
+ int yMin;
+ int xMax;
+ int yMax;
+
+ double sumFound;
+ int nFound;
+
+ /**
+ * Constructs a photometric interpreter that will produce a gray scale
+ * linearly distributed across the RGB color space for values in the range
+ * valueBlack to valueWhite. Note that the two values may be given in either
+ * ascending order or descending order, but they must not be equal. Infinite
+ * values will not result in proper numerical computations.
+ *
+ * @param valueBlack the value associated with the dark side of the gray
+ * scale
+ * @param valueWhite the value associated with the light side of the gray
+ * scale
+ */
+ public PhotometricInterpreterFloat(
+ float valueBlack, float valueWhite) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
+ 32 // pro format height value
+ );
+
+
+ if (valueWhite > valueBlack) {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueBlack, valueWhite, Color.black, Color.white);
+ rangePaletteEntries.add(entry);
+ } else {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueWhite, valueBlack, Color.white, Color.black);
+ rangePaletteEntries.add(entry);
+ }
+ }
+
+ /**
+ * Constructs a photometric interpreter that will use the specified palette
+ * to assign colors to floating-point values.
+ *
+ * @param paletteEntries a valid, non-empty list of palette entries
+ */
+ public PhotometricInterpreterFloat(List<IPaletteEntry> paletteEntries) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
+ 32 // pro format height value
+ );
+
+ if (paletteEntries == null || paletteEntries.isEmpty()) {
+ throw new IllegalArgumentException(
+ "Palette entries list must be non-null and non-empty");
+ }
+
+ for (IPaletteEntry entry : paletteEntries) {
+ if (entry.coversSingleEntry()) {
+ singleValuePaletteEntries.add(entry);
+ } else {
+ rangePaletteEntries.add(entry);
+ }
+ }
+
+ Comparator<IPaletteEntry> comparator = new Comparator<IPaletteEntry>() {
+ @Override
+ public int compare(IPaletteEntry o1, IPaletteEntry o2) {
+ if (o1.getLowerBound() == o2.getLowerBound()) {
+ return Double.compare(o1.getUpperBound(), o2.getUpperBound());
+ }
+ return Double.compare(o1.getLowerBound(), o2.getLowerBound());
+ }
+ };
Review comment:
Java 8 lambda
```
Comparator<IPaletteEntry> comparator = (o1, o2) -> {
if (o1.getLowerBound() == o2.getLowerBound()) {
return Double.compare(o1.getUpperBound(), o2.getUpperBound());
}
return Double.compare(o1.getLowerBound(), o2.getLowerBound());
};
```
##########
File path: src/test/java/org/apache/commons/imaging/examples/tiff/ExampleReadFloatingPointData.java
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.examples.tiff;
+
+import java.awt.Color;
+import java.awt.image.BufferedImage;
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteOrder;
Review comment:
Unused import
##########
File path: src/test/java/org/apache/commons/imaging/examples/tiff/ExampleReadFloatingPointData.java
##########
@@ -0,0 +1,186 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.examples.tiff;
+
+import java.awt.Color;
+import java.awt.image.BufferedImage;
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteOrder;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import javax.imageio.ImageIO;
+import org.apache.commons.imaging.FormatCompliance;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.apache.commons.imaging.common.bytesource.ByteSourceFile;
+import org.apache.commons.imaging.formats.tiff.TiffContents;
+import org.apache.commons.imaging.formats.tiff.TiffDirectory;
+import org.apache.commons.imaging.formats.tiff.TiffRasterData;
+import org.apache.commons.imaging.formats.tiff.TiffRasterStatistics;
+import org.apache.commons.imaging.formats.tiff.TiffReader;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp.IPaletteEntry;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp.PaletteEntryForRange;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp.PaletteEntryForValue;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp.PhotometricInterpreterFloat;
+
+/**
+ * A simple example application that reads the content of a TIFF file containing
+ * floating-point data and extracts its content. TIFF files are * sometimes used to store non-image information for scientific and geophysical
+ * data products, including terrestrial elevation and ocean depth data.
+ *
+ */
+public class ExampleReadFloatingPointData {
Review comment:
Used this file to experiment with the new code. Excellent example!!! :clap: :clap: :clap:
Brilliant! Thanks a lot for this.
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/fp/PhotometricInterpreterFloat.java
##########
@@ -0,0 +1,305 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.fp;
+
+import java.awt.Color;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.Comparator;
+import java.util.List;
+import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.ImageBuilder;
+import org.apache.commons.imaging.formats.tiff.photometricinterpreters.PhotometricInterpreter;
+
+/**
+ * Implements a custom photometric interpreter that can be supplied by
+ * applications in order to render Java images from real-valued TIFF data
+ * products. Most TIFF files include a specification for a "photometric
+ * interpreter" that implements logic for transforming the raw data in a TIFF
+ * file to a rendered image. But the TIFF standard does not include a
+ * specification for a photometric interpreter that can be used for rendering
+ * floating-point data. TIFF files are sometimes used to specify non-image data
+ * as a floating-point raster. This approach is particularly common in GeoTIFF
+ * files (TIFF files that contain tags for supporting geospatial reference
+ * metadata for Geographic Information Systems). Because of the limits of the
+ * stock photometric interpreters, most floating-point TIFF files to not produce
+ * useful images.
+ * <p>
+ * This class allows an Apache Commons implementation to construct and specify a
+ * custom photometric interpreter when reading from a TIFF file. Applications
+ * may supply their own palette that maps real-valued data to specified colors.
+ * <p>
+ * This class provides two constructors:
+ * <ol>
+ * <li>A simple constructor to support gray scales</li>
+ * <li>A constructor to support a color palette (with potential
+ * interpolation)</li>
+ * </ol>
+ * <p>
+ * To use this class, an application must access the TIFF file using the
+ * low-level, TIFF-specific API provided by the Apache Commons Imaging library.
+ *
+ */
+public class PhotometricInterpreterFloat extends PhotometricInterpreter {
+
+ ArrayList<IPaletteEntry> rangePaletteEntries = new ArrayList<>();
+ ArrayList<IPaletteEntry> singleValuePaletteEntries = new ArrayList<>();
+
+ float minFound = Float.POSITIVE_INFINITY;
+ float maxFound = Float.NEGATIVE_INFINITY;
+ int xMin;
+ int yMin;
+ int xMax;
+ int yMax;
+
+ double sumFound;
+ int nFound;
+
+ /**
+ * Constructs a photometric interpreter that will produce a gray scale
+ * linearly distributed across the RGB color space for values in the range
+ * valueBlack to valueWhite. Note that the two values may be given in either
+ * ascending order or descending order, but they must not be equal. Infinite
+ * values will not result in proper numerical computations.
+ *
+ * @param valueBlack the value associated with the dark side of the gray
+ * scale
+ * @param valueWhite the value associated with the light side of the gray
+ * scale
+ */
+ public PhotometricInterpreterFloat(
+ float valueBlack, float valueWhite) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
+ 32 // pro format height value
+ );
+
+
+ if (valueWhite > valueBlack) {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueBlack, valueWhite, Color.black, Color.white);
+ rangePaletteEntries.add(entry);
+ } else {
+ PaletteEntryForRange entry
+ = new PaletteEntryForRange(valueWhite, valueBlack, Color.white, Color.black);
+ rangePaletteEntries.add(entry);
+ }
+ }
+
+ /**
+ * Constructs a photometric interpreter that will use the specified palette
+ * to assign colors to floating-point values.
+ *
+ * @param paletteEntries a valid, non-empty list of palette entries
+ */
+ public PhotometricInterpreterFloat(List<IPaletteEntry> paletteEntries) {
+ // The abstract base class requires that the following fields
+ // be set in the constructor:
+ // samplesPerPixel (int)
+ // bits per sample (array of type int[samplesPerPixel])
+ // predictor (int, not used by this class)
+ // width (int)
+ // height (int)
+ super(
+ 1,
+ new int[]{32}, // bits per sample
+ 0, // not used by this class
+ 32, // pro forma width value
+ 32 // pro format height value
+ );
+
+ if (paletteEntries == null || paletteEntries.isEmpty()) {
+ throw new IllegalArgumentException(
+ "Palette entries list must be non-null and non-empty");
+ }
+
+ for (IPaletteEntry entry : paletteEntries) {
+ if (entry.coversSingleEntry()) {
+ singleValuePaletteEntries.add(entry);
+ } else {
+ rangePaletteEntries.add(entry);
+ }
+ }
+
+ Comparator<IPaletteEntry> comparator = new Comparator<IPaletteEntry>() {
+ @Override
+ public int compare(IPaletteEntry o1, IPaletteEntry o2) {
+ if (o1.getLowerBound() == o2.getLowerBound()) {
+ return Double.compare(o1.getUpperBound(), o2.getUpperBound());
+ }
+ return Double.compare(o1.getLowerBound(), o2.getLowerBound());
+ }
+ };
+
+ Collections.sort(rangePaletteEntries, comparator);
+ Collections.sort(singleValuePaletteEntries, comparator);
Review comment:
```
rangePaletteEntries.sort(comparator);
singleValuePaletteEntries.sort(comparator);
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-625807262
Documentation for the TIFF Floating Point format and predictor (predictor 3) is given in http://chriscox.org/TIFFTN3d1.pdf
The document mentions interleaved formats, which I assume might be used to store complex numbers or vectors. However, I did not implement them because I was unable to find examples. It also mentions some specialized 16-bit and 24-bit floating-point formats. Again, I was not able to find examples (and I think the use of these formats to be unlikely on modern hardware).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30673958)
Coverage increased (+0.7%) to 75.862% when pulling **ad55d50114d6fe09b69b4423d32a91652ea144fe on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30242570)
Coverage increased (+0.7%) to 75.862% when pulling **cc7318674fbace8ebd8a821b7f16ddedf40ce69c on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-629740729
>possible issues for security (those N/0, or loading values from the data without checking boundaries, etc)
Nothing obvious. A fuzzer could still find some images that could lead to exceptions, but I couldn't find a case where it would lead to DDoS attacks (:crossed_fingers:)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] coveralls edited a comment on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
coveralls edited a comment on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-610094475
[](https://coveralls.io/builds/30790905)
Coverage increased (+0.8%) to 75.883% when pulling **ac1ef5220eb4b113b93402ea52ed70f15901565a on gwlucastrig:master** into **1397ca92cd3268b434ad8a18529ee3544bbcf3c5 on apache:master**.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r422657886
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/ImageDataReader.java
##########
@@ -14,6 +14,96 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
+
+ /*
+ * Implementation Notes:
+ *
+ * Additional implementation notes are given in:
+ * DataReaderStrips.java
+ * DataReaderTiled.java
+ *
+ * The TIFF Floating-Point Formats ----------------------------------
+ * In addition to providing images, TIFF files can supply data in the
+ * form of numerical values. As of March 2020 the Commons Imaging library
+ * was extended to support some floating-point data formats.
+ * Unfortunately, the floating-point format allows for a lot of different
+ * variations and only the most widely used of these are currently supported.
+ * At the time of implementation, only a small set of data products were
+ * available. Thus it is likely that developers will wish to extend this capability
+ * as additional test data become available. When implementing extensions
+ * to this logic, developers are reminder that image processing requires
+ * access to literally millions of pixels, so attention to performance
+ * is essential to a successful implementation (please see the notes in
+ * DataReaderStrips.java for more information).
+ * The TIFF floating-point implementation is very poorly documented.
+ * So these notes are included to provide clarification on at least
+ * some aspects of the format.
+ *
+ * The Predictor==3 Case
+ * TIFF specifies an extension for a predictor that is intended to
+ * improve data compression ratios for floating-point values. This
+ * predictor is specified using the TIFF predictor TAG with a value of 3
+ * (see TIFF Technical Note 3, April 8, 2005). Consider a 4-byte floating
+ * point value given in IEEE-754 format. Let f3 be the high-order byte,
+ * with f2 the next highest, followed by f1, and f0 for the
+ * low-order byte. This designation shoulod not be confused with the
+ * in-memory layout of the bytes (little-endian versus big-endian), but
+ * rather their numerical values. The sign bit and upper 7 bits of the exponent
+ * are given in the high-order byte, followed by the remaining sign bit
+ * and the mantissa in the lower.
+ * In many real-valued raster data sets, the sign and magnitude (exponent)
+ * of the values changes slowly which the contents of the mantissa vary in
+ * a semi-random manner, with the information entropy tending to increase
+ * in the lowest ordered bytes. Thus, the high-order bytes have more
+ * redundancy than the low-order bytes and can compress more efficiently.
+ * To exploit this, the TIFF format splits the bytes into groups based on their
+ * order-of-magnitude. This splitting process takes place on a ROW-BY-ROW
+ * basis (note the emphasis, this point is not clearly documented in the spec).
+ * . For example, for row length of 3 pixels -- A, B, and C -- the data
+ * for two rows would be given as shown below (again, ignoring endian issues):
+ * Original:
+ * A3 A2 A1 A0 B3 B2 B1 B0 C3 C2 C1 C0
+ * D3 D3 D1 D0 E3 E2 E2 E0 F3 F2 F1 F0
+ *
+ * Bytes split into groups by order-of-magnitude:
+ * A3 B3 C3 A2 B2 C2 A1 B1 C1 A0 B0 C0
+ * D3 E3 F3 D2 E2 F2 D1 E1 F1 D0 E0 F0
+ *
+ * To further improve the compression, the predictor takes the difference of
+ * each subsequent bytes. Again, the differences (deltas) are computed on
+ * a row-byte-row basis. For the most part, the differences combine
+ * bytes associated with the same order-of-magnitude, though there is
+ * a special transition at the end of each order-of-magnitude set (shown in
+ * parentheses):
+ *
+ * A3, B3-A3, C3-B3, (A2-C3), B2-A2, C2-B2, (A1-C2), etc.
+ * D3, E3-D3, F3-D3, (D2-F3), E3-D2, etc.
+ *
+ * Once the predictor transform is complete, the data is stored using
+ * conventional data compression techniques such as Deflate or LZW.
+ * In practice, floating point data does not compress especially well, but
+ * using the above technique, the TIFF process typically reduces the overall
+ * storage size by 20 to 30 percent (depending on the data).
+ * The TIFF Technical Note 3 specifies 3 data size formats for
+ * storing floating point values:
+ * 32 bits IEEE-754 single-precision standard
+ * 16 bits IEEE-754 half-precision standard
+ * 24 bits A non-standard representation
+ * At this time, we have not obtained data samples for the smaller
+ * representations used in combination with a predictor.
+ *
+ * Interleaved formats
+ * TIFF Technical Note 3 also provides sample code for interleaved
+ * data, such as a real-valued vector or a complex pair. At this time
+ * no samples of interleaved data were available. As a caveat, the specification
+ * that the document provides has disadvantages in terms of code complexity
+ * and performance. Because the interleaved evaluation is embedded inside
+ * the pixel row and column loops, it puts a lot of redundant conditional
+ * evaluations inside the double nested loops. It is recommended that when
+ * interleaved data is implemented, it should get their own block of code
+ * so as not to interfere with the more common non-interleaved floating-point
+ * processing.
+ */
Review comment:
Just double checking... Is your preference that this be included in the general class Javadoc? Alternatively, it could go into either the body of the unpackFloatingPointSamples as either a standard comment or as Javadoc. WHich do you think is best?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-628883600
Excellent Gary! I have time to review it again this weekend. I think it should be ready to merge now if I find nothing else in the review
Should we delay mergimg until you finish your tests?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] kinow commented on a change in pull request #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
kinow commented on a change in pull request #72:
URL: https://github.com/apache/commons-imaging/pull/72#discussion_r426122131
##########
File path: src/main/java/org/apache/commons/imaging/formats/tiff/photometricinterpreters/floatingpoint/IPaletteEntry.java
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.commons.imaging.formats.tiff.photometricinterpreters.floatingpoint;
+
+import java.awt.Color;
+
+/**
+ * Defines an interface for specifying color assignments to floating point
+ * values.
+ */
+public interface IPaletteEntry {
Review comment:
No need to have the `I` in interface names (there were components using `I` and `A`—for abstract— but that changed long ago).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-imaging] gwlucastrig commented on issue #72: IMAGING-251 support for TIFF floating-point formats
Posted by GitBox <gi...@apache.org>.
gwlucastrig commented on issue #72:
URL: https://github.com/apache/commons-imaging/pull/72#issuecomment-616913125
I thought I should point out one aspect of the new API elements that I think is not optimal. When I added the getFloatingPointRasterData() method to the TiffDirectory class, I had it take ByteOrder as an argument. I took this approach because the existing "getTiffImage" methods did the same thing. But as I look at the unit tests and example applications that use it, it seems awkward to make the code handle a data element that is extraneous to the task that it is trying to accomplish. It's just one extra thing for a developer to have to keep track of and doesn't give him any added capability.
I've done some research and studied the TIFF specification, and it's clear that byte order is only specified one place in a TIFF file and doesn't change once the header section of the file is read. So I think a better approach to the new functionality would be to make the TiffDirectory class have byte-order as a member element that was populated by the constructors. Then the getFloatingPointRasterData() would not need it as an argument.
Since I've already said that I've "finished" this Pull Request (pending your review), I won't burden you with more changes... But I may submit the change as a future PR, if you think it a good idea.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org