On Sun, Feb 23, 2014 at 12:24 PM, Christophe <[email protected]> wrote: > > ----- 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 >
we are not removing PPM here, we are renaming it. + /* built-in PNM here refers to anymap format: PPM, PGM, PBM */ + tmp[i++] = "PNM"; PPM is part of PNM formats > 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. all the changes were based on the tests mime package is doing. So i am expecting that to be pretty safe. > > >> @@ -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? Agree with your suggestions, feel free to go ahead and to provide a patch. regards, david -- To unsubscribe, send mail to [email protected].
