PNGFiles.jl icon indicating copy to clipboard operation
PNGFiles.jl copied to clipboard

A hidden bug caused by indirect array (possibly)

Open guo-yong-zhi opened this issue 1 year ago • 4 comments

using Colors, Downloads, PNGFiles
img = PNGFiles.load(Downloads.download("https://raw.githubusercontent.com/guo-yong-zhi/WordCloud.jl/master/res/heart_mask.png"))
# img = img |> collect # this line is the key. Cancel the comment and the error will disappear
@show typeof(img)
for i in eachindex(img)
    try
        img[i]=convert(eltype(img), Colors.alphacolor(RGB(0.0,0.0,0.0), Colors.alpha(img[i])))
    catch e
        println("!!! ", img[i], " #", i)
        rethrow(e)
    end
end

typeof(img) = IndirectArrays.IndirectArray{RGBA{FixedPointNumbers.N0f8}, 2, UInt8, Matrix{UInt8}, OffsetArrays.OffsetVector{RGBA{FixedPointNumbers.N0f8}, Vector{RGBA{FixedPointNumbers.N0f8}}}} !!! RGBA{N0f8}(1.0,0.753,0.796,0.506) #176117 InexactError: trunc(UInt8, 256) Stacktrace: [1] throw_inexacterror(f::Symbol, #unused#::Type{UInt8}, val::Int64) @ Core .\boot.jl:614 [2] checked_trunc_uint @ .\boot.jl:644 [inlined] [3] toUInt8 @ .\boot.jl:706 [inlined] [4] UInt8 @ .\boot.jl:766 [inlined] [5] convert @ .\number.jl:7 [inlined] [6] setindex! @ .\array.jl:966 [inlined] [7] setindex!(A::IndirectArrays.IndirectArray{RGBA{FixedPointNumbers.N0f8}, 2, UInt8, Matrix{UInt8}, OffsetArrays.OffsetVector{RGBA{FixedPointNumbers.N0f8}, Vector{RGBA{FixedPointNumbers.N0f8}}}}, x::RGBA{FixedPointNumbers.N0f8}, i::Int64) @ IndirectArrays C:\Users\me.julia\packages\IndirectArrays\BUQO3\src\IndirectArrays.jl:80 [8] top-level scope @ .\In[3]:7


versioninfo() Julia Version 1.8.3 Commit 0434deb161 (2022-11-14 20:14 UTC) Platform Info: OS: Windows (x86_64-w64-mingw32) CPU: 8 × Intel(R) Core(TM) i5-10210U CPU @ 1.60GHz WORD_SIZE: 64 LIBM: libopenlibm LLVM: libLLVM-13.0.1 (ORCJIT, skylake) Threads: 1 on 8 virtual cores


Status C:\Users\me\.julia\environments\v1.8\Project.toml [35d6a980] ColorSchemes v3.20.0 [5ae59095] Colors v0.12.10 [5789e2e9] FileIO v1.16.0 [82e4d734] ImageIO v0.6.6 [916415d5] Images v0.25.2 [a98d9a8b] Interpolations v0.14.7 [ae8d54c2] Luxor v3.7.0 [f57f5aa1] PNGFiles v0.3.17 [90137ffa] StaticArrays v1.5.19

guo-yong-zhi avatar Apr 14 '23 12:04 guo-yong-zhi

An interesting case. You may already know this, but for those who haven't poked at this themselves, here's what's happening: that image contains 130 distinct values that differ almost entirely by their alpha value (there are only 2 RGB colors in the image). The index array is a Matrix{UInt8}. When you try to set pixel elements to zero(RGB) but preserving their alpha value, you essentially need to recreate all the same values but with black instead of the previous RGB color. Thus, this effectively doubles the number of values, from 130 -> 260 > 255 which is too big to store in the index array.

https://github.com/JuliaArrays/IndirectArrays.jl/blob/9ccd41cd60a312e59e5a847ec76a94938dba7efd/src/IndirectArrays.jl#L78-L80

What's harder is to figure out what to do about it. It would be a loss, IMO, to return a conventional Array if the image is stored in indexed form. Another option is to stop supporting setindex! for IndirectArrays and make them read-only. This has downsides as well as upsides.

timholy avatar Apr 14 '23 14:04 timholy

Can we set them as read-only by default (or warn on write) and provide a writable! function? Or when the index overflows, just providing a better error message to tell the user that the representation range of the current IndirectArray is exceeded?

guo-yong-zhi avatar Apr 14 '23 15:04 guo-yong-zhi

FWIW, if you load the image with expand_paletted=true, you'd get a regular, non-indexed Matrix back without the need to collect it.

Drvi avatar Apr 14 '23 15:04 Drvi

Can we set them as read-only by default (or warn on write)

That used to be true, but support for setindex! is desirable in some circumstances: https://github.com/JuliaArrays/IndirectArrays.jl/pull/12

Or when the index overflows, just providing a better error message

That's my thinking, too. I think that's the right plan.

timholy avatar Apr 14 '23 19:04 timholy