Thumbnails look good here (they match with the thumbnails in gthumb). Only the original images are the not autorotated here.

Actually, I think we have bigger troubles... the image re-sizing code doesn't handle the exif tags properly. We'll have to check all of the image modification tools...

Boosting severity. We have to fix this stuff before we can have a new release.

- Mike

I think _gdk_pixbuf_save_as_jpeg in libgthumb/pixbuf-utils.c needs to call write_orientation_field (filename, GTH_TRANSFORM_NONE).

- Mike

Hmm... I'm not having much luck unraveling this.

Anyway, the easiest way to demonstrate the problem is simply to use "File > Save As" on a jpeg with an orientation tag that is not equal to "top left".

- Mike

I've committed a fix, by adding:

write_orientation_field (dest_filename, GTH_TRANSFORM_NONE);

after each:

jpeg_data_save_file

in src/catalog-web-exporter.c, src/gth-browser.c, and src/gth-viewer.c.

I think that fixes things. Please test.


- Mike

The fix seems to work fine here. However, it is not implemented really efficient. You used the write_orientation_field function, which does rewrite the jpeg file. I think it is possible to modify the exif data before saving the jpeg.

I realize that, but it works well for the first iteration.

I'll be taking another look at it sometime anyway, because the "save_jpeg_data" routine (where the write_orientation_field was added) is defined in both gth-browser.c and gth-viewer.c - pointless duplication of code.

I think read/write_orientation_field routines should be moved into libgthumb/gth-exif-utils.c, and 

save_jpeg_data (GthBrowser *browser, const char *filename)
save_jpeg_data (GthViewer  *viewer, const char *filename)

should be moved there as well, and be redefined as:

save_jpeg_tags_and_reset_orientation (const char *filename, ExifData *exifdata, IptcData *iptcdata)

or something like that. Pending some spare time...


- Mike

Maybe it could be created a new function set_orientation_in_exif_data with the
content of the "for statement" from write_orientation_field.

Is some doing something?

Manuel,

Yes, I think that is a good idea. Can you go ahead and make new save_jpeg_tags_and_reset_orientation and set_orientation_in_exif_dat functions in libgthumb/gth-exif-utils.c? 

- Mike

Created attachment 76215 [details] [review]
set orientation in exif data function

I didn't remove the duplicated code, the reason was that the iptc library is optional.

Maybe is better to leave this for later.

Thanks Manuel! I've committed the patch. I don't understand what you mean about the iptc library. Why does that stop us from removing the duplicated routines?

Also... there is still another issue to figure out. If you select "copy original images", but do not resize the images, then gthumb just copies the file directly.

This normally OK, except that most browsers (like Firefox 2.0) do not support the exif orientation tag, so it isn't oriented correctly on the browser if it has an orientation tag.

I think we need to read the original exif orientation tag, and then use:

apply_transformation_jpeg (window, path, data->transform, FALSE);

step for each file in this case. 

Jef, what do you think? Should this be optional (a UI pref?) or mandatory?


- Mike

Manuel,

can you create a patch that modifies export__copy_image in src/catalog-web-exporter.c, so that for each item in dest_list, it will read the orientation tag, and then apply the physical transform required to reset the tag to top-left?

That is, something like

transform = read_orientation_field (const char *path);
apply_transformation_jpeg (window, path, transform, FALSE);
                           ^^^^^^
                           Problem?

- Mike

