989290 - [Tarako][dolphin] The Image rotated 90 degree after select a captured portrait image from Camera

Status: VERIFIED FIXED

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

Description

[Marvin Khoo [:Marvin_Khoo]]

beside "Edited" or select from Gallery image picker (thumbnails) here is another failed scenario to be considered.

STR:
1. Open contact apps and select “+” to add new contact
2. Select pick image from Camera, and capture a Portrait image
3. A Portrait preview is shown
4. Press select
5. Noticed that the Contact’s avatar image is rotated 90 degree.

Observed:
Once the image is captured and preview (step 3), which is correct but after selected it, the Contact's avatar image shown 90 degree rotated.

Expected Result:
Should be remain portrait.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

I'm guessing the Contact's app isn't respecting the rotational metadata in the image's EXIF header.

Comment 2

[David Flanagan [:djf]]

Yes, this is because Tarako does not rotate images itself and relies on EXIF orientation, which is not supported by the web platform.

To fix it, we need to do one of the things listed here: https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c52

This seems like as good a bug as any to make the fix in.

Comment 3

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

ni? Steven

It seems the best we can do at this moment is https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c55
if we can get Bug 989026 fixed and disable the "camera" option when selecting pictures from Contacts/Message app, can this be accepted? thanks

Comment 4

[David Flanagan [:djf]]

Here's the list of the 5 options I see for addressing this, copied from https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c52


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.

#1 and #5 aren't realistic given the time constraints. #4 is almost guaranteed to run into OOM problems.  #3 seems like it might be possible, but I'm still trying to figure out emscripten. Some help with that would be great.  #2 seems like by far the easiest solution.

I haven't heard any argument for why spreadtrum can't restore whatever rotation solution they had in place. They removed it to try to save memory and lots of things broke. Why can't they just put it back?

More about option 4: decoding a 2mp image requires 8mb of memory. Rotating it requires two decoded copies (the original and the copy) to be in memory at once.  So doing a naive lossy rotation in the camera and/or gallery apps requires 16mb of memory. If we do this during pick activities, the system is already required to keep two apps alive, so that is the hardest possible time to allocate 16mb of memory.

Needinfo jcheng because we've moved this debate from bug 798684 where it doesn't belong to here.

Needinfo fabrice: you never responded to my needinfo request on bug 798684.  This issue, and the approach we use to solve it, will affect many parts of Tarako. We need a decision made here, and as far as I know, you're the one to make it.

Comment 5

[David Flanagan [:djf]]

Here's another approach we could take: wrap a simple web API around the libjpeg library that is already inside Gecko to enable direct blob-to-blob conversions of JPEG files, with a focus on the sorts of things that the jpegtran utility supports: lossless image rotation, lossless image cropping (to the nearest 8px) and maybe even downsampled decoding support if we don't get that alredy from bug 854795.  The new API would be preffed-off by default and would probably start off only being available to certified apps.  I'd be happy to take a crack at designing such an API, though I'm not qualified to implement it.

Jonas: I think I remember you advocating this sort of blob-to-blob JPEG transformation at some point. It is getting really late for 1.3T, but I think we might need something like this.

Comment 6

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

i left comment in https://bugzilla.mozilla.org/show_bug.cgi?id=798684#c62, i can move my response to this bug next time

Comment 8

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

for tarako, let's track Bug 989026

Comment 9

[[:fabrice] Fabrice Desré]

Clearing ni according to comment 8

Comment 10

[David Flanagan [:djf]]

(In reply to Joe Cheng [:jcheng] from comment #8)
> for tarako, let's track Bug 989026

Joe: I feel like I keep saying over and over that bug 989026 is only going to fix half the problem. That bug only addresses the case where the user crops the image.

Fabrice: this feels like a serious, serious issue, and I can't get anyone's attention on it. I feel that we need to ask Spreadtrum to turn rotation back on in their firmware or create a gecko API that exposes libjpeg to JS.  The web is not ready for EXIF orientation. And the Tarako does not have enough memory for us to decode and rotate images.  Please review comments 4 and 5 and share your thoughts.

Comment 11

[Hema Koka [:hema]]

Fabrice, please help move this forward!

hema

Comment 12

[[:fabrice] Fabrice Desré]

Sorry for taking so long to look at that. It seems to me that we have the following options:

- Short term, either use an emscripten'ed jpegtrans, or revert spreadtrum change and deal with the memory usage regression.

- Longer term, work on a gecko-side api.

I'm adding needinfo on azakai to get some help/guidance on the emscripten option.

Comment 13

[Alon Zakai (:azakai)]

If jpegtran can do lossless rotation, that is without decoding and re-encoding the image (which I am not sure of, not familiar with the library - but it sounds like other people here are), then it might be an option to compile it. But seeing numbers like 8MB here make me skeptical. The only way is to build it and see, but it would take a few MB just for the compiled jpegtran source code + JIT overhead, plus a typed array big enough to contain both the original and the rotated image. That typed array will need to exist while the input file exists, so there will be 3 copies in memory at the same time. I'm not sure how big the jpeg image files (not decoded) are, but if 3x that + a few MB for the code + JIT overhead is too much, then this is unlikely to work. Note that there will also be more overhead for how much internal data structures jpegtran has as it does the rotation.

Comment 14

[Alon Zakai (:azakai)]

Attachment: a.out.js

I did a very quick compile of libjpeg and jpegtran. The optimized output is 0.53 MB of JS. Note that this includes all of libjpeg, not just the rotating capability, but I don't know if that would make much of a difference.

Attaching this here, it might be interesting if someone could write a tiny html file and includes this .js, to see how much memory that takes on the device (about:memory can show that; ignore the typed array part which is fixed size).

Comment 15

[Alon Zakai (:azakai)]

(I mean, how much memory to load a page containing it, which means compiling it - this is not functional yet, cannot measure actual rotation of an image.)

Comment 16

[Punam Dahiya [:pdahiya]]

(In reply to Alon Zakai (:azakai) from comment #14)
> Created attachment 8403618 [details]
> a.out.js
> 
> I did a very quick compile of libjpeg and jpegtran. The optimized output is
> 0.53 MB of JS. Note that this includes all of libjpeg, not just the rotating
> capability, but I don't know if that would make much of a difference.
> 
> Attaching this here, it might be interesting if someone could write a tiny
> html file and includes this .js, to see how much memory that takes on the
> device (about:memory can show that; ignore the typed array part which is
> fixed size).

Thanks Alan, i will try loading the attached js with one of our test-apps and update bug with memory logs.

Comment 17

[Punam Dahiya [:pdahiya]]

Attachment: Attaching memory log with libjpeg loaded with ds test-app

Hi Alon

I loaded the js in attachment 8403618 [details] inside index.html of device storage test-app and got memory log from hamachi device with BuildId 20140401160201
OS version 1.5

To see memory logs, unzip attached folder and open below link in firefox
about:memory?file=~/memory-log/about-memory-with-libjpeg/memory-reports
about:memory?file=~/memory-log/about-memory-without-libjpeg/memory-reports


Memory usage for device storage test app  without a.out.js seen : 9.23MB
Memory usage for device storage test app  with a.out.js seen : 26.71MB

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Punam Dahiya from comment #17)

> Memory usage for device storage test app  without a.out.js seen : 9.23MB
> Memory usage for device storage test app  with a.out.js seen : 26.71MB

ouch. 17MB will be hard to take on a tarako...

Comment 19

[Alon Zakai (:azakai)]

It looks like 16MB of those are the asm.js typed array heap. That must be a power of 2, but if jpegtrans can use less, it could be reduced. So, at least there is not any surprising general overhead here, that 16MB is expected (16MB is the default heap size).

The main question left then is, is storing a multiple of the input file in memory at once ok. We need the input and output files to be in the asm.js heap, as well as a copy of them outside, so 4x that of the input file basically. How large are the input files? Is 4x that size too big?

Comment 20

[Punam Dahiya [:pdahiya]]

(In reply to Alon Zakai (:azakai) from comment #19)
> It looks like 16MB of those are the asm.js typed array heap. That must be a
> power of 2, but if jpegtrans can use less, it could be reduced. So, at least
> there is not any surprising general overhead here, that 16MB is expected
> (16MB is the default heap size).
> 
> The main question left then is, is storing a multiple of the input file in
> memory at once ok. We need the input and output files to be in the asm.js
> heap, as well as a copy of them outside, so 4x that of the input file
> basically. How large are the input files? Is 4x that size too big?

from comment#4 - input files to edit can be max 2mp. Setting needinfo flag for djf to validate.

Comment 21

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

(In reply to David Flanagan [:djf] from comment #10)
> (In reply to Joe Cheng [:jcheng] from comment #8)
> > for tarako, let's track Bug 989026
> 
> Joe: I feel like I keep saying over and over that bug 989026 is only going
> to fix half the problem. That bug only addresses the case where the user
> crops the image.

hi David, 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. thanks

Comment 22

[David Flanagan [:djf]]

Alon/Punam: 5mp images on the nexus 4 seem to range from 500 to 1000kb in filesize. So extrapolating down linearly, and assuming that Tarako uses the same JPEG compression values, we'd have files between 200K and 400Kb

Joe: our partner may have agreed that rotating only cropped images is okay, but I just can't see how that will be an acceptable experience for our users, who are the ones that count.  If the user pick an image via an <input type="file"> element in a web page, they won't even seen an option to crop.  And if the user takes a perfectly composed picture that does not need cropping, it also will not be rotated. When a user is adding a photo to the Contacts app, do we really want to tell them that they must crop the photo in order for it to come out in the correct orientation?

Comment 23

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

hi David, i agree with you if we have more time than we have for tarako.
but if we don't even land bug 989026, isn't the user experience even worse? the pictures can always be rotated incorrectly if chosen from contacts/message app. if we land bug 989026, at least the user is possibly able to have a picture with correct orientation

Comment 25

[Jason Smith [:jsmith]]

(In reply to Joe Cheng [:jcheng] from comment #23)
> hi David, i agree with you if we have more time than we have for tarako.
> but if we don't even land bug 989026, isn't the user experience even worse?
> the pictures can always be rotated incorrectly if chosen from
> contacts/message app. if we land bug 989026, at least the user is possibly
> able to have a picture with correct orientation

Shipping software requires that you meet your internal team's quality standards to ship it. As the smoketest report called out today, this is a smoketest blocker for very obvious reasons - the basic workflow of selecting an image from a gallery is returning a wrong orientation of the image, which is basic bustage. Being "possibly" right isn't good enough - we can't assume that users will always crop their images when adding it to a contact. Explicitly lowering the bar when working partners is just going to result in shipping a poor product overall.

Comment 26

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

David, will 

image-orientation: from-image

do the trick here. It will be help if the image is being put in <img>s.

https://developer.mozilla.org/en-US/docs/Web/CSS/image-orientation#Values


(I have just find out this today)

Comment 27

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

Clarify, the previous comment was meant to be a question:

Will |image-orientation: from-image| do the trick here?

Comment 28

[Ben Kelly [:bkelly, not reviewing]]

Contacts recently switched to use CSS background-image.  Looks like it landed in v1.3t:

  https://github.com/mozilla-b2g/gaia/blob/v1.3t/apps/communications/contacts/js/utilities/image_loader.js#L96

This was done as part of bug 967277 to reduce reflows while scrolling.

So I don't think we can use |image-orientation: from-image| directly as that only works on <img> elements.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

+Timothy here as another bug being worked is affected by how we decide to go with background-image vs <img> element in contacts app.

Comment 30

[David Flanagan [:djf]]

Taking this to see if I can move it forward at all.  

Here are the options I suggested in comment 4, but updated thoughts since then.


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

I know we have CSS support with image-orientation:from-image. But that only works for <img> elements, not CSS background-image, and it doesn't help with off-screen images that we use for resizing. I think my original "fix gecko" suggestion meant that images would be decoded with the correct orientation, not just rotated in CSS when displayed, but their actual width and height would respect the EXIF orientation flag. Obviously that is a lot of work, and would probably break the web. So not realistic here.

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

I've since learned that the 2 or 3Mb required to rotate the image in the camera driver are kernel memory and this memory would be permanently allocated and never available for apps.  So that makes this option painful. I still think it might be the least bad choice for 1.3T, however.

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

Alon's investigations in this bug make it sound like this would not actually save us any memory over option 4. It would get us lossless rotation instead of lossy rotation.

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

This is the approach I will attempt now. I think it is going to cause OOM issues (particularly when using the file upload element from the browser) and will not be a suitable solution. But we won't know unless we try, so I'll try.

For a 2mp image, I think it will actually require 16mb, not 8mb, because I'll require two copies of the decoded image.

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

If this was just a matter of using image-orientation:from-image in those apps, that would be simple. But as Ben points out above, Contacts uses background-image. And in any case, both Contacts and SMS resize images, and doing this right requires an EXIF parser.  Even if we could leave the orientation issue to the individual apps it does not scale because any app that uses a pick activity or a file upload element will need to deal with the possibility of rotated images.  We don't want a marketplace full of apps that don't work right on Tarako.

The only other option I can think of is to expose Gecko's copy of libjepg to JavaScript so that I can do a lossless image rotation without using emscripten. Unless someone wants to work really fast on that, though, I don't see us having time to do that, either.

Comment 31

[David Flanagan [:djf]]

Of course, it doesn't help that my Tarako is unrooted and I apparently have no way to flash it. So I can create a patch but I can't test it.

Comment 32

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

David, if you make a patch, I can test it.  Also if you send the device over, I will root it and send it back with a more recent image.

Comment 33

[David Flanagan [:djf]]

Attachment: pull request on github

Naoki: I've set feedback? for you since you volunteered to test this patch.

This patches camera so that when it is invoked for a pick activity it rotates images before returning them. This should solve the pick from camera issues for contacts and sms, unless it causes OOM problems.  (It may produce upside down images, though: I may be rotating in the wrong direction)

This is not ready to land. I still need to fix the issue in gallery as well.

Comment 34

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Comment on attachment 8407204 [details] [review]
pull request on github

1) rebooted phone
2) select contact
3) select add contact
4) select picture
5) select camera

actual: the contact app and camera app dies

04-15 17:38:11.330: I/Gecko(84): NeckoParent::AllocPRemoteOpenFile: FATAL error: app without webapps-manage permission is requesting file '/data/local/webapps/camera.gaiamobile.org/application.zip' but is only allowed to open its own application.zip at /data/local/webapps/communications.gaiamobile.org/application.zip: KILLING CHILD PROCESS
04-15 17:38:11.330: I/Gecko(84): ###!!! [Parent][DispatchAsyncMessage] Error: Value error: message was deserialized, but contained an illegal value
04-15 17:38:11.330: I/Gecko(84): [Parent 84] WARNING: waitpid failed pid:456 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254

Comment 35

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat.txt

logcat of patch

Comment 36

[David Flanagan [:djf]]

Thanks Naoki. That's a very weird error message.  I've got no clue what is going on there.  I've got a Linux laptop on the way, so hopefully I'll be able to get my own Tarako working and play with this myself.

Comment 37

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

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #34)
> actual: the contact app and camera app dies
> 
> 04-15 17:38:11.330: I/Gecko(84): NeckoParent::AllocPRemoteOpenFile: FATAL
> error: app without webapps-manage permission is requesting file
> '/data/local/webapps/camera.gaiamobile.org/application.zip' but is only
> allowed to open its own application.zip at
> /data/local/webapps/communications.gaiamobile.org/application.zip: KILLING
> CHILD PROCESS
> 04-15 17:38:11.330: I/Gecko(84): ###!!! [Parent][DispatchAsyncMessage]
> Error: Value error: message was deserialized, but contained an illegal value
> 04-15 17:38:11.330: I/Gecko(84): [Parent 84] WARNING: waitpid failed pid:456
> errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc,
> line 254

This is indeed very weird so we cannot rule out whether or not djf's patch will make picker OOM on tarako. Brian, can we test this locally in Taipei?

Comment 38

[bhavana bajaj [:bajaj]]

NI :fabrice as discussed in triage as he may have input here.

Comment 39

[[:fabrice] Fabrice Desré]

Comment #34 is not an OOM issue, but I can't reproduce with the steps there without David's patch. At first glance I don't see anything in David's patch that could explain this error either.

Comment 40

[Brian Huang [:brianhuang][:brhuang]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #37)
> (In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from
> comment #34)
> > actual: the contact app and camera app dies
> > 
> > 04-15 17:38:11.330: I/Gecko(84): NeckoParent::AllocPRemoteOpenFile: FATAL
> > error: app without webapps-manage permission is requesting file
> > '/data/local/webapps/camera.gaiamobile.org/application.zip' but is only
> > allowed to open its own application.zip at
> > /data/local/webapps/communications.gaiamobile.org/application.zip: KILLING
> > CHILD PROCESS
> > 04-15 17:38:11.330: I/Gecko(84): ###!!! [Parent][DispatchAsyncMessage]
> > Error: Value error: message was deserialized, but contained an illegal value
> > 04-15 17:38:11.330: I/Gecko(84): [Parent 84] WARNING: waitpid failed pid:456
> > errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc,
> > line 254
> 
> This is indeed very weird so we cannot rule out whether or not djf's patch
> will make picker OOM on tarako. Brian, can we test this locally in Taipei?

Sure, We can try it.

Al, Pls help on this check!! Thanks.

Comment 41

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

FYI, the error message in comment 34 is filed as bug 997641. John reproduce it without Gaia changes.

Comment 42

[Al Tsai [:atsai]]

I tested w/ the newest base image and :djf's patch. I cannot launch camera and contact apps at the same time.

» adb shell b2g-info                                                                                                                                                                          atsai@Al-MBA
                         |     megabytes     |
           NAME PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER   
            b2g  83    1   23.7    0 25.4 27.8 34.3 138.6       0 root   
         (Nuwa) 346   83    1.2    0  0.3  1.5  5.6  49.4       0 root   
     Homescreen 364  346    4.8   18  6.5  9.1 16.8  66.6       8 app_364
 Communications 485  346    3.9    1 12.2 15.6 24.3  70.1       2 app_485
(Preallocated a 529  346    0.7   18  4.9  7.5 14.7  58.4       1 root   

System memory info:

            Total 98.0 MB
     Used - cache 77.0 MB
  B2G procs (PSS) 61.4 MB
    Non-B2G procs 15.6 MB
     Free + cache 20.9 MB
             Free  1.9 MB
            Cache 19.0 MB

Low-memory killer parameters:

  notify_trigger 14336 KB

  oom_adj min_free
        0  4096 KB
        1  5120 KB
        2  6144 KB
        6  7168 KB
        8  8192 KB
       10 20480 KB

Comment 43

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

We need to ensure 2mp limit is set and comment 34 does not happen again before retest this.

Comment 44

[David Flanagan [:djf]]

I've updated the patch in the pull request.  

This patch fixes these things:

1) When you pick an image from gallery, it will be rotated before being returned at the same time that the crop (if any) is being done. So apps that use the pick activity will not need to care about EXIF orientation. This should fix the sideways image problem for Contacts and SMS when picking from gallery.

2) When you share a single image from gallery, it does the same rotation.  (But does not when you share multiple images).

3) It displays a spinner while doing the rotation because on Tarako the rotation takes 3-4 seconds.

4) When you edit an image in gallery, it first rotates the image, if necessary to remove the EXIF orientation. It displays a spinner while doing this.  Then it lets you edit the image, which is now right-side up.

