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

Move away from ImageMagick #24

Closed
timholy opened this issue Oct 22, 2013 · 37 comments · Fixed by #948
Closed

Move away from ImageMagick #24

timholy opened this issue Oct 22, 2013 · 37 comments · Fixed by #948
Labels
enhancement Adds a new feature. performance

Comments

@timholy
Copy link
Member

timholy commented Oct 22, 2013

Relying on ImageMagick seems to be presenting problems for Mac users (see #15, #23), and also leads to slow performance. For at least PNG, JPG, and TIF it would be good to find an alternative, presumably by directly calling the corresponding libraries.

@timholy
Copy link
Member Author

timholy commented Oct 29, 2013

When this happens, add floating-point format to tiff.

@ihnorton
Copy link
Collaborator

Follow-up question: are you ok with bundling these wrappers in Images? I feel like ideally each binding would be available separately from Pkg (or an "ImageIO" package), but that might be a premature optimization at the moment esp. considering Pkg is a tad slow.

@timholy
Copy link
Member Author

timholy commented Nov 23, 2013

I think it may be best to bundle with Images. We already have an infrastructure that avoids loading format-specific code until it's needed, so it shouldn't bloat the time for using Images at all.

@timholy
Copy link
Member Author

timholy commented Nov 23, 2013

@ihnorton and I did some outside digging. libpng's error handling is really problematic because it uses setjmp and longjmp. I think we'd need to write a C wrapper and link against libjulia so we could have the error handler call jl_errorf. But that seems really problematic given that folks will use readline, debug, etc versions. The other alternative is to use a different PNG library. @ihnorton identified a couple such projects.

This has me leaning towards an overall wrapper, either OpenImageIO (as suggested by @ihnorton), FreeImage, or ImageMagick's C API. OpenImageIO looks like the nicest and most actively-maintained of these, but it's a C++ library which could be trouble. (On the other hand, maybe a good excuse to polish C++ infrastructure.) OpenImageIO uses a BSD license; the other two use their own licenses, and they seem fairly permissive but I'd guess they are not MIT-compatible.

@timholy
Copy link
Member Author

timholy commented Nov 23, 2013

I've played a little with OpenImageIO, and even on a Kubuntu 12.04 machine it seems a little nontrivial to build. It appears to require at least a more modern OpenEXR than comes with 12.04. This does suggest that we might need to build the whole library as a binary on all platforms, and if we want to support a lot of file formats this would mean it won't be insignificant to put this together.

@ihnorton
Copy link
Collaborator

MIT-compatible or GPL-compatible?

The ImageMagick license looks fine (attribution clause does not exclude MIT). Furthermore, runtime linking is explicitly stated to not constitute derivation.

The FreeImage license looks fine also, it is LGPL-like (modifications must be distributed, but presumably we won't make any). stackoverflow thinks it is MIT compatible, FWIW.

@timholy
Copy link
Member Author

timholy commented Nov 24, 2013

Thanks, that's very helpful. Of course you're right that we presumably don't need to worry about redistribution, it's likely we'll simply link (hence LGPL is not actually restrictive).

@ihnorton
Copy link
Collaborator

[edit] Thanks to @vtjnash's help and suggestions regarding setjmp, it looks like this is tractable. The setjmp returns properly when I create an error condition by passing a jpeg instead. Note that the setjmp must be a real ccall inside of the if block. If you make a setjmp(buf) = ccall(...) function like I originally did it will segfault, I guess because it needs to be inline.

@ihnorton
Copy link
Collaborator

(the get_width and get_height functions work correctly too; haven't gone further than that yet)

@vtjnash
Copy link

vtjnash commented Nov 25, 2013

@ihnorton I probably should have mentioned that: the function that called setjmp must not return, or it will destroy the stack that it is meant to preserve.

@timholy
Copy link
Member Author

timholy commented Nov 25, 2013

Unless we want to use a wrapper, there's still the problem that the jmp_buf in png_struct appears to be machine-dependent.

ATM I'm exploring ImageMagick's C interface, since my worry is that if PNG and JPEG are presenting challenges, how likely is it that other formats won't present their own idiosyncrasies? This obviously doesn't constitute "moving away from ImageMagick", but the biggest problem with our usage of IM has been the Cmd interface. Wrapping the C library directly should get rid of that problem.

That said, jpeg-turbo may be faster than IM, and that would be a good thing. In other words, I'm viewing IM as a generic fallback as much as anything.

I'm not currently planning to wrap any more of IM than the image loading/unloading code (which looks like ~10 functions).

@ihnorton
Copy link
Collaborator

Unless we want to use a wrapper, there's still the problem that the jmp_buf in png_struct appears to be machine-
dependent.

True, but we don't have to allocate it ourselves.

@timholy
Copy link
Member Author

timholy commented Nov 25, 2013

But since it's at the top of the png_struct, we won't know the byte-offset of any of the other elements.

@ihnorton
Copy link
Collaborator

I think that's ok. The libpng manual explicitly discourages accessing struct fields directly (in part because of jmp_buf being at the beginning), and the library appears to have setter/getter methods for everything that might be needed.

@timholy
Copy link
Member Author

timholy commented Nov 26, 2013

I see, your point is that we don't even really need the png_struct to be defined in Julia, as long as we have an upper bound on its size?

@ihnorton
Copy link
Collaborator

Yes - the size should not be necessary at all because the allocation is done by png_create_read_struct, which returns a Ptr{png_struct}.

@timholy
Copy link
Member Author

timholy commented Dec 2, 2013

OK, I just pushed the io branch. It currently wraps the "wand" interface of ImageMagick (so we have support for many file types), and by comparison to the old Cmd interface it increases the speed of I/O a lot. This needs testing on Windows and other platforms---I expect it won't work, but I won't know what to fix until I try this out on my Windows machine over the next couple of days.

I still intend to wrap libpng, but I wanted to get a generic fallback done first (in part so I can compare whether a direct wrap of libpng is even faster).

@ihnorton
Copy link
Collaborator

ihnorton commented Dec 6, 2013

I will test the io branch on Windows tomorrow.

@timholy
Copy link
Member Author

timholy commented Dec 6, 2013

This may be of particular interest to @rened.

A performance test of the io branch on a directory containing 230 JPEGs (the first time is to read all the images, the second is to read-and-write each image):

# io branch:
# julia> include("testio.jl")
# 230 JPEG images found
# elapsed time: 3.228644434 seconds (338373288 bytes allocated)
# elapsed time: 6.684822187 seconds (693652708 bytes allocated)

# master branch:
# julia> include("testio.jl")
# 230 JPEG images found
# elapsed time: 18.502522107 seconds (1387385220 bytes allocated)
# Failed on 49 with name epithelium_sketch.jpg
# elapsed time: 2350.063560985 seconds (48392770784 bytes allocated)

Just a tiny bit faster :-). #41 may, for JPEGs, be even better.

@rened
Copy link

rened commented Dec 6, 2013

This is great! I was not aware of the brach and will try it out.

@timholy
Copy link
Member Author

timholy commented Dec 6, 2013

That would be great, esp. if you are on OSX. I don't have such a machine to test.

@ihnorton
Copy link
Collaborator

ihnorton commented Dec 8, 2013

Tested against a handful of images on Windows: looks good (and fast!). I used the latest ImageMagick x64 installer. Only change required was the library name:

const libwand = "CORE_RL_wand_"

@timholy
Copy link
Member Author

timholy commented Dec 8, 2013

Wow, that's amazing. I had seen different versions of the library around in old messages, it was hard to tell what was current. I'll make that change. Thanks so much!

@malmaud
Copy link

malmaud commented Dec 9, 2013

It seems like using Homebrew on OS X to install ImageMagick, the file /usr/local/lib/libMagickWand-6.Q16.dylib exists, but not /usr/local/lib/libMagickWand.dylib, as the images package seems to expect. Creating a symlink manually solved the problem.

@malmaud
Copy link

malmaud commented Dec 9, 2013

Also I'm getting an error on the IO branch when trying to use writemime:

julia> io=IOString()
IOBuffer([],true,true,true,false,0,9223372036854775807,1)

julia> writemime(io, MIME("image/png"), imread("f.gif"))
ERROR: no method imagemagick_write_cmd(Image{Uint8,3,Array{Uint8,3}}, ASCIIString)
 in writemime at /Users/malmaud/.julia/Images/src/io.jl:162

@timholy
Copy link
Member Author

timholy commented Dec 9, 2013

Thanks a ton for the report on the library, that will be very useful.

Before merging io, the plan is to replace writemime with a new version that uses either the wand interface or libpng directly. I'll let you know when that's done, since you are so helpful in testing!

@timholy
Copy link
Member Author

timholy commented Dec 11, 2013

OK, writemime has been reimplemented on the io branch. This should fix both of the IJulia issues you reported.

@timholy
Copy link
Member Author

timholy commented Dec 11, 2013

I should add that I attempted to add a BinDeps installer for ImageMagick.

@malmaud
Copy link

malmaud commented Dec 12, 2013

Thanks @timholy, new writemime is working well.

@IanButterworth
Copy link
Member

Given the JLL work recently, I thought, I'd see how far I could get with a ImageIO.jl package that bypasses ImageMagick, using libjpegturbo, libpng, libtiff directly. Research while working on it has uncovered a lot of the initial discussions around this, such as this issue, and #41

Currently I've wrapped each lib, and have functions from libpng and libtiff returning, but libjpegturbo is saying the functions are missing from the dylib. I saw that the branch was deleted in #41, does anyone know where/if I can track down that work?

Also, I don't know how much of an appetite there is for this kind of thing? I've always thought that ImageMagick is a bit slow, but that opinion is rather vague..

WIP: https:/ianshmean/ImageIO.jl

@timholy
Copy link
Member Author

timholy commented Dec 28, 2019

It is potentially quite interesting! Benchmarks would obviously help make the case, as might increased API flexibility (e.g., loading just a rectangular block of an image).

The right place for such work to end up would indeed be an ImageIO package (or perhaps PNG.jl, JPEG.jl, and TIFF.jl packages) coupled with FileIO.

@IanButterworth
Copy link
Member

Some preliminary benchmarking..
The time to loaded image time seems better in this small-image PNG test case (via absorbed code from https:/FugroRoames/LibPNG.jl):

@time using FileIO                      #0.311481 seconds (212.64 k allocations: 12.633 MiB)
@time img1 = load(testimg)              #5.867467 seconds (13.37 M allocations: 680.954 MiB, 6.01% gc time)
@btime img1 = load(testimg)             #487.182 μs (367 allocations: 21.88 KiB)

@time using ImageIO                     #1.287301 seconds (2.25 M allocations: 111.920 MiB, 1.60% gc time)
@time img2 = ModPNG.readimage(testimg)  #0.377399 seconds (1.14 M allocations: 57.108 MiB, 3.07% gc time)
@btime img2 = ModPNG.readimage(testimg) #26.236 μs (15 allocations: 992 bytes)

For LibTurboJpeg, I've only wrapped an encoder so far..
The smaller images are faster, larger are slower

ImageIO.jl - libturbojpeg FileIO - QuartzImageIO.jl FileIO - ImageMagick.jl
rand(RGB{N0f8}, 10, 20) 101.484 μs (46 allocations: 2.81 KiB) 534.594 μs (198 allocations: 12.84 KiB) 460.599 μs (198 allocations: 12.84 KiB)
rand(RGB{N0f8}, 1000, 2000) 92.338 ms (51 allocations: 5.72 MiB) 89.069 ms (201 allocations: 22.90 MiB) 90.524 ms (201 allocations: 22.90 MiB)
rand(RGB{N0f8}, 10000, 20000) 14.329 s (51 allocations: 572.21 MiB) 9.053 s (201 allocations: 2.24 GiB) 9.378 s (201 allocations: 2.24 GiB)

This should all be more structured, but getting an early idea of the benefit seemed worth it. Seems like it might be worth it for smaller images..

@IanButterworth
Copy link
Member

Also, @timholy your suggestion of skipping ImageIO and just pointing FileIO to JPEG.jl, TIFF.jl, PNG.jl is making more and more sense to me, given that we want to minimize both package loading time and image loading time, and people usually work in batches of same filetype images.

And just to cross-link, @tlnagy is working towards TIFF.jl, based on the basic TIFF handling elements of OMETIFF.jl tlnagy/OMETIFF.jl#12 (comment)

@johnnychen94 johnnychen94 reopened this Jan 1, 2020
@johnnychen94 johnnychen94 added enhancement Adds a new feature. performance labels Jan 1, 2020
@norru
Copy link

norru commented Jan 28, 2020

Hi all, while you're collecting requirements, would it be possible to allow the backends which support it to write Float32 pixel data (such as TIFF) directly without a lossy conversion to UNorm? :)

@sdewaele
Copy link

Another motivation for having separate packages such as PNG.jl is open source licenses, especially when we include libraries in relocatable apps. Specifically, ImageMagick depends on Zstd (ImageMagick -> ImageMagick_dll -> Libtiff_jll -> Zstd_jll -> Zstd), which has as a BSD/GPLv2 license.

BTW, thanks a lot to @ianshmean for putting together this PR for LibPNG which should enable me to create a relocatable app with only LibPNG!

@IanButterworth
Copy link
Member

@sdewaele check out JuliaIO/PNGfiles.jl and JuliaIO/ImageIO.jl

We’ve had it out in the wild for a month or so now and it seems to be going well

@sdewaele
Copy link

Thanks @ianshmean, this is even better. With PNGFiles, one has the option to only read/write PNG files, which is exactly what I was looking for. It works well.

Also, it is better to rely on a registered package as opposed to a PR...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adds a new feature. performance
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants