Skip to content
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.

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

8012229: [lcms] Improve performance of color conversion for images with alpha channel #3239

Closed
wants to merge 3 commits into from

Conversation

@mrserb
Copy link
Member

@mrserb mrserb commented Mar 29, 2021

The lcms 2.8 version added support for the cmsFLAGS_COPY_ALPHA flag:
https://sourceforge.net/p/lcms/mailman/lcms-user/thread/001c01d1c0f3%2484a7cb30%248df76190%24%40ProDigitalSoftware.com/#msg35143398

which supports alpha channel transportation

For our usage, it means we can use TYPE_4BYTE_ABGR and TYPE_INT_ARGB directly as a source and destination.

Some numbers:
converting new BufferedImage(3000, 3000, TYPE_INT_ARGB) from CS_sRGB to CS_CIEXYZ via op.filter(src, dst).

//before the fix
//Benchmark                  Mode  Cnt    Score    Error  Units
//ColorAlphaConv.testFilter  avgt    5  603,392 ± 12,932  ms/op
//after the fix
//Benchmark                  Mode  Cnt    Score   Error  Units
//ColorAlphaConv.testFilter  avgt    5  121,624 ± 0,906  ms/op

But still slower than kcms

//kcms jdk8u60
//Benchmark                  Mode  Cnt   Score   Error  Units
//ColorAlphaConv.testFilter  avgt    5  13,256 ± 0,169  ms/op


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8012229: [lcms] Improve performance of color conversion for images with alpha channel

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/3239/head:pull/3239
$ git checkout pull/3239

Update a local copy of the PR:
$ git checkout pull/3239
$ git pull https://git.openjdk.java.net/jdk pull/3239/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3239

View PR using the GUI difftool:
$ git pr show -t 3239

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/3239.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Mar 29, 2021

👋 Welcome back serb! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Mar 29, 2021

@mrserb The following label will be automatically applied to this pull request:

  • 2d

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the 2d label Mar 29, 2021
// is it feature or bug?
return dst.getColorModel().hasAlpha() == src.getColorModel().hasAlpha();
}

Copy link
Member Author

@mrserb mrserb Mar 29, 2021

Thoughts about the method above:

  • I have left the first check as-is, so for any opaque destinations we will pass data to the lcms directly, but I am not sure that this is correct for source/destination with premultiplied alpha.
  • Since I am not sure about premultiplied alpha I have excluded it for the current fix if the destination has an alpha channel.

@@ -190,7 +190,7 @@ JNIEXPORT jlong JNICALL Java_sun_java2d_cmm_lcms_LCMS_createNativeTransform
}

sTrans = cmsCreateMultiprofileTransform(iccArray, j,
inFormatter, outFormatter, renderType, 0);
inFormatter, outFormatter, renderType, cmsFLAGS_COPY_ALPHA);
Copy link
Member Author

@mrserb mrserb Mar 29, 2021

Does not affect performance if the image does not have alpha.

// It is unclear how to hangle pre colors in the opaque DST
//opaqueDst(TYPE_INT_ARGB_PRE, TYPE_4BYTE_ABGR_PRE);
//opaqueDst(TYPE_4BYTE_ABGR_PRE, TYPE_INT_BGR);
}
Copy link
Member Author

@mrserb mrserb Mar 29, 2021

Actually, according to the spec, we should transform pre-alpha to alpha -> then apply ICC transform -> then transform pre-alpha-> alpha.
See class description of the:
https://docs.oracle.com/javase/7/docs/api/java/awt/image/ColorConvertOp.html

But it looks like implementation does something different.

@mrserb mrserb marked this pull request as ready for review Mar 29, 2021
@openjdk openjdk bot added the rfr label Mar 29, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Mar 29, 2021

Webrevs

@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2021

@mrserb This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8012229: [lcms] Improve performance of color conversion for images with alpha channel

Reviewed-by: azvegint

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Mar 31, 2021
@mrserb
Copy link
Member Author

@mrserb mrserb commented Mar 31, 2021

/integrate

@openjdk openjdk bot closed this Mar 31, 2021
@openjdk openjdk bot added integrated and removed ready labels Mar 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Mar 31, 2021

@mrserb Pushed as commit 16acfaf.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr label Mar 31, 2021
@mrserb mrserb deleted the JDK-8012229 branch Apr 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants