GIL IO extension review

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.

Hi Kenny, thanks a lot for your review. On Mon, Dec 6, 2010 at 12:52 PM, Kenny Riddile <kfriddile@gmail.com> wrote:
I'll add the targa code as soon as I can.
The setjmp/longjmp solution is the best have come up with. If there is a better way I like to know. Regarding the read_intxx functions I could change that.
Yes, the documentation has to be updated a bit. I have to add a more boost specific layout for instance. That will be done before I add it the trunk, in case the review is successful Thanks again for your time, Christian
participants (2)
-
Christian Henning
-
Kenny Riddile