[
https://issues.apache.org/jira/browse/IMAGING-310?focusedWorklogId=650680&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-650680
]
ASF GitHub Bot logged work on IMAGING-310:
------------------------------------------
Author: ASF GitHub Bot
Created on: 14/Sep/21 16:36
Start Date: 14/Sep/21 16:36
Worklog Time Spent: 10m
Work Description: 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]
Issue Time Tracking
-------------------
Worklog Id: (was: 650680)
Time Spent: 1h 10m (was: 1h)
> JpegImageParser: Grayscale JPEG file with app14Segment returns
> ColorType.UNKNOWN
> --------------------------------------------------------------------------------
>
> Key: IMAGING-310
> URL: https://issues.apache.org/jira/browse/IMAGING-310
> Project: Commons Imaging
> Issue Type: Bug
> Components: Format: JPEG
> Affects Versions: 1.0-alpha2
> Reporter: John Phillips
> Assignee: Bruno P. Kinoshita
> Priority: Major
> Fix For: 1.0-alpha3
>
> Attachments: sample-grayscale-with-app14segment.jpg
>
> Time Spent: 1h 10m
> Remaining Estimate: 0h
>
> In org.apache.commons.imaging.formats.jpeg. the logic to determine colorType
> (lines 825-970) does not account for a Grayscale image when there is an app14
> segment present.
> When the number of components is 1 or 2, the colorType should always be
> Grayscale. None of the other logic would change that.
> It seems to me that the switch (numberOfComponents) statement should be moved
> to the top of this bit of code, and the rest of the logic should handled
> under Case 3 & 4:
> {noformat}
> // See
> http://docs.oracle.com/javase/6/docs/api/javax/imageio/metadata/doc-files/jpeg_metadata.html#color
> ImageInfo.ColorType colorType = ImageInfo.ColorType.UNKNOWN;
> 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()) {
> etc.
> {noformat}
> Attached sample-grayscale-with-app14segment.jpg, which can be used to
> demonstrate the issue.
> I've modified the code locally, but I'm a newbie at submitting
> pull-requests....
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
