Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#88 closed defect (fixed)

Crash: GrafX (win32) hangs on in load file dialogue.

Reported by: zoriarpg@… Owned by: Thomas Bernard
Priority: major Milestone: 2.6
Component: GrafX2 Version: 2.5
Keywords: Cc:

Description

Issue: GrafX crashes (hang, 'not responding') if you select (single-click) on a GIMP format image (.xcf) in the Load File Dialogue.

System Information : Windows 10, v. 10.0.17134, build 17134
(Please inform me, if you need other, more-specific details, for drivers and libs.)

Specific GrafX build: v2.0, build '96.5%'. (2-2.5.1949), win32
Specific GIMP Build (used to save .xcf file): v2.10.8, Windows

How to Reproduce:

  1. Create an image file in GIMP (I'm using v. 2.10.8)
  2. Save as an XCF, with the following settings:

2(a): Layer Mode (Normal). --this was added to the GIMP XCF spec in v2.10.
2(b) 'Use Slxower, but better compression? : No'

  1. Store a .xcf file (saved in GIMPv2) to a path on your filesystem.
  2. Launch GrafX2
  3. Click F3 to Load a file.
  4. Navigate to the path where you saved the .xcf image.
  5. Click on that file in the lister dialogue.
  6. Graphx instantly hangs. Windows rreports it as 'Not Responding', and it cannot be recovered.
  7. You must now manually force-kill the process.

Suspect: Unhandled data type? Undefined behaviour of some sort. Possibly changes in .xcf that aren't supported in the GrafX implementation of the GIMP XCF spec.

Attachments (5)

default_title_new.tga (56.8 KB ) - added by zoriarpg@… 5 years ago.
TGA FIle, Does not Load.
deftitle1.xcf (169.0 KB ) - added by zoriarpg@… 5 years ago.
XCF File, does not load
SDL_image_XCF_fix_infinite_loop.hg.patch (2.5 KB ) - added by Thomas Bernard 5 years ago.
fix for SDL_image 1.2. Avoid infinite loop in XCF loading code
SDL_image_XCF_v11.hg.patch (4.3 KB ) - added by Thomas Bernard 5 years ago.
patch for SDL_image 1.2 : support for XCF version >= 11 (64bits offsets)
issue_88_fixed.png (45.2 KB ) - added by Thomas Bernard 5 years ago.
I finally made it ! ;)

Download all attachments as: .zip

Change History (21)

comment:1 by Thomas Bernard, 5 years ago

Thanks for the bug report

Could you :

  1. Try to reproduce with the "nighlty" build found here : https://gitlab.com/GrafX2/grafX2/-/jobs/artifacts/master/browse?job=job_win32
  1. If that still crashes, attach your test .xcf file to this ticket.

by zoriarpg@…, 5 years ago

Attachment: default_title_new.tga added

TGA FIle, Does not Load.

by zoriarpg@…, 5 years ago

Attachment: deftitle1.xcf added

XCF File, does not load

comment:2 by zoriarpg@…, 5 years ago

The current build no longer crashes, but it neither shows a preview, nor loads either of these files.

I can comprehend issues with the XCF, but the TGA file is 8b indexed (!!), and working with 8b indexed TGA files (for use with Allegro 4), is our primary reason for working with GrafX. :)

Further, the current image data does not clear when I try to load either of these.

e.g., I load a PNG, then I try to load a TGA. The data from the PNG remains, despite being asked if I wish to clear it; and so forth. Is TarGA unsupported, and if so, for what reason?

Last edited 5 years ago by zoriarpg@… (previous) (diff)

comment:3 by yrizoud@…, 5 years ago

If a format is fully implemented by Grafx2's own code (or through libPNG), we can save and load, and we support a preview in the Load-Save window.
When loading, the file content is examined by every format function in order : Is it a GIF ? a PNG ? etc. Only if all format loaders turn a negative, we do a last try using the SDL_Image library. This one can load (but not save) a few extra formats, so it can help import JPG, TGA and TIFF for example.

I'm not sure, but I think the recent issue was due to the work-in-progress on MOTO format. When you tried loading a TGA or XCF it passed through this loader, didn't detect that these *weren't* MOTO files, and then the SDL_Image loader was never reached.

comment:4 by Thomas Bernard, 5 years ago

yves : GrafX2 2.5.1949 is a version that doesn't include any MOTO Format code...

@zoriarpg : I think you have notice the "nightly build" for Win32 includes 2 versions :
grafx2-sdl-2.6wip2249.win32.exe and grafx2-win32-2.6wip2249.win32.exe.
I guess you installed the "win32" one.
As far as I know, XCF and TGA are formats supported only for loading through the SDL_image library.
If you need to load them, You need to install the "SDL" version.

Anyway, I just reproduced your issue (GrafX hangs on loading deftitle1.xcf) and I am investigating the issue.

comment:5 by Thomas Bernard, 5 years ago

This is definitely a bug in SDL_image 1.2.12.
my SDL_image test ist just stuck in the same infinite loop...
Please report the bug to SDL_image authors :
https://www.libsdl.org/projects/SDL_image/release-1.2.html

I have tried with a SDL2 build (under FreeBSD, using SDL2_image 2.0.2) and it does work a little better :
it does load a 343x224 picture, but all pixels are black.

However, I tried to load deftitle1.xcf in Gimp 2.8.11 and had the error "version 11 of XCF format is not supported"

comment:6 by yrizoud@…, 5 years ago

By the way, I noticed that docs\README-SDL_Image.txt is now very wrong.
1.2.10 got replaced by 1.2.12, the license is not the same as before, and it's not the version I can vouch that I compiled - so I don't know what's required to build it and its child DLLs.
(deftitle1.xcf also hangs SDL_Image 1.2.10, so Grafx2 2.4 users are concerned)

