[sane-devel] [PATCH 1/2] add PNG format to scanimage

[Olaf Meeuwissen]

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
>> support!
>>
>> 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
outputting data?

>>  - 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
https://my.fsf.org/donate                        https://my.fsf.org/join