5) When you pick an image with the camera, the camera reduces its photo size to 1280x960. (Because I was having trouble with larger sizes.)  If the picture taken has EXIF orientation, it rotates the image and saves it back to the sdcard, displaying a spinner while doing this. Then, the blob returned by the activity is a file-backed blob, and has already been rotated, so Contacts and SMS won't get sideways images.

6) As a bonus, this patch fixes the bug where if you were picking wallpaper from camera and took a landscape mode photo you'd get something completely distorted. Camera now handles the pick width and height arguments correctly.  I haven't looked to see if there is already a bug filed for this. 

These fixes work for me on the 3/31 build that came with the latest Tarako base image. I have not tried to update gecko to anything newer.  There are still lots of other bugs going on, and I still see crashes, but I no longer think they are related to image memory.

I think that this patch will resolve bug 989026.

I'm not completely ready for review yet. I want to check over my code, and I'd like to write tests for the new cropResizeRotate() function that is the central part of this patch.  And the code that reduces the size of the photo for a pick from Camera needs to be made build-time configurable.  We also need to test picks from the Browser (with file upload) and may need to reduce the image size in Gallery as well.  (Though that could be a separate bug, I suppose).

So, even though this is not quite done, it is ready for testing and feedback.

Comment 45

[David Flanagan [:djf]]

Comment on attachment 8407204 [details] [review]
pull request on github

Tim: setting feedback? for you since you've been invovled in this. I don't know if you want to look at this code yourself, but maybe you can find someone to read it through. Is John available, for example?

Punam: I think this patch resolves bug 989026, and I'm setting feedback? on you since you were working on that.

Comment 46

[David Flanagan [:djf]]

Setting needinfo for Naoki since he requested notification when there was something ready to test.

Naoki: note that this works for me with an older build. If you're still seeing what you saw in comment 34, maybe you can try 20140331 build which is what I seem to be using.

Comment 47

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat.txt

Based on the bug that was filed for the issue that I was having, it was because I was flashing to the data partition for the gaia.  I had followed : https://intranet.mozilla.org/B2G_Team/Tarako#Black_screen_after_a_make_install-gaia.3F

This time instead I did ROCKETBAR=0 MOZILLA_OFFICIAL=1 NOFTU=1 make production

The main test case (ie comment 0) seems to be placed in the correct view. ( I tried taking the picture in landscape as well as portrait. )  There is no crop feature after taking the picture from the camera.

Also to note, consecutive taking pictures may cause the OOM to occur when switching to the camera, in which case you could end up with a black screen and nothing happening. [ the attached logcat is this case, where the screen goes black ]  The performance seems to degrade.

When trying to select from gallery I am OOMing.  Once I go to the gallery and then select a picture, it will take me to a crop editor.  If I crop, I end up Getting LMK killed.  When I didn't crop and just clicked done, I got a black screen. See the following:

