Re: [boost] Formal Review of IO and Toolbox extensions to Boost.GIL starts TOMORROW

Subject: Re: [boost] Formal Review of IO and Toolbox extensions to Boost.GIL starts TOMORROW
From: Zach Laine (whatwasthataddress_at_[hidden])
Date: 2010-12-09 13:33:00

On Tue, Nov 30, 2010 at 12:01 PM, Mateusz Loskot <mateusz_at_[hidden]> wrote:
> - What is your evaluation of the design?

Good.  I have no complaints.

> - What is your evaluation of the implementation?

It is generally very good, modulo a few bugs (see below).

There are some specific problems with the tests, though.  For
instance, png_read_test.cpp takes an image file, reads it, then writes
it to a temporary file, then re-reads it from the temp file, and
compares the two read-in images for equality.  This strikes me as a
bad test.  It is quite possible to read an image, get the wrong
results, then write those wrong results to disk, re-read them, and
then have the re-read results compare equal to the original read
results.  There may be other tests with a similar organization; I did
not check.

> - What is your evaluation of the documentation?

I was unable to build the docs.  I'm actually surprised that the
review manager allowed this to go to review without pre-generated,
web-accessible docs.

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

Very useful.

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

I only evaluated the PNG loading code, in an existing library that
already uses (a modified) GIL for image file loading.  My library's
modified copy of GIL has numerous fixes to the PNG loading code.  When
I substituted gil.io_new, I found that several files were again
unreadable.

A while back, I communicated all my fixes to GIL's old PNG IO code to
Christian, and at some point, those fixes were in place in an earlier
version of gil.io_new, and I could load up nearly all the files in the
PNG test image set correctly, using that earlier version of
gil.io_new.  The only ones that did not load correctly were the ones
that also don't appear correctly even in the test images as rendered
by Firefox and IE (e.g. notice the differences between the PNGs and
the reference GIFs in the two links at
http://www.schaik.com/pngsuite/pngsuite.html#background ).

Now, there are several PNG files that are not read correctly by
gil.io_new.  Here is the important part of the reading code (the
curious can see the rest at
http://freeorion.svn.sourceforge.net/viewvc/gigi/trunk/GG/src/Texture.cpp?revision=752&view=markup
-- though the linked code does not use gil.io_new, the changes below
are trivial):

    namespace gil = boost::gil;
    namespace fs = boost::filesystem;

    BOOST_STATIC_ASSERT((sizeof(gil::gray8_pixel_t) == 1));
    BOOST_STATIC_ASSERT((sizeof(gil::gray_alpha8_pixel_t) == 2));
    BOOST_STATIC_ASSERT((sizeof(gil::rgb8_pixel_t) == 3));
    BOOST_STATIC_ASSERT((sizeof(gil::rgba8_pixel_t) == 4));

    typedef boost::mpl::vector4<
        gil::gray8_image_t,
        gil::gray_alpha8_image_t,
        gil::rgb8_image_t,
        gil::rgba8_image_t
    > ImageTypes;
    typedef gil::any_image<ImageTypes> ImageType;

    fs::path path(filename);

    if (!fs::exists(path))
        throw BadFile("Texture file \"" + filename + "\" does not exist");

    if (!fs::is_regular_file(path))
        throw BadFile("Texture \"file\" \"" + filename + "\" is not a file");

    std::string extension = boost::algorithm::to_lower_copy(path.extension());

    ImageType image;
    try {
        // First attempt -- try just to read the file in one of the default
        // formats above.
#if GG_HAVE_LIBJPEG
        if (extension == ".jpg" || extension == ".jpe" || extension == ".jpeg")
            gil::read_image(filename, image, gil::jpeg_tag());
        else
#endif
#if GG_HAVE_LIBPNG
        if (extension == ".png")
            gil::read_image(filename, image, gil::png_tag());
        else
#endif
#if GG_HAVE_LIBTIFF
        if (extension == ".tif" || extension == ".tiff")
            gil::read_image(filename, image, gil::tiff_tag());
        else
#endif
            throw BadFile("Texture file \"" + filename + "\" does not
have a supported file extension");
//        std::cerr << "Success! (reading \"" << filename << "\").\n";
    } catch (const std::ios_base::failure &) {
        std::cerr << "Throw! trying again (\"" << filename << "\").\n";
        // Second attempt -- If *_read_image() throws, see if we can convert
        // the image to RGBA.  This is needed for color-indexed images.
#if GG_HAVE_LIBJPEG
        if (extension == ".jpg" || extension == ".jpe" || extension ==
".jpeg") {
            gil::rgba8_image_t rgba_image;
            gil::read_and_convert_image(filename, rgba_image, gil::jpeg_tag());
            image.move_in(rgba_image);
        }
#endif
#if GG_HAVE_LIBPNG
        if (extension == ".png") {
            gil::rgba8_image_t rgba_image;
            gil::read_and_convert_image(filename, rgba_image, gil::png_tag());
            image.move_in(rgba_image);
        }
#endif
#if GG_HAVE_LIBTIFF
        if (extension == ".tif" || extension == ".tiff") {
            gil::rgba8_image_t rgba_image;
            gil::read_and_convert_image(filename, rgba_image, gil::tiff_tag());
            image.move_in(rgba_image);
        }
#endif
    }

The offending files can be found under this SVN URL (for an
open-source video game, hence the crazy names):

https://freeorion.svn.sourceforge.net/svnroot/freeorion/trunk/FreeOrion/default/data/art/

Here are the files:

icons/tech/space_elevator.png
icons/tech/transcendent_architecture.png
icons/tech/pure-energy_metabolism.png
icons/tech/environmental_encapsulation.png
icons/tech/genetic_engineering.png
icons/tech/genetic_medicine.png
icons/tech/nanotech_medicine.png
icons/tech/orbital_farming.png
icons/tech/symbiosis_biology.png
icons/tech/xenological_genetics.png
icons/tech/artificial_minds.png
icons/tech/force-field_harmonics.png
icons/tech/gravitonics.png
icons/tech/mind_of_the_void.png
icons/tech/translingustic_thought.png
icons/tech/xenoarchaeology.png
icons/tech/singularity_generation.png
icons/tech/galactic_exploration.png
icons/tech/space_elevator.png
icons/tech/transcendent_architecture.png
icons/tech/nanotech_medicine.png
icons/tech/space_elevator.png
icons/tech/transcendent_architecture.png
icons/tech/nanotech_medicine.png
misc/meter_bar_shading.png
planets/atmospheres/white_atmosphere.png
planets/atmospheres/turquoise_atmosphere.png
planets/atmospheres/pink_atmosphere.png

> - How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

I only ran the application mentioned above using gil.io_new instead of
the previous customized GIL code it was using.

> - Are you knowledgeable about the problem domain?

Yes.  Specifically, I am very familiar with the PNG format.

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

Not as is.  If the PNG compliance is improved, my vote would be
changed to a yes.

Zach