Related reports: 

Bug 421349 – Image orientation wrong in thumbnails
Bug 168231 – want EXIF rotation information via gdk_pixbuf_new_from_file
Bug 335053 – image thumbnailing to respect rotation EXIF flag

Is this a real world problem or an academic possible problem? Lets just break it down a bit so I understand what use case we got:

A) picture taken with digital camera supporting the EXIF orientation tag turned 90  degrees clockwise (portrait) 

B) picture taken with digital camera NOT supporting the EXIF orientation tag turned 90  degrees clockwise (portrait) 

In both cases Nautilus ignores the EXIF orientation tag, right? 

*IF* Nautilus did respect the EXIF orientation tag it would:

1) display picture A) above correctly but B) would still be displayed landscape. No changes are made to the original picture in either case.

2) generate thumbnails, portrait orientation for A) and landscape for B), right?

Now, since Nautilus does not respect the EXIF orientation tag, the enduser would either 

X) Rotate the images using Nautilus and everything would be great since it would automatically regenerate the thumbnail I guess

Y) Rotate the images using ghtumb which also should update the thumbnail, and everything should be great too.

So the only problem I see is when another application rotates the images without updating the thumbnail really, or do I miss something?

My two cents of thoughts:
In general I would prefer a don't touch approach where the image display rotates the image at display time rather then rotating it physically. This can be done by having an option to adjust/insert the EXIF orientation tag during the rotation phase instead of actually rotate the image, but again this would probably cause more problems with the Makernotes unless it is added after that section. Thumbnail should always respect the orientation tag though when generated so Nautilus and other non aware application can use them. 

I don't have any test images with this tag in them so I may have misunderstood the whole issue here.

// Joakim

Joakim,

This is a real-world problem for those who browse folders using Nautilus. See bug 421349 for an example of one user's experiences.

Regarding your use-case comments:

1. If the exif orientation is "top left" (the normal orientation), or if the image has no exif orientation tag, everything is fine. No problem.

2. If the exif orientation tag is present, and it is something other than top-left, the thumbnail generated by Nautilus will be wrong. It will look different than the image displayed in gThumb or EOG or any other modern viewer. THAT is the basic problem. Since gThumb generates thumbnails correctly, we should try to overwrite Nautilus-generated thumbnails whenever possible (or let the user manually refresh the thumbnails using gThumb).

So the basic question is: how can we detect and overwrite bad Nautilus thumbnails without killing performance? I've outlined 3 possible approaches above.

This bug isn't about rotating images (or anything requiring modifications to the original image file) - those issues were dealt with in bug 343867. I think you've misunderstood something in your X and Y comments - Nautilus isn't used for rotating images. This is just about making the thumbnails look like they should.

This problem would go away if Nautilus were fixed, of course.

(Jef also commented on this in eog bug 390266. All of my comments assume that we use his "#2" approach - store autorotated thumbnails.)

- Mike

Joakim,

Do you think it is practical to write an exif mini-parser that just extracts orientation tags from jpg images, without using libexif? Then we could fix Nautilus directly.

- Mike

Mike,

I think I get it, gthumb displays the image rotated correctly in display time when there is a tag, I just checked that it is the case.

given the same constraints as in the minimal_write function, ie that  a TIFF header is only found in the first APP1 section in the jpeg, it would be easy to retrieve the orientation tag. Fixing Nautilus is one way to do it.

Another possible solution would be to check the tag in gthumb and maybe also, if the orientation tag is not present or set to top-left,  verify that both the image and the thumbnail has the same orientation, ie width > height or vice versa. If they differ we just regenerate the thumbnail.

I look into making a generic minimal_read function for short values to be used in Nautilus or gthumb or both.

// Joakim

