-
-
Notifications
You must be signed in to change notification settings - Fork 855
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
Simplify jpeg encoder and fix for Windows ARM #1304
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1304 +/- ##
==========================================
- Coverage 82.77% 82.76% -0.01%
==========================================
Files 689 689
Lines 30733 30721 -12
Branches 3473 3473
==========================================
- Hits 25439 25427 -12
Misses 4587 4587
Partials 707 707
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.
Can we do a before/after benchmark comparison for EncodeJpeg, with an image different than Car.bmp
?
I suggest re-encoding jpeg420exif.jpg
, but config can be Config.ShortCore31
to get it done faster.
#if NETSTANDARD2_0 | ||
// See https://github.com/SixLabors/ImageSharp/issues/1275 | ||
this.R = source.R; | ||
this.G = source.G; | ||
this.B = source.B; | ||
#else | ||
this = source.Rgb; | ||
#endif | ||
} |
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.
Very nice! 🎉
Remember someone talking about opening a dotnet/runtime issue on twitter, if it exists we should provide this info.
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 didn't open one. (I always struggle to provide a cut down solution to help them). Defo wise though.
I suggest re-encoding jpeg420exif.jpg, but config can be Config.ShortCore31 to get it done faster. @antonfirsov I'm not all that fussed if we lose a percentile or two here. We need to create a hardware accelerated version of our transforms anyway. |
I'm afraid it might be more, that's why I suggest an extensive benchmark run. Concerned about |
We use it in a few places in the decoder, hence why I chose to reuse it as a known path. Anyway.... Here's the before and after results with Before
After
Actually I'm wrong. 4:4:4 was slower just then. Will have to profile to determine the difference. |
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.
The numbers look fine. I guess that's 420, hope it doesn't regress 444 either.
Prerequisites
Description
Fixes #1275
Unsafe
API usage