After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 361913 - Need to improve rotation code efficiency
Need to improve rotation code efficiency
Status: RESOLVED FIXED
Product: gthumb
Classification: Other
Component: general
2.9.x
Other Linux
: Normal normal
: ---
Assigned To: Paolo Bacchilega
Paolo Bacchilega
: 334487 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2006-10-13 11:41 UTC by Michael Chudobiak
Modified: 2007-05-25 19:30 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Improves rotation code flow (3.54 KB, patch)
2006-10-13 13:08 UTC, Michael Chudobiak
none Details | Review
Improves rotation code flow, and adds MCU warning. (4.41 KB, patch)
2006-10-13 15:07 UTC, Michael Chudobiak
none Details | Review
Patch to add support for the jpegtran trim option (2.02 KB, patch)
2006-10-18 14:33 UTC, Jef Driesen
none Details | Review
Update for the new rotation code (5.78 KB, patch)
2006-10-18 14:52 UTC, Jef Driesen
none Details | Review
Jef's improvements and documentation updates (9.69 KB, patch)
2006-10-19 15:27 UTC, Michael Chudobiak
none Details | Review
Update for the new rotation code (4.45 KB, patch)
2006-10-23 14:53 UTC, Jef Driesen
none Details | Review
Add warning dialog for odd-mcu images (6.81 KB, patch)
2006-10-30 22:27 UTC, Jef Driesen
none Details | Review
Better odd-mcu detection (7.70 KB, patch)
2006-11-02 18:12 UTC, Jef Driesen
none Details | Review
Cancel the transform when closing the mcu dialog (522 bytes, patch)
2006-11-09 20:32 UTC, Jef Driesen
none Details | Review
Adjust the exif tags directly inside the jpegtran function (8.65 KB, patch)
2006-11-09 20:58 UTC, Jef Driesen
none Details | Review
Transform the thumbnail data. (8.42 KB, patch)
2006-11-10 23:23 UTC, Jef Driesen
none Details | Review
Fixes error in multiple exif-tag-rotation of series (2.20 KB, patch)
2006-11-12 21:40 UTC, Michael Chudobiak
none Details | Review
Fix small leak inside jpegtran (2.34 KB, patch)
2006-11-15 08:27 UTC, Jef Driesen
none Details | Review
Transform the thumbnail data (v2) (10.69 KB, patch)
2006-11-15 09:55 UTC, Jef Driesen
none Details | Review
Transform the thumbnail data (v3) (8.09 KB, patch)
2006-11-23 15:03 UTC, Jef Driesen
none Details | Review
Transform the thumbnail data (v4) (1.81 KB, patch)
2006-11-24 11:24 UTC, Jef Driesen
none Details | Review
Callback implementation for the jpeg mcu detection (12.35 KB, patch)
2007-01-21 14:50 UTC, Jef Driesen
none Details | Review

Description Michael Chudobiak 2006-10-13 11:41:38 UTC
This is a follow-up bug to:

Bug 343867 – Rotating image should alter EXIF data.
Bug 361701 – Cleanup of the transformation code.

The existing rotation code works properly, but requires too many file accesses to be considered efficient. This needs to be fixed. Jef Driesen has described some approaches in http://bugzilla.gnome.org/show_bug.cgi?id=343867#c150, and will hopefully upload a new patch here :-)

Also, the current jpeg MCU handling needs improvement. I think the rotation code must at a minimum:

1. Warn the user that lossless physical transform is impossible
2. Default to rotation by tag change (i.e., mode override)

and optionally

3. Offer the trim mode.


- Mike
Comment 1 Michael Chudobiak 2006-10-13 12:00:45 UTC
Just for reference, we currently have (in edited form):

  if (image_is_jpeg (path)) {
R    if (!get_exif_tag_short(path,EXIF_TAG_ORIENTATION)) {
          apply_transformation_jpeg (window, path, data->transform);
      }
      else {
RW        update_orientation_field (path, data->transform);
          if (eel_gconf_get_boolean (PREF_ROTATE_RESET_EXIF_ORIENTATION, TRUE)) {
R             gdk_pixbuf_get_file_info(...snip...);
              if (!(height%8) && !(width%8)) {
R             data->transform = read_orientation_field (path);
W             apply_transformation_jpeg (window, path, data->transform);
              }
          }
      }
  }
