Re: [boost] Review request: gil::io

Subject: Re: [boost] Review request: gil::io
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2009-05-13 20:20:20

On Thu, May 7, 2009 at 9:51 AM, Ronald Garcia <garcia_at_[hidden]> wrote:
>> On Thu, May 7, 2009 at 10:11 AM, Ronald Garcia <garcia_at_[hidden]> wrote:
>>>
>>> Hi Christian,
>>>
>>> I have received your request and have added this library to the review
>>> queue.

Here's a pre-review of the library.  I looked almost exclusively at
PNG loading, which is the thing I need most from gil::io.  I found a
lot of issues that really need addressing.  In my opinion, the work so
far is not review ready, due to the lack of adequate tests and docs.
However, the fundamentals look good, and with more complete tests the
end results will hopefully be great.  Now the details:

Documentation:
- What are the minimum requirements of image_read_info<>,
image_read_settings<>, and other templates?  Essentially, I'm asking
for concept documentation for these things.  Even if there is no
common subset among the various specializations, you should state that
explicitly.  Also, forcing yourself to do this will allow you to catch
things like the fact that image_read_info<png_tag>._num_channels,
image_read_info<tiff_tag>._samples_per_pixel and
image_read_info<jpeg_tag>._num_components mean the same thing but have
different spellings.  This should probably be spelled the same in all
specializations, so generic code can use the common members.
- There is no reference section.  It took me a good long while
grepping about in the code to find the things I was looking for (in
particular, image_read_info<>, so I could see what the available
parameters where for PNGs in particular).
- There are several formatting problems with the QuickBook docs, like
_Supported_Formats_ on the tutorial page, which looks wrong
formatting-wise, and points to the root of the GIL documentation tree,
instead of where ever it should point.
- read_image() throws std::runtime_error and std::ios_base::failure.
The old *_read_image() functions only threw std::ios_base::failure.
This should be documented.  Better yet, the new function should only
throw std::ios_base::failure so you don't silently break existing
code.

Implementation:
- Support for direct reads into any_image<>s appears to have been
dropped.  This is a pretty big regression, considering that this usage
pattern was presented as a key selling point in the original GIL docs.
 Is the new recommendation that I use read_image_info to find out the
image properties I care about, and then have N cases for the different
formats I want to support?  This is a lot klunkier than the
direct-read-into-any_image<> method.  It requires me to know what the
different spellings of _num_channels are for all the file types I want
to support; it requires me to know which of my desired image types are
supported for which file formats; and it requires me to write a *lot*
of code that GIL formerly generated for me.
- Macros are defined like ENABLE_GRAY_ALPHA and ADD_FS_PATH_SUPPORT
(maybe others?) that do not begin with BOOST_, GIL_, or BOOST_GIL_.
- Is there a compelling reason why ENABLE_GRAY_ALPHA is not on by
default?  As it stands, it's just one more macro I have to add to my
build system to get support for the full range of image formats.
- When you define ENABLE_GRAY_ALPHA, a compilation error results,
since you left extension/toolbox/gray_alpha.hpp (and in fact all of
toolbox) out of the zip file you posted.
- There are some noisy "comparison between signed and unsigned integer
expressions" warnings coming out of formats/tiff/is_allowed.hpp, line
159 when using GCC.
- formats/png/is_allowed.hpp and formats/jpeg/is_allowed.hpp are
essentially empty.  These really need to be implemented so that I can
get an exception if I try to read e.g. a 16 bit image file into an 8
bit GIL image.  Right now I just get a garbage image.
- There is quite a bit of commonality to the implementations of
is_allowed<>() for the various formats.  This should probably be
factored out and reused.
- Mixed LF and CR/LF line endings (a nit, I know).

Tests:
- You're not testing nearly enough stuff, IMO.  For instance, it
appears that read_image_info<png_tag>()._num_channels is always 0
(because it is never set).  More complete tests should have revealed
this.  Also, when I changed the code to use png_get_channels() to set
the number of channels, my reading code for 16 bit pixels stopped
working, since I was reading into 8 bit images.  This attempt to read
a 16 bit file into an 8 bit GIL image should throw, but doesn't, again
because is_allowed<>() is not defined for PNGs.
- You're not really testing that what you read is what was in the
file.  I can't suggest a good way to do this other than visual
inspection.  You are reading PNGs completely wrong for a fairly large
subset of the PngSuite images (all the interlaced ones, all the ones
with embedded gamma, all the ones with a background color, and all the
ones with nonsquare pixels).  The latter two categories are almost
never used with PNGs, but the former two are first-order PNG features.
- In one of the read_image() overloads, on line 97 of read_image.hpp,
there appear to be missing parentheses after "get_info".  Again, it
looks like this function is simply not being exercised by the tests.

I've attached a patch that fixes PNG reading.  The PNG reader can now
correctly read all the (non-error-case) PngSuite images when used via
read_image(); it throws otherwise.  All the non-error PngSuite images
can be read using read_and_convert_image().

Changes include:
- Implemented is_allowed<>() for the PNG format.
- Placed the TIFF and PNG is_allowed<>() implementations in different
namespaces as a hack to prevent collisions.  This should of course be
refactored.
- Added a user gamma value to image_read_settings<png_tag>.
- Added code to reader<..., png_tag, ...>::get_info() to get the
number of color channels and the gamma factor set in the file.
- Fixed interlaced PNG reading.

Background colors and nonsquare pixels are still not handled properly,
but like I said before, no one will probably care.

Zach