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

Support ARGB32 images #45

Merged
merged 2 commits into from
Nov 5, 2021
Merged

Support ARGB32 images #45

merged 2 commits into from
Nov 5, 2021

Conversation

timholy
Copy link
Member

@timholy timholy commented Nov 5, 2021

@timholy
Copy link
Member Author

timholy commented Nov 5, 2021

The "too many files" from that bug is triggered because FileIO falls back to ImageMagick which struggles to do anything properly 😄. Fix this, and then the saving gets handled by PNGFiles.

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.

Since this PR fixes a broken use case, perhaps we can mark ARGB{N0f8}.(x) as a performance TODO and then merge this?

timholy added a commit to JuliaGraphics/Luxor.jl that referenced this pull request Nov 5, 2021
ImageMagick has largely been supplanted by ImageIO.
I also thought I'd see what you thought about avoiding being
"opinionated" about which specific IO packages are best,
so I moved such dependencies to [extras]. But we could move
such dependencies back to [deps] if you're worried about
providing an out-of-box experience that "just works" without
the user having to choose specific IO package(s).

xrefs:
- JuliaIO/PNGFiles.jl#45
- JuliaIO/FileIO.jl#352
cormullion pushed a commit to JuliaGraphics/Luxor.jl that referenced this pull request Nov 5, 2021
ImageMagick has largely been supplanted by ImageIO.
I also thought I'd see what you thought about avoiding being
"opinionated" about which specific IO packages are best,
so I moved such dependencies to [extras]. But we could move
such dependencies back to [deps] if you're worried about
providing an out-of-box experience that "just works" without
the user having to choose specific IO package(s).

xrefs:
- JuliaIO/PNGFiles.jl#45
- JuliaIO/FileIO.jl#352
@Drvi
Copy link
Collaborator

Drvi commented Nov 5, 2021

Hi and thanks for the fix! Just fyi, I managed to get tests passing by applying the following diff to this PR:

@@ -367,11 +367,11 @@ function _save(png_ptr, info_ptr, image::S;
         end
     else
         image_eltype = eltype(image)
-        if (image_eltype <: BGR || image_eltype <: BGRA || image_eltype <: ABGR)
+        if (image_eltype <: BGR || image_eltype <: BGRA || image_eltype <: ABGR || image_eltype <: ARGB32)
             png_set_bgr(png_ptr)
         end

-        if image_eltype <: AbstractARGB
+        if (image_eltype <: ABGR || image_eltype <: ARGB)
             png_set_swap_alpha(png_ptr)
         end

@@ -455,7 +455,6 @@ end
 _prepare_buffer(x::IndirectArray) = UInt8.(x.index .- first(axes(x.values, 1)))
 _prepare_buffer(x::BitArray) = _prepare_buffer(collect(x))
 _prepare_buffer(x::AbstractMatrix{<:T}) where {T<:Colorant{<:Normed}} = x
-_prepare_buffer(x::AbstractMatrix{ARGB32}) = ARGB{N0f8}.(x)   # unclear why this helps (maybe endianness?)
 _prepare_buffer(x::AbstractMatrix{<:T}) where {T<:UInt8} =  reinterpret(Gray{N0f8}, x)
 _prepare_buffer(x::AbstractMatrix{<:T}) where {T<:UInt16} = reinterpret(Gray{N0f16}, x)
 _prepare_buffer(x::AbstractMatrix{<:T}) where {T<:Normed} = reinterpret(Gray{T}, x)

I.e. by not swapping alpha but treating the colorant as BGR. Apparently, libpng treats the bytes in ARGB32 type as BGRA, so alpha is in the right place, it just the BGR that needs to be "swapped to RGB".

@timholy
Copy link
Member Author

timholy commented Nov 5, 2021

Thanks, @Drvi! I credited that to you and will make you a co-author of the squashed commit.

@timholy timholy merged commit f9b2eb5 into master Nov 5, 2021
@timholy timholy deleted the teh/argb32 branch November 5, 2021 15:58
cormullion pushed a commit to JuliaGraphics/Luxor.jl that referenced this pull request Feb 4, 2022
ImageMagick has largely been supplanted by ImageIO.
I also thought I'd see what you thought about avoiding being
"opinionated" about which specific IO packages are best,
so I moved such dependencies to [extras]. But we could move
such dependencies back to [deps] if you're worried about
providing an out-of-box experience that "just works" without
the user having to choose specific IO package(s).

xrefs:
- JuliaIO/PNGFiles.jl#45
- JuliaIO/FileIO.jl#352
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.

Too many open files
3 participants