Half-baked GIL IO extension review
- How much effort did you put into your evaluation? A quick glance. I am interested in perhaps using the GIL in future and so keep half an eye on it, but I haven't yet had time to really sink my teeth in to it. I currently maintain a couple of C++ libraries for PNG and JPEG I/O [1][2] and wouldn't mind being released of the burden ;) So I'm really looking at the code to see whether it solves *my* problems. Hope that's ok. - What is your evaluation of the design? I think it's fine on the whole. But I'm left with some questions, some arising from real-world uses: 1. is there explicit support for reading images from files with Unicode file names? Or do I have to prepare a stream myself? I ask because there seem to be a lot of overloads for reading from files whose paths are specified by char*s and std::strings. IMO, in anything other than a toy application, these would be useless for writing portable code. And for toy code I'd probably use the PIL[3] (Python) instead. So for *me*, these functions are redundant. I'm aware of this redundancy because it exists in my code too :) 2. is it wise to drag the stuff from e.g. the IJG JPEG library's public headers in to client scope? I ask because I've had clashes with the "boolean" macro before and this would outright prevent me from using the library. The implication of addressing this would probably mean that the GIL would no longer be header-only. That's fine by me, but perhaps it's a design goal of the GIL for some reason? 3. Is there support for things like performing lossless JPEG rotations? Not a big deal to me right now, but potentially I would like to use such a facility in future. 4. Is there support for obtaining the real-world dimensions of a loaded raster in millimetres, say? I work on CAD/graphics applications which need this functionality in order to generate reasonable texture coordinates. - What is your evaluation of the implementation? Looking again at the JPEG code here. As far as I can tell setjmp/longjmp have been used with appropriate care, but I worry what will happen if a method on a Device throws an exception, for example. I've had both Visual C++ 2008 and Apple's g++ 4.0.1 crash on me in this kind of situation. In my code, I attempt to store an exception by doing something akin to boost::current_exception(), then longjmp out to a safe point before re-throwing it. The IJG JPEG library can be compiled as C++ code and there's some new stuff in Version 7 to control whether or not extern "C" blocks are used (look for DONT_USE_EXTERN_C in the IJG source). It would be desirable in my opinion to allow the user of the library the ability to use a build of libjpeg that was compiled as C++. If compiled in such a manner, with a little care it is possible to entirely replace setjmp/longjmp with exceptions. The same is true of libpng and possibly of libtiff. - What is your evaluation of the documentation? Couldn't find a lot. Either my eyes are poor, or the documentation is :) - What is your evaluation of the potential usefulness of the extensions? Potentially very useful. - Are you knowledgeable about the problem domain? I know little about the inner workings of the file formats and compression schemes involved, but have written my own C++ wrappers for libpng and the IJG JPEG library ('libjpeg'). - Do you think the extensions should be accepted as a part of Boost.GIL library? I personally couldn't use it until some of my concerns/requests were addressed. But I certainly think it could be of value to others as-is. However, I would say that there has to be more documentation, assuming I haven't simply missed it... For this reason, my answer at the moment is regrettably 'no'. I hope this was useful and my lack of general GIL knowledge doesn't invalidate too much of my evaluation. Kind regards, Edd [1] http://bitbucket.org/edd/pngxx [2] http://bitbucket.org/edd/jpegxx [3] http://www.pythonware.com/products/pil
On Mon, Dec 6, 2010 at 3:10 PM, Edd Dawson <lists@mr-edd.co.uk> wrote:
Did you use -fexceptions with GCC, and if I'm not mistaken MSVC also has an option to assume that C functions don't throw exceptions, which shouldn't be used for exceptions to work.
Probably boost::copy_exception would be a more appropriate call in this case. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
Hi Emil, On 12/6/2010 11:45 PM, Emil Dotchevski wrote:
I suspect that -fexceptions was not in play but it's conceivable that our build system adds it automatically when compiling C. I'll double check when I'm at work tomorrow.
Well, this was inside a catch(...) as I have to handle arbitrary exceptions as best as possible. But perhaps the GIL imposes a requirement on the set of exceptions that may be thrown by a device? Again, I don't know a whole lot about the wider library yet. Kind regards, Edd
On Mon, Dec 6, 2010 at 4:26 PM, Edd Dawson <lists@mr-edd.co.uk> wrote:
If I'm not mistaken, the default for GCC is to assume C functions can't emit exceptions, -fexceptions tells it that they could. A few C libs seem to be compatible with this setup, for example with libpng I register an error callback function which throws, and it works fine but only if -fexceptions is used. Emil Dotchevski Reverge Studios, Inc. http://www.revergestudios.com/reblog/index.php?n=ReCode
On 12/7/2010 12:47 AM, Emil Dotchevski wrote:
Ah I see. Well, my libraries are configurable at build time to allow that. In the JPEG case for example, defining either JPEGXX_C_LIB_COMPILED_AS_CPLUSPLUS or JPEGXX_CAN_THROW_EXCEPTIONS_THROUGH_C_LIB will allow exceptions to propagate undisturbed. Would it be appropriate to do something similar in the GIL I/O extension? FWIW, I'd like to see it. Kind regards, Edd
"Emil Dotchevski" <emildotchevski@gmail.com> wrote in message news:AANLkTimer-e0pFMq977OQWGwCanBZQWGm-8FK8uKBNvr@mail.gmail.com...
Exceptions work just fine without /EHs (at least with MSVC, which also defaults to extern C=nothrow)...or did you mean in this particular case (throwing through a C lib)? Either way, compiling the entire binary with "extern C exceptions" is an overkill...it is enough to wrap the affected functions in wrappers properly decorated with "throw (...)"...and in addition, depending on how C++ exceptions are implemented on a particular platform, it may be needed to recompile the C lib with C++ exception support enabled (no necessary with MSVC for example)... -- "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
On 06/12/10 23:10, Edd Dawson wrote:
Edd, I'm curious about this one. How do you expect such physical dimension to be calculated? Do you expect a metadata with physical resolution provided e.g. written as TIFF tag? Personally, I'm looking for Boost.GIL in geospatial applications, but there an extension would need to be provided to manage dedicated metadata where coordinate system/reference frame/resolution is provided, so it's possible to deduce physical dimensions.
I hope this was useful and my lack of general GIL knowledge doesn't invalidate too much of my evaluation.
I'm sure your voice is very helpful here and thank you for participating in the review. BTW, I know your libraries. I've been enjoying your blog for quite a while now :-) Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org Member of ACCU, http://accu.org
On 12/7/2010 12:23 AM, Mateusz Loskot wrote:
All I know is that libpng and libjpeg provide access to this information in a crude form. I don't know whether physical dimensions are mandated by the respective formats or if the libraries are implementing common extensions, but in each case, the information is there. In the case of libjpeg, the information is exposed/advertised in the jpeg_decompress_struct and jpeg_compress_struct structures. You can see how I use them here: http://bitbucket.org/edd/jpegxx/src/ea2492a1a4a6/src/read.cpp#cl-62 http://bitbucket.org/edd/jpegxx/src/ea2492a1a4a6/src/write.cpp#cl-75 In libpng, have a look in the docs for e.g. png_get_x_pixels_per_meter. I'm entirely ignorant in the case of libtiff, I'm afraid. I do know that most (all?) versions of Adobe CS do not write these dimensions, as least not by default. I think GIMP can, though.
Well I understand that you can cram all kinds of stuff in to PNGs, but I'm probably not the best person to ask about that.
Aw shucks :) Kind regards, Edd
On 12/7/2010 3:55 PM, Christian Henning wrote:
Made in GIMP: http://dl.dropbox.com/u/3182125/36dpi.png http://dl.dropbox.com/u/3182125/36dpi.jpg Kind regards, Edd
Hi Edd, thanks for taking the time.
You can use string, wstring, char*, and filesystem::basic_path ( when compiling with BOOST_GIL_IO_ADD_FS_PATH_SUPPORT ) to define a filename. But the wide character strings are then converted back std::string. Though, I have to admit that reading a unicode filename isn't supported yet. I'll make this a top priority. Basically I have to add some overloads that use wfopen. Or is there more to do?
Would a namespace do the trick?
No support yet. I think I was looking for a code example at one point but couldn't find one. This feature is already in my todo list.
Is that part of the header information? If not, I don't think this is the right library to do unit conversion. This library should provide ways to read and write all header information and ways to read and write pixels.
Do you mean when an std::fstream throws an exception or when FILE issues an error?
Mhmm, I have to admit I'm unaware of boost::current_exception. Can you point where you use it in your code?
Interesting. I'll have a look.
- What is your evaluation of the documentation?
Couldn't find a lot. Either my eyes are poor, or the documentation is :)
Did you see the .qbk file in the /doc folder?
I think most of your concerns will be taken care once the review has finished. I'll let you know.
In no ways. ;-) Thanks for taking your time. Regards, Christian
Hi Christian, On 12/7/2010 3:49 PM, Christian Henning wrote:
For me, that's probably sufficient.
In what way? (FWIW, after checking I now see "boolean" is a typedef rather than a macro, but there was a clash nonetheless).
I was thinking of when a stream (or streambuf) of whatever kind throws an exception.
I don't actually use boost::current_exception in my code (it doesn't depend on boost at all, currently), but I do something very similar. Here, for instance: http://bitbucket.org/edd/jpegxx/src/ea2492a1a4a6/src/read.cpp#cl-166 The exact type and behaviour of the ex_store_ object there depends on the preprocessor definitions that are present when the library is built. See: http://bitbucket.org/edd/jpegxx/src/ea2492a1a4a6/src/best_exception_store.hp... And for the various exception_store implementations: http://bitbucket.org/edd/imagexx/src/5c44e8ad57e7/include/imagexx/exceptions...
Gah, no. My eyes, then! I must have got lost in the deep folder hierarchy.
I think most of your concerns will be taken care once the review has finished. I'll let you know.
Thanks! Kind regards, Edd
Hi Edd,
I meant something like: namespace libjpeg { extern "C" { #include <jpeglib.h> } } // namespace libjpeg I had the impression your compiler is complaining about redefinition of "boolean".
I think the user should catch the exception. io_new is also throwing exceptions when needed.
I'll have a look. Thanks for pointing out!
;-) I should have build the documentation before the review. It's my fault. Regards, Christian
Hi Christian, On 12/7/2010 9:18 PM, Christian Henning wrote:
Would this work if libjpeg was compiled with a C++ compiler? (though perhaps that won't be supported, in the end).
They might not have a chance when an exception is thrown from a function that is called from C (one of the callbacks installed in jpeg_decompress_struct, for example). All (portable) bets are off in this situation, as far as I know. This is where the boost::current_exception() suggestion comes from. You can catch and store the exception inside the callback (as best as possible) and then rethrow it when you're safely back in C++ land.
I forgot to say in my previous reply, but this now changes my vote as to whether the library should be included to a Yes. Kind regards, Edd
participants (5)
-
Christian Henning -
Domagoj Saric -
Edd Dawson -
Emil Dotchevski -
Mateusz Loskot