----- David Maciejak <[email protected]> a écrit :
> Hi,
>
> I know some of you will say that patch is not sexy :)
> It's my first look into wrlib. I will provide more.
>
> The patch is working on that:
>
> 1) the tests examples were not compiling cause of a missing X11 lib call
> 2) load.c is used to identify the file format, i rebased the checks on
> what is provided
> from mime package, it will be faster
> 3) according to that checks i was able to see that some netpbm support
> type are missing (exactly: ascii graymap (PGM files) and pixmap (PPM)
> and ascii/binary bitmap (PBM))
>
> See the link below for more details.
> http://en.wikipedia.org/wiki/Netpbm_format
>
> inlined & enclosed
>
> regards,
> david
>
> ---
> wrlib/load.c | 18 ++--
> wrlib/load_ppm.c | 270
> +++++++++++++++++++++++++++++++++++++++--------
> wrlib/tests/Makefile.am | 2 +-
> wrlib/tests/testdraw.c | 1 +
> wrlib/tests/view.c | 6 +-
> 5 files changed, 240 insertions(+), 57 deletions(-)
>
> diff --git a/wrlib/load.c b/wrlib/load.c
> @@ -78,8 +78,8 @@ char **RSupportedFileFormats(void)
>
> /* built-in */
> tmp[i++] = "XPM";
> - /* built-in */
> - tmp[i++] = "PPM";
I think it is an error to remove PPM from the list, as:
- it is still supported;
- it might break compatibility with apps using the WRaster library
It is ok to just add PNM to the list as one more format (taking care of the
size of the array).
> @@ -295,7 +295,7 @@ static WRImgFormat identFile(const char *path)
> RETRY( fclose(file) )
>
> /* check for XPM */
> - if (strncmp((char *)buffer, "/* XPM */", 9) == 0)
> + if (strncmp((char *)buffer, "/* XPM", 6) == 0)
I'm not sure it is a good idea to shorten the compare string, the description
of the format does not seem leave that much liberty here.
> @@ -318,7 +316,7 @@ static WRImgFormat identFile(const char *path)
>
> /* check for GIF */
> - if (buffer[0] == 'G' && buffer[1] == 'I' && buffer[2] == 'F')
> + if (buffer[0] == 'G' && buffer[1] == 'I' && buffer[2] == 'F' &&
> buffer[3] == '8')
If you're up to making this detection more correct, you can also make one step
further:
- buffer[4] is always either '7' or '9'
- buffer[5] is always 'a' (yes, lowercase)
> diff --git a/wrlib/load_ppm.c b/wrlib/load_ppm.c
> +static RImage *load_graymap(FILE * file, int w, int h, int max, int raw)
> [...]
>
> - if (!raw)
> + if (raw != '2' && raw != '5')
> return image;
Some historic stuff here, but shouldn't the proper behaviour be to set
RErrorCode to RERR_BADFORMAT, release the image and return NULL, instead of
returning an empty image unrelated to what was in the file?
> + if (raw == '2') {
> + [...]
> +
> + if (val > max || val < 0) {
> + RErrorCode = RERR_BADIMAGEFILE;
> + return NULL;
> + }
Maybe a little RReleaseImage before returning NULL?
> + if (raw == '5') {
> + char *buf;
> + buf = malloc(w + 1);
> + if (!buf)
> + return NULL;
[same here, RErrorCode + RReleaseImage]
> + for (y = 0; y < h; y++) {
> + if (!fread(buf, w, 1, file)) {
> + free(buf);
> + RErrorCode = RERR_BADIMAGEFILE;
> + return NULL;
> + }
[same here, RErrorCode + RReleaseImage]
> -static RImage *load_pixmap(FILE *file, int w, int h, int max, int raw)
> +/* PPM: support for portable pixmap ascii and binary encoding */
> +static RImage *load_pixmap(FILE * file, int w, int h, int max, int raw)
> {
> [...]
>
> - if (!raw)
> + if (raw != '3' && raw != '6')
> return image;
[same here, RErrorCode + RReleaseImage]
>
> ptr = image->data;
> if (max < 256) {
> - i = 0;
> + if (raw == '3') {
> + int x, y, val;
> + for (y = 0; y < h; y++) {
> + for (x = 0; x < w; x++) {
> + for (i = 0; i < 3; i++) {
> + val = pm_getuint(file);
> +
> + if (val > max || val < 0) {
> + RErrorCode = RERR_BADIMAGEFILE;
> + return NULL;
[same here, RReleaseImage]
> + }
> +
> + val = val * 255 / max;
> + *(ptr++) = val;
> + }
> + }
> + }
> + } else if (raw == '6') {
> + char buf[3];
> + while (i < w * h) {
> + if (fread(buf, 1, 3, file) != 3) {
> + RErrorCode = RERR_BADIMAGEFILE;
[same here, RReleaseImage]
> + return NULL;
> + }
> +
> + *(ptr++) = buf[0];
> + *(ptr++) = buf[1];
> + *(ptr++) = buf[2];
> + i++;
> + }
> + }
> + }
> + return image;
> +}
> +
> +/* PBM: support for portable bitmap ascii and binary encoding */
> +static RImage *load_bitmap(FILE * file, int w, int h, int max, int raw)
> +{
> [...]
> +
> + if (raw != '1' && raw != '4')
> + return image;
[same here, RErrorCode + RReleaseImage]
> + ptr = image->data;
> + if (raw == '1') {
> + int i = 0;
> while (i < w * h) {
> - if (fread(buf, 1, 3, file) != 3) {
> + val = pm_getuint(file);
> +
> + if (val > max || val < 0) {
> RErrorCode = RERR_BADIMAGEFILE;
[same here, RReleaseImage]
> return NULL;
> }
> [...]
> diff --git a/wrlib/tests/testdraw.c b/wrlib/tests/testdraw.c
> @@ -561,6 +561,7 @@ int main(int argc, char **argv)
> {
> RContextAttributes attr;
> int visualID = -1;
> + (void) argc;
I would suggest to have a blank line before this line, as it is not a variable
declaration but a statement.
--
To unsubscribe, send mail to [email protected].
