Re: [boost] GIL IO extension review

Subject: Re: [boost] GIL IO extension review
From: Christian Henning (chhenning_at_[hidden])
Date: 2010-12-06 13:56:03

Hi Kenny, thanks a lot for your review.

On Mon, Dec 6, 2010 at 12:52 PM, Kenny Riddile <kfriddile_at_[hidden]> wrote:
> 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.

I'll add the targa code as soon as I can.

> - 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.
>

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.

>
> - 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.

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