`restrict` not preserving image colortype about imagetransformations.jl HOT 8 CLOSED

Fixing this is more subtle than it may seem. The issue: math isn't defined on YCbCr, because the components of this color space do not form a vector space:

julia> c1 = rand(RGB{Float32})
RGB{Float32}(0.984645f0,0.48174405f0,0.1373508f0)

julia> c2 = rand(RGB{Float32})
RGB{Float32}(0.31194997f0,0.9763652f0,0.11036646f0)

julia> z1 = convert(YCbCr, (c1+c2)/2)   # divide by 2 to ensure we stay in-gamut
YCbCr{Float32}(155.26558f0,63.270424f0,129.97824f0)

julia> convert(YCbCr, c1/2).cr + convert(YCbCr, c2/2).cr
257.97824f0

julia> convert(YCbCr, c1/2).cb + convert(YCbCr, c2/2).cb
191.27043f0

julia> z1.cr
129.97824f0

julia> z1.cb
63.270424f0

(Technically that's true of RGB as well, but it's commonplace to ignore that. See the README of ColorVectorSpace for more info.)

restrict accumulates its output as it goes. Consequently the array storage type must be one that supports math.

I suspect the best way to solve this would be to create a new output array at the end, i.e., automatically apply YCbCr.(img_YCbCr_half) just as you are doing manually now. Is it better to "force" the extra conversion step on users or to require that they apply it manually, as you are doing now?

from imagetransformations.jl.

Moving where the restrict happens solves the issue, but I'm not sure that it would be considered a true RGB->YUV420 conversion in that case:

img = rand(RGB{N0f8},100,200)
img_YCbCr = convert.(YCbCr{Float64}, img)
img_YCbCr_half = convert.(YCbCr{Float64}, restrict(img))

from imagetransformations.jl.

Thanks for the report, I agree that we need to preserve the types for consistency. I guess we can achieve this by enhancing

But it might require some time to check the details. I can fix it as part of my GSoC project since this's not urgent.

is there a better way to do RGB{N0f8} -> YUV420 (UInt8)

N0f8(aka Normed{UInt8, 8}) can be treated as UInt8, so you don't need to change the storage type; color conversion only is sufficient. Also, rawview(channelview(img)) might give what you want.

I'm not sure that it would be considered a true RGB->YUV420 conversion in that case

RGB -> YUV420: https://github.com/JuliaGraphics/Colors.jl/blob/master/src/conversions.jl#L762-L767
YUV420 -> RGB: https://github.com/JuliaGraphics/Colors.jl/blob/master/src/conversions.jl#L156-L164

Do I understand your questions well?

from imagetransformations.jl.

Great! My fix is ok for me for now, so not urgent.

As for the RGB -> YUV420 code, that technically does RGB -> YCbCr aka YUV.
YUV420 has half resolution Cb (U) and Cr (V) planes, hence the need for restrict on those planes.

It may be helpful to expand the RGB -> YUV conversions to support the different standard image/video plane compression standards, like YUV420, YUV422: https://ffmpeg.org/doxygen/4.1/pixfmt_8h.html especially given VideoIO is soon to support video encoding through ffmpeg

from imagetransformations.jl.

In that case, I think we can introduce non-parametric colorants alongside with some conversion utils in ColorTypes.jl, just like RGB24.

I'm not familiar with these color formats, so you might need to do it yourself. I think @timholy would be happy to review it. 😄

from imagetransformations.jl.

Is it better to "force" the extra conversion step on users or to require that they apply it manually, as you are doing now?

what about provide a macro in ColorTypes or Colors

@keep_colortype bar(foo(x)) # usage
convert.(eltype(x), bar( convert.(eltype(x), foo(x)) )) # expansion 1
convert.(eltype(x), bar(foo(x))) # expansion 2

Expansion 2 looks better than Expansion 1:

• expansion 2 requires fewer conversions than expansion 1
• if bar doesn't support YCbCr color, expansion 1 will throw errors. (Expansion 2 has the same problem if bar doesn't support RGB color, but I guess we can assume most Color3 functions support RGB images)

automatically apply YCbCr.(img_YCbCr_half) just as you are doing manually now

An alternative choice following the same idea is to provide a keyword argument keep_colortype, for example

function foo(x; keep_colortype = true)
    if keep_colortype
        return convert.(eltype(x), _foo(x))
    else
        return _foo(x)
    end
end

It's more convenient for users compared to the @keep_colortype macro while preserving the possibility for performance tweak.

But to make this argument useful it requires a possibly huge amount of code changes to many functions in JuliaImages; after all, changing a small number of functions in this way doesn't affect the entire ecosystem. For this reason, I think this is not so nice a solution.

from imagetransformations.jl.

Just found a much better solution taking advantages of MappedArrays together with in-place operation.

I don't have the time recently to dig into the structure of ImageTransformations to fix this, but here's a code snippet in ImageNoise that achieves the same goal, in case someone is interested in this:

function (n::AWGN)(out::AbstractArray{<:Number},
                   in::GenericGrayImage)
    out .= clamp01.(in .+ n.σ .* _rand(in) .+ n.μ) # inplace operation
end

for T in (AbstractGray, AbstractRGB)
    @eval function (n::AWGN)(out::AbstractArray{<:$T},
                             in::GenericImage)
        n(channelview(out), channelview(in))
    end
end

# Since generic Color3 aren't vector space, they are converted to RGB images
(n::AWGN)(out::AbstractArray{<:Color3},
         in::GenericImage) =
    n(channelview(of_eltype(RGB, out)), channelview(of_eltype(RGB, in)))

This outputs Array{Lab} for Array{Lab} since we don't create new arrays here.

from imagetransformations.jl.

In response to #68 (comment)

Unless we come up with a new way to handle Array{Lab}, it seems nothing needs to be done to improve the situation here; for performance consideration, it's the caller's responsibility to force type conversion on the result rather than the callee restrict.

For this reason, I'll just close this issue.

from imagetransformations.jl.