04-18 18:22:22.772: D/Sensors(84): activate handle=0; drv=0
04-18 18:22:22.772: D/Sensors(84): Lis3dhSensor: Control set 0
04-18 18:22:22.772: D/Sensors(84): Lis3dhSensor: mEnabled = 0
04-18 18:22:22.782: I/Gecko(84): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv
04-18 18:22:22.802: E/GeckoConsole(84): [JavaScript Error: "NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIMessageSender.sendAsyncMessage]" {file: "resource://gre/modules/ActivitiesService.jsm" line: 15}]
04-18 18:22:22.812: I/GonkMemoryPressure(84): Checking to see if memory pressure is over.
04-18 18:22:22.812: I/GonkMemoryPressure(84): Memory pressure is over.
04-18 18:22:23.002: I/Gecko(84): [Parent 84] WARNING: waitpid failed pid:1011 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
04-18 18:22:23.382: I/GonkMemoryPressure(3352): Checking to see if memory pressure is over.
04-18 18:22:23.382: I/GonkMemoryPressure(3352): Memory pressure is over.
04-18 18:22:23.792: I/Gonk(84): Setting nice for pid 3352 to 18
04-18 18:22:23.792: I/Gonk(84): Changed nice for pid 3352 from 1 to 18.
04-18 18:22:25.002: I/Gecko(84): [Parent 84] WARNING: waitpid failed pid:1011 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
04-18 18:22:25.002: I/Gecko(84): [Parent 84] WARNING: Failed to deliver SIGKILL to 1011!(3).: file ../../../gecko/ipc/chromium/src/chrome/common/process_watcher_posix_sigchld.cc, line 118
04-18 18:22:26.772: E/GeckoConsole(3352): Content JS ERROR at app://homescreen.gaiamobile.org/gaia_build_defer_index.js:386 in loadSVConfFileError: Failed parsing singleVariant configuration file [js/singlevariantconf.json]:  [Exception... "File error: Not found"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: app://homescreen.gaiamobile.org/gaia_build_defer_index.js :: loadFile :: line 378"  data: no]

Comment 48

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I should add that I did see the spinner in the gallery.  That was a nice touch.

Comment 49

[David Flanagan [:djf]]

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #47)

> 
> The main test case (ie comment 0) seems to be placed in the correct view. (
> I tried taking the picture in landscape as well as portrait. )  There is no
> crop feature after taking the picture from the camera.

\o/.  No crop is expected in this case.
 
> Also to note, consecutive taking pictures may cause the OOM to occur when
> switching to the camera, in which case you could end up with a black screen
> and nothing happening. [ the attached logcat is this case, where the screen
> goes black ]  The performance seems to degrade.

I don't understand this part. Perhaps worth filing as a separate bug, with detailed STR.

> When trying to select from gallery I am OOMing.  Once I go to the gallery
> and then select a picture, it will take me to a crop editor.  If I crop, I
> end up Getting LMK killed.

Darn. This was working for me, at least some of the time. Though the build I was using had enough instabilities that I was often rebooting it. I don't know if I ever tested camera and then tested gallery right after that.

In any case, this may mean that I need to reduce the size of the image before rotating/cropping it.

> When I didn't crop and just clicked done, I got
> a black screen. See the following:

That sounds weird. I can't tell from the logcat excerpt whether it was an OOM or something else.  I know I had this case working at some point as well, though I supposed I could have broken it before I updated the patch.

Comment 50

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

Comment on attachment 8407204 [details] [review]
pull request on github

I've only read crop_resize_rotate.js and I think you have use of all the Gecko tricks available to us.

Look at the code probably will not help us in term of making sure this really works; we need

-- a unit test suite with good coverage that make sure every possible input results the correct output
-- (optional) a test app to manually run the library on with the cases above, allowing us to measure the memory peaks of each.

Comment 51

[James Zhang (Spreadtrum)]

Can we land this patch before 4.25 -- spreadtrum PTR2 test cycle.

Comment 52

