-
-
Notifications
You must be signed in to change notification settings - Fork 888
Webp: Use byte arrays instead of Dictionary's for lookups #1800
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1800 +/- ##
==========================================
+ Coverage 87.05% 87.30% +0.25%
==========================================
Files 936 936
Lines 47638 47832 +194
Branches 6017 6009 -8
==========================================
+ Hits 41469 41761 +292
+ Misses 5177 5082 -95
+ Partials 992 989 -3
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah much better! It'd be nice to document what each table provides but not a requirement.
| p[offset - step3] = WebpLookupTables.Clip1[p2 + a3 + 255]; | ||
| p[offset - step2] = WebpLookupTables.Clip1[p1 + a2 + 255]; | ||
| p[offsetMinusStep] = WebpLookupTables.Clip1[p0 + a1 + 255]; | ||
| p[offset] = WebpLookupTables.Clip1[q0 - a1 + 255]; | ||
| p[offset + step] = WebpLookupTables.Clip1[q1 - a2 + 255]; | ||
| p[offset + step2] = WebpLookupTables.Clip1[q2 - a3 + 255]; |
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.
Bit late to the party but:
Do we always shift with the same value for the same lookup? Shouldn't we define a helper method then or at least add an explanatory 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.
Bit late to the party but
Maybe I was a bit to quick in merging it.
Do we always shift with the same value for the same lookup?
yes, in C it is defined like this:
const uint8_t* const VP8kclip1 = &clip1[255];
See dec_clip_tables.c in the libwebp source.
I thought there might be a way to do the same in C#, but i could not come up with a better solution then this.
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.
I will create a follow up PR with helper methods
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.
Maybe you could use a Span that starts at the 255 offset?
Prerequisites
Description
During profiling I noticed alot of time with lossy decoding is spend in System.Collections. Turns out using Dictionary's for lookup tables was not the best idea. I have changed that now to byte array's. it reduces the lossy decoding time from 645.7 ms to 280.8 ms.
Before:
After: