FLTK logo

STR #3491

FLTK matrix user chat room
(using Element browser app)   FLTK gitter user chat room   GitHub FLTK Project   FLTK News RSS Feed  
  FLTK Apps      FLTK Library      Forums      Links     Login 
 Home  |  Articles & FAQs  |  Bugs & Features  |  Documentation  |  Download  |  Screenshots  ]
 

Return to Bugs & Features | Roadmap 1.3 | SVN ⇄ GIT ]

STR #3491

Application:FLTK Library
Status:1 - Closed w/Resolution
Priority:4 - High, e.g. key functionality not working
Scope:3 - Applies to all machines and operating systems
Subsystem:Image Support
Summary:Fl_GIF_Image cannot handle some animated GIF files
Version:1.3.4
Created By:moldi_m
Assigned To:AlbrechtS
Fix Version:1.3.5
Fix Commit:35e03733f48397819902a4ef2ebff9bcb8676f54
Update Notification:

Receive EMails Don't Receive EMails

Trouble Report Files:


Name/Time/Date Filename/Size  
 
#1 moldi_m
12:41 Aug 28, 2018
colormap_test.gif
3k
 
 
#2 moldi_m
06:35 Aug 31, 2018
patch_3491.txt
2k
 
 
#3 moldi_m
04:08 Sep 05, 2018
patch_3491_2.txt
2k
 
 
#4 moldi_m
07:12 Jan 02, 2019
ani_1.gif
3k
 
 
#5 moldi_m
07:12 Jan 02, 2019
gif_crash.cpp
1k
 
 
#6 moldi_m
03:15 Jan 03, 2019
gif_com.c
7k
 
 
#7 AlbrechtS
09:19 Jan 05, 2019
patch_3491_v3.diff
3k
 
 
#8 moldi_m
03:42 Jan 06, 2019
Sunflower_32wo.gif
36k
 
 
#9 moldi_m
03:43 Jan 06, 2019
Sunflower_32.gif
36k
 
 
#10 AlbrechtS
06:20 Jan 06, 2019
patch_3491_v4.diff
3k
 
     

Trouble Report Comments:


Name/Time/Date Text  
 
#1 moldi_m
12:41 Aug 28, 2018
Fl_GIF_Image cannot handle some animated GIFs and this will also crash Fl_Shared_Image. The problem happens with GIFs which do not have an GlobalColorMap. This is allowed if every of the following images have a local color map.

The problem now is, that Fl_GIF_Image tries to calculate the size of the not available global color map. The result is always 2 but should be 0! There also should be no warning and no dummy color map at this time.

When it comes to the first image there MUST be a local color map and the size must be re-calculated since 2 is much too low. If there is no local color map AND no global color map, this is an error. I have added 2 lines, which seem to solve the problem:

