Formal Review of IO and Toolbox extensions to Boost.GIL starts TOMORROW
Dear All, According to the Boost Formal Review Schedule [1], review of Christian Henning's extensions to the Boost Generic Image Library (Boost.GIL, [2]), it is: * Boost.GIL.IO * Boost.GIL.Toolbox starts on December 1st and lasts until December 10th, 2010. [1] http://www.boost.org/community/review_schedule.html [2] http://www.boost.org/doc/libs/release/libs/gil/doc/index.html =========== What is it? =========== The Boost.GIL.IO extension provides an easy to use interface for reading and writing various image formats. It also includes a framework for adding new formats. The Boost.GIL.IO is indent to replace the current IO extension which is part of Boost for several years now. Features: * A unified way to read and write image encoded in BMP, JPEG, PNG, PNM, and TIFF formats. The capabilities to read and write in various formats have improved dramatically. * Image data can be provided via standard file or string streams The Boost.GIL.Toolbox provides new color spaces and other small code to ease programming with Boost.GIL. Features: * Implementation of color spaces: Gray_Alpha, HSL, HSV, LAB, and XYZ * Utilities to support dynamic image workflows and color conversions. * Collection of metafunctions to determine alignment, similarity and homogeneity at pixel level. The Boost.GIL as well as the proposed extensions are provided in form of a headers-only library Although, some image formats come with their format dependency, it is corresponding third-party libraries: JPEG - libjpeg PNG - libpng TIFF - libtiff =================== Getting the library =================== The latest version of both extensions can be downloaded as Zip package: http://gil-contributions.googlecode.com/files/boost_review.zip or directly from the Subversion repository: http://code.google.com/p/gil-contributions/source/browse/trunk/gil_2/ and the docs can built as usual with bjam and quickbook tools from within libs/gil/io_new/doc directory The libs/gil/io_new/test/readme.txt provides a step by step guide to configuring, building and running the unit tests. The boost_review.zip is about 20MB due to its extensive collection of test images. They are part of the test suite to make sure that different variations of each image format is read and written correctly. Please, be aware no guarantee can be given that all formats in their all variants are completely supported. ================ Writing a review ================ If you feel the new IO and Toolbox are interesting extensions to the Boost.GIL library, then please submit your review to the developer list (preferably), or to the review manager (mateusz at loskot dot _n_e_t_) Here are some questions you might want to answer in your review: - What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the extensions? - Did you try to use the extensions? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain? And finally, every review should answer this question: - Do you think the extensions should be accepted as a part of Boost.GIL library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Mateusz Review Manager for the proposed Boost.GIL.IO and Boost.GIL.Toolbox extensions -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org Member of ACCU, http://accu.org
Hi Christian, Mateusz and others Mateusz Loskot <mateusz@loskot.net> wrote:
According to the Boost Formal Review Schedule [1], review of Christian Henning's extensions to the Boost Generic Image Library (Boost.GIL, [2]), it is: * Boost.GIL.IO * Boost.GIL.Toolbox starts on December 1st and lasts until December 10th, 2010.
The boost_review.zip is about 20MB due to its extensive collection of test images. They are part of the test suite to make sure that different variations of each image format is read and written correctly.
It looks like these test images are not included in your Google Code repository, which is not unreasonable; how do you plan to handle this in the Boost repository, if accepted? Are there any precedents elsewhere in Boost? Also, can you point out which images and tests exercise the error paths that we discussed at length (e.g. setjmp/longjmp) a few months ago? Thanks, Phil.
Hi Phil, good to hear from you. On Fri, Dec 3, 2010 at 1:54 PM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
Hi Christian, Mateusz and others
Mateusz Loskot <mateusz@loskot.net> wrote:
According to the Boost Formal Review Schedule [1], review of Christian Henning's extensions to the Boost Generic Image Library (Boost.GIL, [2]), it is: * Boost.GIL.IO * Boost.GIL.Toolbox starts on December 1st and lasts until December 10th, 2010.
The boost_review.zip is about 20MB due to its extensive collection of test images. They are part of the test suite to make sure that different variations of each image format is read and written correctly.
It looks like these test images are not included in your Google Code repository, which is not unreasonable; how do you plan to handle this in the Boost repository, if accepted? Are there any precedents elsewhere in Boost?
Yeah, I separated them for the time being. I don't think it's big deal to add a bunch of test images to the boost source tree. It isn't really that big after all. When running the test suite I write out a lot of pictures which amounts to 5MB. But this is only done once since the files are being overridden at the next run. If there is resistance to add 20MB in test images I would have to resort to images in memory ( std::stringstream ). I cannot recommend such approach. The biggest problem I can see is the configuration of the test suite. There are some manual steps involved, as they are described in the readme.txt. I need to talk to the boost build guys to get the configuration done automatically. But that's for after the review.
Also, can you point out which images and tests exercise the error paths that we discussed at length (e.g. setjmp/longjmp) a few months ago?
Every time you read/write a jpeg or a png you use the setjmp/longjmp combination. I have tested it on my own machine and it works pretty well. There are no tests that are targeted specifically at setjmp/longjmp. Does that answer your question? Regards, Christian
Christian Henning wrote:
On Fri, Dec 3, 2010 at 1:54 PM, Phil Endecott wrote:
Also, can you point out which images and tests exercise the error paths that we discussed at length (e.g. setjmp/longjmp) a few months ago?
Every time you read/write a jpeg or a png you use the setjmp/longjmp combination.
Really? My understanding was that longjmp is only called when there is an error. Can you explain the code path that involves longjmp when a file is read or written successfully? Cheers, Phil.
Hi Phil, On Sat, Dec 4, 2010 at 12:13 PM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
Christian Henning wrote:
On Fri, Dec 3, 2010 at 1:54 PM, Phil Endecott wrote:
Also, can you point out which images and tests exercise the error paths that we discussed at length (e.g. setjmp/longjmp) a few months ago?
Every time you read/write a jpeg or a png you use the setjmp/longjmp combination.
Really? My understanding was that longjmp is only called when there is an error. Can you explain the code path that involves longjmp when a file is read or written successfully?
Sorry, there is a misunderstanding here. I should have clearer. For instance, when reading a jpeg you set a mark with setjmp() see read.hpp[49] in the jpeg folder. Just before that I set the error handler "error_exit()" which will be called in case of an error. It's only here that longjmp will be called. Though, you are correct in your assumption that longjmp is only called when there is an error. Hope you find some time to review my work. Regards, Christian
Christian Henning wrote:
On Fri, Dec 3, 2010 at 1:54 PM, Phil Endecott wrote:
Also, can you point out which images and tests exercise the error paths that we discussed at length (e.g. setjmp/longjmp) a few months ago?
Sorry, there is a misunderstanding here. I should have clearer. For instance, when reading a jpeg you set a mark with setjmp() see read.hpp[49] in the jpeg folder. Just before that I set the error handler "error_exit()" which will be called in case of an error. It's only here that longjmp will be called. Though, you are correct in your assumption that longjmp is only called when there is an error.
Great, we're agreed. So, can you now point out which tests exercise this setjmp/longjmp error path? Thanks, Phil.
Hi Phil,
Sorry, there is a misunderstanding here. I should have clearer. For instance, when reading a jpeg you set a mark with setjmp() see read.hpp[49] in the jpeg folder. Just before that I set the error handler "error_exit()" which will be called in case of an error. It's only here that longjmp will be called. Though, you are correct in your assumption that longjmp is only called when there is an error.
Great, we're agreed. So, can you now point out which tests exercise this setjmp/longjmp error path?
There are no tests that read an erroneous images. I remember testing setjmp/longjmp on my laptop and it worked with Windows. If you insists I can add some tests to prove my point. Christian
Christian Henning wrote:
There are no tests that read an erroneous images. I remember testing setjmp/longjmp on my laptop and it worked with Windows. If you insists I can add some tests to prove my point.
I'm not in a position to "insist" on anything, and it's not a question of "proving a point". It seems to me that the combination of setjmp/longjmp, C++-calling-C-calling-C++, exceptions and destructors that is involved in this error-handling mechanism is going to poke into a buggy corner on at least one of the platforms that Boost claims to support, and it would be much better for that to be detected during testing, rather that when a user accidentally tries it on a production system. I would also say that if you have gone to the trouble of creating test images that are corrupted in sufficiently subtle ways to trigger these error paths, then it would be valuable to make them public. Regards, Phil.
Hi Phil,
I'm not in a position to "insist" on anything, and it's not a question of "proving a point". It seems to me that the combination of setjmp/longjmp, C++-calling-C-calling-C++, exceptions and destructors that is involved in this error-handling mechanism is going to poke into a buggy corner on at least one of the platforms that Boost claims to support, and it would be much better for that to be detected during testing, rather that when a user accidentally tries it on a production system.
I would also say that if you have gone to the trouble of creating test images that are corrupted in sufficiently subtle ways to trigger these error paths, then it would be valuable to make them public.
My corrupted images were merely .txt files. ;-) When reading the header libjpeg would issue an error and the io extension will throw an exception. Now, we can argue such testing is insufficient and I would agree but that's what we have for now. I'm gladly incorporate some corrupted image reading into the test suite. Christian
On 6/12/2010 17:19, Christian Henning wrote:
My corrupted images were merely .txt files. ;-) When reading the header libjpeg would issue an error and the io extension will throw an exception. Now, we can argue such testing is insufficient and I would agree but that's what we have for now. I'm gladly incorporate some corrupted image reading into the test suite.
Christian
Have you looked into how browsers test against malicious attacks? IIRC they take valid images and change them in a "educatedly random" fashion. (i.e. all kinds of header corruption) As images are a common attack vector for malicious attacks I think that kind of testing is quite important. Fabio
Hi Fabio,
Have you looked into how browsers test against malicious attacks? IIRC they take valid images and change them in a "educatedly random" fashion. (i.e. all kinds of header corruption) As images are a common attack vector for malicious attacks I think that kind of testing is quite important.
I think you bring up a valid point. I'll make an entry in the todo list to add some invalid reads. Now how do I create a good cross selection of invalid jpeg, tiff, png, bmp, and pnm images? I'm open for suggestions. Christian
On 6/12/2010 19:11, Christian Henning wrote:
Hi Fabio,
Have you looked into how browsers test against malicious attacks? IIRC they take valid images and change them in a "educatedly random" fashion. (i.e. all kinds of header corruption) As images are a common attack vector for malicious attacks I think that kind of testing is quite important.
I think you bring up a valid point. I'll make an entry in the todo list to add some invalid reads. Now how do I create a good cross selection of invalid jpeg, tiff, png, bmp, and pnm images? I'm open for suggestions.
Note that I also do not have any first hand experience with it, but from what I have heard some forms of randomized (with a logged or fixed seed) fault injection (https://secure.wikimedia.org/wikipedia/en/wiki/Fault_injection) or fuzz-testing (https://secure.wikimedia.org/wikipedia/en/wiki/Fuzz_testing) is quite effective for that kind of testing. A quick google search turned up this (http://www.securiteam.com/tools/6P00B1FNFM.html) for a jpeg fuzzer (haven't checked the license though) I think adding something like this to the test suite would be the most efficient approach, especially since scripted fuzzing does not take too much diskspace. HTH Fabio
Hi Fabio, On Mon, Dec 6, 2010 at 4:08 PM, Fabio Fracassi <f.fracassi@gmx.net> wrote:
Note that I also do not have any first hand experience with it, but from what I have heard some forms of randomized (with a logged or fixed seed) fault injection (https://secure.wikimedia.org/wikipedia/en/wiki/Fault_injection) or fuzz-testing (https://secure.wikimedia.org/wikipedia/en/wiki/Fuzz_testing) is quite effective for that kind of testing. A quick google search turned up this (http://www.securiteam.com/tools/6P00B1FNFM.html) for a jpeg fuzzer (haven't checked the license though)
I think adding something like this to the test suite would be the most efficient approach, especially since scripted fuzzing does not take too much diskspace.
Don't you think that adding a fuzzer to the boost source tree is a little dangerous? Same goes "fuzzed" jpegs since they might be picked up by a virus scanner. I rather not add such dodgy data into the boost community. That doesn't mean I shouldn't test that. I just have to find a better way. Anyone has a good idea? Regards, Christian
On 7/12/2010 21:21, Christian Henning wrote:
Hi Fabio,
On Mon, Dec 6, 2010 at 4:08 PM, Fabio Fracassi<f.fracassi@gmx.net> wrote:
Note that I also do not have any first hand experience with it, but from what I have heard some forms of randomized (with a logged or fixed seed) fault injection (https://secure.wikimedia.org/wikipedia/en/wiki/Fault_injection) or fuzz-testing (https://secure.wikimedia.org/wikipedia/en/wiki/Fuzz_testing) is quite effective for that kind of testing. A quick google search turned up this (http://www.securiteam.com/tools/6P00B1FNFM.html) for a jpeg fuzzer (haven't checked the license though)
I think adding something like this to the test suite would be the most efficient approach, especially since scripted fuzzing does not take too much diskspace.
Don't you think that adding a fuzzer to the boost source tree is a little dangerous? Same goes "fuzzed" jpegs since they might be picked up by a virus scanner.
I didn't think about virus scanners. I don't think a fuzzer script would be a problem though, the fuzzed images might be another matter. If it is really a problem, The fuzzer tests could be separated to another archive.
I rather not add such dodgy data into the boost community.
I don't think they are dodgy, fuzzing is a legitimate testing facility. AFAIK Mozilla and other Browsers use it. regards Fabio
"Phil Endecott" <spam_from_boost_dev@chezphil.org> wrote in message news:1291649043264@dmwebmail.dmwebmail.chezphil.org...
I'm not in a position to "insist" on anything, and it's not a question of "proving a point". It seems to me that the combination of setjmp/longjmp, C++-calling-C-calling-C++, exceptions and destructors that is involved in this error-handling mechanism is going to poke into a buggy corner on at least one of the platforms that Boost claims to support, and it would be much better for that to be detected during testing, rather that when a user accidentally tries it on a production system.
IMO the easiest way to handle the setjmp/longjmp issues for those that have to use the LibJPEG and/or LibPNG backends would be to simply skip the xjmp functions altogether and throw proper C++ exceptions from the error handlers...This might require (re)building the backend lib (and possibly linking statically) with C++ exceptions enabled on some platforms but this might prove to be easier in the end/long run... -- "What Huxley teaches is that in the age of advanced technology, spiritual devastation is more likely to come from an enemy with a smiling face than from one whose countenance exudes suspicion and hate." Neil Postman
Hi, 2010/12/7 Domagoj Saric <dsaritz@gmail.com>:
[...]
IMO the easiest way to handle the setjmp/longjmp issues for those that have to use the LibJPEG and/or LibPNG backends would be to simply skip the xjmp functions altogether and throw proper C++ exceptions from the error handlers...This might require (re)building the backend lib (and possibly linking statically) with C++ exceptions enabled on some platforms but this might prove to be easier in the end/long run...
I think we should get rid of those backends entirely. Phil stated interesting use cases in a different thread, that might not be accomplished with these libraries. Additionally to that I made bad experiences with libpng and non broken but somehow not conformant png images. There are alternatives to using libpng and libjpeg: * LodePNG: http://members.gamedev.net/lode/projects/LodePNG/ * STB_Image http://nothings.org/stb_image.c I havent used the stb jpeg loader, but I switched to lodepng for the non-gil code. Regards, Andreas Pokorny
On 08/12/10 20:43, Andreas Pokorny wrote:
2010/12/7 Domagoj Saric<dsaritz@gmail.com>:
[...]
IMO the easiest way to handle the setjmp/longjmp issues for those that have to use the LibJPEG and/or LibPNG backends would be to simply skip the xjmp functions altogether and throw proper C++ exceptions from the error handlers...This might require (re)building the backend lib (and possibly linking statically) with C++ exceptions enabled on some platforms but this might prove to be easier in the end/long run...
I think we should get rid of those backends entirely. Phil stated interesting use cases in a different thread, that might not be accomplished with these libraries. Additionally to that I made bad experiences with libpng and non broken but somehow not conformant png images.
Andreas, Thank you for sharing your opinion. However, I'm afraid I am not sure how to count your post. Shall we consider your post as a review and voting? If yes, I'm afraid it is not complete enough to be taken as a review. Would you be willing to post more complete review and clearly cast your vote by specifying "yes" or "no"? Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org Member of ACCU, http://accu.org
"Andreas Pokorny" <andreas.pokorny@gmail.com> wrote in message news:AANLkTinNYQxRcDrkD_Hrw2z6YASbDypLUdTWhQbOXeb3@mail.gmail.com...
I think we should get rid of those backends entirely. Phil stated interesting use cases in a different thread, that might not be accomplished with these libraries. Additionally to that I made bad experiences with libpng and non broken but somehow not conformant png images.
There are alternatives to using libpng and libjpeg: * LodePNG: http://members.gamedev.net/lode/projects/LodePNG/ * STB_Image http://nothings.org/stb_image.c
I havent used the stb jpeg loader, but I switched to lodepng for the non-gil code.
IMO GIL.IO should not make the decision on the backends used (as io and io_new currently do), instead it should provide wrappers around different backends that provide a unified interface and let the user decide which backend to use (as io2 does)... For the same reason a decision to simply switch from LibPNG to LodePNG would be equally 'wrong'...as there are many factors that influence the choice of the backend that a particular user might want (or need to use)...e.g. a GUI library might force/prefer the use of a particular backend... But the whole xjmp issue is a bit overblown IMO as there are two solutions that are easily implementable, one is the 'always-safe-albeit-less-efficient-one', to wrap all LibXXX calls that can call longjmp and convert the call in setjmp to a C++ exception and the other is to simply throw from the error callbacks for platforms/compilers/build setups that can throw through the LibXXX code... Furthermore, if no unwindable objects are used during the decoding of an image (as is the case with io2 when the target is a raw/non-virtual GIL image) the solution is even simpler (no need for constant setjmp calls in wrappers)... -- "What Huxley teaches is that in the age of advanced technology, spiritual devastation is more likely to come from an enemy with a smiling face than from one whose countenance exudes suspicion and hate." Neil Postman
Andreas Pokorny wrote:
2010/12/7 Domagoj Saric <dsaritz@gmail.com>:
IMO the easiest way to handle the setjmp/longjmp issues for those that have to use the LibJPEG and/or LibPNG backends would be to simply skip the xjmp functions altogether and throw proper C++ exceptions from the error handlers...This might require (re)building the backend lib (and possibly linking statically) with C++ exceptions enabled on some platforms but this might prove to be easier in the end/long run...
I think we should get rid of those backends entirely. Phil stated interesting use cases in a different thread, that might not be accomplished with these libraries.
No, the case that you're referring to works fine with libjpeg, libpng, and libtiff. What it doesn't work with is the way that Christian's code has integrated them with GIL.
There are alternatives to using libpng and libjpeg: * LodePNG: http://members.gamedev.net/lode/projects/LodePNG/ * STB_Image http://nothings.org/stb_image.c
I havent used the stb jpeg loader, but I switched to lodepng for the non-gil code.
A scheme more like what Domagoj and I have been proposing would allow one to add support for additional backends quite easily. Regards, Phil.
=================== Getting the library ===================
The latest version of both extensions can be downloaded as Zip package:
http://gil-contributions.googlecode.com/files/boost_review.zip
or directly from the Subversion repository:
http://code.google.com/p/gil-contributions/source/browse/trunk/gil_2/
and the docs can built as usual with bjam and quickbook tools from within libs/gil/io_new/doc directory
It would be great if pre-built docs were online somewhere - have I missed them? Phil.
Hi Phil, On Sat, Dec 4, 2010 at 6:58 PM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
=================== Getting the library ===================
The latest version of both extensions can be downloaded as Zip package:
http://gil-contributions.googlecode.com/files/boost_review.zip
or directly from the Subversion repository:
http://code.google.com/p/gil-contributions/source/browse/trunk/gil_2/
and the docs can built as usual with bjam and quickbook tools from within libs/gil/io_new/doc directory
It would be great if pre-built docs were online somewhere - have I missed them?
No you didn't miss them. I have never build any documentation with boost build. Sorry. Christian
No you didn't miss them. I have never build any documentation with boost build. Sorry.
Christian
It would really help to have the documentation, though, and I guess that most potential reviewers have never built any documentation with boost build. Since the documentation is part of the review, I would even say that the availability of compiled documentation should be a requirement for review. And I assume that the task of building the documentation is very small compared to the task of writing it :-) Just my 2c. Regards, Roland
Hi Roland,
It would really help to have the documentation, though, and I guess that most potential reviewers have never built any documentation with boost build. Since the documentation is part of the review, I would even say that the availability of compiled documentation should be a requirement for review.
And I assume that the task of building the documentation is very small compared to the task of writing it :-)
Just my 2c.
Valid 2c. I'll look into it. Christian
Dear All, We have just past the halfway point for the formal review of Boost.GIL.IO and Boost.GIL.Toolbox. It has been quiet so far and we haven't received any votes. We still have plenty of time and I'd like to strongly encourage the Boost Community to participate in the review actively. Every comment is important to the authors and will be appreciated. Every vote counts. Thanks, Mat On 30/11/10 18:01, Mateusz Loskot wrote:
Dear All,
According to the Boost Formal Review Schedule [1], review of Christian Henning's extensions to the Boost Generic Image Library (Boost.GIL, [2]), it is: * Boost.GIL.IO * Boost.GIL.Toolbox starts on December 1st and lasts until December 10th, 2010.
[1] http://www.boost.org/community/review_schedule.html [2] http://www.boost.org/doc/libs/release/libs/gil/doc/index.html
=========== What is it? ===========
The Boost.GIL.IO extension provides an easy to use interface for reading and writing various image formats. It also includes a framework for adding new formats. The Boost.GIL.IO is indent to replace the current IO extension which is part of Boost for several years now.
Features:
* A unified way to read and write image encoded in BMP, JPEG, PNG, PNM, and TIFF formats. The capabilities to read and write in various formats have improved dramatically.
* Image data can be provided via standard file or string streams
The Boost.GIL.Toolbox provides new color spaces and other small code to ease programming with Boost.GIL.
Features:
* Implementation of color spaces: Gray_Alpha, HSL, HSV, LAB, and XYZ * Utilities to support dynamic image workflows and color conversions. * Collection of metafunctions to determine alignment, similarity and homogeneity at pixel level.
The Boost.GIL as well as the proposed extensions are provided in form of a headers-only library Although, some image formats come with their format dependency, it is corresponding third-party libraries:
JPEG - libjpeg PNG - libpng TIFF - libtiff
=================== Getting the library ===================
The latest version of both extensions can be downloaded as Zip package:
http://gil-contributions.googlecode.com/files/boost_review.zip
or directly from the Subversion repository:
http://code.google.com/p/gil-contributions/source/browse/trunk/gil_2/
and the docs can built as usual with bjam and quickbook tools from within libs/gil/io_new/doc directory
The libs/gil/io_new/test/readme.txt provides a step by step guide to configuring, building and running the unit tests.
The boost_review.zip is about 20MB due to its extensive collection of test images. They are part of the test suite to make sure that different variations of each image format is read and written correctly.
Please, be aware no guarantee can be given that all formats in their all variants are completely supported.
================ Writing a review ================
If you feel the new IO and Toolbox are interesting extensions to the Boost.GIL library, then please submit your review to the developer list (preferably), or to the review manager (mateusz at loskot dot _n_e_t_)
Here are some questions you might want to answer in your review:
- What is your evaluation of the design? - What is your evaluation of the implementation? - What is your evaluation of the documentation? - What is your evaluation of the potential usefulness of the extensions? - Did you try to use the extensions? With what compiler? Did you have any problems? - How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study? - Are you knowledgeable about the problem domain?
And finally, every review should answer this question:
- Do you think the extensions should be accepted as a part of Boost.GIL library?
Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
Mateusz Review Manager for the proposed Boost.GIL.IO and Boost.GIL.Toolbox extensions
-- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org Member of ACCU, http://accu.org
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
Hi Zach, thanks a lot for review.
It is generally very good, modulo a few bugs (see below).
What means "modulo a few bugs"?
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.
This would assume that both png reader and writer have some bugs in common. I consider that less probable. This test is not perfect, I agree, but I cannot just do a byte compare between the original and copy since the header information might different. If both file have the exact pixel values, that's a good indicator that writing and reading does something right. But I agree with you that testing image writing and reading can be problematic when the same library is used. Maybe I should use ImageMagick as the reference reader and my own lib as the writer in this case.
- 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.
I'm working on it. It turns out that building quickbook is too complicated for me. There are several threads on the boost-user and boost-build mailing lists to overcome my troubles. In case my proposal is accepted I'll make sure I can build the documentation before I add anything to boost. Sorry for inconvenience.
- 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.
I just did a quick test with space_elevator.png you mention below. May it be that you forgot to set the BOOST_GIL_IO_ENABLE_GRAY_ALPHA symbol? The following code works just fine: #define BOOST_GIL_IO_ENABLE_GRAY_ALPHA 1 #include <boost/gil/gil_all.hpp> #include <boost/gil/extension/io_new/png_read.hpp> using namespace boost; using namespace gil; int main() { gray_alpha8_image_t img; read_image( "C:\\chhenning\\zach\\art\\icons\\tech\\space_elevator.png" , img , png_tag() ); return 0; } Regards, Christian
Christian Henning wrote:
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.
This would assume that both png reader and writer have some bugs in common.
No it doesn't. Imagine a bug in the reader that caused the data to be all 0. Phil.
"Phil Endecott" <spam_from_boost_dev@chezphil.org> wrote in message news:1291925453226@dmwebmail.dmwebmail.chezphil.org...
Christian Henning wrote:
This would assume that both png reader and writer have some bugs in common.
No it doesn't. Imagine a bug in the reader that caused the data to be all 0.
To follow Lubomir in giving Christian more support, the 'clear-cut' "no it doesn't" seems (even to me :) a bit too harsh as Christian did admit that the test is not perfect and, statistically speaking (as he spoke of 'good indication'), he was not so far from truth (i.e. what is the probability of the reader reading all zeros and what of the reader and writer having a common bug...)... Knowing how much time I spent on io2 without writing a single test I can only imagine the time spent by Christian on writing all the other extensions and the tests for them. As it is becoming more and more apparent that even the amount of tests Christian did write is not enough to cover all the use cases that keep coming up I must again say that the most effective solution, at least when tests are concerned, would be for the reviewers to also join in constructively for example by writing (pseudo-code) tests that exercise their use cases... -- "What Huxley teaches is that in the age of advanced technology, spiritual devastation is more likely to come from an enemy with a smiling face than from one whose countenance exudes suspicion and hate." Neil Postman
Domagoj Saric wrote:
"Phil Endecott" <spam_from_boost_dev@chezphil.org> wrote in message news:1291925453226@dmwebmail.dmwebmail.chezphil.org...
Christian Henning wrote:
This would assume that both png reader and writer have some bugs in common.
No it doesn't. Imagine a bug in the reader that caused the data to be all 0.
To follow Lubomir in giving Christian more support, the 'clear-cut' "no it doesn't" seems (even to me :) a bit too harsh as Christian did admit that the test is not perfect and, statistically speaking (as he spoke of 'good indication'), he was not so far from truth (i.e. what is the probability of the reader reading all zeros and what of the reader and writer having a common bug...)...
My "imagine..." line was intended to represent the whole class of faults that would not be detected by this testing strategy. For example, the last bug I had in my PNG loader was that it lost the right-most pixel of odd-width images when loading into a 16-bit destination. This would not be detected by this read-write-read-compare approach. Regards, Phil.
On 09/12/10 18:33, Zach Laine wrote:
On Tue, Nov 30, 2010 at 12:01 PM, Mateusz Loskot wrote:
- 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.
Zach, I confess it is my mistake. I'm sorry for inconvenience. I've probably assumed everyone is able to run bjam in the doc folder to get the HTML output as I usually do. It was assumption running too far. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org Member of ACCU, http://accu.org
Hi all,
Zach,
I confess it is my mistake. I'm sorry for inconvenience. I've probably assumed everyone is able to run bjam in the doc folder to get the HTML output as I usually do. It was assumption running too far.
It's my mistake. I apologize. Nathan is able to build and will send me a zip file. I'll post the documentation as soon as I get it. Christian
participants (9)
-
Andreas Pokorny -
Christian Henning -
Domagoj Saric -
Domagoj Saric -
Fabio Fracassi -
Mateusz Loskot -
Phil Endecott -
Roland Bock -
Zach Laine