Re: [boost] GIL - late review

From: Ullrich Koethe (koethe_at_[hidden])
Date: 2006-11-02 09:58:18

Hi Lubomir,

Lubomir Bourdev wrote:
> 
> As I already mentioned, we also would like very much to see image
> processing algorithms added to GIL, whether or not it is accepted in
> Boost.

Of course, but I'd like to postpone the discussion about algorithms until 
after the decision regarding boost.

>>I'd like to see the following additions to the image class:
>>- constructors that allow copy construction of the pixels values
> 
> 
> You mean deep-copy the images? The image class has a copy constructor
> which deep-copy's the images.
> 

No, I was unclear. I mean that it should be possible to provide an initial 
pixel value (which is copied into all pixels), like

    some_image_t img(a_size, an_initial_pixel_value);

> 
>>- resize() functions with default construction and copy 
>>construction of 
>>the newly allocated pixel values (without resize(), the default 
>>constructor image() is pointless).
> 
> 
> There is a function called resize_clobber_image. Is that what you are
> looking for?

Probably. But not being a native English speaker, I have no idea what 
'clobber' stands for. In addition, I'd expect resize() to be a member 
function of the image class, similar to std::vector. I agree that the old 
pixel values need not be preserved. The above comment about an initial 
pixel value applies to resize() as well:

    sime_image.resize(new_size, an_initial_pixel_value);

The policy should be: the member function image::resize() just resizes the 
memory without copying pixel values, whereas the free functions 
resize...() interpolate (or drop) pixel values, but assume the right 
amount of memory to be already allocated.

> 
> 
> Why does VIGRA have a separate class for RGB Value then? What is special
> about RGB?
> 

RGB has some specific functions (e.g. red(), luminance()), and I'm not 
arguing against an RGB value class. I'm just saying that

1. It may not be appropropriate to represent each and every color model by 
their own class.
2. If a color model does not have specific functions (in the current 
version of the code base), a specific class is definitely not warranted.

This applies especially to the lab and hsb classes in the current GIL version.

> There are lots of functions that take the color information into
> account. Color information goes down to the very basics. 

Yes, but there are other solutions than providing a new class, e.g. a 
string tag in the image class. It is not as type safe, but simpler and 
more flexible. Type safety may not be the main consideration here, because 
type errors in color space conversions are easily visible during testing.

> You are right that some color conversion combinations are not yet
> provided. This has nothing to do with the performance to do color
> conversion, but more with the fact that there is a combinatorial
> explosion in the number of conversions - every color space to every
> other.

That's another area were coercion is needed. By providing rules for 
multistep conversion, the actual conversion code can be generated 
automatically from a few basic building blocks.

> Color conversion between RGB and LAB could be implemented just like it
> is done between, say, RGB and CMYK.
> Is there any performance advantage to implementing it differently?
>

No, but the RGB-LAB conversion is so expensive that the only practical 
relevant use of an lab_view is

    copy_pixels(lab_view(some_rgb_image), view(lab_image));

Naive users may not be aware of the cost, and may use an lab_view as an 
argument to convole(), say. Therefore, I'd prefer a functor-based solution 
in that case, such as

    transform_image(view(rgb_image), view(lab_image), rgb2lab());

This cannot be accidentaly misused.

> Their color space is devicen_t<N> where N is the number of channels.
> (We are open to suggestions for a better name)
> 

I suggest static_vector (which should actually be a boost module in its 
own right, as an improvement over the old boost::array - or does it 
already exists somewhere in boost?). static_vector should provide most of 
the std::vector interface (less the resizing operations), basic arithmetic 
(component-wise +, -, *, scalar *, /), equality ==, !=, norm() and dot(). 
Look at vigra::TinyVector, which has worked well for us. static_vector can 
also serve as a base class for other color spaces.

> 
> As described in my paper, the types can be equivalent in the context of
> the algorithm.
> For example, for bitwise-copy, copying "signed short to signed short"
> and copying "unsigned short to unsigned short" can be treated as
> equivalent.
> 

Yes, but a copy between uniform types is about the only example were this 
works.

> 
> I can see how automatically converting one type to another can be
> useful, but I am not sure how this relates to the topic of
> dynamic_image.

Suppose you want to support something like:

  transform(dynamic_image1, dynamic_image2, dynamic_image3, _1 + _2);

for a large set of possible pixel types. Then your method of code bloat 
reduction by binary compatibility only applies to very few instances. One 
also needs coercion, i.e. one splits transform() into two parts:

- converting all input into a one among a few supported forms (e.g. leave 
uniform type input untouched, but convert mixed type input to the highest 
input type)

- doing the actual computation with only the few supported type combinations.

So, only a lot of conversion functions are needed (which are simple and 
don't bloat the code too much), whereas all complicated functions are 
instantiated only for a few types.

> I agree, on a case by case basis. We are trying to get what we believe
> are the bottlenecks as efficient as possible. It would be quite a job to
> make all parts of GIL completely optimal. Very often extra efficiency
> comes at the cost of code size, extra stuff in data structures, or
> decreasing the performance in other places of the code... Sometimes the
> tradeoff is not worth it.
> 

Agreed. But sometimes the necessity of reinterpret_cast is a sign of 
suboptimal design or implementation decisions.

> For example, currently view.end() is not very efficient as it is
> implemented as "return begin()+size()".
> If you say:
> for (it=view.begin(); it!=view.end(); ++it) {...}
> 

Well, I surely hope that the optimizer will take care of this?

Regards
Ulli
-- 
  ________________________________________________________________
|                                                                |
| Ullrich Koethe  Universitaet Hamburg / University of Hamburg   |
|                 FB Informatik        / Dept. of Informatics    |
|                 AB Kognitive Systeme / Cognitive Systems Group |
|                                                                |
| Phone: [phone]                Vogt-Koelln-Str. 30 |
| Fax:   [phone]                D - 22527 Hamburg   |
| Email: u.koethe_at_[hidden]               Germany             |
|        koethe_at_[hidden]                        |
| WWW:   http://kogs-www.informatik.uni-hamburg.de/~koethe/      |
|________________________________________________________________|