Matteo Croce writes: > 2015-09-12 9:13 GMT+02:00 Olaf Meeuwissen <paddy-h...@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 -- sane-devel mailing list: sane-devel@lists.alioth.debian.org http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/sane-devel Unsubscribe: Send mail with subject "unsubscribe your_password" to sane-devel-requ...@lists.alioth.debian.org
