sksamuel / scrimage

JVM - Java, Kotlin, Scala image processing library
https://sksamuel.github.io/scrimage
Apache License 2.0
1.05k stars 140 forks source link

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

Open jmiettinen opened 8 months ago

jmiettinen commented 8 months ago

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

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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

sksamuel commented 7 months ago

Awesome

sksamuel commented 5 months ago

How are things looking on this @jmiettinen

stale[bot] commented 3 months ago

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.