else
    apply_transformation_generic (window, path, data->transform);


The R/W mark reads/writes for a jpeg that is rotated physically. Yuck. We could do a lot better by keeping the same basic outline, but passing orientation tag along as a parameter rather than reading / writing it at every step.

- Mike

Comment 2 Michael Chudobiak 2006-10-13 13:08:10 UTC
Created attachment 74631 [details] [review]
Improves rotation code flow

Here is my attempt at improving rotation code flow - patch above, and summarized code is below for comparison:

    if (image_is_jpeg (path)) {
R       orientation = get_exif_tag_short(path,EXIF_TAG_ORIENTATION);
        if (orientation) {
R           gdk_pixbuf_get_file_info(get_file_path_from_uri (path),&width,&height);
            if (eel_gconf_get_boolean (PREF_ROTATE_RESET_EXIF_ORIENTATION, TRUE)
                    && !(height%8) && !(width%8)) {
                data->transform = get_next_transformation(orientation, data->transform);
W               apply_transformation_jpeg (window, path, data->transform);
            }
            else {
                update_orientation_field (path, data->transform);
            }
        }
        else {
            apply_transformation_jpeg (window, path, data->transform);
        }
    }
    else
        apply_transformation_generic (window, path, data->transform);


Please test. (Don't commit yet, please - needs testing!)

- Mike
Comment 3 Michael Chudobiak 2006-10-13 13:09:53 UTC
If you try to build against CVS, beware of possibly broken makefile, bug 361927.
Comment 4 Michael Chudobiak 2006-10-13 13:44:19 UTC
Next step - add MCU warning dialog.
Comment 5 Michael Chudobiak 2006-10-13 15:07:40 UTC
Created attachment 74638 [details] [review]
Improves rotation code flow, and adds MCU warning.

New patch. This reduces read/write accesses like the previous patch, but adds two different warnings to cover MCU problems. It seems to work well, and is considerably more efficient.

I think this patch can be committed - but Paolo can you check that I've done internationalization properly?
Comment 6 Paolo Bacchilega 2006-10-13 17:48:23 UTC
patch applied to CVS, thanks.
Comment 7 Jef Driesen 2006-10-18 14:33:34 UTC
Created attachment 74954 [details] [review]
Patch to add support for the jpegtran trim option

(In reply to comment #0)
> 3. Offer the trim mode.

Implementing the jpegtran trim mode is quite easy, because the necessary code is already present. With this patch the new mode is added as a new parameter to the jpegtran function, but remains disabled for now.
Comment 8 Jef Driesen 2006-10-18 14:52:21 UTC
Created attachment 74956 [details] [review]
Update for the new rotation code

(In reply to comment #5)
> New patch. This reduces read/write accesses like the previous patch, but adds
> two different warnings to cover MCU problems. It seems to work well, and is
> considerably more efficient.

There is a potential source for errors in update_orientation_field. If different IFD's contain different orientation values, they are all updated (resulting in different values again), instead of writing only one identical value. (I even remember seeing a related bug for gthumb.)

I removed the reset/update_orientation_field functions with one generic write_orientation_field function. reset_orientation_field functionallity can be obtained by passing GTH_TRANSFORM_NONE to the new function. The update_orientation_field function should be replaced by a call to get_next_transformation before the new function. The usage of the new function is also more consistent with the apply_transformation_* functions, because they all need a get_next_transformation function call too.
Comment 9 Michael Chudobiak 2006-10-18 15:40:25 UTC
A quick test looks good for both patches. I don't actually understand the "different IFD" problem, though. Can you explain what the problem is in greater detail?

Some suggestions:

1) For clarity (and future maintainability) a comment should be added in the GthTransform typedef that explains that these are the names of transforms corresponding to each Exif orientation value (1-8) required to return the orientation value to 1 without changing the displayed image. It's a bit confusing because GthTransform and ExifShort are used interchangeably in places (can that be improved?).

2) The get_next_value... functions should use GthTransform values (like GTH_TRANSFORM_FLIP_V) rather than hard-coded numbers (like "4").

3) One new line in the rotation function needs an extra tab (the patch is mis-formatted).


- Mike


