Repository navigation
Jpeg 12 bit support - #784
Conversation
|
|
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Great effort here, really appreciated!
A couple of minor changes required plus you're failing the Stylecop checks so if you can get those fixed we're rocking.
| public void NormalizeColorsInplace() | ||
| public void NormalizeColorsInplace(float maximum) | ||
| { | ||
| Vector4 CMin4 = new Vector4(0F); |
There was a problem hiding this comment.
These values are being calculated more than once in the same partial struct.
There was a problem hiding this comment.
What we can do is to refactor the normalization logic into it's own stateful type (preferably a struct), where minimum/maximum/half values are nonstatic members initialized in the constructor.
| private float MaximumValue { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets the maximum value of a sample |
|
It occurs to me that this adds decoding support not encoding so the image data will be lost. We should get the work done to allow supporting encoding also. |
antonfirsov
left a comment
antonfirsov
left a comment
There was a problem hiding this comment.
@feliwir good job, and thanks again for this improvement!
Just a quick initial web review, will clone the code later this week. Still left a few imrovement requests in comments.
Question:
- Is it possible to have 12 bit precision jpegs for colorspaces other than YCbCr? (eg. CMYK, YCCK, RGB)
- If yes, can we get some test images for these cases. adding them to our test suite?
| public void NormalizeColorsInplace() | ||
| public void NormalizeColorsInplace(float maximum) | ||
| { | ||
| Vector4 CMin4 = new Vector4(0F); |
There was a problem hiding this comment.
What we can do is to refactor the normalization logic into it's own stateful type (preferably a struct), where minimum/maximum/half values are nonstatic members initialized in the constructor.
| { | ||
| public FromCmyk() | ||
| : base(JpegColorSpace.Cmyk) | ||
| public FromCmyk(int precision) |
There was a problem hiding this comment.
I prefer to have more type safety in the codebase. (-1, 17 and 222 are invalid values for precision ;) ). I suggest an enum having the following form, so you can easily cast it to int when necessary.
enum JpegPrecision
{
EightBits = 8,
TwelveBits = 12
}There was a problem hiding this comment.
Hmmm... Read section 4.7 here. Lossless jpeg supports additional precision so we should bear that in mind when naming our enum. https://www.w3.org/Graphics/JPEG/itu-t81.pdf
For DCT-based processes, two alternative sample precisions are specified: either 8 bits or 12 bits per sample. Applications which use samples with other precisions can use either 8-bit or 12-bit precision by shifting their source image samples appropriately. The baseline process uses only 8-bit precision. DCT-based implementations which handle 12-bit source image samples are likely to need greater computational resources than those which handle only 8-bit source images. Consequently in this Specification separate normative requirements are defined for 8-bit and 12-bit DCT-based processes.
For lossless processes the sample precision is specified to be from 2 to 16 bits
There was a problem hiding this comment.
Oops. The options I see:
- Keep the type as is (
int), and introduce someDebugGuardchecks in the code - Introduce an enum defining all entries for 2-16
- Since we do not support lossless jpeg, define only
EightBitsandTwelveBitsfor now, and YAGNI out the rest, letting our future selves to find out how to manage the lossless case.
I'm pretty much for option 3.
| { | ||
| ValidateRgbToYCbCrConversion( | ||
| new JpegColorConverter.FromYCbCrBasic(), | ||
| new JpegColorConverter.FromYCbCrBasic(8), |
There was a problem hiding this comment.
We should add tests for 12 bit precision case.
Yes, when 12bit support was added to the jpeg specification it covered all color spaces. On that note we cannot preserve that color data when saving the jpeg. We'll need to add the equivalent colorspace transformation code to the encoder with any additional requirements. |
|
@JimBobSquarePants then we should find or generate 12 bit test jpegs for all the colorspaces, the feature is only partially tested otherwise! |
|
@antonfirsov Just spent the last 20 mins scouring the internet for an official source of test images and I've come up with nothing. libjpeg-turbo's repo contains 1 12bit file that doesn't appear to be supported by anything I have on my machine. https://github.com/libjpeg-turbo/libjpeg-turbo/tree/master/testimages |
|
Ok, this means that those aren't real use cases on pure statistical basis, so we can leave them untested. Standard meets reality. |
|
@feliwir @antonfirsov I had some time so I updated everything in the branch, removed some of the Linq stuff from the color converter and merged the reference image. We're getting very close to having this ready to merge 🎉 Question though. Shall we had encoding 12bit jpegs to this PR or open a new Issue/PR. We'll need to create optimized color conversion methods to handle this. |
|
We could add encoding, but not sure if i could help with that too much :D |
|
@antonfirsov When you have a chance can you please conduct a final review? |
|
I the improvement opportunities I mentioned are still there (introducing an enum for 8bit/12bit, test cases for 12 bit color conversion), but let's just merge this as-is, and do the improvements later! |
|
Merged. Thanks @feliwir ! |
Jpeg 12 bit support
4 participants
Description
libjpegandlibjpeg-turbo