989026 - [Gallery] - Image Editor /Pick activity should honor EXIF orientation data while editing/cropping an image

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Gallery, defect)

Description

[Punam Dahiya [:pdahiya]]

In Gallery for images with EXIF orientation data, image editor should honor the EXIF orientation data and rotate the image to display it correctly. The same issue is seen in pick activity (invoked via sms/contacts) while cropping the picked image.

STR:
1. Download image https://github.com/recurser/exif-orientation-examples/blob/master/Portrait_6.jpg.

Above image has EXIF.orientation data 6 that maps to rotate an image by 90 deg.

2. Open the image in gallery. Image will display correct.
3. Click on edit icon to edit the image.
4. Image is not rotated by 90 deg and displays incorrect.

Expected:
Image should rotate by 90 and displays correct

The above issue is also seen while cropping the above image picked using pick activity from SMS or contacts.

Comment 1

[David Flanagan [:djf]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c38 and https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c46 for some context for this bug.

Comment 2

[David Flanagan [:djf]]

Punam,

Joe Cheng requests an estimate of the time required to fix this bug.

Comment 3

[Punam Dahiya [:pdahiya]]

Hi Joe

I have started looking into this bug and hope to get it fixed by mid next week. Please note the scope of this bug is 

1. Display images with EXIF orientation right inside edit view and crop view of gallery.
2. Save the edited images and cropped images (via. pick activity) with correct orientation, so that it display right after edit/crop.

For uncropped and unedited images, the proposed solutions and fix is tracked in bug https://bugzilla.mozilla.org/show_bug.cgi?id=989290. Thanks

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

Thanks Punam

if my understanding is right
your #1. This will fix the photo editor to display proper orientation when editing portrait photo taken by the tarako camera 

regarding #2, I left a comment https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c55

Thanks

Comment 5

[Joe Cheng [:jcheng] (please needinfo)]

1.3T+, this seems to be the best we can do for now to fix most of the use cases for when attaching photos from applications with correct orientation

Comment 6

[Punam Dahiya [:pdahiya]]

Attachment: PR with fix of Bug 989026

Hi David

I have attached the PR with fix for Bug 989026. Please review. 

Thanks!

Comment 7

[Hema Koka [:hema]]

Please note that this is only solving the problem partially. We need to get this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=989290 addressed and possibly we can get away from landing punam's patch.

Joe, please help follow through for bug 989290

Comment 8

[Hema Koka [:hema]]

(In reply to Hema Koka [:hema] from comment #7)
> Please note that this is only solving the problem partially. We need to get
> this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=989290 addressed and
> possibly we can get away from landing punam's patch.
> 
> Joe, please help follow through for bug 989290

btw review is on hold, please look at djf's comments in bug 989290

Comment 9

[Joe Cheng [:jcheng] (please needinfo)]

cross posting here https://bugzilla.mozilla.org/show_bug.cgi?id=989290#c21

Hi Hema, I am aware of this and before we can find a proper solution that solves everything, we have also talked with partner to accept the cropped image solution for now. This is why i'd like bug 989026 to land in Tarako first. Will we be able to move forward with this bug first? thanks

Comment 10

[David Flanagan [:djf]]

Comment on attachment 8400432 [details] [review]
PR with fix of Bug 989026

Sorry this review has taken so long.

I think you're slightly on the wrong track here. Maybe if you rotate the preview image that is displayed by the image editor you'll get crop handles working right?  I think that would be a better approach than using CSS rotation for the image editor.

I've left some other comments on github as well.

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

let's discuss this again

Comment 12

[Jason Smith [:jsmith]]

To my understanding, we're no longer shipping cropping support on 1.3T. Doesn't that mean we can unblock this?

Comment 13

[David Flanagan [:djf]]

This is still a 1.3T+ blocker.

1) It doesn't make any sense to remove the crop feature since we still have to do image rotation and image rotation takes more memory than cropping

2) This bug was also for image editing.

On the plus side, however, I'm pretty sure I've resolved this bug as part of my patch for bug 989290.

Comment 14

[Joe Cheng [:jcheng] (please needinfo)]

should we dup this bug to bug 989290? thanks

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

It seem that bug 989290 only lands on 1.3T and this bug shall remain open for impl on master. If so we should remove 1.3T+ flag here. David?

Comment 16

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> It seem that bug 989290 only lands on 1.3T and this bug shall remain open
> for impl on master. If so we should remove 1.3T+ flag here. David?

The changes in 989290 get the orientation correct, so images now look right while being edited. When I was working on that bug and testing, Spredtrum had reduced the camera resolution to 1280x960 and the image editor worked.

Now the camera resolution has increased again to 1600x1200, and I'm finding that the Gallery crashes sometimes when trying to save an edited image. I'm thinking, therefore, that in order to be able to reliably edit images we are going to have to reduce their size in the same way that we reduce the size of images being picked.

So I'd like to keep this bug open as a 1.3T blocker for that image editor fix.

Comment 17

[David Flanagan [:djf]]

Attachment: link to patch on github

Comment 18

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #15)
> It seem that bug 989290 only lands on 1.3T and this bug shall remain open
> for impl on master. If so we should remove 1.3T+ flag here. David?

Actually, you're right. I'm moving this patch to bug 998095 which is more specifically about the OOM issue, and am changing this bug to 1.4? so we can consider whether we want to get this patch in 1.4 as well.

Comment 19

[David Flanagan [:djf]]

So bug is now the bug we'll use for porting the gallery-specific parts of the 989290 patch to master (and possibly 1.4).

Comment 20

[David Flanagan [:djf]]

This issue was fixed in 1.3T by bug 989290 and bug 998095.

Bug 1002593 has been opened to fix this issue in master.

So resolving this bug now.