Speed up RGB <--> XYZ conversions by kimikage · Pull Request #352 · JuliaGraphics/Colors.jl

kimikage · 2019-09-17T21:47:53Z

This improves srgb_compand and invert_srgb_compand(renamed rgb->srgb) with almost no loss in accuracy. (#351)

kimikage · 2019-09-17T22:05:26Z

Benchmark

julia> versioninfo()
Julia Version 1.2.0
Commit c6da87ff4b (2019-08-20 00:03 UTC)
Platform Info:
  OS: Windows (x86_64-w64-mingw32)
  CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-6.0.1 (ORCJIT, skylake)

before (`be3e641`)

julia> @benchmark convert(RGB{Float64}, x) setup=(x=XYZ{Float64}(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     76.185 ns (0.00% GC)
  median time:      163.042 ns (0.00% GC)
  mean time:        170.986 ns (0.00% GC)
  maximum time:     435.670 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     970

julia> @benchmark convert(XYZ{Float64}, x) setup=(x=RGB{Float64}(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     5.382 ns (0.00% GC)
  median time:      202.278 ns (0.00% GC)
  mean time:        209.065 ns (0.00% GC)
  maximum time:     391.925 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     966

julia> @benchmark convert(XYZ{Float64}, x) setup=(x=RGB24(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     8.411 ns (0.00% GC)
  median time:      207.477 ns (0.00% GC)
  mean time:        214.632 ns (0.00% GC)
  maximum time:     480.270 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     963

after

julia> @benchmark convert(RGB{Float64}, x) setup=(x=XYZ{Float64}(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     20.059 ns (0.00% GC)
  median time:      47.944 ns (0.00% GC)
  mean time:        50.982 ns (0.00% GC)
  maximum time:     145.436 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     997

julia> @benchmark convert(XYZ{Float64}, x) setup=(x=RGB{Float64}(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     5.416 ns (0.00% GC)
  median time:      54.764 ns (0.00% GC)
  mean time:        56.797 ns (0.00% GC)
  maximum time:     159.478 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     997

julia> @benchmark convert(XYZ{Float64}, x) setup=(x=RGB24(rand(),rand(),rand()))
BenchmarkTools.Trial:
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     6.899 ns (0.00% GC)
  median time:      7.600 ns (0.00% GC)
  mean time:        7.897 ns (0.00% GC)
  maximum time:     46.901 ns (0.00% GC)
  --------------
  samples:          10000
  evals/sample:     1000

kimikage · 2019-09-17T22:16:37Z

 cnvt(::Type{CV}, c::LCHuv) where {CV<:AbstractRGB}  = cnvt(CV, convert(Luv{eltype(c)}, c))
 cnvt(::Type{CV}, c::Color3) where {CV<:AbstractRGB}    = cnvt(CV, convert(XYZ{eltype(c)}, c))

-cnvt(::Type{CV}, c::RGB24) where {CV<:AbstractRGB{N0f8}} = CV(N0f8((c.color&0x00ff0000)>>>16,0), N0f8((c.color&0x0000ff00)>>>8,0), N0f8(c.color&0x000000ff,0))


I think this is actually equivalent to cnvt(::Type{CV}, c::AbstractRGB) where {CV<:AbstractRGB} = CV(red(c), green(c), blue(c))(L82) and is helpful only when the following method is active.
However, I don't have confidence. Is there a systematic and formal technique for verification?

This might show you a couple of options for checking:

julia> using ColorTypes, FixedPointNumbers julia> cnvt1(::Type{CV}, c::RGB24) where {CV<:AbstractRGB{N0f8}} = CV(N0f8((c.color&0x00ff0000)>>>16,0), N0f8((c.color&0x0000ff00)>>>8,0), N0f8(c.color&0x000000ff,0)) cnvt1 (generic function with 1 method) julia> cnvt2(::Type{CV}, c::RGB24) where {CV<:AbstractRGB} = CV(((c.color&0x00ff0000)>>>16)/255, (((c.color&0x0000ff00))>>>8)/255, (c.color&0x000000ff)/255) cnvt2 (generic function with 1 method) julia> x = RGB24(0, 0, 0) RGB24(0.0N0f8,0.0N0f8,0.0N0f8) ## Method 1: look at the generated code julia> @code_llvm cnvt1(RGB{N0f8}, x) ; @ none:1 within `cnvt1' define { { i8 }, { i8 }, { i8 } } @julia_cnvt1_16624(%jl_value_t addrspace(10)*, { i32 } addrspace(11)* nocapture nonnull readonly dereferenceable(4)) { top: ; ┌ @ Base.jl:20 within `getproperty' %2 = getelementptr inbounds { i32 }, { i32 } addrspace(11)* %1, i64 0, i32 0 ; └ ; ┌ @ int.jl:293 within `&' %3 = load i32, i32 addrspace(11)* %2, align 4 ; └ ; ┌ @ int.jl:448 within `>>>' @ int.jl:440 %4 = lshr i32 %3, 16 ; └ ; ┌ @ /home/tim/.julia/packages/FixedPointNumbers/870tH/src/normed.jl:7 within `Type' ; │┌ @ int.jl:454 within `rem' %5 = trunc i32 %4 to i8 ; └└ ; ┌ @ int.jl:448 within `>>>' @ int.jl:440 %6 = lshr i32 %3, 8 ; └ ; ┌ @ /home/tim/.julia/packages/FixedPointNumbers/870tH/src/normed.jl:7 within `Type' ; │┌ @ int.jl:454 within `rem' %7 = trunc i32 %6 to i8 %8 = trunc i32 %3 to i8 ; └└ %.fca.0.0.insert = insertvalue { { i8 }, { i8 }, { i8 } } undef, i8 %5, 0, 0 %.fca.1.0.insert = insertvalue { { i8 }, { i8 }, { i8 } } %.fca.0.0.insert, i8 %7, 1, 0 %.fca.2.0.insert = insertvalue { { i8 }, { i8 }, { i8 } } %.fca.1.0.insert, i8 %8, 2, 0 ret { { i8 }, { i8 }, { i8 } } %.fca.2.0.insert } julia> @code_llvm cnvt2(RGB{N0f8}, x) ; @ none:1 within `cnvt2' define { { i8 }, { i8 }, { i8 } } @julia_cnvt2_16636(%jl_value_t addrspace(10)*, { i32 } addrspace(11)* nocapture nonnull readonly dereferenceable(4)) { top: ; ┌ @ Base.jl:20 within `getproperty' %2 = getelementptr inbounds { i32 }, { i32 } addrspace(11)* %1, i64 0, i32 0 ; └ ; ┌ @ int.jl:293 within `&' %3 = load i32, i32 addrspace(11)* %2, align 4 ; └ ; ┌ @ int.jl:448 within `>>>' @ int.jl:440 %4 = lshr i32 %3, 16 %5 = and i32 %4, 255 ; └ ; ┌ @ int.jl:59 within `/' ; │┌ @ float.jl:271 within `float' ; ││┌ @ float.jl:260 within `Type' @ float.jl:66 %6 = uitofp i32 %5 to double ; │└└ ; │ @ int.jl:59 within `/' @ float.jl:401 %7 = fdiv double %6, 2.550000e+02 ; └ ; ┌ @ int.jl:448 within `>>>' @ int.jl:440 %8 = lshr i32 %3, 8 %9 = and i32 %8, 255 ; └ ; ┌ @ int.jl:59 within `/' ; │┌ @ float.jl:271 within `float' ; ││┌ @ float.jl:260 within `Type' @ float.jl:66 %10 = uitofp i32 %9 to double ; │└└ ; │ @ int.jl:59 within `/' @ float.jl:401 %11 = fdiv double %10, 2.550000e+02 ; └ ; ┌ @ int.jl:293 within `&' %12 = and i32 %3, 255 ; └ ; ┌ @ int.jl:59 within `/' ; │┌ @ float.jl:271 within `float' ; ││┌ @ float.jl:260 within `Type' @ float.jl:66 %13 = uitofp i32 %12 to double ; │└└ ; │ @ int.jl:59 within `/' @ float.jl:401 %14 = fdiv double %13, 2.550000e+02 ; └ %15 = call { { i8 }, { i8 }, { i8 } } @julia_Type_16637(%jl_value_t addrspace(10)* addrspacecast (%jl_value_t* inttoptr (i64 139813171978912 to %jl_value_t*) to %jl_value_t addrspace(10)*), double %7, double %11, double %14) ret { { i8 }, { i8 }, { i8 } } %15 } ## Method 2: benchmark julia> using BenchmarkTools julia> @btime cnvt1(RGB{N0f8}, $x) 0.016 ns (0 allocations: 0 bytes) RGB{N0f8}(0.0,0.0,0.0) julia> @btime cnvt2(RGB{N0f8}, $x) 5.234 ns (0 allocations: 0 bytes) RGB{N0f8}(0.0,0.0,0.0)

While they give the same result, this method is far more efficient. It's a performance optimization.

(The 0.016ns is not to be taken seriously, anything much smaller than 1ns just means the compiler elided the whole computation during benchmarking.

Thank you for your kind reply and I am sorry for misleading you.
I understand that the former (cnvt1) is more efficient than the latter (cnvt2) and this is the raison d'être for the former.
So, my question is why the latter method is defined. If the latter method is not defined, the former method loses the raison d'être because there is the higher level method, which generates the same code.

Colors.jl/src/conversions.jl

Line 80 in be3e641

cnvt(::Type{CV}, c::AbstractRGB) where {CV<:AbstractRGB} = CV(red(c), green(c), blue(c))

Ah, yes, if it generates the same code then it can be eliminated.

kimikage · 2019-09-17T22:23:35Z

 cnvt(::Type{CV}, c::Color3) where {CV<:AbstractRGB}    = cnvt(CV, convert(XYZ{eltype(c)}, c))

-cnvt(::Type{CV}, c::RGB24) where {CV<:AbstractRGB{N0f8}} = CV(N0f8((c.color&0x00ff0000)>>>16,0), N0f8((c.color&0x0000ff00)>>>8,0), N0f8(c.color&0x000000ff,0))
-cnvt(::Type{CV}, c::RGB24) where {CV<:AbstractRGB} = CV(((c.color&0x00ff0000)>>>16)/255, (((c.color&0x0000ff00))>>>8)/255, (c.color&0x000000ff)/255)


Is there the reason to convert to Float64 instead of N0f8?

It probably would be better to define this one as returning CV(cvt(RGB{N0f8}, c))

When the latter method is changed to CV(cnvt(RGB{N0f8}, c)), the latter loses the raison d'être, too. This is the reason why I deleted the two methods. However, I am a little confused about the complicated method definitions.

It's not a short file and it accumulated over time, over which the "technology" in ColorTypes was improving. It's entirely possible there are methods that were necessary formerly but which are no longer needed. As long as the tests pass and you don't notice any performance regressions, I think you're good to go.

I agree. I'll check the test cases and the performance again.

codecov · 2019-09-17T22:24:58Z

Codecov Report

Merging #352 into master will increase coverage by 0.25%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
+ Coverage   74.06%   74.32%   +0.25%     
==========================================
  Files          10       10              
  Lines         775      775              
==========================================
+ Hits          574      576       +2     
+ Misses        201      199       -2

Impacted Files	Coverage Δ
src/conversions.jl	`96.58% <100%> (+0.68%)`	⬆️

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 be3e641...184d3ab. Read the comment docs.

kimikage · 2019-09-17T22:33:18Z

 cnvt(::Type{XYZ{T}}, c::YIQ) where {T} = cnvt(XYZ{T}, convert(RGB{T}, c))
 cnvt(::Type{XYZ{T}}, c::YCbCr) where {T} = cnvt(XYZ{T}, convert(RGB{T}, c))

-cnvt(::Type{XYZ{T}}, c::RGB24) where {T} = cnvt(XYZ{T}, convert(RGB{T}, c))


Now cnvt(::Type{XYZ{T}}, c::AbstractRGB) welcomes RGB{N0f8}.

timholy

This looks great. Just make sure that the deleted methods don't cause any performance regressions, and this is good to merge.

timholy · 2019-09-18T09:02:45Z

 cnvt(::Type{CV}, c::Color3) where {CV<:AbstractRGB}    = cnvt(CV, convert(XYZ{eltype(c)}, c))

-cnvt(::Type{CV}, c::RGB24) where {CV<:AbstractRGB{N0f8}} = CV(N0f8((c.color&0x00ff0000)>>>16,0), N0f8((c.color&0x0000ff00)>>>8,0), N0f8(c.color&0x000000ff,0))
-cnvt(::Type{CV}, c::RGB24) where {CV<:AbstractRGB} = CV(((c.color&0x00ff0000)>>>16)/255, (((c.color&0x0000ff00))>>>8)/255, (c.color&0x000000ff)/255)


It probably would be better to define this one as returning CV(cvt(RGB{N0f8}, c))

timholy · 2019-09-18T09:04:29Z

+const invert_srgb_compand_n0f8 = [invert_srgb_compand(v/255) for v = 0:255] # LUT
+
+function invert_srgb_compand(v::N0f8)
+    invert_srgb_compand_n0f8[v.i + 1]


Suggested change

invert_srgb_compand_n0f8[v.i + 1]

invert_srgb_compand_n0f8[retinterpret(UInt8, v) + 1]

I know it's longer, but this uses an "official" interface rather than digging into the struct.

Entirely optional, you can leave it like it is.

Oh, and pre-calculating this seems like a good idea!

kimikage · 2019-09-19T17:17:37Z

Assessment of the elimination of the two methods

As Codecov says, the current test-set does not have the test cases which call the two methods.

The chains of conversion which may call the two methods should have at least one of the two type of (sub) chain (except the direct conversion).

upstream X → RGB24 → AbstractRGB
RGB24 → AbstractRGB → downstream Y

Direct Conversion

The direct means that it is not intermediate conversion, i.e. it starts with RGB24 and ends with AbstractRGB.
Let me get straight to the point, there is no direct conversion which call the two methods. The conversion completes within ColorTypes.jl as the following comment suggests:

Colors.jl/src/conversions.jl

Lines 33 to 35 in be3e641

    
           # Note that ColorTypes handles the cases where Odest == Osrc, or they 
        
           # are both subtypes of AbstractRGB.  Therefore, here we only have to 
        
           # deal with conversions between different spaces.

convert(AbstractRGB, RGB24())
- of course, this calls convert(::Type{C}, c::C) where {C<:Colorant} = c in ColorTypes.jl.(practically, inlined)
convert(AbstractRGB{N0f8}, RGB24())
- same as above. (RGB24<:AbstractRGB{N0f8} == true)
convert(RGB, RGB24())
- this calls ColorTypes._convert(RGB{N0f8}, RGB, RGB24, RGB()) in ColorTypes.jl.
convert(RGB{Float32}, RGB24())
- almost the same as above except the first argument (RGB{Float32}).

Although there are the exceptions convert(::Type{RGB24}, c) in Colors.jl, which are preferred over convert(::Type{C}, c) where {C<:Colorant} in ColorTypes.jl, the source type of the two method is RGB24 and they do not matter.

upstream `X` → `RGB24` → `AbstractRGB`

The callers of cnvt(RGB24, c) may call the two methods as something like convert(AbstractRGB, cnvt(RGB24, c)). However, again, the outer or downstream convert() completes within ColorTypes.jl. Therefore, what we should trace are cnvt(::Type{AbstractRGB}, c) or its variants.
The targets should be found here, but no one call the two methods.
Thus, the upstream chain is not linked to the public (i.e. exported) interfaces.

`RGB24` → `AbstractRGB` → downstream `Y`

Because the upstream chain is not linked, the concatenated chain "upstream X → RGB24 → AbstractRGB→ downstream Y" is not linked, too.
Moreover, the chain which starts with RGB24 → AbstractRGB, does not call the two methods implicitly or indirectly as mentioned above, and there is no explicit call, too.

Conclusion

The two methods are already dead.

kimikage · 2019-09-20T09:08:45Z

Since I found that the two methods are unused and have no effect on performance, I will, paradoxically, revert the elimination.
My original intention was that the two methods should be eliminated as a result of fixing the latter one as I commented in [1], [2], and [3].
Althoug the intention was based on the premise that they may be called, it was different to the reality. So, eliminating the two and this are totally different issues. Thus, the issue should be report separately.

I will modify them and push -f. Then, let's merge it!
Edit:
I made a mistake, although the changed code works fine.

kimikage · 2019-09-21T05:45:08Z

Oh, I'm sorry, but I forgot rebase.

timholy · 2019-09-21T08:22:02Z

No worries, merge commits are fine as far as I'm concerned. I decide on a case-by-case basis whether I want to keep commits separate. For single-commit PRs I try to remember, but I forget frequently.

Thanks for tackling this!

kimikage · 2019-09-22T04:25:08Z

Now I understand what happened or what I actually did. I fixed a typo retinterpret after rebase master and reverting the elimination. I made a mistake at that time.
I'll be more careful.

kimikage commented Sep 17, 2019

View reviewed changes

kimikage requested a review from timholy September 17, 2019 22:35

timholy approved these changes Sep 18, 2019

View reviewed changes

kimikage mentioned this pull request Sep 21, 2019

Conversion methods should be organized #354

Closed

Speed up RGB <--> XYZ conversions

184d3ab

kimikage force-pushed the rgb_xyz branch from 65e6354 to 184d3ab Compare September 21, 2019 04:14

kimikage merged commit d64fb1a into JuliaGraphics:master Sep 21, 2019

kimikage deleted the rgb_xyz branch September 21, 2019 04:47

kimikage mentioned this pull request Sep 21, 2019

[RFC] Whether RGB <-> XYZ conversion should be more faster or be kept simple #351

Closed

	invert_srgb_compand_n0f8[v.i + 1]
	invert_srgb_compand_n0f8[retinterpret(UInt8, v) + 1]

Conversation

kimikage commented Sep 17, 2019

Uh oh!

kimikage commented Sep 17, 2019

Benchmark

before (be3e641)

after

Uh oh!

kimikage Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Sep 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kimikage Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timholy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kimikage commented Sep 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Assessment of the elimination of the two methods

Direct Conversion

upstream X → RGB24 → AbstractRGB

RGB24 → AbstractRGB → downstream Y

Conclusion

Uh oh!

kimikage commented Sep 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kimikage commented Sep 21, 2019

Uh oh!

timholy commented Sep 21, 2019

Uh oh!

kimikage commented Sep 22, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

before (`be3e641`)

kimikage Sep 17, 2019 •

edited

Loading

kimikage Sep 18, 2019 •

edited

Loading

kimikage Sep 17, 2019 •

edited

Loading

codecov Bot commented Sep 17, 2019 •

edited

Loading

kimikage Sep 17, 2019 •

edited

Loading

kimikage commented Sep 19, 2019 •

edited

Loading

upstream `X` → `RGB24` → `AbstractRGB`

`RGB24` → `AbstractRGB` → downstream `Y`

kimikage commented Sep 20, 2019 •

edited

Loading