Thanks Ron, QQ. Why is there no dates on any upcoming reviews? Also, am I responsible to find a review manager? Regards, Christian On Thu, May 7, 2009 at 10:11 AM, Ronald Garcia <garcia@osl.iu.edu> wrote:

Hi Christian,

I have received your request and have added this library to the review queue.

Cheers, ron

On May 4, 2009, at 9:41 AM, Christian Henning wrote:

...

Hi to all,

the new gil::io extension is ready for review. The release candidate is available from the link below containing the header files, tests, and documentation. There are jamfiles available to run the tests and to compile the documentation.

http://gil-contributions.googlecode.com/files/rc2.zip

This file is almost 9 MB due to the amount of test images I'm using for testing. If that's a problem I can certainly create a slimmer release candidate.

A lot of things have changed for this extension but also a lot of things haven't. The interface is more or less the same. But the list of features has been grown by a lot. For instance, all formats support now in-memory read and write. The tiff extension supports a lot more formats to be read and written. n-bit images are now supported for all formats that include those. etc. etc.

Right now the lib is called gil::io_new but once it's accepted it will be renamed to gil::io.

Of course, I'm open for all kinds of input, critiques, and insights.

Thanks a lot to all people who have helped me on this release!

Regards, Christian _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

_______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

On Thu, May 7, 2009 at 9:51 AM, Ronald Garcia <garcia@osl.iu.edu> wrote:

...

On Thu, May 7, 2009 at 10:11 AM, Ronald Garcia <garcia@osl.iu.edu> 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

On Thu, May 14, 2009 at 10:28 AM, Christian Henning <chhenning@gmail.com> wrote:

...

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

Yes, it was just a little brief. To use the library to do default loads, I don't need anything but what's in the tutorial. If I want to specify a gamma when loading a PNG, I immediately need to look at code, since there is no reference.

...

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

Yes, I did, but on Windows, I have no idea. Maybe a post to the docs list would help.

...

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.

Good to know.

...

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

Cygwin has the common UNIX program dos2unix. I'd download Cygwin and use that.

...

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.

For the nonsquare pixels, you need to use the values in the pHYs chunk to determine the aspect ratio, and then scale the image. This is probably best done as a post-loading step -- after you load the image into for instance a 4x32 image, scale it up to 32x32 before returning it. The background color can be found in the bKGD chunk. Simply alpha blend the loaded image onto the background color. Note that the result needs to be full-alpha. The transformation from a 4x32 image to a 32x32 image, and the transformation from transparency mask / alpha channel plus background color to a flattened, full-alpha image raise some interesting questions. Should the results in image_read_info<> be 1) just the properties directly read out of the file -- in our examples, that would be a 4x32 image, or an image with transparency / alpha; 2) just the derived properties, as they would exist after the entire loading process is complete -- a 32x32 image, or an image with no transparency; 3) or both? Further, in the case of 2, should we advertise the final image as having no alpha channel, or should it have one that is set to 100% in every pixel? I don't have good answers for these questions, which is why I skipped the implementation of these infrequently used features in the PNG loader. Zach

Hi all, Good, I'll use dos2unix then. But when do I have to use it? Every time I create a new release candidate? Or let me ask in a different way, who is introducing these different line ending? Can I tell Visual Studio not to do that? Christian

Christian Henning wrote:

Hi all,

Good, I'll use dos2unix then. But when do I have to use it? Every time I create a new release candidate? Or let me ask in a different way, who is introducing these different line ending? Can I tell Visual Studio not to do that?

Visual Studio perfectly supports all EOL characters, even mixed ones in single file. So, if you open a file with Unix EOL, it is LF, then VS will ask you if you want to convert it to CRLF. Just answer no and everything should be fine, no need to apply dos2unix before commits. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org

But what do to with non-square pixels? Gil doesn't support this

...

kind of thing yet.

For the nonsquare pixels, you need to use the values in the pHYs chunk to determine the aspect ratio, and then scale the image. This is probably best done as a post-loading step -- after you load the image into for instance a 4x32 image, scale it up to 32x32 before returning it.

I frequently have to process batches of video frames that often have non sq. pixels ; I'd be frustated to have a lossy PNG loader in GIL. I don't think that silently rescaling an image with nonsquare pixels is good practice. A nonsquare pixel is still a pixel. Imagine that you want to apply a filter to a non sq. pix. PNG file, if GIL silently changes the image size, there's no way for you to get back the original image size and pixels. Not to mention that depending on the image type you may want to use different filter kernels to do the resize. IMHO the best solution would be to simply provide a way to know the pixel aspect ratio, and let users of the lib handle it the way they want, i.e. rescale or not or whatever. Cheers,

I frequently have to process batches of video frames that often have non sq. pixels ; I'd be frustated to have a lossy PNG loader in GIL.

I don't think that silently rescaling an image with nonsquare pixels is good practice. A nonsquare pixel is still a pixel. Imagine that you want to apply a filter to a non sq. pix. PNG file, if GIL silently changes the image size, there's no way for you to get back the original image size and pixels. Not to mention that depending on the image type you may want to use different filter kernels to do the resize.

IMHO the best solution would be to simply provide a way to know the pixel aspect ratio, and let users of the lib handle it the way they want, i.e. rescale or not or whatever.

I can only agree here. Gil makes it very easy to create a new image type based on a pixel type. When reading non-sq pixels I would suggest to create a new pixel type containing the pixel's aspect ratio. Perhaps, we can even have such information in a image_view type. As for the io extension I would try to refrain from any post processing as much as possible, especially when using read_image( ... ). Regards, Christian