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 Float32 for the matrix coefficients for low precision colors #482

Merged
merged 1 commit into from
Jun 23, 2021

Conversation

kimikage
Copy link
Collaborator

@kimikage kimikage commented Jun 2, 2021

This also changes the definition of the sRGB transform matrix slightly. (cf. #355 (comment))

cf. #477

@codecov
Copy link

codecov bot commented Jun 2, 2021

Codecov Report

Merging #482 (dcd3281) into master (9f27da0) will decrease coverage by 0.02%.
The diff coverage is 96.77%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #482      +/-   ##
==========================================
- Coverage   93.42%   93.40%   -0.03%     
==========================================
  Files           9        9              
  Lines        1050     1061      +11     
==========================================
+ Hits          981      991      +10     
- Misses         69       70       +1     
Impacted Files Coverage Δ
src/conversions.jl 99.38% <92.30%> (-0.32%) ⬇️
src/algorithms.jl 60.95% <100.00%> (-0.37%) ⬇️
src/utilities.jl 98.87% <100.00%> (+0.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9f27da0...dcd3281. Read the comment docs.

Copy link
Member

@johnnychen94 johnnychen94 left a comment

Choose a reason for hiding this comment

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

Feel free to resolve these comments if they don't make sense.

src/algorithms.jl Outdated Show resolved Hide resolved
src/algorithms.jl Outdated Show resolved Hide resolved
@kimikage kimikage force-pushed the matmul_f32 branch 2 times, most recently from 64f2269 to 3e4173f Compare June 2, 2021 09:03
@johnnychen94

This comment has been minimized.

@kimikage
Copy link
Collaborator Author

kimikage commented Jun 9, 2021

I will submit some additional PRs to mitigate the precompilation problem.

@kimikage
Copy link
Collaborator Author

kimikage commented Jun 20, 2021

Benchmark

using Colors, FixedPointNumbers, BenchmarkTools
N = 1000;
rgb_f64 = rand(RGB{Float64}, N, N);
rgb_f32 = rand(RGB{Float32}, N, N);
rgb_n0f8 = rand(RGB{N0f8}, N, N);
xyz_f64 = XYZ.(rgb_f64);
xyz_f32 = XYZ.(rgb_f32);
yiq_f64 = YIQ.(rgb_f64);
yiq_f32 = YIQ.(rgb_f32);

@btime convert.(XYZ, $rgb_f64);
@btime convert.(XYZ, $rgb_f32);
@btime convert.(XYZ, $rgb_n0f8);
@btime convert.(RGB, $xyz_f64);
@btime convert.(RGB, $xyz_f32);
@btime convert.(RGB{N0f8}, $xyz_f32);

@btime convert.(YIQ, $rgb_f64);
@btime convert.(YIQ, $rgb_f32);
@btime convert.(YIQ, $rgb_n0f8);
@btime convert.(RGB, $yiq_f64);
@btime convert.(RGB, $yiq_f32);
@btime convert.(RGB{N0f8}, $yiq_f32);
(v0.12.8) before after
RGB{Float64}-->XYZ{Float64} 56.330 ms 30.209 ms 30.318 ms
RGB{Float32}-->XYZ{Float32} 25.483 ms 25.528 ms 23.325 ms
RGB{N0f8} -->XYZ{Float32} 3.547 ms 3.506 ms 2.396 ms
XYZ{Float64}-->RGB{Float64} 39.305 ms 36.156 ms 34.093 ms
XYZ{Float32}-->RGB{Float32} 55.132 ms 47.960 ms *36.715 ms
XYZ{Float32}-->RGB{N0f8} 55.942 ms 47.224 ms *42.136 ms

*: see also PR #493

before after
RGB{Float64}-->YIQ{Float64} 5.379 ms 5.245 ms
RGB{Float32}-->YIQ{Float32} 3.076 ms 2.373 ms
RGB{N0f8} -->YIQ{Float32} 9.060 ms 2.006 ms
YIQ{Float64}-->RGB{Float64} 5.709 ms 5.755 ms
YIQ{Float32}-->RGB{Float32} 3.398 ms 2.546 ms
YIQ{Float32}-->RGB{N0f8} 18.193 ms 16.755 ms

This also changes the definition of the sRGB transform matrix slightly.
@kimikage kimikage marked this pull request as ready for review June 22, 2021 03:18
@kimikage
Copy link
Collaborator Author

Now that the RGB<-->XYZ conversions avoids the precompilation problem, I think we are ready to merge this.

@kimikage kimikage merged commit 9710272 into JuliaGraphics:master Jun 23, 2021
@kimikage kimikage deleted the matmul_f32 branch June 23, 2021 02:52
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.

2 participants