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

Use enlargeable types in to_luma functions. #1215

Closed
wants to merge 1 commit into from

Conversation

kaj
Copy link
Member

@kaj kaj commented May 11, 2020

This is an attempt to fix #1214, using the Enlargeable trait to select a suitable type for the calculation in rgb_to_luma and bgr_to_luma.

Integer pixel types will use suitable integer pixel types for the calculation, while floating point pixel types will use floating point types.

This PR also provides the Enlargeable trait for all Primitive types (luckily, Primitive is not implemented for 128 bit numeric types).

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

This is an attempt to fix image-rs#1214, using the `Enlargeable` trait to
select a suitable type for the calculation in `rgb_to_luma` and
`bgr_to_luma`.

Integer pixel types will use suitable integer pixel types for the
calculation, while floating point pixel types will use floating point
types.

This PR also provides the `Enlargeable` trait for all `Primitive`
types (luckily, `Primitive` is not implemented for 128 bit numeric
types).
@CrackedP0t
Copy link

I'll post my benchmark results here too:

Here's a Gist with the code I used

With this benchmark, the old float method takes ~70 ms, and both the int and enlarge methods take ~20 ms, with the enlarge method usually running a few ms faster.

I think this PR is an excellent solution, but I'm still very interested in the maintainers' opinions

@HeroicKatora HeroicKatora added the next: breaking Information tag for PRs and ideas that require an interface break label May 17, 2020
@HeroicKatora
Copy link
Member

Maintainers opinion: This is a breaking change. There should be a note somewhere so that something similar can be adopted for 0.24, especially since it also improves performance, but I don't think the accuracy fix itself is worth a major version on its own. In particular it would imply that downstream users wouldn't automatically receive the fix which is half of the motivation, right?

@HeroicKatora HeroicKatora added this to In progress in Version 0.24 Feb 13, 2021
@HeroicKatora HeroicKatora mentioned this pull request Feb 13, 2021
22 tasks
@HeroicKatora HeroicKatora changed the base branch from master to next March 10, 2021 20:39
@HeroicKatora HeroicKatora deleted the branch image-rs:next October 23, 2021 12:56
Version 0.24 automation moved this from In progress to Done Oct 23, 2021
@HeroicKatora HeroicKatora moved this from Done to In progress in Version 0.24 Oct 23, 2021
@HeroicKatora
Copy link
Member

This was accidentally auto-closed because its base branch got promoted to master and then removed. That's quite unfortunate. I'm still aware of this but if you could do a rebase and reopen it against master that would be perfect.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next: breaking Information tag for PRs and ideas that require an interface break
Projects
Version 0.24
  
In progress
Development

Successfully merging this pull request may close these issues.

to_luma methods can have floating point issues
3 participants