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 2021/09/13 00:30:25 UTC
[GitHub] [commons-imaging] kinow commented on a change in pull request #163: IMAGING-310 JpegImageParser.getImageInfo() Refactor colorType logic to first look at numberOfComponents
kinow commented on a change in pull request #163:
URL: https://github.com/apache/commons-imaging/pull/163#discussion_r706933392
##########
File path: src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
##########
@@ -824,44 +824,44 @@ public ImageInfo getImageInfo(final ByteSource byteSource, final Map<String, Obj
// See http://docs.oracle.com/javase/6/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#color
ImageInfo.ColorType colorType = ImageInfo.ColorType.UNKNOWN;
- // Some images have both JFIF/APP0 and APP14.
- // JFIF is meant to win but in them APP14 is clearly right, so make it win.
- if (app14Segment != null && app14Segment.isAdobeJpegSegment()) {
- final int colorTransform = app14Segment.getAdobeColorTransform();
- switch (colorTransform) {
- case App14Segment.ADOBE_COLOR_TRANSFORM_UNKNOWN:
- if (numberOfComponents == 3) {
- colorType = ImageInfo.ColorType.RGB;
- } else if (numberOfComponents == 4) {
- colorType = ImageInfo.ColorType.CMYK;
+ switch (numberOfComponents) {
+ case 1:
+ colorType = ImageInfo.ColorType.GRAYSCALE;
+ break;
+ case 2:
+ colorType = ImageInfo.ColorType.GRAYSCALE;
+ transparent = true;
+ break;
+ case 3:
+ case 4:
+ // Some images have both JFIF/APP0 and APP14.
+ // JFIF is meant to win but in them APP14 is clearly right, so make it win.
+ if (app14Segment != null && app14Segment.isAdobeJpegSegment()) {
+ final int colorTransform = app14Segment.getAdobeColorTransform();
+ switch (colorTransform) {
+ case App14Segment.ADOBE_COLOR_TRANSFORM_UNKNOWN:
+ if (numberOfComponents == 3) {
+ colorType = ImageInfo.ColorType.RGB;
+ } else if (numberOfComponents == 4) {
+ colorType = ImageInfo.ColorType.CMYK;
+ }
+ break;
+ case App14Segment.ADOBE_COLOR_TRANSFORM_YCbCr:
+ colorType = ImageInfo.ColorType.YCbCr;
+ break;
+ case App14Segment.ADOBE_COLOR_TRANSFORM_YCCK:
+ colorType = ImageInfo.ColorType.YCCK;
+ break;
+ default:
+ break;
}
- break;
- case App14Segment.ADOBE_COLOR_TRANSFORM_YCbCr:
- colorType = ImageInfo.ColorType.YCbCr;
- break;
- case App14Segment.ADOBE_COLOR_TRANSFORM_YCCK:
- colorType = ImageInfo.ColorType.YCCK;
- break;
- default:
- break;
- }
- } else if (jfifSegment != null) {
- if (numberOfComponents == 1) {
- colorType = ImageInfo.ColorType.GRAYSCALE;
- } else if (numberOfComponents == 3) {
- colorType = ImageInfo.ColorType.YCbCr;
- }
- } else {
- switch (numberOfComponents) {
- case 1:
- colorType = ImageInfo.ColorType.GRAYSCALE;
- break;
- case 2:
- colorType = ImageInfo.ColorType.GRAYSCALE;
- transparent = true;
- break;
- case 3:
- case 4:
+ } else if (jfifSegment != null) {
+ if (numberOfComponents == 1) {
Review comment:
This condition is now impossible since we are in the switch-case for `numberOfComponents == 3 | 4`.
I think the previous logic was closer to the documentation from [Oracle on Java/colors metadata](https://docs.oracle.com/javase/6/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#color):
> If a JFIF APP0 marker segment is present, the colorspace is known to be either grayscale or YCbCr. If an APP2 marker segment containing an embedded ICC profile is also present, then the YCbCr is converted to RGB according to the formulas given in the JFIF spec, and the ICC profile is assumed to refer to the resulting RGB space.
@jephillips34 what about this fix (diff from `master`)?
```diff
diff --git a/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java b/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
index 288c2398..1cc0fcae 100644
--- a/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
+++ b/src/main/java/org/apache/commons/imaging/formats/jpeg/JpegImageParser.java
@@ -834,6 +834,8 @@ public class JpegImageParser extends ImageParser implements XmpEmbeddable {
colorType = ImageInfo.ColorType.RGB;
} else if (numberOfComponents == 4) {
colorType = ImageInfo.ColorType.CMYK;
+ } else if (numberOfComponents == 1) {
+ colorType = ImageInfo.ColorType.GRAYSCALE;
}
break;
case App14Segment.ADOBE_COLOR_TRANSFORM_YCbCr:
diff --git a/src/test/java/org/apache/commons/imaging/formats/jpeg/specific/JpegImageParserTest.java b/src/test/java/org/apache/commons/imaging/formats/jpeg/specific/JpegImageParserTest.java
index 3f86af4d..b2ec1d52 100644
--- a/src/test/java/org/apache/commons/imaging/formats/jpeg/specific/JpegImageParserTest.java
+++ b/src/test/java/org/apache/commons/imaging/formats/jpeg/specific/JpegImageParserTest.java
@@ -22,7 +22,9 @@ import java.awt.image.BufferedImage;
import java.io.File;
import java.io.IOException;
+import org.apache.commons.imaging.ImageInfo;
import org.apache.commons.imaging.ImageReadException;
+import org.apache.commons.imaging.common.bytesource.ByteSource;
import org.apache.commons.imaging.common.bytesource.ByteSourceFile;
import org.apache.commons.imaging.formats.jpeg.JpegImageParser;
import org.apache.commons.imaging.formats.jpeg.decoder.JpegDecoderTest;
@@ -49,4 +51,18 @@ public class JpegImageParserTest {
assertEquals(-16777216, image.getRGB(0, 0));
assertEquals(-12177367, image.getRGB(198, 13));
}
+
+ /**
+ * For IMAGING-310. Verify that a JPEG image, with an APP14 (Adobe segment) present,
+ * and 1 component must return the image color type as grayscale.
+ */
+ @Test
+ public void testGrayscaleImageWithApp14Segment() throws IOException, ImageReadException {
+ final File imageFile = new File(
+ JpegDecoderTest.class.getResource("/images/jpeg/IMAGING-310/sample-grayscale-with-app14segment.jpg")
+ .getFile());
+ final JpegImageParser parser = new JpegImageParser();
+ ImageInfo imageInfo = parser.getImageInfo(new ByteSourceFile(imageFile));
+ assertEquals(ImageInfo.ColorType.GRAYSCALE, imageInfo.getColorType());
+ }
}
```
The test uses the image from your JIRA issue. The test fails without the patch above. If you are happy with this, feel free to update this branch and it should be ready to be merged :+1:
Thanks!
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.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org