798684 - [gallery] Editing a photo strips it of EXIF data

Status: RESOLVED DUPLICATE of bug 1178812

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

Description

[Parul Mathur]

Attachment: img_20121004-184532.jpg: Original photo with EXIF data

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:15.0) Gecko/20100101 Firefox/15.0.1
Build ID: 20120905151427

Steps to reproduce:

Otoro phone, build 2012-10-04
Git commit info: a33396
Test Case: Verify - exif data for any photograph taken on camera
#1868 All photos taken with gaia camera app should have stored exif data, which can be checked.
https://moztrap.mozilla.org/runtests/run/228/env/161/


Actual results:

Photos taken with the Camera app have EXIF data, but after editing in the Gallery app that EXIF data is lost.


Expected results:

Editing a photo in the Gallery app should not strip it of its EXIF data.

Comment 1

[Parul Mathur]

Attachment: img_20121004-184532.edit1.jpg: Photo after editing and no EXIF data.

Comment 2

[Dietrich Ayala (:dietrich)]

David, the data should be copied over to the new photo right?

Not blocking because the original photo is intact.

Comment 3

[Dietrich Ayala (:dietrich)]

... and if a change is required, renominate for blocking?

Comment 4

[James Zhang (Spreadtrum)]

Attachment: Rotate picture by its orientation metadata

Photo's orientation metadata is ignored in gallery/camera app.
1.Fix photo orientation by default. 2.Rotate photos copied from other devices in gallery view

Comment 5

[James Zhang (Spreadtrum)]

It's qcom chipset hard code, please use the EXIF data to rotate picture, then tara and nexus-s will show picture properly.

Comment 6

[James Zhang (Spreadtrum)]

Attachment: unagi display rotate picture

unagi also has this issue, use attachment picture.
Pc and tara with patch display properly, and unagi display rotate 90 degrees.
Please apply gaia patch to support EXIF data.

Comment 7

[Marco Chen [:mchen]]

Hi David,

As I know that you are the owner of gallery, so could you give the help on this case? Or please let me know the correct one. 

Very Thanks.

Comment 8

[David Flanagan [:djf]]

Marco,

I'm not sure I understand the question...

It is a known bug that editing images strips all EXIF data. This is how Canvas.toBlob() works.

The gallery has never used exif orientation data to display photos, so I'm surprised to hear that removing that data changes the way photos are displayed.

Are you saying that some devices don't rotate photos but just set an EXIF property and that gecko is honoring the embedded orientation when it displays the photo? If so, that is going to break other parts of the gallery that extract the photo resolution from the JPEG file.

I'm not sure what to suggest here since I don't yet understand the bug.

Comment 9

[Marco Chen [:mchen]]

