900425 - Photo is rotated showing not correctly/Please Rotate picture by its orientation metadata

Status: RESOLVED FIXED

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

Description

[renfeng.mei]

Attachment: IMG_0001.jpg

=== steps:

1. push a photo from pc to sdcard.

2. open gallary to show the photo

=== what happed / errors
1. the photo is rotated, showing not correctly.
but it is showing correctly in pc or android mobile.
and it does not work on unagi.

=== Reason
1. have not read EXIF information when showing photo

=== Resolution
1. I suggest gaia to read EXIF information.
I have test the patch which can resolve this problem.
I suggest to merge this patch.

please see the attachment(photo/patch)

Comment 1

[renfeng.mei]

Attachment: Rotate picture by its orientation metadata

I suggest to merge to master

Comment 2

[andy.liu]

add keven.

Comment 3

[Steven Yang [:styang]]

David, could yo help to check this out? Thanks!

Comment 4

[David Flanagan [:djf]]

Comment on attachment 784314 [details] [diff] [review]
Rotate picture by its orientation metadata

Review of attachment 784314 [details] [diff] [review]:
-----------------------------------------------------------------

I'm giving an r- on this patch for several reasons:

1) The EXIF parsing code is from 2008 and parses the file as a binary string instead of using typed arrays.

2) The Gallery metadata parsing code already has an EXIF parser that uses typed arrays

3) The patch appears to manually rotate images using canvas, and this is bad for three reasons:

a) it will take too much memory and time to rotate a full-size image like this in the camera app.

b) rotating like this causes a jpeg decode and encode, which reduces the quality of the image

c) canvas.toBlob() removes all EXIF information from the resulting JPEG image.

4) The patch does not fix the Gallery app, and the steps to reproduce this bug points out that the gallery should honor the rotation bit.

Comment 5

[David Flanagan [:djf]]

Renfeng: could you please clarify your concern with this bug? Do you only care about images loaded onto the SD card from a PC and viewed in the gallery, or are we trying to support devices where the camera does this kind of "soft rotation" by only setting the EXIF orientation flag? I do not have any such device and would not be able to test a patch.

I am also setting needinfo for Tom Herold, who is planning to look at this issue for the Gallery.  Tom: I just wanted to bring this bug to your attention.  I'm hoping we can fix this in a better way in gallery by reading the EXIF orientation flag during metadata parsing and rotating it as needed.  

