Last Comment Bug 876553 - Preview images in filepicker dont use exif orientation tag
: Preview images in filepicker dont use exif orientation tag
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla24
Assigned To: Timothy Arceri
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-05-27 18:28 PDT by Timothy Arceri
Modified: 2013-05-30 09:15 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Use correct preview orientation if set in tag (1.16 KB, patch)
2013-05-28 15:03 PDT, Timothy Arceri
karlt: review-
Details | Diff | Splinter Review
preview_orientation_v2.patch (1.23 KB, patch)
2013-05-28 17:15 PDT, Timothy Arceri
karlt: review+
Details | Diff | Splinter Review
Patch v3 (1.26 KB, patch)
2013-05-28 18:32 PDT, Timothy Arceri
karlt: review+
philip.chee: checkin+
Details | Diff | Splinter Review

Description Timothy Arceri 2013-05-27 18:28:48 PDT
As originally report here: https://bugzilla.gnome.org/show_bug.cgi?id=534976

The GTK filepicker does not rotate the preview image base on the exif orientation tag.
This can be corrected using gdk_pixbuf_apply_embedded_orientation() which was added in GTK 2.12. As the current build requirement is only 2.10 I'm proposing this be added as part of the port to GTK 3.0 as it hardly seems worth bumping the GTK 2 version at this point.
Comment 1 Karl Tomlinson (:karlt) 2013-05-27 18:41:14 PDT
Most Mozilla apps are now built against GTK+-2.18 or newer.
http://www.mozilla.org/en-US/firefox/21.0/system-requirements/

The 2.10 build requirement remains just because of bug 795354.

However, I expected GTK code to determine image previews for its filepicker.  I'm surprised if there is Gecko code providing the previews for the GTK filepicker.
Comment 2 Timothy Arceri 2013-05-27 18:51:24 PDT
GTK does not do not provide previews by default the widget is created on the application side and passed to the GTK filechooser. I have not got around to testing this yet but I think the fix is to add a call to gdk_pixbuf_apply_embedded_orientation() in the UpdateFilePreviewWidget() function of nsFilePicker.cpp

http://hg.mozilla.org/mozilla-central/file/6e45e9f62d21/widget/gtk2/nsFilePicker.cpp#l82
Comment 3 Philip Chee 2013-05-27 21:59:39 PDT
> Depends on: 795354
In general SeaMonkey bugs should not block core bugs. We appreciate your concerns however. CC: Callek.
Comment 4 Philip Chee 2013-05-27 22:30:14 PDT
> In general SeaMonkey bugs should not block core bugs
After discussing this with Callek, if we (SeaMonkey) break, we'll manage one-way-or-another.
However there are other reasons for the 2.10 dependency, e.g. older linux distros Mozilla still wants to support,
Comment 5 Timothy Arceri 2013-05-27 23:47:17 PDT
(In reply to Philip Chee from comment #4)
> However there are other reasons for the 2.10 dependency, e.g. older linux
> distros Mozilla still wants to support,

The last time the version was bumped up support for older distros was discussed in bug 418885 in the end it looks like the consensus was that the older distros should package the required librarys. However it the case of that bug 2.10 had only been around for around 1 year. 2.18 seems to be the suggested next jump and has been around for almost 4 years (Sep 2009) so I wouldnt expect as greater uproar or shock.

Either way its obviously not my call. Karl are you happy to update the dependency to 2.18?
Comment 6 Karl Tomlinson (:karlt) 2013-05-28 01:30:45 PDT
Firefox and Thunderbird are built against 2.18 and have a run-time dependency > 2.10.  (I can't remember exactly.)

I recommend surrounding the gdk_pixbuf_apply_embedded_orientation code with a preprocessor conditional block using GTK_CHECK_VERSION.

https://developer.gnome.org/gtk2/2.24/gtk2-Feature-Test-Macros.html#GTK-CHECK-VERSION:CAPS
Comment 7 Timothy Arceri 2013-05-28 15:03:15 PDT
Created attachment 755042 [details] [diff] [review]
Use correct preview orientation if set in tag

Thanks for your help Karl. This patch fixes the issue.

----------------------------------------------------------------------------
Unrelated to this patch. When I did a grep for GTK_CHECK_VERSION it came up with a number of what looks to be old checks for gtk 2.10 and under. Are these still required or would you like it if I created a new bug for this and made a patch to remove any old code used in these calls.

./widget/gtk2/nsScreenManagerGtk.cpp:#if GTK_CHECK_VERSION(2,2,0)
./widget/gtk2/nsScreenManagerGtk.cpp:#endif // GTK_CHECK_VERSION(2,2,0)
./widget/gtk2/nsNativeThemeGTK.cpp:#if GTK_CHECK_VERSION(2,10,0)
./widget/gtk2/nsNativeThemeGTK.cpp:#endif // GTK_CHECK_VERSION(2,10,0)
./widget/gtk2/nsScreenGtk.cpp:#if GTK_CHECK_VERSION(2,0,0)
./toolkit/xre/nsAppRunner.cpp:#if GTK_CHECK_VERSION(2,8,0)
Comment 8 Karl Tomlinson (:karlt) 2013-05-28 16:30:27 PDT
(In reply to Timothy Arceri from comment #7)
> When I did a grep for GTK_CHECK_VERSION it came up
> with a number of what looks to be old checks for gtk 2.10 and under. Are
> these still required or would you like it if I created a new bug for this
> and made a patch to remove any old code used in these calls.
> 
> ./widget/gtk2/nsScreenManagerGtk.cpp:#if GTK_CHECK_VERSION(2,2,0)
> ./widget/gtk2/nsScreenManagerGtk.cpp:#endif // GTK_CHECK_VERSION(2,2,0)
> ./widget/gtk2/nsNativeThemeGTK.cpp:#if GTK_CHECK_VERSION(2,10,0)
> ./widget/gtk2/nsNativeThemeGTK.cpp:#endif // GTK_CHECK_VERSION(2,10,0)
> ./widget/gtk2/nsScreenGtk.cpp:#if GTK_CHECK_VERSION(2,0,0)
> ./toolkit/xre/nsAppRunner.cpp:#if GTK_CHECK_VERSION(2,8,0)

A patch to remove these would be great, thanks.
Comment 9 Karl Tomlinson (:karlt) 2013-05-28 16:36:27 PDT
Comment on attachment 755042 [details] [diff] [review]
Use correct preview orientation if set in tag

gdk_pixbuf_apply_embedded_orientation() doesn't actually modify or unref the src pixbuf
https://developer.gnome.org/gdk-pixbuf/stable/gdk-pixbuf-Utilities.html#gdk-pixbuf-apply-embedded-orientation
and so another g_object_unref() is required.  An additional pointer variable will be necessary because the src and return value may be different pixbufs.
Comment 10 Timothy Arceri 2013-05-28 17:15:50 PDT
Created attachment 755099 [details] [diff] [review]
preview_orientation_v2.patch

Yeah sorry I realised my mistake after posting the patch. Hopefully this one should do the trick.
Comment 11 Timothy Arceri 2013-05-28 18:32:04 PDT
Created attachment 755125 [details] [diff] [review]
Patch v3

Sorry for the noise. This patch does the same thing as the v2 one Karl reviewed but I fixed up the issue with it needing to be applied after the v1 (this is only the second time I've used Mercurial I didnt realise that creating a patch commited the change locally)
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-05-30 09:15:46 PDT
https://hg.mozilla.org/mozilla-central/rev/d50edb6a79aa

Note You need to log in before you can comment on or make changes to this bug.