Skip to content
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

Improve performance of grayscale function and better clone standards #1844

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

MASACR99
Copy link

@MASACR99 MASACR99 commented Jan 18, 2023

Hey! I was working with your library and found out that the grayscale function was too slow, specially, compared to the examples you use, for example:
DynamicImage::ImageLuma8(image.into_luma8());

After some investigation I found the issue inside the grayscale function, when using the code above there's a call to the dynamic map function which works much faster than the imageops::grayscale generic function.

I also changed a few unneeded conversions to just clone since the type is already the same (in this case for example ImageLumaA8 was being converted with imageops even though it's not needed)

After those changes the DynamicImage grayscale function is now faster than the example code above.

Pictures proving the new grayscale is faster:

Old performance:
OldPerformance

New performance:
NewPerformance

Edit: Forgot to remove comment from license agreement

I license past and future contributions under the dual MIT/Apache-2.0 license,
allowing licensees to choose either at their option.

Comment on lines +635 to +637
DynamicImage::ImageRgb8(_) => dynamic_map!(*self, |ref p| DynamicImage::ImageLuma8(p.convert())),

DynamicImage::ImageRgba8(_) => dynamic_map!(*self, |ref p| DynamicImage::ImageLumaA8(p.convert())),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why the dynamic_map is useful here. The dynamic_map! macro just expands to a match statement exactly like the one we're currently in:

image/src/dynimage.rs

Lines 107 to 118 in 37d3de2

match $dynimage {
DynamicImage::ImageLuma8($image) => $action,
DynamicImage::ImageLumaA8($image) => $action,
DynamicImage::ImageRgb8($image) => $action,
DynamicImage::ImageRgba8($image) => $action,
DynamicImage::ImageLuma16($image) => $action,
DynamicImage::ImageLumaA16($image) => $action,
DynamicImage::ImageRgb16($image) => $action,
DynamicImage::ImageRgba16($image) => $action,
DynamicImage::ImageRgb32F($image) => $action,
DynamicImage::ImageRgba32F($image) => $action,
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure of why it's faster this way, I just looked at the difference between the grayscale function and into_luma8 and thought it could be a good idea to get it working faster (in fact for some reason it's even faster now with grayscale than into_luma8)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, just saw that I didn't remove the markdown comment to accept the license agreement, I edited the main comment to include it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect that this would be at least as fast as your dynamic_map! version:

DynamicImage::ImageRgba8(p) => DynamicImage::ImageLumaA8(p.convert()),

@fintelia
Copy link
Contributor

I'd feel better about this if we had some theory for why convert is faster than imageops::grayscale. Are we sure they're producing identical results?

@HeroicKatora
Copy link
Member

The conceptual difference: imageops::grayscale does an external loop over pixels, convert is the interior iteration version of this loop. Due imageops being generic it can't statically know the best iteration strategy for the layout, nor can it communicate the same bounds checks to the compiler. It's not very surprising to me.

@MASACR99
Copy link
Author

@fintelia Just ran a few tests to check that the resulting images of grayscale are correct. They are! I'll add a few images of the result of the "new" function
grayscale_function_1
grayscale_function
grayscale_function

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants