jephillips34 commented on a change in pull request #163:
URL: https://github.com/apache/commons-imaging/pull/163#discussion_r708447620
##########
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:
Bruno, thank you for reviewing this.
I agree that your much smaller change, simply adding the check for only one
component inside the handling of ADOBE_COLOR_TRANSFORM_UNKNOWN would allow the
colorType of my test image to be assigned correctly as grayscale. It would not
cover the case of a 2 component Grayscale image with alpha channel, but that
could be easily added as an additional condition in that same code block.
However, that would leave us with 3 separate blocks of code, all checking for
one or two components, and setting those to grayscale.
I thought it would be more correct to pull that common element out of those
three code blocks, and only leave the special-case appSegment handling for the
3- and 4-component cases, where they might actually affect the colorType
determination.
Also, it just makes sense to me to first look at number of components, given
the nature of the Grayscale colorType....by definition, an image with only one
color component is Grayscale...it could not be anything else, regardless of any
appSegment details.
I did leave the check for one component in the jfifSegment code block,
though I realize that the condition is now impossible, and I would be agreeable
to removing that.
What do you think?
-John
--
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]
