1178290 - Upload to Flickr seriously alter picture metadata and size

Status: RESOLVED FIXED

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

Description

[Hubert Figuiere [:hub]]

-Go to Flickr.com. Login.
-Go to upload a picture
-Select a picture
-Tap upload
-Picture is uploaded

Expected:
-The title is the name of the picture
-The size is the original size
-Exif information is there

Observed
-The title is "blob"
-The size is barely 1MPix (original is ~20 since I am on the foxfooding device)
-Exif data is present

I am pretty sure all of these problems are on our side.

Comment 1

[David Flanagan [:djf]]

Are you selecting the picture from Gallery or from Camera? I imagine that you used Gallery and that the bug is with that app, not with Browser.

The root cause here is that the Aries device uses EXIF orientation. If you take landscape photo (with the physical shutter button on the upper right of the device) then I think you'll see the original photo being uploaded. If that works properly then the bug is related ot EXIF. If it still does not work, then some of the fault is with Flickr.

The picture is being aggressively resized because Aries relies on EXIF orientation, so we have to rotate it before allowing any other app to see it. For low-memory devices, we have to be very careful about the memory required for large images, especially during a pick activity when two apps must remain open.  What is probably happening is that the gallery is using #-moz-samplesize=4 when decoding the image to rotate it. This means width and height are 1/4 of the original, resulting in 1/16th of the original megapixels.

If we put a gallery.json config file in the spark config directory we could fix this, but it would then break when people try out spark on lower-memory devices.

Or, more complicatedly, we could convert the gallery to use runtime memory checks rather then build time configuration options and rotate the full-size image on 2gb devices like Aries.

The loss of the file name and EXIF data is also caused by the fact that we have to rotate the image in Gallery before passing it back to the requesting application.

Hub: can you check whether this bug goes away for landscape photos?

Doug: Are you willing to make the distros/spark directory specific to the Aries foxfooding device? If so, we can put useful gallery.json and camera.json configuration files there  to tune those apps to the device. Note that doing this may cause bugs when spark is run on devices like Flame, however.

Punam: we were just talking at Whistler about all the image size configuration options in gallery. Here's a case where a runtime query of available memory on the device would be better than a build-time configuration option. I'm needinfoing you in case this is a bug you'd like to take.

Comment 2

[David Flanagan [:djf]]

Changing this to a gallery bug, though I think that Camera probably has the same or similar issues as well.

Comment 3

[Hubert Figuiere [:hub]]

