[boost] GIL - late review

From: Ullrich Koethe (koethe_at_[hidden])
Date: 2006-11-01 17:01:52

Hi all,

I was away from e-mail and unable to submit my GIL review in time. I hope 
it is still useful.

 > Please always explicitly state in your review, whether you think the
 > library should be accepted into Boost.

I vote to accept GIL into boost after revision. In addition, I consider 
addition of an algorithm library in the not-too-distant future mandatory 
in order to realize the full potential of the image classes.

 >  - What is your evaluation of the design?

I like the design of the image classes (including the dynamic_image) and 
the heavy use of views as a major algorithm interface. The various 
iteration abstractions are also quite powerful. The proof-of-concept 
implementation of the VIGRA interoperability layer was quite encouraging.

I'd like to see the following additions to the image class:
- constructors that allow copy construction of the pixels values
- resize() functions with default construction and copy construction of 
the newly allocated pixel values (without resize(), the default 
constructor image() is pointless).

Regarding the debate about DataAccessor vs. proxy-based iterator adapter: 
it seems that compilers are now sufficiently mature to make both methods 
work equally well.

I'm less convinced of the pixel classes (which, as I argued in another 
post, should be renamed into pixel_value or just value). IMHO, a pixel 
class without a set of operations specific to this particular pixel type 
is not very useful. As far as I can tell, there are not even conversion 
functions between RGB, LAB and HSB (and, unlike the RGB-CMYK conversion, 
these are too expensive to be implemented as pixel adapters).

I recommend to either
- drop the specific color classes for the time being and use a general 
static_vector<3, T> together with  accordingly changed pixel_value_tag 
concepts, or
- add a set of functions justifying the existence of the color classes.

Regarding pixel type conversion and mixed-valued functions: It was already 
pointed out that the resulting code bloat can easily get out of control, 
especially in the context of the dynamic_image. I read Lubomir's OOPSLA 
paper and think that it only offers a partial solution - it only covers 
the case were different type combinations actually create the same binary 
code. IMHO, this is not the most problematic case.

Consider just the scalar pixel types 'unsigned byte', 'signed short', 
'unsigned short', 'int', and 'float'. All of them are regularly occuring 
in VIGRA algorithms, and no combination will be binary equal to any other. 
What is needed is a consistent set of coercion rules. For example, 
'unsigned char' x 'unsigned char' might be implemented natively (for 
speed), while 'unsigned char' x 'float' will be implemented by a coercion 
of the first pixel to 'float', followed by a native 'float' x 'float' 
operation.

These rules, together with appropriate result_of types, can be easily 
formulated in a set a traits classes, and should also be customizable on a 
per-function-call basis. Coercion may not be a prerequisite for the 
usefulness of the core part of GIL, but it is critical for the 
dynamic_image and the future image algorithm library (including the color 
conversion function suggested above).

In comparision to the VIGRA import/export library, GIL/io is not very mature.

 >  - What is your evaluation of the implementation?

It is mostly OK. I would like the following improvements:

- there are several comments saying 'potential performance problem' (or 
equivalent) in the code. The code should be changed to eliminate these 
problems because the affected functions are performance critical.

- C-style casts and reinterpret_casts should be eliminated (if this is 
impossible in the latter case, the casts should be guarded by 
static_assert to do the right thing).

- proper initialization of the pixel values should be added (e.g. by 
uninitialized_fill).

- I didn't check in detail whether all functions (especially constructors 
and destructors) are fully exception safe. The authors should make a 
statement about this.

- code readability should be improved (more line-breaks).

- the test suite should be made part of the distribution.

 >  - What is your evaluation of the documentation?

It is inclomplete. I especially missed help on how to create custom pixel 
adapters, both in the read-only and read-write cases. The design guide is 
too abstract for my taste, it should contain more examples and cross 
references to the tutorial.

 >  - What is your evaluation of the potential usefulness of the library?

Images are a basic abstraction in most programms involving any kind of 
graphics. So they should definitely be part of boost.

 >  - Did you try to use the library? With what compiler? Did you have
 > any problems?

I used GIL mostly to test interoperability with VIGRA. It compiled without 
a major problem using cygwin gcc 3.4.4 on Windows XP SP2. However, when 
the dependencies (libtiff, libjpeg, libpng) are not in their standard 
locations, there is no mechanism to help finding them (the Makefile has to 
be edited by hand).

 >  - How much effort did you put into your evaluation? A glance? A quick
 > reading? In-depth study?

Quite a lot of code review, but not as much actual programming.

 >  - Are you knowledgeable about the problem domain?

I've been developing image processing libraries for over 10 years, and am 
the main author of the VIGRA library.

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/      |
|________________________________________________________________|