Grayscale Jpeg encoding #1586
Grayscale Jpeg encoding #1586
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #1586 +/- ##
==========================================
+ Coverage 83.65% 83.69% +0.04%
==========================================
Files 746 747 +1
Lines 32991 33055 +64
Branches 3680 3692 +12
==========================================
+ Hits 27597 27665 +68
+ Misses 4680 4676 -4
Partials 714 714
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Great addition, thank you very much! A few remarks, otherwise looks good.
Can anyone in @SixLabors/core check if the [WithFile(TestImages.Png.BikeGrayscale, nameof(BitsPerPixel_Quality), PixelTypes.L8)] case works for them and produces output locally? I have some troubles with ImageMagic on my PC.
src/ImageSharp/Formats/Jpeg/Components/Encoder/L8ToYConverter.cs
Outdated
Show resolved
Hide resolved
I have tested it (on windows), it works fine, it produces a gray image of the bike testimage. @antonfirsov what problem do you see on your pc? |
|
@brianpopow don't mind it, must be local to my environment (+ want the PR review to stay on topic). Will report back on Slack if it will keep bothering me 😄 Thanks for checking! |
| { | ||
| if (this.ColorType == null) | ||
| { | ||
| bool isGrayscale = typeof(TPixel) == typeof(L8) || typeof(TPixel) == typeof(L16); |
There was a problem hiding this comment.
We should be storing the JpegColorType in the JpegMetadata class upon decode and querying both places with the encoder options taking precedence.
La16 and La32 should trigger grayscale also.
There was a problem hiding this comment.
Added La16 and La32 to JpegEncoder and added test cases for both.
There was a problem hiding this comment.
Unresolving the conversation, since I think @JimBobSquarePants also asked for a decoder extension here, will try to explain what he likely meant in more detailed manner:
- Define an optional
JpegColorType?property inJpegMetadata - Detect
JpegColorTypeinJpegDecoderCoreand store it into the image'sJpegMetadata. - In the encoder, also trigger the grayscale path in case if
JpegMetadata.JpegColorTypeis set toJpegColorType.GrayScale
@ynse01 let us know if you feel comfortable and confident adding this. If not, I recommend to not block the PR on this, and open a tracking issue for the addition.
There was a problem hiding this comment.
I gave it a try. Please indicate if this is what you guys expect.
|
Thank you for the great feedback. I have refactored my solution such, that the JpegSubsample.Grayscale option is no longer needed. I realized that it would have been hard to explain to users what it did. Please have another look. |
Prerequisites
Description
Implemented Grayscale JPEG encoding, as requested in issue #808 (first bullet).
This means, that for L8 or L16 pixel types, by default the JpegEncoder creates Grayscale, 1-component jpegs. This can also be forced upon other pixel type, by setting the ColorType property in the JpegEncoderOptions.
Grayscale Jpeg encoding performs better and results in a smaller file size.
Extended the JpegEncoderTests to cover Grayscale encoding and L8 pixel types also.