(In reply to comment #4)
> orientation tags from jpg images, without using libexif? Then we could fix
> Nautilus directly.

Or, more specifically: gnome_thumbnail_factory_generate_thumbnail needs to be fixed.

> I look into making a generic minimal_read function for short values to be used
in Nautilus or gthumb or both.

That would be great!

- Mike

Code to read orientation tags:

http://sylvana.net/jpegcrop/jpegexiforient.c

We need to correct libgnomeui/gnome-thumbnail-pixbuf-utils.c:_gnome_thumbnail_load_scaled_jpeg I think.

- Mike

Michael,

Fixing a problem of nautilus in gthumb is not the right approach and would lead to unnecessary complexity in gthumb. It should be fixed in nautilus. So I'm leaning towards closing this bug. Do you agree?

Jaap

Created attachment 86314 [details] [review]
metadata_ideas patch to allow reading of EXIF tags

Mike,

I am almost done with minor change to gth_exif_utils.c to add a minimal_exif_read function. The write part works as before but the patch is not ready yet, it needs more testing and adjustments of error checking. It is only uploaded as a reference for discussion.

It adds the reversed functionality of the minimal_write function but is identical in behaviour except for direction of copy and that it returns only the first tag value it finds.

// Joakim

Created attachment 86320 [details] [review]
metadata_ideas patch to allow reading of EXIF tags   	 

So this patch gives you a generic and minimalistic reader for EXIF tags. I tested it for orientation and Date tags and it seems to work. Maybe you want to use it to sort on EXIF date too..? These functions can be used to fix Nautilus too ofcourse. 

// Joakim

Cool patch, Joakim! I've committed it to metadata-ideas, and I updated read_orientation_field to use gth_minimal_exif_tag_read. I'll have to do the same for the date sorting code, as you mentioned.

- Mike

(In reply to comment #8)
> Fixing a problem of nautilus in gthumb is not the right approach and would lead
> to unnecessary complexity in gthumb. It should be fixed in nautilus. So I'm
> leaning towards closing this bug. Do you agree?

Jaap,

Well, let's keep this bug open for a while, because several intertwined issues are being dealt with.

However, you are right - fixing Nautilus should be the priority, and I think we have the basic building blocks to do it. Here's my outline:

1. We need to add fast orientation-tag-reader to libgnomeui/gnome-thumbnail-pixbuf-utils.c, using a simplified version of Joakim's code.

2. We need to cut-and-paste gThumb's 
   libgthumb/pixbuf-utils.c:_gdk_pixbuf_transform
   libgthumb/pixbuf-utils.c:_gdk_pixbuf_copy_mirror
   libgthumb/pixbuf-utils.c:_gdk_pixbuf_copy_rotate_90

functions into libgnomeui/gnome-thumbnail-pixbuf-utils.c

3. Similar to libgthumb/image-loader.c:image_loader_sync_pixbuf, we need to modify libgnomeui/gnome-thumbnail-pixbuf-utils.c:_gnome_thumbnail_load_scaled_jpeg to use the tag reader and _gdk_pixbuf_transform.


Steps 2 and 3 are easy. Step 1 is a little more tricky, because _gnome_thumbnail_load_scaled_jpeg works on VFS URIs, rather than simple local files. Joakim, do you think you can make a tag reader that works with the _gnome_thumbnail_load_scaled_jpeg?

- Mike

Mike, 

I doubt I will have a lot of time during this weekend, the spring arrived here and  I need to fix a few outdoorthings, but I can for sure take a look at it later next week or so. 

I really don't know how much you can simplify the code I wrote unless you want to remove the error checking... The additional code to be generic is not that much extra actually and given you need to read and/or write in more than one place the total sum of code/complexity/maintenenance will exceed these functions I think. 

I would suggest just a wrapper function for the minimal_read function to replace the read_orientation_field() function, and move it to the Nautilus project. Regarding the VFS URI stuff I don't really know what it is and/or what todo about it. Let me know what you have in mind here.

// Joakim

(In reply to comment #13)
> I doubt I will have a lot of time during this weekend, the spring arrived here
> and I need to fix a few outdoorthings

Same here... and I have to file tax returns too...


> I really don't know how much you can simplify the code I wrote unless you want
> to remove the error checking...

I just meant that Nautilus doesn't need the writing code, just the reading code. It's needs are simpler than gThumb's needs.


> I would suggest just a wrapper function for the minimal_read function to
> replace the read_orientation_field() function, and move it to the Nautilus
> project. 

I don't want to add public functions to Nautilus (which other applications, like gThumb, can share). That gets too complicated (for me, anyway - I have no experience with that). Let's just do the minimum coding required to fix Nautilus for now.


> Regarding the VFS URI stuff I don't really know what it is and/or what
> todo about it. Let me know what you have in mind here.

Basically it just means that you can't use fopen and fclose. You need to use gnome_vfs_open and gnome_vfs_close (see the _gnome_thumbnail_load_scaled_jpeg code for example). That's not a big deal. I was wondering if the tag-reading code can fit into the existing file-reads, or if we have to open/close the file two times (i.e., read tag, then read full jpeg data), which seems wasteful. You're much more familiar with the jpeg parsing code than I am, so maybe it's more obvious to you...

- Mike

(In reply to comment #14)
> (In reply to comment #13)
> > I really don't know how much you can simplify the code I wrote unless you want
> > to remove the error checking...
> I just meant that Nautilus doesn't need the writing code, just the reading
> code. It's needs are simpler than gThumb's needs.
> <snip>
> That gets too complicated (for me, anyway - I have no
> experience with that). Let's just do the minimum coding required to fix
> Nautilus for now.

ok, but in the moment people start to want some substantial maintenance/enhencements for these pieces of code in both Nautilus and Gthumb I would prefer to make a shared library of it rather than divert the implementation into two variants of the same problem domain. It would be against my religion, or something. ;-) 

I too don't know how to do make a library in a Gnome global context yet, maybe it would be better then if I break the code out and made a standalone library of it at some point of time so it can be shared by more projects. We'll see.

> > Regarding the VFS URI stuff I don't really know what it is and/or what
> > todo about it. Let me know what you have in mind here.
> 
> Basically it just means that you can't use fopen and fclose. You need to use
> gnome_vfs_open and gnome_vfs_close (see the _gnome_thumbnail_load_scaled_jpeg
> code for example). That's not a big deal. I was wondering if the tag-reading
> code can fit into the existing file-reads, or if we have to open/close the file
> two times (i.e., read tag, then read full jpeg data), which seems wasteful.
> You're much more familiar with the jpeg parsing code than I am, so maybe it's
> more obvious to you...

I'll see if I can fit the code in somewhere using an open file descriptor rather than reopen the file again. It sounds doable to break the functionality into two call levels like filename vs databuffer  

Well now I need to do some more painting.... :-)

// Joakim

> ok, but in the moment people start to want some substantial
> maintenance/enhencements for these pieces of code in both Nautilus and Gthumb I
> would prefer to make a shared library of it rather than divert the
> implementation into two variants of the same problem domain. It would be
> against my religion, or something. ;-) 

Hmm. Maybe we should think about this more.

We should probably fix the jpeg loaders for gdk_pixbuf_new_from_file and gdk_pixbuf_new_from_file_at_scale... and then patch gThumb and Nautilus to use them.

- Mike

OK, I think I've figured it out, after reading various related bug reports...

We need to:

1) Modify the gdk_pixbuf jpeg loader to read the orientation tag using some variant of Joakim's minimal tag reader code.

2) Modify the jpeg loader to call gdk_pixbuf_set_option, setting a value (1-8) for a rotation key, as a way of providing the rotation info to the calling application.

