Re: [boost] [GIL] new IO extension review

Subject: Re: [boost] [GIL] new IO extension review
From: Christian Henning (chhenning_at_[hidden])
Date: 2010-12-07 16:05:55

Hi Domagoj,

thanks for your lengthy review and your valuable insight.

>
> For the sake of brevity (and tight time management :) I will not repeat all
> the details here, rather try to reiterate through them briefly (omitting
> tests, examples and/or detailed argumentation) while perhaps adding some new
> thoughts...
> Mateusz, can you please confirm whether this approach (to assume that the
> "[gil] New IO release" notes and conclusions are part of the final review
> discussion) is OK or should I somehow repack that discussion into the new
> one..?

I think it's good enough. You made you point very clear. ;-)

We should definitely "sit down" after the review and see what should
be the next step. I hope you're committed to walk the next few miles.

> ------ What is your evaluation of the design? ------
>
> While the design can be considered an improvement from GIL.IO I nonetheless
> consider it lacking. As the new interface already constitutes a breaking
> change, this opportunity could have been used to provide a much more
> powerful and flexible interface (while still maintaining ease of use).
> A brief reiteration from "[gil] New IO release", the free function interface
> is inflexible/weak because it:
> - makes it difficult or practically impossible to reuse underlying backend
> instances across calls which, for example, has serious performance
> implications for ROI based access or when you want to read the image
> info/metadata before reading the image...
> - (as is done now) couples the choice of the backend with the choice of the
> image format (i.e. it makes it much more difficult to use/add alternate
> backend implementations for various image formats, for example if one
> wants/has to use LodePNG or GDI+ or ... as a backend)
> - does not allow access to the underlying backend which is necessary both
> for faster/easier solving of real world problems (either later found
> deficiencies in the interface or more complex/unconventional/corner use
> cases) and a more future-proof design (that can be more easily extended
> without breaking changes and/or introduction of additional overheads)

This review has shown me what some real world use cases would look
like. I agree there are some short comings. I think in the end I was
more committed into getting more image formats read, like bit_aligned
images which I find a very important use case.

But your point is and has been well taken.

>
>
> ------ What is your evaluation of the implementation? ------
>
> Considering that the GIL.IO implementation already has an inefficient
> implementation (which was even hinted by the "this will be simplified"
> comments in the code) I would hope io_new to provide a step forward in this
> regard...unfortunately it turns out it is a big step backward, both in terms
> of code bloat and code speed (as demonstrated by the tests and in-depth
> analysis in the "[gil] New IO release" thread)...
> In fact, the mentioned test practically demonstrates a 'theoretical' effect:
> on an Intel i5 CPU, with a relatively large cache, io and io2 benefit from
> being compiled/optimized for speed (as opposed for size) while io_new
> actually executes slower when compiled for speed (probably because the
> inherent code size increase/excessive inlining which overflows even a large
> CPU cache)...this effect was not demonstrated when the same test was run a
> much weaker Via C7-M CPU (with a much smaller cache)...

Mhmm, not sure what to take from this comment. When do you consider
inlining appropriate and when not. As far as I know the Spirit lib
practically lives from inlining. Are you saying they overdo it, as
well.

I don't where to draw the line and rather have the compiler do its work.

>
> In addition to efficiency concerns, the io_new implementation (or 'internal
> design') is also inflexible in certain areas where it causes
> duplication/repetitive code between different backend wrappers (thus not
> making it easier to extend with new backends).
>

I try to reuse code that can be shared between backends. I don't
understand your point. Can you point out what is inflexible?

> ------ What is your evaluation of the potential usefulness of the
> extensions? ------
>
> A makeover for GIL.IO is long over due and the general ideas that Christian
> put in io_new are mostly very welcome even though I have serious objections
> as to how those ideas were actually put in practice...

I think this is the first positive comment. ;-)

> Yes, for example the broken TIFF reading-with-conversion, Christian did fix
> it later but the fix seems more like a patch as it demonstrates an inherent
> inflexibility (mentioned at the end of the implementation section) in the
> io_new design that requires the backend-wrapper maintainer to provide the
> (possibly very long) conversion switch cases for each backend...
>

Back to negativity. Yes there was a bug back in September. I fixed it,
not much more I can do about that.

I hope you realize that providing a IO extension is more of a can of
worms. Apart from all the different use cases, each image type comes
with a lot of baggage and fine tuning potentials. No to mention all
the supported formats. TIFF is the best and worst of both worlds. ;-)
There are over 400 tests in my proposal and it's still not enough.

>
> ------ Do you think the extensions should be accepted as a part of Boost.GIL
> library? ------
>
> (To make it clear) The way they are now - no.
>
> However, as mentioned earlier, a makeover of GIL.IO is undoubtedly needed
> and the work Christian has done so far is truly massive. For example, the
> properties system could prove to be quite useful (as a C++ abstraction for
> the various options and metadata specific to each image format) if done
> right (if not coupled to a particular backend, so that, for example, the PNG
> properties can equally be used, if supported, with LibPNG, LodePNG, GDI+,
> WIC ....)...

I'm not sure if sharing properties across backends makes sense. They
all provide their own set. Even if they provide some matching
properties ( like image height ) these properties might have different
data types.

> Actually, IMO some of the extensions (like new GIL image/pixel formats)
> should maybe enter GIL itself, not merely as extensions (this would possibly
> even make it easier to implement the more complex IO backends/formats, like
> TIFF, that support those types of images)...

I actually think GIL should move specific pixel types into a separate
extension. I understand GIL as some sort of meta-library without the
knowledge of rgb or cymk.

Thanks again for your long review. I think I lost some nerves along
the way but I very much appreciate your efforts.

Christian