Re: [boost] gil::io "non-review" (was: [gil] Can not open test.jpg)

Subject: Re: [boost] gil::io "non-review" (was: [gil] Can not open test.jpg)
From: Phil Endecott (spam_from_boost_dev_at_[hidden])
Date: 2010-03-23 12:29:02

Christian Henning wrote:
> Phil Endecott wrote:
>> In jpeg/read.hpp you have a namespace-scope static jmp_buf. ?I immediately
>> worry about thread-safety. ?Even in a single-threaded app I can't see how
>> this works if you have more than one jpeg file open. ?Then you setjmp() in
>> the constructor. ?The jmp_buf is invalid as soon as that scope returns, so
>> if libjpeg calls your error_exit() from after the ctor has finished (e.g. in
>> jpeg_read_scanlines()) it will crash. ?(Have I grossly misunderstood how
>> this works?)
>
> Let's forget multi-thread safety for a bit. During the construction of
> the my jpeg_decompress_mgr I tell libjpeg's jpeg_error_mgr to use my
> own error handler ( static member function called error_exit() ).
> After this, I save the current environment into the marker ( int[16]
> buffer ) using the setjmp() function. The marker is a static global
> inside the gil::detail namespace. Once an error occurs error_exit() is
> called which leads to a longjmp which then jumps back to the setjmp
> point.

It only jumps back to the setjmp point if the stack context in which 
setjmp was called is still valid.  setjmp is called from the 
jpeg_decompress_mgr constructor.  You make libjpeg calls (e.g. 
jpeg_read_scanlines()) after the constructor has finished, right?  This 
will cause undefined behaviour if errors occur in those calls when it 
invokes longjmp.  (Have you tested this?)

> Next we clean up and issue an exception. Hope this makes sense.

No, it doesn't make sense.  Sorry if I am missing something important.  
Maybe someone else would like to have a look at this code?  It's here:  
http://gil-contributions.googlecode.com/svn/trunk/gil_2/boost/gil/extension/io_new/formats/jpeg/read.hpp 
near the start of the file.

> In a single-treaded application how would this procedure be a problem?
> I mean how can you have more than one jpeg file open?

Errr...  you cannot have more than one jpeg file open?  I suppose not.  
I guess that illustrates the limitations of this approach.

> For multi-threaded usage the above is problematic since the marker can
> be overridden. In case the user reads several jpeg files at one time
> this can lead to undefined behavior. One solution would be to create a
> marker for each read operation. Do you thing that's a valid approach?

No; it seems to me that the whole idea of using setjmp/longjmp is 
broken.  But as I say I have only spent a few minutes looking at this 
code.  I would be very grateful if someone else could take a look at it.

Regards,  Phil.