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
Hi Lubomir, apologies in advance for not covering all the points...short on time ;) "Lubomir Bourdev" <lbourdev@adobe.com> wrote in message news:286B66C8-D267-417C-9442-58E4A491EBFD@adobe.com...
All we can hope for is push the boundary as much as we can, and leave the rest for the next update.
As is often the case...however there are more future-proof and less future-proof designs/interfaces (judged by how much can you change/add/remove without breaking compatibility and/or performance)...Which I think is what you also stressed below:
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.
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.
I understand you had limited time to go through all the, now really numerous posts, but I think I've shown (in the pre-review discussion) that these are not minor quibbles...The free function interface has issues both in terms of performance and in terms flexibility/power of access to the backend that it gives...So far shown attempts to fix those where all shifts towards public classes/objects representing the image to be read/written (and/or further increase in the number of parameters)... Furthermore, the classes approach can also provide a oneliner interface as io2 already does (as shown in the pre-review discussion) using static member functions...it is provided for all backends in a single place by the CRTP base class...so you can for example simply write libpng_image::read( ... ) or libtiff_image::write( ... ) which will construct the local reader/writer object for you and do the rest of the required work....
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.
Actually there were two file formats (JPEG and PNG) and the speed difference was ~37.5% (the size difference was ~30%)...In fact simply looking at/stepping through the implementation of io and io_new (which I've elaborated in more detail previously) is enough to 'see' the inefficiency w/o testing but I did the test anyway... I simply cannot follow the logic of making a priori slow software (by ignoring obvious inefficiencies) and then making it fast later (unless of course for the later selling of some 'clever' corporate marketing fog which obviously is not the case here)... There would be no need for serious performance improvements if it weren't for the, unfortunately so prevalent, 'habit'/'school of coding' that simply by default uses STL containers/constructs for just about anything without even blinking an eye... And the injudicious use of STL containers coupled with the inherent template bloat are the major reasons why io_new (and the current io) are slow (not counting the issues inherent in the interface/design)... In this context I actually do not blame Christian almost at all for this but the 'climate' that fosters this "Way"... It is no wonder that 'we live in the age' of several-megabyte-single-dialog software if 'every' library author thinks it OK to add a few tens or even a couple of hundreds of unnecessary kilobytes of code because it supposedly will not be noticeable because of disk access or some similar 'by-the-book' excuse (that is rarely actually tested)... ps. Ideally, in most cases there should be no 'inner-loop' in GIL.IO (other than those within the backend library itself)...
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.
There are assumptions in this which you must/should test before you start believing in them...I have already done numerous tests and analysis (not only for GIL.IO) that prove those assumptions wrong... That iostreams are evil is a simple _fact_ demonstrated countless times by numerous people...And it seems there is no longer any point in arguing this fact as it is simply ignored by people that seem to have some sort of a priori trust in the STL (and you can't rationally argue with 'blind faith') as if it is some 'holy artefact' of the C++ language when in fact it is sometimes its stumbling block... When I discuss/consider 'efficiency' I always consider both code speed and size (as this also affects the speed even if sometimes not of the program itself when tested/considered in isolation but of the whole system affected by N such 'bloating decisions'...i.e. link the result of sizeof( Windows Vista installation folder ) to its performance...)...In this light it is IMO wrong for any library author to make any bloat-inducing decisions based on assumptions of the type 'oh the OS call I make will always be slower than anything I can possibly do'...By that rationale Boost.ASIO could then allocate random std::strings before making a network call 'that will surely be slower than anything done in a local process'... With that said, I have no problem with providing an iostream interface (as an additional option, that has no influence on other code) only with providing _only_ an iostream interface (which AFAICT is currently the case for in memory images...luckily not for all of io_new)...
...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
Of course, there are things I don't like in it :) As you said it is not under review ;)
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.
Of course, I agree fully (except for the last part, that io_new has a fundamentally right design)... My vote 'no' was not a 'rejection' vote: as I've stated numerous times I think Christian has done a truly massive piece of work...but...a massive piece of work can also require a massive review process (as this is turning out to be)...of which it can be expected to give a few 'unsatisfactory' marks in the process before reaching a successful end...much like a mentor might not simply reject a student's thesis but return it for a work up... The 'massiveness' of the thing in our hands is further pointed out by the fact that we have so far mostly or even exclusively seen reviews of only the IO extension which comprises only a part of Christian's work (which I suspect might also be 'frustrating' for Christian...as even a negative review is often better than no review at all)...
- 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.
For example adding another set of tags could further complicate things/implementation (even maybe slowing compilation) with another required tag->implementation mapping...Directly using the backend wrapper (e.g. libjpeg_image::... ) seems IMHO simpler with no adverse effects...
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.
Actually this is what I tried to do at the very beginning when I started to work on io2 with the GDI+ backend (the gp_view classes at the end of gp_image.hpp, it is relatively easy to do this with GDI+ and WIC) but I stopped after an initial prototype version as I of course had to catch up and concentrate on a zilion other features either already present in io_new or simply of a higher priority... ps. There is also one important aspect (that has AFAICT only been briefly mentioned so far) and that is of a header only library...having dealt with a good number of different backends (and their various 'quirks') and being able to extract/produce a fair amount of non-template code I'd also vote for a change to a non-header-only library for various reasons... -- "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 Domagoj,
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.
I understand you had limited time to go through all the, now really numerous posts, but I think I've shown (in the pre-review discussion) that these are not minor quibbles...The free function interface has issues both in terms of performance and in terms flexibility/power of access to the backend that it gives...So far shown attempts to fix those where all shifts towards public classes/objects representing the image to be read/written (and/or further increase in the number of parameters)...
Furthermore, the classes approach can also provide a oneliner interface as io2 already does (as shown in the pre-review discussion) using static member functions...it is provided for all backends in a single place by the CRTP base class...so you can for example simply write libpng_image::read( ... ) or libtiff_image::write( ... ) which will construct the local reader/writer object for you and do the rest of the required work....
I'm still crunching on the better design that your are proposing. I did some reading up on CRTP and as I as I understand it now it's used for compile time polymorphism. Though you have libpng_image::read( ... ) and libtiff_image::write( ... ) but would you have a lodepng::read( ... ) as well or is that png backend somehow plugged into the reader class differently?
It is no wonder that 'we live in the age' of several-megabyte-single-dialog software if 'every' library author thinks it OK to add a few tens or even a couple of hundreds of unnecessary kilobytes of code because it supposedly will not be noticeable because of disk access or some similar 'by-the-book' excuse (that is rarely actually tested)...
One of the good things we have with io_new is that the header files let you define what to include quite well. Only reading jpeg then just include jpeg_read.hpp. No writing part of jpeg or any other backend is included.
ps. Ideally, in most cases there should be no 'inner-loop' in GIL.IO (other than those within the backend library itself)...
??? How do you read progressive png then? Meaning you have to read an image a couple of times to actual get the final result. I rather relieve the user from doing that.
That iostreams are evil is a simple _fact_ demonstrated countless times by numerous people...And it seems there is no longer any point in arguing this fact as it is simply ignored by people that seem to have some sort of a priori trust in the STL (and you can't rationally argue with 'blind faith') as if it is some 'holy artefact' of the C++ language when in fact it is sometimes its stumbling block...
Are all iostreams bad? Or do you only speak for STL's ones? To me the std::streams only represent an interface. It has been demonstrated in some other email thread that you can use boost::iostreams with io_new. In case you don't like all streams what would be a better way of supporting in-memory images. Please remember a common interface is needed for flexibility.
The 'massiveness' of the thing in our hands is further pointed out by the fact that we have so far mostly or even exclusively seen reviews of only the IO extension which comprises only a part of Christian's work (which I suspect might also be 'frustrating' for Christian...as even a negative review is often better than no review at all)...
If you speak of the Toolbox than I don't really care much. Introducing the Toolbox is just a way for me and hopefully others to add more and more small tools over time. Right now it only consists of a couple of color spaces and some metafunctions that io_new needs.
For example adding another set of tags could further complicate things/implementation (even maybe slowing compilation) with another required tag->implementation mapping...Directly using the backend wrapper (e.g. libjpeg_image::... ) seems IMHO simpler with no adverse effects...
Each tag comes with a set of headers, xxx_read.hpp, xxx_write.hpp, xxx_all.hpp, and xxx_io_old.hpp. If you don't include such headers no compile time slowing down should occur. I understand that having an xxx_image class to wrap a backend makes sense to me. At least it gives you the access to the backend when needed.
ps. There is also one important aspect (that has AFAICT only been briefly mentioned so far) and that is of a header only library...having dealt with a good number of different backends (and their various 'quirks') and being able to extract/produce a fair amount of non-template code I'd also vote for a change to a non-header-only library for various reasons...
Mhmm, we are just wrapping some backend. ASIO is header only too, correct? I think I read somewhere you're becoming a father soon. Congratulations! Regards, Christian
"Christian Henning" <chhenning@gmail.com> wrote in message news:AANLkTimUnOcdfEZfUsyHQ8oGxCE5r-JTgcnaYHLp9a4L@mail.gmail.com... Hi, Christian...allow me to only touch/answer a few points until I get more time...
Though you have libpng_image::read( ... ) and libtiff_image::write( ... ) but would you have a lodepng::read( ... ) as well or is that png backend somehow plugged into the reader class differently?
Yes(1st question)/no(2nd question), as said, this is (automatically) provided for all backends in a single place (by the base class)...nothing special to it actually...probably similar to your read_image() function...step into it and it will be clear...
One of the good things we have with io_new is that the header files let you define what to include quite well. Only reading jpeg then just include jpeg_read.hpp. No writing part of jpeg or any other backend is included.
This (source code inclusion) is not what I was referring to (I'd expect any half decent compiler to handle this) but to using things like STL containers where you do not actually need them under the excuse that the overhead will be shadowed by something else in the code path (e.g. backend or OS call) that cannot be avoided anyway...
ps. Ideally, in most cases there should be no 'inner-loop' in GIL.IO (other than those within the backend library itself)...
??? How do you read progressive png then? Meaning you have to read an image a couple of times to actual get the final result. I rather relieve the user from doing that.
That's why I said 'most' cases...and I didn't 'count'/consider the loop that reads scanlines/tiles, that is necessary with some backends, as an inner but an outer loop...
In case you don't like all streams what would be a better way of supporting in-memory images. Please remember a common interface is needed for flexibility.
I already recommended and explained the approach with boost::iterator_range<> (the way io2 does it) ... which is probably also what Phil and Roland proposed... e.g.: using namespace boost; using namespace boost::gil; unsigned char const my_in_memory_jpeg[] = {....}; typedef libjpeg_image::reader_for<memory_chunk_t>::type in_memory_libjpeg_reader_t; in_memory_libjpeg_reader_t my_reader( my_in_memory_jpeg ); my_reader.copy_to( ... );
I think I read somewhere you're becoming a father soon. Congratulations!
Thanks ;) -- "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:
"Lubomir Bourdev" <lbourdev@adobe.com> wrote in message news:286B66C8-D267-417C-9442-58E4A491EBFD@adobe.com...
The free function interface has issues both in terms of performance and in terms flexibility/power of access to the backend that it gives...So far shown attempts to fix those where all shifts towards public classes/objects representing the image to be read/written (and/or further increase in the number of parameters)...
Furthermore, the classes approach can also provide a oneliner interface as io2 already does (as shown in the pre-review discussion) using static member functions...it is provided for all backends in a single place by the CRTP base class...so you can for example simply write libpng_image::read( ... ) or libtiff_image::write( ... ) which will construct the local reader/writer object for you and do the rest of the required work....
Domagoj, Have you considered using classes plus free functions as follows, rather than your CRTP base class approach?: class jpeg_image_reader { // models image_reader concept public: .... void read_row(pixel_t* data); // or whatever }; template <typename T> // t models image_reader concept void read_image(T& image_reader, gil::image dest) { // call T::read_row() in a loop etc. } I'm curious to know what you get from CRTP that's not possible with this sort of non-inheritance approach. Regards, Phil.
"Phil Endecott" <spam_from_boost_dev@chezphil.org> wrote in message news:1292111408233@dmwebmail.dmwebmail.chezphil.org...
I'm curious to know what you get from CRTP that's not possible with this sort of non-inheritance approach.
For example default implementations for some utility member functions and meta-functions... But, this is probably the age old dilemma where to draw the boundary between free and member functions...to which I am open but just don't have the time for it right now... -- "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 Domagoj, On Dec 11, 2010, at 7:18 AM, Domagoj Saric wrote:
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.
I understand you had limited time to go through all the, now really numerous posts, but I think I've shown (in the pre-review discussion) that these are not minor quibbles...The free function interface has issues both in terms of performance and in terms flexibility/power of access to the backend that it gives...So far shown attempts to fix those where all shifts towards public classes/objects representing the image to be read/written (and/or further increase in the number of parameters)...
But free functions are just like methods except they have an extra hidden parameter pointing to the class instance. I think the discussion here is not free functions vs classes but whether or not to maintain a state. Please note that both you and Christian use classes to maintain the state; Christian simply wraps the classes into free functions for simple things like read and write an entire image. But to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly. So not much design changes are necessary. Adding new functionality may be needed but less, if any, changes of the existing functionality.
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.
Actually there were two file formats (JPEG and PNG) and the speed difference was ~37.5% (the size difference was ~30%)...In fact simply looking at/stepping through the implementation of io and io_new (which I've elaborated in more detail previously) is enough to 'see' the inefficiency w/o testing but I did the test anyway...
First of all, I don't believe your tests represent real use scenarios. If you loop over the same image, the image gets into the OS cache and therefore the I/O access bottleneck is no longer there. It is not typically the case that the image is in the OS cache before you start reading it. A better test case is to have many images and do a single loop over them. So I cannot trust the 30% number that you report. My prediction is that if you do it for fresh images the differences will be far smaller than 30%. The second point is that if the code has never been profiled, there are usually lots of easy ways to speed it up without major changes.
I simply cannot follow the logic of making a priori slow software (by ignoring obvious inefficiencies) and then making it fast later (unless of course for the later selling of some 'clever' corporate marketing fog which obviously is not the case here)... There would be no need for serious performance improvements if it weren't for the, unfortunately so prevalent, 'habit'/'school of coding' that simply by default uses STL containers/constructs for just about anything without even blinking an eye... And the injudicious use of STL containers coupled with the inherent template bloat are the major reasons why io_new (and the current io) are slow (not counting the issues inherent in the interface/design)...
These are some strong statements here, deserving a separate thread that I am sure many Boosters will have a lot to say about. Here are my two cents: 1. Using STL, or templates in general does not always lead to template bloat. It simply makes it easier to generate bloated code, sometimes inadvertently, sometimes on purpose. 2. Template bloat does not always mean that the code will run slowly. In fact, it often means the code will run faster because certain choices are done at compile time instead of at run time. 3. STL is a very well designed library. That said, there are some STL implementations that are really very bad, so you have to be careful which one you use. In addition, it is very easy to shoot yourself in the foot with the STL.
In this context I actually do not blame Christian almost at all for this but the 'climate' that fosters this "Way"...
It is no wonder that 'we live in the age' of several-megabyte-single-dialog software if 'every' library author thinks it OK to add a few tens or even a couple of hundreds of unnecessary kilobytes of code because it supposedly will not be noticeable because of disk access or some similar 'by-the-book' excuse (that is rarely actually tested)...
ps. Ideally, in most cases there should be no 'inner-loop' in GIL.IO (other than those within the backend library itself)...
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.
There are assumptions in this which you must/should test before you start believing in them...I have already done numerous tests and analysis (not only for GIL.IO) that prove those assumptions wrong...
Again, I believe the tests you have reported must be redone by reading 1000 DIFFERENT images, not the same image.
That iostreams are evil is a simple _fact_ demonstrated countless times by numerous people...And it seems there is no longer any point in arguing this fact as it is simply ignored by people that seem to have some sort of a priori trust in the STL (and you can't rationally argue with 'blind faith') as if it is some 'holy artefact' of the C++ language when in fact it is sometimes its stumbling block...
I never said anything in support or against streams. Please note that streams are not part of the STL. Here is a handy reference for the STL: http://www.sgi.com/tech/stl
...<<<Skipped>>>... With that said, I have no problem with providing an iostream interface (as an additional option, that has no influence on other code) only with providing _only_ an iostream interface (which AFAICT is currently the case for in memory images...luckily not for all of io_new)...
And this is what Christian has done. If your input is not a stream, the code doesn't use streams; it operates straight to FILE*. But if you start with a stream, then it uses a stream. Did I understand the code correctly, Christian? If so, does this address your objection then, Domagoj? Lubomir
Hi all,
...<<<Skipped>>>... With that said, I have no problem with providing an iostream interface (as an additional option, that has no influence on other code) only with providing _only_ an iostream interface (which AFAICT is currently the case for in memory images...luckily not for all of io_new)...
And this is what Christian has done. If your input is not a stream, the code doesn't use streams; it operates straight to FILE*. But if you start with a stream, then it uses a stream. Did I understand the code correctly, Christian? If so, does this address your objection then, Domagoj?
Yes, the device is a template parameter and when no stream is used the code wouldn't use istream_device< format > for reading. Christian
Hi Lubomir, Lubomir Bourdev wrote:
I think the discussion here is not free functions vs classes but whether or not to maintain a state. Please note that both you and Christian use classes to maintain the state; Christian simply wraps the classes into free functions for simple things like read and write an entire image. But to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly.
I've not noticed these classes and this functionality in Christian's code; could you point out which code you're referring to? Thanks, Phil.
Hi Phil, On Mon, Dec 13, 2010 at 11:02 AM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
Hi Lubomir,
Lubomir Bourdev wrote:
I think the discussion here is not free functions vs classes but whether or not to maintain a state. Please note that both you and Christian use classes to maintain the state; Christian simply wraps the classes into free functions for simple things like read and write an entire image. But to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly.
I've not noticed these classes and this functionality in Christian's code; could you point out which code you're referring to?
Each backend defines a reader< format_tag > class which is derived from reader_base. Same goes for when writing images. Does that answer your question? Christian
Christian Henning wrote:
Hi Phil,
On Mon, Dec 13, 2010 at 11:02 AM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
Hi Lubomir,
Lubomir Bourdev wrote:
I think the discussion here is not free functions vs classes but whether or not to maintain a state. Please note that both you and Christian use classes to maintain the state; Christian simply wraps the classes into free functions for simple things like read and write an entire image. But to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly.
I've not noticed these classes and this functionality in Christian's code; could you point out which code you're referring to?
Each backend defines a reader< format_tag > class which is derived from reader_base. Same goes for when writing images.
Those classes are not part of the public interface though, are they? (Sorry, I should have said "in the interface" before.) Lubomir, when you wrote "to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly", were you suggesting that the user should use these private classes, or that they could do this by extending the extension, or something else? Thanks, Phil.
On Dec 13, 2010, at 9:28 AM, Phil Endecott wrote:
Lubomir, when you wrote "to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly", were you suggesting that the user should use these private classes, or that they could do this by extending the extension, or something else?
I was pointing out that the library designer could build on top of these existing classes in order to extend the library to new functionality, such as reading an image in parts. And in such cases, since the state has to be maintained across multiple calls, it is probably a good idea to make these classes public, the way Domagoj has done it. All of this is discussing future functionality that would require adding new API. The currently proposed public API does not have to change to accommodate the future needs. Lubomir
Hi Phil,
Those classes are not part of the public interface though, are they? (Sorry, I should have said "in the interface" before.)
No, they are hidden in the detail namespace. I think I could change that to introduce state for reading and writing.
Lubomir, when you wrote "to do something more complicated, such as read the image in parts, one can simply build that functionality using the classes directly", were you suggesting that the user should use these private classes, or that they could do this by extending the extension, or something else?
Nothing is set in stone. ;-) Christian
Lubomir Bourdev wrote:
If your input is not a stream, the code doesn't use streams; it operates straight to FILE*. But if you start with a stream, then it uses a stream.
"Not a stream" does not always imply that it's a file. The case that I, and I think Domagoj, would like to see handled better is when the data is in memory. For example, libjpeg has a function jpeg_mem_src() which could be used as follows: void read_jpeg_image_from_mem(const char* begin, const char* end, gil::image dest) { ..... jpeg_mem_src(...., begin, end-begin); ..... } So there is basically no overhead. Wrapping a block of memory up in a stream, and then unwrapping it again to pass it to the image library's C API, and pulling in another Boost library like Boost.IOStreams to do so, looks like overkill. Regards, Phil.
Hi Phil,
void read_jpeg_image_from_mem(const char* begin, const char* end, gil::image dest) { ..... jpeg_mem_src(...., begin, end-begin); ..... }
So there is basically no overhead. Wrapping a block of memory up in a stream, and then unwrapping it again to pass it to the image library's C API, and pulling in another Boost library like Boost.IOStreams to do so, looks like overkill.
As far as I understand jpeg_mem_src code only initializes the jpeg_source_mgr with the in-memory data source callbacks. These callbacks are used during the reading process to fill up buffers and to skip data. io_new is having its own set of these callbacks, which admittedly are a very close resemblance to libjpeg's ones. Though, jpeg_mem_src doesn't read anything. And yes, you're right it looks a bit like overkill. But it's not a huge one, I believe. Regards, Christian
"Christian Henning" <chhenning@gmail.com> wrote in message news:AANLkTikN6HxUezBRR6HwhD+1EOJD=bnjZSgTvJO_+DM5@mail.gmail.com...
And yes, you're right it looks a bit like overkill. But it's not a huge one, I believe.
Why believe if you can test ;) Use/expand the example I gave in the previous post (with a memory_chunk_t reader) and step through the generated code (with the disassembly window open) and compare it to the Boost.IOStreams+io_new combination... -- "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
"Lubomir Bourdev" <lbourdev@adobe.com> wrote in message news:C2C30445-BD2B-496F-B079-D7966943B269@adobe.com... Let me for now try to at least clear some confusion/misunderstanding... << First of all, I don't believe your tests represent real use scenarios. If you loop over the same image, the image gets into the OS cache and therefore the I/O access bottleneck is no longer there. It is not typically the case that the image is in the OS cache before you start reading it. A better test case is to have many images and do a single loop over them. So I cannot trust the 30% number that you report. My prediction is that if you do it for fresh images the differences will be far smaller than 30%.
Yes, the test I used is purely synthetic and hardly represents any real use case. However it is good for measuring/pointing at the overall overhead/'fat' that each of the libraries induces which is important to consider for any library (in the light of the 'global picture argument' I voiced earlier)... Regardless of the above, the argument of 'shadowing by expensive IO/OS/backend calls/operations' is still fallacious as previously explained... ps. again the number was ~37%...the 30% was for the code size overhead/difference (for which it is also worthwhile to note that it included the mutual 'starting' size of the CRT plus the two backend libs...if those were excluded the difference would probably be an order of magnitude larger)... << The second point is that if the code has never been profiled, there are usually lots of easy ways to speed it up without major changes.
Of course, but - why wait for a profiler to tell me that a needless string or vector is, well, needless - it might turn out that for some 'repairs' major changes (maybe even to the interface) will be required after all... Plus, maybe not in theory but yes in practice, it is better to catch, discuss and correct these issues at review-time (even if some of them could be corrected later without breaking changes) because, in a review process, we have a critical mass of people which can create the significant incentive required to actually make the changes... Take for example these very objections that we are discussing now...little heed was given to them months ago when I first brought them up but now it is obviously a different story ;)
These are some strong statements here, deserving a separate thread that I am sure many Boosters will have a lot to say about. Here are my two cents:
1. Using STL, or templates in general does not always lead to template bloat. It simply makes it easier to generate bloated code, sometimes inadvertently, sometimes on purpose.
Neither did I claim that it _always_ leads... To clear any confusion, at least when templates are concerned, in no way do I think of templates in any 'bad' way...ask my boss...I maximally torture our compilers and make him unhappy on a daily basis with 'extra extra' long build times ;) I was speaking of a 'school of coding' that fosters _injudicious_ use of certain STL constructs (as if they were 'free') and placed that in the context of io_new (and io) which sometimes uses STL containers (and redundant copying) where it is not necessary (non-virtual targets) as well as templates where these are not necessary (again, for non-virtual targets)... considering that this redundant usage is combined (the STL containers and copying is used precisely in the mentioned template functions) and considering that the STL containers (just like any unwindable objects) create EH states (and thus hidden unwind funclets) for each of the instantiated function templates one does not need a profiler to see that this will add some 'cholesterol' to your binary... This is also one example that shows that the 'nineties'/'new-throw-virtual' style of C++ coding (out of which STL came) does not mix so well with more modern approaches (characterized more by keywords like 'template', 'meta' and 'compile-time')...
2. Template bloat does not always mean that the code will run slowly. In fact, it often means the code will run faster because certain choices are done at compile time instead of at run time.
Of course, but then this does not actually constitute bloat (as making a choice at compile time will actually remove the code path(s) not taken)...unless of course this 'choice' is irrelevant and/or small compared to the rest of the templated function body (which then gets template-replicated causing bloat)...which would be an anti-pattern or an example of injudicious use of templates...which I sometimes found to be the case in io and io_new (not using a non-template function for non-virtual targets)...
3. STL is a very well designed library. That said, there are some STL implementations that are really very bad, so you have to be careful which one you use. In addition, it is very easy to shoot yourself in the foot with the STL.
A few things might speak against giving the 'very well' mark too easily, e.g. the required improvements and refinements that it got (or even still has not got) for later (re)discovered deficiencies many/most of which were first provided by boost (e.g. array, intrusive and ptr containers, error_code...)... also: http://www.ultimatepp.org/www$uppweb$vsstd$en-us.html http://www.ultimatepp.org/www$uppweb$vsd$en-us.html << And this is what Christian has done. If your input is not a stream, the code doesn't use streams; it operates straight to FILE*. But if you start with a stream, then it uses a stream. Did I understand the code correctly, Christian? If so, does this address your objection then, Domagoj?
The 'forced streams' issue exists with in-memory images not with other sources/'devices'... -- "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" <domagoj.saric@littleendian.com> wrote in message news:ie84mm$hun$1@dough.gmane.org...
"Lubomir Bourdev" <lbourdev@adobe.com> wrote in message news:C2C30445-BD2B-496F-B079-D7966943B269@adobe.com... << First of all, I don't believe your tests represent real use scenarios. If you loop over the same image, the image gets into the OS cache and therefore the I/O access bottleneck is no longer there. It is not typically the case that the image is in the OS cache before you start reading it. A better test case is to have many images and do a single loop over them. So I cannot trust the 30% number that you report. My prediction is that if you do it for fresh images the differences will be far smaller than 30%.
Yes, the test I used is purely synthetic and hardly represents any real use case. However it is good for measuring/pointing at the overall overhead/'fat' that each of the libraries induces which is important to consider for any library (in the light of the 'global picture argument' I voiced earlier)...
Actually I take this back...i.e. I (re)claim that the test posted is fully relevant and not that far from real use cases on several grounds: - the overhead of decompression/decoding of (PNG and JPEG) images (+ the calls/access to low level OS/kernel APIs) is perfectly sufficient to test whether the injudicious use of STL containers, memory allocations and copying is in fact a 'negligible' overhead (as was claimed and proven false by the test) - decoding images without physical disk access/cached images is a frequently encountered scenario for example for anyone looking through their collection of digital photos where any decent photo viewing application will try to load-ahead images and when the delay between images is still apparent even without any significant disk activity (plus the scenario where you move backwards through already seen and thus cached images) - decoding images without any disk access/in-memory images is also a valid scenario (for example small static/binary resource images such as application icons, backgrounds...)... - and finally, again:
Regardless of the above, the argument of 'shadowing by expensive IO/OS/backend calls/operations' is still fallacious as previously explained...
-- "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 Lubomir, thanks for reviewing.
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.
Has it really been four years? ;-)
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.
I have to admit I'm a bit weak with concepts. I like the general idea but never really done anything in this regard. I hope the boost community can help out.
The documentation is adequate, but requires cleanup and fixing of lots of typos.
On my list. I just have to find a lost "native English" soul that has some time at hand. But I'll try my best myself.
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.
On my list.
- 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.
I would like to understand Domagoj design first to make a better decision.
- 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.
Me too, I like to hear what other people think. I tend more towards the iterators. It's easier to use and to test for it.
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.
We already have discussed some mutual involvement. Just waiting for the review to finish.
- Improve the language in the documentation section.
Not surprised here. ;-)
- Make sure the test images are license-free. If the license status is unknown, change them to other license-free images.
I have been very careful in selecting the images. All authors have been informed and they either given their consent or didn't bother replying.
- Do some profiling and performance optimization of the most common formats. Add some test cases to measure speed of common scenarios.
On my list.
- 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.
cool technique. Will keep it in mind.
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.
Noted. Thanks again for your time. Christian
Christian Henning wrote:
- Make sure the test images are license-free. If the license status is unknown, change them to other license-free images.
I have been very careful in selecting the images. All authors have been informed and they either given their consent or didn't bother replying.
Can you elaborate on "didn't bother replying"? Copyright defaults to "all rights reserved". Regards, Phil.
Hi Phil, On Sat, Dec 11, 2010 at 6:33 PM, Phil Endecott <spam_from_boost_dev@chezphil.org> wrote:
Christian Henning wrote:
- Make sure the test images are license-free. If the license status is unknown, change them to other license-free images.
I have been very careful in selecting the images. All authors have been informed and they either given their consent or didn't bother replying.
Can you elaborate on "didn't bother replying"? Copyright defaults to "all rights reserved".
According to my records I did contact the author of the BMP Suite but he never replied back to me. There is no indication of any license on his website. http://entropymine.com/jason/bmpsuite/ I think I'll add a readme.txt which points back to his website. Regards, Christian
participants (5)
-
Christian Henning -
Domagoj Saric -
Domagoj Saric -
Lubomir Bourdev -
Phil Endecott