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

Faster color conversions #651

Merged
merged 4 commits into from
Feb 19, 2022

Conversation

rfuest
Copy link
Member

@rfuest rfuest commented Feb 19, 2022

This PR changes the implementation of the color conversion to be more efficient when no hardware divider is available. The division is replaced by a cheap right shift. It is as fast as the LUT based implementation mentioned in #650, but doesn't need to store any tables in flash.

Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

Tiny nit, otherwise looks good. Nice use of all that const magic!

I went to run a benchmark for this and realised we don't have any for the colour conversion stuff. It shouldn't block this PR, but perhaps worth considering now or later? At any rate, how does it compare to the timings in #650?

core/src/pixelcolor/conversion.rs Outdated Show resolved Hide resolved
Co-authored-by: James Waples <james@wapl.es>
@rfuest
Copy link
Member Author

rfuest commented Feb 19, 2022

I went to run a benchmark for this and realised we don't have any for the colour conversion stuff. It shouldn't block this PR, but perhaps worth considering now or later?

IMO this would be something that needs to run on real target hardware. A beefy desktop CPU won't have a big problem dealing with some additional divisions.

At any rate, how does it compare to the timings in #650?

On my Cortex M0 test setup the benchmark result is within a few milliseconds of the LUT based implementation. But the LUT based approach might have advantages on platforms with less than 32 bit word width, no hardware multiplier and/or no barrel shifter. At the moment I think the color conversions are now fast enough until someone reports otherwise.

@rfuest rfuest merged commit 6e26493 into embedded-graphics:master Feb 19, 2022
@rfuest rfuest deleted the faster-color-conversion branch February 19, 2022 19:55
@jamwaffles
Copy link
Member

True, although it would have been interesting to see any deltas if they were somewhat representative of other HW. At any rate, 50% is a pretty good improvement!

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

2 participants