Repository navigation
Adds support for Tiff's with jpeg compression - #1734
Conversation
…e larger strip size
|
Is placing rgb as option to |
yeah separating subsampling and color space is a good suggestion |
|
Or we can just rename
As it was decided (?) that the next release would be a major one I assume we can break it. YCbCr can have other subsampling stuff like 422, 411, 410 even but I think it would be better to add them as options to 'ColorSpace' instead of 2 different options one of which would work only with YCbCr. |
That really sounds like the better solution, but I am undecided, if we really should introduce a breaking change for a Tiff related feature. |
| redBlock[i] = c.R; | ||
| greenBlock[i] = c.G; | ||
| blueBlock[i] = c.B; |
There was a problem hiding this comment.
Why no Unsafe.Add here? So it's a bound check on each access.
Together with the previous suggestion, this should produce quite nice machine code*.
* except the movsxd from the int-indexing, which can be avoided by nint / IntPtr but I can't remember where we landed with that as older runtimes won't support this without hacks (sorry).
There was a problem hiding this comment.
I think we can support nint, we just need to change our build process. For reasons I cannot recall we're building the code with explicit target frameworks which I can't see a reason to do since nobody ever does this. We only need to test with individual frameworks and build things normally.
There was a problem hiding this comment.
Just rereviewed this. redBlock etc are actually Block8x8F instances. There's no bounds checks there in release.
@gfoidl However, in the internal indexer we are creating the extra movsxd but I think we'll leave this as-is for now since passing nint would require a significant refactoring.
There was a problem hiding this comment.
Do you know if casting to nint is good?
(quote from the email GH sent me).
A direct cast
int a = 42;
nint b = (nint)a;
produces a movsxd. This can be avoided by casting to uint-first. So nint b = (nint)(uint)a;, then it's just a mov (which the cpu doesn't need to execute due register renaming, but it still needs to be fetched and decoded, so it hasn't zero-cost, but it's quite cheap).
So for the API one can keep int (to be CLS-compliant, etc.) and just perform the (nint)(uint) cast to get the best of both worlds.
Note: the JIT can't do this / isn't allowed to do this optimization for us, as int can have negative values. Here is a place where we can be smarter than the compiler, and say explicitely by the uint cast, that we only have >= 0 values.
Next week I'm out of office, but then I could have a skim over the indexers, etc. to try to remove the movsxd instructions.
There was a problem hiding this comment.
I didn’t think of that double cast when experimenting earlier. Great stuff, thanks!
Different color spaces for jpeg were in plans for quite a while now #808 so it's good I think :) This would also prepare the ground for remaining CMYK and YccK color spaces. This can be introduced as a new property in non-breaking manner:
Internal code would use new |
|
We don’t need to worry about breaking changes with Tiff at all. We haven’t shipped anything other than dev builds with it (and won’t) until 2.0 |
|
@JimBobSquarePants renaming/new |
|
@br3aker I don't follow. We already have |
|
@JimBobSquarePants my latest proposal was to remove the IMO it's more understandable to have a single parameter
While not removing parameter (marking it obsolete though) itself so it won't break anyone. |
|
I guess.... They're separate concerns but combining them makes sense in this instance.
We'd just make the breaking change. It's 2.0, it's also free software - People can recompile. |
|
Making breaking changes is not fun, I understand. But I believe it's for good here, possibility of Rgb with Ratio420 would be 'strange', to say the least :) |
|
Do you have any docs talking about subsampling with Rgb? I can't find anything to say anything other than YCbCr does it. |
Oh no, no, sorry for inconvenience. I meant right now it's possible to write something like: var encoder1 = new JpegEncoder
{
ColorType = JpegColorType.Luminance,
Subsample = JpegSubsample.Ratio420
};
// With this commit adding Rgb as option
var encoder2 = new JpegEncoder
{
ColorType = JpegColorType.Rgb,
Subsample = JpegSubsample.Ratio420
};Both of which make no sense as chroma sub-sampling only works with YCbCr as it clearly separates luminance from chrominance. Providing only color type would solve these. P.S. |
|
Ah right yeah, that all makes sense and I concur. Let’s fix it properly |
# Conflicts: # src/ImageSharp/Formats/Jpeg/JpegDecoderCore.cs
|
@brianpopow did a bit of testing with rgb stuff and actually solved it: First of all, actual solution: Explanation (?): RGB is not JFIF compatible 'format' - there's no rgb mentions in the itu specs. Standard JPEG with YCbCr is a JPEG FIF file - it must contain As far as I know rgb jpegs are more of an Adobe experiment over the actual jpeg format so it explains a lot. Some software (if not most) uses APP0 to determine if image is a standard jpeg and I would dare to say it is kind of wrong - while it's technically correct as there's no format specification for anything tl;dr drop APP0 marker, we should separate Second interesting thing I found - libjpeg-turbo (and old libjpeg I guess) uses standard jpeg luminance quantization table and luminance huffman table for all rgb channels. While writing different tables/huffman for each channel is okay as long as correct indices for them are provided - we already do that so it's fine. It' still better to use same quantization table for each channel though :P |
@br3aker thank you a lot for investigating this issue, its very helpful. I will try to change the encoder as you suggested. |
…icationHeader. Do not write WriteJfifApplicationHeader with RGB.
brianpopow
changed the title
# Conflicts: # tests/ImageSharp.Tests/TestImages.cs # tests/Images/Input/Tiff/Calliphora_ccitt_fax4.tiff
JimBobSquarePants
left a comment
JimBobSquarePants
left a comment
There was a problem hiding this comment.
Looks great; Nice work! 👍
| { | ||
| // Table identifiers. | ||
| ReadOnlySpan<byte> headers = new byte[] | ||
| ReadOnlySpan<byte> headers = stackalloc byte[] |
There was a problem hiding this comment.
Here stackalloc is actually a de-optimization.
See #1734 (comment) and https://vcsjones.dev/csharp-readonly-span-bytes-static/ for some background for this C#'s compiler optimization to refer to the static data directly without the need to allocate (either stack or heap) something.
This sharplab demonstrates the difference too.
Maybe it's best to annotate each place where we introduce that optimization with a comment like
// This uses a C#'s compiler optimization that refers to the static data segment of the assembly,
// and doesn't incur any allocation at all.to make it clear that the new byte[] is there intentional and doesn't allocate.
There was a problem hiding this comment.
I was under the impression that optimisation only worked for static instances?
There was a problem hiding this comment.
Just read the doco. Method body optimisation is a new one for me! I’ll revert
4 participants
Prerequisites
Description
This PR adds support for decoding and encoding of tiff's with jpeg compression. See adobe tech note 2 for details, spec pdf is in the tiff folder: ff8dfde
For this to work, I need to add a new method
LoadTablesto the JpegDecoderCore, which only loads quantization and/or Huffman tables. Those are only stored once in at tiff with jpeg compression, ifJPEGTablesare present in a Striped tiff (Tiff with pixel data is stored in multiple segments).Also the JpegEncoder needs to store the data as RGB instead of YCbCr.
There is now a new option fornote: this PR introduces additional enum values forJpegSubsampleto do that.JpegColorTypefor YCbCr with the subsampling rates and RGB (to encode the image as jpeg with RGB colorspace), which is a breaking change for the JpegEncoder optionsnote: #1732 is prerequisite for this PR