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: +49 (0)40 42883-2573 Vogt-Koelln-Str. 30 | | Fax: +49 (0)40 42883-2572 D - 22527 Hamburg | | Email: u.koethe@computer.org Germany | | koethe@informatik.uni-hamburg.de | | WWW: http://kogs-www.informatik.uni-hamburg.de/~koethe/ | |________________________________________________________________|