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 783200 - Background chooser ignores EXIF orientations
Background chooser ignores EXIF orientations
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Background
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Debarshi Ray
Control-Center Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-05-29 15:53 UTC by slatchurie
Modified: 2017-06-07 16:34 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
background: handle EXIF orientation in the chooser (3.99 KB, patch)
2017-05-29 15:57 UTC, slatchurie
none Details | Review
handle EXIF orientation in the chooser (4.14 KB, patch)
2017-05-31 16:00 UTC, slatchurie
none Details | Review
patch without gdk_pixbuf_apply_embedded_orientation (425.74 KB, image/png)
2017-06-01 14:15 UTC, slatchurie
  Details
background: handle EXIF orientation in chooser (5.18 KB, patch)
2017-06-01 15:27 UTC, slatchurie
none Details | Review
screenshot (430.64 KB, image/png)
2017-06-03 11:30 UTC, slatchurie
  Details
background: handle EXIF orientation in chooser (4.99 KB, patch)
2017-06-07 16:32 UTC, Bastien Nocera
committed Details | Review

Description slatchurie 2017-05-29 15:53:26 UTC
The thumbnails in the chooser ignore the EXIF orientations embedded in the files.

It is a part of this newcomers gnome-desktop bug https://bugzilla.gnome.org/show_bug.cgi?id=516177
I have already made a patch for mutter https://bugzilla.gnome.org/show_bug.cgi?id=783125

The previews in the main panel are handled by gnome-desktop's functions, I'll send a patch there.
Comment 1 slatchurie 2017-05-29 15:57:14 UTC
Created attachment 352795 [details] [review]
background: handle EXIF orientation in the chooser
Comment 2 Bastien Nocera 2017-05-30 06:09:16 UTC
Review of attachment 352795 [details] [review]:

::: panels/background/bg-pictures-source.c
@@ +293,2 @@
   g_object_set_data_full (G_OBJECT (stream), "item", g_object_ref (item), g_object_unref);
+  gdk_pixbuf_new_from_stream_async (G_INPUT_STREAM (stream),

This is probably a performance regression (we couldn't skip the majority of the decoding as we don't use the "at_scale" variant), but there probably isn't an easy way to fix this.

However, as the majority of files wouldn't have EXIF orientation tags, and I think that the EXIF tags are processed when using the "at_scale" variant, I'd explicitly check whether rotation is necessary when the scaled image is available, and launch another unscaled load in that case, and that case only.

That would mean loading pictures with EXIF data twice, but would make loading pictures without it much quicker.
Comment 3 slatchurie 2017-05-31 16:00:10 UTC
Created attachment 352956 [details] [review]
handle EXIF orientation in the chooser

Thank you for your guidance.
Here is an updated patch.

Also don't forget to push https://bugzilla.gnome.org/show_bug.cgi?id=783125 it is the most important part.
Comment 4 Bastien Nocera 2017-06-01 09:59:41 UTC
Review of attachment 352956 [details] [review]:

::: panels/background/bg-pictures-source.c
@@ +192,3 @@
 
+  /* Process embedded orientation */
+  rotated = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (item), "rotated"));

"rotation-applied"

Also please move this to a separate function, so that you can exit early from it if necessary.

@@ +200,3 @@
+        {
+          gint transform = (int) g_ascii_strtoll (orientation_string, NULL, 10);
+          if (transform > 4)

After transforming this into an early exit:
if (*orientation_string == '1' || *orientation_string == '2' ...

@@ +204,3 @@
+              /* the file has to be reloaded */
+              GFile *file = g_file_new_for_uri (uri);
+              if (file != NULL)

g_file_new_for_uri() cannot fail.

@@ +218,3 @@
+    }
+
+  GdkPixbuf *tmp_pixbuf = gdk_pixbuf_apply_embedded_orientation (pixbuf);

Variable declarations need to be at the top of the block.

@@ +296,3 @@
   g_object_set_data_full (G_OBJECT (stream), "item", g_object_ref (item), g_object_unref);
+  rotated = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (item), "rotated"));
+  if (rotated)