3) Modify gThumb to check for this rotation key, and use it if present.

4) Modify libgnomeui/gnome-thumbnail-pixbuf-utils.c to do the same.


The nice thing is that this adds the functionality incrementally, without breaking any existing code.


Some code re-use observations:

a) libgnomeui/gnome-thumbnail-pixbuf-utils.c has two methods to read jpegs:
     _gnome_thumbnail_load_scaled_jpeg
     gnome_gdk_pixbuf_new_from_uri_at_scale

I'm not sure why this is. I just removed gThumb's "custom" jpeg loader code, because gdk_pixbuf_new_from_file_at_scale is just as fast. I think gnome-thumbnail-pixbuf-utils.c should be modified to simply use gdk_pixbuf_new_from_file_at_scale.

b) After reading the rotation tag, gThumb calls libgthumb/pixbuf-utils.c:_gdk_pixbuf_copy_mirror and _gdk_pixbuf_copy_rotate_90. we should probably be using gdk_pixbuf_rotate_simple and gdk_pixbuf_flip, unless I'm missing something. I will investigate further.


- Mike

(In reply to comment #17)
> b) After reading the rotation tag, gThumb calls
> libgthumb/pixbuf-utils.c:_gdk_pixbuf_copy_mirror and
> _gdk_pixbuf_copy_rotate_90. we should probably be using
> gdk_pixbuf_rotate_simple and gdk_pixbuf_flip, unless I'm missing something. I
> will investigate further.

I've committed a change so that we use gdk_pixbuf_rotate_simple and gdk_pixbuf_flip now. Seems to work.

- Mike

Mike,

could you point me in some directions where I can find the source you want me to work on. Where is this gdk_pixbuf source and to what version should I create diffs to? 

// Joakim

gdk_pixbuf_new_from_file_at_scale and gdk_pixbuf_new_from_file are here:

http://svn.gnome.org/viewcvs/gtk+/trunk/gdk-pixbuf/gdk-pixbuf-io.c?view=markup

To load jpeg files, they call the functions in the jpeg loader (which is what we need to modify):

http://svn.gnome.org/viewcvs/gtk+/trunk/gdk-pixbuf/io-jpeg.c?view=markup

I'm not sure where gdk_pixbuf_set_option is; I haven't looked (we don't need to change it - just use it). The API is here of course: http://developer.gnome.org/doc/API/2.0/gdk-pixbuf/index.html

Just so you know, I haven't worked on gtk code before and someone else would probably have to do the committing. This is new territory for me.


- Mike

You'll probably also would like to look at bug 168231
Over there addition to gtk+ is discussed

The GTK+ trunk gives me some ./configure problems:

checking for GLIB - version >= 2.13.1... 
*** 'pkg-config --modversion glib-2.0' returned 2.13.1, but GLIB (2.12.4)

I did compile GLIB trunk and installed it (and Pango) as requested by the GTK+ ./configure script but the installation did not overcome the "but" statement above. Any idea what that means and how to fix it?

// Joakim

It's picking up the glib version you have installed from your distro.

So you probably need to change PKG_CONFIG_PATH (don't know by heart if that is exactly the name of the variable, but it's something like that) environment variable to find the .pc variable of you glib


What I personally always do is always build gnome from CVS with jhbuild [1]. I guess in your guess that's also the easiest because you want to check if nautilus works correctly after your patch

[1] http://live.gnome.org/Jhbuild

After learnt to know subversion I try to avoid CVS that has caused me endless of merging troubles in the past ;-) This did the trick:

 export PKG_CONFIG_PATH=/home/joakim/work/glib/trunk/
 ./configure

Thanks for the tip Jaap

// Joakim

Ok, so I have assimilated with some of the jpeg loader code. Below is psuedo code patch, not compilable code just yet, describing what I would like to do:

