---------- Forwarded message ---------- From: David Capello <davidcapello@xxxxxxxxx> Date: Feb 11, 2008 10:25 AM Subject: Re: Patch for fli.c To: Sven Neumann <sven@xxxxxxxx> Hi Sven, On Feb 10, 2008 10:11 AM, Sven Neumann <sven@xxxxxxxx> wrote: > On Sat, 2008-02-09 at 20:25 -0200, David Capello wrote: > > > On the other hand, I think that the GFLI plug-in has other problems > > (in the gfli.c file). > > IMO we should remove this plug-in from the main distribution. It doesn't > seem to be very useful for the main target audience and it appears to be > buggy and unmaintained. That could be (because the FLI/FLC formats are very old and rarely used in this time). But the current code is better than nothing. > David, perhaps you want to take over maintainance of this plug-in and > keep it updated in the plug-in registry? Sincerely I have a lot of work with Allegro Sprite Editor right now. So maybe in another time (or life :) > We would prefer if you opened one or even several bug reports at > http://bugzilla.gnome.org/ with a description of the problems you see in > the plug-in. You can then attach your patches there. I don't think it should be necessary because the only problem that the user can see directly (that frames with changes in palette really don't change) is already documented in the same gfli.c file as a "Current disadvantage", literally it says: "doesn't support palette changes between two frames" :) The problems are internal to the fli.c utility file (that I was using for my personal app), and happend when you're writing a file with color palette changes, but the Gimp doesn't support them (neither reading nor writing). So that is not a visible/reproducible bug. > > The patch is for this specific file: > > http://svn.gnome.org/svn/gimp/trunk/plug-ins/gfli/fli.c > > It fixes two problems: > > * loading of fli files that specified width=height=0 (that means 320x200) > > Where is this specified? No, It isn't. Is like an "undocument" feature. I test it some time ago with an original FLI file created with the Autodesk Animator Pro. The FLI file couldn't be readed with the GIMP, but it was played with the Allegro library. So I searched in the code of the Allegro library and I found that patch. I'll try to find the Autodesk Animator Pro to create it again and upload the FLI file to test it. > > * writing of differential color chunks (FLI_COLOR and FLI_COLOR_2). > > Again, where is this specified? I don't know the file format, and I > don't think anyone else here has in-depth knowledge about it. So if you > want us to accept patches, you better point us to the official > documentation of the file format and explain what exactly the GIMP > plug-in is not doing correctly here. Sorry for this, here you have a some documents: http://www.ddj.com/architect/184408954 (official) http://www.martinreddy.net/gfx/anim/FLI.txt http://www.martinreddy.net/gfx/anim/FLC.txt http://www.compuphase.com/flic.htm The GIMP doesn't fail because it doesn't support "palette changes between two frames", so the patch fix problems for an internal utility file (fli.c). In anycase the code is unreachable using the GIMP, so there aren't a visible problem right now. Anyway I can explain you why the fli.c fails writing FLI_COLOR chunks. See the "fli_write_color" in the "fli.c" file http://svn.gnome.org/svn/gimp/trunk/plug-ins/gfli/fli.c In that routine the first FLI_COLOR chunk (if old_cmap==NULL) is written right. Then, if some color in the palette changes (old_cmap != cmap) the problems appear. int fli_write_color(..., unsigned char *old_cmap, unsigned char *cmap) { unsigned short num_packets; ... num_packets=0; if (old_cmap==NULL) { /* This works fine (the GIMP just use this, nothing more) */ ... } else { /* The next code is unreachable by the GIMP (but it's used in my application)... */ /* Err... problems, two "num_packets" in the same routine... */ unsigned short num_packets, cnt_skip, cnt_col, col_pos, col_start; num_packets=0; col_pos=0; do { cnt_skip=0; while ((col_pos<256) && (old_cmap[col_pos*3+0]==cmap[col_pos*3+0]) && (old_cmap[col_pos*3+1]==cmap[col_pos*3+1]) && (old_cmap[col_pos*3+2]==cmap[col_pos*3+2])) { cnt_skip++; col_pos++; } /* In the next line col_start must be "col_pos*3". col_pos goes from 0 to 255 at most... col_start goes from 0 to 767 (see below...) */ col_start=col_pos; cnt_col=0; while ((col_pos<256) && !((old_cmap[col_pos*3+0]==cmap[col_pos*3+0]) && (old_cmap[col_pos*3+1]==cmap[col_pos*3+1]) && (old_cmap[col_pos*3+2]==cmap[col_pos*3+2]))) { cnt_col++; col_pos++; } if (cnt_col>0) { num_packets++; ... while (cnt_col>0) { /* ..Here we are using the col_start as a index for cmap... a cmap is a color palette of 256 RGB entries, that is 768 bytes */ fli_write_char(f, cmap[col_start++]>>2); fli_write_char(f, cmap[col_start++]>>2); fli_write_char(f, cmap[col_start++]>>2); cnt_col--; } } } while (col_pos<256); } ... return 0; } I hope that will be helpful. _______________________________________________ Gimp-developer mailing list Gimp-developer@xxxxxxxxxxxxxxxxxxxxxx https://lists.XCF.Berkeley.EDU/mailman/listinfo/gimp-developer
