[boost] GIL IO extension review

Subject: [boost] GIL IO extension review
From: Kenny Riddile (kfriddile_at_[hidden])
Date: 2010-12-06 12:52:47

Sorry if this review seems short, but most of my time is spoken for. 
Still, I think this extension is useful enough that I found time to 
review it.  I've been using the new version of the GIL IO extension for 
over a year now and am glad to finally see it up for review.  I've 
primarily used it for loading various image formats for use as OpenGL 
textures.  I recently gained some experience with extending the library 
to support an additional image format (TARGA) as well.

- What is your evaluation of the design?

My use case for the library is conceptually simple, so I would expect 
the library to make it simple to accomplish.  The provided read_image() 
and write_view() functions are about as simple an interface as one could 
expect, but without sacrificing flexibility and extensibility due to the 
usage of tag dispatching and standard iostreams.  As a user, I have no 
complaints about the design and how it integrates with GIL.

- What is your evaluation of the implementation?

I hadn't looked at the implementation much prior to recently adding 
support for reading and writing TARGA images, and my evaluation of the 
implementation is from that standpoint.  The fact that I (of all people) 
was able to easily add support for a new format without much 
documentation on the subject suggests that the relevant parts of the 
implementation are easily understandable.  I mostly used the BMP 
implementation as a reference, and didn't notice anything egregious or 
suspicious.  I have some concerns about the usage of setjmp/longjmp in 
the PNG and JPEG implementations (honestly I get suspicious any time I 
see those).  I think there was discussion elsewhere about adding 
additional test cases to exercise the error cases that result in a 
longjmp so that all of the supported platforms can be verified.  I think 
this is a good idea.  Another nitpick that came up when adding TARGA 
support are the read_int8, read_int16, read_int32, write_int8, etc. 
member functions of the Device models in io_device.hpp.  To me, these 
names suggest that these functions return and accept signed integers 
when in reality they return and accept unsigned integers.

- What is your evaluation of the documentation?

The documentation was sufficient enough for me to quickly figure out how 
to read and write images from all of the supported formats.  The section 
on extending the library to support new formats was enough to get me 
started, but I ended up just relying on the existing BMP code as a 
reference.  This suggests that maybe that section of the documentation 
should be fleshed out a little more.

- What is your evaluation of the potential usefulness of the extensions?

This new IO extension is extremely useful.  It nicely fills huge gaps in 
the existing IO extension, such as loading from streams instead of only 
being able to supply a file name.  The interface is simple enough to 
integrate into just about any design, while still providing both 
flexibility and extensibility.

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

I've used the proposed IO extension to read and write BMP, PNG, JPEG, 
and TARGA files with both MSVC9 and GCC in combination with the last 
several Boost releases.  I had no problems.

- Do you think the extensions should be accepted as a part of Boost.GIL
library?

Yes, provided additional tests are added to exercise currently untested 
error cases, and they pass on all platforms officially supported by 
Boost.  This extension is just as simple to use as the existing GIL IO 
extension, yet is better in every way.  Therefore, I see no reason not 
to accept it as a replacement.