a) Place the gth_minimal_exif routines into a shared library (libmexif maybe?)
b) Change prefix from "gth_minimal_exif" to "mexif" 
c) Add a sublevel to the API taking a file descriptor instead of a name as argument, using suffix _fd 
d) Make the changes to io-jpeg.c as described below
e) Change gth_exif_utils.c to use mexif_ calls 

This would give us (me) the advantage of less code to maintain and also beeing able to expand on the functionality without the burden of updating code in many places. Let me know what you think // Joakim


--- io-jpeg.c   (revision 17605)
+++ io-jpeg.c   (working copy)
@@ -312,6 +312,9 @@
                return NULL;
        }
 
+       /* check for orientation tag */
+       is_otag = mexif_tag_read_fd(f,  EXIF_TAG_ORIENTATION, &otag, sizeof(otag), 0);
+
        /* load header, setup */
        jpeg_create_decompress (&cinfo);
 
@@ -357,6 +360,13 @@
                return NULL;
        }
 
+       /* if orientation tag was found set an option to remember its value */
+       if (is_otag == MEXIF_OK) {
+               sprintf(otag_str, "%d", otag);
+               gdk_pixbuf_set_option(pixbuf, "orientation", otag_str);
+       }
+
+
        dptr = pixbuf->pixels;
 
        /* decompress all the lines, a few at a time */
@@ -951,6 +961,8 @@
 
                                        return FALSE;
                                }
+                      } else if (strcmp (*kiter, "orientation") == 0) {
+                              /* Do stuff with orientation */
                        } else {
                                g_warning ("Bad option name '%s' passed to JPEG saver",
                                           *kiter);

(In reply to comment #25)
> a) Place the gth_minimal_exif routines into a shared library (libmexif maybe?)
> 
> This would give us (me) the advantage of less code to maintain and also beeing
> able to expand on the functionality without the burden of updating code in many
> places. Let me know what you think // Joakim


Joakim,

gdk-pixbuf is low-level code that is used everywhere in gnome, so I think it might be a better idea to not add an external library dependency. I think it would be better to keep the orientation tag reader code inside gdk-pixbuf (with no writer code), and keep it as small and clean as possible. 

This reduces the chance of security problems, I think. See http://bugzilla.gnome.org/show_bug.cgi?id=168231#c3 - similar objections were made about linking to libexif.

- Mike

While I see your point I am not sure where to put a JPEG specific routine like this in the generic part of gdk-pixbuf code. If I put it in the io-jpeg.c it seems to me that we may have problems access it from gthumb or we have to break the module policy that seems to exist. A lot of guesswork here so please correct me.

The gdk-pixbuf loader modules API could perhaps add a meta_data function entry so that these kind of functions could be hosted for several formats, like PNG chunks and similar.

My last argument for not stripping the code is that the code will not decrease a lot and the algorithm will be the same so it will be equally insecure, if it is.

But if you just point me to a file where the function can be accessable throughout gnome I will put the stripped down version there, no problem. Let me know what naming convention you want me to use too for this routine.

Other than that, did I find the right spot to add the functionality you were looking for? 

// Joakim

Joakim,

I meant that io-jpeg.c would have a minimal tag reader to extract the orientation tag, and gThumb would have a separate (partly duplicated) reader/writer. That keeps io-jpeg.c reasonably secure, and we don't have to figure out all the code-sharing issues, which are non-trivial :-)

However, if you really, really want to share code, then I'm not quite sure where it should go. io-jpeg.c would make sense, but I don't know if there is a policy against that. Maybe gdk-pixbuf-io.c? 

So I'm not sure... maybe ask in bug 168231, and see if Matthias Clasen can suggest a suitable approach.

Regarding the pseudo-code: The read part looks good to me.

I'm not sure I see the point of writing the orientation tag, however. If you save a pixbuf, presumably the pixbuf will be oriented in a "top-left" fashion already. I think that part should be left out, at least for now.

- Mike

Mike,

I am on it, and will remove the write part alltogether and put the remains in the io-jpeg.c 

In the future I may put the EXIF stuff we make here into an external libmexif project just to support non-gnome projects too, and also to learn how to make a GNU project. That could be good fun.

// Joakim

 

Created attachment 86605 [details] [review]
GTK+ trunk patch to make jpeg loader set orientation option in pixbuff 

It compiles but I don't have any code to test it with. // Joakim

Thanks, Joakim. I'll try to do serious testing this weekend.

- Mike

Joakim,

I patched the metadata-ideas branch so that it checks for a "rotation" option key before trying a libexif read, so that you can test your io-jpeg changes. Please try it.

See the end of libgthumb/file-utils.c:gth_pixbuf_new_from_uri for details.

I haven't tried compiling io-jpeg yet...

- Mike

Joakim,

I think the patch needs to return 0 if readsize < 32, to avoid invalid pointers in "while (i < 32)".

- Mike

Created attachment 86667 [details] [review]
GTK+ trunk patch to make jpeg loader set orientation option in pixbuff

Thanks Mike, you are even more paranoic than I am ;-) I also added a repositioning of the file pointer in the case subsequent code assumes it is pointing somewhere in particular, like at the start of the file.

