[sane-devel] [PATCH 1/2] add PNG format to scanimage
paddy-hack at member.fsf.org
Sun Sep 13 09:15:42 UTC 2015
Matteo Croce writes:
> 2015-09-12 9:13 GMT+02:00 Olaf Meeuwissen <paddy-hack at member.fsf.org>:
>> Hi Matteo,
>> Thanks for taking the trouble to add optional PNG and JPEG output
>> I've done a quick review of both patches and there are a few things that
>> I'd like to point out:
>> - Why do you let users select PNG/JPEG outputs even when the support is
>> not available when HAVE_LIBPNG and/or HAVE_LIBJPEG are undefined?
> If the user selects JPEG/PNG when support is not compiled in it gets
> an error message:
> fprintf(stderr, "XXX support not compiled in\n");
I saw you added that in the new patches you sent. Wouldn't it make more
sense to catch that during option processing rather than when you start
>> - Aren't you leaking memory? The malloc is inside a do-while but the
>> free is on the outside.
> the malloc is in "if(first_frame)" so it's called only once per file
I thought that was the case but wasn't quite sure. I'll pull the code
through valgrind before pushing a proposed upgrade just in case.
>> - It would be nice to update the scanimage manual page to mention the
>> (potential) support for these image formats.
> right, will do in v2
One more thing I noticed, you don't special case 1-bit SANE_FRAME_GRAY
scans for JPEG. The JPEG format does not support 1-bit monochrome so
you'll have to do something about that. Your options are refusing to
create an image all together or expanding to 8-bit monochrome.
Hope this helps,
Olaf Meeuwissen, LPIC-2 FSF Associate Member since 2004-01-27
Support Free Software Support the Free Software Foundation
More information about the sane-devel