[David Flanagan [:djf]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #50)
> Comment on attachment 8407204 [details] [review]
> WIP pull request
> 
> I've only read crop_resize_rotate.js and I think you have use of all the
> Gecko tricks available to us.
> 
> Look at the code probably will not help us in term of making sure this
> really works; we need
> 
> -- a unit test suite with good coverage that make sure every possible input
> results the correct output

Agreed. I've been thinking about how to do this, and I think I have a way.

> -- (optional) a test app to manually run the library on with the cases
> above, allowing us to measure the memory peaks of each.

That's an interesting idea. I'm not sure I'll have the time for that. How do we measure memory peaks? Is that what about:memory does?

(In reply to James Zhang from comment #51)
> Can we land this patch before 4.25 -- spreadtrum PTR2 test cycle.

James: This (and related) bugs is my top priority for the week, and will try to get it landed in time. Naoki is reporing in comment 47 that it isn't working for Gallery, and Tim is requesting substantial tests in comment 50. Given those uncertainties, I can't make a firm commitment to the 4/25 date, however.

Comment 53

[Punam Dahiya [:pdahiya]]

Here are my initial findings on testing the attached patch on below environment
Buildid: 20140325060053
Device:Tarako
OsVersion: 1.3.0.0

Tested the attached patch for below cases
1. Attach image using camera to contacts/sms app
Successfully able to attach rotated image to contacts and messaging app.

2. Attach image picked from gallery to contacts/sms app
Successfully able to attach rotated image to contacts and messaging app.

3. Edit Image inside gallery app. Image shows rotated inside editor and 
successfully saves edited image.

However in all the above cases contacts/gallery/messaging app crashes on 
exiting respective app by pressing home key.  To observe the crash attach the image from gallery /camera in contacts /sms and than exit by pressing home key.

For gallery, edit and save the image. Immediately exit gallery app by pressing home key, it crashes gallery app. 
I tested on below build and crashes could be because of 20140325060053 build. 

Will update bug once i have tarako device flashed with latest build.

Comment 54

[David Flanagan [:djf]]

I have updated my Tarako to last night's build.  Picking from camera seems to work for me.  Picking from Gallery sometimes works and sometimes OOMs.  To be safe, I think I should add a build-time config option that allows us to reduce the size of the image when cropping or rotating it.

Relatedly, we may need to add code to gallery to prevent cropping if the image is too big to crop. For jpegs, we can use #-moz-samplefactor to resize before cropping.  For non-jpeg images, we won't have to rotate, but we may want to restrict the size for which we allow cropping. Displaying an image takes 4 bytes per pixel, but cropping can take 8 bytes per pixel, so if the maximum image size we're willing to display is 2mp, then maybe the maximum size we should be willing to crop is half that.

Comment 55

[David Flanagan [:djf]]

I've updated my patch with some bug fixes and build time changes so that (when properly configured) the camera pick activity will return 640x480 images and the gallery pick activity will return 800x600 (or smaller if there is cropping).  Everything is properly rotated, and because we're dealing with smaller images, there is a lot less memory used.  It seems a lot stabler to me.

To test this, add the environment variable GAIA_MEMORY_PROFILE=low to the make command.  What I do is:

   GAIA_MEMORY_PROFILE=low make profile   # rebuild the config.js files
   APP=camera make install-gaia           # push the camera app
   APP=gallery make install-gaia          # push the gallery app

But that is just because I haven't learned yet what variables to set to install all of gaia without filling up the partition.

Setting the low memory profile will also change the default maximum photo and maximum image size to 2mp instead of 5mp.

I still need to write tests, but I think this is closer to something that we can land.

Naoki: are you available to test this again?  I'm interested in all possible image picks: using sms, contacts, email, homescreen (for wallpaper) and settings (for wallpaper) to pick images from both gallery and camera. In camera, taking pictures at different device orientations and in gallery picking images with no crop, a small crop and a large crop.  If not you, Naoki, can we get other QA resources assigned to this?

Comment 56

[Punam Dahiya [:pdahiya]]

Tested the updated attached patch with  build 20140422014001 and the use cases in comment #53 works well.

However still sees app crashing on exiting sms/contacts/gallery by pressing home button, 
though i have seen similar experience intermittent for other apps email, clock as well.

Noticed that camera pick activity saves the portrait picked image, rotated with resolution 600 x 800
and gallery pick activity is saving it at 480x640 without cropping. Is that expected?

Also, pictures taken in camera app are saved at resolution 1280x960 (portrait) with EXIF orientation saved. Images taken using camera via pick activity gets rotated and by default saved on sdcard (without EXIF data) with resolution 960x1280.

Comment 57

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: logcat.txt

I used : GAIA_MEMORY_PROFILE=low ROCKETBAR=0 MOZILLA_OFFICIAL=1 NOFTU=1 make production

I think the black screen issue is something separate as you suggested.  I only got it to reproduce 2 times out of 30 attempts.  It looks like both apps were LMK'ed and I got stuck in the in between state of apps.

For email attachments, do we care about video versus non video?

1) w/ testing with various apps, the camera app portion seems sluggish and the image looks pixelated after the snap shot.
2) for the gallery selection,  the crop handles seems not as responsive sometimes. 
3) Switching between landscape/portrait for gallery and camera resizes correctly; there seems to be some performance issues resizing when hitting memory pressure.  (I suppose this is expected).
4) a small crop failed to load for me for the homescreen ( see attached logcat ); I think under memory pressure there's a chance that the file might not get created properly?  I can't seem to reproduce this.  It only occurred once during my testing.
5) occasionally I can get a slight split in the picture (ie as if it was scanning part of the image and then I shifted the picture down some)

