Created attachment 352795 [details] [review]
background: handle EXIF orientation in the chooser

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.

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.

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.

(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

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.

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.

Created attachment 353110 [details]
screenshot

Screenshot with test pictures from Comment 5

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.

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.

Attachment 353347 [details] pushed as 5508226 - background: handle EXIF orientation in chooser

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