[boost] GIL io_new review

Subject: [boost] GIL io_new review
From: Lubomir Bourdev (lbourdev_at_[hidden])
Date: 2010-12-10 14:56:14

First I apologize for not being active in the past several years in the Boost community due to time constraints. I only had a few hours to do this review, but I also reviewed Christian's code a couple of years ago.

When we introduced GIL to Boost I was hesitant to include the I/O extension in the Boost submission. I felt that doing the I/O right is a major project and it is better to do it right or not at all. However, without any I/O support it would be very hard to use GIL, so we had to include some I/O extension that provides reasonable support for common formats.

Christian has been (and still is) very passionate to make the next generation I/O. He started working on this at least four years ago and have been improving and polishing the code ever since. The problem with I/O is that you can never declare success. There are an unlimited number of file formats, back-ends, ways to read and write the image, image representations, compression settings, etc. For each of these one needs to deal with completeness, performance, test cases, failure modes, platform dependencies... All we can hope for is push the boundary as much as we can, and leave the rest for the next update.

What is important in the first release is to get the design right. Once we have a solid foundation we can add a lot of functionality quickly and transparently. Domagoj had some constructive criticism so I will start with that. On the issue of using classes vs free functions, I don't think there is a huge difference either way. The the new_io code borrows the style from GIL, which uses free functions as algorithms (if you call I/O "algorithms"). GIL in turn borrows this style from the STL. Using functions one can extend functionality with overloading (but there are ways to do this with classes too) and it is nicer to be able to read/write a file with one line, without having to first construct a reader/writer and then call it. But these are minor quibbles.

On the issue of performance, I don't think one test with one file format being 30% slower proves that there are serious performance issues. My guess is that  Christian has been focusing on adding functionality and never got around to work on performance improvements, which usually means that we can get a lot of speedup by doing some simple optimizations in the inner loops.

On the issue of using/not using streams, I don't have a strong opinion, except that stream support for I/O was one of the highest requested functionalities in GIL's review. Domagoj, you should have been more vocal at that time. Christian simply responds to the wishlist of the Boost community. In general, I feel that performance might be a bit overhyped in recent discussions. The image I/O operations are often dominated by disk access and by compression, etc. I don't think avoiding a copy of the pixels would make much of a difference in the majority of uses. That said, we do want to make sure GIL I/O is as fast as it can be.

On the issue of supporting different backends, I agree with Domagoj that this is important and we should make sure to support it. In Christian's design everything is done in reader and writer classes, which are currently specialized for a specific file format and backend. One could imagine creating classes for other backend+file format combinations without much (any?) changes to the current design. The reader/writer files from the same backend could have common base classes or share functionality in other ways as appropriate.

- What is your evaluation of the design?

The design is fine, but I would like the requirements for adding new file formats / backends to be made more explicit. Specifically we want a concept for a file format reader/writer/etc and adding new file formats would constitute providing models for these concepts.

- What is your evaluation of the implementation?

It looks fine but haven't played with it much.

- What is your evaluation of the documentation?

The documentation is adequate, but requires cleanup and fixing of lots of typos.

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

Very useful. However, there is some missing functinality which in my opinion is important to have:
1. The ability to read an image without specifying the file format at compile time
2. The ability to support multiple back-ends
3. The ability to efficiently read/write an image in parts

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

I did not do anything non-trivial with them.

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

About 3 hours looking at the code (but I reviewed a verion about 3 years ago)

- Are you knowledgeable about the problem domain?

I am familiar with GIL but not very much with file format libraries.

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

Let us consider our options carefully.
We could do a "reject" on the basis that there is another alternative libarary (Domagoj's library). I don't think this is a good idea. I looked briefly at his library; there are things I like about it but also things I don't. There are certainly a lot of things that are missing. I will refrain from discussing it because it is not under review but the point is that in its current state it is not superior to what Christian has. Domagoj can improve his library further, but so can Christian, and Christian has been working on this for four years now and had submitted his library for review long time ago. This is a huge effort on behalf of Christian, he produced some great and very useful work and a reject will be demoralizing after so many years of work, and is unjustified given that fundamentally he has the right design.

We could do an "accept". I would be ok with this, but there is some important functionality that is missing and adding it will require moderate work and some changes to the interface

So my recommendation is "conditional accept" after the above identified must-have functionality has been added, specifically:

- Support for reading images whose file format is unknown at compile time. That involves the ability to register file formats / backends, associate them with given extensions and then call the appropriate reader based on the extension of the filename.
- Support for different backends. It appears that this can be done simply the same way a file format can be extended, i.e. have "png_libpng" and "png_lodepng" tags. It is not immediately obvious to me why this would be a bad design choice; if someone disagrees they should elaborate.
- Support for efficient reading/writing of parts of images that are too large to fit in memory. There are two ways to proceed: one is to have an input/output iterator that enforces sequential access, and another is to allow for user-driven access at the cost of sometimes severe performance. I'd like to hear the opinion of people who would use this more advanced options.

My preference is that Christian joins forces with Domagoj to collaborate on these and take the best of their libraries to gil.io. Looks like Domagoj also spent a lot of time and effort on this problem and has gained a lot of valuable intuition. I don't want to see his efforts go to waste either.

Other todo items:

- Improve the language in the documentation section.
- Make sure the test images are license-free. If the license status is unknown, change them to other license-free images.
- Do some profiling and performance optimization of the most common formats. Add some test cases to measure speed of common scenarios.
- Make sure the test cases provide complete coverage. For example, set a breakpoint in every method, every switch statement and branch. Run your tests and remove any breakpoints that are encountered. Look at the breakpoints that were not hit and add tests for them.

Finally, here is an idea that I like, but may be too much work to do in the current release:
I would like to have a GIL virtual image view associated with a given file. That would allow reading/writing to simply be copy_pixels, and reading into a different format to be copy_and_convert_pixels. Reading a given ROI could simply be done by creating a subimage_view. The cool thing about this is one could in theory make changes to images
without even loading them in memory. The caviat is that doing this right and efficiently is very hard, requires clever caching, plus a wrong access pattern can result in horribly bad performance.

Lubomir