Skip to content

8364583: ColorConvertOp fails for CMYK → RGB conversion #27785

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

8364583: ColorConvertOp fails for CMYK → RGB conversion #27785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/java.desktop/share/classes/java/awt/image/ColorConvertOp.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ private BufferedImage nonICCBIFilter(BufferedImage src,
idx = 0;
for (int x = 0; x < w; x++) {
pixel = srcRas.getDataElements(x, y, pixel);
color = srcCM.getNormalizedComponents(pixel, color, 0);
color = srcCM.getNormalizedComponents(pixel, null, 0);
Copy link
Member

@mrserb mrserb Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is executed for each pixel, it will generate garbage equal to the image size. Maybe we can cleanly split the usage of the two vars here? Note that the bug only occurs when the source image does not use an ICC profile, but this change would increase garbage for both ICC and non-ICC profiles.

Copy link
Contributor Author

@prrace prrace Oct 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean keep the original returned float[] color separate from the one that's later over-written.
I'd thought about that but I don't think it is worth it.
It can also be over-written at line 835. There's just too much state tracking needed to save
the GC a tiny bit of effort freeing small transient objects.
Also it already is re-initialised for each scanline. See line 807, so it isn't (quite) the same array for the entire image anyway.

Copy link
Member

@mrserb mrserb Oct 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can also be over-written at line 835.

Interesting, is it possible that that line has the same bug?
color = dstColorSpace.fromCIEXYZ(dstColor);
Does dstColor always have the same number of components as CIEXYZ?

Is the logic of using CIEXYZ for mix of non-/ICC source and non-/ICC destination actually correct?

Copy link
Contributor Author

@prrace prrace Oct 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fromCIEXYZ is defined on ColorSpace, not ICC_ColorSpace.

It requires 3 (or more) components, and then dstColorSpace will return an array of colors in its own colorspace.
The dstColor parameter is always created with at least 3 components based on the dstNumComp
And if there's no bug in dstColorSpace it should return an array of the right length for itself.
So if there's a bug it isn't obvious to me.

Copy link
Member

@mrserb mrserb Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems the destination handles it properly; I tested it with the following example:

import java.awt.Transparency;
import java.awt.color.ColorSpace;
import java.awt.image.BufferedImage;
import java.awt.image.ColorConvertOp;
import java.awt.image.ComponentColorModel;
import java.awt.image.DataBuffer;
import java.awt.image.WritableRaster;

import static java.awt.color.ColorSpace.TYPE_2CLR;
import static java.awt.color.ColorSpace.TYPE_FCLR;
import static java.awt.color.ColorSpace.TYPE_GRAY;

public final class TestCCP {

    private static final int WIDTH = 10;
    private static final int HEIGHT = 10;

    private static class CustomColorSpace extends ColorSpace {
        private final int numComponents;

        private CustomColorSpace(int type, int numComponents) {
            super(type, numComponents);
            this.numComponents = numComponents;
        }

        @Override
        public float[] toRGB(float[] colorvalue) {
            return new float[3];
        }

        @Override
        public float[] fromRGB(float[] rgbvalue) {
            return new float[numComponents];
        }

        @Override
        public float[] toCIEXYZ(float[] colorvalue) {
            return new float[3];
        }

        @Override
        public float[] fromCIEXYZ(float[] colorvalue) {
            return new float[numComponents];
        }
    }

    private static final ColorSpace[] CS = {
            ColorSpace.getInstance(ColorSpace.CS_CIEXYZ),
            ColorSpace.getInstance(ColorSpace.CS_GRAY),
            ColorSpace.getInstance(ColorSpace.CS_LINEAR_RGB),
            ColorSpace.getInstance(ColorSpace.CS_PYCC),
            ColorSpace.getInstance(ColorSpace.CS_sRGB),
            new CustomColorSpace(TYPE_GRAY, 1),
            new CustomColorSpace(TYPE_2CLR, 2),
            new CustomColorSpace(TYPE_FCLR, 15)
    };

    public static void main(String[] args) {
        for (ColorSpace srcCS : CS) {
            for (ColorSpace fromCS : CS) {
                for (ColorSpace toCS : CS) {
                    for (ColorSpace dstCS : CS) {
                        BufferedImage src = createTestImage(srcCS);
                        BufferedImage dst = createTestImage(dstCS);

                        new ColorConvertOp(fromCS, toCS, null).filter(src, dst);
                    }
                }
            }
        }
    }

    private static BufferedImage createTestImage(ColorSpace cs) {
        ComponentColorModel cm = new ComponentColorModel(cs, false, false,
                                                         Transparency.OPAQUE,
                                                         DataBuffer.TYPE_BYTE);
        WritableRaster raster = cm.createCompatibleWritableRaster(WIDTH,
                                                                  HEIGHT);
        return new BufferedImage(cm, raster, cm.isAlphaPremultiplied(), null);
    }
}

Copy link
Contributor

@honkar-jdk honkar-jdk Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mrserb @prrace

Are we testing the else block - the one for "possible non-ICC src, possible CSList, possible non-ICC dst" with the test provided by Sergey?

Ln#872 in the else block has a similar line color = srcCM.getNormalizedComponents(spixel, color, 0); and the above test runs fine if normComponents is set to either color or null.

if (needSrcAlpha) {
alpha[x] = color[srcNumComp];
}
Expand Down
51 changes: 51 additions & 0 deletions test/jdk/java/awt/image/ColorConvertOp/ColorConvertOpCMYK.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* Copyright (c) 2025, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
* under the terms of the GNU General Public License version 2 only, as
* published by the Free Software Foundation.
*
* This code is distributed in the hope that it will be useful, but WITHOUT
* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
* version 2 for more details (a copy is included in the LICENSE file that
* accompanied this code).
*
* You should have received a copy of the GNU General Public License version
* 2 along with this work; if not, write to the Free Software Foundation,
* Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
*
* Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
* or visit www.oracle.com if you need additional information or have any
* questions.
*/
import java.io.File;

import java.awt.color.ColorSpace;
import java.awt.image.BufferedImage;
import java.awt.image.ColorConvertOp;
import java.awt.image.ColorModel;

import javax.imageio.ImageIO;

/*
* @test
* @bug 8364583
* @summary Verify CMYK images work with ColorConvertOp
*/

public class ColorConvertOpCMYK {

public static void main(String[] args) throws Exception {
String sep = System.getProperty("file.separator");
String dir = System.getProperty("test.src", ".");
String prefix = dir + sep;
File file = new File(prefix + "black_cmyk.jpg");
BufferedImage source = ImageIO.read(file);
ColorModel sourceModel = source.getColorModel();
ColorSpace cs = ColorSpace.getInstance(ColorSpace.CS_sRGB);
ColorConvertOp convertOp = new ColorConvertOp(cs, null);
BufferedImage rgb = convertOp.filter(source, null);
Copy link
Contributor

@prsadhuk prsadhuk Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rgb variable can be removed...

}
}
Binary file added test/jdk/java/awt/image/ColorConvertOp/black_cmyk.jpg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.