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

WIP: broader use of ColorValues and getting rid of limits with fixed-point numbers #135

Closed
wants to merge 59 commits into from

Conversation

timholy
Copy link
Member

@timholy timholy commented Jul 28, 2014

Currently this is waiting on a couple of other pull requests (JuliaMath/FixedPointNumbers.jl#2 and JuliaAttic/Color.jl#51), but I thought I'd post it for general consideration. This has two aims:

Get rid of array dimensions that correspond to color

This switches to ColorValue immutables for essentially all imread operations. (OK, not quite, but it's a major step in that direction.) Formerly, this was not possible without converting image pixel values to Float64, because ColorValue immutables were only supported for Float64. There are several reasons why forcing users to make such conversions would be undesirable. To overcome this barrier, one of my pending pull requests makes ColorValues parametric, and this allows us to use them for other data types.

Get rid of the limits property

Images has a fairly complicated infrastructure for scaling, which is primarily used for display by IJulia or ImageView. The default is that images stored in Uint8 are scaled from 0x00 to 0xff, whereas a Float32 image is scaled from 0.0f0 to 1.0f0. This causes a problem if you convert the underlying representation from Uint8 to Float32, which can easily happen if you filter or otherwise transform the image. A related problem is how to scale Uint16 images; if the scaling is always 0x0000 to 0xffff, then images produced by a 10-bit camera will always render as near-black. Handling these two problems is the main reason for the existence of the limits property, to allow arithmetic operations/file-format readers/users to annotate the image so that appropriate contrast limits are chosen.

However, maintaining the limits property under arithmetic operations is annoying and error-prone, and in some cases there is no obviously-correct way to do it. Recently @StefanKarpinski suggested a different approach: "virtually" change the pixel values, remapping unsigned integers implicitly to the range 0.0 to 1.0, but without changing the underlying (in-memory) representation. Hence my work on fixed-point numbers. I've implemented variants for all common bitdepth cameras.

I'm pretty excited that this will clean up a lot of stuff: once this gets merged, the default contrast limits can just be 0.0 to 1.0, although of course the user will be able to manually override them. But going forward this would only need to be something passed to the display code, and not an image property. Nevertheless it should ensure that, for most images, display "just works." That it will do so while allowing us to zero-copy pass the data to ImageView (and someday, OpenGL) is a big bonus.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1873739 on teh/fixednumbers into cad5e97 on master.

@lucasb-eyer
Copy link
Collaborator

Great, looking forward to see how this pans out!

  • How would I extract say the red channel of a picture?
  • The fixed-point stuff is really neat but, if not well executed/documented, it might be "too clever" and confuse everyone.

@timholy
Copy link
Member Author

timholy commented Jul 28, 2014

How would I extract say the red channel of a picture?

red = [img[i,j].r for j = 1:size(img,2), i = 1:size(img,1)]

The fixed-point stuff is really neat but, if not well executed/documented, it might be "too clever" and confuse everyone.

Agreed. It need a whole page of documentation to explain it, which is probably its major negative. On the other hand, most users may not really notice anything, as long as they see that their images look like floating-point images.

@kmsquire
Copy link
Collaborator

kmsquire commented Aug 4, 2014

I'm looking forward to this!

@timholy
Copy link
Member Author

timholy commented Aug 19, 2014

OK, after some pretty massive changes (with an overall reduction in the number of lines), this is almost ready to go. Just waiting on a couple of PRs in other packages. I also need to fix the OSX reader and some specific file formats (like NRRD).

At this point, most images need very little in the way of properties. Once julia 0.4 is a little more mature and I can write NamedAxesArrays properly, in many cases we may be able to do away with the Dict altogether. Of course it will still be available for additional metadata, but the "core" properties should all be inferrable from the parameters.

@StefanKarpinski
Copy link

What's your impression of how the custom numeric types for [0,1] are working out? @JeffBezanson just used that in a presentation today and it went over pretty well modulo a few missing methods.

@timholy
Copy link
Member Author

timholy commented Aug 20, 2014

I'm liking it so far. Since one of the main purposes was for display scaling, I'll know better once I start porting ImageView, but overall it just feels like the right way to go. I'm even mulling over whether to consider defining a new UfixedNaN16 type to add a NaN object these integer representations (by carving out a sentinel value). I doubt I'll do it, but there are some potential advantages.

If @JeffBezanson is missing methods, then he should merge JuliaMath/FixedPointNumbers.jl#7. 😄 Of course there are surely more. I'd be interested in hearing a more detailed report from him.

One thing this has pointed out is that, as more information gets nested into parameters, the need for triangular dispatch is growing. Here are a couple of examples. For the first, basically what I wanted was

scaleinfo{T<:Ufixed, CV<:Union(ColorValue{T},AbstractAlphaColorValue{T})}(img::AbstractArray{CV}) = ScaleNone(img)

but I was able to figure out how to get it by splitting out that two-argument _scaleinfo function. Nevertheless, that solution probably took 10 minutes to figure out, whereas the triangular dispatch solution is "obvious". The second seems like it should be simple, and I'm probably missing something, but I couldn't figure out any easy way to write it without resorting to accessing the parameters field. (Presumably a two-argument solution would also work here, but I haven't thought hard about it.)

@kmsquire
Copy link
Collaborator

Just for curiosity, what other package PRs are you waiting for? Is this branch in shape enough to be tested?

@timholy
Copy link
Member Author

timholy commented Aug 21, 2014

Jeff just merged one in FixedPointNumbers, and I merged one in Color. Looks like Jeff tagged FixePointNumbers, so I suspect that one is good. Color probably needs to be tagged, or just use master. Bottom line: everything else is merged.

On this branch, what I'm doing now is going through the documentation and testing all the examples (and also doing a lot of rewriting). So that suggests it's pretty close. Momentarily, I'll do a force-push of my current status. From that point on, I'll try not to do any more force-pushes so you can feel free to check it out.

There is one more big functional change I do want to make, however, before we release this to the public: I realized that with immutable Colors, I believe `getindex(img::Image, ::Range, ::Range, ::Range) can now safely return an Image rather than an array (you can guarantee that the colorspace isn't changing, whereas snipping out 2:3 from RGB would have required invalidation of the colorspace property).

@timholy
Copy link
Member Author

timholy commented Aug 21, 2014

OK, you've got the most current version.

@kmsquire
Copy link
Collaborator

Awesome! Looking forward to trying this out.

In addition to getting things working, this optimizes arithmetic
operations on AbstractRGBs to make them inline better and
reduce the number of multiplications.
This may be needed for a robust solution to the RGB/sRGB problem
This may make read/write comparisons more robust across
different library versions
@timholy timholy closed this Sep 1, 2014
@timholy timholy deleted the teh/fixednumbers branch September 1, 2014 14:44
@timholy timholy restored the teh/fixednumbers branch September 1, 2014 14:44
@timholy timholy mentioned this pull request Sep 1, 2014
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.

6 participants