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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
