JuliaIO / ImageMagick.jl

Thin Wrapper for the library ImageMagick
Other
53 stars 37 forks source link

Julia 1.7 performance regression on JPEG image #208

Closed johnnychen94 closed 2 years ago

johnnychen94 commented 2 years ago

CRef: https://github.com/JuliaImages/Images.jl/issues/992

The following patch fixes the performance issue, but I'm lost in the memory orders...

diff --git a/src/ImageMagick.jl b/src/ImageMagick.jl
index db26f64..b0c36fb 100755
--- a/src/ImageMagick.jl
+++ b/src/ImageMagick.jl
@@ -149,13 +149,14 @@ function load_(file::Union{AbstractString,IO,Vector{UInt8}}, permute_horizontal=
     sz, T, cs, channelorder = _metadata(wand)

     # Allocate the buffer and get the pixel data
-    buf = Array{T}(undef, sz)
-    exportimagepixels!(rawview(channelview(buf)), wand, cs, channelorder)
+    buf = Array{UInt8}(undef, (length(T), sz...))
+    exportimagepixels!(buf, wand, cs, channelorder)

     orient = getimageproperty(wand, "exif:Orientation", false)
     default = (A,ph) -> ph ? vertical_major(A) : identity(A)
     oriented_buf = get(orientation_dict, orient, default)(buf, permute_horizontal)
-    view ? oriented_buf : collect(oriented_buf)
+    colorful_buf = length(T) == 1 ? reinterpret(T, reshape(buf, sz)) : reinterpretc(T, buf)
+    view ? colorful_buf : collect(colorful_buf)
 end

cc: @etibarg

johnnychen94 commented 2 years ago

See also https://github.com/JuliaImages/ImageCore.jl/issues/174

yakir12 commented 2 years ago

I can confirm. It takes 1.5 seconds to load a 6 MB image

using FileIO, Colors
img = rand(RGB, 1440, 2560)
file = "img.jpg"
save(file, img)

using BenchmarkTools
filesize(file) # 6422686 bytes
@btime load(file); #  1.478 s (44236892 allocations: 1.51 GiB)

on

(tmp) pkg> status
      Status `~/tmp/Project.toml`
  [5789e2e9] FileIO v1.11.2
  [6218d12a] ImageMagick v1.2.2

julia> versioninfo()
Julia Version 1.7.0
Commit 3bf9d17731 (2021-11-30 12:12 UTC)
Platform Info:
  OS: Linux (x86_64-linux-gnu)
  CPU: AMD Ryzen Threadripper 2950X 16-Core Processor
  WORD_SIZE: 64
  LIBM: libopenlibm
  LLVM: libLLVM-12.0.1 (ORCJIT, znver1)
johnnychen94 commented 2 years ago

Upstream patch https://github.com/JuliaLang/julia/pull/43347 should fixes this, so keep patient on Julia 1.7.1...

lorenzoh commented 2 years ago

Just checked on 1.7.1, the problem persists

yakir12 commented 2 years ago

Yeah, can confirm, it is not resolved in 1.7.1.

For my MWE above I still get:

julia> @btime load(file);
  1.680 s (44236892 allocations: 1.51 GiB)
johnnychen94 commented 2 years ago

Unfortunately that PR isn't merged yet. I'll see if I can continue https://github.com/stevengj/JpegTurbo.jl and bring up the JPEG wrapper before Feb.

johnnychen94 commented 2 years ago

I'll see if I can continue https://github.com/stevengj/JpegTurbo.jl and bring up the JPEG wrapper before Feb.

Almost ready now. Just need to wait for a few days before registering it. Feel free to try https://github.com/johnnychen94/JpegTurbo.jl out.

johnnychen94 commented 2 years ago

A better solution is now available at https://github.com/johnnychen94/JpegTurbo.jl. For ImageMagick itself, this should be fixed for Julia 1.8.

I currently have very little interest in maintaining the old v6 version. Our plan is to gradually deprecate this package with the newer ImageIO.