The default bug view has changed. See this FAQ.

Preview images in filepicker dont use exif orientation tag

RESOLVED FIXED in mozilla24

Status

()

Core
Widget: Gtk
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Timothy Arceri, Assigned: Timothy Arceri)

Tracking

Trunk
mozilla24
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 627699
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.
(Assignee)

Comment 2

4 years ago
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
(Assignee)

Updated

4 years ago
No longer blocks: 627699
Depends on: 795354

Comment 3

4 years ago
> Depends on: 795354
In general SeaMonkey bugs should not block core bugs. We appreciate your concerns however. CC: Callek.

Comment 4

4 years ago
> 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,
No longer depends on: 795354
See Also: → bug 795354
(Assignee)

Comment 5

4 years ago
(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?
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
(Assignee)

Comment 7

4 years ago
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)
Attachment #755042 - Flags: review?(karlt)
(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 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.
Attachment #755042 - Flags: review?(karlt) → review-
(Assignee)

Comment 10

4 years ago
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.
Attachment #755042 - Attachment is obsolete: true
Attachment #755099 - Flags: review?(karlt)
Attachment #755099 - Flags: review?(karlt) → review+
(Assignee)

Comment 11

4 years ago
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)
Attachment #755099 - Attachment is obsolete: true
Attachment #755125 - Flags: checkin?
(Assignee)

Updated

4 years ago
Attachment #755125 - Flags: review?(karlt)
Attachment #755125 - Flags: review?(karlt) → review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed

Comment 12

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50edb6a79aa
Assignee: nobody → t_arceri
Status: NEW → ASSIGNED
Keywords: checkin-needed
Target Milestone: --- → mozilla24

Updated

4 years ago
Attachment #755125 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/d50edb6a79aa
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.