Other things I ran into : 
1) SMS doesn't automatically turn data on when sending attachments.  You will see a spinner but it won't send.
2) There was a problem when trying to attach a video/photo and send it off email.  The photo attached to the email, as did the video; it's not sending and an error appears "Sending mail failed"; looking at the log it looks like an email/activesync error:
04-23 09:45:35.949: I/Gecko(473): [31mWERR: Error: Error getting OPTIONS URL[0m
04-23 09:45:35.949: I/Gecko(473): [31mWERR: Error connecting to ActiveSync: Error: Error getting OPTIONS URL[0m
04-23 09:45:35.959: I/GeckoDump(473): [32mLOG: compose: callback triggered, err: unknown[0m
3) I had some issues connecting to gmail for whatever reason.

Comment 58

[David Flanagan [:djf]]

Thanks for your feedback Punam and Naoki!  Responses inline below.

(In reply to Punam Dahiya from comment #56)
> Tested the updated attached patch with  build 20140422014001 and the use
> cases in comment #53 works well.
> 
> However still sees app crashing on exiting sms/contacts/gallery by pressing
> home button, 
> though i have seen similar experience intermittent for other apps email,
> clock as well.

I'm guessing that means that the homescreen has already been killed, and restarting it causes other apps to die maybe?

> 
> Noticed that camera pick activity saves the portrait picked image, rotated
> with resolution 600 x 800
> and gallery pick activity is saving it at 480x640 without cropping. Is that
> expected?

That's expected the other way around. The camera should be taking 480x640 photos for pick activities. And the gallery should be resizing (and rotating) 1600x1200 photos to 600x800 when you pick them.
 
> Also, pictures taken in camera app are saved at resolution 1280x960
> (portrait) with EXIF orientation saved. Images taken using camera via pick
> activity gets rotated and by default saved on sdcard (without EXIF data)
> with resolution 960x1280.

That is not expected. When the camera app is used normally, I still expect it to take 1600x1200 photos.  I'll have to investigate that.

(In reply to Naoki Hirata :nhirata (please use needinfo instead of cc) from comment #57)

> For email attachments, do we care about video versus non video?
> 
 
Not for this bug. I haven't touched video.

> 1) w/ testing with various apps, the camera app portion seems sluggish and
> the image looks pixelated after the snap shot.

I'm not sure why it would look pixelated. Could you attach a screenshot of that?  

Sluggish is expected, I guess. I display a spinner while the camera app is rotating the image, so hopefully that helps. Maybe we also need to display a spinner while waiting for the camera to start up initially...

> 2) for the gallery selection,  the crop handles seems not as responsive
> sometimes. 

I don't think this patch affects that, but it is driving me crazy, so maybe I'll try to fix it.  (I'm pretty sure this is a regression that was found and fixed elsewhere.)

> 3) Switching between landscape/portrait for gallery and camera resizes
> correctly; there seems to be some performance issues resizing when hitting
> memory pressure.  (I suppose this is expected).

I'm glad that works. Not related to this bug, however.

> 4) a small crop failed to load for me for the homescreen ( see attached
> logcat ); I think under memory pressure there's a chance that the file might
> not get created properly?  I can't seem to reproduce this.  It only occurred
> once during my testing.

I can't tell anything from the logcat. It occurs to me that one source of variablity here is whether the gallery is scanning or not.  I should probably (perhaps in a separate bug) add code so that in a pick activity, once the user selects an image, we stop scanning for new images.  Or, maybe better, I should make the scanning process more memory efficient with #-moz-samplesize so that scanning doesn't contribute to OOMs.

> 5) occasionally I can get a slight split in the picture (ie as if it was
> scanning part of the image and then I shifted the picture down some)

Do you recall whether that was in the camera or the gallery?  In either case, though, I'm going to chalk that up to a graphics bug and unrelated to this code.  (Unless you were seeing it persistently in the wallpaper or contacts or sms image that was picked.)

Comment 59

[David Flanagan [:djf]]

My hope is to finish this patch today (late tonight most likely) and have it reviewed tomorrow so that it can land by Friday's deadline (per James' request in comment #51).

Still to be done:

1) Modify the Gallery and Camera share code so that they rotate and resize the images in the same way that the pick activity does.

2) Move maxPickImageSize support code directly into the cropResizeRotate() utility function rather than in the gallery pick code so that it can be reused by the gallery and camera share code.

3) Write a massive test for cropResizeRotate

4) Investigate the issues that Punam and Naoki reported above.

Comment 60

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Attachment: IMG_0012.jpg

> Do you recall whether that was in the camera or the gallery?  In either
> case, though, I'm going to chalk that up to a graphics bug and unrelated to
> this code.  (Unless you were seeing it persistently in the wallpaper or
> contacts or sms image that was picked.)

I believe it was the camera and the camera took the photo like this.  attached is the clipping.  The top part lagged a little when snapping a picture.  The file itself doesn't look pixelated... I guess it's the display of the image that renders bad.  I'll see if I can grab a screenshot soon.

Comment 61

[David Flanagan [:djf]]

This is weird.  4 days ago (when I was using a version of gonk that was a couple weeks old) the camera firmware supported image sizes of 1600x1200, and that is what the camera app was using.  I have images of that size on my sdcard.

I notice now that the camera firmware now reports only supporting 1280x960 and 640x480.

Anyone know what happened here? As the module owner for the Camera app, I wish someone had thought to mention this change to me!

If this is changed in the firmware, we are going to have to change any marketing material to say that the phone has a 1 megapixel camera instead of a 2 megapixel camera.  I think that with the patch in this bug we should be able to go back to 1600x1200. (But we'll have to test, of course).

Comment 62

[David Flanagan [:djf]]

Comment on attachment 8407204 [details] [review]
pull request on github

Punam and Russ,

I think this pull request is ready for review now.

It is big, and I'll try to spend some time tomorrow annotating it on github to make it more understandable so it is easier to review.  The main new thing is shared/js/media/crop_resize_rotate.js and its unit tests.  This new function is then used in various places to:

1) downsample and rotate images that are picked from the camera app or shared by the camera app

2) downsample and rotate images that are picked from or shared by the gallery app

3) rotate a preview image before displaying crop handles while doing image picking.

4) rotate an image before editing it so that it displays correctly in the gallery image editor.

This patch also includes changes to the build system (in Makefile and build/application-data.js) so that we get different build-time customization defaults for gallery and camera when we set GAIA_MEMORY_PROFILE=low. This allows those apps to behave differently (downsampling images, e.g.) on Tarako than they do on more capable devices.

Comment 63

[David Flanagan [:djf]]

Comment on attachment 8407204 [details] [review]
pull request on github

Yuren,

This patch needs to land on Friday, and it includes minor changes to the build-time configuration code in Makefile and application-data.js.  I made these changes after discussion with Tim, but would also appreciate your feedback on them.

Comment 64

[David Flanagan [:djf]]

Attachment: default 2.9rem spinner size

Comment 65

[David Flanagan [:djf]]

Attachment: propsed 5rem spinner size

Comment 66

[David Flanagan [:djf]]

Attachment: 5rem spinner in an actual screenshot from Tarako

Comment 67

[David Flanagan [:djf]]

Attachment: default 2.9rem spinner size on Tarako

Comment 68

[Amy Lee [:amylee] UX]

Comment on attachment 8412086 [details]
5rem spinner in an actual screenshot from Tarako

Hi David, 

I'm good with the new larger size of the spinner. Two questions:

1. Is that spinner centered to the screen? I think it should be centered to the area below the header since right now it looks like it sitting too high up.