----
      ch = NEXTBYTE;
      BitsPerPixel = ((ch & 0x70) >> 4) + 1; // <<<
      ColorMapSize = 1 << ((ch & 7) + 1);    // <<<
      Interlace = ((ch & 0x40) != 0);
      if (ch&0x80) {
        // read local color map
...
----

I have attached a troble causing file named "colormap_test.gif". Since it seems I only can attach one file I coppy the test code here. It is a modified version from "Erco's FLTK Cheat Page".

----
#include <FL/Fl.H>
#include <FL/Fl_Window.H>
#include <FL/Fl_Shared_Image.H>
#include <FL/Fl_GIF_Image.H>
#include <FL/Fl_Box.H>

int main() {
    const char * filename = "colormap_test.gif";

    fl_register_images();
    Fl_Window     win(720,486);
    Fl_Box        box(10,10,720-20,486-20);
    Fl_GIF_Image gif(filename);
    box.image(gif);
    win.show();
    return(Fl::run());
}
----

Please let me know if you need more information.

kind regards
Manfred
 
 
#2 moldi_m
14:42 Aug 28, 2018
Sorry, but i have to add a small correction.

The calculation of BitsPerPixel is wrong when working with the ImageHeader since these Bits are unused there.

This calculation should be done before with the "Logical Screen Descriptor" where the global color map is indicated (or not). And this value is valid for the whole file.

BTW, i have seen in some files, that the BitsPerPixel value is sometimes smaller than I would expect when comparing this with the color map size. I don't know if this is important.

regards Manfred
 
 
#3 moldi_m
06:37 Aug 31, 2018
I finaly added the attached changes to my system and it works for me. My applications do not longer crash from such files.

But I am unsure about the error handling at the end of the changes.

regards
Manfred
 
 
#4 AlbrechtS
11:49 Aug 31, 2018
Thanks for the report and the patch. I'm looking into it...  
 
#5 moldi_m
03:05 Sep 04, 2018
I have just read the GIF specification from the w3c. They are more detailed than John Mianos book "Compressed Image File Formats".

From that it becomes clear that the Bits_Per_Pixel value in the Logical Screen Descriptor is only a information about the original Image. Therefore the BitsPerPixel variable used in Fl_GIF_Image should derivated from the color map size (as in the original file) and should be moved to the place, where the size of the active color map is calculated.

sorry for that mistake.
Manfred
 
 
#6 AlbrechtS
03:58 Sep 04, 2018
Hmm, thanks for this correction. I'm not sure if I understand the consequences. Do you intend to upload a new patch?

I also read the GIF specification but got distracted by other work and could not yet compare with actual code and your patch. A new patch (or confirmation that the old patch is still valid) would be very much appreciated. TIA.
 
 
#7 moldi_m
09:21 Sep 04, 2018
> I'm not sure if I understand the consequences.

As far as I have seen, the decoder does not use this value. But I have seen another strange code sequence in the original file which depends on this:

----
...
  if (BitsPerPixel >= CodeSize)
  {
    // Workaround for broken GIF files...
    BitsPerPixel = CodeSize - 1;
    ColorMapSize = 1 << BitsPerPixel;
  }
...
----
I have no idea what sort of "broken GIFs" are handled here. But I think my coorection will keep this code functional.

 > Do you intend to upload a new patch?

I will do this tomorrow after some more tests.
regards
Manfred
 
 
#8 moldi_m
04:10 Sep 05, 2018
I just uploaded the corected version of the patch.
Manfred
 
 
#9 AlbrechtS
07:48 Jan 01, 2019
Reduced priority to 3 (moderate).

This issue should not block the release of FLTK 1.3.5.
 
 
#10 moldi_m
09:23 Jan 01, 2019
I am sad to hear this because without that patch my application crashes when such an image is moved to Fl_Shared_Image.

Happy new year
Manfred
 
 
#11 AlbrechtS
10:20 Jan 01, 2019
I'm sorry for this but we need to set priorities. Our team is so small and progress is so slow that we need to do at least tiny steps. A new release is more than important particularly because Apple changed so much in their new macOS 'Mojave' release that they broke FLTK.

I can't promise anything, but ... can you post a small but complete demo program that crashes with a valid gif file (such as the one you uploaded already)? This could help much to test this issue. TIA.

Happy new year as well!
 
 
#12 moldi_m
07:22 Jan 02, 2019
I understand there are other priorities.

I just uploaded a small test programm to show the crash. Unfortunately that does not always happen immediately and so the programm makes a D-tour.

The trouble maker is loaded when the button is pressed. So you also need the second image (ani_1.gif), I just uploaded.

Manfred
 
 
#13 AlbrechtS
15:31 Jan 02, 2019
Hi Manfred, thanks for uploading the demo program and the new image. I can confirm that your demo program crashes when the button to load the "bad" image is clicked.

I could also see that our test/prixmap_browser demo program crashes when you try to open/display colormap_test.gif (no matter if another image was opened before). That seems to be a bug - at least if the image is correct according to the Gif specs.

I'm investigating the issue. Your patch seems to fix the crash, that's good. But I can't tell (yet) if it would break other programs - which should not happen, hopefully. We need to verify that your patch is correct WRT Gif image specification. I assume it is (you wrote that you read the spec), but we still need to verify...

Are you aware of some test service (or source code) that can be used to check an existing Gif image whether it conforms to the specification? This would be good to know.

I still can't promise anything, but I'm further looking into it.
 
 
#14 moldi_m
03:14 Jan 03, 2019
On my system (Ubuntu Mate 14.04) the pixmap_browser does not crash imediately. But it shows strange patterns.

The first file was made by Gimp. For the second file I moved the color tables manualy to the local images and on the first attempt i forgot one.

Then the program EOM (Eye of Mate) gave me an error message saying:
"There is no global color map and one of the images does not have an local colormap".

I don't have an official test program. But for one of my projects, a editor for comments in pictures, I wrote a little test program. It's not perfect since it does not decode the files completely, but it prints the file positions, so you can compare with an hex editor. The program is based on information from the book: "Compressed Image File Formats" by  John Miano.

I will upload the test program. Don't be confused by the name "gif_com.c".

Manfred
 
 
#15 AlbrechtS
13:14 Jan 03, 2019
Thanks for the info and the test program. The test program is very helpful to see the innards of the Gif image files. Thank you.

I was using Ubuntu 18.04 and ran my tests with FLTK 1.4.0 -- maybe this makes a difference. I applied your patch and used it with some more Gif images, reading the specs and comparing the code. Looks fine so far, but I'll do some more tests, maybe tomorrow. I'll keep you updated.
 
 
#16 AlbrechtS
08:48 Jan 05, 2019
FTR: the GIF specification can be found at
https://www.w3.org/Graphics/GIF/spec-gif89a.txt
 
 
#17 AlbrechtS
09:19 Jan 05, 2019
Manfred, I noticed that your patch disabled loading images w/o color tables which the old code allowed (or at least tried to do).

I implemented your patch and (re-)added the ability to load images w/o color tables by loading a gray-scale color table as it was done in the old code. However, I also followed the standard and set the first two entries in the built-in (default) color table to black and white, respectively.

Can you please test and verify the uploaded new patch (patch_3491_v3.diff)? I think you know the standard and the code pretty well, so I'd like you to check the patch.

I tested the patch with several GIF images and I hope it works with your images as well. BTW, do you have (or can you produce) an image w/o any color table (no global color table and no local one for the first image)? I'd appreciate if I had such an image (please upload if possible).

Testing the patch: if you enable (uncomment) the code in line 220:

  // ColorMapSize = 0; // *TEST* ignore existing color tables

then *all* GIF images should load with a warning

  Fl::warning("%s does not have a color table, using default.\n", infname);

and show as gray-scale images (somehow). This worked for me pretty well.

I'd like to get your feedback on this patch for FLTK 1.3.x, and if it is positive I believe we can commit this after a little cleanup, of course.

What do you think?
 
 
#18 moldi_m
10:46 Jan 05, 2019
I hope i can test the new patch later this evening or tomorrow morning. In the moment i am searching a bug when reading meta data from png files.

I have not realised that the original code tries to generate a gray-map. I only saw that the loop was very short because of the wrong color table size (always 2 without a global color map).

I used the ColorMapSize=0 as indicator for the absence of a global color map.

And yes, i have read in the specs that NO color map is allowed but I think this is not practicable when using 'stand alone' files.

About the warning: I think a warning should only be sent if there is no GLOBAL color map AND no local color map because a local color table has priority. And it is very important that no color index from the decoded data tryes to access a color behind the actual used table.

About the gray map I see 2 possible solutions:
 * a gray map of maximum size
 * a gray map of optimum size - but this would require to analyse the
   maximum used color index. In the moment I have no idea how to do that.

And yes, I think I can remove the color table from a GIF file. But I have to modify my helper program for that.

About the different error pictures: I think that the amout of memory may have something to do with that. My new PC has 16 GiB and one of my older PCs only 0.5 GiB.
 
 
#19 AlbrechtS
12:46 Jan 05, 2019
Yeah, no color table is allowed, and there are two options to resolve this: (a) use the "previous" global color table, and (b) use a default or system color table. As you said, case (a) doesn't make sense for standalone image files, so I chose (b) which was also the solution in the previous code.

I understand that you didn't have time to look closer at the code, so I may add some thoughts about the gray color map.

First of all, its size: as you can see I put the code to create the color map directly before decoding the image. At that point the variable 'CodeSize' has been read from the image (first byte of image/LZW data) and 'BitsPerPixel' has been initialized from the image header (and has not been overwritten from any color table).

The "Workaround for broken GIF files" has been applied as well. BTW: I'm not sure that this "workaround" is 100% correct, but as long as we don't see such a broken GIF file, who knows.

Anyway, I use the calculated BitsPerPixel size to build the gray color table and use black and white in index 0 and 1 resp. There is also a (commented out) option to fill the color table to maximum size (256) but I believe we don't need this.

Note that this is really necessary to avoid a regression, even if the behavior if an image doesn't have a color map is "undefined". We should not change this in a 1.3.x release, that's why I added it again. My test scenario (uncomment line #220) showed that it works in my own tests.

About the different error pictures: don't worry, undefined behavior is ... undefined(!), i.e. if you overwrite data or do anything else in an undefined way, interpret compressed data in a wrong way, you can't compare error behavior on different systems.

Thanks for your help improving the library.
 
 
#20 AlbrechtS
12:58 Jan 05, 2019
I forgot one point: yes, a warning is only issued if no color table was found, i.e. neither a global nor a local one.  
 
#21 moldi_m
03:42 Jan 06, 2019
I have just tested version 3 of the patch. It works so far but the gray scale option looks funny sometimes. This is when the used colors are not sorted in the original file.

I also made a GIF withot any color map. This causes my application to crash again!
Gimp and feh opens them as black images and EyeOfMate cpmplains the absence of any color map and refuses to open.

There is a minor problem with my example file "Sunflower_32wo.gif". Without a color map I do not know what to use as background color index (byte at position 0x0B). But this has nothing to do with the crash.

The crash is caused by the wrong value of ColorMapSize. This value is invalid if there is no color table. The crash does not happen if the value at position 0x0A is changed to 0x04 or 0x44. But I set the low order bits to 0 because all files I found with no global color map did so.

I think, if we do not ask the decoder what maximum color index is used, it is better to always use a maximum sized color map, since crash prevention has priority.

If one does not provide a color map, he cannot expect the picture is shown correctly.

regard
Manfred
 
 
#22 AlbrechtS
06:20 Jan 06, 2019
Please try again with uploaded 'patch_3491_v4.diff'. The only thing I fixed was the missing initialization of BitsPerPixel in the no-color-table case. Note that BitsPerPixel is a redundant (helper) variable anyway. It's used for color table size calculation but it not read or used for image decompression.

The GIF w/o color table loads fine with this patch.

You wrote:
> I think, if we do not ask the decoder what maximum color index is
> used, it is better to always use a maximum sized color map, since
> crash prevention has priority."

The variable CodeSize is read from the compressed image (its first byte) and should be equivalent to the number of "bits per pixel + 1" in the image, hence we use the proper size to build the default (gray-scale) color table.

> If one does not provide a color map, he cannot expect the picture
> is shown correctly.

Yep, exactly. Images looking "funny" are better than "nothing" IMHO, and I don't care about sort order of colors or anything else. ;-)

Is the v4 patch OK for you?
 
 
#23 moldi_m
07:54 Jan 06, 2019
Yes, the v4 patch works fine now.

I know about the first byte following the image header or color table. But I did not realised the relationship with the color table.

Sorry for causing so much trouble.
Best regards
Manfred
 
 
#24 AlbrechtS
08:30 Jan 06, 2019
Don't worry, it's all fine. Your help and input is much appreciated.

BTW, I took the chance to familiarize myself with the GIF image spec to prepare for another long-standing issue: animated GIF files, i.e. displaying GIF with real animation in FLTK. See, e.g. STR 829:
https://www.fltk.org/str.php?L829

I think I can commit the changes in v4 after cleanup so we'll get them into 1.3.5. I'll post an update when done.
 
 
#25 AlbrechtS
09:25 Jan 06, 2019
Fixed in Git repository.

(Reset STR prio to HIGH).

Please test and confirm.

The fix will be in FLTK 1.3.5.
 
 
#26 moldi_m
04:25 Jan 07, 2019
> Please test and confirm.

Sorry, I don't know how to do that. I have not yet git installed and I have not used it before.

Also I cannot see the new version on GitHub.
 
 
#27 AlbrechtS
05:25 Jan 07, 2019
If you don't have git you can still access the newest git content by downloading a .zip file from GitHub. Go to our GitHub repo:
https://github.com/fltk/fltk

Select branch 'branch-1.3' since this is where I committed (there's a selection somewhere on the top left side):
https://github.com/fltk/fltk/tree/branch-1.3

Then you can use the green button on the right hand side (Clone or download) to "Download ZIP" which is this URL:
https://github.com/fltk/fltk/archive/branch-1.3.zip

Of course you can use the above link directly but I wanted to describe the procedure...
 
 
#28 AlbrechtS
05:32 Jan 07, 2019
PS: using git can be hard with all its options, but for just downloading and updating from a git source is simple:

$ git clone https://github.com/fltk/fltk.git

(see the Download button as well for the link), then

$ cd fltk

If you want to use branch-1.3 you need to check it out, otherwise you're on the 'master' branch

$ git checkout branch-1.3

To update to the latest commit(s):

$ git pull

That's it, see also the article "Git for FLTK Users" here:
https://www.fltk.org/articles.php?L1349
and more links on our download page:
https://www.fltk.org/software.php#GIT
 
 
#29 moldi_m
02:06 Jan 08, 2019
> Select branch 'branch-1.3
That was it!

I tried to make a completely clean install but I got an error about a missing file.

configure: error: cannot run /bin/bash ./config.sub

Is it save to copy that file from the older .zip?
 
 
#30 AlbrechtS
04:17 Jan 08, 2019
You should be able to run 'make' directly (w/o configure) - this should copy the missing files and run configure. If this doesn't work you can copy the missing files from the misc/ directory: misc/config.sub + misc/config.guess. It would also be safe to copy them from your old zip file but the files in misc/ would likely be newer.

Another option is to run `automake --add-missing' to copy the files from your local system.
 
 
#31 moldi_m
05:30 Jan 08, 2019
I just copied the 2 files from GitHub and it works now. I have tested the new version with 2 of my programms.

But there is another point I think you should know. It has nothing to do with this STR. I have tested with ABI version 1.03.00 and 1.03.04. With version 1.03.04 the fractal demo crashes. I made that test because I observed that recently when installing FLTK on my Notebook. I was thinking that was a problem with notebooks graphics card.

Regards
Manfred
 
 
#32 AlbrechtS
08:14 Jan 08, 2019
Thanks for confirmation that it works.

Regarding the other issue with the fractals demo: can you please be so kind to file another bug report (STR) so we can close this one? It is always good (and required for maintenance) that we have separate STR's for separate issues. I'll test this, but anyway, otherwise it might get forgotten... BTW: I'm reading all bug reports so I'll see when you report this in another STR (and other devs as well). Please don't forget to add info about your system configuration (platform, i.e. Linux, Windows, macOS, system version, FLTK version, etc.). In this particular case it might be necessary to know such facts. TIA.
 
 
#33 AlbrechtS
10:50 Jan 09, 2019
Fixed in master (1.4.0) as well.

Commit: 1a1492a174fd0ec2942b16f4fc96f9cbd639675c

Closing this STR now. Thanks for all your help.
 
     

Return to Bugs & Features ]

 
 

Comments are owned by the poster. All other content is copyright 1998-2024 by Bill Spitzak and others. This project is hosted by The FLTK Team. Please report site problems to 'erco@seriss.com'.