(In reply to David Flanagan [:djf] from comment #1)
> Are you selecting the picture from Gallery or from Camera? I imagine that
> you used Gallery and that the bug is with that app, not with Browser.

Gallery.

> Hub: can you check whether this bug goes away for landscape photos?

This was a landscape photo.

BTW resizing should keep the EXIF. Rotating should keep the EXIF.

Comment 4

[David Flanagan [:djf]]

(In reply to Hubert Figuiere [:hub] from comment #3)
> 
> > Hub: can you check whether this bug goes away for landscape photos?
> 
> This was a landscape photo.

Would you try the other landscape orientation, then? I really think this bug should not happen for photos without EXIF.  (Unless the file selection code that invokes the activity is specifying a size limit...)

> 
> BTW resizing should keep the EXIF. Rotating should keep the EXIF.

Yes I agree, but we can't do that right now. Diego wrote an EXIF parser/writer long ago but it never quite got to a landable state and we got pulled off on to other things.  In both cases (resizing and rotating) it is not correct to just keep all of the EXIF. It needs to be correctly edited.  Maybe this will be the bug and the device that makes EXIF editing a priority again.   It should probably be a separate bug, though. Let's keep this one about the resizing issue.

Comment 5

[Hubert Figuiere [:hub]]

(In reply to David Flanagan [:djf] from comment #4)
> (In reply to Hubert Figuiere [:hub] from comment #3)
> > 
> > > Hub: can you check whether this bug goes away for landscape photos?
> > 
> > This was a landscape photo.
> 
> Would you try the other landscape orientation, then? I really think this bug
> should not happen for photos without EXIF.  (Unless the file selection code
> that invokes the activity is specifying a size limit...)

Vertical image from gallery too. All the same. Not sure how to check the file selection code for image constraints.

But if I take from the camera directly, EXIF is gone (vertical image only) but the title is consistent, so is the size.

To summarize:
- EXIF is stripped on rotation
- If we pick image from gallery it is downsized and the name removed.

I think we can split this into two bugs.

Comment 6

[Hubert Figuiere [:hub]]

Filed bug 1178812 for the EXIF part.

Comment 7

[Punam Dahiya [:pdahiya]]

I am able to replicate the issue where the image uploaded from gallery to flickr.com has size reduced and image name changed to blob. This is not happening for images uploaded from camera.

On Aries from gallery app, image size 5248 x 3936 when uploaded is reduced to 984 x 1312 indicating pick activity is down sampling image reducing width and height by 1/4. Taking bug to investigate and submit fix. Thanks!

Comment 8

[Punam Dahiya [:pdahiya]]

Sorry typo, should be 5248 x 3936 is reduced to 1312 x 984

Comment 9

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug1178290 > mozilla-b2g:master

Comment 10

[Punam Dahiya [:pdahiya]]

I investigated this issue on both Flame and Aries and able to replicate this issue on both devices.

Flame has a 5 MP camera and pictures taken using Flame are of resolution ~ 1944 x 2592

Aries has a 20 MP camera and pictures taken using Aries are of resolution ~ 3936 x 5248

On Flame this issue is a regression due to fix of Bug 1169407. The fix restrict outputSize of picked image to half of CONFIG_MAX_IMAGE_PIXEL_SIZE, if CONFIG_MAX_PICK_PIXEL_SIZE is not defined. 

For Flame this means output size is reduced to ~2.6 MPixels (half of default 5242880) and all picked images get downsized using crop_resize_rotate API.

https://github.com/mozilla-b2g/gaia/commit/eecd9d2bb220e10792762fc274c910cb3febbd72#diff-df502a989874ab279298b77bfc36d872R289


In 2.2 and before Bug 1169407 fix (in master - See Bug 1065877, Bug 1064600 for fix and pick refactoring ) , if pick config option is not set, output size of picked image is not halved and kept same as max image size config option as 5242880 pixels and picked image unless cropped is not downsized and returned back as its original size.

Aries has 2048 MB memory and images being 3926x 5248 should be able to decode at 20 MP, so unless we have build time config pick option set to 20 MP, for pick activity default output size of 5MP will always be less and images will always be downsized.

Comment 11

[Punam Dahiya [:pdahiya]]

Hi David

As suggested in #comment 1, I have attached patch where in we are setting CONFIG_MAX_PICK_PIXEL_SIZE at run time same as image decode size based of device memory. For Aries with 2048 MB memory, this will set pick output size to 20971520 pixels which should handle 3936 x 5248 images.

For Flame
with memory  > 1024 , it will set pick output size to 10 MP,
with memory < 1024, it will set pick output size to 5 MP
with memory < 512 , it will set pick output size to 3 MP

which means for low memory flame we will still be down sizing all picked images. Please review. Thanks!

Comment 12

[Hubert Figuiere [:hub]]

To upload, we should provide the file in full size. Nothing less.

m.flickr.com is ask for a file to upload, uploads it. The whole 7+MB JPEG stream payload.

Not sure exactly why we insist on downgrading it. It is not about displaying it or editing it.

Comment 13

[Punam Dahiya [:pdahiya]]

(In reply to Hubert Figuiere [:hub] from comment #12)
> To upload, we should provide the file in full size. Nothing less.
> 
> m.flickr.com is ask for a file to upload, uploads it. The whole 7+MB JPEG
> stream payload.
> 
> Not sure exactly why we insist on downgrading it. It is not about displaying
> it or editing it.

That's correct, we insist on downgrading only for devices that are low memory and will crash because of two apps open at the same time in pick flow.

Comment 14

[David Flanagan [:djf]]

(In reply to Punam Dahiya from comment #13)
> (In reply to Hubert Figuiere [:hub] from comment #12)
> > To upload, we should provide the file in full size. Nothing less.
> > 
> > m.flickr.com is ask for a file to upload, uploads it. The whole 7+MB JPEG
> > stream payload.
> > 
> > Not sure exactly why we insist on downgrading it. It is not about displaying
> > it or editing it.
> 
> That's correct, we insist on downgrading only for devices that are low
> memory and will crash because of two apps open at the same time in pick flow.

Actually, Hub has a good point here. Why are we reducing the size on the Flame? Since no rotation is necessary, the image does not need to be decoded we can just return the blob...

For Aries, we've got the EXIF issue forcing the image to be rotated, and I thought that was the underlying issue here...

Comment 15

[David Flanagan [:djf]]

Comment on attachment 8634902 [details] [review]
[gaia] punamdahiya:Bug1178290 > mozilla-b2g:master

r- because this patch modifies a shared file to depend on a variable defined by a non-shared file.  The patch breaks tests, and it probably also breaks the camera app since it might not define CONFIG_MAX_PICK_PIXEL_SIZE.

If we need to get a fix in right away to solve this serious foxfooding issue, you can probably modify this patch to do something like this:

  if ('CONFIG_MAX_PICK_PIXEL_SIZE' in window && window.CONFIG_MAX_PICK_PIXEL_SIZE === 0) {
    window.CONFIG_MAX_PICK_PIXEL_SIZE = MediaFrame.maxImageDecodeSize
  } 

This is still a bad hack, though, because one module is modifying what is supposed to be a constant, so I'd only r+ a patch like that if we also filed a new 2.5+ bug to fix this more thoroughly.

For a more thorough fix, we should probably just get rid of all the static config stuff, query memory dynamically on startup and set the appropriate values then. That will require changes to the build script. I think we can safely assume that the memory query will complete much more quickly than the user can tap on a thumbnail and decode an image.

Comment 16

[Punam Dahiya [:pdahiya]]

(In reply to David Flanagan [:djf] from comment #15)
> Comment on attachment 8634902 [details] [review]
> [gaia] punamdahiya:Bug1178290 > mozilla-b2g:master
> 
> r- because this patch modifies a shared file to depend on a variable defined
> by a non-shared file.  The patch breaks tests, and it probably also breaks
> the camera app since it might not define CONFIG_MAX_PICK_PIXEL_SIZE.
> 
> If we need to get a fix in right away to solve this serious foxfooding
> issue, you can probably modify this patch to do something like this:
> 
>   if ('CONFIG_MAX_PICK_PIXEL_SIZE' in window &&
> window.CONFIG_MAX_PICK_PIXEL_SIZE === 0) {
>     window.CONFIG_MAX_PICK_PIXEL_SIZE = MediaFrame.maxImageDecodeSize
>   } 
> 

Agreed modifying shared /js/media_frame.js is impacting camera app and other shared module and not the best solution for this issue. I will look at setting CONFIG_MAX_PICK_PIXEL_SIZE to MediaFrame.maxImageDecodeSize inside pick.js for noCrop and when CONFIG_MAX_PICK_PIXEL_SIZE is not defined so that we are not downsizing the images in pick nocrop flow.

https://github.com/mozilla-b2g/gaia/blob/master/apps/gallery/js/pick.js#L287

However we still need to  allow 10 megapixels of image decode size per gigabyte of memory instead of existing 8 megapixels in shared/js/media_frame.js for Aries device with 2048 MB mem.

> This is still a bad hack, though, because one module is modifying what is
> supposed to be a constant, so I'd only r+ a patch like that if we also filed
> a new 2.5+ bug to fix this more thoroughly.
> 
> For a more thorough fix, we should probably just get rid of all the static
> config stuff, query memory dynamically on startup and set the appropriate
> values then. That will require changes to the build script. I think we can
> safely assume that the memory query will complete much more quickly than the
> user can tap on a thumbnail and decode an image.

Comment 17

[Punam Dahiya [:pdahiya]]

Hi David

Updated patch to set outputsize to MediaFrame.maxImageDecodeSize that's calculated based on device memory.
I am also fixing an error seen in logs for no crop flow 'cropEditor.cropRegion is undefined' which is because of variable name previously changed in ImageEditor from cropRegion to cropOverlayRegion.

Attached patch fixes downsizing issue reported in this bug for Aries device. I tested and for pictures taken with Aries with EXIF rotation 0 (rotate device counter clockwise once and take picture), the image name is returned. 

For all the other images taken using Aries, image is rotated using CropResizeRotate API and blob name returned is blob.jpeg. To fix this we could update pick activity to accept noRotate flag as data from calling app. If calling app sets noCrop, noRotate to true, cropResizeRotate API will return the original file blob without modifying it. IMO that should be a separate bug as it will need FilePicker to be updated to call pick activity with noRotate flag.Thanks

Comment 18

[David Flanagan [:djf]]

Comment on attachment 8634902 [details] [review]
[gaia] punamdahiya:Bug1178290 > mozilla-b2g:master

This looks good to me. It should have landed months ago, but got buried in my review queue and I forgot about it.

Comment 19

[David Flanagan [:djf]]

Comment on attachment 8634902 [details] [review]
[gaia] punamdahiya:Bug1178290 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression. Our old code just wasn't written with 20mp photos in mind, and this patch brings it up to date.

[User impact] if declined: photos from the foxfooding device will be unnecessarily downsized when the user tries to use them in other apps or upload them.

[Testing completed]: Punam has tested

[Risk to taking this patch] (and alternatives if risky): not risky

[String changes made]: none

Comment 20

[David Flanagan [:djf]]

Comment on attachment 8634902 [details] [review]
[gaia] punamdahiya:Bug1178290 > mozilla-b2g:master

Mahe,

Are you handling 2.5 uplift requests? 

This is not a blocker bug, but it is serious papercut for foxfooders on Aries. The patch has been sitting in my review queue for months. It is a simple fix that should have landed some time ago, but I dropped the ball. I'd love to get it in to 2.5 because it will make a difference for anyone who takes pictures with their foxfooding device.

Comment 21

[Punam Dahiya [:pdahiya]]

Thanks David for review. Patch landed on master

https://github.com/mozilla-b2g/gaia/commit/3568ea50aa6d67583e2f66f69ce1a04ed139bdfc

Comment 22

[Mahendra Potharaju [:mahe]]

(In reply to David Flanagan [:djf] from comment #20)
> Comment on attachment 8634902 [details] [review]
> [gaia] punamdahiya:Bug1178290 > mozilla-b2g:master
> 
> Mahe,
> 
> Are you handling 2.5 uplift requests? 
> 
David, Yes. Am handling the 2.5 uplift requests. Thanks for the detailed notes on this. Will ensure it lands on 2.5 branch. 

Thanks

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to revert this in https://github.com/mozilla-b2g/gaia/commit/06de78d2c61c084956640c480280ba518b2fe29f for various Gu failures in pick_test.js:

https://treeherder.mozilla.org/logviewer.html#?job_id=3236570&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=3236359&repo=b2g-inbound
https://treeherder.mozilla.org/logviewer.html#?job_id=3236557&repo=b2g-inbound

Comment 24

[Punam Dahiya [:pdahiya]]

Thanks Wes, it's my bad. I will fix the test and submit patch again. Reopening for fixing failing tests.

Comment 25

[Mahendra Potharaju [:mahe]]

Wes, once the tests are fixed, please uplift it to 2.5. 

Thanks

Comment 26

[GitHub Autolander Bot]

Attachment: [gaia] punamdahiya:Bug-1178290 > mozilla-b2g:master

Comment 27

[Punam Dahiya [:pdahiya]]

(In reply to Autolander from comment #26)
> Created attachment 8682570 [details] [review]
> [gaia] punamdahiya:Bug-1178290 > mozilla-b2g:master


https://treeherder.mozilla.org/#/jobs?repo=gaia&revision=fb456fbb07425749445610af030a268a6b705e78

Comment 28

[Punam Dahiya [:pdahiya]]

Comment on attachment 8682570 [details] [review]
[gaia] punamdahiya:Bug-1178290 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): not a regression. Our old code just wasn't written with 20mp photos in mind, and this patch brings it up to date.

[User impact] if declined: photos from the foxfooding device will be unnecessarily downsized when the user tries to use them in other apps or upload them.

[Testing completed]: Punam has tested

[Risk to taking this patch] (and alternatives if risky): not risky

[String changes made]: none

Comment 29

[Punam Dahiya [:pdahiya]]

Moved 2.5 approval flag on the updated patch and landed on master.

https://github.com/mozilla-b2g/gaia/commit/5d7111fa51caf65e7d5ce6c6c9100c1ba12d2e5a

Comment 30

[Mahendra Potharaju [:mahe]]

Comment on attachment 8682570 [details] [review]
[gaia] punamdahiya:Bug-1178290 > mozilla-b2g:master

Thanks for fixing the tests. 

Approved for 2.5

Comment 31

[Carsten Book [:Tomcat]]

on 2.5 -> https://hg.mozilla.org/integration/gaia-2_5/rev/bd0e69771054

Comment 32

[Hubert Figuiere [:hub]]

It is all good now.

(metadata stripping on a vertical picture is a different story, we have a bug for that one)