GNOME Bugzilla – Bug 140443
standard file formats, use PNG instead of .PAT for pattern files
Last modified: 2004-06-03 12:03:00 UTC
I would like to see the Gimp use more standardized file formats. I would like to see more integration between the Gimp and Gnome and or KDE. Both Gnome and KDE ship a variety of desktop wallpapers and textures in Jpeg and PNG format. If the Gimp were to use PNG (and or JPEG) for its pattern files then it would be a simple case to add an extra patterns folder from the preference dialog and the Gimp would get a whole lot more patterns and textures essentially for free because most distribtions are already ship them. (there is probably a way this could be done automatically). Conversely if the Gimp uses standard file formats then other programs that are interested in textures will be able to share and reuse the texture files that ship with the Gimp. Potentially this could save the Gimp developers work maintaining the Gimp data extras and instead through Freedesktop.org Gnome and KDE might do the work of providing background images wallpapers (a lovely collection of high resolution sample images!) and textures. This benefits Gnome and KDE as much as it benefits the Gimp and may serve as an opportunity to get external developers to help out if it (and other future plans) were mentioned in any upcoming release notes. 'Tis an idea. Consider it, accept it, reject it, do what you will. (If you are considering rejecting the idea, please think carefully if it should just be postponed rather than rejected outright)
Nothing wrong about your idea. It's the right thing to do.
changed the subject line, PNG wont always be the most appropriate format for textures, sometimes jpegs will be a better choice.
A lossy format for patterns? You must be kidding. Alan, I hereby ask not do any further changes to GIMP bug summaries. If you think a bug summary needs to be changed, please suggest the change and we will do it for you.
I know of course taht Jpegs are not ideal for storing textures but some people do it anyway and it would help to be able to use standard file formats to be used as patterns, rather than limiting patterns to any one file format.
gdk-pixbuf is what is going to be used so any gdk-pixbuf supported format will work. What's your point?
Created attachment 28124 [details] [review] Patch to enable GdkPixbuf patterns Here's a patch to get all GdkPixbuf supported files loaded as patterns. Its error handling hasn't been fully tested and it's a patch against 2.0 (although it patches cleanly against the HEAD). One decision which might be controversial is to have the "normal" gimp_pattern_load function load GdkPixbuf stuff, and the old gimp_pattern-load moved to gimp_pattern_legacy_load. This seemed reasonable to me, but it's not a major thing (gimp_pattern_load isn't used anywhere else), and if there's a decent way to name the new function feel free to change it and the handler in gimp.c. Dave.
Created attachment 28125 [details] [review] Improved error handling (propagate error) Fixed up a bit.
Why is the old pattern loader now the "legacy" loader in your patch?
Your error handling needs more cleanups. You cannot put filenames into error messages directly but need to use gimp_utf8_from_filename(). Also I don't understand why you aren't using error->message to give a more detailed (and probably more useful) error messsage. Also I think that we don't need to deprecate the old pattern format (and it's loader).
I thought I made it clear that the naming of the functions was not important. The old .pat format was called the "legacy" format because it's a GIMP only format, it's the older format, and it makes sense to me to use (as the original poster said) standard file formats where they can be used, which would imply deprecating (at some stage) the .pat format, which has absolutely nothing over png. For the error handling, the error cannot be handled here, we know the error domain, so the error is actually sufficiently descriptive. The error doesn't get reset here, and is propagated to the pattern dock factory, where it can be handled more elegantly, and the original error->message gets used. It made sense to include the filename somewhere in the error message. But I don't know GErrors very well, if there are some mistakes in how I handled it, I guess you can feel free to change things. Dave.
The point is simply that filenames are not necessarily UTF-8 encoded and thus cannot be used directly in g_message(). You need to use gimp_filename_to_utf8() just as you did afew lines above in your patch. Also it would certainly make sense to use error->message if the file can't be opened. That gives the user a chance to find out what went wrong (permission problems for example).
Created attachment 28280 [details] [review] Updated patch whhich aalso retrieves a tEXt chunk from png patterns as the pattern name. Updated the patch. Name setting is slightly improved (and would be greatly improved if GdkPixbuf didn't throw away comments in general). Someone suggested to me that it's silly for the GIIMp to deleggate file reading to GdkPixbuf when it would just use its own loaders - I guess that would require loading plug-ins before data, and suppressing progress bars. It would also mean long start-up times if there were a lot of non-native patterns, but is the idea worth thinking about? Cheers, Dave.
Created attachment 28282 [details] [review] Don't try to free strings returned from gdk_pixbuf_get_option
I don't see any advantage in using the PNG plug-in over gdk-pixbuf which we are already using anyway. Please don't use gimp_utf8_from_filename() for anything else but error reporting. g_filename_to_utf8() is the GLib API to use. gimp_utf8_from_filename() was only added to provide an easy way to convert to UTF-8 w/o having to free the result. It is not meant to be used in common code paths.
I think the patch can be committed and the remaining issues can be taken care of in CVS.
> it's silly for the GIMP to deleggate file reading to GdkPixbuf when it would just use its own loaders perhaps but presumably the GIMP is going to use gdkpixbuf anyway at least for some things (correct me if I'm wrong) and for simple flat formats that gdkpixbuf can fully and properly support, pushing the file formats to gdkpixbuf also pushes the maintainance burden or at least shares it with the other GTK users. Not sure if this is relevant (i guess it depends on how the pattern previews are generated) but on Planet.Gnome.org Federico mentioned that in some case it is more efficient to use gdk-pixbuf-new-from-file-at-size http://developer.gnome.org/doc/API/2.0/gdk-pixbuf/gdk-pixbuf-file-loading.html#gdk-pixbuf-new-from-file-at-size
That would only make sense if we had the size already but we don't.
The advantage of using our plug-ins is getting the gimp-comment parasite, which is a nice way to name patterns. That, and support for lots more formats than GdkPixbuf. But there's lots of hassle too. Should this go into both branches, or just 2.1? Given that it's a small local change, it could probably slip into 2.0 without anyone noticing. Cheers, Dave.
Fix committed to HEAD: 2004-06-03 Dave Neary <bolsh@gimp.org> * app/core/gimp.c: * app/core/gimpdatafactory.c: * app/core/gimppattern.[ch]: Add support for GdkPixbuf patterns, so now all of png, jpex, pnm, xbm, bmp, gif, ico, pcx, ras, tga, xpm and tiff can be used for patterns.
The disadvantage of using our plug-ins is that it would slow things down dramatically. So that's basically not an option. If we need more support for options in gdk-pixbuf (for reading and writing comments), then we need to provide patches for gdk-pixbuf. That's what I did when I needed the tXt chunk for implementing thumbnail support. BTW, your use of gdk_pixbuf_get_option() is wrong. If you want to read the comment field from a PNG file you need to use gdk_pixbuf_get_option (pixbuf, "tEXt::Comment"); Please remove the line that tries to read an option named "comment". This simply doesn't exist. See also http://www.libpng.org/pub/png/spec/1.1/PNG-Chunks.html#C.tEXt for a list of predefined keywords.
OK, please don't do anything, I will clean this up.
2004-06-03 Sven Neumann <sven@gimp.org> * app/core/gimpdatafactory.c (gimp_data_factory_load_data): removed commented-out message. * app/core/gimppattern.[ch]: fixed handling of errors and PNG comments in new pattern loader. Renamed functions for consistency with other data loaders. * app/core/gimp.c: changed accordingly.
The idea of comment was to have things Just Work when I got around to patching the various formats in GdkPixbuf (which I had planned to after opening bug #143608 about it). Of course, I could just update this when we're using a GdkPixbuf that supports it. By the way, since the GIMP saves a iTXt chunk in preference to a tEXt chunk for the comment parasite, would it be reasonable to add the ability to get an iTXt::Comment out too (although looking at the png spec, iTXt doesn't seem to be mentioned). Anyway - please feel free to tidy this up if you want. Cheers, Dave.
gdk-pixbuf handles iTXt chunks transparently.