[
https://issues.apache.org/jira/browse/IMAGING-310?focusedWorklogId=655833&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-655833
]
ASF GitHub Bot logged work on IMAGING-310:
------------------------------------------
Author: ASF GitHub Bot
Created on: 27/Sep/21 21:47
Start Date: 27/Sep/21 21:47
Worklog Time Spent: 10m
Work Description: kinow commented on a change in pull request #163:
URL: https://github.com/apache/commons-imaging/pull/163#discussion_r717069346
##########
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:
Hi @jephillips34
>Bruno, thank you for reviewing this.
>Bruno, I forgot to thank you for writing and setting up the test case.
Thank **you** for raising the issue and providing a PR. It's extremely
helpful.
>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.
:+1:
>However, that would leave us with 3 separate blocks of code, all checking
for one or two components, and setting those to grayscale.
Yeah, I preferred your version, but I think a unit test failed, and then
that led me to reading the specs to remind me of why there was that `if
(jfifSegment != null)` block. Maybe we could find a way to accommodate the fix
for the issue, and also improve the code.
>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.
We need to handle this case:
>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.
So if we are able to keep the existing logic, have all unit tests to pass,
then we can definitely simplify the code.
At the moment I believe we have three cases for choosing the color type in
the `JpegImageParser`. The first being when you have an App14 segment and it's
an Adobe JPEG. The color type is chosen using the Adobe color transform from
that segment.
The second handles the case when a JFIF segment is present, using just the
number of components.
The third and final case has special logic for choosing color type & alpha
based on number of components, SOFn segments, etc.
From a quick look this morning, I think there 1st case needs to stay the way
it's, as it's related to the Adobe JPEG. The second and third cases are a bit
more similar, but if you look at the switch/if statements, it's different how
each case is handling three components for instance.
So I am not sure if we would be able to have 1 block handling Adobe JPEG,
JFIF, and the last case.
But feel free to update this PR with code if you have any suggestions. It
might be easier to discuss with some example code to see how we can improve
(IMO one big improvement would be split that method into smaller methods,
making it easier to distinguish these cases of Adobe, JFIF, etc.)
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]
Issue Time Tracking
-------------------
Worklog Id: (was: 655833)
Time Spent: 1.5h (was: 1h 20m)
> 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: 1.5h
> 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)
