On Tue, Nov 30, 2010 at 12:01 PM, Mateusz Loskot <mateusz@loskot.net> 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/defa... 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