-
-
Notifications
You must be signed in to change notification settings - Fork 887
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.
gfoidl
left a comment
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).
gfoidl
left a comment
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
|
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:
|
JimBobSquarePants
left a comment
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
enumwhich 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 removedYcckencoding simply because it's not popular and we didn't support it anyway (we still can decodeYccKthough). 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