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

Quality behaving differently from reference encoder #53

Closed
jojje opened this issue Aug 4, 2024 · 3 comments · Fixed by #54
Closed

Quality behaving differently from reference encoder #53

jojje opened this issue Aug 4, 2024 · 3 comments · Fixed by #54
Assignees
Labels
bug Something isn't working Rust Something about Rust side bindings

Comments

@jojje
Copy link

jojje commented Aug 4, 2024

In the later releases (and latest: v1.2.5) the interpretation of quality seems to have changed.
As far as I recall, earlier releases followed the reference encoder's interpretation more or less.

When looking at the jpegxl-rs documentation it seems that library is using distance rather than quality, but calling the variable quality still. Those two concepts are completely distinct. The latter requires a very specific transformation to the former. Could this explain the divergent behavior from the reference implementation that I observe below for your plugin?

Right now, with quality set to 90 for some random test image, I get the following encoding statistics for your plugin vs the reference encoder:

Quality: 90

Effort 1
  plugin    [size: 40267440, duration: 2.402]
  reference [size: 13734293, duration: 2.710]

Effort 2
  plugin    [size: 40267440, duration: 2.394]
  reference [size: 13734293, duration: 2.714]

Effort 3
  plugin    [size: 37676149, duration: 2.429]
  reference [size: 12893847, duration: 2.815]

Effort 4
  plugin    [size: 37803418, duration: 2.541]
  reference [size: 13350005, duration: 3.044]

Effort 5
  plugin    [size: 26061484, duration: 3.516]
  reference [size: 12424286, duration: 4.807]

Effort 6
  plugin    [size: 24412418, duration: 3.964]
  reference [size: 12201280, duration: 5.452]

Effort 7
  plugin    [size: 28384223, duration: 12.607]
  reference [size: 12206682, duration: 5.540]

The test script so you can reproduce the observation:

import subprocess
import shlex
import sys
from time import time
from io import BytesIO

from PIL import Image
import pillow_jxl

def encode_plugin(filename, quality, effort):
    bio = BytesIO()
    t = time()
    Image.open(filename).save(bio, format='jxl', quality=quality, effort=effort)
    d = time() - t
    return len(bio.getvalue()), d

def encode_cli(filename, quality, effort):
    cmd = f'cjxl -q {quality} -e {effort} "{filename}" -'
    t = time()
    p = subprocess.run(shlex.split(cmd), check=True, capture_output=True)
    d = time() - t
    return len(p.stdout), d

filename = sys.argv[1]
quality = 90

print('Quality:', quality)
for effort in range(1,8):
    size_plugin, time_plugin = encode_plugin(filename, quality, effort)
    size_refenc, time_refenc = encode_cli(filename, quality, effort)
    print(f'\nEffort {effort}')
    print(f'  plugin    [size: {size_plugin}, duration: {time_plugin:.3f}]')
    print(f'  reference [size: {size_refenc}, duration: {time_refenc:.3f}]')

Versions used:

  • python 3.10.13 (windows 64 bit)
  • pillow-jxl-plugin: 1.2.5
  • Pillow: 10.0.1
  • Reference encoder (cjxl): v0.10.3 4a3b22d [AVX2,SSE2]
@jojje jojje changed the title Quality behaving differently than reference encoder Quality behaving differently from reference encoder Aug 4, 2024
@Isotr0py
Copy link
Owner

Isotr0py commented Aug 5, 2024

Yes, the quality used in jpegxl-rs encoder is the distance (which varies from 0..25) in fact. And we map jpeg quality to distance at python side in earlier version (#29)

However, jpegxl-rs updated a set_jpeg_quality method to map jpeg quality to distance through the libjxl C-API JxlEncoderDistanceFromQuality in v0.10.4. We migrate to this implementation in v1.2.5, and I think there may be something wrong in migration which causes the divergence.

I will have a check at the Rust side implementation for this.

@Isotr0py Isotr0py self-assigned this Aug 5, 2024
@Isotr0py Isotr0py added bug Something isn't working Rust Something about Rust side bindings labels Aug 5, 2024
@Isotr0py
Copy link
Owner

Isotr0py commented Aug 5, 2024

OK, I found what is the cause. The issue is the default value of use_container and use_original_profile, which we set it true in plugin by default, while cjxl set it false.

Plugin

use_container = info.get("use_container", True)
use_original_profile = info.get("use_original_profile", True)

cjxl

 --container=0|1
    0 = Avoid the container format unless it is needed (default)
    1 = Force using the container format even if it is not needed.

Benchmarks after setting use_container=False:

Quality: 98

Effort 1
  plugin    [size: 4600335, duration: 1.322]
  reference [size: 4600228, duration: 1.364]

Effort 2
  plugin    [size: 4600335, duration: 1.391]
  reference [size: 4600228, duration: 1.363]

Effort 3
  plugin    [size: 2786269, duration: 1.264]
  reference [size: 2786162, duration: 1.277]

Effort 4
  plugin    [size: 2839899, duration: 1.317]
  reference [size: 2839792, duration: 1.578]

Effort 5
  plugin    [size: 3522876, duration: 1.736]
  reference [size: 3522769, duration: 1.953]

Effort 6
  plugin    [size: 3524057, duration: 1.928]
  reference [size: 3523950, duration: 2.041]

Effort 7
  plugin    [size: 3638554, duration: 1.927]
  reference [size: 3638447, duration: 2.088]

I will fix this and clarify the default behavior of the plugin.

@jojje
Copy link
Author

jojje commented Aug 5, 2024

Awesome. Will upgrade as soon as you've released a new version.
Thanks for the swift investigation.

There was no urgency, since I could fall back on the reference encoder, but it's nice to not having to round-trip to a subprocess when using Pillow and numpy. Fewer moving parts :)

PS. Very strange that the container had such a large effect. Afaik the storage overhead for the container is just a few bytes unless something besides the bitstream is placed in the container; like gigantic metadata boxes. If you disable the container, keep in mind that you'll probably break the code you have that handles metadata transfer. That's why it's noted as "when not needed" in the reference implementation, since if anything other than the bitstream needs to be stored, a container is required as the bitstream itself has no place for such information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Rust Something about Rust side bindings
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants