juce_loadJPEGImageFromStream currently ignores all jpeg decoding errors silentlty.
This is very problematic in our code, which can be told to decode jpeg images on possibly very, very bad input streams (read: mp3 picture frames ).
When this happens, the jpglib routines continue execution on a very corrupted internal state (read: null pointers )
Furthermore, from what I read in the full jpglib source and usage examples, it seems like ERREXIT should not be silently ignored and c client code should use setjmp/longjmp handlers to resume execution in user code. For us C++ folks, this amounts to exception throwing and handling.
The following code is a rather simple fix that does just that.
It also contains a check that skips decoding of zero-sized images.
Also the catch clause should be pushed after the scope of the big if… since ERREXIT calls are scattered all over jpglib functions, not only the header decoding part.
One thing that seems a pity is to leak the jpegDecompStruct, by not calling jpeg_destroy_decompress if it fails… do you know if it’s safe to still call that when it has gone wrong?
It should not be leaked, indeed, but I’m pretty sure it’s not, since the libjpg code contains this:
GLOBAL(void)
jpeg_destroy_decompress (j_decompress_ptr cinfo)
{
jpeg_destroy((j_common_ptr) cinfo); /* use common routine */
}
and the fatalErrorHandler function calls jpeg_destroy.
Apparently, in libjpg, decoder and encoders are treated the same way, regarding dynamic allocations.
/* Routines that are to be used by both halves of the library are declared
* to receive a pointer to this structure. There are no actual instances of
* jpeg_common_struct, only of jpeg_compress_struct and jpeg_decompress_struct.
*/
struct jpeg_common_struct {
jpeg_common_fields; /* Fields common to both master struct types */
/* Additional fields follow in an actual jpeg_compress_struct or
* jpeg_decompress_struct. All three structs must agree on these
* initial fields! (This would be a lot cleaner in C++.)
*/
};
typedef struct jpeg_common_struct * j_common_ptr;
typedef struct jpeg_compress_struct * j_compress_ptr;
typedef struct jpeg_decompress_struct * j_decompress_ptr;
The file ``example.c’’ from the official libjpg distribution contains this error handler:
METHODDEF(void)
my_error_exit (j_common_ptr cinfo)
{
/* cinfo->err really points to a my_error_mgr struct, so coerce pointer */
my_error_ptr myerr = (my_error_ptr) cinfo->err;
/* Always display the message. */
/* We could postpone this until after returning, if we chose. */
(*cinfo->err->output_message) (cinfo);
/* Return control to the setjmp point */
longjmp(myerr->setjmp_buffer, 1);
}
and jumps to this point on return from longjmp:
if (setjmp(jerr.setjmp_buffer)) {
/* If we get here, the JPEG code has signaled an error.
* We need to clean up the JPEG object, close the input file, and return.
*/
jpeg_destroy_decompress(&cinfo);
fclose(infile);
return 0;
}
So I guess it is safe to destroy the cinfo struct after decoding failed.
oh yes, sorry - I didn’t notice that call there.
One other thing - I’m not too bothered about getting the error message, and was trying to avoid code bloat by not linking to jerror.c if possible. How badly do you need to get that error string?
I won’t care that much about the jpglib error messages, once I’ll be done with this attached picture frame mess
Anyway, would it harm to have it, at least in debug builds ?
The format_message is only 40 lines long and does not depend on anything else but sprintf … maybe some copy/paste/reformat/preprocessor_conditionals could avoid injecting the whole jerror.c code into juce in debug builds, and bypass it entirely in release ones.
It’s just that if you include code to get the message, your exe gets bloated with every possible error message, formatting functions, etc., which is a waste if it’s never needed.
).