-
-
Notifications
You must be signed in to change notification settings - Fork 875
Jpeg encoder complete rewrite #2120
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
Jpeg encoder complete rewrite #2120
Conversation
Need to write a ton of tests now.... |
MultiplyToAverage(sourceRow, averageMultiplier); | ||
|
||
// copy to the first 8 slots | ||
sourceRow.Slice(0, packedWidth).CopyTo(this.ColorBuffer.DangerousGetRowSpan(i / factors.Height)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is one of the reasons 420 subsampling encoding got slower - we absolutely need to move ready-to-encode color strides to the first 8 indices. This potentially can be resolved at the TPixel -> RGB conversion step - we can rearrange color strides before doing subsampling but it's a very tricky thing to do, I've decided to make this PR as small as possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Half way viewed (have to leave now, will continue later).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finished my review -- didn't look at test code (due to missing knowledge there).
Looks good 👍🏻
src/ImageSharp/Formats/Jpeg/Components/Encoder/ComponentProcessor.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs
Outdated
Show resolved
Hide resolved
src/ImageSharp/Formats/Jpeg/Components/Encoder/HuffmanScanEncoder.cs
Outdated
Show resolved
Hide resolved
I have not seen issue #2117 on mac yet, only on windows so far, but it could be related. I will restart the CI to see, if it was a fluke or always happens. |
Looks like it's not a fluke. I'll start writing tests this week for this PR and try to add debug checks for memory access, maybe it's related to some out of bounds index access. |
Finally had a moment to finish this:
|
Looks like New encoder architecture mirrors decoder architecture. It should allow us to write encoding expansions like optimized huffman table generation or progressive encoding. P.S. |
Very exciting! Will review this asap. |
Looks like I've messed up linked issues, here's the complete list of what's resolved by this PR:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks awesome. Thanks again for such a fantastic addition to the library!
Prerequisites
New architecture
Jpeg encoder now have a brand new architecture which allows us to define encoding 'configs' like this:
Which allow us to define any allowed subsampling setup or even a 'custom' scenario with any number of components if user needs to. This API is internal for now - user can only choose encoding mode via
enum
which we had a long time ago. Maybe one day we can make it public - who knows.Due to this change: #1713 is resolved and #1476 is resolved.
It would also be fairly easy to implement optimized huffman tables or progressive encoding with current architecture.
API changes
3. I've removedYcck
encoding simply because it's not popular and we didn't support it anyway (we still can decodeYccK
though). This issue is partially done: #808Performance
Encoder got a bit slower but I think new architecture's worth it - we were really fast before, now we are still fast enough.
Main branch
PR