(In reply to David Flanagan [:djf] from comment #8)
> Are you saying that some devices don't rotate photos but just set an EXIF
> property and that gecko is honoring the embedded orientation when it
> displays the photo?

Hi David & James,

I think we can re-name and focus this bug on

  Does pre-installed gallery app of Fx OS support the orientation data from EXIF?

We should make this decision first so
  a. If we declare that pre-installed gallery didn't support it then this bug will be a feature limitation.
  b. If we declare we support this feature, then we need to re-write some parts of gallery in order to fit this requirement.

Hi David,

May I know that does EXIF supporting is listed on gallery's feature list?

Thanks.

Comment 10

[James Zhang (Spreadtrum)]

Hi Marco,

   We have filed a new Bug 900425.

Comment 11

[Marco Chen [:mchen]]

Hi David,

May I know your suggestion on comment 9? Thanks.

Comment 12

[David Flanagan [:djf]]

Marco,

Let's leave this bug open, as filed. I've assigned it to Tom Herold, who will work on it after working on the more specific image rotation case.

Bug 900425 is the bug that will honor EXIF image rotation in gallery and camera.

Bug 891030 is the bug that will add UX to the Gallery app to change the EXIF image rotation.

And this bug has nothing to do with image rotation: it will just copy EXIF data from an original image to an edited version of that image.

Comment 13

[Marco Chen [:mchen]]

Hi David,

Thanks for your great support and response here.

Comment 14

[Hema Koka [:hema]]

Considering for 1.3 once the dependent bug 891030 lands

Comment 15

[James Zhang (Spreadtrum)]

This feature is very improt for tarako.

Comment 17

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

dup a 1.3T+ bug to this bug, 1.3T+

Comment 18

[Punam Dahiya [:pdahiya]]

Attachment: PR updating edited images with EXIF data from original image

Hi David

Please review attached PR that updates edited images with EXIF data from original image.

Thanks

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8393115 [details] [review]
PR updating edited images with EXIF data from original image

Punam,

This looks great. A couple of nits and one easy change to make in the code.

I haven't had time to test it myself, but I assume you have, and that your tests have included editing an image and then editing the edited image to prove that jpeg.js can read the exif data it writes.

Do you have access to a Tarako?  IIUC, the issue here is that we need to persist the EXIF rotation data or photos on the Tarako won't be displayed with the correct orientation.  Are you able to observe that behavior and verify that this patch fixes it?

I'm giving r- because I fear that we need to do exactly the same thing in the activity code when we crop an image. (Sorry that that did not occur to me before.)  I think that on Tarako when the user picks an image for SMS or Contacts and crops it it might come out wrong if we don't add EXIF data to it.  Could you check on that?  If there is already a different bug filed for that, I'd be happy to land this one now, but it seems simplest to me to fix both saving edited images and transferring cropped images in a single patch.

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #19)
> Comment on attachment 8393115 [details] [review]
> PR updating edited images with EXIF data from original image
> 
> Punam,
> 
> This looks great. A couple of nits and one easy change to make in the code.

Thanks David. I will update PR with your feedback in github.
> 
> I haven't had time to test it myself, but I assume you have, and that your
> tests have included editing an image and then editing the edited image to
> prove that jpeg.js can read the exif data it writes.

Yes, i have tested and it works for above case
> 
> Do you have access to a Tarako?  IIUC, the issue here is that we need to
> persist the EXIF rotation data or photos on the Tarako won't be displayed
> with the correct orientation.  Are you able to observe that behavior and
> verify that this patch fixes it?

I do have access to Tarako but i can't flash it with from my mac. I understand Taipei can help test the patch. https://bugzilla.mozilla.org/show_bug.cgi?id=980914#c11

I will ask Joe's help to test it.
> 
> I'm giving r- because I fear that we need to do exactly the same thing in
> the activity code when we crop an image. (Sorry that that did not occur to
> me before.)  I think that on Tarako when the user picks an image for SMS or
> Contacts and crops it it might come out wrong if we don't add EXIF data to
> it.  Could you check on that?  If there is already a different bug filed for
> that, I'd be happy to land this one now, but it seems simplest to me to fix
> both saving edited images and transferring cropped images in a single patch.

Yes, we should fix it as part of this bug. I will update the PR and set the review flag again. Thanks

Comment 21

[Punam Dahiya [:pdahiya]]

Hi Joe

Can you pl. help test the attached patch (attachment 8393115 [details] [review] ) on Tarako for the original issue filed in bug 980914 (Photos on the Tarako won't be displayed with the correct orientation on editing due to missing EXIF data). 

The attached patch successfully copies the EXIF data from original image opened using thumbnail list in gallery to edited image.

Please note the only fix missing in attached patch is when the user crops image using pick activity from SMS or contacts. 

Thanks

Comment 22

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

awesome, thanks Punam
James, mind giving it a try? Thanks

Comment 23

[James Zhang (Spreadtrum)]

Yang will try it, thanks.

Comment 24

[Punam Dahiya [:pdahiya]]

Comment on attachment 8393115 [details] [review]
PR updating edited images with EXIF data from original image

Hi David

I have updated the patch to update images with EXIF data when the user crops image using pick activity from SMS or contacts. Please review

Thanks!

Comment 25

[Punam Dahiya [:pdahiya]]

(In reply to James Zhang from comment #23)
> Yang will try it, thanks.

Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
https://github.com/mozilla-b2g/gaia/pull/17353

Comment 26

[yang.zhao]

(In reply to Punam Dahiya from comment #25)
> (In reply to James Zhang from comment #23)
> > Yang will try it, thanks.
> 
> Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> https://github.com/mozilla-b2g/gaia/pull/17353

hi,Punam
   I've applied the patch,but the image displays right in gallery but is rotated when crop it using pick activity from contacts.

Comment 27

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

hi Punam, base on partner feedback, it seems like the we need to cover the scenario from contacts? thanks

Comment 28

[Punam Dahiya [:pdahiya]]

(In reply to yang.zhao from comment #26)
> (In reply to Punam Dahiya from comment #25)
> > (In reply to James Zhang from comment #23)
> > > Yang will try it, thanks.
> > 
> > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > https://github.com/mozilla-b2g/gaia/pull/17353
> 
> hi,Punam
>    I've applied the patch,but the image displays right in gallery but is
> rotated when crop it using pick activity from contacts.

Thanks James for helping test. PR https://github.com/mozilla-b2g/gaia/pull/17353 has fix for writing EXIF data into edited image either by clicking on thumbnail from thumbnail list in gallery or cropping using pick activity from contacts or SMS.

Can you please confirm that you are seeing EXIF data in edited images in both the above cases? 

I have tarako device installed with 1.3t build, BuildId: 20140310141338. I tried to replicate the original issue of image rotating on edit in gallery but not able to replicate. Here's the video of steps followed
http://www.youtube.com/watch?v=SvhQgAnbXLo

It will help us if you can provide the video or exact steps to replicate the image rotation bug. Thanks

Comment 29

[Punam Dahiya [:pdahiya]]

(In reply to yang.zhao from comment #26)
> (In reply to Punam Dahiya from comment #25)
> > (In reply to James Zhang from comment #23)
> > > Yang will try it, thanks.
> > 
> > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > https://github.com/mozilla-b2g/gaia/pull/17353
> 
> hi,Punam
>    I've applied the patch,but the image displays right in gallery
Thanks Yang. Can you please confirm that image was rotating in gallery before applying the patch and with the patch applied the image started displaying right?

 but is
> rotated when crop it using pick activity from contacts.

Comment 30

[James Zhang (Spreadtrum)]

(In reply to Punam Dahiya from comment #29)
> (In reply to yang.zhao from comment #26)
> > (In reply to Punam Dahiya from comment #25)
> > > (In reply to James Zhang from comment #23)
> > > > Yang will try it, thanks.
> > > 
> > > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > > https://github.com/mozilla-b2g/gaia/pull/17353
> > 
> > hi,Punam
> >    I've applied the patch,but the image displays right in gallery
> Thanks Yang. Can you please confirm that image was rotating in gallery
> before applying the patch and with the patch applied the image started
> displaying right?
> 
>  but is
> > rotated when crop it using pick activity from contacts.
It display right in gallery but wrong in editor, without patch.

Comment 31

[yang.zhao]

Yes,just like what James said.Without the patch,the image also displays right in gallery but wrong in contacts.

Comment 32

[Punam Dahiya [:pdahiya]]

(In reply to yang.zhao from comment #31)
> Yes,just like what James said.Without the patch,the image also displays
> right in gallery but wrong in contacts.

Thanks Yang for confirming. Can you please provide info requested in comment#28. Thanks

Comment 33

[Punam Dahiya [:pdahiya]]

(In reply to Punam Dahiya from comment #28)
> (In reply to yang.zhao from comment #26)
> > (In reply to Punam Dahiya from comment #25)
> > > (In reply to James Zhang from comment #23)
> > > > Yang will try it, thanks.
> > > 
> > > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > > https://github.com/mozilla-b2g/gaia/pull/17353
> > 
> > hi,Punam
> >    I've applied the patch,but the image displays right in gallery but is
> > rotated when crop it using pick activity from contacts.
> 
> Thanks James for helping test. PR
> https://github.com/mozilla-b2g/gaia/pull/17353 has fix for writing EXIF data
> into edited image either by clicking on thumbnail from thumbnail list in
> gallery or cropping using pick activity from contacts or SMS.
> 
> Can you please confirm that you are seeing EXIF data in edited images in
> both the above cases? 
> 
> I have tarako device installed with 1.3t build, BuildId: 20140310141338. I
> tried to replicate the original issue of image rotating on edit in gallery
> but not able to replicate. Here's the video of steps followed
> http://www.youtube.com/watch?v=SvhQgAnbXLo
> 
> It will help us if you can provide the video or exact steps to replicate the
> image rotation bug. Thanks

Setting NeedInfo flag for above information. Thanks

Comment 34

[yang.zhao]

(In reply to Punam Dahiya from comment #33)
> (In reply to Punam Dahiya from comment #28)
> > (In reply to yang.zhao from comment #26)
> > > (In reply to Punam Dahiya from comment #25)
> > > > (In reply to James Zhang from comment #23)
> > > > > Yang will try it, thanks.
> > > > 
> > > > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > > > https://github.com/mozilla-b2g/gaia/pull/17353
> > > 
> > > hi,Punam
> > >    I've applied the patch,but the image displays right in gallery but is
> > > rotated when crop it using pick activity from contacts.
> > 
> > Thanks James for helping test. PR
> > https://github.com/mozilla-b2g/gaia/pull/17353 has fix for writing EXIF data
> > into edited image either by clicking on thumbnail from thumbnail list in
> > gallery or cropping using pick activity from contacts or SMS.
> > 
> > Can you please confirm that you are seeing EXIF data in edited images in
> > both the above cases? 
> > 
> > I have tarako device installed with 1.3t build, BuildId: 20140310141338. I
> > tried to replicate the original issue of image rotating on edit in gallery
> > but not able to replicate. Here's the video of steps followed
> > http://www.youtube.com/watch?v=SvhQgAnbXLo
> > 
> > It will help us if you can provide the video or exact steps to replicate the
> > image rotation bug. Thanks
> 
> Setting NeedInfo flag for above information. Thanks

I can't open the youtube website.But I've also tried the version in March 10.It's OK,because we had a patch in camera then,please see bug 980914 comment #0,but now James has removed the patch in camera for it needs extra 3M memory,and hoped mozilla could resolve it in gallery to save memory.Please ask James for detail.Thank you.

Comment 35

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

(In reply to yang.zhao from comment #34)
> (In reply to Punam Dahiya from comment #33)
> > (In reply to Punam Dahiya from comment #28)
> > > (In reply to yang.zhao from comment #26)
> > > > (In reply to Punam Dahiya from comment #25)
> > > > > (In reply to James Zhang from comment #23)
> > > > > > Yang will try it, thanks.
> > > > > 
> > > > > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > > > > https://github.com/mozilla-b2g/gaia/pull/17353
> > > > 
> > > > hi,Punam
> > > >    I've applied the patch,but the image displays right in gallery but is
> > > > rotated when crop it using pick activity from contacts.
> > > 
> > > Thanks James for helping test. PR
> > > https://github.com/mozilla-b2g/gaia/pull/17353 has fix for writing EXIF data
> > > into edited image either by clicking on thumbnail from thumbnail list in
> > > gallery or cropping using pick activity from contacts or SMS.
> > > 
> > > Can you please confirm that you are seeing EXIF data in edited images in
> > > both the above cases? 
> > > 
> > > I have tarako device installed with 1.3t build, BuildId: 20140310141338. I
> > > tried to replicate the original issue of image rotating on edit in gallery
> > > but not able to replicate. Here's the video of steps followed
> > > http://www.youtube.com/watch?v=SvhQgAnbXLo
> > > 
> > > It will help us if you can provide the video or exact steps to replicate the
> > > image rotation bug. Thanks
> > 
> > Setting NeedInfo flag for above information. Thanks
> 
> I can't open the youtube website.But I've also tried the version in March
> 10.It's OK,because we had a patch in camera then,please see bug 980914
> comment #0,but now James has removed the patch in camera for it needs extra
> 3M memory,and hoped mozilla could resolve it in gallery to save
> memory.Please ask James for detail.Thank you.

hi Yang Zhao, can you record a video of you testing the patch and show the scenario that does not work for you? thanks

Comment 36

[Steven Yang [:styang]]

(In reply to yang.zhao from comment #34)
> (In reply to Punam Dahiya from comment #33)
> > (In reply to Punam Dahiya from comment #28)
> > > (In reply to yang.zhao from comment #26)
> > > > (In reply to Punam Dahiya from comment #25)
> > > > > (In reply to James Zhang from comment #23)
> > > > > > Yang will try it, thanks.
> > > > > 
> > > > > Thanks James, Yang. Here's the link to PR that will apply cleanly to 1.3T
> > > > > https://github.com/mozilla-b2g/gaia/pull/17353
> > > > 
> > > > hi,Punam
> > > >    I've applied the patch,but the image displays right in gallery but is
> > > > rotated when crop it using pick activity from contacts.
> > > 
> > > Thanks James for helping test. PR
> > > https://github.com/mozilla-b2g/gaia/pull/17353 has fix for writing EXIF data
> > > into edited image either by clicking on thumbnail from thumbnail list in
> > > gallery or cropping using pick activity from contacts or SMS.
> > > 
> > > Can you please confirm that you are seeing EXIF data in edited images in
> > > both the above cases? 
> > > 
> > > I have tarako device installed with 1.3t build, BuildId: 20140310141338. I
> > > tried to replicate the original issue of image rotating on edit in gallery
> > > but not able to replicate. Here's the video of steps followed
> > > http://www.youtube.com/watch?v=SvhQgAnbXLo
> > > 
> > > It will help us if you can provide the video or exact steps to replicate the
> > > image rotation bug. Thanks
> > 
> > Setting NeedInfo flag for above information. Thanks
> 
> I can't open the youtube website.But I've also tried the version in March
> 10.It's OK,because we had a patch in camera then,please see bug 980914
> comment #0,but now James has removed the patch in camera for it needs extra
> 3M memory,and hoped mozilla could resolve it in gallery to save
> memory.Please ask James for detail.Thank you.

Please try the patch on current build, we should ignore what you did in camera driver. Thanks!

Comment 37

[yang.zhao]

Attachment: VID_20140325_193106.3gp

The picture is photoed vertically.It displays right in gallery but is rotated in contacts.

Comment 38

[David Flanagan [:djf]]

This bug was originally filed for the general issue of preserving EXIF data when the Gallery saves an edited image.  That was important but low-priority. For Tarako, we now have an urgent high-priority need for a subset of this functionality. Note that for Tarako, this is no longer just a Gallery bug, because it will affect any app that resizes an image (like sms and contacts).

To understand this bug you have to understand how the Tarako firmware is different than other devices we've worked with. For the sake of round numbers, assume we have a camera sensor that is 800x600 pixels.  If you hold the phone in portrait mode, the camera firmware on all the other devices we've worked with rotate the 800x600 image from the sensor and save it as a JPEG image that is 600x800. The firmware on the Tarako can't do that (or can't do it without taking a lot of memory).  It always writes a JPEG file that is 800x600, but includes a byte in the EXIF data to record the rotation of the image.  Before displaying the image, we have to read the orientation data so we know how to rotate it.

Over the summer of 2013 our intern Tom Herold implemented this in Gallery and Camera: those apps can read the orientation byte and display images correctly.  From the video above, it appears that his work did not include the image editor portion of the gallery.  This is probably because we were also planning to add an image rotation feature in the editor so deferred the work there.  Or maybe he did fix the image editor and we've since regressed it.

So, when dealing with photos from the Tarako's camera, two things are necessary:

1) we have to retain the EXIF data because that is where the rotation information is stored

2) we have to honor that EXIF rotation information when displaying the image.

This bug was about #1  This is hard. The only web API for encoding a JPEG image is the Canvas.toBlob() function.  It creates a JPEG file without any EXIF data.  So this bug was about all the hard work of parsing EXIF data out of the original file, modifying it as necessary, and then inserting it into the JPEG file created by Canvas.toBlob().

But even when we preserve the EXIF data in an edited image, that doesn't handle issue #2.  The video attached above shows the contacts app displaying a sideways image. That was an uncropped image, so it was passed directly to the contacts app with unmodified EXIF data.  Fortunately, issue #2 is easier than issue #1.  Firefox handles it with a CSS property: image-orientation: from-image.  See https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation  (I did not learn about this property until after Tom had completed his Gallery fixes, so note that Gallery rotates images more manually rather than using this CSS property.) To get Contacts to display the image correctly it might simply be a matter of setting this CSS.

On the other hand, apps like Contacts and SMS include code to resize images.  Contacts needs to do this to create thumbnails. And SMS needs to do this to produce smaller versions of images that will fit in an MMS message. The only way they have to resize an image is to use Canvas.toBlob() which strips the EXIF data. And once the EXIF data is gone, the CSS image-orientation property will not help to display the image correctly.  Those apps do not have to preserve EXIF data, but they may need to take orientation into account when resizing the image so they rotate the image when they copy it, to produce an image that does not need EXIF orientation data.

So what I'm saying here is that reverting the Tarako camera firmware to save memory by using EXIF rotation instead of real rotation does not just affect Gallery and Camera: it also affects anything that uses images that come from the camera.

Here are some options I see for resolving this issue.  These are in order from best to worst:

1) Change the Tarako firmware back to do real rotation. I see in the comments above that this would take 3M of memory, and that surprises me. The rotation should be a lossless rearrangement of blocks in the jpeg file, so in the worst case, I would expect it to require twice the file size of the original image, which would probably be under 1M.  And with a streaming solution it could probably be done using only one copy of the file. I'm not a JPEG expert. But if it could be done in the firmware with less memory, that would be the best solution.

2) We could implement lossless jpeg rotation in JavaScript and add that to the Camera app and have it rewrite the JPEG files created by the camera firmware. That way we'd have to fix it only in one app.

3) We could drop this bug and its EXIF saving stuff.  We'd fix the image editor so it respected EXIF orientation.  And then we'd modify Gallery and Camera so that for any pick activity, they always did the rotation themselves before returning a blob to the caller. This would mean that all other apps would get images that would display correctly without any additional work in those other apps.  Because this constrains the fix to just two apps, it seems like the better way to go.

4) We could proceed with this EXIF saving patch and also fix the image editor issue, and also try to patch SMS and Contacts and any other app that displays images.  But that seems like the wrong way to go.

If we go with options 1 through 3, then we should still eventually land the code in this patch, but it doesn't have to happen for 1.3T.

Comment 39

[David Flanagan [:djf]]

James: when you had image rotation in the firmware, were you doing a lossless rotation with libjpeg or something similar?  Did it really take 3M of memory?

Comment 40

[James Zhang (Spreadtrum)]

(In reply to David Flanagan [:djf] from comment #39)
> James: when you had image rotation in the firmware, were you doing a
> lossless rotation with libjpeg or something similar?  Did it really take 3M
> of memory?

I have updated the memory usage status, it take about 2MB memory for 1600x1200 picture.
Dafeng is the camera HAL owner, he will give you detail information.

And I think if you get picture with rotation EXIF data by sdcard or bluetooth or network download, it also has this issue. So it's not spreadtrum camera rotation issue, it's common issue.

Comment 41

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

triage: let's land this patch 1) we have to retain the EXIF data because that is where the rotation information is stored
and we should open another bug for 2) we have to honor that EXIF rotation information when displaying the image. in any app

Steven will discuss with James further on the rest of the scenarios. thanks

Comment 42

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #41)
> triage: let's land this patch 1) we have to retain the EXIF data because
> that is where the rotation information is stored
> and we should open another bug for 2) we have to honor that EXIF rotation
> information when displaying the image. in any app

Did you notice that of the four ways forward that I proposed, landing this patch is my very least recommended option?  Eventually I don want to add the ability to preserve EXIF data in edited images. That is important. But doing that in a rush and uplifting to 1.3T scares me.

The patch is 3300 lines long and the EXIF writing code is not well tested. Since this PR was created, there has been a bug filed against the upstream EXIF handling library. Also landing this patch allows gallery to preserve EXIF data, but doesn't do anything for apps that get an image from Gallery and want to resize it.  We're talking about quite a bit of destabilizing work here in the other apps that display and resize images.

> Steven will discuss with James further on the rest of the scenarios. thanks

Comment 43

[David Flanagan [:djf]]

(In reply to David Flanagan [:djf] from comment #38)
> 
> 2) We could implement lossless jpeg rotation in JavaScript and add that to
> the Camera app and have it rewrite the JPEG files created by the camera
> firmware. That way we'd have to fix it only in one app.
> 

I've spent some time reading up on JPEG internals, and I now think that this proposal is unrealistically complex unless someone has already written the code to do this in JavaScript.

Comment 44

[David Flanagan [:djf]]

(In reply to James Zhang from comment #40)
> (In reply to David Flanagan [:djf] from comment #39)
> > James: when you had image rotation in the firmware, were you doing a
> > lossless rotation with libjpeg or something similar?  Did it really take 3M
> > of memory?
> 
> I have updated the memory usage status, it take about 2MB memory for
> 1600x1200 picture.
> Dafeng is the camera HAL owner, he will give you detail information.

Thanks. Now that I've learned more about JPEG, I realize that a lossless rotation probably takes more memory than I thought.  I somehow thought that the MCU blocks could be re-arranged without any decompression, but I see now that huffman decodeing and re-encoding is necessary on the blocks, so 2M of memory no longer seems so high to me.  Sorry to question that.

> 
> And I think if you get picture with rotation EXIF data by sdcard or
> bluetooth or network download, it also has this issue. So it's not
> spreadtrum camera rotation issue, it's common issue.

What other phones emit unrotated photos with EXIF flags?  Is that common for feature phone cameras?  The fundamental problem, I think, is that the web platform pretty much ignores EXIF rotation. It may be common to use EXIF rotation in low-end devices, but it is very uncommon on the web. So what should a low-end web device do? 

Do we try to make all apps EXIF-aware? I doubt we have time to fix all of the Gaia apps correctly.  And teaching web developers with apps in the marketplace that they should care about EXIF rotation is probably very hard. Third party apps that use photos from the camera probably won't work right on Tarako.

Do we break compatibility with the web and ship Tarako with a version of gecko that honors EXIF rotation by default?

Comment 45

[David Flanagan [:djf]]

I did a little bit of comparison.  I started with this EXIF rotate image https://github.com/recurser/exif-orientation-examples/blob/master/Portrait_6.jpg

I created a trivial HTML file:

   <img src="Portrait_6.jpg">

Firefox, Chrome, or Safari all ignore the EXIF rotation data and display the image in landscape mode.

In all three browsers, if I just view the jpeg file directly (without the HTML file) the EXIF rotation is honored and the file is displayed correctly.

Next, I emailed the photo to myself:

  Thunderbird ignored the EXIF rotation 

  Zimbra ignored the EXIF rotation when offering a preview of the attachment. But when I clicked on the attachment it displayed the jpeg in a tab by itself (without HTML) so it did display correctly.

  gmail on both desktop and Android honored the EXIF rotation and displayed the image correctly.

Next, I tested android.  After viewing the image in the Android gmail app, I saved it.  Then, I tried to use it in Android:

  - When I view the image in the Android gallery app, the EXIF orientation is honored (just like FxOS Gallery app does)

  - When I attach the image to an MMS message, the EXIF orientation is ignored (just like in FxOS)

  - When I use the image for a contact, Android first forces me to crop it using the gallery app.  I belive this cropping step produces a correctly rotated image without EXIF data.  In any case, the cropped portion of the image is displayed correctly in the Android contacts app. This is what I would expect in FxOS as well, if we forced a square crop for contact pictures like we used to do long ago.

  - When I use the image as wallpaper, Android has me crop it first, and that seems to fix the orientation, so it displays correctly.  (On FxOS the gallery image editor and crop code does not honor EXIF orientation they way it should so wallpaper is sideways for FxOS).

Comment 46

[David Flanagan [:djf]]

So here are my latest thoughts on this.

1) The web platform does not support EXIF orientation and does not even expose an EXIF parsing mechanism that allows apps to support it easily.  We could try to fix this in gecko, so that images would automatically be decoded in the orientation specified by EXIF, but I'm guessing that that would break compatibility in important ways.  But it might still be worth discussing this.  What if there was a manifest variable or a mozbrowser attribute or something so that apps could declare whether they wanted EXIF support or not.  The browser app could ignore EXIF for compatibility with desktop browsers.  But apps like SMS and Contacts could be configured so that EXIF orientation was honored.  If Gecko automatically rotated our images for us, then the Gallery image editor and cropping code would always produce correctly-oriented images, so even if EXIF data was not saved with those images they would display correctly in other apps.  I'd guess that it is unlikely that we could make the necessary Gecko changes in the 1.3T timeframe.

2) I'd still like it if the Tarako camera firmware just always rotated the images for real and never used EXIF orientation.  That makes the problem go away.  Though if we are expecting to interoperate with a ecosystem of feature phones that rely on exif orientation (and I don't know if we are--clarification here would be good) then just fixing the issue on Tarako won't be enough.  I also understand that if the camera firmware can't do this rotation natively on the raw sensor data then adding a lossless rotation step after the fact requires a lot of memory, especially if it is being done in the parent process or the kernel or wherever that code runs.  But given that Spreadtrum has code that can make this happen with a 2mb overhead, and give our time constraints, switching this rotation code back on might be our least bad option.

3) If doing a lossless jpeg rotation takes too much memory to do in the firmware or in gonk, maybe it would be more acceptable to do it in the child process.  If we used emscripten to compile jpegtrans we could have the camera fix the orientation of all photos before saving them.  See bug 951710 for some related experiments. Apparently we'd end up with hundreds of kb of JavaScript code if we did this, and that scares me.  Maybe it is worth experimenting with this. If we have a code that Camera can use to do lossless rotation, then Gallery could use it, too, when it encounters EXIF-rotated images from other sources.

4) If we can't just isolate the problem in the camera and ensure that all images it produces are correctly rotated, then we could try instead to make Camera and Gallery be the only two apps that need to be EXIF-aware. For this approach, we'd have to fix the Gallery's image editing and cropping code so that it supports EXIF orientation, and then we'd ensure that any time an app used an activity to pick an image from Camera or Gallery we always gave them one that was properly rotated without EXIF orientation.  In the case where we are resizing or cropping the image, this is easy because we have to decode it to do that anyway. The problem with this approach is that we'd have to decode and re-encode any image that had EXIF orientation even if the user did not crop it. Any time the user picked a portrait mode photo on Tarako, we'd decode it and re-encode it.  For 2mp photos, this is 8mb of memory, and loss of image quality.  Given that this is a pick activity, it means that two apps have to be running at the same time and neither one can be killed. Experience tells me that the 8mb of memory to decode and rotate the image is likely to kill the homescreen or other background apps. (We could reduce the memory overhead by automatically resizing all images when they are picked. If we cut all 2mp images down to 1mp when picked that would halve the memory requirements. It might be hard to say we have a 2mp camera on the phone if we discard half the pixels like this, but there would still be full 2mp image files on the phone, but you'd only be able to get to them via USB) The advantage to this approach, though, is that all other apps can ignore EXIF orientation, just as they normally do on the web. Any app that receives an image though a pick activity can be assured that the image is correctly rotated.  (Only apps that use device storage to access the image files themselves will need to be EXIF aware).  

5) If we can't afford the memory to rotate every image when it is picked, then we have to modify all apps that use picked images to make them EXIF-aware.  (Note that even under this approach, we still don't have to fix this particular EXIF writing bug.  When we save an edited or cropped image, we don't have to restore its EXIF data but we do have to be sure to save it with proper rotation so that EXIF orientation is not required.) Making apps EXIF-aware means:

  - apps that display images with <img> tags will use image-orientation: from-image in their CSS

  - apps that display images with CSS background-image might have to handle rotation manually. I'm unclear about this

  - apps that resize images (contacts, sms, and possibly the wallpaper setting code in Settings and Homescreen?) will have to take EXIF orientation into account. These apps will have to use the jpeg_metadata_parser.js code developed for gallery so that they can determine the correct orientation. We'll want to write some sort of shared utility function for doing this. (Resizing is done with an off-screen image, so the CSS image-orientation solution doesn't help here, unless maybe there is a hack where we use an on-screen image with image-orientation:from-image and opacity:0 for performing the resize.)

 - We'll also have to accept that marketplace apps that use photos will have compatibility problems on Tarako. A lot of developer education will be required to address this to teach app developers that they should care about EXIF orientation. Since the web platform does not expose EXIF orientation, we're going to be telling web developers that they have to work around the web platform and parse JPEG files to extract the EXIF orientation flags and then honor those. 

The problem with this approach is that we can't isolate the problem to one or two apps and modifying four or five apps will be difficult, time consuming, and destabilizing.

Looking over these choices, I don't like any of them.  The 2mb overhead in the camera firmware seems like the least bad choice to me, but I don't know enough about memory on Tarako to know whether that is actually an option.

Setting needinfo on a bunch of people here because I think there is an important decision to make here.  If you've been needinfo'ed ignore the title of this bug and just read from about comment 38 on.

needinfo Fabrice: because it is an important 1.3T issue.

needinfo Jonas: because of the more general issue of EXIF orientation support on the web

needinfo Milan: in case the graphics team has some kind of magic that can help here.

needinfo Jeff because you emscriptenified cjpeg in bug 951710 and I wonder if you'd consider doing the same for a stripped-down rotation only version of jpegtrans so we can do lossless image rotation in JS.

needinfo James: because in comment #40 you said that we should be concerned about photos with EXIF orientation from other sources. Could you elaborate on that? Where would those photos come from? From feature phones?

Comment 47

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

(In reply to David Flanagan [:djf] from comment #42)
> (In reply to Joe Cheng [:jcheng] from comment #41)
> > triage: let's land this patch 1) we have to retain the EXIF data because
> > that is where the rotation information is stored
> > and we should open another bug for 2) we have to honor that EXIF rotation
> > information when displaying the image. in any app
> 
> Did you notice that of the four ways forward that I proposed, landing this
> patch is my very least recommended option?  Eventually I don want to add the
> ability to preserve EXIF data in edited images. That is important. But doing
> that in a rush and uplifting to 1.3T scares me.
> 
> The patch is 3300 lines long and the EXIF writing code is not well tested.
> Since this PR was created, there has been a bug filed against the upstream
> EXIF handling library. Also landing this patch allows gallery to preserve
> EXIF data, but doesn't do anything for apps that get an image from Gallery
> and want to resize it.  We're talking about quite a bit of destabilizing
> work here in the other apps that display and resize images.
> 
> > Steven will discuss with James further on the rest of the scenarios. thanks

yes i did and i also understand that we are under time constraints so that my proposal is to land whatever we have came up with now (photo editor will save the EXIF information so that at least after editing, photo will still show the correct orientation in gallery app, at least we fixed one use case)

Regarding other use cases such to display photo in correct orientation in contacts photo / attaching photo in MMS / photo editor, will try to talk to partner to see if we can live with this for now

Comment 48

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #47)

> 
> yes i did and i also understand that we are under time constraints so that
> my proposal is to land whatever we have came up with now (photo editor will
> save the EXIF information so that at least after editing, photo will still
> show the correct orientation in gallery app, at least we fixed one use case)

The problem is that when I asked Punam to resume the work on saving EXIF information I didn't realize that the image editor and cropping code ignored the EXIF information.  I don't know how we forgot that or how we regressed that, but that is the bug we need to fix, so that the image editor/cropper always saves images with the proper orientation so that EXIF data is not needed.  That's the bug that needs to be fixed (not sure if we have a bug filed yet) at a minimum.  I don't think that the patch that is currently attached is strictly needed and it seems quite risky to me.
 
> Regarding other use cases such to display photo in correct orientation in
> contacts photo / attaching photo in MMS / photo editor, will try to talk to
> partner to see if we can live with this for now

That sounds good. My experiments with Android indicate that EXIF orientation support is not complete on that platform either.

Comment 49

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #48)
> (In reply to Joe Cheng [:jcheng] from comment #47)
> 
> > 
> > yes i did and i also understand that we are under time constraints so that
> > my proposal is to land whatever we have came up with now (photo editor will
> > save the EXIF information so that at least after editing, photo will still
> > show the correct orientation in gallery app, at least we fixed one use case)
> 
> The problem is that when I asked Punam to resume the work on saving EXIF
> information I didn't realize that the image editor and cropping code ignored
> the EXIF information.  I don't know how we forgot that or how we regressed
> that, but that is the bug we need to fix, so that the image editor/cropper
> always saves images with the proper orientation so that EXIF data is not
> needed.  That's the bug that needs to be fixed (not sure if we have a bug
> filed yet) at a minimum.

Just created the bug. Here's the bug number https://bugzilla.mozilla.org/show_bug.cgi?id=989026

  I don't think that the patch that is currently
> attached is strictly needed and it seems quite risky to me.
>  
> > Regarding other use cases such to display photo in correct orientation in
> > contacts photo / attaching photo in MMS / photo editor, will try to talk to
> > partner to see if we can live with this for now
> 
> That sounds good. My experiments with Android indicate that EXIF orientation
> support is not complete on that platform either.

Comment 50

[Milan Sreckovic [:milan] (needinfo for best results)]

Jeff is the right person to comment on the "magic" part that was listed as my question.

Comment 51

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

Hi David,

From Taipei triage, it was felt that Bug 989026 seems to provide a good solution.
Do you think we can have it done for Tarako given that we still have couple weeks of time? and could we have an effort estimation on what's required to get this done? Thanks

Comment 52

[David Flanagan [:djf]]

Joe,

Punam is working on that bug and I've asked her to give an estimate. I expect it will be done next week.

Note, however, that fixing bug 989026 will not be enough by itself, since that only affects edited and cropped images.  If we want uncropped portrait-mode photos to be displayed correctly by apps like SMS and Contacts then we also have to do one of these things:

1) Fix gecko to support EXIF orientation. Not going to happen in time.

2) Revert Spreadtrum's recent change, pay the memory price, and do real rotation of the image in the camera firmware.  This is the only quick and easy solution.

3) Use emscripten on jpegtrans and do real lossless rotation of the image in the camera app. It is not clear what the memory and performance penalty of this would be. Research by someone who understands emscripten could be helpful here.

4) Modify camera and gallery to do lossy image rotation when the user picks an image without cropping. This will take 8mb of memory. So picks of portait mode photos without cropping would be as expensive as picks with cropping, and would probably cause background apps to die.

5) Modify the SMS and Contacts apps to know about EXIF orientation. This might be difficult to do in the time available.

Comment 53

[David Flanagan [:djf]]

We can use bug 989290 for implementation of whichever of those five options we choose.

Comment 54

[James Zhang (Spreadtrum)]

I think gallery editor can change the EXIF data and rotate the image then crop.

Comment 55

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

(In reply to David Flanagan [:djf] from comment #52)
> Joe,
> 
> Punam is working on that bug and I've asked her to give an estimate. I
> expect it will be done next week.
> 
> Note, however, that fixing bug 989026 will not be enough by itself, since
> that only affects edited and cropped images.  If we want uncropped
> portrait-mode photos to be displayed correctly by apps like SMS and Contacts
> then we also have to do one of these things:
> 

Thanks David, since we'd like SMS/Contacts app to go through the picker all the time when selecting photo to attach to a message/contact, i'd assume the picker always rotate the pictures correctly.

so looking at this from end user point of view, in the contacts/message app, if user chooses the "gallery" option when attaching pictures, bug 989026 should handle this use case. 

the uncropped portrait-mode photos that you are referring to, if looking at this from end user point of view, in the contacts/message app, if user chooses the "camera" option when attaching pictures, this is when user will still see photo which is not properly rotated

Can you please confirm my understanding? thanks

Comment 56

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #55)

> Thanks David, since we'd like SMS/Contacts app to go through the picker all
> the time when selecting photo to attach to a message/contact, i'd assume the
> picker always rotate the pictures correctly.
> 
> so looking at this from end user point of view, in the contacts/message app,
> if user chooses the "gallery" option when attaching pictures, bug 989026
> should handle this use case. 
> 

Bug 989026 will only fix things for the case where the user actually crops the image. If they are not given the option to crop or do not use that option, then no rotation will happen.

> the uncropped portrait-mode photos that you are referring to, if looking at
> this from end user point of view, in the contacts/message app, if user
> chooses the "camera" option when attaching pictures, this is when user will
> still see photo which is not properly rotated

Correct. Bug 989026 won't fix picking from the camera either.

> Can you please confirm my understanding? thanks

Comment 57

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's probably worth always rotating the pixel information whenever we're editing the image. However I don't think we have any option but to rely on that other software will do the right thing and honor EXIF data, as spotty as the support sadly is.

So: Let's rotate the pixel data any time we're already paying the price of re-encoding the image. But otherwise lets just rely on that other apps do the right thing. I don't really see any good alternatives.

Comment 58

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

Attachment: 2014-04-02-02-57-53.png

(In reply to David Flanagan [:djf] from comment #56)
> Bug 989026 will only fix things for the case where the user actually crops
> the image. If they are not given the option to crop or do not use that
> option, then no rotation will happen.

in the attached image, i see this picker when i try to pick a contact photo from contacts app using gallery

do you mean if i don't change the size of the selection (if the selection is the full image), this means no cropping and therefore no rotation will happen?
so i must change the size of the selection in order for the image to be cropped and therefore apply the correct rotation?

if this is the case, can we also apply crop / rotation even if the selection is the full image? Thanks

Comment 59

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

let's track bug 989026 for tarako and move this bug to 1.5? thanks

Comment 60

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #58)
> Created attachment 8400132 [details]
> 2014-04-02-02-57-53.png
> 
> (In reply to David Flanagan [:djf] from comment #56)
> > Bug 989026 will only fix things for the case where the user actually crops
> > the image. If they are not given the option to crop or do not use that
> > option, then no rotation will happen.
> 
> in the attached image, i see this picker when i try to pick a contact photo
> from contacts app using gallery
> 
> do you mean if i don't change the size of the selection (if the selection is
> the full image), this means no cropping and therefore no rotation will
> happen?

Yes, that's what I mean. Also, however, some apps (and the <input type="file"> selector) invoke the pick activity in such a way that cropping is not allowed at all, and in that case no rotation will happen at all.

> so i must change the size of the selection in order for the image to be
> cropped and therefore apply the correct rotation?
> 
> if this is the case, can we also apply crop / rotation even if the selection
> is the full image? Thanks

That is one of the possible approaches. We have to do one of the 5 things listed in comment #52.  This is option 4. I think it will cause serious problems. It will require as much memory as actually cropping the image, but we already removed the cropping option for <input type='file'> image picks because the memory required to crop was causing the browser to exit.  So bringing back a forced crop to rotate images is probably destined to fail.  When the activity is in progress the system has to keep two apps alive, which means that memory pressure is at its highest. It is not, therefore, a good time to add an extra 16mb memory requirement.

As I've said before, I think we should just ask spreadtrum to turn lossless rotation back on in their camera firmware.  If they can do it with 2mb memory use in the camera driver, that is much better than the 16mb that it will take to do it in the camera and gallery apps.

but not the approach I recommend, because of the very high memory usage it entails.

Comment 61

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #59)
> let's track bug 989026 for tarako and move this bug to 1.5? thanks

Bug 989026 is specific to cropping in the gallery and by itself will not be enough.  Bug 989290 would be the better meta bug for tracking the entire issue.

Comment 62

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

(In reply to David Flanagan [:djf] from comment #60)
> That is one of the possible approaches. We have to do one of the 5 things
> listed in comment #52.  This is option 4. I think it will cause serious
> problems. It will require as much memory as actually cropping the image, but
> we already removed the cropping option for <input type='file'> image picks
> because the memory required to crop was causing the browser to exit.  So
> bringing back a forced crop to rotate images is probably destined to fail. 
> When the activity is in progress the system has to keep two apps alive,
> which means that memory pressure is at its highest. It is not, therefore, a
> good time to add an extra 16mb memory requirement.

I would assume Bug 989026 can do the cropping and rotation in the picker.
Just a wild idea, if the user does not change the area selection in the picker (therefore selection is the full image), can we process this image with what's done in Bug 989026 and choose 99% of the image (so 1% of it is cropped or just leave out 1 pixel at all the edges) and do the rotation?

(In reply to David Flanagan [:djf] from comment #61)
> (In reply to Joe Cheng [:jcheng] from comment #59)
> > let's track bug 989026 for tarako and move this bug to 1.5? thanks
> 
> Bug 989026 is specific to cropping in the gallery and by itself will not be
> enough.  Bug 989290 would be the better meta bug for tracking the entire
> issue.

i will respond in bug 989290 as well. we probably will not be able to fix the entire issue within the tarako timeframe so i am trying to understand what's the best that can be done for tarako. thanks

Comment 63

[[:fabrice] Fabrice Desré]

David, I saw a lot of discussions happening while I was PTO. Do you still need my input here?

Comment 64

[Punam Dahiya [:pdahiya]]

Comment on attachment 8393115 [details] [review]
PR updating edited images with EXIF data from original image

The attached patch is bit rotted and needs to be revisited after the fixes landed as part of bug 989290. Canceling the review request on the patch.

Comment 65

[David Flanagan [:djf]]

Bulk edit to clear old and out of date needinfo requests that I never responded to. I'm assuming that these are no longer relevant. If you are still waiting for an answer from me, please set needinfo? again.

Comment 66

[Hubert Figuiere [:hub]]

Bug 1178812 is related.

Comment 68

[Hubert Figuiere [:hub]]

This is dealt with bug 1178812.

Comment 69

[Hubert Figuiere [:hub]]

clearing NI