GNOME Bugzilla – Bug 439567
Add functions to transform pixbufs based on orientation tags
Last modified: 2010-07-10 04:06:45 UTC
This is a follow-up to bug 428725, where the ability to add tiff and jpeg/exif orientation tags was added to the pixbuf loaders. Next, it would be nice to have a gdk-pixbuf function that can transform the pixbufs based on the orientation tags that have been read. Patch to follow shortly. - Mike
Created attachment 88429 [details] [review] Patch to add gdk_pixbuf_transform_orientation and gdk_pixbuf_transform_orientation_embedded
The patch above adds two new functions: gdk_pixbuf_transform_orientation - transforms a pixbuf based on a user-supplied transform value. The is used to implement dk_pixbuf_transform_orientation_embedded, and is useful on its own to implement certain rotation functions (i.e., gThumb can use it for Tools>Rotate). gdk_pixbuf_transform_orientation_embedded - transforms a pixbuf based on the orientation option associated with it. Needed by most apps to complete the tif/jpeg loading process. The valid transforms now have a descriptive enum. The gdk_pixbuf_get_option documentation has been improved. - Mike
Do we really need GTK_TRANSFORM_NO_VALUE? In practice, it is always equivalent to GTK_TRANSFORM_NONE. The function can still silently treat all invalid values as GTK_TRANSFORM_NONE (like we already do now) without needin an extra constant. Also the name gdk_pixbuf_transform_orientation_embedded does not describe the function very well. When I read embedded, I'm expecting some kind of in-place modification of "embedded data". I think gdk_pixbuf_transform_orientation_auto is a better name for the function.
Created attachment 88466 [details] [review] Patch to add gdk_pixbuf_transform_orientation and gdk_pixbuf_transform_orientation_auto Hi Jef, > Do we really need GTK_TRANSFORM_NO_VALUE? I thought it would be nice to provide a explicit meaning for the zero value, but I really can't of a situation where it would be used, so I've dropped it. > modification of "embedded data". I think gdk_pixbuf_transform_orientation_auto > is a better name for the function. Yes, I like that better too. A revised patch is attached... - Mike
A patch to fix Nautilus thumbnailing based on these new functions is available in bug 440978. - Mike
Created attachment 88804 [details] [review] patch to add transform functions This revised patch just changes the assertions slightly, to check for null input pixbufs. - Mike
Could someone review/approve/reject this? It blocks bug 440978. I have commit access... - Mike
I should mention that gdk_pixbuf_transform_orientation and the GtkTransform enum are basically copied from gThumb, so they aren't new code... - Mike
Created attachment 89601 [details] [review] added updates to $MODULE.symbols and $MODULE-sections.txt
I have one (last?) version of the patch coming, to tidy up the docs. - Mike
Created attachment 89621 [details] [review] New transform functions This adds proper documentation for the GdkTransform enum. I think I have the gtk-doc stuff right now... - Mike
Sorry for taking long to review this. Here is my initial reaction: The gdk_pixbuf_transform_orientation_auto function is fine, even though I am not really sold on the name. How about something like gdk_pixbuf_apply_embedded_orientation ? Wrt. to the transform enum and the gdk_pixbuf_transform_orientation function, I am not really happy with how it duplicates existing api, in the form of GdkPixbufRotation and gdk_pixbuf_rotate_simple, gdk_pixbuf_flip
Matthias, Thanks for taking a look! 1) The "gdk_pixbuf_apply_embedded_orientation" name is fine by me. 2) The _embedded function is much more important than the non-embedded function, so I can strip out the latter if you want to minimize API changes / overlap. (I don't see how to preserve the non-embedded function without partial overlap of the existing API.) I will develop a revised patch. - Mike
Created attachment 91015 [details] [review] Simplified patch Here is a new patch, with just the one simpler, renamed function, as requested. However, there is a problem that I do not understand, and I need help with... The tiff loader crashes on my jhbuild system, unless I revert the patch from bug 403255. This happens with and without my patch installed, so it is not directly related to my patch. My patch comments-out gdk-pixbuf-loader.c:726 ("g_propagate_error (error, tmp);"), as a temporary hack. Commenting out that line makes the tiff loader work, with and without my patch. I'm not sure why I didn't have this problem a few weeks ago when working on the earlier patch. I seem to recall warnings, but no crashes. Matthias, do you see the problem? - Mike
Created attachment 91016 [details] [review] Minor correction in function description.
Thanks, committed without the workaround. Can you attach an image that causes the problem for you ? 2007-07-02 Matthias Clasen <mclasen@redhat.com> * gdk-pixbuf.c (gdk_pixbuf_get_option): Document the "orientation" option. * gdk-pixbuf.symbols: * gdk-pixbuf-core.h: * gdk-pixbuf-util.c (gdk_pixbuf_apply_embedded_orientation): New function to handle Exif orientation information in tiff and jpeg images. (#439567, Michael Chudobiak)
Created attachment 91056 [details] Crash-causing tif
Created attachment 91057 [details] backtrace
The tif crash has been reported as a separate bug, bug 453365. Closing this bug as fixed. - Mike