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

Improve JPEG writing performance when there's transparency in the source image #282

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jmiettinen
Copy link

I was looking into memory usage of some of the services we run at work and there ImmutableImage#removeTransparency ended up allocating surprisingly lot.
This affect performance in two ways: needing more GC to be run and needing to create the objects (Pixel) themselves.

I first removed the creation of Pixel[] in various method and replaced it with a streaming approach but then realized that while this takes allocations down, performance still leaves something to consider: operating pixel by pixel means that to get ARGB values of the image, we'll need to, for each pixel, consult color model.

In the end, it's better to use Graphics2D which is meant for operations like this.
However, using it changes results. My understanding is that there's a difference in how alpha compositing is done in AWT and what we're doing in MutableImage#replaceTransparencyInPlace.

Tasks this PR consists of

  • Write a simple test case for the affected functionality.
  • Write a JMH test case for the affected functionality. JpegWriterTest uses the results from before I started as a gold standard.
  • Decide if the new results of transparency removal are correct.
  • Based on the above step, remove potentially vestigial parts such as PixelConsumer, PixelUtils.replaceTransparencyWithColor or whole ImmutableImageTest.

Test case "JpegWriterTest.jpeg writer replaces transparent with white" fails now because of the alpha compositing issue.

Fileresults_fyi.md records some performance effects of the changes.

@sksamuel
Copy link
Owner

sksamuel commented Nov 4, 2023

However, using it changes results. My understanding is that there's a difference in how alpha compositing is done in AWT and what we're doing in MutableImage#replaceTransparencyInPlace.

What's the change ?

@jmiettinen
Copy link
Author

As described shortly here, we get different results for ARGB -> RGB transformation.

So ARBG values of (165, 173, 0, 17) was earlier transformed into (201, 90, 101) and now into (202, 90, 101).

That's why the test is failing as it's comparing that we'll get exactly the same pixels and after JPEG compression, this change one intensity level change turns into a slightly bigger change.

My guess for the reason would be that JDK uses floating-point-based math and PixelTools.replaceTransparencyWithColor uses integer math.

@jmiettinen
Copy link
Author

So what could I do to get this forward?

Would comparing results of transparency removal to, say, Imagemagick's results make sense?

My understanding of the difference we observe in this PR is that PixelTools.replaceTransparencyWithColor uses integer math and thus truncating "rounding" whereas JDK-method uses floating point rounding where 201.9x gets rounded to 202 when it earlier was rounded (truncated) to 201.

@sksamuel
Copy link
Owner

I think we should align with the JDK then. If you want to make a PR to change this I will make a release. @jmiettinen

@jmiettinen
Copy link
Author

I can do it, probably in the next few days of time.

@sksamuel
Copy link
Owner

Awesome

@sksamuel
Copy link
Owner

How are things looking on this @jmiettinen

Copy link

stale bot commented Mar 17, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Mar 17, 2024
@sksamuel sksamuel closed this Apr 23, 2024
@sksamuel sksamuel reopened this Jun 3, 2024
@stale stale bot removed the wontfix label Jun 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants