Performance improvements for build_histogram and adjust_histogram

[timholy]

I started out optimizing imhist (due to some benchmarks posted on the #image-processing slack channel) but switched to build_histogram as the more "sane" implementation.

Setup:

julia> using Images, TestImages, BenchmarkTools

julia> img = testimage("light");

julia> imgg = Gray.(img);

Master:

julia> @btime build_histogram(img);
ERROR: MethodError: no method matching build_histogram(::Array{RGB{Normed{UInt8,8}},2}, ::Int64, ::RGB{Normed{UInt8,8}}, ::RGB{Normed{UInt8,8}})
Closest candidates are:
  build_histogram(::AbstractArray, ::Integer) at /home/tim/.julia/dev/Images/src/exposure.jl:326
  build_histogram(::AbstractArray, ::Integer, ::Union{Real, Color{T,1} where T}, ::Union{Real, Color{T,1} where T}) at /home/tim/.julia/dev/Images/src/exposure.jl:321
  build_histogram(::AbstractArray{T,N} where N, ::AbstractRange) where T<:(Color{T,3} where T) at /home/tim/.julia/dev/Images/src/exposure.jl:330
  ...
Stacktrace:
 [1] build_histogram(::Array{RGB{Normed{UInt8,8}},2}, ::Int64) at /home/tim/.julia/dev/Images/src/exposure.jl:326 (repeats 2 times)
...

julia> @btime build_histogram(img, 256);
ERROR: MethodError: no method matching build_histogram(::Array{RGB{Normed{UInt8,8}},2}, ::Int64, ::RGB{Normed{UInt8,8}}, ::RGB{Normed{UInt8,8}})
Closest candidates are:
  build_histogram(::AbstractArray, ::Integer) at /home/tim/.julia/dev/Images/src/exposure.jl:326
  build_histogram(::AbstractArray, ::Integer, ::Union{Real, Color{T,1} where T}, ::Union{Real, Color{T,1} where T}) at /home/tim/.julia/dev/Images/src/exposure.jl:321
  build_histogram(::AbstractArray{T,N} where N, ::AbstractRange) where T<:(Color{T,3} where T) at /home/tim/.julia/dev/Images/src/exposure.jl:330
  ...
Stacktrace:
 [1] build_histogram(::Array{RGB{Normed{UInt8,8}},2}, ::Int64) at /home/tim/.julia/dev/Images/src/exposure.jl:326
...

julia> @btime build_histogram(img, 256, 0, 1);
  11.475 ms (5 allocations: 386.42 KiB)

julia> @btime adjust_histogram(Equalization(), img, 256);
  49.846 ms (15 allocations: 5.63 MiB)

julia> @btime adjust_histogram(Equalization(), imgg, 256);
  32.206 ms (9 allocations: 393.09 KiB)

This PR:

julia> @btime build_histogram(img);
  902.893 μs (3 allocations: 2.27 KiB)

julia> @btime build_histogram(img, 256);
  2.631 ms (6 allocations: 4.52 KiB)

julia> @btime build_histogram(img, 256, 0, 1);
  922.566 μs (6 allocations: 4.53 KiB)

julia> @btime adjust_histogram(Equalization(), img, 256);
  20.114 ms (15 allocations: 5.63 MiB)

julia> @btime adjust_histogram(Equalization(), imgg, 256);
  3.804 ms (11 allocations: 395.30 KiB)

Comparison with imhist/histeq:

julia> @btime imhist(img, 256);
  14.448 ms (5 allocations: 386.06 KiB)

julia> @btime histeq(img, 256);
  29.787 ms (6 allocations: 1.13 MiB)

Not nearly as much an improvement for adjust_histogram. There is more that could be done but it would be more of an architecture change.

These were inspired by comparison to PIL and OpenCV. With OpenCV, the naive approach fails for color images (https://stackoverflow.com/questions/15007304/histogram-equalization-not-working-on-color-image-opencv) and for PIL the quality is much worse than what we achieve:

Original:

Julia's adjust_histogram:

PIL's ImageOps.equalize:

You can see much more dramatic changes in hue with PIL.

[zygmuntszpak]

Thank you very much for your wonderful insights Tim. I was actually intending to remove all of the exposure.jl implementations from Images.jl and replace them by reexporting from ImageContrastAdjustment.jl. The build_histogram code etc. lives there at the moment.

Perhaps we should open this pull-request over at ImageContrastAdjustment.jl?

[zygmuntszpak]

I'm intrigued why the lazy mappedarray is more performant than converting the entire image to Gray using broadcast Gray.(img). I assumed that it is better to avoid any type of conversion inside hot loops. I would be grateful if you could take a moment to explain what is going on "under the hood" that makes this faster.

[timholy]

Since you only need each value once, there is no advantage in creating the array, so lazy mapping avoids the cost of allocation.

[johnnychen94]

I feel our expectation on pushing Images.jl to v1.0 should be on API stabilization, code rearrangement, documentation, and versioning. Maintainable work on performance optimization and multi-threads requires more effort than we can pay in a short period (say, three months) and I
personally view them as v1.x enhancements.

---

One thing that might be irrelevant is, shall we consider to add more interchangeability to other libraries? Although I like the manner we reason in JuliaImages, PIL, scikit-images, and OpenCV are really good libraries and I think we should take advantage of them. Some folks around me are asking me to provide an OpenCV.jl wrapper even though I know nothing about C/C++ (e.g., opencv/opencv#16233.) JSoC 19' also listed this as a project option but nobody is really working on it so far.

By saying this I think we need to standardize a way to "borrow" the functionality of other libraries for those we're not able to implement in a short time(lack of knowledge or time), with the artifacts feature in Julia 1.3 I think it's worth doing.

[timholy]

I agree that across-the-board performance enhancement is something to work on later. But while we have a couple of folks writing benchmarks, I want them to know that we can fix some of the problems they are observing. A subset of methods in JuliaImages have been very carefully optimized, but it doesn't attract contributors if we're too slow on common tasks.

And yes, I'm keenly interested in interop particularly with OpenCV. It's quite likely that the easiest way to systematically benchmark against OpenCV will be to have good wrappers.

[timholy]

Will ImageContrastAdjustment move to JuliaImages?

[zygmuntszpak]

What's the process involved in moving the repo to JuliaImages? I believe it makes sense to move it, but I'd like to understand what moving it entails and what the implied social contract is once the repo is in JuliaImages. For example, at the moment the Documenter script that builds the documentation is linked with my Github account using my credentials. I wouldn't know how to configure the equivalent in JuliaImages.

[johnnychen94]

I've never done such work before, but as far as I understand, the porting strategy is

1. duplicate the repository to JuliaImages by maintainers/owners (i.e., @timholy and others), and give the previous owner(i.e., @zygmuntszpak ) the full permission.
2. set up CI and Documenter under JuliaImages by maintainers/owners, change the license if there's a need.
3. submit a PR in the General registry to modify the package URL here, which might need agreements on both sides. This redirects new pulls to the duplicated repository.
4. post-operation: archive the original repository for reference usage(e.g., legacy issues, PRs, commits and statistics), and optionally deactivate related CI by the previous owner.

I think step 2, 3, and 4 can be done parallelly. Please correct me if there's anything I missed to consider.

For example, at the moment the Documenter script that builds the documentation is linked with my Github account using my credentials.

These will be set under JuliaImages independently with the social account linked to JuliaImages, just consider them as two repositories that happens to have the same name. Only owners of JuliaImages or repositories under that have the permission to set up those secrets (for Documenter.jl).

[kmsquire]

You should just be able to transfer the repo without duplicating it. Look under the repo settings. Steps 2 and 3 are still relevant.

[timholy]

I'd like to understand what moving it entails and what the implied social contract is once the repo is in JuliaImages.

In terms of social contract it's probably no different than for anything else that Images.jl depends on. Once Images.jl moved to JuliaImages, I don't think it makes sense to have Images.jl depend on anything "image-processing specific" that isn't hosted at JuliaImages. That's just insurance that important dependencies won't get abandoned by someone who doesn't merge PRs and doesn't add collaborators with sufficient permissions (not that I expect to have to worry about that in your case, but it's a good policy).

If you have more specific questions about "social contract" let us know.

[johnnychen94]

You should just be able to transfer the repo without duplicating it

Yes, I thought the previous URL will become invalid, but since it's actually get redirected to the new URL automatically, this isn't an issue. Transfering the whole repository is sufficient for step 1, thanks for the note @kmsquire

FYI, from https://help.github.com/en/github/administering-a-repository/transferring-a-repository#whats-transferred-with-a-repository

All links to the previous repository location are automatically redirected to the new location.

[johnnychen94]

Let's move the porting discussions #841

[zygmuntszpak]

I've transferred the ImageContrastAdjustment repository to the JuliaImages organisation, and have opened a pull-request to the General registry which updates the repo URL (JuliaRegistries/General#7492). @timholy I would be grateful if you could incorporate the changes you made in this pull-request in the ImageContrastAdjustment repo instead.

[timholy]

Thanks @zygmuntszpak! Would you like to do the honors of rebasing Images.jl on ImageContrastAdjustment? I also think that would be a good time to deprecate imhist and similar. (I can do it if you don't want to.)

[zygmuntszpak]

I had actually started the process of updating the Images code on a local branch and paused because I noticed that ImageContrastAdjustment had a replacement for everything except the clahe function. Hence the new implementation of clahe JuliaImages/ImageContrastAdjustment.jl#16

I am not entirely familiar with the deprecation strategy. Do I just reexport the functions in ImageContrastAdjustment and put a deprecation message in the existing imhist etc. functions?

[timholy]

Yep, that's pretty much it. You can use something along the lines of

Base.depwarn("`imhist` will be removed in a future release, please use `build_histogram` instead.", :imhist)