=== 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)

I suggest to merge to master

add keven.

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

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.

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).

(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?

(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?

(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 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.

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

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 on attachment 793281 [details] Link to Pull Request Looks good, Tom. Thanks for your hard work on this.

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