From: Ullrich Koethe (koethe_at_[hidden])
Date: 2006-11-01 17:01:52
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
- 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'
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
- 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.
-- ________________________________________________________________ | | | Ullrich Koethe Universitaet Hamburg / University of Hamburg | | FB Informatik / Dept. of Informatics | | AB Kognitive Systeme / Cognitive Systems Group | | | | Phone: +49 (0)40 42883-2573 Vogt-Koelln-Str. 30 | | Fax: +49 (0)40 42883-2572 D - 22527 Hamburg | | Email: u.koethe_at_[hidden] Germany | | koethe_at_[hidden] | | WWW: http://kogs-www.informatik.uni-hamburg.de/~koethe/ | |________________________________________________________________|
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk