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

Subject: Re: [boost] Review request: gil::io
From: Christian Henning (chhenning_at_[hidden])
Date: 2009-05-14 11:28:11

Hi Zach,

thanks a lot for your pre-review. Please read below.

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

I agree more tests are necessary. There are already about 120 tests
and counting.

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

image_read_info<> is suppose to represent the image's header, inc all
information needed for reading but also for other meta-information,
like exposure settings, for instance.

image_read_setting<> gives the user the chance to setup reading
parameters. One example is the dct_method when reading a jpeg file.

The same goes when writing images.

I would love to generalize these structure by defining concepts. I'm
not necessarily an expert in this field and would need help from
others. In case somebody wants to help please contact me.

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

A reference section is missing, I agree. Did you start with the tutorial?

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

I define [link _Supported_Formats_ Supported Image Formats] and I
thought it would link to the [h2 Supported Image Formats] more down in
the documentation. But honestly, I'm a novice when it comes to
Quickbooks. I once compiled the doc to a XML file but never could
never figure out how to create html or pdf.

Did you try to compile the doc? If so, can you show me how to do that?

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

Done.

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

Support for dynamic images has been added to the trunk and will be
part in the next release candidate.

> - Macros are defined like ENABLE_GRAY_ALPHA and ADD_FS_PATH_SUPPORT
> (maybe others?) that do not begin with BOOST_, GIL_, or BOOST_GIL_.

Will do. I'll try BOOST_GIL_XXX.

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

ENABLE_GRAY_ALPHA introduces a dependency to the toolbox. I'm starting
to think to include the toolbox to the review. It would be very neat
to have more color_spaces and algorithms in gil. But first I got to
clean up that box. ;-)

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

I'll investigate. I'm trying hard to eliminate all warnings possible.

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

I think you might be right and I should have a runtime check for all
formats, as well.

> - Mixed LF and CR/LF line endings (a nit, I know).

Any idea how to get rid of those? I'm on Windows and don't know of a
good tool to do that.

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

Can there ever be enough testing? ;-) As, I said above, I'll be adding
is_allowed<> for all other formats.

The next planned step is to introduce read and write tests for all
formats. This way I can prove all my claims in the documentation are
correct.

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

I'll have a look at your changes. Thanks a lot!!

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

Mhmm, interesting. I'll fix that and add the appropriate 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().

Thanks!!!

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

I care. Any ideas on how to do that. For instance I could imagine to
create an image type with both a background image and the actual
image. But what do to with non-square pixels? Gil doesn't support this
kind of thing yet.

All in all thanks again for your insight. Your requested code changes
are rather easy to implement and I'll go ahead and finish those. As
for tests and documentation, luckily the review queue is moving quite
slow. ;-) I'll do my best to improve the current situation.

>
> Zach
>
Christian