-
Notifications
You must be signed in to change notification settings - Fork 143
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
Scaffolding for direct benchmarking of crate::filter::unfilter
.
#413
Conversation
That looks great! Feel free to file any issues or let me know if you see any cases where performance is worse than you'd expect. |
FWIW, I assume that the |
I don't think it's related (but we ran into similar). I believe mips was demoted to tier 3, so might not have up to date toolchains |
Yeah, the mips failure is unrelated. In the main image crate we switched to powerpc for big endian testing (image-rs/image#1987), so probably just need to do the same thing here. I should hopefully be able to take a look at the rest of the PR later tonight |
src/common.rs
Outdated
@@ -695,6 +687,18 @@ impl Info<'_> { | |||
} | |||
|
|||
impl BytesPerPixel { | |||
pub(crate) fn for_prediction(bpp: usize) -> Self { |
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.
Could you call this from_usize
or similar?
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.
Done. I assume that you plan to squash the last 2 small commits (cargo fmt
+ this rename) when merging?
Really excited to see the progress to enable using SIMD from safe Rust code! Avoiding, or ideally forbidding, unsafe code is one of the big differentiators that we can provide over the C libraries that are generally used for image decoding. But that's often meant that we either have to forgo potential performance improvements or tolerate some unsafe in order to use SIMD |
Can you PTAL? The new benchmarks from this PR should help with work on performance improvements of
fn unfilter
.The real motivation for this PR is that it is a prelude to a series of commits at bf2c26b...1c78a3f that results in a significant decoding speed-up (measured on Intel). Special thanks to @veluca93 for help with various SIMD questions that I had. Results that I got from running the benchmarks:
decode/kodim02.png
: -26.6% timedecode/kodim17.png
: -25.7% timedecode/kodim07.png
: -27.1% timedecode/kodim23.png
: -21.8% timeunfilter/filter=Sub/bpp=3
: -71.5% timeunfilter/filter=Sub/bpp=6
: -86.1% timeunfilter/filter=Avg/bpp=3
: -71.8% timeunfilter/filter=Avg/bpp=6
: -70.6% timeunfilter/filter=Paeth/bpp=3
: -51.4% timeunfilter/filter=Paeth/bpp=6
: -62.9% timedecode/...
andunfilter/...
benchmarks are unchangedI note that the speed-up above depends on
std::simd
which hasn't yet been stabilized, so I thought that it would be best to have a separate PR for just the benchmarks. (i.e. the new benchmarks seem desirable on their own, even if we decide not to pursue thestd::simd
-based improvements)/cc @calebzulawski as FYI, since the improvements are possible thanks to
std::simd
and rust-lang/rust#86656.