Inconsistency in handling hues #379
Comments
|
Just out of my curiosity, @johnnychen94, what are your concerns? Use of unnormalized hues? Backward compatibility? Additional costs for the normalizing? |
|
Hmmm, I'm not familiar with this topic, but as far as I can think of, normalizing a channel needs |
|
I see. Because of the problems which you point out, I focused only on the hue, which should be simply normalized to [0, 360], not to [0, 1]. For example: normalize_hue(c::T) where {T <: HSV} = T(mod(c.h, 360), c.s, c.v)BTW, I think the color gamut problem must be solved somewhere (cf. JuliaGraphics/ColorTypes.jl#140). It is probably too early to put the gamut support in Colors.jl v1.0. |
|
I'm thinking of such kind of normalization: function normalize_hue(x::T; maxvalue, minvalue) where T<:HSV
HSV(gamutmax(T)[1]*(x.h-minvalue)/(maxvalue-minvalue), x.s, x.v)
end
julia> normalize_hue(HSV(-360, 1, 1); maxvalue=360, minvalue=-360)
HSV{Float32}(0.0f0,1.0f0,1.0f0)
As you can see, if we set |
|
Although I don't think the idea is bad, I can't imagine the practical use cases. At least, the behavior with the hues which are not normalized to [0,360] is currently undefined (not documented). |
|
When a particular field represents an angle in degrees, generally we should interpret that number modulo 360. It's probably just that no one really needed this systematically before, or not enough to submit a PR, or we lacked tests to catch changes which broke this behavior. It would be fine to fix this. |
|
I will modifiy the conversion codes for julia> HSV(colorant"red")
HSV{Float32}(360.0f0,1.0f0,1.0f0)Of course, this is definitely red, but this gives strange results in the interpolation. Edit: Line 120 in d07286d |
BenchmarkI think the current RGB <--> {HSV, HSL, HSI} conversions are too slow.:fearful: Scriptusing BenchmarkTools
using FixedPointNumbers
using Colors
rgb_n0f8 = rand(RGB{N0f8},64,64);
rgb_f32 = rand(RGB{Float32},64,64);
rgb_f64 = rand(RGB{Float64},64,64);
argb_n0f8 = rand(ARGB{N0f8},64,64);
argb_f32 = rand(ARGB{Float32},64,64);
argb_f64 = rand(ARGB{Float64},64,64);
rgbs = (rgb_n0f8, rgb_f32, rgb_f64, argb_n0f8, argb_f32, argb_f64);
for C in (HSV, HSL, HSI)
AC = alphacolor(C)
for T in (Float32, Float64)
for mat in rgbs
XC = eltype(mat) <: Color ? C{T} : AC{T}
println(eltype(mat), "-> $XC")
@btime convert.($XC, view($mat,:,:))
end
for mat in rgbs
XC = eltype(mat) <: Color ? C{T} : AC{T}
println(eltype(mat), "<- $XC")
h = convert.(XC, mat)
r = convert.(eltype(mat),h)
all(t->isapprox(t...),zip(r, mat)) || @warn("failed") # verification
@btime convert.(eltype($mat), view($h,:,:))
end
end
endJulia v1.3.1 x86_64-w64-mingw32julia> versioninfo()
Julia Version 1.3.1
Commit 2d5741174c (2019-12-30 21:36 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)RGB --> HSV (unit: μs)
HSV --> RGB (unit: μs)
RGB --> HSL (unit: μs)
HSL --> RGB (unit: μs)
RGB --> HSI (unit: μs)
HSI --> RGB (unit: μs)
|
|
Note that |
Benchmark 2Julia v1.0.5 x86_64-pc-linux-gnu (Debian on WSL)julia> versioninfo()
Julia Version 1.0.5
Commit 3af96bcefc (2019-09-09 19:06 UTC)
Platform Info:
OS: Linux (x86_64-pc-linux-gnu)
CPU: Intel(R) Core(TM) i7-8565U CPU @ 1.80GHz
WORD_SIZE: 64
LIBM: libopenlibm
LLVM: libLLVM-6.0.0 (ORCJIT, skylake)RGB --> HSV (unit: μs)
HSV --> RGB (unit: μs)
RGB --> HSL (unit: μs)
HSL --> RGB (unit: μs)
RGB --> HSI (unit: μs)
HSI --> RGB (unit: μs)
|
|
BTW, I was going to send a PR for the improved version of #351 after a consensus was reached on the gamut or |
|
FYI, unfortunately, Julia nightly build can optimize the following div60(x::T) where T <: Union{Float32, Float64} = muladd(x, T(1/960), x * T(0x1p-6))IMO, such an optimization is too drastic, but some people want it, so it is not a bug. |
|
Quiz: Do you know what is different? 😄 function _hsx_to_rgb(im::UInt8, v::T, n::T, m::T) where T <:Union{Float16, Float32, Float64}
vu, nu, mu = reinterpret.(Unsigned, (v, n, m)) # prompt the compiler to use conditional moves
r = ifelse((im & 0b100001) != 0x0, vu, ifelse((im & 0b010010) != 0x0, nu, mu))
g = ifelse((im & 0b000110) != 0x0, vu, ifelse((im & 0b001001) != 0x0, nu, mu))
b = ifelse((im & 0b011000) != 0x0, vu, ifelse((im & 0b100100) != 0x0, nu, mu))
return reinterpret.(T, (r, g, b))
endfunction _hsx_to_rgb(im::UInt8, v::T, n::T, m::T) where T <:Union{Float16, Float32, Float64}
vu, nu, mu = reinterpret.(Unsigned, (v, n, m)) # prompt the compiler to use conditional moves
r = ifelse((im & 0b100001) == 0x0, ifelse((im & 0b010010) == 0x0, mu, nu), vu)
g = ifelse((im & 0b000110) == 0x0, ifelse((im & 0b001001) == 0x0, mu, nu), vu)
b = ifelse((im & 0b011000) == 0x0, ifelse((im & 0b100100) == 0x0, mu, nu), vu)
return reinterpret.(T, (r, g, b))
end |
Both are black magic spell to me 😕 |
|
I think this is "visually" easy to understand, so not so black. 😄 if hue < 60; im = 0b000001 # ---------+
elseif hue < 120; im = 0b000010 # --------+|
elseif hue < 180; im = 0b000100 # -------+||
elseif hue < 240; im = 0b001000 # ------+|||
elseif hue < 300; im = 0b010000 # -----+||||
else ; im = 0b100000 # ----+|||||
end # ||||||
(hue < 60 || hue >= 300) === ((im & 0b100001) != 0x0)I will add this comment to the source code. |
|
In short, the root of the speed problem is the inlining. As pointed out earlier in FixedPointNumbers, there is no advantage to appending Edit: |
😱 if Sys.ARCH !== :i686
_fma(x, y, z) = fma(x, y, z)
else
_fma(x, y, z) = muladd(x, y, z)
end😓 |
|
Hmmm, perhaps the color processing world doesn't need that kind of accuracy? |
If the main discussion is somewhere besides JuliaGraphics/ColorTypes.jl#150, can you provide a link? |
Originally posted by @kimikage in #407 julia> versioninfo()
Julia Version 1.5.0-DEV.376
Commit ea067fb221 (2020-03-01 09:36 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-9.0.1 (ORCJIT, skylake)
julia> div60(x::Float64) = fma(x, 1/960, x * 0x1p-6); # What I just want is, 1 `vmul` and 1 `vfmadd`
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vmulsd xmm1, xmm0, qword ptr [rax]
movabs rax, 761602336
vfmadd132sd xmm0, xmm1, qword ptr [rax]
...snip...
julia> div60(x::Float64) = muladd(x, 1/960, x * 0x1p-6); # but I don't want to use `fma()`, so, let's use `muladd()`
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vmulsd xmm0, xmm0, qword ptr [rax] # Do you know the intent that I used the distributive law?
...snip...
julia> div60(x::Float64) = x * (1/960) + x * 0x1p-6; # OK, keep it simple stupid
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vmulsd xmm1, xmm0, qword ptr [rax]
movabs rax, 761602592
vmulsd xmm0, xmm0, qword ptr [rax]
vaddsd xmm0, xmm1, xmm0 # Good! But you can get rid of this `add` by `fma`!
...snip...
julia> div60(x::Float64) = muladd(x, 1/960, muladd(x, 0x1p-6, 0.0)); # OK, OK. You can optimize `muladd` into `mul`.
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vxorpd xmm1, xmm1, xmm1
vfmadd231sd xmm1, xmm0, qword ptr [rax] # Nice double `vfmadd`s!
movabs rax, 761601960
vfmadd132sd xmm0, xmm1, qword ptr [rax]
...snip...
julia> div60(x::Float64) = muladd(x, 1/960, muladd(x, 0x1p-7, x * 0x1p-7)); # You know the distributive law, right?
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vmulsd xmm1, xmm0, qword ptr [rax] # 1 `vmul` is OK,
vaddsd xmm1, xmm1, xmm1 # but did you forget the distributive law?
movabs rax, 761602088
vfmadd132sd xmm0, xmm1, qword ptr [rax]
...snip...
julia> div60(x::Float64) = muladd(x, 1/960, muladd(x, -0x1p-7, x * 0x3p-7)); # I apologize for the same 0x1p-7 confusing you.
div60 (generic function with 1 method)
julia> @code_native syntax=:intel div60(1.0)
...snip...
movabs rax, offset .rodata.cst8
vmulsd xmm0, xmm0, qword ptr [rax] # You know the distributive law!
# But you should learn that floating-point arithmetic does not have full associativity!
...snip...💢💢💢 |
|
Solved(?) by #407 (comment) Please report when you get wrong colors or significant slowdowns. HSx-->RGB conversions seems to work fine. 😄 |
Unnormalized hues are somewhat useful in the interpolation. In other words, the interpolation across 0° is not possible with only the normalized hues.
However, the manners of input range checking in conversions vary (cf. #378 ), and some conversions cannot handle unnormalized hues correctly.
I think that the strictness of input range checking should be more consistent. Apart from that, it might be a good idea to add some utility functions related to the hue, such as
normalize_hue(c).The text was updated successfully, but these errors were encountered: