972779 - [Tarako] Saving picture after editing cause Gallery stop

Status: RESOLVED FIXED

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

Description

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

STR:
0. Take a picture use camera
1. Launch Gallery
2. View a picture
3. Edit the picture
4. Tap on "Done" after modification
   --> The performance of saving a pic after editing is bad. Sometimes it took over 5s to save pic. Sometimes it may cause Gallery stop.

Comment 1

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

can we confirm on the latest build? (please provide build ID)
and provide a video? 
thanks

Comment 2

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

Attachment: STR

Attached is the video. Build number: 20140217032035

Comment 3

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

triage: 1.3T+ as this is a broken feature on tarako

Comment 4

[bhavana bajaj [:bajaj]]

Hema, can you please help find someone who can investigate this ?

Comment 5

[Sotaro Ikeda [:sotaro]]

(In reply to pcheng from comment #0)
> 3. Edit the picture
> 4. Tap on "Done" after modification
>    --> The performance of saving a pic after editing is bad. Sometimes it
> took over 5s to save pic. Sometimes it may cause Gallery stop.

Gallery stop might be caused by killed by lowmemory killer. Can we have a logcat log and dmesg log?

Comment 6

[Hema Koka [:hema]]

Do we have Tarako devices in MV? If so, can have Punam help investigate.

Please provide info Sotaro asked

Comment 7

[[:fabrice] Fabrice Desré]

(In reply to Hema Koka [:hema] from comment #6)
> Do we have Tarako devices in MV? If so, can have Punam help investigate.
> 
> Please provide info Sotaro asked

I have one that can be used for that.

Comment 8

[Marvin Khoo [:Marvin_Khoo]]

Tarako / Android Gallery provide Crop. Rotate function only. no effect editing features. fyi.

Comment 9

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

Hema, assuming your team will get a tarako device from Fabrice for this. do you mind assigning this bug to someone? thanks

Comment 10

[David Flanagan [:djf]]

What size (in megapixels) are the images you are trying to edit?

Does the Tarako build have a custom camera.json file in GAIA_DISTRIBUTION_DIR to set the maximum image size?  If not, you're allowing up to 5mp images which are probably too big for that device.

Comment 11

[David Flanagan [:djf]]

Also, 5s to save an edited image is actually a pretty normal amount of time.

Comment 12

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

(In reply to David Flanagan [:djf] from comment #10)
> What size (in megapixels) are the images you are trying to edit?
> 
> Does the Tarako build have a custom camera.json file in
> GAIA_DISTRIBUTION_DIR to set the maximum image size?  If not, you're
> allowing up to 5mp images which are probably too big for that device.

The image I used is taken by Tarako. The size is 93KB, resolution is 480*640.

With the latest tarako build 20140310152650, the performance is better. croping/applying filters and saving can be done within 5s.

Comment 13

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

(In reply to pcheng from comment #12)
> (In reply to David Flanagan [:djf] from comment #10)
> > What size (in megapixels) are the images you are trying to edit?
> > 
> > Does the Tarako build have a custom camera.json file in
> > GAIA_DISTRIBUTION_DIR to set the maximum image size?  If not, you're
> > allowing up to 5mp images which are probably too big for that device.
> 
> The image I used is taken by Tarako. The size is 93KB, resolution is 480*640.
> 
> With the latest tarako build 20140310152650, the performance is better.
> croping/applying filters and saving can be done within 5s.

James, it seems like from the latest testing, editing picture can be done now. Do you still observe this on your side? and is this still a blocker for you? Thanks

Comment 14

[James Zhang (Spreadtrum)]

Yes, I reduce the camera resolution to 0.5M for test only, and our customer need 2M camera.
It's still blocker issue if I change resolution to 2M.

Comment 15

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

can you please change it back to 2M so the problem can be seen? 
we should reconfirm the problem once the camera is back to 2M. Thanks

Comment 16

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #15)
> can you please change it back to 2M so the problem can be seen? 
> we should reconfirm the problem once the camera is back to 2M. Thanks

This bug depend on resolution of picture.

Comment 17

[Gregor Wagner [:gwagner]]

So are my assumptions right that the tarako camera produces 2M pics and we are having problems processing 2M pics?

Comment 18

[Punam Dahiya [:pdahiya]]

Tried steps in #comment0  on below environment
Device: Tarako
BuildId:20140310141338
Platform 28.0
Os Version 1.3

Picture taken from camera was of resolution 480x640 and size 103 KB. 
I was able to edit pictures in ~ 1-2 secs

Can you please attach the image taken from Tarako that's making gallery crash? Thanks




Can you pl. attach the images taken from tarako that's making gallery crash

Comment 19

[James Zhang (Spreadtrum)]

Please revert device/sprd and kernel change for 0.5M camera.
Then you can reproduce this issue.
The root cause is gallery image editor should support exif data, then kernel can shrink 3M camera rataion reserved memory.

in device/sprd
git revert 055ef046ffa0c9e399b3b7bb28901776bbfc2daf

commit 055ef046ffa0c9e399b3b7bb28901776bbfc2daf
Author: dafeng.xu <[e-mail]>
Date:   Fri Mar 7 17:20:36 2014 -0500

    Bug #287001 cut the camera runtime memory
    
    [bug number  ]287001
    [root cause  ]the camera resolation is too high
    [changes     ]ajust the camera resolation
    [side effects]none
    [reviewers   ]james.zhang
    
    Change-Id: I007f43534571d8f6e9ed67a49aeb7490af0a2140



in kernel
git revert c72664fa690318ee526d5aa8fa3bcc278c3da480

commit c72664fa690318ee526d5aa8fa3bcc278c3da480
Author: dafeng.xu <[e-mail]>
Date:   Fri Mar 7 17:26:01 2014 -0500

    Bug #287001 cut the camera runtime memory
    
    [bug number  ]287001
    [root cause  ]the camera resolation is too high
    [changes     ]ajust the camera resolation
    [side effects]none
    [reviewers   ]james.zhang
    
    Change-Id: I552a6a34d86a9b322c9a34ad1693fa39b9f92fcf

Comment 20

[Andreas Gal :gal]

ImageEditor.prototype.getFullSizeBlob = function(type, done, progress) {
  const TILE_SIZE = 1024;
  var self = this;

The current tile size is 1024*1024*RGBA=4MB. James, can you try to reduce that to 256 and 512 and see whether that helps? If it does we can make this dependent on the device or the image size of some other indicator that memory is tight.

Gregor, we need a crash dump for this to see where exactly we run out of memory.

Comment 21

[Andreas Gal :gal]

(also, I bet a smaller tile makes saving the image faster since we have to hit zram less, just a guess)

Comment 22

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

ni? Gregor so it shows up in his NI queue

Comment 23

[Andreas Gal :gal]

If the TILE size change isn't enough then we should check whether we can use the WEBGL_lose_context extension to eagerly give up the WebGL context. Just setting it to null only releases the OpenGL context after the next GC. The WebGL context is no longer needed once we are done with the ImageProcessing object so we could do something like .destroy() on it.

http://www.khronos.org/registry/webgl/extensions/WEBGL_lose_context/

Comment 24

[James Zhang (Spreadtrum)]

I think I should change camera resolution back to 2M first.

Comment 25

[James Zhang (Spreadtrum)]

OK, please sync code in device/sprd and kernel, camera resolution is 2M now.

Comment 26

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

Hema, do you think someone from your team can look at this as well? there are several gallery related tarako bugs. Thanks

Comment 27

[Steven Yang [:styang]]

(In reply to James Zhang from comment #24)
> I think I should change camera resolution back to 2M first.

Yes, please. It really makes us confused the real situation. We should use the configurations for end product's SPEC.

Comment 28

[Punam Dahiya [:pdahiya]]

Attachment: Logcat - Editing a 2MB image crash gallery on tarako

Attaching the log cat  for crash seen on tarako device when trying to edit a 2MB image loaded in gallery app via sdcard. Please note 2MB image was loaded from sdcard and not taken from tarako camera.

I am also attaching log cat of successful edit of a picture - 103KB taken from Tarako camera.

Both the log cat are from tarako device with BuildId:20140310141338

Comment 29

[Punam Dahiya [:pdahiya]]

Attachment: Logcat - tarako_103KB_Edit Image_success

Comment 30

[[:fabrice] Fabrice Desré]

There's not much in this logcat but it's likely we just OOM. Punam, can you attach the picture you were trying to edit?

Comment 31

[Punam Dahiya [:pdahiya]]

Attachment: Test Image used in attachment 8389919

Comment 32

[Evelyn Hung]

James, can you test what Andreas said in comment 20?

Comment 33

[James Zhang (Spreadtrum)]

(In reply to Evelyn Hung [:evelyn] from comment #32)
> James, can you test what Andreas said in comment 20?

I'm too busy, assign to yang.zhao.
Yang, please provide this data.

Comment 34

[Hema Koka [:hema]]

Joe Cheng -- Punam is helping with the other EXIF Tarako bug. As discussed over email, can you see if John Hu or Gary can help with this one. 

Thanks!
Hema

Comment 35

[Ting-Yu Chou [:ting] (away)]

Just tested with TILE_SIZE 256 & 512, the saving went much smoother.

Comment 36

[yang.zhao]

Tested with TILE_SIZE 256 & 512,the image could be saved successfully and the saving process went smoother too.

Comment 37

[Andreas Gal :gal]

\o/. Thanks Yang.

Comment 38

[Andreas Gal :gal]

Gregor, can you please patch this for me? It seems 512 is fine and that shouldn't affect 5MP performance too much either. djf can review this.

Comment 39

[Gregor Wagner [:gwagner]]

Attachment: link

Comment 40

[Gregor Wagner [:gwagner]]

Comment on attachment 8391417 [details] [review]
link

I don't know this part of the code but I guess this is what you asked for.

Comment 41

[David Flanagan [:djf]]

Comment on attachment 8391417 [details] [review]
link

I love reviewing one line patches!

This looks great. I recall going back and forth on the best tile size.  I suppose it must have been slightly quicker with the bigger tile. But 512x512 seems totally reasonable, and as noted, will result in smoother updating of the progress bar, too.

Comment 42

[Gregor Wagner [:gwagner]]

https://github.com/mozilla-b2g/gaia/commit/f3c4a2fc0cfbeccc9c595f06b436707fa2e6ca0a

Comment 43

[Gregor Wagner [:gwagner]]

Comment on attachment 8391417 [details] [review]
link

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 44

[Michael Henretty [:mikehenrty][:mhenretty]]

v1.3: https://github.com/mozilla-b2g/gaia/commit/0151b9a66d01023bd27524686873af678b6aa1a6

Comment 45

[yang.zhao]

(In reply to Andreas Gal :gal from comment #37)
> \o/. Thanks Yang.

hi,Andreas
    Now it's ok to save pictures after editing in gallery,but it's not ok if you pick an image and edit it from contacts.Is this the same issue to this one?

Comment 46

[Andreas Gal :gal]

Yang, lets file a new bug since this one is done here. Can you please attach steps to reproduce the bug? Thanks!

Comment 47

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

Verified fixed in todays build

1.3t Environmental Variables:
Device: Tarako 1.3t
BuildID: 20140428014001
Gaia: 8895b180ed636069473703d0e7b73086989601ce
Gecko: 7caf4b5abfce
Version: 28.1
Firmware Version: sp6821