kinow commented on a change in pull request #106:
URL: https://github.com/apache/commons-imaging/pull/106#discussion_r521287464
##########
File path: src/main/java/org/apache/commons/imaging/common/ImageBuilder.java
##########
@@ -161,17 +185,37 @@ public ImageBuilder getSubset(final int x, final int y,
final int w, final int h
}
if (x + w > width) {
throw new RasterFormatException(
- "subimage (x+width) is outside raster");
+ "subimage (x+width) is outside raster");
}
if (y < 0 || y >= height) {
throw new RasterFormatException("subimage y is outside raster");
}
if (y + h > height) {
throw new RasterFormatException(
- "subimage (y+height) is outside raster");
+ "subimage (y+height) is outside raster");
}
+ }
- ImageBuilder b = new ImageBuilder(w, h, hasAlpha);
+ /**
+ * Gets a subset of the ImageBuilder content using the specified parameters
+ * to indicate an area of interest. If the parameters specify a rectangular
+ * region that is not entirely contained within the bounds defined
+ * by the ImageBuilder, this method will throw a RasterFormatException.
+ * This run- time exception is consistent with the behavior of the
+ * getSubimage method provided by BufferedImage.
Review comment:
:+1: I thought we would adjust (reduce) the area, but good to be
consistent too.
##########
File path:
src/test/java/org/apache/commons/imaging/formats/tiff/TiffAlphaRoundTripTest.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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;
+
+import java.awt.AlphaComposite;
+import java.awt.Color;
+import java.awt.Graphics2D;
+import java.awt.image.BufferedImage;
+import java.io.File;
+import java.nio.file.Path;
+import java.util.HashMap;
+
+import org.apache.commons.imaging.ImageFormats;
+import org.apache.commons.imaging.Imaging;
+
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Performs a round-trip that writes an image containing Alpha and then reads
it
+ * back.
+ * Selected non-opaque pixels are tested for correctness,
+ */
+public class TiffAlphaRoundTripTest {
+
+ @TempDir
+ Path tempDir;
+
+ @Test
+ public void test() throws Exception {
+
+ // This test will exercise two passes to test the implementation
+ // of the TIFF support for writing and reading images containing
+ // an alpha channel. In the first pass, the alpha writing is enabled
+ // in the second pass it is suppressed.
Review comment:
:+1: very useful comments! Thanks!
##########
File path:
src/main/java/org/apache/commons/imaging/formats/tiff/constants/TiffTagConstants.java
##########
@@ -347,6 +347,8 @@
public static final TagInfoShorts TIFF_TAG_EXTRA_SAMPLES = new
TagInfoShorts(
"ExtraSamples", 0x152, -1,
TiffDirectoryType.TIFF_DIRECTORY_ROOT);
+ public static final int EXTRA_SAMPLE_ASSOCIATED_ALPHA = 1;
+ public static final int EXTRA_SAMPLE_UNASSOCIATED_ALPHA = 2;
Review comment:
I think we have now associated, unassociated, and pre-multiplied
(associated) used in the code. I didn't see straight. Is there any form that is
preferred @gwlucastrig ? Should we try to make it more uniform?
##########
File path: src/main/java/org/apache/commons/imaging/common/ImageBuilder.java
##########
@@ -68,17 +71,45 @@
* requirements for the ImageBuilder or resulting BufferedImage.
*/
public ImageBuilder(final int width, final int height, final boolean
hasAlpha) {
+ checkDimensions(width, height);
Review comment:
Great idea! Thanks!!! :+1:
##########
File path:
src/test/java/org/apache/commons/imaging/formats/tiff/TiffAlphaRoundTripTest.java
##########
@@ -0,0 +1,134 @@
+/*
+ * 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;
+
+import java.awt.AlphaComposite;
+import java.awt.Color;
+import java.awt.Graphics2D;
+import java.awt.image.BufferedImage;
+import java.io.File;
+import java.nio.file.Path;
+import java.util.HashMap;
+
+import org.apache.commons.imaging.ImageFormats;
+import org.apache.commons.imaging.Imaging;
+
+import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+import org.junit.jupiter.api.io.TempDir;
+
+/**
+ * Performs a round-trip that writes an image containing Alpha and then reads
it
+ * back.
+ * Selected non-opaque pixels are tested for correctness,
+ */
+public class TiffAlphaRoundTripTest {
+
+ @TempDir
+ Path tempDir;
+
+ @Test
+ public void test() throws Exception {
+
+ // This test will exercise two passes to test the implementation
+ // of the TIFF support for writing and reading images containing
+ // an alpha channel. In the first pass, the alpha writing is enabled
+ // in the second pass it is suppressed.
+ for (int i = 0; i < 2; i++) {
+ // Step 0, create a buffered image that includes transparency
+ // in the form of two rectangles, one completely opaque,
+ // and one giving 50 percent opaque red.
+ int width = 400;
+ int height = 400;
+ BufferedImage image0;
+ if (i == 0) {
+ image0 = new BufferedImage(width, height,
BufferedImage.TYPE_INT_ARGB);
+ } else {
+ image0 = new BufferedImage(width, height,
BufferedImage.TYPE_INT_RGB);
+ }
+ Graphics2D g2d = image0.createGraphics();
+ g2d.setColor(Color.red);
+ g2d.fillRect(0, 0, width, height);
+ g2d.setComposite(AlphaComposite.getInstance(AlphaComposite.SRC));
+ g2d.setColor(new Color(0, 0, 0, 0));
+ g2d.fillRect(100, 100, 100, 100);
+ g2d.setColor(new Color(0xff, 0, 0, 0x80));
+ g2d.fillRect(200, 200, 100, 100);
+
+ // Step 1: write the Buffered Image to an output file and
+ // then read it back in. This action will test the
+ // correctness of a round-trip test.
+ File file = new File(tempDir.toFile(),
"TiffAlphaRoundTripTest.tif");
+ file.delete();
+ HashMap<String, Object> params = new HashMap<>();
+
+ Imaging.writeImage(image0, file, ImageFormats.TIFF, params);
+ BufferedImage image1 = Imaging.getBufferedImage(file);
+
+ // Step 2: create a composite image overlaying a white background
+ // with the results from the TIFF file.
+ BufferedImage compImage
+ = new BufferedImage(width, height,
BufferedImage.TYPE_INT_ARGB);
+ g2d = compImage.createGraphics();
+ g2d.setColor(Color.white);
+ g2d.fillRect(0, 0, width, height);
+ g2d.drawImage(image1, 0, 0, null);
+
+ // Step 3, verify that the correct values are in the image.
+ int test1 = compImage.getRGB(150, 150); // in the transparent
rectangle
+ int test2 = compImage.getRGB(250, 250);
+ if (i == 0) {
+ doPixelsMatch(150, 150, 0xffffffff, test1);
+ doPixelsMatch(250, 250, 0xffff7f7f, test2);
+ } else {
+ doPixelsMatch(151, 151, 0xff000000, test1);
+ doPixelsMatch(251, 251, 0xffff0000, test2);
+ }
+ }
+ }
+
+ void doPixelsMatch(int x, int y, int a, int b) {
+ if (!componentMatch(a, b, 0, 2)
+ || !componentMatch(a, b, 8, 2)
+ || !componentMatch(a, b, 16, 2)
+ || !componentMatch(a, b, 24, 2)) {
+
+ String complaint = String.format("Pixel mismatch at (%d,%d):
0x%08x 0x%08x",
+ x, y, a, b);
+ fail(complaint);
+ }
+ }
+
+ /**
+ * Checks to see if a pixel component (A, R, G, or B) for two specified
+ * values are within a specified tolerance.
+ *
+ * @param a the first value
+ * @param b the second value
+ * @param iShift a multiple of 8 telling how far to shift values
+ * to extract components (24, 16, 8, or zero for ARGB)
+ * @param iTolerance a small positive integer
+ * @return true if the components of the values match
+ */
+ boolean componentMatch(int a, int b, int iShift, int iTolerance) {
Review comment:
Methods like this could even be grouped later in a JUnit rule, or some
utility classes :+1:
##########
File path:
src/main/java/org/apache/commons/imaging/formats/tiff/datareaders/DataReaderTiled.java
##########
@@ -141,32 +141,37 @@ private void interpretTile(final ImageBuilder
imageBuilder, final byte[] bytes,
// the tile is padded to beyond the tile width
j1 = xLimit;
}
- if (predictor == 2) {
- // pre-apply the predictor logic before feeding
- // the bytes to the photometric interpretor.
+
+ if (predictor ==
TiffTagConstants.PREDICTOR_VALUE_HORIZONTAL_DIFFERENCING) {
+ applyPredictorToBlock(tileWidth, i1 - i0, samplesPerPixel,
bytes);
+ }
+
+ if (bitsPerPixel == 24) {
+ // 24 bit case, we don't mask the red byte because any
+ // sign-extended bits get covered by opacity mask
for (int i = i0; i < i1; i++) {
int k = (i - i0) * tileWidth * 3;
- int p0 = bytes[k++] & 0xff;
- int p1 = bytes[k++] & 0xff;
- int p2 = bytes[k++] & 0xff;
- for (int j = 1; j < tileWidth; j++) {
- p0 = (bytes[k] + p0) & 0xff;
- bytes[k++] = (byte) p0;
- p1 = (bytes[k] + p1) & 0xff;
- bytes[k++] = (byte) p1;
- p2 = (bytes[k] + p2) & 0xff;
- bytes[k++] = (byte) p2;
+ for (int j = j0; j < j1; j++, k += 3) {
+ final int rgb = 0xff000000
+ | (bytes[k] << 16)
+ | ((bytes[k + 1] & 0xff) << 8)
+ | (bytes[k + 2] & 0xff);
+ imageBuilder.setRGB(j, i, rgb);
}
}
- }
-
- for (int i = i0; i < i1; i++) {
- int k = (i - i0) * tileWidth * 3;
- for (int j = j0; j < j1; j++, k += 3) {
- final int rgb = 0xff000000
- | (((bytes[k] << 8) | (bytes[k + 1] & 0xff)) << 8)
- | (bytes[k + 2] & 0xff);
- imageBuilder.setRGB(j, i, rgb);
+ } else if (bitsPerPixel == 32) {
+ // 32 bit case, we don't mask the high byte because any
+ // sign-extended bits get shifted up and out of result.
+ for (int i = i0; i < i1; i++) {
+ int k = (i - i0) * tileWidth * 4;
+ for (int j = j0; j < j1; j++, k += 4) {
+ final int rgb
+ = ((bytes[k] & 0xff) << 16)
+ | ((bytes[k + 1] & 0xff) << 8)
+ | (bytes[k + 2] & 0xff)
+ | (bytes[k + 3] << 24);
+ imageBuilder.setRGB(j, i, rgb);
+ }
Review comment:
The change in `DataReaderStrips.java ` and here is quite similar.
Probably more code duplication that could be unified/simplified later? WDYT
@gwlucastrig (you worked on the code recently, so you might have a better idea)?
##########
File path:
src/main/java/org/apache/commons/imaging/formats/tiff/write/TiffImageWriterBase.java
##########
@@ -287,7 +330,20 @@ public void writeImage(final BufferedImage src, final
OutputStream os, Map<Strin
final int width = src.getWidth();
final int height = src.getHeight();
- int compression = TIFF_COMPRESSION_LZW; // LZW is default
+ final ColorModel cModel = src.getColorModel();
+ final boolean hasAlpha = cModel.hasAlpha() && checkForActualAlpha(src);
Review comment:
Initially I thought maybe we didn't have to check for actual alpha in
the image, since the colour model was already defining that it has alpha.
But this is for writing images, so it should be used just once...
@gwlucastrig worst case scenario I guess would be a super large file, with no
alpha. In that case We end up reading the whole image in memory (in chunks, but
nevertheless...).
Do you think this could be a problem in the future? I'm fine with this
change for now. If anybody reports any slowness we can just use a parameter to
control whether this check is performed or not :man_shrugging: WDYT?
##########
File path: src/main/java/org/apache/commons/imaging/common/ImageBuilder.java
##########
@@ -242,16 +266,29 @@ private BufferedImage makeBufferedImage(
WritableRaster raster;
final DataBufferInt buffer = new DataBufferInt(argb, w * h);
if (useAlpha) {
- colorModel = new DirectColorModel(32, 0x00ff0000, 0x0000ff00,
- 0x000000ff, 0xff000000);
- raster = Raster.createPackedRaster(buffer, w, h,
- w, new int[]{0x00ff0000, 0x0000ff00, 0x000000ff,
- 0xff000000}, null);
+ colorModel = new DirectColorModel(
+ ColorSpace.getInstance(ColorSpace.CS_sRGB),
Review comment:
Why is it sRGB? I couldn't see in the diff if we had it before. But why
couldn't it be just `TYPE_RGB` or some other type?
----------------------------------------------------------------
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:
[email protected]