If you can do this by modifying shared/js/media/media_frame.js, then the fix will for Gallery will also fix the filmstrip and pick preview views in the Camera app, if that becomes necessary.  (Though in that case, we'd want to move apps/gallery/js/JPEGMetadataParser.js into shared/js/media/ so that Camera can use it too).

Comment 6

[renfeng.mei]

(In reply to David Flanagan [:djf] from comment #5)
> Renfeng: could you please clarify your concern with this bug? Do you only
> care about images loaded onto the SD card from a PC and viewed in the
> gallery, or are we trying to support devices where the camera does this kind
> of "soft rotation" by only setting the EXIF orientation flag? I do not have
> any such device and would not be able to test a patch.
> 
> I am also setting needinfo for Tom Herold, who is planning to look at this
> issue for the Gallery.  Tom: I just wanted to bring this bug to your
> attention.  I'm hoping we can fix this in a better way in gallery by reading
> the EXIF orientation flag during metadata parsing and rotating it as needed.
> 
> 
> If you can do this by modifying shared/js/media/media_frame.js, then the fix
> will for Gallery will also fix the filmstrip and pick preview views in the
> Camera app, if that becomes necessary.  (Though in that case, we'd want to
> move apps/gallery/js/JPEGMetadataParser.js into shared/js/media/ so that
> Camera can use it too).

David, 
Hope to be resolved all the picture from whatever pc or camera app.
And the patch i commited can resolve the two situation.

I agree with your suggestion that move the mataparser to shared directory.
Will Tom do this?

Comment 7

[David Flanagan [:djf]]

(In reply to renfeng.mei from comment #6)

> David, 
> Hope to be resolved all the picture from whatever pc or camera app.

Thanks for clarifying that.

> And the patch i commited can resolve the two situation.

I don't understand: your patch doesn't appear to resolve the situation for a pictured downloaded from a PC and displayed in the gallery.  Also, as I noted in my review, it is not acceptable (for performance or memory usage reasons) to manually rotate images in the camera app.  If the camera hardware does not do that, we have to modify the code that displays those photographs (in gallery and in the camera) to rotate them with a CSS transform.

We need to do this in Gallery anyway, for bug 891030. Tom Herold or I will be working on that bug, and I suspect that the fix there will also fix this bug.
> 
> I agree with your suggestion that move the mataparser to shared directory.
> Will Tom do this?

Comment 8

[renfeng.mei]

(In reply to David Flanagan [:djf] from comment #7)
> (In reply to renfeng.mei from comment #6)
> 
> > David, 
> > Hope to be resolved all the picture from whatever pc or camera app.
> 
> Thanks for clarifying that.
> 
> > And the patch i commited can resolve the two situation.
> 
> I don't understand: your patch doesn't appear to resolve the situation for a
> pictured downloaded from a PC and displayed in the gallery.  Also, as I
> noted in my review, it is not acceptable (for performance or memory usage
> reasons) to manually rotate images in the camera app.  If the camera
> hardware does not do that, we have to modify the code that displays those
> photographs (in gallery and in the camera) to rotate them with a CSS
> transform.
> 
> We need to do this in Gallery anyway, for bug 891030. Tom Herold or I will
> be working on that bug, and I suspect that the fix there will also fix this
> bug.
> > 
> > I agree with your suggestion that move the mataparser to shared directory.
> > Will Tom do this?


Thanks david, I got your point. The patch i supplied is not acceptable (for performance or memory usage reasons).

Comment 9

[Tom Herold]

Attachment: Link to Pull Request

Comment 10

[David Flanagan [:djf]]

Comment on attachment 793281 [details]
Link to Pull Request

Tom,

This looks like good work, but I'm sending it back for changes.

I think there are errors in MetadataParser. I recommend that you pass the orientation to the createThumbnailAndPreview() function so that you can create a rotated thumbnail, and also include the orientation object in the returned metadata.

Also, I'm not convinced that media_frame.js needs to change as much as you have changed it. Consider handling the rotation earlier in displayImage() or in computeFit() so that width and height get swapped earlier. Does that simplify the calulations and allow you to make fewer changes?

If not, please add more comments to the code so I understand why the new container is needed, e.g.

Comment 11

[Tom Herold]

Attachment: sketch of rotation + CSS origin center property

Comment 12

[Tom Herold]

Attachment: sketch for left/ top translation

Comment 13

[Tom Herold]

Attachment: sketch to explain itemWidth itemHeight reversal and rotation

Comment 14

[Tom Herold]

Attachment: Test image for 180° rotation

Comment 15

[Tom Herold]

Attachment: Test image for 270° rotation

Comment 16

[Tom Herold]

For more test images look here: https://github.com/recurser/exif-orientation-examples

Comment 17

[David Flanagan [:djf]]

Comment on attachment 793281 [details]
Link to Pull Request

Thanks for the link to the test images. Those are great and really demonstrate that the rotation and mirroring are working right!

I'm giving r- because of issues with the metadata parser changes and a couple of formatting nits.  See my comments on github. There are a couple of problems with your change where you pass the metadata object to createThumbnailAndPreview(), and I think the easiest thing might be to switch back to the old way of handling it and pass the orientation explicitly when needed.

Comment 18

[David Flanagan [:djf]]

Comment on attachment 793281 [details]
Link to Pull Request

Looks good, Tom. Thanks for your hard work on this.

Comment 19

[David Flanagan [:djf]]

Landed on master: https://github.com/mozilla-b2g/gaia/commit/4df267fb47c464904078a3f2f67d6b144a091aa3