(In reply to comment #11)
> I don't understand what you mean about
> the iptc library. Why does that stop us from removing the duplicated routines?

Since the library is optional, you can't have a function with a IptcData parameter, at least not without some #ifdef's.

> Also... there is still another issue to figure out. If you select "copy
> original images", but do not resize the images, then gthumb just copies the
> file directly.
> 
> This normally OK, except that most browsers (like Firefox 2.0) do not support
> the exif orientation tag, so it isn't oriented correctly on the browser if it
> has an orientation tag.
> 
> I think we need to read the original exif orientation tag, and then use:
> 
> apply_transformation_jpeg (window, path, data->transform, FALSE);
> 
> step for each file in this case.

That sounds good to me, because the exif orientation tag is not the only tag that needs to be updated (image dimensions, thumbnail). The transformation code does that already (partially).

BTW, in the current code, the exif data from the GthBrowser/GthViewer structure is modified. I think that will cause suprising side effects when modifying the image a second time (but I didn't check that). Using the transform function does not have this (potential) problem.

> Jef, what do you think? Should this be optional (a UI pref?) or mandatory?

I think mandatory is the best option for exporting images to a webalbum.

(In reply to comment #13)
> Since the library is optional, you can't have a function with a IptcData
> parameter, at least not without some #ifdef's.

Sorry, I figured that out right after posting...

> That sounds good to me, because the exif orientation tag is not the only tag
> that needs to be updated (image dimensions, thumbnail). The transformation code
> does that already (partially).

Well, the image dimensions don't change in this case (this is only an issue when resizing is not selected). Just the orientation and thumbnail should change, I guess. (If the image is resized, a different routine is used and the image is saved from the pixbuf, rather than copying the original file.)

> BTW, in the current code, the exif data from the GthBrowser/GthViewer structure
> is modified. I think that will cause suprising side effects when modifying the
> image a second time (but I didn't check that). Using the transform function
> does not have this (potential) problem.

Hmm... I think you are right. There is some oddness after you modify an image (without saving yet) and then use tools>rotate. I have to study that...

> > Jef, what do you think? Should this be optional (a UI pref?) or mandatory?
> 
> I think mandatory is the best option for exporting images to a webalbum.

Yes, agreed.

- Mike

> > is modified. I think that will cause suprising side effects when modifying the
> > image a second time (but I didn't check that). Using the transform function
> > does not have this (potential) problem.
> 
> Hmm... I think you are right. There is some oddness after you modify an image
> (without saving yet) and then use tools>rotate. I have to study that...

Actually... I think the current code is safe if you successively use different "Image" menu tools (which operate on the pixbuf in memory, which the image loader has transformed to top-left format), but things get confusing if you call Tools>Rotation without saving first, because the screen will show an unsaved pixbuf, but the rotation tool will operate on the original saved file. Maybe we need to add a check for unsaved pixbufs before using the lossless rotation tool?

- Mike

(In reply to comment #13)
> (In reply to comment #11)
>
> > Also... there is still another issue to figure out. If you select "copy
> > original images", but do not resize the images, then gthumb just copies the
> > file directly.
> > 
> > This normally OK, except that most browsers (like Firefox 2.0) do not support
> > the exif orientation tag, so it isn't oriented correctly on the browser if it
> > has an orientation tag.
> > 
> > I think we need to read the original exif orientation tag, and then use:
> > 
> > apply_transformation_jpeg (window, path, data->transform, FALSE);
> > 
> > step for each file in this case.
> 
> That sounds good to me, because the exif orientation tag is not the only tag
> that needs to be updated (image dimensions, thumbnail). The transformation code
> does that already (partially).
> 
> BTW, in the current code, the exif data from the GthBrowser/GthViewer structure
> is modified. I think that will cause suprising side effects when modifying the
> image a second time (but I didn't check that). Using the transform function
> does not have this (potential) problem.
> 
> > Jef, what do you think? Should this be optional (a UI pref?) or mandatory?
> 
> I think mandatory is the best option for exporting images to a webalbum.

I agree.  If there's a pref for this at all, make it a hidden one.  We don't need to clutter up the export dialog with a setting for this.

Rennie

Hmm...

1) I think we need to modify:

gth_window_activate_action_tools_jpeg_rotate
gth_window_activate_action_tools_jpeg_rotate_right
gth_window_activate_action_tools_jpeg_rotate_left

to include something like

        if (gth_window_get_image_modified (window)) {
                GthViewer *viewer = (GthViewer*) window;
                ask_whether_to_save (viewer, load_image__image_saved_cb);
        }                                    ^^^ not sure about this

or something similar, to catch unexpected pixbuf/file differences. 


2) The export__copy_image function needs something like:

transform = read_orientation_field (const char *path);
apply_transformation_jpeg (window, path, transform, FALSE);

Manuel, can you come up with a patch for #1 and #2?

3) Jef: do you want the set_orientation_in_exif_data changes reverted? If so, maybe upload a patch to show what you think is safest... you have a better grip on this stuff than me.


This stuff is never-ending...


- Mike

(In reply to comment #13)
> BTW, in the current code, the exif data from the GthBrowser/GthViewer structure
> is modified. I think that will cause suprising side effects when modifying the
> image a second time (but I didn't check that). Using the transform function
> does not have this (potential) problem.

It looks that is a problem.
To make a copy of the exif data first could solve.


(In reply to comment #17)
> Manuel, can you come up with a patch for #1 and #2?

I don't want to break anything, again.
This is being confuse to me, i need to see what is going on.

> I don't want to break anything, again.
> This is being confuse to me, i need to see what is going on.

Manuel,

Don't worry - I don't think you broke anything! I don't think the current approach actually causes any problems, although it might in the future. (The tool>rotation issue would be a problem either way). But if we can think of a safer approach, it's probably worth doing.

And yes - the exif tag stuff is confusing - you missed the fun of bug 343867 :-)

- Mike

(In reply to comment #14)
> > That sounds good to me, because the exif orientation tag is not the only tag
> > that needs to be updated (image dimensions, thumbnail). The transformation code
> > does that already (partially).
> 
> Well, the image dimensions don't change in this case (this is only an issue
> when resizing is not selected). Just the orientation and thumbnail should
> change, I guess. (If the image is resized, a different routine is used and the
> image is saved from the pixbuf, rather than copying the original file.)

I was not referring to adjusting the dimensions in the sense of resizing, but to swapping horizontal and vertical tags for certain transformations. And there are also some other tags that we could update (see also bug 343867 comment 77).

We should centralize that functionality in one place only, otherwise maintaining it will be a nightmare.

(In reply to comment #17)
> 1) I think we need to modify:
> 
> gth_window_activate_action_tools_jpeg_rotate
> gth_window_activate_action_tools_jpeg_rotate_right
> gth_window_activate_action_tools_jpeg_rotate_left
> 
> to include something like
> 
>         if (gth_window_get_image_modified (window)) {
>                 GthViewer *viewer = (GthViewer*) window;
>                 ask_whether_to_save (viewer, load_image__image_saved_cb);
>         }                                    ^^^ not sure about this
> 
> or something similar, to catch unexpected pixbuf/file differences. 

That applies to all functions under the Tools menu. Because as far as I know, they all operate on files, rather than the active pixbuf.

> 
> 2) The export__copy_image function needs something like:
> 
> transform = read_orientation_field (const char *path);
> apply_transformation_jpeg (window, path, transform, FALSE);

That looks good, except for the fact that apply_transformation_jpeg can only overwrite a file, not copy. The underlying jpegtran can be used to perform a simple copy (with JXFORM_NONE) AND adjusting the exif tags at the same time (when my patch is finished). I don't know if there is a drawback compared to using the jpeg_data_* functions (which we are using now in many places).

> 3) Jef: do you want the set_orientation_in_exif_data changes reverted? If so,
> maybe upload a patch to show what you think is safest... you have a better grip
> on this stuff than me.

It doesn't matter for the moment.

Created attachment 76635 [details] [review]
Changes orientation-tag update method.

To address Jef's concerns about possibly unintended consequences of modifying the orientation tags of pixbufs in memory, I have partly reverted an earlier patch.

Now, the "File>Save As" functions use the "safer" file-based write_orientation_field function to reset the orientation tag. This is less efficient, but it does not mess with the in-memory exif data associated with the pixbuf. This wasn't a current source of problems, but it might cause unexpected issues in the future, because the data is persistent.

The web exporter still uses the new memory-based set_orientation_in_exif_data function, because the exif data that it modifies is not persistent. The elimination of a file save is very helpful in speeding up album generation, too.

There are still some outstanding issues here (see comment 17):

1) The export__copy_image function needs a reset tag step.
2) The Tool functions should force a save/cancel if there are unsaved pixbufs selected.

- Mike

(In reply to comment #20)
> > 2) The export__copy_image function needs something like:
> > 
> > transform = read_orientation_field (const char *path);
> > apply_transformation_jpeg (window, path, transform, FALSE);
> 
> That looks good, except for the fact that apply_transformation_jpeg can only
> overwrite a file, not copy. The underlying jpegtran can be used to perform a
> simple copy (with JXFORM_NONE) AND adjusting the exif tags at the same time
> (when my patch is finished). I don't know if there is a drawback compared to
> using the jpeg_data_* functions (which we are using now in many places).

Jef,

Can you take a look at export__copy_image and offer a patch to it to add the transform-to-top-left step?

It's beyond my skill - the existing export__copy_image function in src/catalog-web-exporter.c is asynchronous, and uses progress callbacks. We could use the underlying jpegtran to copy as you suggest, but there would be no progress dialogs (I guess).

- Mike

Never mind the above plea for help. I'm figuring it out...

- Mike

Created attachment 76706 [details] [review]
Transforms "original images" to top-left in web album export

OK, here is my patch to transform copied original images back to top-left.

The original export__copy_image function used an asynchronous copy-list function (gnome_vfs_async_xfer). I don't understand why a list function was used. I changed it to a single-item non-async function (so I could be sure when the new file was ready for transformation), gnome_vfs_xfer_uri.

Jef (and anyone else), I'd appreciate any feedback on the patch. I'm sure it could be more elegant, but my programming skills aren't that dazzling. It seems to work, anyway.

- Mike

I've committed a slightly modified version of this patch.

I'm opening a new bug 376308 for the save-before-tool-usage issue.

Closing this as fixed.


- Mike