GNOME Bugzilla – Bug 343867
Rotating image should alter EXIF data
Last modified: 2007-04-12 09:15:52 UTC
Please describe the problem: When auto-rotating images during import, based on the EXIF data, gthumb does not update the "orientation" tag in the EXIF data. The correct behavior (I believe) is to update the EXIF data to be "top-left", after auto-rotation. I believe this is the correct behavior because (a) it is the behavior of other applications, such as exiftran; and (b) it seems that the EXIF spec states that the "Orientation" tag describes the layout of the image data (so it does not refer, for example, to the orientation of the camera when the picture was taken). Steps to reproduce: 1. Take a picture with a Canon Powershot G3 camera, oriented on its side. 2. Connect the camera to your gnome desktop and import, allowing autorotate 3. View EXIF data of the imported image. Actual results: You see "Orientation bottom-left" or something. Expected results: You should see "Orientation top-left". Does this happen every time? Yes. Other information: EXIF spec http://exif.org/Exif2-2.PDF clearly states the orientation tag reflects the layout of the image pixels in the file.
Very closely related to bug 120839; in fact, this particular issue is already mentioned in bug 120839 comment 7.
(In reply to comment #1) > Very closely related to bug 120839; in fact, this particular issue is already > mentioned in bug 120839 comment 7. Yes, indeed. Though as that comment points out, Bug #120839 is really a request for an enhancement. The report here in this bug is really describes incorrect behavior: perhaps it deserves its own bug.
This issue affects using the gthumb importer with a third-party app such as F-spot. It would also probably affect the implementation of 120839 (I think). Rotating images on import is useful, but the metadata should be updated to reflect this.
This used to work but appears to have broken in around 2.7.6 (at least that version is broken for me).
*** Bug 354379 has been marked as a duplicate of this bug. ***
Apparently, this issue was introduced with this bug: bug #318828 Reverting the patch should solve this issue. I'm speaking here as F-Spot contributor and this issue is very annoying for us since gthumb is the default gnome importer for most of the desktops. regards, Stephane
Sorry, I talked to fast. Reverting the patch will probably not solve this issue :(
I believe setting the exif orientation to top left whenever changing the data (at least on any rotation, including one explicitely asked by the user) would be the proper thing to do. That would at the very least fix interoperability problems with software that uses the exif data to properly display the picture. Actually, a possibly better solution would be to implement http://bugzilla.gnome.org/show_bug.cgi?id=120839 and http://bugzilla.gnome.org/show_bug.cgi?id=172427
Yes, setting it to top-left if you've correctly oriented the image-data is the proper thing to do. Otherwise, even importing an image more than once with gthumb will continue to rotate the image. The exif specification also lists several other fields you should update any time you've modified the image, modification time being a prime example.
Hello, I usually use Gthumb to rotate automaticaly photos using EXIF datas. Version 2.6.5 worked fine. Exif data was updated to top-left. Now i get version 2.7.6 (Ubuntu Dapper). Automatic rotation works, but Exif data is no more updated! So now each photo rotated with this new version has a bogus Exif rotaion tag data! I have done a build of breezy package 2.6.5 for dapper, and updating exif data roation works again. But this is a too bad work arround :( Lastest version on edgy (2.7.8) is like 2.7.6 . This is a critical bug from my point of view, because it's a regression, and it's hard to detected (i do not verify exif data on each rotation, i was pretty happy with this cool feature).
Javier, the bug was introduced in 2.6.9 by this: bug #318828 But, apparently, this topic is too sensible too be discussed by crack smokers (I tried, see the discussion on 318828) :)
Thanks Stephane, i'm going to bug #318828, to add my opinion ... I can't believe that this critical bug is a FEATURE !
Hi everyone! Reading bugs http://bugzilla.gnome.org/show_bug.cgi?id=318828, http://bugzilla.gnome.org/show_bug.cgi?id=155863 and this one, I see there is still no fix applied to gthumb regarding the corrupted behaviour of gthumb with exif orientation tag not being updated when doing lossless jpeg autorotation. I had to fix my images manually with exiv2 tool using command: for ii in *.jpg ; do exiv2 -M "set Exif.Image.Orientation Short 1" $ii; done (credit goes to Stephane Delcroix). I browsed the gthumb source code and if I'm correct a simple fix to this problem would be to revert the change in rotation-utils.c that happened between versions 1.1 and 1.2. See: http://cvs.gnome.org/viewcvs/gthumb/src/rotation-utils.c?r1=1.1&r2=1.2 Could this be applied ASAP? I can't use f-spot and gthumb together because of this issue :(
This is a confusing multi-faceted issue, that needs a proper plan of attack. Can I have some comments on this proposal (pretty version at http://www.avtechpulse.com:8080/gthumb/gthumb-exif-rotation-issues): # Add a new preference in Edit > Pref > Gen * Transform data and reset EXIF data to "top left" on import and rotate = reset_exif_orientation * Default to "No" # Auto Rotation on Import * If reset_exif_orientation==no, gthumb should import the photos, and respect the camera-supplied EXIF rotation data. No data transformation. This is the "modern" approach. * If reset_exif_orientation==yes, reset the orientation to "top-left", and losslessly rotate the images. This is the "old" approach, for maximum compatibility. Possible data corruption for non-8x8 jpegs. # Tool > Rotate * If reset_exif_orientation==no, just add and/or adjust the orientation tag, and do not transform the data. (Added bonus: this avoids the 8x8 problem entirely, see bug 329129). See bug 172427. * If reset_exif_orientation==yes, reset the orientation to "top-left", and losslessly rotate the images. * File formats other than tiff and jpeg: transform the data. # Auto Rotate on Display * respect EXIF tags in all modes (thumbnails, view, fullscreen, slideshow)!
(In reply to comment #14) I think this proposal is the correct way to solve the problem. Everyone will have the possibility to choose between the two options (physical rotation or updating the exif rotation tag). That's important, because my preference is not necessary the same as yours. But regardless of my choice (or yours in case we exchange images), the display will always be correct. But instead of a simple yes/no preference (at least for the user interface), i would prefer something less confusing. Something like: Image transformations (used on import & rotations): * Update EXIF tag (if possible) * Transform image data Because with the yes/no setting, the user gets no information on what "no" means.
Jef, Yes, you are right about the pref dialog. Maybe this is a slightly clearer version: When importing and rotating: * Use Exif orientation tag if possible (recommended) * Transform data and reset Exif tag to "top left"
Michael, I really like your proposal and I think it's the best compromise between "modern" and "old" approach. However, I still have a question: how should we proceed about this issue to get it fixed asap? And how can I help?
Tadej, Short answer: Submit a patch. Long answer: Phase 1 is to outline the problem and the solution. The approach in comment 14 and comment 16 is my attempt at that. If anyone sees flaws in the proposal, please speak up. Phase 2 is to actually code a patch against cvs. This is a pretty big job, because it touches on just about every part of gthumb (open, save, import, rotate, view, slideshow, fullscreen, thumbnails, etc, etc). If you can code, or know someone who can, this is the place to post your patch! If not, and you are in a hurry, consider hiring someone to produce a GPL patch (I've used both "mabaxv" and "openhacker" at rentacoder.com to produce patches for gthumb, at reasonable cost, that were beyond my skill level or available time. Rentacoder is a godsend for matching intent with ability!) I have a few patches under development at the moment (as well as a real job), so I'm not going to touch this code for a few weeks. If no one else steps forward, I'll look at the coding further then. I do not think that the main developer (Paolo) is going to look at this in the near term, based on my attempts to communicate with him. - Mike
*** Bug 172427 has been marked as a duplicate of this bug. ***
*** Bug 120839 has been marked as a duplicate of this bug. ***
*** Bug 325077 has been marked as a duplicate of this bug. ***
Created attachment 73546 [details] [review] Patch for the proposed user interface and preference I made a patch for the proposed user interface and the preference. That is probably the easiest part, but at least it's a start. I don't have any experience with gconf, so I'm not sure about the change to the schema file. On my system, after installation, gconf-editor says 'This key has no schema'. But for the rest it seems ok.
Created attachment 73558 [details] [review] Version 2 of the pref GUI patch Great start Jef! I've uploaded a modified version of your patch. I've changed the dialog text and the variable names, mostly to remove explicit references to the jpeg format. Exif is supposed to work for tiff, too, so I've made it more generalized. I think the dialog options are a bit more detailed now, without being too long. Please have a look. After the pref GUI is nailed down, I think that the next logical step would be to fix the display code for thumbnails, image view, fullscreen, and slideshows (probably in that order) so that the EXIF orientation tag is actually used. After that, the rotation and import functions could be examined.
My proposed pseudo-code for the next step, thumbnail fix. (It could be all wrong, mind you...) Any takers for coding? 1) In the beginning of do_load_internal in libgthumb/jpeg-utils.c, add a step to check the EXIF orientation tag of the input file, if present. If the orientation tag is 1-4, do nothing yet. If the orientation is 5-8, swap the target_width and target_height values, because a rotation will be needed. See http://jpegclub.org/exif_orientation.html for exif tag meanings. 2) At the end of do_load_internal, rotate/mirror the thumbnail using something based on update_rotation_from_exif_data (src/rotation-utils.c) so that the thumbnail pixbuf is physically rotated/mirrored to match the input exif tag. (The output thumbnail will have no exif tag, equivalent to a "top left" setting). Similar code will be needed elsewhere, so hopefully there can be lots of code re-use. Hopefully this makes sense...
Actually, I notice that Nautilus ignores the Exif orientation tag when generating thumbnails, so this may mess up thumbnail sharing for a time. I guess we'll need to patch Nautilus too. Sigh.
I think that libexif should be made mandatory for gthumb compile, rather than optional. Can someone supply a patch to update the configure files appropriately?
Michael, I wrote a patch that does something similar about two years ago, and it was rejected. I'm sure that it won't apply cleanly to the latest tree, but it might be useful as a starting point for someone. I can't remember the exact specifics on how it worked, and unfortunately, I don't have time to update it myself right now. The patch is still available at http://bugzilla.gnome.org/show_bug.cgi?id=150611
Thanks, Rennie, I wasn't aware of your earlier attempts. Sort of disheartening to see what happened to them... Another note: This bug relates to Nautilus Bug 335053 – image thumbnailing to respect rotation EXIF flag.
Created attachment 73586 [details] [review] Adjust the exif orientation tag to "top-left" when transforming a jpeg image I made another patch to adjust the exif orientation tag to "top-left" when transforming a jpeg image. I introduced a slightly modified version of the function "update_orientation_field". I did not touch the original function, because that is exactly what we need to perform a transformation using the exif tag.
Jef: Thanks! Will you contribute additional code to weave this all together? Rennie: Would you update your patch against CVS if we said "pretty please"? :-) - Mike
Okay, I'll see what I can do. Before I start, this is how my patch worked: 1. At the beginning of image_loader_sync_pixbuf() in libgthumb/image-loader.c (As I understand it, this gets called whenever any image is loaded, for any reason.), I call get_exif_tag_short() to get the exif orientation tag. 2. Farther down in image_loader_sync_pixbuf(), I perform a lossless rotation/mirror of the image using _gdk_pixbuf_copy_mirror(); if there is no orientation tag, I do nothing. (This is similar to what apply_transformation_generic() in src/rotation-utils does; apply_transformation_jpeg() changes the file on disk.) 3. I moved gth-exif-utils.c (the file containing get_exif_tag_short()) from src/ to libgthumb/ since I'm now calling routines from it inside libgthumb, and I updated the makefiles accordingly. Does this approach look appropriate to all? I could easily move the rotation code from image-loader.c to jpeg-utils.c (assuming that only jpeg images have exif tags). By the way, the way that temporary file names are generated in apply_transformation_jpeg() in src/rotation-utils.c is vulnerable to race conditions; there is a risk that someone could exploit this to overwrite arbitrary files with the permissions of the user running gthumb. The best way to fix this would be to use the mkdtemp() method described in http://www.dwheeler.com/secure-programs/Secure-Programs-HOWTO/avoid-race.html. Rennie
Rennie, Great news! One point: TIFF files can have exif tags. I'm not sure if gthumb handles them right now, but new code should not assume exif is for jpegs only. I think your approach for modifying image display is sound. Please keep in mind that the thumbnail file generation code might need to re-use some of your code. (Your patch already covers thumbnail display.)
*** Bug 143603 has been marked as a duplicate of this bug. ***
Created attachment 73680 [details] [review] Patch to automatically rotate images according to EXIF data when loaded Here's what I've got. The patch is really huge since I had to move gth-exif-utils.* from src/ to libgthumb/ and src/jpegutils/ to libgthumb/jpegutils (and update the makefiles accordingly), since libgthumb/image_loader.c now depends on stuff in both. The actual modifications to libgthumb/image_loader.c were pretty simple. If this patch is accepted, someone's going to have to play with the CVS tree to move all the files around. This patch also fixes thumbnails generated by gthumb. I'm not particularly familiar with how the memory management in the gnome libs works, so could someone please check my modifications in image_loader.c for memory leaks?
Michael, Rennie, Jef, great progress! I'll try to patch and recompile gthumb later today and give you some feedback.
Great, Rennie, this move things along very nicely! Rennie's patch does appear to correctly display Exif-rotated images, and generate correctly oriented png thumbnails (for the .thumbnails directory). So, to summarize: DONE: Preference GUI works (patch in comment 23) DONE: Exif-aware thumbnail generation works (patch in comment 34) DONE: Exif-aware image display works in all modes (patch in comment 34). TO DO: rewrite rotate-on-import to respect new PREF_USE_EXIF_ORIENTATION preference (i.e., either preserve exif tag, or transform data and reset exif tag*). TO DO: rewrite tools-rotate to use the new PREF_USE_EXIF_ORIENTATION preference. Remembering to take into account the initial exif tag, either calculate a new exif tag, or reset exif tag* and transform the data accordingly. *PARTIAL: new function available to reset exif tag after rotate (in comment 29). ISSUES: patch in comment 34 requires special submission to CVS to accomodate file movement, because CVS doesn't handle file moves gracefully. Any takers for the "TO DO"s? - Mike
I have a patch coming soon for the importer function.
Created attachment 73721 [details] [review] Patch to fix importer. Here is the importer patch. Easier than I thought! This requires the patches in: comment 23 (gui patch, v2) comment 29 (reset tag) comment 34 (display fix) comment 37 (this patch - importer) However, I would appreciate it if a C guru could explain how this syntax: eel_gconf_get_boolean (PREF_PHOTO_IMPORT_ADJUST_ORIENTATION, TRUE) applies to a single-argument function like eel_gconf_get_boolean. From the context, it appears that if PREF_PHOTO_IMPORT_ADJUST_ORIENTATION is null (i.e., the preference doesn't exist), then "TRUE" is assumed. Is this right? How does that work? Is the comma operator being funky, or am I missing a subtlety here?
Status: just need a patch for the tool>rotate functions!
(In reply to comment #38) Michael, eel_gconf_get_boolean() is declared in libgthumb/gconf-utils.h as taking two parameters - a const char* and a gboolean. The second argument is the default, in case the requested preference doesn't exist. There's no trickery involved. Perhaps you were thinking of gconf_value_get_bool()?
(In reply to comment #40) Oops, silly me. The eel2 package also has an eel_gconf_get_boolean routine. I assumed it was that one, rather than another one defined inside gthumb. Sigh. - Mike
Created attachment 73849 [details] [review] Patch to fix the rotation tool I updated the rotation tool to use the new PREF_USE_EXIF_ORIENTATION preference. Currently, transformations using the exif orientation tag are only supported for jpeg images. This is because the (existing) update_orientation_field function works on jpeg data. If there is no exif orientation tag already present in the image, I fall back to the lossless jpeg transformation (also when compiling without libexif support). I did that because I think it's not a good idea to add a rotation tag to an image which had no exif tags before. Maybe it would be better to check for the presence of an exif header, instead of this specific rotation tag, but it's a start.
Jef, Hurray! The last piece. Everybody please test by applying these patches against CVS, in order: comment 23 (gui patch, v2) comment 29 (reset tag) comment 34 (display fix) comment 38 (importer patch) comment 42 (rotate tool patch) Can someone make a "roll-up" patch incorporating all of these, for convenience? I'm not sure which options have to be enabled in the "cvs diff" command to catch the moved library files... whatever I'm doing now doesn't catch them :-(
One thing that quickly becomes apparent during testing is that many old photos were incorrectly imported by gthumb if you had a camera which set the orientation tag. The image was transformed, but the tag was not reset. So with the new (correct) code, the images look wrong. Perhaps a Tool > Reset Orientation Tag item needs to be added to fix the inevitable screwed-up photos? Thoughts? (I tried using the jpegexiforient.c tool to fix them in bulk. However, for some reason, it can't read the orientation tag of the previously-rotated files. Maybe an earlier bug?)
Jef, I think something is wrong in your code. If you use the tools>rotate function to rotate 90 degrees clockwise (for example), the preview looks OK, but the full image is actually rotated counter-clockwise...
Created attachment 73881 [details] [review] Patch to fix the rotation tool (v2) The new patch also disables the "Adjust photo orientation" checkbox. Because that is useless if we are using the exif orientation tag to do the transformation. And using it anyway resulted in a double transformation. And that's certainly not what we wanted.
Created attachment 73884 [details] [review] Adjust the exif orientation tag to "top-left" when transforming a jpeg image (v2) The new patch removes the comment refering to bug 318828 (which i think is fixed by applying this patch).
(In reply to comment #45) > I think something is wrong in your code. If you use the tools>rotate function > to rotate 90 degrees clockwise (for example), the preview looks OK, but the > full image is actually rotated counter-clockwise... This issue is not caused by the rotation. It's a bug in the image loader code. I have a working fix already and will prepare a patch (probably tomorrow).
Created attachment 73929 [details] [review] Patch for the bug in the image loader code I have finished my patch to fix the bug in the image loader code. Some transformations were performed incorrectly. The exif orientation tag specifies how the stored image should be displayed, and not the other way around. This is not a cumulative patch. You'll have to partially reverse the original patch (only for the file "libgthumb/image-loader.c") before applying this patch! You can use something this command to do that: filterdiff -p1 -i libgthumb/image-loader.c original.patch | patch -p1 -R
Ah, that works better now. Rotations / flips / mirrors seem to work as advertised. To re-cap, the patch is now just 7 easy steps :-) as follows: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p1 < gthumb-20060929-exif-rotate.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v2.patch filterdiff -p1 -i libgthumb/image-loader.c gthumb-20060929-exif-rotate.patch | patch -p1 -R patch -p0 < gthumb_image_loader.patch
Tadej, Can you test the new set of patches (instructions in comment 50)? - Mike
Jef, something is still wrong. Take an image that has a "top left" tag. Note the point on the image that is in the top left corner (let's call it the datum). Rotate the the image 90 degrees clockwise. The image does appear to be rotated 90 degrees clockwise in gthumb, but the orientation tag is "left bottom", not "top right". This isn't right - the part of the image that was at the datum should now be right-top after a 90 degree clockwise rotation. The image is as desired, but the tag is wrong! If you view the image in xnview (I have it on Windows 2000), you will see it rotated so that the datum is in fact in the "left bottom" position. I think that the original transforms were wrong (reversed), and you fixed the display code to compensate. The original transformations were meant to reverse the orientation back to "top-left", not to implement a new rotation (forward). You need to fix the transforms (in get_next_value_rotation_90, get_next_value_mirror, and get_next_value_flip) and revert the display patch, I think. Tricky stuff. - Mike
Created attachment 73951 [details] [review] Patch to fix rotation matrix New patch to fix transform matrix. This replaces Jef's last patch. The new recipe is: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p1 < gthumb-20060929-exif-rotate.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v2.patch patch -p0 < matrix.patch
I think things are looking pretty good now. I think (fingers crossed) everything works.
Created attachment 73958 [details] [review] Patch to fix the rotation tool (v3) Michael, you were right. I did compensate the rotation bug. Your patch for the rotation matrix seems to work fine here, but the other get_next_value_* functions need a patch too. I have incorporated them in this new patch for the rotation tool. (I don't have the permission to make your matrix patch obsolete, so you'll have to do that yourself.)
Created attachment 73960 [details] [review] Patch to cleanup the image loader code The image loader code can be simplified for the case of GTH_EXIF_ORIENTATION_BOTTOM_LEFT. We are doing 2 mirror/flip operations now, while we can obtain the same result with only one transform. Maybe it is also a good idea to move this transformation code to a separate function? Remember to partially reverse the original patch again before applying this patch!
OK, I concur with your new matrices, after doodling on grid paper... marking my patch obsolete. For testers (anybody? hello?), please use: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p1 < gthumb-20060929-exif-rotate.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch
Michael, Jef, you are doing an amazing job, keep it up! I try to test it all tommorow and report my feedback. Also, Michael, thank you for your instructions, it's hard to keep up with all the patches comming in :)
(In reply to comment #56) > Remember to partially reverse the original patch again before applying this > patch! Jef, Could you post the actual, exact command to do this, so I can add it to the end of my "recipe"? I can handle juggling multiple patches on buggy code, but once we start patching the patches my brain starts to melt... - Mike
(In reply to comment #59) > Could you post the actual, exact command to do this... The command is exactly the same as before: patch -p1 < gthumb-20060929-exif-rotate.patch filterdiff -p1 -i libgthumb/image-loader.c gthumb-20060929-exif-rotate.patch | patch -p1 -R or simply apply the patch, but exclude that one file: filterdiff -p1 -x libgthumb/image-loader.c gthumb-20060929-exif-rotate.patch | patch -p1
(In reply to comment #46) > Created an attachment (id=73881) [edit] > Patch to fix the rotation tool (v2) > > The new patch also disables the "Adjust photo orientation" checkbox. Because > that is useless if we are using the exif orientation tag to do the > transformation. And using it anyway resulted in a double transformation. And > that's certainly not what we wanted. > Are there still conditions under which that checkbox is enabled? If not, then it should be removed entirely.
Created attachment 73989 [details] [review] Patch to automatically rotate images according to EXIF data when loaded (v2) One more for the patch soup... This one moves the image-loader changes into a new function, as per comment 56. It incorporates my old patch (obsoleting it) and also incorporates Jef Driesen's cleanup of my old patch, (thanks, Jef!), so that the whole filterdiff/reverse process is no longer necessary. (Jef, could you please obsolete your image loader patch?) The patch procedure that I used is this: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061003-exif-rotate.patch I tested importing, display, rotation, and thumbnail generation, and encountered no problems. Michael, would you still like libexif to be made mandatory for compilation (as per comment 26)? I can whip up a patch for that if you'd like. Rennie
(In reply to comment #61) > (In reply to comment #46) > > The new patch also disables the "Adjust photo orientation" checkbox. Because > > that is useless if we are using the exif orientation tag to do the > > transformation. And using it anyway resulted in a double transformation. And > > that's certainly not what we wanted. > > Are there still conditions under which that checkbox is enabled? If not, then > it should be removed entirely. That checkbox is still enabled when doing a physical transformation of the image data. I think the label "Adjust photo orientation" is very confusing here. What it does is *reading* the exif data and use that information for the rotation tool. I think this should be renamed to "Read photo orientation" (or something similar). If we use the exif orientation tag to do the transformation, the image is already presented with the correct transformation and re-reading the tag results in a double transformation. That's why I disabled it for that case only. (In reply to comment #44) > One thing that quickly becomes apparent during testing is that many old photos > were incorrectly imported by gthumb if you had a camera which set the > orientation tag. The image was transformed, but the tag was not reset. So with > the new (correct) code, the images look wrong. > > Perhaps a Tool > Reset Orientation Tag item needs to be added to fix the > inevitable screwed-up photos? Thoughts? We could introduce a new checkbox "Reset photo orientation" for that. Checking it would simulate reading a top-left exif orientation tag (regardless of the real value of the tag). The case of the physical transform will need some more attention, because the displayed image (autorotated with the incorrect tag) does not match the real image. This means the real image only needs a correct top-left tag and no physical transformation, unless we also perform some manual transform (using the buttons). The simple solutions would be to disable those buttons, just like the behaviour of the "Adjust photo orientation" checkbox now. (In reply to comment #62) > This one moves the image-loader changes into a new function, as per comment 56. > It incorporates my old patch (obsoleting it) and also incorporates Jef > Driesen's cleanup of my old patch, (thanks, Jef!), so that the whole > filterdiff/reverse process is no longer necessary. > > I tested importing, display, rotation, and thumbnail generation, and > encountered no problems. There is a memory issue somewhere in your new code. When starting gthumb from the commandline, I get multiple "GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed" messages. But I don't see the problem immediately, because I don't understand the GdkPixbuf memory model very well. (Even in my imageloader patch, the temp GdkPixbuf is never released with g_object_unref. But it worked without errors. Does it happen automagically somehow?) Regardless of the current issues, in your new function you also seem to release the incoming GdkPixbuf. I would avoid doing something like that, because I think that is dangerous behaviour. Certainly when it's not documented. Maybe you should modify your patch to leaves the file "libgthumb/image-loader.c" intact, so we can work on that file without having to partially reverse your large patch with every attempt. > (Jef, could you please obsolete your image loader patch?) I can only do that when uploading a new patch. Someone with more rights will have to do that. But after reading this reply it's maybe not necessary anymore ;-)
(In reply to comment #63) > > it should be removed entirely. > That checkbox is still enabled when doing a physical transformation of the > image data. I think the label "Adjust photo orientation" is very confusing > here. What it does is *reading* the exif data and use that information for the .... > We could introduce a new checkbox "Reset photo orientation" for that. Checking > it would simulate reading a top-left exif orientation tag (regardless of the I think simplicity and consistency should be the twin goals here. I think it would be best to remove the "adjust photo orientation" entirely, and assume that any exif info supplied with the image is valid and useful. For cases when this assumption is wrong, we should add a separate Tool > Reset Exif Orientation Tag tool. This will be uncommon (I hope), so it should NOT be in the main rotate dialog - make it a separate "specialty" tool. Maybe we could add a note in the rotation dialog (under the "lossless" note) mentioning the availability of the orientation reset tool for screwed-up exif data. - Mike
Obsoleting patch-of-a-patch 73960...
(In reply to comment #62) > > Michael, would you still like libexif to be made mandatory for compilation (as > per comment 26)? I can whip up a patch for that if you'd like. > > Rennie I'm not sure... what does everyone else think? libexif is small and ubiquitous; are there any modern distros without it? It would let us remove great chunks of ifdefs for cleaner code. Anyone see any drawbacks to making libexif mandatory? I guess the same could be said of libjpeg, too, come to think of it. Who doesn't have libjpeg? gthumb uses jpegtran if jpeglib isn't present, but jpegtran is usually bundled with libjpeg in most distros, which defeats the purpose of all those ifdefs. Paolo, are you following this thread? Do you have any thoughts on this? - Mike
The UI is still a little wacky... There is both a "Tool > Rotate Images" AND an "Image > Transform > Rotate" menu entry. (Similarly, there is a "Tool > Scale Images" AND an "Image > Resize" entry) The Tool dialogs can operate on multiple images, whereas the Image dialogs require a single open image. The Tool dialogs will save images, but the image ones don't. (I think). Which is all a bit confusing. Should we remove the existing "Image" rotate/scale functions, and possible move the tool functions into the Image menu? Or leave the "Image" entries there, but make them launch the same dialog boxes and functionality as the "Tool" entries? Or leave things as-is? Maybe this is material for another bug report (maybe bug 145402), but I'll bring it up here anyway since we've got such good discussion, expertise, and progress happening here... - Mike
I'm having two problems with the comment 62 recipe... 1) Take a "top-left" image. Swap the right and left sides (is that a flip or mirror? anyway, use the horizontal arrow). Rotate 90 degrees CW twice. Image no longer loads successfully after the second rotate. The browser just shows an icon, rather than a thumbnail. I haven't seen this behaviour before; maybe it was introduced in the image loader clean-up. 2) Memory errors, as mentioned previously: (gthumb:4686): GLib-GObject-CRITICAL **: g_object_ref: assertion `G_IS_OBJECT (object)' failed (gthumb:4686): GLib-GObject-CRITICAL **: g_object_unref: assertion `G_IS_OBJECT (object)' failed
I've confirmed that there is a regression - the horizontal swap + 2 rotates works correctly in the comment 57 patch recipe, but fails in the comment 62 recipe.
It's the GTH_EXIF_ORIENTATION_BOTTOM_LEFT view that's failing. Jef, you tweaked this recently (comment 56). Please check the code again... - Mike
It's a copy & paste error by Rennie. He has assigns the result of _gdk_pixbuf_copy_mirror to variable "temp" instead of variable "result". My original patch (which is now made obsolete) is still correct.
Created attachment 74008 [details] [review] Patch to automatically rotate images according to EXIF data when loaded (v3) Thanks for tracking down the bug, Jef. New patch and recipe: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch
Re Comment #64, Mike: Have you missed the entire conversion and the point of why this bug is open in the first place? If you're physically rotating the image, it is paramount to reset the Exif orientation to Top-Left. That is the new orientation of the image, plain and simple. WYSIWYG.
Created attachment 74014 [details] [review] Automatically rotate images according to EXIF data when loaded (v3) (In reply to comment #63) > > There is a memory issue somewhere in your new code. When starting gthumb from > the commandline, I get multiple "GLib-GObject-CRITICAL **: g_object_unref: > assertion `G_IS_OBJECT (object)' failed" messages. But I don't see the problem > immediately, because I don't understand the GdkPixbuf memory model very well. > (Even in my imageloader patch, the temp GdkPixbuf is never released with > g_object_unref. But it worked without errors. Does it happen automagically > somehow?) Those errors appear to be coming from the call to g_object_unref (priv->animation) in get_file_info_cb(). It would seem that the pointer returned by gdk_pixbuf_animation_get_static_image() in image_loader_sync_pixbuf() is actually something internal to priv->animation and doesn't need to be freed there if we decide not to use it. (So there shouldn't be any memory leaks there.) (In reply to comment #63) > > Maybe you should modify your patch to leaves the file > "libgthumb/image-loader.c" intact, so we can work on that file without having > to partially reverse your large patch with every attempt. Good idea; I've split the image-loader.c changes and the dependencies into two separate patches, so that if any more changes are needed, we don't need to go through all this trouble. (The dependency patch will be in the next post.) (In reply to comment #71) > > It's a copy & paste error by Rennie. He has assigns the result of > _gdk_pixbuf_copy_mirror to variable "temp" instead of variable "result". My > original patch (which is now made obsolete) is still correct. Sorrry, my bad. I've fixed that in this version. Rennie
(In reply to comment #63) Rennie(In reply to comment #72) > Created an attachment (id=74008) [edit] > Patch to automatically rotate images according to EXIF data when loaded (v3) > > Thanks for tracking down the bug, Jef. New patch and recipe: > > patch -p0 < gthumb-exif-v2.patch > patch -p0 < gthumb_reset_exif_orientation_v2.patch > patch -p0 < importer.patch > patch -p0 < gthumb_rotate_v3.patch > patch -p1 < gthumb-20061004-exif-rotate.patch > Never mind my last comment and patch; Michael's patch covers it. I should have hit refresh before posting. Rennie
(In reply to comment #73) > Re Comment #64, Mike: > > Have you missed the entire conversion and the point of why this bug is open in > the first place? If you're physically rotating the image, it is paramount to > reset the Exif orientation to Top-Left. Pat, No, I haven't missed the conversation. That's not what's being debated. Yes, the OUTPUT tag is reset to top-left after physical rotation. The question (I think, someone correct me politely if I'm wrong) is, should there be an option to respect/ignore the INPUT orientation tag for calculation purposes? I say: no, assume the input tag is useful under all normal conditions (regardless of the rotation method used). Provide a separate tool to reset the input tag if this assumption is wrong.
I wrote a lot of the exif-handling code for GIMP, and having researched the right way to do it at the time, can maybe give some helpful pointers. The exif specs define precisely which fields an image-manipulating program should modify when it saves an image after altering it, and what it should do with them. I wrote out a document summarizing this information, which you can find at: http://cvs.gnome.org/viewcvs/gimp/devel-docs/exif-handling.txt?rev=1.1&view=markup You can also find the GIMP code written for exif-handling when saving a modified image at: http://cvs.gnome.org/viewcvs/gimp/plug-ins/jpeg/jpeg-exif.c?rev=1.15&view=markup in the function jpeg_setup_exif_for_save(). Note that GIMP currently uses libexif for exif manipulation. The code for processing exif when loading an image is in the same file. By the way, regarding the orientation tag, the decision we at GIMP made was, when it is anything other than top-left, to ask the user whether to use it -- we did this because we knew there are programs that don't modify the tag correctly when saving an image, including older versions of GIMP. There was a lot of argument about it, though. If you are interested in the discussion, look at: http://bugzilla.gnome.org/show_bug.cgi?id=121810 Hope this helps, -- Bill
Thanks Bill, it's really useful to hear from gimp! Maybe I'm being too optimistic in assuming input exif tags will normally be valid. I guess we could rename (& invert) the checkbox to something like "Ignore original Exif rotation tag" after all (only for use in physical rotate mode, of course). Jef, Rennie - what do you think? - Mike
Ignore my previous comment. Further thought suggests it's a bad idea... I think this checkbox should disappear entirely. It doesn't make sense to optionally ignore the input orientation tag in rotate mode only, if the image loader ALWAYS respects it now (i.e., folder view, image view, etc). If the input tag is wrong, just keeping hitting the rotate button... So, to re-summarize my vacillating views: delete the "adjust orientation" checkbox in all modes, and add a Tool > Reset Orientation Tag function for bulk resets of photos previously mangled by gthumb and/or gimp. - Mike
(In reply to comment #78) > Thanks Bill, it's really useful to hear from gimp! > > Maybe I'm being too optimistic in assuming input exif tags will normally be > valid. I guess we could rename (& invert) the checkbox to something like > "Ignore original Exif rotation tag" after all (only for use in physical rotate > mode, of course). > > Jef, Rennie - what do you think? > > - Mike > I'd say that our goal here is to allow the user to perform any transformation that s/he wants, and have the Exif tag correctly reflect the image data afterwards. I guess that we could break usage of the rotation tool into the following cases: 1. A user wants to correct an image that doesn't display properly in Exif-aware programs because the Exif orientation and JPEG data don't match. The current patches, which change only the Exif tags, cover this. 2. A user wants to correct an image that doesn't display properly in Exif-unaware programs because the JPEG data doesn't have a "top-left" orientation. If we want to cover this case, we need to provide a way to reset the Exif tag to "top-left" and perform a transformation on the JPEG data. 3. A user wants to perform a transformation on an image for display in an Exif-aware program (like our patched gthumb). In this case, changing only the Exif data, like our patches do, is adequate. 4. A user wants to perform a transformation on an image for display in an Exif-unaware program. Again, to cover this case, we need to provide a way to reset the Exif tag to "top-left" and perform a transformation on the JPEG data. Right now, we only make transformations by changing Exif data, but given what the user is trying to do, I think that we should actually change the JPEG data. Perhaps when rotating, we should default to resetting the Exif tag to "top-left" (if it is present) and performing the transformation on the JPEG data. We could provide an option to "Update only Exif orientation" that would use our current behaviour of changing the orientation tag and leaving the JPEG data alone. In this case, I think that we could lose the "Adjust photo orientation" option entirely. Personally, I wouldn't object if pictures were always rotated to "top-left" when imported, as long as the Exif tag was updated correctly and gthumb still correctly handled pictures with other Exif orientations. (In reply to comment #79) The problem with a separate "Reset orientation tag" tool, as I see it, is that unless we assume knowledge of the tool that did the mangling, the tool doesn't know what the correct orientation value is without prompting the user, requiring a very similar interface to what we have for rotation. (I have a few images that were mangled to "right-top" orientations.) My suggestion should allow users to fix mangled orientations using the rotation dialog with a minimum of confusion. Rennie
(In reply to comment #80) > Right now, we only make transformations by changing Exif data, but given what > the user is trying to do, I think that we should actually change the JPEG data. > Perhaps when rotating, we should default to resetting the Exif tag to > "top-left" (if it is present) and performing the transformation on the JPEG > data. We could provide an option to "Update only Exif orientation" that would Rennie, That's what the first patch in the recipe does! It gives you the choice of whether to just use exif tags, or physical rotate data and reset to "top left". See the Edit > Pref > General tab, and comment 14 for the "master plan". - Mike
It is a violation of the exif specification to do a rotation simply by changing the exif tag -- the exif specs say clearly that any software that modifies an image should save the result with top-left orientation. I don't believe that specs should be followed religiously in situations where they don't make sense, but the default should always be to follow them unless there is a very good reason not to. Also you should be aware that there are still plenty of jpeg-handling programs out there that don't understand exif tags at all.
Re: Comment #76 @ Michael. Pardon me, I misunderstood. Yes, the input tag could be assumed good. If it's not set by a camera, it defaults to Top-Left. It only goes bad if people used previous versions of gThumb.
(In reply to comment #82) > It is a violation of the exif specification to do a rotation simply by changing > the exif tag -- the exif specs say clearly that any software that modifies an > image should save the result with top-left orientation. Hmm, good point. Perhaps we should abandon the rotate-by-exif-tag-alone feature, since it is "illegal" and might be incompatible with older software? That just involves trimming the patches, so it isn't hard. Then the Edit > Pref > General radio boxes would just control what to do on import: do nothing (respect tag + data), or physically transform data + reset tag. (The UI labels would have to be altered a bit, of course). So, my thinking now: Import: keep the current choice of modes (respect tag/data vs transform data/reset tag), but re-word the UI labels Rotate: delete the rotate-by-tag-alone mode, always transform and reset to top-left Input tags: always assume they are valid (i.e., remove the "adjust orientation" checkbox from the rotation dialog) Reset tag tool: add it, to fix old gthumb mangling.
The spec does not apply here, simply because the user is most likely to do this to correctly incorrectly-written Exif, like from cameras lacking a tilt sensor.
OK, maybe reducing choice isn't a good idea right now... and we (or maybe just me) are getting distracted by small details. Let's focus on minor tweaks to the current patches, so they work reasonably well and can be committed to CVS. These patches fix so much in gthumb as it is... I will modify the pref gui, so that the physical transform mode is the default, for maximum compatibility and minimum conflict with the exif standard. Jef, I think the "Adjust photo orientation" box is confusing and useless (since the image loader always uses the exif info, why should rotate ignore it?). Can you add a patch to remove it, so that the rotation code always reads the incoming tag? I can add a Tool > Reset Orientation Tag later. Let's wrap this up... - Mike
Created attachment 74025 [details] [review] Make physical transform default mode in pref GUI. Updated pref gui patch. New patch recipe: patch -p0 < gthumb-exif-v3.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch Pat, can you test these patches too?
(In reply to comment #81) > (In reply to comment #80) > > That's what the first patch in the recipe does! It gives you the choice of > whether to just use exif tags, or physical rotate data and reset to "top left". > See the Edit > Pref > General tab, and comment 14 for the "master plan". > > - Mike Sorry, I had thought that that only applied to images as they were being imported. In that case, your suggestion to add a separate "reset Exif orientation" tool should work - although if the Exif tag is incorrect and the JPEG data is not in "top-left" format, then an additional rotation will be required to fix things. If we're adding tools, we might consider adding one to convert a non-"top-left" image into "top-left" orientation. I think that there is a problem with the image preview in the rotation dialog - when I fed it an image with "left-bottom" exif and physical orientation and told it to "adjust photo orientation", the preview rotated 90 degrees counterclockwise, but it gave the correct result (a "top-left"-oriented image) when I hit "apply". If course, if we get rid of the "adjust photo orientation" option entirely, this should go away. Rennie
(In reply to comment #77) > By the way, regarding the orientation tag, the decision we at GIMP made was, > when it is anything other than top-left, to ask the user whether to use it -- > we did this because we knew there are programs that don't modify the tag > correctly when saving an image, including older versions of GIMP. There was a > lot of argument about it, though. If you are interested in the discussion, > look at: > > http://bugzilla.gnome.org/show_bug.cgi?id=121810 There is one very important difference here. GIMP is an image editor, where you open one image at a time, to modify the actual pixel data (resizing, cropping, changing colors, or whatever). Asking the user makes perfectly sense here. But gthumb is mainly an image viewer/organizer with some very basic image manipulations. Asking the user for every single image is certainly a very bad decision (imagine viewing a slideshow or galery with that)! Instead of asking every time, we should ask only once and store that choice in a preference. Or set a good default value that works for most users, and give the possibility to change that. In the current proposal (see comment 14), we assume always the 'autorotate for display purpose' setting. (In reply to comment #82) > It is a violation of the exif specification to do a rotation simply by changing > the exif tag -- the exif specs say clearly that any software that modifies an > image should save the result with top-left orientation. I don't believe that > specs should be followed religiously in situations where they don't make sense, > but the default should always be to follow them unless there is a very good > reason not to. > > Also you should be aware that there are still plenty of jpeg-handling programs > out there that don't understand exif tags at all. I agree with you about following the standards. But I think we are not violating the Exif specification by doing a rotation simply by changing the Exif tag. If we are doing such a tranform, we are *not* modifying the image (that's the whole idea!). We only change the way an exif-aware application should display the image. And I think that is exactly the purpose of this orientation tag. Isn't that the reason why digital cameras have an orientation sensor in the first place? Doing this kind of transform also has the advantage that we do not have to worry about adjusting other tags, because they will remain valid. (As a sidenote, that is also the reason I'm doing this kind of orientation only when there is already exif data present in the file (my patch still needs some improvement here). That is typically the case for images from a digital camera. I think adding only a single orientation tag does not make much sense.) If we do the transform by modifying the pixel data (lossless or not), we should of course reset the orientation tag to top-left, just as the exif specification says. But that was already taken care of by one of my patches (and agreed upon by others). To fulfil with the requirements of the exif specification, we should also adjust those other tags. Some of them are already changed by gthumb, others are not.
Created attachment 74049 [details] [review] Patch for the image loader code (v3) I made a new patch for the image loader code. The transformations are moved to a separate function. The main difference with the previous patches (by Michael, see comment 72 or Rennie, see comment 74) is that it should properly fix the memory issues. I also do not pass the filename (to read the exif tag from) as a parameter, but directly the orientation value. This way the function can be reused more easily when the orientation value does not come from a file.
Created attachment 74053 [details] [review] Patch to delete checkbox, and re-work rotation code Here is a new patch that deletes the problematic "Adjust photo orientation" checkbox. It also re-works the "apply_transformation" function in src/dlg-jpegtran.c. Now, all images (regardless of mode) use the rotate-using-the-exif-tag approach. For exif-tagmode, processing stops there. For physical rotate mode, the standard transform-backwards and reset-to-top-left routines are called. The new scheme should handle any combination of input exif tags and rotation methods. Jef, can you look at that part carefully, since I'm messing with your code? :-) I haven't looked at your most recent patch, so there might be conflicts to be merged.
This is my updated recipe for testing (which does not yet include Jef's latest patched, which I haven't had time to review): patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch patch -p1 < delete-checkbox.patch It seems to work for me (except for the memory errors). To do: incorporate Jef's stuff, stomp memory errors, think about making libexif and libjpeg mandatory and removing many ifdefs, and think about an bulk-exif-reset-tag tool. - Mike
Sorry, type, that should be: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch patch -p2 < delete-checkbox.patch
Jef, I still get g_object_unref (but no more g_object_ref) errors with your patch installed, when rotating in tag-only-mode. The new patch recipe, including Jef's latest, is: patch -p0 < gthumb-exif-v2.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch patch -p2 < delete-checkbox.patch filterdiff -p1 -i libgthumb/image-loader.c gthumb-20061004-exif-rotate.patch | patch -p1 -R patch -p0 < gthumb_image_loader_v3.patch - Mike
Damn it, I can't type this morning. Sorry for the extra messages. The correct latest recipe is: patch -p0 < gthumb-exif-v3.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch patch -p2 < delete-checkbox.patch filterdiff -p1 -i libgthumb/image-loader.c gthumb-20061004-exif-rotate.patch | patch -p1 -R patch -p0 < gthumb_image_loader_v3.patch
If I can make another unwelcome suggestion, you would probably find it a good bit easier to create a cvs branch than to juggle 8 patches at once.
Bill, Your suggestions are most welcome. You're probably right, we should be taking advantage of cvs. But wouldn't we need cvs write/commit permission to create and use it? (I've requested cvs write access, but my request seems to be in limbo. Know anyone who can grant it?) The main gthumb maintainer doesn't seem to be actively pursuing development...
Created attachment 74094 [details] [review] Combined patch, 2006-10-05 To make life easier, here is a complete, combined patch for everything so far. To install, just one step: patch -p1 < combined-2006-10-05.patch Please test! Known issues: unref errors when rotating. Jef, Rennie, if you're OK with the combined-patch approach, I'll obsolete the earlier piecemeal patches.
(In reply to comment #98) > Jef, Rennie, if you're OK with the combined-patch approach, I'll obsolete the > earlier piecemeal patches. That's fine by me. I'm not getting any error messages when rotating, either in exif mode or physical mode. However, when I open an image that is not in "top-left" orientation in the rotation tool while in exif-rotate mode, it seems to automatically perform a rotation without me doing anything. When I gave it a "left-bottom" or "right-top" image, it automatically rotated it to "bottom-right"; when I loaded a "bottom-right" image, it automatically rotated to "top-left". Other than that, everything seems to work for me. Rennie
I think a lot of the confusing in this bug is caused by the fact that we are dealing with multiple issue. I have written a document to summarize all the problems and some of the proposed solutions. It is available at my personal website: http://users.telenet.be/sacn/tmp/gthumb_rotation.html It won't stay there forever, but I can attach it to this bug also. (In reply to comment #98) > Jef, Rennie, if you're OK with the combined-patch approach, I'll obsolete the > earlier piecemeal patches. I don't think that's a good idea, because there are still some issues left. A combined patch will also prevent the gthumb developers to commit partial patches that are already completely finished (like my reset to top-left patch, attachment 73884 [details] [review]). (In reply to comment #99) > However, when I open an image that is not in "top-left" > orientation in the rotation tool while in exif-rotate mode, it seems to > automatically perform a rotation without me doing anything. When I gave it a > "left-bottom" or "right-top" image, it automatically rotated it to > "bottom-right"; when I loaded a "bottom-right" image, it automatically rotated > to "top-left". This is most likely caused by the removal of the "Adjust photo orientation" checkbox. The function update_from_exif_data, which is responsible for its action, will now *always* read the exif orientation tag, causing a double rotation (in exif transformation mode). But I didn't verify this.
Rennie, Oops, you're right, something got screwed up in the Exif-tag rotation mode (probably during my changes). It doesn't happen if you just press [ or ], only if the full dialog is brought up. I'll look into that later today... - Mike
Jef, OK, no combo patches. I'm obsoleting it anyway, since something is broken in my delete-checkbox.patch. (It was more work than I expected removing the checkbox.) I'll take a look at your web site... - Mike
Created attachment 74127 [details] [review] Delete checkbox, correctly? The image preview callback was updating the rotation based on the exif tag. Fixed that! Please revert delete-checkbox.patch and install delete-checkbox-v2.patch. patch -p0 < gthumb-exif-v3.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061004-exif-rotate.patch patch -p1 < delete-checkbox-v2.patch filterdiff -p1 -i libgthumb/image-loader.c gthumb-20061004-exif-rotate.patch | patch -p1 -R patch -p0 < gthumb_image_loader_v3.patch
Jef, Some comments on your web site text, and other stuff: 1) You mention that other exif tags should be updated when an image is saved, according to the Exif spec. (For instance, datetime, software, etc). I think this is material for a different bug, because there are a lot of issues to juggle. The DateTime issues are particularly sensitive - I don't want gthumb re-writing DateTime just because it rotated the image. (I guess there is a DateTimeOriginal tag too, but I'm not clear if anyone uses it.) See also: Bug 339648 - some EXIF flags are filtered out (this seems like an urgent bug, actually) Bug 323943 - convert to jpeg format lost the exif information Bug 326101 - add ability to remove exif data Bug 343021 - Editing EXIF metadata Bug 142590 - gthumb should sort images by exif-Date Bug 313579 - Should use the ImageDescription EXIF tag as comment 2) libexif support - maybe mention that libexif (and libjpeg) should be made mandatory for gthumb. What do you think about that? I like the enforced compatibility and simplified code that would result. 3) I think that we can assume "autorotation for display" will be one and only operating mode. Ignoring exif tags just invites future incompatibility which benefits no one... Are we agreed on that, or is there any controvery? 4) Please try my corrected delete-checkbox-v2.patch and see if life is livable without the "Adjust photo orientation" checkbox (or whatever you wish to call it) :-) 5) The PREF_PHOTO_IMPORT_ADJUST_ORIENTATION preference is currently hidden and set to defaulted to "TRUE", as you mention. This is only used when the physical-transform method is selected. (In Exif-transform mode, the images are always imported totally unchanged.) I guess we can leave this as-is: gthumb will display and rotate the image correctly, in either transform mode, regardless of whether the data was transformed on import or not. Do you agree? Other notes: A) I'm not getting unref errors anymore, with the latest patches - I guess that's fixed! B) To do for this bug: - clean up patches a bit so filterdiff isn't needed - add a reset tool - maybe make libexif/libjpeg mandatory - ??? Anything else? Are we nearly done? C) Maybe to do in the future (i.e. another bug report) - deal with other exif tags more properly, maybe add editing - add tiff/exif support Any other outstanding issues? - Mike
(In reply to comment #104) > 1) You mention that other exif tags should be updated when an image is saved, > according to the Exif spec. (For instance, datetime, software, etc). I think > this is material for a different bug, because there are a lot of issues to > juggle. This is certainly material for another bug. Only the orientation tag is relevant for this bug (it's already complicated enough). But I mentioned it anyway for completeness and more important, to show that the exif transformation does not have this problem. > 2) libexif support - maybe mention that libexif (and libjpeg) should be made > mandatory for gthumb. What do you think about that? I like the enforced > compatibility and simplified code that would result. I agree with you. I thought that was clear enough from the text. libexif (and also libjpeg) is small and is already available on almost every linux system. I can't imagine an image viewer without jpeg support. Even if gthumb is compiled without libjpeg, the underlying gtk will probably depend on it. And for libexif, the number of images with exif data will only increase. Who doesn't own a digital camera these days? > 3) I think that we can assume "autorotation for display" will be one and only > operating mode. Ignoring exif tags just invites future incompatibility which > benefits no one... Are we agreed on that, or is there any controvery? That is also my opinion. But I didn't want to limit the discussion to only a few cases (e.g the ones I like). I tried to step back and provide an overview of (almost) all posible cases, without going into detail about actual implementation issues or even a final decision. > 4) Please try my corrected delete-checkbox-v2.patch and see if life is livable > without the "Adjust photo orientation" checkbox (or whatever you wish to call > it) :-) If the autorotation feature is the one and only display mode, I see no problem removing the checkbox (unless I missed something somewhere). > 5) The PREF_PHOTO_IMPORT_ADJUST_ORIENTATION preference is currently hidden and > set to defaulted to "TRUE", as you mention. This is only used when the > physical-transform method is selected. (In Exif-transform mode, the images are > always imported totally unchanged.) I guess we can leave this as-is: gthumb > will display and rotate the image correctly, in either transform mode, > regardless of whether the data was transformed on import or not. Do you agree? I agree again here. The only thing I would like to add that if the autorotation feature is implemented, the default could maybe changed to false here. But that is just a small detail and will probably depend on the level of exif support of other applications you want to view your images with. > A) I'm not getting unref errors anymore, with the latest patches - I guess > that's fixed! We are getting closer... > B) To do for this bug: > > - clean up patches a bit so filterdiff isn't needed A separate patch for each problem is probably the easiest way to work with newer versions of a patch. > - add a reset tool I'm not really sure we will need this after all, because I think it can be done with the rotation tool also. > - maybe make libexif/libjpeg mandatory If there is a volunteer... > - ??? Anything else? Are we nearly done? I think we have most issues covered. One thing that can use some improvement is the actual rotation code. In your latest patch you are updating exif tags, reading it again, applying the jpeg transform and resetting the tag again. That could need some cleanup, but it works of course... For the rest, I hope we get some feedback from the gthumb developers soon.
Created attachment 74132 [details] [review] Revised patch that incorporates Jef's new pixbuf transformation function Merged two patches, to simplify patching. Now: patch -p0 < gthumb-exif-v3.patch patch -p0 < gthumb_reset_exif_orientation_v2.patch patch -p0 < importer.patch patch -p0 < gthumb_rotate_v3.patch patch -p1 < gthumb-20061006-exif-rotate.patch patch -p1 < delete-checkbox-v2.patch Personally, I think we might be ready to request a CVS commit of the above patches. That should make further minor tweaking a bit simpler, and it will immediately improve gthumb dramatically.
Jef, Great, I think we're still agreeing on everything even through the confusion! > > - add a reset tool > > I'm not really sure we will need this after all, because I think it can be done with the rotation tool also. It isn't necessary at all, but it might be nice to help everyone who had their images mangled by old versions of gthumb. However, it is low priority and I think we should try to get a commit without waiting for it. > > - maybe make libexif/libjpeg mandatory > > If there is a volunteer... Rennie volunteered earlier. Rennie, can you tweak the make files to make libexif and libexif mandatory, and remove the --enable-exif, --disable-exif, --enable-jpeg, and --disable-jpeg configure flags? After that, we can remove the ifdefs at our leisure, in another patch. > the actual rotation code. In your latest patch you are updating exif tags, > reading it again, applying the jpeg transform and resetting the tag again. That > could need some cleanup, but it works of course... I'm not a born coder, so please optimize whatever you like! I've just glued together the pieces in what I think is a reliable and easy to follow way (i.e., do the exif transform on the tags first, and if the physical transform mode is selected, apply a reverse physical transform and reset the tag.) I don't think the code properly accounted for existing exif orientation tags when doing physical transforms before. > For the rest, I hope we get some feedback from the gthumb developers soon. Heh. - Mike
> For the rest, I hope we get some feedback from the gthumb developers soon. I've sent an email to paolo.bacchilega@libero.it requesting some feedback about our basic approach and the likelihood of commit. - Mike
Okay, I have just created a branch of gthumb called "exif-testing", and emailed paolo to explain what I did and why. I will commit to it any patch that Jef and Michael agree on. I don't intend to review the patches in any way, and it is entirely up to Paolo to decide whether and when to merge the branch into HEAD. If you want to participate, you should start by checking out the branch, using cvs checkout -b exif-testing gthumb Do this as a fresh checkout; don't try to modify your existing gthumb cvs. Any later "cvs update" will automatically update from the exif-testing branch, then. Also, I think this bug report has gotten too long, and have opened bug #360183 ("Exif handling development, continuation of bug #343867") as a place to continue the discussion, if that's okay with you guys.
Bill, Hey, thanks for the branch! That's great! The actual checkout command would seem to be: cvs -z3 co -r exif-testing gthumb (I don't see a "-b" option on my system). I would be grateful if you would go ahead and commit everything in comment 106. Please be aware that several files move, which requires special cvs handling I guess. See comment 34. Personally, I prefer to keep using this thread, for a while at least. Starting a new thread tosses away valuable background information, and I don't see any benefits. Everything still loads quickly... - Mike
Michael, Yes, you're right about the cvs command of course. I have committed all the patches listed in comment #106, to the exif-testing branch. I don't have the access needed to magic-move files, but that only needs to be done at the point when the old versions are deleted, and no files were removed yet by these patches. When the time comes to ask somebody to do it, instructions a bit more explicit than those in comment 34 would be helpful.
Created attachment 74155 [details] [review] Patch to make libexif and libjpeg mandatory (In reply to comment #107) > Rennie volunteered earlier. Rennie, can you tweak the make files to make > libexif and libexif mandatory, and remove the --enable-exif, --disable-exif, > --enable-jpeg, and --disable-jpeg configure flags? > > After that, we can remove the ifdefs at our leisure, in another patch. Here's a patch to the autoconf scripts to make libjpeg and libexif mandatory. As requested, I didn't try to remove the #ifdefs from the code in this patch. Once they're all gone, we can remove the appropriate AC_DEFINEs from configure.in, but there's no harm in leaving them in. (In reply to comment #103) > The image preview callback was updating the rotation based on the exif tag. > Fixed that! Please revert delete-checkbox.patch and install > delete-checkbox-v2.patch. Thanks! That issue is now gone. (In reply to comment #111) > I have committed all the patches listed in comment #106, to the exif-testing > branch. I don't have the access needed to magic-move files, but that only > needs to be done at the point when the old versions are deleted, and no files > were removed yet by these patches. When the time comes to ask somebody to do > it, instructions a bit more explicit than those in comment 34 would be helpful. Thanks for committing those. It'll make further work much easier. I don't know the best way to move files in CVS (I use subversion for my own work). What my patch did was to move - src/gth-exif-utils.c to libgthumb/gth-exif-utils.c - src/gth-exif-utils.h to libgthumb/gth-exif-utils.c - src/jpegutils/* to libgthumb/jpegutils/ The rest of the changes were to existing files and require no special handling. Rennie
(In reply to comment #112) > Here's a patch to the autoconf scripts to make libjpeg and libexif mandatory. > As requested, I didn't try to remove the #ifdefs from the code in this patch. I just meant they didn't have to be removed right away - if you're feeling bored, please go ahead and do it :-) Can you re-format your patch using the "diff -u" unified format option - I have a hard time reading the > and < characters in your patch. + and - are much easier for my eyes. > - src/gth-exif-utils.c to libgthumb/gth-exif-utils.c > - src/gth-exif-utils.h to libgthumb/gth-exif-utils.h ^^ fixed > - src/jpegutils/* to libgthumb/jpegutils/ We should keep track to see if any changes occur in these files, or if it's just a straight move. - Mike
Created attachment 74160 [details] [review] Patch to make libexif and libjpeg mandatory (v2) (In reply to comment #113) > Can you re-format your patch using the "diff -u" unified format option - I have > a hard time reading the > and < characters in your patch. + and - are much > easier for my eyes. Done. > > - src/gth-exif-utils.c to libgthumb/gth-exif-utils.c > > - src/gth-exif-utils.h to libgthumb/gth-exif-utils.h > ^^ fixed > > - src/jpegutils/* to libgthumb/jpegutils/ > > We should keep track to see if any changes occur in these files, or if it's > just a straight move. At this point, it's just a straight move. However, that's going to change once we strip the #ifdefs out of those files. Rennie
Created attachment 74164 [details] [review] Add print and properties to context menus Hmm, since we have our own branch to work on, I would like to implement a few trivial bug fixes that the main developer has ignored. While we have to be careful not to get carried away, I think there's no harm in a few selected pet peeve fixes. I want to add print + properties items to the context menus, because it drives me crazy that they aren't there... Anybody have issues with the attached micro-patch? If not, I'll asked that it be committed. Two other things: 1) Since we're actually getting things committed, please include Changelog updates in your patches from now on (like this one, for example). 2) I'm obsoleting the earlier patches, since they're committed now. - Mike
I would prefer that you not include the ChangeLog updates in the patches, but rather as a comment when you ask for a commit. They're easier to handle correctly that way.
Oops, shows what I know. Jef, could you check Rennie's makefile patch? It looks OK to me, but I'm not a makefile expert (hey... maybe we could recode gthumb in perl? just kidding...) - Mike
Created attachment 74174 [details] [review] Useful error for rotation failure. Another patch: gthumb can't actually save tiff files (!), and certain other formats like svg. (Tiff support is being added in gdk-pixbuf, upstream). Currently, gthumb fails silently (no rotation occurs), which is confusing because the preview shows a rotated image. Here is a mini-patch to throw a useful error. Please take a look, and shout back. I'd like to submit the three active patches to Bill in a batch. - Mike
Hmm, I'm having a surprising issue: If I: chmod 444 sample.jpg and rotate/apply sample.jpg, it overwrites the file, even though it is marked read-only! In fact, if I: chown root.root sample.jpg and run gthumb as me (non-root), it overwrites the file, and assigns ownership to me. So, initially in my home directory (/home/mjc) I have: [mjc@server2 bin]$ ll ~ -r--r--r-- 1 root root 80615 Oct 6 15:43 wagon1.jpg [mjc@server2 bin]$ ./gthumb ~/wagon1.jpg and hit "]" to rotate, close gthumb, and then I get: [mjc@server2 bin]$ ll ~ -r--r--r-- 1 mjc mjc 80317 Oct 6 15:47 wagon1.jpg That can't be right... does that happen to anyone else?
Created attachment 74180 [details] [review] Patch to remove unnecessary #ifdefs This patch removes all #ifdefs for libexif and libjpeg. I left the AC_DEFINEs for HAVE_LIBEXIF and HAVE_LIBJPEG in configure.in, in order to not break any other patches that are floating around that use them. Note that this patch modifies several of the files that were moved from src/ to libgthumb/. Originally, gthumb tried to support both 0.5.x and 0.6.x versions of libexif, which apparently have different interfaces. These changes remove support for libexif 0.5.x. If it is deemed important, I can put that support back. Rennie
Thanks Rennie, that will really tidy up the code! I'll double-check it over the weekend. Is anyone following this thread stuck with libexif 0.5.x? Seems unlikely... - Mike
(In reply to comment #119) > Hmm, I'm having a surprising issue: > > If I: > chmod 444 sample.jpg > > and rotate/apply sample.jpg, it overwrites the file, even though it is marked > read-only! > > In fact, if I: > chown root.root sample.jpg > > and run gthumb as me (non-root), it overwrites the file, and assigns ownership > to me. Confirmed. It doesn't completely ignore permissions - it failed to open a file owned by root with permissions of 0400 - but I had to set the ext3 'immutable' attribute on a file before gthumb stopped reassigning ownership of files that it could open. I'm not sure what could be causing this - gthumb's not installed with setuid root or anything like that. I also get this behaviour with gthumb 2.7.7, from the standard FC5 package repositories. This at least implies that it's not our fault. I reccomend that you submit a separate bug report for this issue. It's a major security issue, especially given bug 358894. Rennie
Thanks Rennie, it's filed as bug 360265.
Thanks to all for working on this issue. I have some comments: 1) I suggest to remove the EXIF option from the preferences dialog and add the option "Reset EXIF orientation" (you can think of a better name here) to both the Rotate Images and Import Photos dialogs. When this option is activated the image is rotated physically and the orientation tag is set to top-left, when the option is deactivated (the default) it simply changes the exif orientation tag. I don't like this option to stay in the main preferences dialog because it is not an option that changes the behaviour of the application, it's rather an option that applies to some specific tool, so it's better to show it when the tool is used. 2) I agree to make the libexif and libjpeg libraries mandatory and remove the #ifdefs in the code.
Paolo, Thanks for the feedback! You're probably right about the UI. We'll need two prefs then, which does give more flexibility. I'll take a look at that... - Mike
(In reply to comment #115) > Add print and properties to context menus You should open a new bug for this! Adding completely unrelated stuff will make this bug only more difficult to follow. (In reply to comment #117) > Jef, could you check Rennie's makefile patch? It looks OK to me, but I'm not a > makefile expert (hey... maybe we could recode gthumb in perl? just kidding...) I didn't had the time to take a look at it yet, but I will have a look in the next days. But my makefile knowledge is also very limited. (In reply to comment #120) > This patch removes all #ifdefs for libexif and libjpeg. I left the AC_DEFINEs > for HAVE_LIBEXIF and HAVE_LIBJPEG in configure.in, in order to not break any > other patches that are floating around that use them. They can stay for a while. > Originally, gthumb tried to support both 0.5.x and 0.6.x versions of libexif, > which apparently have different interfaces. These changes remove support for > libexif 0.5.x. If it is deemed important, I can put that support back. I think it is safe to remove support for libexif 0.5.x. It's dated and you can't even get it from the libexif website anymore. (In reply to comment #124) > I suggest to remove the EXIF option from the preferences dialog and add the > option "Reset EXIF orientation" (you can think of a better name here) to both > the Rotate Images and Import Photos dialogs. When this option is activated the > image is rotated physically and the orientation tag is set to top-left, when > the option is deactivated (the default) it simply changes the exif orientation > tag. > I don't like this option to stay in the main preferences dialog because it is > not an option that changes the behaviour of the application, it's rather an > option that applies to some specific tool, so it's better to show it when the > tool is used. I can live with that decision ;-) But I have some more questions. Can we keep the added gconf preference to make the choice in the dialog persistent? Or does the user has to check/uncheck the checkbox everytime? For the name, I would maybe choose something more general, because the technical term exif is probably not known by some users. But I can't think of a good alternative right now. My first thought was the old "Adjust photo orientation" (e.g. the oposite of your "Reset EXIF orientation"), but that is going to be very confusing for the importer (where it is equivalent with passing the image unchanged).
Created attachment 74237 [details] [review] New preferences style (import and rotate) Here is my attempt to change the UI as per Paolo's recommendation. There are now two gconf prefs, PREF_RESET_EXIF_ORIENTATION_IMPORT and PREF_RESET_EXIF_ORIENTATION_ROTATE. They default to true (i.e., reset tag to top-left by default, for maximum compatibility with other programs). I'm newish to gconf and glade, so beware ... but the code seems to work. The dialog text I've chosen is "Transform orientation to top-left". I'm open to improvements, though.
> You should open a new bug for this! Adding completely unrelated stuff will make > this bug only more difficult to follow. OK... I'll have to touch base with Bill Skaggs to see if I can mess with the branch for other bug outside of the exif project. (This is bug 99422, and it's 4 years old, has a 4-line patch, and it drives me crazy.) I also have a major red-eye reduction tool patch coming... - Mike
(In reply to comment #126) > I can live with that decision ;-) But I have some more questions. Can we keep > the added gconf preference to make the choice in the dialog persistent? Or does > the user has to check/uncheck the checkbox everytime? yes, the option will be saved in a gconf key. >For the name, I would > maybe choose something more general, because the technical term exif is > probably not known by some users. But I can't think of a good alternative right > now. My first thought was the old "Adjust photo orientation" (e.g. the oposite > of your "Reset EXIF orientation"), but that is going to be very confusing for > the importer (where it is equivalent with passing the image unchanged). a less technical alternative could be: "Reset image orientation" or "Reset photo orientation".
Created attachment 74248 [details] [review] New preferences style (import and rotate), v2 Revised patch - uses "Reset photo orientation".
I've created a gthumb-2-8 branch and applied the patches to HEAD.
Created attachment 74270 [details] [review] Rotation refinement for "edge cases" Great, it's nice to see our work hit HEAD! Here is a patch that cleans up the rotation code a bit to achieve several things: 1. Less convoluted execution when no orientation tag is present. 2. Do not physical-transform if the dimensions are not multiples of 8 (jpeg mcu) and an exif orientation tag is present, to avoid inevitable data corruption (bug 329129). It reverts to tag-change only for safety. 3. Provide error message if rotate-save fails due to unsupported file type. 4. Add comments!
Created attachment 74274 [details] [review] Rotation refinement for "edge cases", v2 Revision of previous patch - added proper width/height initialization.
Outstanding issues: 1) Import function - tidy up code, similar to rotation tool code 2) Import function - thumbnails from camera do not reflect exif orientation tag 3) Reset orientation tag tool, maybe. I think that's all... - Mike
Oh, one security issue: Paolo, could you look at / commit Rennie's related temp file improvements in bug 358894 (patch at http://bugzilla.gnome.org/attachment.cgi?id=73823&action=view)? - Mike
Created attachment 74290 [details] [review] Updates documentation of rotation tools. This patch updates the gthumb manual to reflect the Exif rotation issues. Some of the functionality described in the manual update reflects changes in the new patch 74274, "Rotation refinement for edge cases, v2", so they should be committed together.
The import code looks OK, and the import thumbnails rely on the camera and libgphoto. I don't think they can be shown correctly (i.e., reflecting the orientation tag). But that's a minor detail. So, I suggest that Patch 74274 - Rotation refinement for "edge cases", v2 Patch 74290 - Updates documentation of rotation tools be reviewed/committed, and that this bug be closed. I suggest anyone interested in coding a "Reset Exif Tag" tool open a new bug. Thanks Rennie, Jef, and everyone else! The process of fixing this was a shining model of open source collaboration, I think. - Mike
Created attachment 74336 [details] [review] New user interface and preferences I don't think this bug is already finished. I have some comments on the new user interface and some other issues: 1. I think the checkbox "Reset photo orientation" has the worst possible label. Let me explain why. First, what we are actually doing with this checkbox enabled is applying a physical transformation. Resetting the exif orientation tag to top-left is a only a small part of that. It's more a technical detail of our implementation and not really relevant to the end user. Second, the word "reset" suggest undoing some previous action and that does certainly not apply here. In the rotation dialog, it can even cause a conflict with the meaning of the reset button. I came up with the following solution: In the rotation dialog, I would replace the label of the checkbox with "Adjust photo orientation" (maybe with the word "only" appended). This label clearly indicates that the transformation will be performed by modifying the exif orientation tag. Leaving it unchecked will perform a physical transformation of the image data. But that doesn't need any further explanation, because that is exactly what most users will expect from a transformation. And it is also known behaviour from previous versions of gthumb. This way, the new checkbox represents a newly added feature. If the user doesn't understand its meaning from the label only, he can take a look at the documentation. In the import dialog, I would replace the label of the checkbox with "Keep photo orientation". This clearly indicates that the image remains unchanged. I prefer this label over using the opposite "Autorotate images" (or something similar). Because using the words "photo orientation" maintains the link with the rotation dialog and it is also consistent with the other option "Keep original filenames". In previous versions of gthumb, the physical transform was the default setting anyway (with the hidden preference), so this checkbox represents again a new feature. 2. There was already a (hidden) gconf key for the import dialog. We should use that (optionally rename it) instead of adding another one. The gconf key for the rotation dialog should be moved from "general" to a dialog specific location. 3. I think we also need to update the schema files to reflect the changes to the gconf keys. 4. We should provide a consistent default value for the new gconf keys. We should either choose adjust photo orientation (for the newer modern approach) or physically transform the image data (for maximum compatibility with previous versions and/or other applications) for both dialogs. I would choose the first, but what will most existing gthumb users (who are already used to the second option) prefer?
Hi Jef, At this point, the exact label doesn't really matter to me... I don't think there is a perfect label that is short, technically accurate, and user friendly. I'm happy with what's there, but if you can convince Paolo something is better, and supply the patch, that's fine by me. I went through considerable effort to update the documentation for the "Help" button in the rotate tool to explain what exactly the (current) "Reset photo orientation" checkbox does, in technical detail. I think it's more important to make the "Help" function full and accurate than to worry about the perfect label. (Though I see typos in my docs today... patch to follow). Please update the docs if you patch... The import function has no help button, unfortunately. In fact, the manual does not appear to mention the import function at all! I think that the import button desperately needs a help button (to describe the "Reset photo orientation" checkbox, and the import function in general). Any volunteers? (I've opened a new bug for that: bug 360866.) The new gconf keys do default to "True", as I've coded it. Physical transforms are used as the default for maximum compatibility, which I think is the only reasonable default. If you use a new label, it should encourage the user to adopt this mode. I used new key names for two reasons: so I could give them descriptive, consistent names, and so that they would default to "True" when the user installed the new version. Yes, the schema files seem to be out-of-date. - Mike
Jef, Whoops, I didn't notice your attachment! I'll try the patch and provide feedback... - Mike
Jef, I don't like your UI changes, after trying them. I'll explain why: 1). Import - "Keep photo orientation". I think this encourages the user to not reset the orientation tag to top left. For most users, I think compatibility should trump other concerns. If it is labeled "Reset photo orientation" and is on by default, I think most users will just leave the three checkboxes enabled, and thus stay in the most compatible mode. 2). Rotate - "Adjust photo orientation". Sort of a meaningless phrase. The average user will say "of course I want to adjust the orientation, that's what rotate is for". So again, this encourages the least compatible mode. I know I originally supported tag-change-only as the default mode, but after seeing how unevenly exif is supported, I think we should aim to avoid unnecessary incompatibilities for the user. Paolo, any additional thoughts on this? I'll correct my schema errors in a bit.
Created attachment 74342 [details] [review] roll-up patch of tweaks and doc fixes Paolo & everyone, I've adjusted my "tweaking" patches a bit to incorporate Jef's schema fixes (partially) and correct some of the documentation. Here is what this "roll-up" patch does: 1. Less convoluted execution when no orientation tag is present. 2. Do not physical-transform if the dimensions are not multiples of 8 (jpeg mcu) and an exif orientation tag is present, to avoid inevitable data corruption (bug 329129). It reverts to tag-change only for safety. 3. Provide error message if rotate-save fails due to unsupported file type. 4. Add comments to the rotation code for clarity. 5. Updates rotation help file to describe exif subtleties. 6. Renames some of the gconf keys, for greater consistency with other keys, and to remove them from the general section. 7. FYI, the keys are set to default to the physical transform mode. This doesn't follow Jef's suggestions for UI changes, because I don't like them (sorry!). - Mike
Created attachment 74346 [details] [review] roll-up patch v2 Sorry, had to add one additional tweak: 1-7. as above 8. remove non-existent "Tools_JPEGRotate_Auto" from pop-up menu. - Mike
Created attachment 74359 [details] [review] New user interface and preferences (v2, new defaults) (In reply to comment #141) > I don't like your UI changes, after trying them. I'll explain why: > > 1). Import - "Keep photo orientation". I think this encourages the user to not > reset the orientation tag to top left. For most users, I think compatibility > should trump other concerns. If it is labeled "Reset photo orientation" and is > on by default, I think most users will just leave the three checkboxes enabled, > and thus stay in the most compatible mode. > > 2). Rotate - "Adjust photo orientation". Sort of a meaningless phrase. The > average user will say "of course I want to adjust the orientation, that's what > rotate is for". So again, this encourages the least compatible mode. I respect that you don't like my proposal. But I'm afraid your argument against my proposal applies to the "Reset photo orientation" label as well. You want to prevent the user from changing the default physical transformation (assuming he doesn't understand the difference with adjusting the exif orientation tag). I don't think you should do that by means of the label. As you say, most users will never alter the default settings (for whatever reason), so that is a more appropriate way to obtain the same result. There is simply no way to prevent the user from experimenting, because the checkbox is there, regardless of how it's labeled. We can only give the checkbox a label that describes the functionality as good as possible. And personally, I think "Reset photo orientation" does not do that. Without any further knowledge (as an average user) I can't directly associate any functionality with the "Reset photo orientation" label. For example on import from the camera, will it undo the rotation of my images (which I can already see rotated on my camera)? And what is the value after such a 'reset'? The user is simple unable to know from the label only, that activating this checkbox will result in a transformation of the image data. The same goes for the rotation dialog. Why would I want to use a reset here, when there is already a button for that? Why would I want to reset the transformations I just made with the buttons? I admit that "Adjust photo orientation" is not 100% perfectly, but it describes its purpose quite well (an extra tooltip could already suffice to clear the confusing here). > I know I originally supported tag-change-only as the default mode, but after > seeing how unevenly exif is supported, I think we should aim to avoid > unnecessary incompatibilities for the user. If compatibility with other exif-unaware applications is the problem, it's only a matter of changing some default values in my patch. I don't mind to change that. See the attached patch for the required changes.
Jef, OK, I'm willing to look for a compromise... both proposals so far are pretty lame... but a big part of my reluctance to mess with the labels is that Paolo has already approved (actually it was his suggestion) "Reset photo orientation", and it seems silly to make the effort of patching if the end result is only slightly less lame :-) Anyway, some ideas (just brainstorming): "Use most-compatible mode" "Use most-compatible mode (reset tags)" "Use tag-only mode (advanced)" "Use Exif tags (advanced)" "Exif tag change mode (advanced)" These are short(ish) and more technical, but that is probably a good thing... the non-technical labels so far have been almost meaningless. I think "Use Exif tags (advanced)" is my personal favorite from the above list. It discourages new users from using it. Do you like it? If you want to patch with these or something similar, can you base your patch on my patch 74346? It incorporates your schema corrections, so it is just a matter of renaming the preferences and/or keys, possibly swapping TRUE/FALSEs, and updating the documentation of the rotation checkbox. My patch has a few useful improvements to the rotation code flow, error reporting, and corrections to the pop-up menus. - Mike
Actualy I think "reset photo orientation" is confusing, I suggest to revert to "reset EXIF orientation". I know this is a technical term, but at least technical users will understand what it is :) Michael, instead of Tools_JPEGRotate_Auto in the popup menu we could have a Tools_ResetExifOrientation_Auto command which simply resets the exif orientation of the selected images.
Paolo, Do you want the Tools_ResetExifOrientation_Auto command to always reset to top-left, or offer all the eight possible orientations? "Reset Exif Orientation" works for me... In the meantime, can you commit patch 74346, so I can use it as a base to work from? - Mike
Can we settle for middle ground? How about "Reset orientation tag." Clear, but not technical.
...always reset to top-left and rotating phisically the image according to the exif orientation. Patch applied.
Created attachment 74481 [details] [review] New rotation code flow (without jpeg mcu handling) (In reply to comment #149) > ...always reset to top-left and rotating phisically the image according to the > exif orientation. This describes the functionallity of the checkbox much better than "reset exif/photo orientation". But for the roation dialog, I would leave the last part ("image according to the exif orientation") out, because that is not true anymore once you apply a manual transformation by means of the buttons. (I would also remove the first part, but I think you already knew that. Not only because I think it's only an implementation detail, but otherwise the label gets simply too long. We can always give more detailed information in a tooltip.) This "physical transformation (according to the exif orientation)" label (or some variation of that) has my support. (In reply to comment #145) > My patch has a few > useful improvements to the rotation code flow, error reporting, and corrections > to the pop-up menus. Error reporting and corrections to the pop-up menus looks good. But I have some comments on the new rotation code flow. First, the current code is inefficient and could use some improvements (see also my comment 105): update_orientation_field (path, data->rot_data); if (eel_gconf_get_boolean (PREF_ROTATE_RESET_EXIF_ORIENTATION, TRUE)) { update_rotation_from_exif_data (path, data->rot_data); apply_transformation_jpeg (window, path, data->rot_data); } What this does now, is updating the existing exif orientation tag in the file (update_orientation_field), reading the result again (update_rotation_from_exif_data) for the jpeg transform (apply_transformation_jpeg) and finally resetting the tag to top-left (reset_orientation_field inside apply_transformation_jpeg). All these functions (with the exception of update_rotation_from_exif_data) rewrite the original file (or create a temporary file). This means a new/temporary file is created at least 3 times (4 in case both a rotation and a mirror/flip operation is performed). For large images and/or slow disks/networks this process takes more time then necessary. The first update_orientation_field can be skipped if the update_rotation_from_exif_data function is modified to really update the requested rotation with the data from exif orientation tag (and not simply read the orientation tag like it does now). if (eel_gconf_get_boolean (PREF_ROTATE_RESET_EXIF_ORIENTATION, TRUE)) { update_rotation_from_exif_data (path, data->rot_data); apply_transformation_jpeg (window, path, data->rot_data); } else { update_orientation_field (path, data->rot_data); } As a side note: Now that libexif is mandatory, we have two structures to specify a transformation: RotationData (from rotation-utils.h) and GthExifOrientation (from gth-exif-utils.h). Maybe it would be a lot easier to keep only one of them, in combination with GthTransform (from typedefs.h) directly. Second, I have two problems with the current jpeg minimal coding unit handling (introduced in comment 142): 1. In the case there is no exif orientation tag present (and thus cannot be used to perform the transformation), data corruption will always happen. There is no solution here. 2. In the case there is an exif orientation tag present, the transformation is always performed by adjusting this tag, even if the user explicitly requested a physical transformation. I think ignoring the user's preference is a very bad choice. For example, I would use gthumb in exif-tranformation-mode, to keep my pictures as intact as possible. Only for creating a picture cd/dvd for showing on other equipment (standalone dvd player, unknown viewer on another pc), I would choose the physical transform for maximum compatibility. But now gthumb refuses to rotate them properly (if the image dimensions are not multiples of 8). Both cases will also make it very difficult to predict the outcome of the transformation, especially when rotating many images at once. Some possible solutions: 1. Use the lossless jpeg transform anyway and inform the user about the possible corruptions that can occur. This could be done by adding an additional small note to the rotation dialog, and a more detailed explanation in the documentation. This note could be something like: "JPEG transformations are lossless. However, if the image dimensions are not multiples of 8, some (reversible) data corruption can occur." Another method is showing a warning message to the user, but only when one or more non 8x8 aligned images are detected in the selection. This is certainly less scary and we have the possibility to inform the user about the alternatives (e.g. generic transformation or exif transformation), even if the user did not read the documentation. If the user still wants to go on with the lossless jpeg transform, he can still do that in both cases. 2. Use the generic transformation for non 8x8 aligned jpeg images. This introduces no artifacts, but is not lossless of course. All this can be combined in a very clean (pseudo) code flow: if (image_has_exif_support && !pref_reset_orientation) // Exif transform else if (image_is_jpeg [&& no_mcu_size_problems]) // Lossless jpeg transform else // Generic transform For the moment (without tiff support), image_has_exif_support would be equivalent with image_is_jpeg && exif_orientation_tag_present. But once tiff support is added to the update_orientation_field function, we only have to remove the image_is_jpeg part. The no_mcu_size_problems check will depend on the discussion above and can even be implemented as a preference. I have implemented some of these ideas as a proof of concept in the attached patch. For the moment, I left out the jpeg mcu handling code.
Jef, Thanks for the rotation code flow improvements! I have to study them over the next couple of days to fully understand them, but you are certainly right that the existing code was wasteful with file accesses. Regarding the 8x8 issue: I think there are two reasonable ways to go here: 1) Add a warning that the selected mode is being over-ridden to avoid data loss. (I had planned to add this, at some point). This is easy, and stops the user from selecting a data-loss mode. Whether you like this or not depends how much freedom you want to grant the user. (I'm OK with a few constraints - can't please everyone anyways). 2) Add the equivalent of the jpegtran -trim option - i.e., lop off the few rows/columns outside the 8x8 boundaries (after a warning). Many other image viewers offer to do this (xnview, for example). This is more work, though, for diminishing returns... I think I really prefer #1. This is an obscure edge case, and #1 is an easy way of stopping the user from losing data. Yet another label proposal - can everyone live with: "Use physical transforms"? Maybe we should have a 2.9.0 release now, to start getting functional exif code out there? - Mike
or maybe "Apply physical transforms"?
Other notes: 1. Bugzilla doesn't have 2.8.x and 2.9.x tags yet. Paolo, can you add them? 2. I've opened Bug 361416 – Reset Exif orientation tag tool, to discuss the reset tool. - Mike
Created attachment 74492 [details] [review] Adds tool-tips, improves manual, changes UI label. This patch does 3 things: 1. Changes the UI label to "Apply physical transform". I think this gets to the crux of what exactly the checkbox does. And if it doesn't: 2. Detailed tool-tips have been added to the checkboxes. They are different for the import and rotation dialogs. Please have a look. 3. The manual documentation has been clarified a bit. This patch should co-exist peacefully with Jef's experimental optimization patch. Paolo, if you are OK with the "Apply physical transform" label, please commit it. - Mike
Patch was committed - thank you! - Mike
Jef, Have you tested your patch on a variety of images? It just doesn't seem to work for me. Nothing is rotating correctly, really. - Mike
Created attachment 74503 [details] [review] Patch to update README file. This patch updates the README file, reflecting the mandatory libexif and libjpeg requirements, as well as a few general updates. Nothing controversial. Please commit. - Mike
Created attachment 74559 [details] [review] Adds Tool > Reset Exif orientation This roll-up patch does the following: 1. Adds a Tool > Reset Exif orientation function, for fixing images incorrectly imported and auto-rotated by previous versions of gthumb. 2. Updates the README file, indicating the correct library requirements. 3. Updates the NEWS file, to reflect the exif improvements. 4. Makes the short command line options work (trivial patch from bug 349747) 5. Makes print scaling work (trivial patch from bug 353681). The reset tool works nicely, though it could be prettier. Two nice files are added to implement the reset function: src/dlg-reset-exif.c src/dlg-reset-exif.h This should be a straightforward patch. Please review/commit. Thanks! - Mike
Created attachment 74560 [details] [review] Adds Tool > Reset Exif orientation (corrected) Sorry, the previous patch had a small typo in it. This is the corrected patch. This roll-up patch does the following: 1. Adds a Tool > Reset Exif orientation function, for fixing images incorrectly imported and auto-rotated by previous versions of gthumb. 2. Updates the README file, indicating the correct library requirements. 3. Updates the NEWS file, to reflect the exif improvements. 4. Makes the short command line options work (trivial patch from bug 349747) 5. Makes print scaling work (trivial patch from bug 353681). The reset tool works nicely, though it could be prettier. Two new files are added to implement the reset function: src/dlg-reset-exif.c src/dlg-reset-exif.h This should be a straightforward patch. Please review/commit. Thanks! - Mike
(In reply to comment #150) > As a side note: Now that libexif is mandatory, we have two structures to > specify a transformation: RotationData (from rotation-utils.h) and > GthExifOrientation (from gth-exif-utils.h). Maybe it would be a lot easier to > keep only one of them, in combination with GthTransform (from typedefs.h) > directly. I have openened a bug 361701 for this and attached a patch there. (In reply to comment #154) > Adds tool-tips, improves manual, changes UI label. Looks good to me... (In reply to comment #156) > Have you tested your patch on a variety of images? It just doesn't seem to work > for me. Nothing is rotating correctly, really. I tested with a few jpegs and pngs, both in exif-mode and physical-transform-mode. Maybe I made a mistake somewhere. I'll look at it again. (In reply to comment #159) > Created an attachment (id=74560) [edit] > Adds Tool > Reset Exif orientation (corrected) > > This should be a straightforward patch. Please review/commit. Thanks! I'll test in the next days...
The reset tool patch has been committed. Thank you, Paolo! I'm closing this bug, because we now have fully functional and usable exif orientation code in cvs. This should appear in releases 2.9.0 and later. Paolo, could you make a 2.9.0 release now? Jef is working to improve the efficiency and elegance of the code - see bug 361701. Technical discussions can continue there. I believe that this is the only remaining activity linked to this bug. If you see other issues with the new exif code, please file a new bug for it. Thanks everyone! - Mike
Created attachment 74623 [details] The document mentioned in comment 100 I don't see any issues left in this bug. Except for one thing. If it's possible, I would commit patch 73884 also to the gthumb-2-8 branch. That will stop the creation of more images with an incorrect exif orientation tag. Once the newer gthumb (with proper exif support) is released, the transition to the new version will be easier (e.g. less images to fix). All the rest is not directly related with this bug: 1. JPEG lossless transform issues (mcu related) For more information see comment 150 and 151. See also bug 329129. We'll still have to come up with some kind of warning message and/or implement the equivalent of the jpegtran -trim option. Implementing that last part is quite easy, because the necessary code is already present. we only need to 'activate' it. 2. Improvements to the efficiency of the code Some parts are already done (bug 361701), but there is still room for some improvements (see comment 150). 3. JPEG code inside src is not removed from CVS. We moved the jpeg support code from src to libgthumb. But in the CVS repository, this code still exists. I also attached the document mentioned in comment 100.
I've applied the patch to the gthumb-2-8 branch, and removed src/jpegutils/* and src/gth-exif-utils.[ch] from CVS (did I missed some other file to remove?)
And I've opened Bug 361913 – Need to improve rotation code efficiency. Let's carry on there. Paolo, I think that you have removed everything that needed to be removed. - Mike
One last question... does the physical rotation code update the Exif-embedded thumbnail, if present? Should it? - Mike
(In reply to comment #164) > And I've opened Bug 361913 – Need to improve rotation code efficiency. Let's > carry on there. I'll take a look at it there. (In reply to comment #165) > One last question... does the physical rotation code update the Exif-embedded > thumbnail, if present? Should it? The thumbnail is not modified. I don't know if the thumbnail is actually used by some applications/cameras. If it is, I think we should update it also. The exiftran utility does this by default.
(In reply to comment #166) > > One last question... does the physical rotation code update the Exif-embedded > > thumbnail, if present? Should it? > > The thumbnail is not modified. I don't know if the thumbnail is actually used > by some applications/cameras. If it is, I think we should update it also. The > exiftran utility does this by default. Bug 334487 suggests that the thumbnail is used by some applications... that's probably an appropriate bug report to post follow-ups or patches to. - Mike
There are some loose ends that still need fixing - the image modification and web album tools aren't handling exif orientation tags correctly. See bug 370013. - Mike
*** Bug 406610 has been marked as a duplicate of this bug. ***
Another follow-up bug for exif orientation issues: Bug 428725 – What to do with broken Nautilus thumbnails? - Mike
All applications that are sharing the thumbnail cache are affected by this problem. I wrote a more detailed comment on the issue in bug 390266 comment #2 (for the EOG viewer).