Comment 10 Jef Driesen 2006-10-19 14:23:56 UTC
(In reply to comment #9)
> A quick test looks good for both patches. I don't actually understand the
> "different IFD" problem, though. Can you explain what the problem is in greater
> detail?

In the Exif data there can be multiple so called IFD: 0th (Primary Image Data), 1st (Thumbnail Data), Exif, GPS and Interoperability IFD. Some tags can be present in more then one such IFD. If such a tag is changed, it should be updated in every IFD.

I also found the bug again that I mentioned in comment 8. It's bug 333102.

> 1) For clarity (and future maintainability) a comment should be added in the
> GthTransform typedef that explains that these are the names of transforms
> corresponding to each Exif orientation value (1-8) required to return the
> orientation value to 1 without changing the displayed image. It's a bit
> confusing because GthTransform and ExifShort are used interchangeably in places
> (can that be improved?).

The comment is certainly a good idea.

The reason why I left the ExifShort in some places, is that GthTransform contains no '0' value. For places where a GthTransform is used, that is fine because all possible orientations/transforms are already covered. But the get_exif_tag_short function can also return zero, to indicate some error (no exif data or tag in the file). We use that in the rotation code to detect if a transform by means of the exif orientation tag is supported. The recommended read_orientation_field will always return a valid GthTransform, but is therefor not suitable to do that detection anymore. To be on the safe side, I made sure that all functions that accept a GthTransform treat invalid input as GTH_TRANSFORM_NONE.

Originally I had two enums, one GthExifOrientation (more or less the current GthTransform with names matching the exif names, plus a zero value) and one GthTransform, which was defined in terms of GthExifOrientation (or the other way around of course):

typedef enum { /*< skip >*/
	GTH_EXIF_ORIENTATION_NONE = 0,
	GTH_EXIF_ORIENTATION_TOP_LEFT,
	GTH_EXIF_ORIENTATION_TOP_RIGHT,
	GTH_EXIF_ORIENTATION_BOTTOM_RIGHT,
	GTH_EXIF_ORIENTATION_BOTTOM_LEFT,
	GTH_EXIF_ORIENTATION_LEFT_TOP,
	GTH_EXIF_ORIENTATION_RIGHT_TOP,
	GTH_EXIF_ORIENTATION_RIGHT_BOTTOM,
	GTH_EXIF_ORIENTATION_LEFT_BOTTOM
} GthExifOrientation;

typedef enum { /*< skip >*/
	GTH_TRANSFORM_NONE = GTH_EXIF_ORIENTATION_TOP_LEFT,
	GTH_TRANSFORM_ROTATE_90 = GTH_EXIF_ORIENTATION_RIGHT_TOP,
	GTH_TRANSFORM_ROTATE_180 = GTH_EXIF_ORIENTATION_BOTTOM_RIGHT,
	GTH_TRANSFORM_ROTATE_270 = GTH_EXIF_ORIENTATION_LEFT_BOTTOM
	GTH_TRANSFORM_FLIP_H = GTH_EXIF_ORIENTATION_TOP_RIGHT,
	GTH_TRANSFORM_FLIP_V = GTH_EXIF_ORIENTATION_BOTTOM_LEFT,
	GTH_TRANSFORM_TRANSPOSE = GTH_EXIF_ORIENTATION_LEFT_TOP,
	GTH_TRANSFORM_TRANSVERSE = GTH_EXIF_ORIENTATION_RIGHT_BOTTOM,
} GthTransform;

But I dropped that in my final patch.

> 2) The get_next_value... functions should use GthTransform values (like
> GTH_TRANSFORM_FLIP_V) rather than hard-coded numbers (like "4").

Hard-coded numbers cannot be avoided with the lookup table approach. Even if the values in the table are replaced with the GTH_TRANSFORM_* constants, the index (e.g. the position in the table) remains hard-coded. The only way to prevent hard-coded numbers is replacing the lookup table with a switch statement.

> 3) One new line in the rotation function needs an extra tab (the patch is
> mis-formatted).

The indentation of the code from CVS was already messed up. I didn't touch any existing code. I suppose it's caused by a mixture of tabs and spaces for indentation.
Comment 11 Michael Chudobiak 2006-10-19 15:27:12 UTC
Created attachment 75019 [details] [review]
Jef's improvements and documentation updates

Jef,

