[boost] GIL io_new review

Subject: [boost] GIL io_new review
From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2010-12-07 14:48:03

Back in 2006 I had some code that used ImageMagick's Magick++ library 
to read and write images in various formats and do some basic 
processing.  That library works well and is very feature-full, but is 
not what you would call "Boost like", so I was excited when GIL came 
along.  I looked at it at the time of its review, and I thought at 
first that I must have missed a big chunk of the documentation where 
all the algorithms were described!  Of course, it has no image 
processing algorithms; it's just a layer; if you want to scale or 
rotate or merge images, you have to do that yourself.  I don't think I 
wrote a review in the end; my feeling was - in comparison with 
ImageMagick - "come back when you've finished" - but I didn't want to 
say that in public.

So it's great that now, 4 years later!, we are finally getting some GIL 
extensions.  Thank you Christian for - I hope! - getting the ball 
rolling.  I hope we'll see more.

Having said that, I suspect that even with lots of extensions GIL might 
not really be what I need.  Here's a typical problem that I will have 
to address in the next few weeks:  I have a 1e12 pixel image - a map - 
which is supplied as a few thousand 5000x5000 TIFF tiles; I have to 
join them together, tweak the colours, draw some lines on top, and 
re-chop it into a few million 256x256 PNG tiles.  The issue of course 
is that that image can't be held in RAM.  I will do this with my own 
wrappers around libtiff and libpng, in rows or chunks.  So, do the 
image concepts (and concrete classes) that GIL defines accommodate that 
sort of problem?  Even if they do, is a "virtual image" of some sort a 
good way of doing it?  I suspect not.  But even if the image concept is 
not useful, presumably other GIL concepts like pixel formats and 
algorithms in future extensions (pixel format conversion, blending) 
could still be useful.

So, along comes Christian's IO extension.  Wrapping the "legacy" image 
libraries is something that I have done a few times now and it can be 
painful, not least because of the need to read a lot of documentation; 
the libraries all differ and it would be great to get a unified set of 
wrappers that provide a sane C++ interface. But as you can see, I don't 
really want something that reads and writes complete GIL images, and 
that is what Christian's extension does.  Ideally, this extension would 
have separated out the wrapping of the libraries from the actual GIL 
image integration, with the GIL image integration just being a layer on 
top of the image library wrappers.

Since it doesn't do that, it's not much use to me.  It is, however, 
better than what GIL currently has.  So for that reason I suggest that 
it should be accepted.

> - What is your evaluation of the design?

In addition to my comments above, I note that it's necessary to jump 
through hoops (using Boost.IOStreams) just to read an image from 
memory.  Surely this should be a fundamental operation; it would be 
much better to have a ctor that takes begin and end pointers for the 
encoded image data.

> - What is your evaluation of the implementation?

When I first looked at the library I noticed that the error handling 
was clearly broken.  Although that seems to have been resolved, it has 
given me a poor impression of the quality (i.e. likely correctness) of 
the implementation.

It seems that this error handling has received almost no testing.  
Thorough testing will be difficult for this library because as well as 
all the usual permutations of platform features, compilers etc. the 
variations in the image libraries must also be considered.  The library 
version and compiler settings could both affect the correct operation 
of this code.

I have some concerns about the likely performance of the code due to 
excessive copying of the data both in and out of the image library.  
Although I've not measured the performance impact, I feel it would be 
very desirable (and is certainly possible) for this library to have 
zero copying.

> - What is your evaluation of the documentation?

Satisfactory.

(As a meta-comment - I think libraries being reviewed really should 
have their docs available online somewhere, and preferably also their 
source code in a source browser of some sort.  In fact, I thought this 
was already a requirement.  Can it be added to the review managers' 
checklist for future reviews?)

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

It's better than what was there before.

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

No, I've not compiled anything.

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

Probably half a day during the review, and a similar amount of time a 
few months ago.

> - Are you knowledgeable about the problem domain?

I've written a few wrappers for libjpeg, libpng and libtiff.  I've 
never used GIL though I've read extensively about it.

> And finally, every review should answer this question:
> - Do you think the extensions should be accepted as a part of Boost.GIL
> library?

On the grounds that this is better than what is currently there, and on 
the assumption that Domagoj Saric is not imminently going to post 
something else that would supersede this, I believe that it should be accepted.

(I have not looked at the "toolbox" at all, and I've not seen it 
mentioned in any other reviews yet.  My "yes" is just for the io_new functionality.)

Regards,  Phil.