// Joakim

(In reply to comment #34)
> Thanks Mike, you are even more paranoic than I am ;-) I also added a
> repositioning of the file pointer in the case subsequent code assumes it is
> pointing somewhere in particular, like at the start of the file.

If we aren't paranoid, it will come back to haunt us pretty quickly in the form of hard-to-understand bug reports... :-(

Can your patch handle an improper JFIF APP0 marker between SOI and the Exif APP1 marker? If it finds an APP0 marker, it might need more than 32 bytes to find the APP1 marker, right? (My knowledge is a bit shaky here.)

We have to accommodate that situation, because gThumb was writing both until Jef fixed it (bug 411861).

- Mike

Joakim,

I think we should actually move the get_orientation function into gdk-pixbuf-io.c, and then we could call it from both io-jpeg.c AND io-tiff.c.

tiff files have the same orientation issues as jpeg-exif files (bug 158538), of course.


- Mike

I've updated the gthumb metadata-ideas branch so that it checks for a "rotation" option key for all file types, not just jpeg.

- Mike

(In reply to comment #35)
> (In reply to comment #34)
> > Thanks Mike, you are even more paranoic than I am ;-) I also added a
> > repositioning of the file pointer in the case subsequent code assumes it is
> > pointing somewhere in particular, like at the start of the file.
> 
> If we aren't paranoid, it will come back to haunt us pretty quickly in the form
> of hard-to-understand bug reports... :-(

Yes, I meant it as something good. The code has error checks below that part that should take care of such a situation and since we are only reading I think this function would not trigger an expoitable exception even if fed with specially crafted data, but the explicit test as you suggested is of course better. Also we need to fix that part since TIFF:s put their IFD:s anywhere in the image and from what I have seen often after the compressed data.

> 
> Can your patch handle an improper JFIF APP0 marker between SOI and the Exif
> APP1 marker? If it finds an APP0 marker, it might need more than 32 bytes to
> find the APP1 marker, right? (My knowledge is a bit shaky here.)

As it is written now it handles only correctly formatted EXIF JPEG:s but it is not that hard to add support for disregard ghost APP0 markers.

> 
> We have to accommodate that situation, because gThumb was writing both until
> Jef fixed it (bug 411861).

Ok I take a look at it.

// Joakim

(In reply to comment #36)

> I think we should actually move the get_orientation function into
> gdk-pixbuf-io.c, and then we could call it from both io-jpeg.c AND io-tiff.c.
> 
> tiff files have the same orientation issues as jpeg-exif files (bug 158538), of
> course.

It will require some more work since TIFF:s are more free format than EXIF:s, but I've already looked into supporting TIFF:s so I think it is possible to add such support. If I manage to do that I move it to gdk-pixbuf-io.c

// Joakim

(In reply to comment #39)
> It will require some more work since TIFF:s are more free format than EXIF:s,
> but I've already looked into supporting TIFF:s so I think it is possible to add
> such support. If I manage to do that I move it to gdk-pixbuf-io.c

Oh, I thought tiffs were a simpler case. Oops. If it is too complicated, just forget it. tiffs are low priority, and if the code is much more complicated it might be better to have a separate specialized tag reader in io-tiff.c. Whatever you think...

- Mike

Joakim,

I've installed jhbuild (which uses svn, by the way) to test your patches.

I'm having one problem with the patch: the "orientation" key is reported properly by the jpeg loader if the file is loaded using gdk_pixbuf_new_from_file, but not if it is loaded using gdk_pixbuf_new_from_file_at_scale.

In other words, it doesn't work when I'm trying to generate scaled thumbnails.

I don't understand how scaled image loading works right now, so I don't know what the problem is. However, it seems obvious that a call to get_orientation is missing somewhere in io-jpeg.c....

- Mike

Created attachment 86790 [details] [review]
Updated patch

Here is an updated patch, where get_orientation works on the jpeglib-generated data buffer, rather than a file buffer. (Clearly generality for tiff files is impossible now.)

Scaling mode (thumbnails) still don't work, because the loader code goes pixbuf > animation > pixbuf, losing the pixbuf options along the way. Silly. Anyway, hopefully that can be fixed.

- Mike

Created attachment 86792 [details] [review]
Working patch!

OK, this patch works for full-sized and scaled jpegs. Hurray! It can be tested by applying this patch against gtk+ svn (jhbuild), and then running the metadata-ideas branch of gthumb.

Joakim - does "EXIF_IDENT_STRING" need to be checked both endian ways?

I added a "pixbuf_options" hash to the loader priv struct, to preserver pixbuf options even after being converted to an animation (and back again).

- Mike

> Joakim - does "EXIF_IDENT_STRING" need to be checked both endian ways?

To answer my own question: no.

Also, I should mention that the JFIF issue is gone now, thanks to the use of jpeglib.

- Mike

Mike,

cool work using the libjpeg structures! A pitty we can't use it for TIFFs though.
 
joakim@montecristo:~/work$ strings ~/Desktop/Nikonpicture.jpg | fgrep Exif
EExif
joakim@montecristo:~/work$ strings ~/Desktop/Minoltapicture.JPG | fgrep Exif
Exif

// Joakim

 

Created attachment 86824 [details] [review]
GTK+ patch to make tiff loader set orientation option in pixbuf

(In reply to comment #40)
> (In reply to comment #39)
> > It will require some more work since TIFF:s are more free format than EXIF:s,
> > but I've already looked into supporting TIFF:s so I think it is possible to add
> > such support. If I manage to do that I move it to gdk-pixbuf-io.c
> 
> Oh, I thought tiffs were a simpler case. Oops. If it is too complicated, just
> forget it. tiffs are low priority, and if the code is much more complicated it
> might be better to have a separate specialized tag reader in io-tiff.c.
> Whatever you think...

I think you are wrong about the complexity for tiffs. libtiff has built-in support for reading tags (hence the name "Tagged Image File Format"). Obtaining the value of the orientation tag is very simple. I didn't test my patch (except for making sure it does compile), but I think it should work. And if it doesn't, it can be the start for something better...

> I think you are wrong about the complexity for tiffs. libtiff has built-in
> support for reading tags (hence the name "Tagged Image File Format"). Obtaining
> the value of the orientation tag is very simple. I didn't test my patch (except
> for making sure it does compile), but I think it should work. And if it
> doesn't, it can be the start for something better...

Jef, 

I started testing the existing functionality of the tiff loader, so we could slip in your patch. As always, things are more complicated than they seem. Sigh.

Bizarrely, the existing tiff loader corrects for for orientation tag values 1-4, but values 5-7 are interpreted as 1-4. The tiff loader will do flips and mirrors, but not rotates! So tiff loading is totally mangled...

I haven't thought about what should be done about it, yet.


- Mike

*** Bug 168231 has been marked as a duplicate of this bug. ***

(In reply to comment #46)
> for making sure it does compile), but I think it should work. And if it
> doesn't, it can be the start for something better...

The incremental loader probably has to be changed also (gdk_pixbuf__tiff_image_load_increment). 

- Mike

(In reply to comment #46)
> Created an attachment (id=86824) [edit]
> GTK+ patch to make tiff loader set orientation option in pixbuf
> 
> (In reply to comment #40)
> > (In reply to comment #39)
> > > It will require some more work since TIFF:s are more free format than EXIF:s,
> > > but I've already looked into supporting TIFF:s so I think it is possible to add
> > > such support. If I manage to do that I move it to gdk-pixbuf-io.c
> > 
> > Oh, I thought tiffs were a simpler case. Oops. If it is too complicated, just
> > forget it. tiffs are low priority, and if the code is much more complicated it
> > might be better to have a separate specialized tag reader in io-tiff.c.
> > Whatever you think...
> 
> I think you are wrong about the complexity for tiffs. libtiff has built-in
> support for reading tags (hence the name "Tagged Image File Format"). Obtaining
> the value of the orientation tag is very simple. I didn't test my patch (except
> for making sure it does compile), but I think it should work. And if it
> doesn't, it can be the start for something better...
> 

I didn't mean that the TIFF format was more complex but the code would to support it, e.g. the IFDs are often located last in the file causing another read of data for each file. 

// Joakim

Created attachment 86903 [details] [review]
GTK+ patch to make tiff loader set orientation option in pixbuf

(In reply to comment #47)
> Bizarrely, the existing tiff loader corrects for for orientation tag values
> 1-4, but values 5-7 are interpreted as 1-4. The tiff loader will do flips and
> mirrors, but not rotates! So tiff loading is totally mangled...

This might be related to the usage of TIFFReadRGBAImageOriented. According to the documentation [1] it supports only the first four orientation values. A possible workaround could be using TIFFReadRGBAImage instead or not setting the orientation option for the values 1-4. But it might be more complex.

[1] http://www.remotesensing.org/libtiff/man/TIFFReadRGBAImage.3tiff.html

(In reply to comment #49)
> The incremental loader probably has to be changed also
> (gdk_pixbuf__tiff_image_load_increment). 

I'm not really sure how the incremental loader works, but I think it might work if we move the extra code from the patch to tiff_image_parse. That function is called from both gdk_pixbuf__tiff_image_load and gdk_pixbuf__tiff_image_stop_load. If I'm correct, that last function is used for finalizing an incremental load.

(In reply to comment #51)

> This might be related to the usage of TIFFReadRGBAImageOriented. According to
> the documentation [1] it supports only the first four orientation values. A
> possible workaround could be using TIFFReadRGBAImage instead or not setting the
> orientation option for the values 1-4. But it might be more complex.

No, TIFFReadRGBAImageOriented just allows you to specify a "starting" orientation (an offset). Using TIFFReadRGBAImage makes no difference, it still would still treat orientations 5-8 the same way as 1-4.

libtiff is broken, in other words, for 5-8. (It is obvious in the source code: libtiff/tif_getimage.c:setorientation).

We can not disable the libtiff auto-rotation for 1-4, so we need to extract the orientation and do extra processing for 5-8, so everything is treated consistently.

This implies that the jpeg loader should go ahead and do the required flips/rotations too, for consistency (which is not in the current patch). If that breaks someone's application, they can read the orientation tag and apply a reverse transform I guess. In the scaled mode, the returned image width and height will be swapped. Does anyone object to that?


> I'm not really sure how the incremental loader works, but I think it might work
> if we move the extra code from the patch to tiff_image_parse. That function is
> called from both gdk_pixbuf__tiff_image_load and
> gdk_pixbuf__tiff_image_stop_load. If I'm correct, that last function is used
> for finalizing an incremental load.

I'll check that out, when I get some time in the next week or two or three...

- Mike

The libtiff problems have been reported at:

http://bugzilla.remotesensing.org/show_bug.cgi?id=1548

I expect they will be resolved at the libtiff end. I suppose we can still add orientation tag reporting.

- Mike

Created attachment 87423 [details] [review]
Updated tiff/jpeg loader patch

Here is an updated patch.

It provides the orientation option value for jpegs (full-size and thumbnail) and tiff (full-size only). The tiff orientation is processed to account for the libtiff oddities, which are not going to be fixed in libtiff.

The patch does not work for the tiff incremental (i.e., thumbnail / scaled) loader yet. gdk_pixbuf__tiff_image_stop_load is not used to "finalize" an incremental load, exactly - it cleans up after a load, wiping out the pixbuf.

So that needs to be figured out yet.

The patch also simplifies the tiff loader by mandating the use of libtiff 3.6.0 or higher (which allows us to remove some ifdef's).

- Mike

Some initial comments after looking through the patch:

- Some coding style issues:
  * Please don't use C99 comments ( // )
  * foo(bar) should be foo (bar)

- man bcmp says: This function is deprecated; use memcmp() in new programs
 
- how does get_orientation() work ? I only see readsize being
  initialized to 0, and then later there is a while ((i < readsize - 2))
  loop. 

- nice comment explaining the tiff orientation oddness

(In reply to comment #55)
> Some initial comments after looking through the patch:
> 
> - Some coding style issues:
>   * Please don't use C99 comments ( // )
>   * foo(bar) should be foo (bar)

My fault, I try to adopt to the local coding standard where I can but some things I think it makes it harder to read. This is my first open source contributions so I guess I get used to the coding style over time.

> 
> - man bcmp says: This function is deprecated; use memcmp() in new programs

Yes I agree, some leftovers from old times.... 

> 
> - how does get_orientation() work ? I only see readsize being
>   initialized to 0, and then later there is a while ((i < readsize - 2))
>   loop. 

readsize isn't used anymore and can be removed, the function used to read from a file but Mike changed it to operate on in memory buffers. I guess that's why the  pacth says "needs more work"

> 
> - nice comment explaining the tiff orientation oddness
> 
Mikes comment, see the C99 commenting style ;-)

// Joakim

Created attachment 87454 [details] [review]
corrected patch - includeds configure.in changes, etc

Matthias,

Thanks for the comments; I'll get things cleaned up tonight I hope.

The previous patch file left out the changes to the parent directory (configure.in, README.in, etc). Oops. A new patch is attached correcting that. I still have to fix some other things.

- Mike

Created attachment 87456 [details] [review]
Fixed get_orientation according to comments made

Mike,

I had some time to spend on this so I fixed the get_orientation function as requested and also simplified it a but further and improved error checking. It is dry coded but compiles so it needs to be verified.

// Joakim

Joakim,

Please check / test your code again:

if (i + 2 < exif_marker->data_length)
and
if (i + tags * 12 < exif_marker->data_length)

are probably supposed to use ">", not "<", I'm pretty sure. It doesn't work otherwise. 

I haven't had tried to review it more thoroughly...


- Mike

Created attachment 87470 [details] [review]
Should-be-fully-functional patch

I found the tiff thumbnailing issue; this new patch appears to work for all tiff/jpeg orientation tags. Hurray!

I still need to review the recent changes in get_orientation fully... but it seems to work.

- Mike

Mike,

you are correct, thanks for spotting it. I just have to install the full gnome build environment so I can try out the GTK stuff myself, that will be the day... 

// Joakim

Created attachment 87560 [details] [review]
Final patch?

Here is a tidied-up version of the patch. I've added a pile of comments to the get_orientation code, because I'm a big fan of comments.

Anyway, the patch appears to work both in theory and in practice. I checked Joakim's new code and it looks good. It can be tested by using exiftool to set the orientation tag on a variety of tif and jpg files, and then using gThumb from the metadata-ideas branch (not trunk or gthumb-2-10) to view them.

Matthias, I guess we pass it over to you now... I haven't reviewed or committed to gtk before.

- Mike

The next step will be to patch the Nautilus thumbnailers, of course...

- Mike

*** Bug 434194 has been marked as a duplicate of this bug. ***

Created attachment 87868 [details] [review]
Updated patch with formatting corrections

This is a minor update of the previous patch.

It just fixes some foo(bar) > foo (bar) formatting.

- Mike

I don't understand why it is necessary to copy the pixbuf options in the pixbuf loader, and re-set them in gdk_pixbuf_loader_get_pixbuf(). It is still the same pixbuf, so that should not be necessary, right ?

Well, the loader provides a pixbuf which gets converted into an animation (in gdk_pixbuf_loader_prepare), and then gets converted back to a pixbuf (in gdk_pixbuf_loader_get_pixbuf).

The pixbuf->options were getting lost in this process; that's why I added a partial copy of the pixbuf options. I haven't looked at the underlying animation code to understand if this is expected behavior or not; I just assumed it was.

It is a little bit hackish that only the "orientation" option is being preserved. It's not clear to me if the options hash is exposed enough to copy it entirely.

If someone cleverer than me sees a more elegant way to do this, it would be most welcome...

- Mike

Actually, something does seem wrong about the copy-options business. It "shouldn't" need a copy. I'll try to figure it out...

- Mike

Yeah, it shouldn't be necessary. If there is something broken at a lower level, we should fix that.

OK, I figured it out: The pixbuf options are being lost in scaled mode when gdk-pixbuf-scaled-anim.c:get_scaled_pixbuf calls gdk_pixbuf_scale_simple.

gdk_pixbuf_scale_simple makes a new pixbuf without copying the options.

So... what's the correct solution? Preserve the option(s) in gdk_pixbuf_scale_simple, get_scaled_pixbuf, or gdk_pixbuf_loader_prepare/gdk_pixbuf_loader_get_pixbuf (the current patch)?

- Mike

Ah, thanks for tracking that down. 
Not quite sure yet whats the best place to fix this.
We only care about preserving options during loading, really.
And none of the other pixbuf operations pay any attention to preserving
options. 
But it is probably the easiest to copy the options in the scaled anim 
code.

Created attachment 87976 [details] [review]
Simplified patch

Matthias,

Here is the revised patch. The option-copying now occurs (much more compactly) in gdk-pixbuf-scaled-anim.c:get_scaled_pixbuf.

- Mike

Can I commit this?

- Mike

I'd rather copy all options if we copy any at all. Preserving just the orientation doesn't seem right to me.

OK, fair enough - but I need some guidance on how to do that...

Is there an easy way to copy options in bulk? (The public API option functions are pretty minimal.) Or do I need to make a new (public?) function that is a mash-up of gdk_pixbuf_get_option and gdk_pixbuf_set_option, which g_strdup's all of the keys and values individually, in a loop? Or something else?

- Mike

Something like this: 

        GQuark  quark;
        gchar **options;
 
        quark = g_quark_from_static_string ("gdk_pixbuf_options");
        options = g_object_get_qdata (G_OBJECT (orig), quark);

        if (options)
          g_object_set_qdata (G_OBJECT (copy), quark, g_strdupv (options));

Created attachment 88393 [details] [review]
orientation patch, preserving all options

Thanks, Matthias, for your patience and guidance!

The new patch preserves all pixbuf options during the scaled load, as requested. And it seems to work, too...

- Mike

Looks good, please commit to trunk if you have svn access.

I've committed it. Thanks!

- Mike

Created attachment 88412 [details] [review]
Patch to add gdk_pixbuf_transform_orientation

Matthias,

here is a follow-up patch to provide a utility function to auto-rotate the pixbuf based on the orientation option. Having this in gdk-pixbuf will eliminate pointless duplication in gThumb, Nautilus, eog, etc.

Let me know what you think...

- Mike

I can see the value of having such a function in gdk-pixbuf. 
Some comments: 


The docs need a "Since: 2.12" at the end.


I don't think the function should modify the orientation option
on the orignal, since it does not change the orientation of the original,
it returns a new pixbuf. Repeatedly calling the function on the same
image will work just fine, it will return two individual, correctly
oriented copies. Calling the function on the returned pixbuf will also
work correctly, since the new pixbuf returned by the first call will
not have an orientation tag on it.


You should check orientation_string for being NULL _before_ calling
g_ascii_strtoll on it.


I also think it would be good to have some documentation about the meaning
of the orientation option, so that people could do this themselves if they
wanted, or just to display the orientation with a meaningful name, maybe.
But I see that we have no documentation about any options right now...

Also, please open separate bugs for followups in the future. Makes it easier to track things.

Matthias,

I have opened a new bug report (bug 439567) and uploaded an improved patch there.

- Mike

I closed this bug as the patch got applied and subsequent work is going on in bug 439567

I think there is still one issue left in this bugreport that needs some more attention: the nautilus thumbnailers do not take into account the new orientation option.

I also created a follow-up bug 439994, to discuss the inconsistent behavior of TIFF loader.

(In reply to comment #85)
> I think there is still one issue left in this bugreport that needs some more
> attention: the nautilus thumbnailers do not take into account the new
> orientation option.

Yes, that can be a follow-up to bug 439567, I think, because any Nautilus patch will depend on that gtk patch.

- Mike

The follow-up bug report for Nautilus/libgnomeui is bug 440978.

- Mike