Last edited 5 years ago by yrizoud@… (previous) (diff)

comment:7 by Thomas Bernard, 5 years ago

I think that is a bad idea to have (outdated!) .dll files in the source repository.
Any developper can use "make 3rdparty" to build them automatically.
One improvement would be to automatically copy LICENCE of each .dll put in bin to the doc directory for them to be included in distribution packages.

in reply to:  7 ; comment:8 by yrizoud@…, 5 years ago

Replying to Thomas Bernard:

I think that is a bad idea to have (outdated!) .dll files in the source repository.

Some are difficult to build, it has always been an issue on Windows. SDL_image is especially tricky because many file formats are optional: Saying "this is SDL_image 1.2.12 DLL" is not precise, you don't know if this DLL supports JPG (and through which version of libjpg)
Even libpng had several incompatible API changes over time, so it was good that a single source control commit could keep track of the simultaneous change in DLL and fileformats.c.

Any developper can use "make 3rdparty" to build them automatically.

It's nice that you could make your own toolchain this way, but for example I can't run it as it has many more requirements than grafx2 itself : wget, patch... Recoil won't compile with my version of gcc libraries.
In the end, the current SDL_image.DLL no longer runs at all on my machine, while a six-month-old (with same version number 1.2.12) worked. I suppose one of the image-specific dependencies introduced an higher requirement than before, such as "Windows 8 and above". There is still a reason for keeping a set of DLL that work fine from Windows 98 to 10.

One improvement would be to automatically copy LICENCE of each .dll put in bin

These files almost never indicate what they refer to, which is why I started every readme-xxx.txt by "The following files: <xxx.dll xxx-extra.dll> is <library xxx>". Informing the end-user of what he's getting is mandatory for many licenses.
If you change a third-party file (version, origin or build variant), there's normally one line of one doc/readme-xxx.txt that needs be updated as well. If any information is no longer true, scrap it.

by Thomas Bernard, 5 years ago

fix for SDL_image 1.2. Avoid infinite loop in XCF loading code

by Thomas Bernard, 5 years ago

Attachment: SDL_image_XCF_v11.hg.patch added

patch for SDL_image 1.2 : support for XCF version >= 11 (64bits offsets)

comment:9 by Thomas Bernard, 5 years ago

The SDL image code is not very safe :(
I have sent my patches to the SDL_image maintainers.

@zoriarpg : can you test the "sdl" patched version ?

https://gitlab.com/miniupnp/grafX2/-/jobs/127889497/artifacts/browse

in reply to:  8 comment:10 by Thomas Bernard, 5 years ago

Replying to yrizoud@…:

Replying to Thomas Bernard:

I think that is a bad idea to have (outdated!) .dll files in the source repository.

Some are difficult to build, it has always been an issue on Windows. SDL_image is especially tricky because many file formats are optional: Saying "this is SDL_image 1.2.12 DLL" is not precise, you don't know if this DLL supports JPG (and through which version of libjpg)
Even libpng had several incompatible API changes over time, so it was good that a single source control commit could keep track of the simultaneous change in DLL and fileformats.c.

The repository includes 8 years old (!!!) .DLL's without the matching .h/.lib files to correctly build. In height years, numerous security issues have been fixed in the various libraries.
You say libpng had several incompatible API changes over time, so that's very risky to include a DLL without providing the matching .h files.
There are a few #if PNG_LIBPNG_VER in fileformats.c to support theses API changes, but that requires the correct .h file to compile correctly.

Any developper can use "make 3rdparty" to build them automatically.

It's nice that you could make your own toolchain this way, but for example I can't run it as it has many more requirements than grafx2 itself : wget, patch... Recoil won't compile with my version of gcc libraries.
In the end, the current SDL_image.DLL no longer runs at all on my machine, while a six-month-old (with same version number 1.2.12) worked. I suppose one of the image-specific dependencies introduced an higher requirement than before, such as "Windows 8 and above". There is still a reason for keeping a set of DLL that work fine from Windows 98 to 10.

Is that the SDL_image.dll from the repo or the Win32/sdl "nightly build" who doesn't run on your machine ?

About recoil, can you make a bug report, with your GCC version ?

about wget, you can download the sources packages by yourself and put them in the archives/ subdirectories, so the Makefile won't try to download them.

comment:11 by Thomas Bernard, 5 years ago

Owner: changed from pulkomandy to Thomas Bernard
Status: newaccepted

comment:12 by yrizoud@…, 5 years ago

You're right. I was defending old choices because I don't regret them, but it doesn't mean your different choices today won't be the right ones from now on.

Don't hesitate to scrap things that nobody is still using or can't maintain/test :

  • "make ziprelease" block, if you are using something else,
  • nsis installer if you prefer anything else
  • any legacy of using svn version number to identify wip version,
  • target-specific build instructions that only worked on 2.4 etc.

SDL_image.dll : It's when I want to test-run your repo's developments, the 'artifacts' generated by each commit. As I understand it, the gitlab build job overrides the /bin/*.dlls from the repository. I just now tested on a different machine, and here I have a very clear message : "missing libtiff-5.dll".

comment:13 by Thomas Bernard, 5 years ago

I open a new issue #90 for the libtiff-5.dll issue

by Thomas Bernard, 5 years ago

Attachment: issue_88_fixed.png added

I finally made it ! ;)

comment:15 by Thomas Bernard, 5 years ago

Resolution: fixed
Status: acceptedclosed

should be fixed in the nightly build ( 2.6wip2335 )

comment:16 by Thomas Bernard, 5 years ago

Note: See TracTickets for help on using tickets.