2. Are you using a png graphic for the spinner? Because if we are increasing the size, I will need to submit a new spinner png file for you so we don't end up with a fuzzy looking spinner, unless somehow it's an SVG you're using. Thanks!

Comment 69

[David Flanagan [:djf]]

(In reply to Amy Lee [:amylee] from comment #68)

> 2. Are you using a png graphic for the spinner? Because if we are increasing
> the size, I will need to submit a new spinner png file for you so we don't
> end up with a fuzzy looking spinner, unless somehow it's an SVG you're
> using. Thanks!

The building block I'm using uses a png, and we can't change the building block on 1.3t at this late date, so I'll just reduce the size to the default 2.9rem.

Comment 70

[Russ Nicoletti [:russn]]

Comment on attachment 8407204 [details] [review]
pull request on github

I've updated the PR with my comments, that David has already answered and/or addressed.

Comment 71

[Punam Dahiya [:pdahiya]]

Comment on attachment 8407204 [details] [review]
pull request on github

Hi David

I have reviewed the patch for camera and gallery flows where we are using cropResizeRotate and it looks good and has my r+. I see the fix of bug 992426 is not uplifted for 1.3T and cropped images attached to messages are getting saved under .gallery/ . 

Thanks
Punam

Comment 72

[David Flanagan [:djf]]

Landed to v1.3t: https://github.com/mozilla-b2g/gaia/commit/293b056683b506ec1b4787d0fbf6033cc30fa953

Comment 73

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Comment on attachment 8407204 [details] [review]
pull request on github

looks good to me, will you land this change to master? we should have this configuration for master.

Comment 74

[John Hammink]

Possibly fixed but ended up running in to this issue instead:

filed https://bugzilla.mozilla.org/show_bug.cgi?id=1001675
Bug 1001675 nor, --, ---, nobody, NEW, [Tarako] selecting pictures from camera (for contacts) causes OOM

Gaia      293b056683b506ec1b4787d0fbf6033cc30fa953
Gecko     https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/d91121fbca90
BuildID   20140425014003
Version   28.1

Comment 75

[Natalya Kot [:nkot]]

rotation part seems to be fixed, i've tested using MMS STR: create new message -> Camera attachment -> take picture, select and send ==> the image displays in correct portrait orientation

Comment 76

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

This bug need a documentation update, and features need to land on master (let's create another bug for that?)

https://developer.mozilla.org/en-US/Firefox_OS/Developing_Firefox_OS/Market_customizations_guide#camera.json_%28Gallery_and_Camera_app_image_sizes%29

Comment 77

[David Flanagan [:djf]]

Agreed that we should open new bugs for landing this stuff on master (and possibly 1.4)

- one bug for the GAIA_MEMORY_PROFILE=low changes to the build system. Maybe Yuren could take this

- one bug for the gallery EXIF rotation fixes. This is the biggest part of the patch and one that we might want to get into 1.4. Maybe Russ could take this

- one bug for EXIF rotation fixes in the new camera code base. This will only matter, though, for devices like Tarako that rely on EXIF rotation.  Maybe Justin.

Comment 78

[Russ Nicoletti [:russn]]

Bug 1002593 created for gallery changes mentioned in comment #77.

Comment 79

[James Burke [:jrburke]]

I just filed bug 1003958 to track getting GAIA_MEMORY_PROFILE=low into master, however, if bug 1002593 will do that instead, just let me know and I can close out the other bug (or feel free to close as a dupe).

Comment 80

[Derek Harris [:DerekH] - Dont need info. Last day 6/5/15]

This is fixed on today's Tarako build


1.3t Environmental Variables:
Device: Tarako 1.3t
BuildID: 20140430014008
Gaia: f9b62bd1135f4edf8317fff1c2947b9d99438d79
Gecko: ba50f734b815
Version: 28.1
Firmware Version: sp6821

Comment 81

[Peipei Cheng (needinfo if you need my action)]

we also need this fix in v1.4 for dolphin. Thanks!

Comment 82

[James Zhang (Spreadtrum)]

(In reply to pcheng from comment #81)
> we also need this fix in v1.4 for dolphin. Thanks!

Thanks Peipei.
Dolphin camera driver is OK, it will rotate image.
But we also need v1.3t patch cherry-pick to v1.4 for image with EXIF data.

Comment 83

[Ivan Tsay (:ITsay)]

(In reply to pcheng from comment #81)
> we also need this fix in v1.4 for dolphin. Thanks!

This is the feature request for v1.4 and the patch is complicated to be uplifted to v1.4. Please ask partner to cherry pick this one to their build if they really need it.

Comment 84

[James Zhang (Spreadtrum)]

(In reply to pcheng from comment #81)
> we also need this fix in v1.4 for dolphin. Thanks!

I think we need another bug, support EXIF data.

Comment 85

[Wayne Chang [:wchang]]

Given comment 82, this bug is not applicable to Dolphin. 
The request for Dolphin to handle non-EXIF images would then be considered a new feature request here.

James, as agreed please feel free to file another bug for this request but please understand that it will likely not get approved for 1.4.

(In reply to James Zhang (Spreadtrum) from comment #84)
> I think we need another bug, support EXIF data.

Comment 86

[James Zhang (Spreadtrum)]

Hi Wayne, 

   1.4 dolphin 256MB need maxGifImageFileSize customization change.

Comment 87

[Wayne Chang [:wchang]]

James,

Since it's customization, can your team handle it? What's your expectation here? Maybe you should file a new bug with your needs.

Comment 88

[jingmei.zhang]

  I have filed a new bug for this issue:

 >1.4 dolphin 256MB need maxGifImageFileSize customization change

  https://bugzilla.mozilla.org/show_bug.cgi?id=1045416

Comment 89

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