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

Disable libjxl lossless mode for float16 #17624

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kiwixz
Copy link
Contributor

@kiwixz kiwixz commented Oct 8, 2024

Closes #17487.

As discussed in the issue, libjxl lossless encoding of float16 is actually broken.
This patch disables the real lossless mode of libjxl, while still asking for a distance of 0 (basically lossless).

It didn't feel useful to mention this in tooltip of the quality slider.

@TurboGit TurboGit added this to the 5.0 milestone Oct 8, 2024
@TurboGit TurboGit added bugfix pull request fixing a bug priority: medium core features are degraded in a way that is still mostly usable, software stutters difficulty: trivial some changes in a couple of functions release notes: pending depends: external lib labels Oct 8, 2024
@kmilos
Copy link
Contributor

kmilos commented Oct 8, 2024

This patch disables the real lossless mode of libjxl, while still asking for a distance of 0 (basically lossless).

Although a neat workaround, not 100% sure this is the way to go, there might be other side effects of settings distance to 0 here (IIRC it's been mentioned that could be considerably worse than true lossless mode for file size and encoding time, especially if original color space is kept)? It'd be ideal if we just totally disable this for fp16...

@kiwixz
Copy link
Contributor Author

kiwixz commented Oct 8, 2024

Tested on the good old Debian logo:

quality color profile size
100 (distance 0) (original) 27K
100 original 19K
100 internal 11K
99 original 16K
99 internal 8.5K
98 original 15K
98 internal 7.8K

Looking at the pixels samples, the distance 0 is indeed closer to original.
I think we should give the best possible quality when user cranks the slider to the right, regardless of the size.
We would really give lossless if this was possible but it's not.

Now the next question could be: "Should lossless be optional for quality 100?", but that's out of scope for this PR.

@TurboGit
Copy link
Member

I feel that the discussion is not fully resolved, should this be accepted or a better way found? In other word what is the status of this PR?

@kmilos
Copy link
Contributor

kmilos commented Oct 21, 2024

"Should lossless be optional for quality 100?", but that's out of scope for this PR.

IMHO not, as that was the design premise and what the tooltip promised/promises...

Until this is fixed upstream, I'd still rather just totally skip fp16 at q=100/d=0 and limit this to q=99 (which is already ridiculously high anyway for the lossy mode)...

@kiwixz
Copy link
Contributor Author

kiwixz commented Oct 21, 2024

Until this is fixed upstream, I'd still rather just totally skip fp16 at q=100/d=0 and limit this to q=99 (which is already ridiculously high anyway for the lossy mode)...

It's sensible, but then we need to make a special case for quality=100 to specify uses_original_profile which makes float16 ever more diverging from all other formats... It's a lot of effort for an edge case that virtually nobody will encounter; upstream is plain broken and nobody really noticed.
On the other hand this trivial patch sets distance to 0 and you end up with a big file, but as you say q=99 is already ridiculously high anyway so I does not make sense to waste more time on this.

@TurboGit TurboGit added the wip pull request in making, tests and feedback needed label Oct 21, 2024
@kmilos
Copy link
Contributor

kmilos commented Oct 24, 2024

How about this?

  1. Let's wait w/ this until the code freeze, in the hope something happens upstream and we maybe at least get a new JPEGXL_NUMERIC_VERSION we can use to gate this hack with. If not, I'm ok(-ish) merging as is at that point (pragmatically better than spending effort on excluding this mode properly indeed, or reverting lossless float support altogether).
  2. In any case, we should add a line to known issues in the release notes, something like "Due to an upstream issue, exporting JPEG XL in 16-bit float at quality 100 (distance 0) is not truly/mathematically lossless [currently | when using libjxl <= 0.x.y]"

@TurboGit TurboGit modified the milestones: 5.0, 5.2 Nov 29, 2024
@kmilos
Copy link
Contributor

kmilos commented Nov 29, 2024

I don't think we can punt this to 5.2, it crashes dt currently?

@kiwixz
Copy link
Contributor Author

kiwixz commented Dec 2, 2024

I rebased on master and mentioned the problem in the release notes.

Considering there is no sign of life in the relevant issues upstream, I think we should just merge this now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix pull request fixing a bug depends: external lib difficulty: trivial some changes in a couple of functions priority: medium core features are degraded in a way that is still mostly usable, software stutters release notes: pending wip pull request in making, tests and feedback needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

jxl - check option combinations & bit depth
3 participants