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

[Matteo Croce]

2015-09-13 11:15 GMT+02:00 Olaf Meeuwissen <paddy-hack at member.fsf.org>:
> 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?

Right, it would be better

>>>  - 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.

ok, let me know if you find some leak.

>>>  - 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.

I'll try to expand it to 8 bit gray

> 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



-- 
Matteo Croce
OpenWrt Developer
  _______                     ________        __
 |       |.-----.-----.-----.|  |  |  |.----.|  |_
 |   -   ||  _  |  -__|     ||  |  |  ||   _||   _|
 |_______||   __|_____|__|__||________||__|  |____|
          |__| W I R E L E S S   F R E E D O M
 -----------------------------------------------------
 CHAOS CALMER
 -----------------------------------------------------
  * 1 1/2 oz Gin            Shake with a glassful
  * 1/4 oz Triple Sec       of broken ice and pour
  * 3/4 oz Lime Juice       unstrained into a goblet.
  * 1 1/2 oz Orange Juice
  * 1 tsp. Grenadine Syrup
 -----------------------------------------------------