Jpeg quantization metadata #1706
Conversation
…sics (not tested)
br3aker
left a comment
br3aker
left a comment
There was a problem hiding this comment.
Have some questions about internal API
tests/ImageSharp.Tests/Formats/Jpg/JpegDecoderTests.Metadata.cs
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #1706 +/- ##
==========================================
- Coverage 84.41% 84.36% -0.05%
==========================================
Files 822 822
Lines 36063 36022 -41
Branches 4207 4204 -3
==========================================
- Hits 30443 30391 -52
- Misses 4802 4816 +14
+ Partials 818 815 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
@JimBobSquarePants @antonfirsov Did some research on estimator and it works very well actually predicting matrices produced by the encoder: LuminanceChrominanceThose are implemented as disabled tests which outputs exactly these results so it can be checked in the future if for whatever reason estimator would be changed or when transition to 16-bit ints is done. Summary While Luma has max variance of So my proposal for variance thresholds is: We can leave those values as they are even considering we store those tables as floats. Internally estimator uses casts to int so we should be fine. Why quality < 25 isn't present in the tests? Tests show that |
gfoidl
left a comment
gfoidl
left a comment
There was a problem hiding this comment.
A micro-optimization hint.
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
To determine the variance thresholds, it seems worth to check how far do the variance values go with non-standard tables (I guess they are not slightly altered standard ones?). Maybe it's worth to keep the thresholds higher?
|
Looks like I was commenting on outdated code. Exposing |
|
@br3aker keeping |
|
@antonfirsov I would like to say there is but I can't. There were no requests of such functionality. We could mimic JPEGsnoop functionality but JPEGsnoop already exists so yeah, it's better to hide everything and expose general |
| /// Note that jpeg image can have different quality for luminance and chrominance components. | ||
| /// This property return average for both qualities and sets both qualities to the given value. | ||
| /// </remarks> | ||
| public int Quality |
There was a problem hiding this comment.
Would be a breaking change, but wouldn't it be better to define this as int? to better communicate the LuminanceQuality == null && LuminanceQuality == null case?
There was a problem hiding this comment.
Oops, forgot about it being int? in current state. Will work on it tomorrow with no public luma/chroma in mind.
There was a problem hiding this comment.
No it's public int Quality on the current master. I think we should consider changing it to int?, I doubt there are many users we would break.
There was a problem hiding this comment.
Yep. Break it. We cannot afford to hold onto properties that cause complexity. We simply don't have the maintainer numbers to manage continuously backwards compatible codebase.
I also vote max or we never really fix anything due to loss of detail/extra noise.
There was a problem hiding this comment.
Got ya, thanks for your thoughts!
There was a problem hiding this comment.
[Bit late to the party, since changes are already pushed but ...]
- Custom tables
We need to optimize to our most popular use case, which is thumbnail making. Assuming the user operation is a significant downscale, is the re-encoded thumbnail Jpeg "more correct" with the original tables or the default ones? I can't decide, such a downscale changes the frequency characteristics of the image a lot.
Sidenote: If I was to develop a thumbnail app, I would always set the Quality of the thumbnail to a higher value, even if it was low originally. Does ImageSharp.Web do anything like this @JimBobSquarePants?
intorint?
To me metadata.Quality == null would represent the case that the decoder was unable to estimate the quality of the input image nicely, while returning metadata.Quality == 75 is quite an arbitrary choice. I vote for the breaking change (just let's make sure to apply the label).
There was a problem hiding this comment.
We need to optimize to our most popular use case, which is thumbnail making. Assuming the user operation is a significant downscale, is the re-encoded thumbnail Jpeg "more correct" with the original tables or the default ones?
Barely noticable for 2x downsampling. So we have following situation:
Default tables:
good re-encoding with minimal change
good encoding of a heavily changed image
Original tables:
even better re-encoding with mininal change
good encoding of a heavily changed image
To me
metadata.Quality == nullwould represent the case that the decoder was unable to estimate the quality of the input image nicely, while returningmetadata.Quality == 75is quite an arbitrary choice. I vote for the breaking change (just let's make sure to apply the label).
Not sure if we are talking about the same thing. metadata.Quality is not used during encoding process. Internal metadata.LuminanceQuality & metadata.ChrominanceQuality fields are used. Using quality = max(luma, chroma) for encoding would kill the quality if we are not talking about something extreme like thumbnails. Public quality is strictly a user way of knowing what was the decoded image quality. metadata.Quality setter sets luma & chroma to the same value. In other words, internal implementation won't really change and we either do (simplified, do that for both luma and chroma qualities):
int quality = encoder.Quality ?? metadata.Quality ?? 75; or int quality = encoder.Quality ?? metadata.Quality;
There was a problem hiding this comment.
Ok @br3aker . After a thorough read of your quality determination logic I agree with your findings and find the refactoring of the various Quality properties to be good. The single metadata property information is useful for individuals who want to use a custom encoding quality based upon decoded information.
I don't think we should bother preserving the existing tables though. It's highly unlikely that individuals will decode an image for processing without significant change to the input pixels and most thumbnailing operations are closer to 10x reduction than 2x.
There was a problem hiding this comment.
@JimBobSquarePants thanks for the review! I might miss something but it's not breaking, no? We ended up leaving Quality property as int. Or it's all about two private properties in the metadata?
Agreed with table preservation. Did a bit of internal testing w/ and w/o using original tables, re-encoding without any modifications produces images with difference in size for about 0.1%. There is a binary difference but minutes of inspection with photoshop magnification tells that I can't spot it by eye.
What's still unresolved is quality < 25: new current implementation can't say for sure what actual quality jpeg has if it was encoded with quality less than 25. But I don't think many people would ever work with such jpegs and even if they would - they should save it as png to save as much detail as possible (or even legacy Jpeg XL re-encoding sometime in the future). So yep, tables bring a little to no difference, especially if any modification is involved.
There was a problem hiding this comment.
@br3aker Ah sorry, my bad! I could have sworn I'd seen a change but I was wrong!
Thanks for doing the extra research. I agree with you on quality < 25. I really cannot see anyone doing that for anything other than experimental purpose.
|
Update
I guess it's ready for final review. |
|
@JimBobSquarePants as I suspected, langver 9.0 broke something, I'm just too newbie to determine what. Problem is so deeply nested and actually doesn't pop without At least we know for sure that we need to rollback to 8.0 or whatever version was used. |
|
@br3aker Sorry about all this flip-flopping on this PR. I'll do a proper sit-down review ASAP for you! That |
|
@JimBobSquarePants having my code reviewed by .net professionals is worth the wait. I understand that you have a lot of work aside this project so no worries, take your time! About that |
Haha. No pressure! I'll get on this tomorrow night. |
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
@br3aker If you're happy with this then I'm happy. Thanks again for your help and patience!
| /// Note that jpeg image can have different quality for luminance and chrominance components. | ||
| /// This property return average for both qualities and sets both qualities to the given value. | ||
| /// </remarks> | ||
| public int Quality |
There was a problem hiding this comment.
Ok @br3aker . After a thorough read of your quality determination logic I agree with your findings and find the refactoring of the various Quality properties to be good. The single metadata property information is useful for individuals who want to use a custom encoding quality based upon decoded information.
I don't think we should bother preserving the existing tables though. It's highly unlikely that individuals will decode an image for processing without significant change to the input pixels and most thumbnailing operations are closer to 10x reduction than 2x.
5 participants
Prerequisites
Description
fixes #1705
Color deduction is improved, tested on some images - results are promising!
Moved everything quantization table-wise to the
Quantization.csImage at #845 had invalid test 'quality', it should have been rounded, not ceiled, set it from 99 to 98 for now.
TODO
nintusage