Thanks for the clarifications. I wasn't aware of the multi-orientation-tag possibility.

I've attached a roll-up patch that does the following:

1. Adds support for the trim mode (Jef's patch 74954)
2. Fixes tag updating when multiple IFDs have orientation tag (Jef's patch 74956).
3. Adds comments to the GthTransform typedef, to explain its purpose.
4. Updates the README file to reflect new intltool minimum requirement.
5. Fixes zoom keybinding (bug 345339).

Paolo, can you commit this?

- Mike
Comment 12 Paolo Bacchilega 2006-10-19 17:42:52 UTC
patch applied, thanks.
Comment 13 Jef Driesen 2006-10-23 14:53:15 UTC
Created attachment 75244 [details] [review]
Update for the new rotation code

I have some additional comments on the current rotation code flow:

1. The normal flow and the special cases (e.g for problematic jpeg mcu images) are not clearly separated. Every time I look at the code, I have to read it a second time to see what's going on. In bug 343867 comment 150, I already proposed a more readable version. I have updated that patch here and added the current jpeg mcu handling. I think this version is prettier to the eyes and more flexible for further improvement (read further).

For now, we always assumed the exif transform is only supported for jpeg images. TIFF images can also have an orientation tag, but they don't seem to be supported by libexif either (at least not by the exif commandline tool). When I use another utility to set the tag (e.g. exiftool -Orientation="Rotate 90 CW" filename.tif), I get unexpected results (a double rotation). But this behaviour is the same for the old gthumb, eog and nautilus. Maybe this is a gtk bug?

2. In the same comment, I also mentioned why I think overriding the user preference (and forcing an exif transform) is a bad idea. (e.g. What if I really need the physical transform? Now my only option is another application.) I would only show a warning dialog here, and optionally give the user the possibility to cancel the transform. If the user proceeds and is not happy with the resulting distortion, he can still reverse them and choose the exif transform next time. We could also offer the trim mode as an option in the dialog.

3. The warning dialogs do not mention the actual filename. That can be confusing when transforming many images at once.
Comment 14 Michael Chudobiak 2006-10-23 15:55:12 UTC
Hi Jef,

I agree; your code is smoother, and the odd-mcu code could be improved.

Do you have time to re-work the odd-mcu code with a new dialog so that it offers the choices outlined below? (I can take a look at it, but I don't actually know how to write wait-for-gui-input code at the moment... I can learn, but it will take a while...)

1. rotate with distortion
2. rotate by exif tag (offer only when tag is present)
3. rotate with trim
4. cancel

I think that will accommodate absolutely everyone :-)

You are right about the filename-dialog warning too, of course.

- Mike
Comment 15 Jef Driesen 2006-10-24 13:18:07 UTC
I can take a closer look, but it will also take some time.
Comment 16 Jef Driesen 2006-10-30 22:27:50 UTC
Created attachment 75696 [details] [review]
Add warning dialog for odd-mcu images

Here is my first attempt to implement the warning dialog. This patch includes my previous patch from comment 13.

To keep things simple, I did not add a button for the exif transform. (Also because the user can choose cancel and retry with the "Apply physical transform" checkbox disabled in this case). For the rest everything seems to work fine.
Comment 17 Michael Chudobiak 2006-10-31 21:30:25 UTC
Great! I'll check it out tomorrow.

- Mike
Comment 18 Michael Chudobiak 2006-11-01 16:22:59 UTC
Jef,

I'm having problems with the trim code, using the test image from http://bugzilla.gnome.org/attachment.cgi?id=58355&action=view.

If I download that image, bring it up in gthumb folder view, and hit "]" three times, the trim option is offered three times (and I accept the trim option three times), but the trim doesn't actually happen on the first and second rotates. The trim is successful on the third rotate.

Can you reproduce this?

- Mike
Comment 19 Jef Driesen 2006-11-02 18:12:03 UTC
Created attachment 75872 [details] [review]
Better odd-mcu detection

I can confirm your experiment, but it's not a real bug. Depending on the requested transformation, the jpegtran code can handle partial mcu's (on the right and/or the bottom of the image) without the need for trimming. I found this information on:

http://www.impulseadventure.com/photo/rotation-partial-mcu.html

This means that our mcu-problem-detection code is not accurate enough. I found this code to do a better detection:

http://jpegclub.org/jpegtran/

I added this to my previous patch (comment 16) and it works much better now. In your example, there is no warning dialog anymore for the first two rotations.
Comment 20 Michael Chudobiak 2006-11-02 20:34:31 UTC
Ah, much better. Patch committed (I can do that now!).

Do we need to worry about MCU = 16, which I guess is possible in theory, or is it too obscure?

- Mike
Comment 21 Michael Chudobiak 2006-11-03 14:12:47 UTC
Jef,

FYI, there is another Exif issue:

Bug 370013 – Exif orientation problems in web album generation

- Mike
Comment 22 Jef Driesen 2006-11-06 13:53:28 UTC
(In reply to comment #20)
> Do we need to worry about MCU = 16, which I guess is possible in theory, or is
> it too obscure?

I have no idea if MCU = 16 is actually used. We could read the value from the jpeg header (like is explained on the website I mentioned in comment 19), but that is certainly more complicated than simply assuming MCU = 8.
Comment 23 Jef Driesen 2006-11-09 20:32:50 UTC
Created attachment 76295 [details] [review]
Cancel the transform when closing the mcu dialog

When the mcu dialog is closed with the window close button, the transform is applied. I would expected to cancel the transform instead. This patch fixes that.
Comment 24 Jef Driesen 2006-11-09 20:58:48 UTC
Created attachment 76297 [details] [review]
Adjust the exif tags directly inside the jpegtran function

I made a patch to adjust the exif tags (reset orientation to top-left, swap dimension related tags when necessary) directly inside the jpegtran function. This is much more efficient (no more temporary files for every operation). I moved several functions from rotation-utils.[ch] to jpegtran.c and removed some unused functions.

For the implementation, I used some ideas from the exiftran utility, which can also transform the thumbnail. I tried it's thumbnail code for a quick test, and it seems to work. But it certainly needs some more work. That's why I did not include it in this patch. Only an empty stub is added.

PS: Can someone re-open this bug? I don't have the necessary permissions.
Comment 25 Michael Chudobiak 2006-11-09 21:17:49 UTC
The mcu dialog patch has been committed. I'll look at the new patch.

- Mike
Comment 26 Michael Chudobiak 2006-11-10 13:28:14 UTC
Great work, Jef! You really know your stuff! Everything seems to work correctly.

I've committed the jpegtran patch.

- Mike
Comment 27 Jef Driesen 2006-11-10 23:23:09 UTC
Created attachment 76358 [details] [review]
Transform the thumbnail data.

This is the code I used for the quick test I mentioned in comment 24. It seems to work, but that's all I can say about it. I could use some help on this one, because I don't understand all this jpeglib code very well.
Comment 28 Michael Chudobiak 2006-11-11 11:07:04 UTC
*** Bug 334487 has been marked as a duplicate of this bug. ***
Comment 29 Michael Chudobiak 2006-11-11 11:17:55 UTC
Do you have any particular concerns about the patch?

I'll ask Rennie to follow this bug, too.

- Mike
Comment 30 Michael Chudobiak 2006-11-12 21:11:54 UTC
Jef,

I checked with Paolo, and he says he isn't a jpeg expert either. So you are the ranking jpeglib expert here. I've tested the thumbnail patch quickly, and it seems to work, so I've committed it.

Unfortunately, a major bug seems to have crept in recently (before this patch). I noticed it while testing large series of images, to check for memory leaks in the latest patch.

With the rotation mode set to non-physical (exif tag change), select 20 images in the browser and hit "]". Works fine. Then hit "[" to reverse the changes. This time, only one out of every four images is correctly rotated back. The others are in different orientations (cycling 90 degrees each time).

I haven't debugged this at all yet.

- Mike



Comment 31 Michael Chudobiak 2006-11-12 21:40:35 UTC
Created attachment 76447 [details] [review]
Fixes error in multiple exif-tag-rotation of series

OK, found the bug: data->transform was being changed during rotation, which was a bad idea. We should have made a copy. Patch attached. I will commit it shortly.

I'm surprised this wasn't caught sooner...

- Mike
Comment 32 Michael Chudobiak 2006-11-12 21:45:23 UTC
Patch committed.
Comment 33 Jef Driesen 2006-11-13 15:35:21 UTC
(In reply to comment #29)
> Do you have any particular concerns about the patch?

The main problem is the error handling. Calling exit in case of an error is too drastic here. That is also one of the reason, the default error handler is replaced in the jpegtran function.

For the reading/writing to/from files, jpeglib provides the necessary functions. But for the thumbnail we have an in-memory image. The jpeglib documentaton advises to write a specialized source and destination manager [1]. My patch 76358 added those functions from the exiftran utility (nothing more than a simple copy and paste). I'll try to come up with a better implementation.

[1] http://www4.informatik.uni-erlangen.de/DE/Services/Doc/graphics/doc/jpeg/libjpeg.html#section-4.5

(In reply to comment #31)
> OK, found the bug: data->transform was being changed during rotation, which was
> a bad idea. We should have made a copy. Patch attached. I will commit it
> shortly.

Could you also verify the issue we discussed in bug 370013 comment 13 (and follow-ups). That is probably very similar to this one.
Comment 34 Michael Chudobiak 2006-11-14 12:52:54 UTC
> Could you also verify the issue we discussed in bug 370013 comment 13 (and
> follow-ups). That is probably very similar to this one.

I think the set_orientation_in_exif_data approach in bug 370013 needs to be re-worked to make it more robust and future-proof (i.e., to centralize exif-tag-updating). I plan to take another look at it after this bug settles down a bit, because you are making frequent improvements to the jpeg code. However, I don't think that the modification of the exif data in memory in set_orientation_in_exif_data is a real problem - the functions that deal with the pixbuf don't care what the exif orientation is (except for the loader). Those functions that do care read the exif tags and the image data in from the file, rather than an existing pixbuf (e.g., apply_transformation in dlg-jpegtran.c). But I expect that this issue will go away shortly...

> The main problem is the error handling. Calling exit in case of an error is too drastic here.

What set of circumstances could be expected to trigger these errors? Is there an easy way to delete the thumbnail entirely if something goes wrong? I don't think any applications are bothered by the absence of embedded thumbnails.

- Mike










Comment 35 Jef Driesen 2006-11-15 07:45:49 UTC
(In reply to comment #34)
> > Could you also verify the issue we discussed in bug 370013 comment 13 (and
> > follow-ups). That is probably very similar to this one.
> 
> I think the set_orientation_in_exif_data approach in bug 370013 needs to be
> re-worked to make it more robust and future-proof (i.e., to centralize
> exif-tag-updating). I plan to take another look at it after this bug settles
> down a bit, because you are making frequent improvements to the jpeg code.
> However, I don't think that the modification of the exif data in memory in
> set_orientation_in_exif_data is a real problem - the functions that deal with
> the pixbuf don't care what the exif orientation is (except for the loader).
> Those functions that do care read the exif tags and the image data in from the
> file, rather than an existing pixbuf (e.g., apply_transformation in
> dlg-jpegtran.c). But I expect that this issue will go away shortly...

I think you could be right about the issue on reading the exif data from file or memory. But even if there are no side effects (now), it's not the most clean solution. We should certainly document this behaviour.

> > The main problem is the error handling. Calling exit in case of an error is too drastic here.
> 
> What set of circumstances could be expected to trigger these errors? Is there
> an easy way to delete the thumbnail entirely if something goes wrong? I don't
> think any applications are bothered by the absence of embedded thumbnails.

Corrupt data and a too small output buffer (for the transformed thumbnail) are the only sources of error I can think of right now. Removing the thumbnail (or even recreating a new one) would be an option for the first case. But the second case is different story. The buffer should be made large enough (we don't know the size in advance) or increase it on the fly.
Comment 36 Jef Driesen 2006-11-15 08:27:42 UTC
Created attachment 76628 [details] [review]
Fix small leak inside jpegtran

I found a little memory and file descriptor leak inside jpegtran. It happens only when something goes wrong, so it's not very urgent. I attached a patch to fix this little bug.

(In reply to comment #22)
> (In reply to comment #20)
> > Do we need to worry about MCU = 16, which I guess is possible in theory, or is
> > it too obscure?
> 
> I have no idea if MCU = 16 is actually used. We could read the value from the
> jpeg header (like is explained on the website I mentioned in comment 19), but
> that is certainly more complicated than simply assuming MCU = 8.

Adding a new function to read the image dimensions and the real MCU value from the image file will results in code duplication and multiple file reads (because we read the file again inside jpegtran). But I came up with a nice idea to avoid all the trouble. Instead of trying to find out what action to take in advance (e.g. before calling jpegtran), we could do it directly inside jpegtran. This has the advantage that we have access to the real values. The logic to take an appropriate action when an problematic images is detected, is supplied as callback function. It think this is a very flexible approach and centralizes the code in one place. What do you think of this idea?
Comment 37 Jef Driesen 2006-11-15 09:55:46 UTC
Created attachment 76629 [details] [review]
Transform the thumbnail data (v2)

Here is the next step in my attempt to rewrite the thumbnail rotation. This code is functional equivalent with the previous one, but is more readable. I moved all the jpeg callback functions to a separate file (jpeg-memory-mgr.[ch]) and implemented two new functions (jpeg_memory_src and jpeg_memory_dest) as I explained in comment 33.
Comment 38 Michael Chudobiak 2006-11-15 14:04:45 UTC
Jef,

I committed the memory/file descriptor leak patch. Good catch!

Also, I committed some changes in bug 370013. The album exporter keeps the in-memory tag updates because it is safe to do so (and the efficiency gains are desirable, because it iterates over many files), but the "File > Save As" functions have reverted to the write_orientation_field approach for maximum safety and clarity (efficiency isn't a real concern in those single-file operations). There is more work to be done, though, and things may change again.

I like your jpegtran/callback idea! The jpeg code has too many layers in it as a result of its evolution and feature-additions, so opportunities to simplify and centralize are most welcome - and you are good at that!

I'll look at v2 of the thumbnail patch when I have some more time.

- Mike
Comment 39 Michael Chudobiak 2006-11-16 15:57:07 UTC
I have committed your most recent patch (with two new files).

Jef, when do you want to trigger a 2.9.0 release? Let me know when we reach a good spot to temporarily freeze the code and I'll ask Paolo to push out a release.

I have a workable patch for the web-album exif export issue now (bug 370013, please have a look), so the 2.9.0 release really just depends on the work in this bug.

- Mike

Comment 40 Michael Chudobiak 2006-11-19 12:14:12 UTC
Compiler warning:

jpegtran.c: In function 'update_exif_thumbnail':
jpegtran.c:237: warning: passing argument 2 of 'jpeg_memory_dest' from incompatible pointer type
Comment 41 Michael Chudobiak 2006-11-19 12:19:14 UTC
Other warnings:

In file included from /usr/include/jpeglib.h:28,
                 from ../libgthumb/jpegutils/transupp.h:52,
                 from ../libgthumb/jpegutils/jpegtran.h:29,
                 from dlg-jpegtran.c:50:
/usr/include/jconfig.h:12:1: warning: this is the location of the previous definition
In file included from ../libgthumb/jpegutils/jpegtran.h:23,
                 from rotation-utils.c:35:
../config.h:59:1: warning: "HAVE_STDLIB_H" redefined
In file included from /usr/include/jpeglib.h:28,
                 from ../libgthumb/jpegutils/transupp.h:52,
                 from rotation-utils.c:34:
/usr/include/jconfig.h:12:1: warning: this is the location of the previous definition

- Mike
Comment 42 Jef Driesen 2006-11-19 15:21:06 UTC
(In reply to comment #39)
> Jef, when do you want to trigger a 2.9.0 release? Let me know when we reach a
> good spot to temporarily freeze the code and I'll ask Paolo to push out a
> release.

I think almost everything is ready here. Except for:

1. Improving error handling for the thumbnail rotation. Right now, we still call exit (hidden inside the default jpeglib error handler). We can re-use most of the code from the main jpegtran function for that. I think this should be fixed before releasing.

2. As an addition to #1, we could discard (or even recreate) the thumbnail when there is an error. Discarding is easy, recreating is probably more difficult.

3. Resizing the thumbnail buffer on the fly if required. Now we allocate twice the size of the original thumbnail, which is probably more than large enough.

4. The callback implementation for mcu related issues.

(In reply to comment #40 and comment #41)
> Compiler warning:

Adding a cast or changing the function arguments, fixes the first warning. I don't have those other warnings.
Comment 43 Jef Driesen 2006-11-23 15:03:27 UTC
Created attachment 77063 [details] [review]
Transform the thumbnail data (v3)

I added error handling to the thumbnail transformation code. I re-used the code from the main jpegtran function. The only difference is that errors are not reported back to the caller (e.g. no GError parameter) and the thumbnail is removed (silently).

I also fixed the compiler warning, changed some function parameters to 'const' and fixed some indentations.
Comment 44 Michael Chudobiak 2006-11-23 15:27:46 UTC
Patch committed! Thanks!

- Mike
Comment 45 Jef Driesen 2006-11-24 11:24:56 UTC
Created attachment 77094 [details] [review]
Transform the thumbnail data (v4)

Small improvement to my previous patch. I removed the check for invalid thumbnail data, because that is handled by the jpeg library anyway (and probably much better than we did). Also, if bad data was found, it remained unchanged. With this new patch it is discarded as well.

I tested with two images with corrupted thumbnails (one with a truncated thumbnail and one with garbage inserted before the real data) and the thumbnail gets removed correctly.
Comment 46 Michael Chudobiak 2006-11-24 20:00:54 UTC
Committed!

- Mike
Comment 47 Michael Chudobiak 2006-11-30 21:13:48 UTC
Hey Jef... while you're hacking away at tags and IFDs, maybe keep bug 339648 in mind. We are missing access to a whole set of GPS-specific exif tags, because they are in a separate IFD.

- Mike
Comment 48 Jef Driesen 2006-12-01 09:07:00 UTC
That bug is about displaying exif tags and has very little to do with rotation...
Comment 49 Michael Chudobiak 2007-01-10 21:14:16 UTC
Hi Jef,

Can we close this bug, or do you plan more refinements?

- Mike
Comment 50 Jef Driesen 2007-01-11 09:06:12 UTC
I'm still working on the callback implementation for the jpeg mcu detection. Besides that, I think there are no important issues left. Maybe only item #3 (see comment 42), but it's very unlikely to cause problems.
Comment 51 Jef Driesen 2007-01-14 19:51:37 UTC
The reason why there is not much progress on the callback idea, is that I have a problem implementing the Cancel action. I don't know how to abort jpegtran properly. I tried two different approaches:

1. Aborting jpegtran with a return statement.

This results in creating an empty temporary file because the mcu check is performed before the actual transformation. If the jpegtran return value indicated success, gthumb overwrites the original file with the empty file, resulting in dataloss. On the other hand, returning a non zero value (indicating an error), makes it impossible to tell the difference between a real error and aborting. This is important for notifying the user about real errors.

2. Resetting the transform to JXFORM_NONE

This method does not create an empty file, but has another side effect. jpegtran always reset the exif orientation tag (if present) to top-left. So this is not always the same as cancelling the transform.
Comment 52 Jef Driesen 2007-01-21 14:50:06 UTC
Created attachment 80810 [details] [review]
Callback implementation for the jpeg mcu detection

I now have a working patch to implement the callback idea. For the Cancel action, I decided to use option #1 (see comment 51). The difference between a real error and aborting, is provided by the GError parameter. The user is only notified if this parameter has data.

For convenience, I also provided three pre-defined 'callbacks': JPEGTRAN_MCU_CONTINUE (or NULL), JPEGTRAN_MCU_TRIM and JPEGTRAN_MCU_CANCEL.
Comment 53 Michael Chudobiak 2007-01-23 14:16:27 UTC
Thanks, Jef!

I'll look at the patch - it will take me a while to review & understand it.

(Off-topic: Do you use RAW photos at all? If you're interested, check out bug 145564).

- Mike
Comment 54 Michael Chudobiak 2007-01-26 21:15:04 UTC
Jef,

I have applied the patch to trunk and the vfs-testing branch. Thanks!

FYI, I plan to modify some of the file access code, so that it will support gnome-vfs (like fopen in jpeg_data_load_file / jpeg_data_save_file).

- Mike
Comment 55 Michael Chudobiak 2007-01-26 21:16:01 UTC
I forgot to say: nice code! The apply_transformation sure looks a lot cleaner now.

- Mike
Comment 56 Michael Chudobiak 2007-02-16 00:02:17 UTC
Jef,

there is an odd jpeg/exif issue reported in bug 408185. Please have a look...

- Mike