That's wrong if it's rotated 180 degrees, or any of the other transform other than 90 degrees either way.
Comment 5 slatchurie 2017-06-01 11:17:36 UTC
(In reply to Bastien Nocera from comment #4)
> @@ +296,3 @@
>    g_object_set_data_full (G_OBJECT (stream), "item", g_object_ref (item),
> g_object_unref);
> +  rotated = GPOINTER_TO_INT (g_object_get_data (G_OBJECT (item),
> "rotated"));
> +  if (rotated)
> 
> That's wrong if it's rotated 180 degrees, or any of the other transform
> other than 90 degrees either way.

Sorry, I don't get this one. Can you elaborate please ?

It was tested with the 8 possible orientations with these pictures :
https://github.com/recurser/exif-orientation-examples
Comment 6 slatchurie 2017-06-01 14:15:49 UTC
Created attachment 353008 [details]
patch without gdk_pixbuf_apply_embedded_orientation

(In reply to Bastien Nocera from comment #4)

I guess between my bad english and the poor choice of names you didn't understand what the patch does.

It performs gdk_pixbuf_apply_embedded_orientation for EXIF orientations 1, 2, 3, 4.
But for EXIF orientations 5, 6, 7, 8, the width and height of the thumbnail are wrong. The file needs to be reloaded with swapped values in order to have the correct dimensions after gdk_pixbuf_apply_embedded_orientation is performed.

Here is a picture of the patch WITHOUT gdk_pixbuf_apply_embedded_orientation.
Comment 7 slatchurie 2017-06-01 15:27:46 UTC
Created attachment 353018 [details] [review]
background: handle EXIF orientation in chooser

The chooser ignores EXIF orientations embedded in the pictures when the thumbnails are generated.

Because the EXIF informations are available after the file has been loaded, it has to be reloaded if the transformed dimensions don't match the thumbnails constraints.
The transformations are directly applied for EXIF orientations 1, 2, 3 and 4.
For EXIF orientations 5, 6, 7 and 8, the pictures are reloaded with swapped dimensions before the transformations are applied.



I hope the code is clearer.
Comment 8 slatchurie 2017-06-03 11:30:38 UTC
Created attachment 353110 [details]
screenshot

Screenshot with test pictures from Comment 5
Comment 9 Bastien Nocera 2017-06-07 16:06:25 UTC
Review of attachment 353018 [details] [review]:

Please make sure to not use lines longer than 72 characters in the commit message.

I'll upload a new version in a second.

::: panels/background/bg-pictures-source.c
@@ +142,3 @@
+picture_needs_rotation (GdkPixbuf *pixbuf)
+{
+  const gchar *orientation_string = gdk_pixbuf_get_option (pixbuf, "orientation");

I split up the declaration and assignment, and renamed the variable "str", which is long enough given the simplicity of this function.

@@ +212,3 @@
+       * the file has to be reloaded.
+       */
+      GFile *file = g_file_new_for_uri (uri);

Separated the assignment and declaration again.

@@ +222,3 @@
+    }
+
+  tmp_pixbuf = gdk_pixbuf_apply_embedded_orientation (pixbuf);

Split up in a separate function, and removed "tmp_pixbuf" variable.

@@ +301,3 @@
+  if (needs_rotation)
+    {
+      /* swap width and height for EXIF orientations 5, 6, 7 and 8 */

Reworded this.
Comment 10 Bastien Nocera 2017-06-07 16:32:19 UTC
Created attachment 353347 [details] [review]
background: handle EXIF orientation in chooser

The chooser ignores EXIF orientations embedded in the pictures when the
thumbnails are generated.

Because the EXIF informations are available after the file has been
loaded, it has to be reloaded if the transformed dimensions don't
match the thumbnails constraints.

The transformations are directly applied for EXIF orientations 1, 2,
3 and 4. For EXIF orientations 5, 6, 7 and 8, the pictures are reloaded
with swapped dimensions before the transformations are applied.
Comment 11 Bastien Nocera 2017-06-07 16:34:06 UTC
Attachment 353347 [details] pushed as 5508226 - background: handle EXIF orientation in chooser

Tested with:
https://github.com/recurser/exif-orientation-examples