After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 439567 - Add functions to transform pixbufs based on orientation tags
Add functions to transform pixbufs based on orientation tags
Status: RESOLVED FIXED
Product: gdk-pixbuf
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: gtk-bugs
gtk-bugs
Depends on:
Blocks: 440978
 
 
Reported: 2007-05-19 01:37 UTC by Michael Chudobiak
Modified: 2010-07-10 04:06 UTC
See Also:
GNOME target: ---
GNOME version: 2.19/2.20


Attachments
Patch to add gdk_pixbuf_transform_orientation and gdk_pixbuf_transform_orientation_embedded (7.22 KB, patch)
2007-05-19 01:38 UTC, Michael Chudobiak
none Details | Review
Patch to add gdk_pixbuf_transform_orientation and gdk_pixbuf_transform_orientation_auto (7.06 KB, patch)
2007-05-20 01:41 UTC, Michael Chudobiak
none Details | Review
patch to add transform functions (6.95 KB, patch)
2007-05-25 15:23 UTC, Michael Chudobiak
none Details | Review
added updates to $MODULE.symbols and $MODULE-sections.txt (7.89 KB, patch)
2007-06-08 12:36 UTC, Michael Chudobiak
none Details | Review
New transform functions (8.43 KB, patch)
2007-06-08 19:32 UTC, Michael Chudobiak
needs-work Details | Review
Simplified patch (6.20 KB, patch)
2007-07-02 10:34 UTC, Michael Chudobiak
none Details | Review
Minor correction in function description. (6.16 KB, patch)
2007-07-02 10:37 UTC, Michael Chudobiak
committed Details | Review
Crash-causing tif (148.45 KB, image/tiff)
2007-07-02 20:33 UTC, Michael Chudobiak
  Details
backtrace (17.71 KB, text/plain)
2007-07-02 20:34 UTC, Michael Chudobiak
  Details

Description Michael Chudobiak 2007-05-19 01:37:32 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
Comment 1 Michael Chudobiak 2007-05-19 01:38:55 UTC
Created attachment 88429 [details] [review]
Patch to add gdk_pixbuf_transform_orientation and gdk_pixbuf_transform_orientation_embedded
Comment 2 Michael Chudobiak 2007-05-19 01:45:01 UTC
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
Comment 3 Jef Driesen 2007-05-19 21:11:35 UTC
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.
Comment 4 Michael Chudobiak 2007-05-20 01:41:58 UTC
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
Comment 5 Michael Chudobiak 2007-05-24 16:52:09 UTC
A patch to fix Nautilus thumbnailing based on these new functions is available in bug 440978.

- Mike
Comment 6 Michael Chudobiak 2007-05-25 15:23:52 UTC
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
Comment 7 Michael Chudobiak 2007-05-29 14:19:49 UTC
Could someone review/approve/reject this? It blocks bug 440978. I have commit access...

- Mike
Comment 8 Michael Chudobiak 2007-06-06 15:49:26 UTC
I should mention that gdk_pixbuf_transform_orientation and the GtkTransform enum are basically copied from gThumb, so they aren't new code...

- Mike
Comment 9 Michael Chudobiak 2007-06-08 12:36:18 UTC
Created attachment 89601 [details] [review]
added updates to $MODULE.symbols and $MODULE-sections.txt
Comment 10 Michael Chudobiak 2007-06-08 17:15:30 UTC
I have one (last?) version of the patch coming, to tidy up the docs.

- Mike
Comment 11 Michael Chudobiak 2007-06-08 19:32:33 UTC
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
Comment 12 Matthias Clasen 2007-06-29 18:01:08 UTC
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
Comment 13 Michael Chudobiak 2007-06-29 18:44:08 UTC
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
Comment 14 Michael Chudobiak 2007-07-02 10:34:42 UTC
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
Comment 15 Michael Chudobiak 2007-07-02 10:37:52 UTC
Created attachment 91016 [details] [review]
Minor correction in function description.
Comment 16 Matthias Clasen 2007-07-02 14:49:16 UTC
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)
Comment 17 Michael Chudobiak 2007-07-02 20:33:39 UTC
Created attachment 91056 [details]
Crash-causing tif
Comment 18 Michael Chudobiak 2007-07-02 20:34:09 UTC
Created attachment 91057 [details]
backtrace
Comment 19 Michael Chudobiak 2007-07-03 12:28:47 UTC
The tif crash has been reported as a separate bug, bug 453365.

Closing this bug as fixed.

- Mike