GNOME Bugzilla – Bug 143470
GnomeThumbnailFactory does not save Thumb::Image::{Width,Height}
Last modified: 2007-03-16 23:38:34 UTC
The GnomeThumbnailFactory object does not currently save the "Thumb::Image::{Width,Height}" metadata, as suggested by the spec.
Created attachment 28220 [details] [review] Patch to save "tEXt::Thumb::Image::{Width,Height}" metadata
The specification states that the height and width of the _original_ image should be saved. Your patch seems to save the height and width of the thumbnail.
Doh!
Created attachment 28252 [details] [review] Updated patch This patch stores the original height/width as string GObject data on the pixbufs generated by _generate_thumbnail(), and reads those from _save_thumbnail(). It's not particularly pretty, but it works, and there's a high likelyhood that if someone is using _save_thumbnail() they probably got the pixbuf from _generate_thumbnail().
Comment on attachment 28252 [details] [review] Updated patch The patch still applies; can it please be reviewed and checked in?
This is probably something that needs to be reviewed by Alex and he's away for another week :-/
Alex, is this important enough to ask for freeze break permission?
I doubt its important enough, since we've released multiple versions with this problem already.
Getting it in now before we freeze again :-)
gnome_thumbnail_factory_save_thumbnail() generates a critical warning at gtk+ level in gdk-pixbuf/io-png.c. Maybe gtk+ should be modified too (test for values[i] != NULL before calling g_convert).
sorry: you need glib cvs, and the crash is in function: real_save_png() at line ~864. text_ptr[i].text = g_convert ...
at last: gnome_thumbnail_factory_save_thumbnail() width/height == NULL, that's the problem.
The regression is being tracked in bug 327323.
Is this really working? Inspection of metadata in thumbnails created with latest libgnomeui from SVN doesn't have it: $ ls -lh bc4799f35f4efa285e8dc29a0cbadae0.png -rw------- 1 claudio claudio 22K 2007-03-06 00:12 bc4799f35f4efa285e8dc29a0cbadae0.png $ pngmeta --all bc4799f35f4efa285e8dc29a0cbadae0.png pngmeta: PNG metadata for bc4799f35f4efa285e8dc29a0cbadae0.png: image-format: PNG image-colors: 8 image-width: 128 image-height: 96 image-type: RGB, non-interlaced Thumb::URI: file:///home/claudio/fotos/2007-02-24-santiago/dscn5888.jpg Thumb::MTime: 1172788630 Software: GNOME::ThumbnailFactory
Created attachment 84208 [details] [review] obtains metadata correctly and propagate it to the final pixbuf There were two main reasons why previous approach couldn't work. - The dimensions which were intended to be saved as metadata are the dimensions of the created thumbnail, not the dimensions from the original image. The factory has no access to that information, therefore, the helper functions which create the thumbnails are the ones to attach the metadata to the Pixbuf generated. - When the obtained thumbnail was bigger than the desired one, in the scaling process the metadata was not passed to the pixbuf at the final size. Attached patch fixes this issues. The only thing I am not 100% sure of is if I'm supposed to use gdk_pixbuf_set_option ().
When using fixed buffers like that, at least use snprintf instead of sprintf. Also, we don't want all uses of the general pixbuf reading to set tEXt::Thumb::Image::Width, as that will be written out when saved (and the loaded pixbuf might not be a thumbnail!). I think a better approach is the g_object_set_data approach currently used, only we have to move it into the loader functions. Call it 'gnome-original-width' or something. We can store it with GINT_TO_POINTER instead of as a string to avoid using unnecessary memory for apps that don't need that. (Of course, if the file generated by a thumbnailer has tEXt::Thumb::Image::Width set we should prefer that over gnome-original-width.
Created attachment 84321 [details] [review] improved patch Ok, this patch follows your advice. I only set the metadata when the thumbnail is finally generated. The loaders only use the intermediate "gnome-original-width/height" as you suggested.
If the thumbnailer writes the width/height and we have to scale it we still loose that in the last patch. Otherwise it looks ok.
Claudio, could you fix the remaining problems then?
(In reply to comment #18) > If the thumbnailer writes the width/height and we have to scale it we still > loose that in the last patch. Do you mean the properties "gnome-original-width/height"? or the metadata? If you mean the properties, that's true. But I don't think these are needed anywhere else. If you meant the metadata, this is set in the patch to the pixbuf resulting after scaling, so I don't see a problem.
Created attachment 84431 [details] [review] consider metadata added by external thumbnailer This patch tries to preserve the metadata that could be found in a thumbnail generated by a helper thumbnailer (which I had neglected). The "gnome-original-width/height" properties will only be used in the case we fallback to gdk-pixbuf.
Looks good to me (if you tested it)
It's tested. I'll take your it as "commit-after_freeze"(In reply to comment #22) > Looks good to me (if you tested it) I've already tested it, so I'll take this as a permission to commit. Marking as accepted-commit_after_freeze.
Committed to trunk (rev. 5317): 2007-03-16 Claudio Saavedra <csaavedra@alumnos.utalca.cl> * libgnomeui/gnome-thumbnail-pixbuf-utils.c: (_gnome_thumbnail_load_scaled_jpeg): Set "gnome-original-width/height" properties in the generated pixbuf. * libgnomeui/gnome-vfs-util.c: (size_prepared_cb), (gnome_gdk_pixbuf_new_from_uri_at_scale): Set "gnome-original-width/height" properties in the generated pixbuf. * libgnomeui/gnome-thumbnail.c: (gnome_thumbnail_factory_generate_thumbnail), (gnome_thumbnail_factory_save_thumbnail): Set the "gnome-original-width/height" properties as metadata in the generated thumbnail. Preserve such info if the helper thumbnailer already provided it. Correctly set tEXt::Image::Width and tEXt::Image::Height metadata tags in thumbnails. Closes bug #143470 and bug #356930.