Environment: ------------------------------------------------------- Gaia bbba09c732c35d9434ecb04d5e2e41d05511f4d9 Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/65b4c58a7da9 BuildID 20140410164004 Version 28.1 ro.build.version.incremental=eng.cltbld.20140410.210129 ro.build.date=Thu Apr 10 21:01:38 EDT 2014 Steps to reproduce: ------------------------------------------------------- 1. Launch SMS 2. Create a new message 3. Tap on the clip icon on the top right corner 4. Choose gallery 5. Select a picture with large pixel value 6. Tap on "Done" on the top right corner Actual results: ------------------------------------------------------- Wait for a few seconds, a white screen appears. The white screen appears for about 2 seconds, then SMS app exits, and it goes back to home screen.

triage: 1.3T+ broken feature

08-24 04:07:29.523 I/log ( 1690): <4>0[ 1412.754825] lowmem_shrink select 1620 (Messages), adj 2, size 12317, to kill 08-24 04:07:30.403 I/Gecko ( 83): 08-24 04:07:30.403 I/Gecko ( 83): ###!!! [Parent][MessageChannel] Error: Channel error: cannot send/recv 08-24 04:07:30.403 I/Gecko ( 83): 08-24 04:07:30.453 I/Gonk ( 83): Setting nice for pid 1661 to 18 08-24 04:07:30.453 I/Gonk ( 83): Changed nice for pid 1661 from 18 to 18. 08-24 04:07:30.453 I/Gecko ( 83): [Parent 83] WARNING: waitpid failed pid:1661 errno:10: file

ni? Steve/Julien

Hmm... Maybe we need to apply image downsampling before actual ressizing. I'll check this tomorrow.

Can we have such big images using the Camera? If the answer is "no" (that means the user put the big images himself on the sdcard) then I think we should not block.

NI reporter for comment 8

Adding qawanted here to see if we can reproduce with smaller images.

Tried on build 20140415164003, and this can be reproduced by doing following steps: ---------------------------------------------------------------------- 1. Delete all the pictures in the sd card of the testing device 2. Copy some different pictures to the sd card (I copied about 27 pictures for testing), and then unplug the device or disable the usb storage 3. Launch SMS 4. Create a new message 5. Tap on the clip icon on the top right corner 6. Select 'Gallery' --> It starts to load pictures Actual results: ------------------------------------------------------------- After loading for a few seconds, a white screen appears for about 2 seconds, then Gallery and SMS exit, and it goes back to home screen. Build Info: ----------------------------------------------------------- Gaia c62bff0bfb8b069c962dfae2640e931cc9ad1bdf Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7e764399b4fc BuildID 20140415164003 Version 28.1 ro.build.version.incremental=eng.cltbld.20140415.201139 ro.build.date=Tue Apr 15 20:11:46 EDT 2014

(In reply to Bingqing Li from comment #11) > 2. Copy some different pictures to the sd card (I copied about 27 pictures > for testing) None of the pictures is larger than 1M

Here is the logcat

It seems similar to bug #995936.

Steve, do you think this would benefit from bug 996516 ?

Mmm comment 11 is different than comment 0, please file a new bug for comment 11 as this is related to Gallery and not SMS.

(In reply to Julien Wajsberg [:julienw] from comment #15) > Steve, do you think this would benefit from bug 996516 ? I'm afraid not... :( bug 996516 only focus on thumbnail creation process. But once the process finished the occupied memory is almost the same. We just have a decision about this general oom issue on tarako: 1) Set gallery hard limit to 2M px resolution. Image resolution over this limit will not appear in gallery.(I'll tag the bug if I find it) 2) Disable all the image croping in pick activity because croping using webGL lib might cost 3 times memory. So I will verify if the message app is safe for attaching image which resolution is less than 2M. If we still face OOM issue I will apply the image downsampling url first before resizing.

(In reply to Julien Wajsberg [:julienw] from comment #16) > Mmm comment 11 is different than comment 0, please file a new bug for > comment 11 as this is related to Gallery and not SMS. comment 11 maybe related with bug 993993,I'm not sure.

(In reply to Jason Smith [:jsmith] from comment #10) > Adding qawanted here to see if we can reproduce with smaller images. This issue does *not* reproduce for me on the 04/17/14 1.3T build when using smaller resolution images. Side note: the issue mentioned in comment 11 does reproduce for me when I have the attached "pic for testing" on the device's SD Card. Device: Tarako v1.3T MOZ BuildID: 20140417004002 Gaia: a8d2d399f2939f4845abaa0df57abab241a2c782 Gecko: d97dad54cb61 Version: 28.1 Firmware Version: sp6821

(In reply to Jason Smith [:jsmith] from comment #10) > Adding qawanted here to see if we can reproduce with smaller images. 1. --------------------------------------------------------------- picture A: Type jpeg, size 228.8KB, resolution 1280x800 picture B: Type jpeg, size 368KB, resolution 1920x1080 Launch SMS -> Create a new message -> Attach picture A -> Attach picture B ==> This issue can be reproduced. 2. --------------------------------------------------------- Five pictures taken from camera. Resolution 1280x960. Size 300~500 Launch SMS -> Create a new message -> Attach those five pictures ===> This issue cannot be reproduced.

Issue DOES occur for me on latest 1.3T following STR in comment 11. Using 121 pictures, all <500kb. 1.3T Environmental Variables: Device: Tarako 1.3T MOZ BuildID: 20140418014001 Gaia: f0872318d46781bb59d0a13a2e1fffb115b8ca2b Gecko: 32bb713e6d0d Version: 28.1 Firmware Version: sp6821a_gonk4.0_user.pac

I'd say it's more a Gallery issue then...

(In reply to Bingqing Li from comment #20) > (In reply to Jason Smith [:jsmith] from comment #10) > > Adding qawanted here to see if we can reproduce with smaller images. > > 1. > --------------------------------------------------------------- > picture A: Type jpeg, size 228.8KB, resolution 1280x800 > picture B: Type jpeg, size 368KB, resolution 1920x1080 > > Launch SMS -> Create a new message -> Attach picture A -> Attach picture B > > ==> This issue can be reproduced. > > > 2. > --------------------------------------------------------- > Five pictures taken from camera. > Resolution 1280x960. > Size 300~500 > > Launch SMS -> Create a new message -> Attach those five pictures > > ===> This issue cannot be reproduced. Hi Bingqing, Since image size limit should be set to 2mp, attachment 8405249 [details] seems not a vialidate test image for 1.3T. I tried some HD image(1920x1080) but I can't reproduce this issue. Could you provide image you mentioned in comment 20? Thanks.

(In reply to Steve Chung [:steveck] from comment #23) Here is picture A

(In reply to Steve Chung [:steveck] from comment #23) Here is picture B

Thanks for the testing images, I can reproduce now but not every time. Since resizing one image is safe now, I'll give a patch that could avoid resizing images at the same time.

Hi Jerry, sorry that bothering you some memory issue(maybe related to canvas): In current message app code base, we will create a temporary canvas for resizing image blob. But the memory usage will increase with 15+ MB additional size during this process, and it will not drop down immediately even I reset the canvas size to 0 and set to null. The memory will still decrease after resized(about few seconds) but the consumption will stack up if we attach huge image continuously. Is there any method that I can free the memory immediately, or other important note while using canvas? Thanks!

For 2d canvas, if you asign a size to canvas, b2g will release the buffer first. So if you reset the canvas size to zero, the canvas memory usage should decrease. You can use "get_about_memory" tool to check the memory usage.

Hi Julien, this patch use moz downsampling postfix for reducing the memory comsuption while resizing the image. I also change the resizing sequence to one by one instead of resizing all images at once, but it can not solve the problem because OOM still happens sometimes(if we only change the resizing sequence)

Comment on attachment 8410830 [details] [diff] [review] 0001-Downsampling-fix.patch Review of attachment 8410830 [details] [diff] [review]: ----------------------------------------------------------------- You still need to configure your git diff to 8 lines of context ;) For older version of git, it doesn't work, and you need to specify "-U8" to format-patch; maybe that's your issue here. I don't understand why you don't simply use the ratio as sample size? Also, looks like the changes in compose.js are already in master? I'll try to move forward bug 983172 today, it looks like it's more and more necessary. ::: apps/sms/js/utils.js @@ +354,5 @@ > var imageHeight = img.height; > + var targetWidth = > + mozSamplesize ? imageWidth : imageWidth / ratio; > + var targetHeight = > + mozSamplesize ? imageHeight : imageHeight / ratio; I don't understand this: so we don't use the ratio anymore if we use mozSampleSize? Isn't it wrong? @@ +372,5 @@ > > function ensureSizeLimit(resizedBlob) { > if (resizedBlob.size < limit) { > + canvas.width = canvas.height = 0; > + canvas = null; is it really necessary to set width/height to 0 if we set the canvas object itself to null? @@ +387,5 @@ > // size limitation. > + canvas.width = canvas.height = 0; > + canvas = null; > + if (mozSamplesize && mozSamplesize < 4) { > + obj.mozSamplesize *= 2; yeah it really looks like you should use the ratio instead... @@ +389,5 @@ > + canvas = null; > + if (mozSamplesize && mozSamplesize < 4) { > + obj.mozSamplesize *= 2; > + } else { > + delete object.mozSamplesize; "object" is not defined here

You should also clear the img.src just after drawing to the canvas to release that memory as well. See: https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/image_thumbnail.js#L45 I tried to get a common routine to implement this in bug 986229, but we're not there yet.

Sorry I lost some information for the patch: It's v1.3t only patch, that's why some changes is already in master(and I think it wuould git little help in this issue). The reason why I didn't use the ratio for the sample size is because I want to force the image down sample to next level. If the ratio is only slightly larger that 1, down sample will take the closest and valid integer and down sample might not work as we expected. And I did add the context in the gitconfig... maybe I need to add -U8 manually every time :-/

(In reply to Steve Chung [:steveck] from comment #32) > Sorry I lost some information for the patch: It's v1.3t only patch, that's > why some changes is already in master(and I think it wuould git little help > in this issue). That's what I supposed but I was surprised it applied so well on master ;) > > The reason why I didn't use the ratio for the sample size is because I want > to force the image down sample to next level. If the ratio is only slightly > larger that 1, down sample will take the closest and valid integer and down > sample might not work as we expected. Still, you could use the ratio and calculate a mozSampleSize from it (samplesize = Math.ceil(ratio); or force it to at least 2 (samplesize = Math.max(2, ratio)). > > And I did add the context in the gitconfig... maybe I need to add -U8 > manually every time :-/ Or update your git ;) IIRC this works fine with git 1.8 but not git 1.7.

(In reply to Julien Wajsberg [:julienw] from comment #33) > (In reply to Steve Chung [:steveck] from comment #32) > > Sorry I lost some information for the patch: It's v1.3t only patch, that's > > why some changes is already in master(and I think it wuould git little help > > in this issue). > > That's what I supposed but I was surprised it applied so well on master ;) But actually, we don't need this part in 1.3 because we can't attach several images from an activity.

Hi Marvin, although I've informed you the limitation of this patch, I think it should be necessary to leave a note here: it's only available for jpeg image, and backend didn't support that returning smaller decoded non-jpeg image resource. That means we still facing OOM when attaching huge png/bmp/... image. I suggest we could land this patch for jpeg solution and open another bug for non-jpeg image. We might need to propose another limitation while attaching non-jpeg image. but I think would should let our partner knows this limitation if they still facing oom problem with non-jpeg image frequently.

I update the patch a little bit with: - Avoiding the moz sample size postfix for non-jpeg image. Although applying postfix to non-jpeg will still return the resource without any problem, the memory will be aquired separately if the url is different(even the decoded resource is the same). Here we will access another resizing method directly instead of unnecessary and memory wasted sample size trying on non-jpeg image. - Apply Math.ceil(ratio) for initial sample size: We still need the sample size and ratio to prevent if sample size does not work as we expected. If we give 2 round down sampling but still exceed the limit, we switch back to original resizing method. - Clear image src(Thanks Ben!) About the resizing part in compose, if there is multipule images in the composer, image need to be resized will start loading at the same time originally. In the new patch we will make sure all the image loading and resizing will not happend at the same time(but it will take more time for whole process definitely).

Comment on attachment 8411684 [details] [diff] [review] Downsampling-fix v2 Review of attachment 8411684 [details] [diff] [review]: ----------------------------------------------------------------- About "canvas.width = canvas.height = 0", have you asked someone to know if it's better with it? I saw that you removed it, is it because of my comment, or did you ask? I actually don't know :) ::: apps/sms/js/utils.js @@ +386,5 @@ > // We will resize the blob if image quality = 0.25 still exceed > // size limitation. > + canvas = null; > + if (mozSamplesize && mozSamplesize < 4) { > + obj.mozSamplesize *= 2; I'm sorry, I still don't understand this. Why do you want to increase sampleSize without increasing the ratio?

(In reply to Julien Wajsberg [:julienw] from comment #37) > Comment on attachment 8411684 [details] [diff] [review] > Downsampling-fix v2 > > Review of attachment 8411684 [details] [diff] [review]: > ----------------------------------------------------------------- > > About "canvas.width = canvas.height = 0", have you asked someone to know if > it's better with it? I saw that you removed it, is it because of my comment, > or did you ask? I actually don't know :) okey, I got some conclusion after discussion with media team: - Setting the canvans width/height to 0 will clear the buffer 'immediately' - Setting canvas to null might help a little bit that make this canvas object gc as early as possible So in this case, we definitely need to reset the canvas dimension to free the resource instead of relying on gc scheduling. Doing both thing would be the best even the canvas object might be trivial after buffer cleared. BTW setting the image src to empty string also has the same effct that help buffer released ASAP. > > ::: apps/sms/js/utils.js > @@ +386,5 @@ > > // We will resize the blob if image quality = 0.25 still exceed > > // size limitation. > > + canvas = null; > > + if (mozSamplesize && mozSamplesize < 4) { > > + obj.mozSamplesize *= 2; > > I'm sorry, I still don't understand this. Why do you want to increase > sampleSize without increasing the ratio? sampleSize is for downsampling, and ratio is for original size calculation method. Here is an example: 1) A jpg image enters resizing 1st round with sampleSize applied 2) After downsampling, if the size still exceed, double the sampleSize and try again(if sampleSize not over the threshold) 3) If the size still exceed and sampleSize over threshold, use the original ratio for resizing (original canvas size calculation method). So ratio should not be adjusted before this step.

(In reply to Steve Chung [:steveck] from comment #38) > (In reply to Julien Wajsberg [:julienw] from comment #37) > > Comment on attachment 8411684 [details] [diff] [review] > > Downsampling-fix v2 > > > > Review of attachment 8411684 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > About "canvas.width = canvas.height = 0", have you asked someone to know if > > it's better with it? I saw that you removed it, is it because of my comment, > > or did you ask? I actually don't know :) > > okey, I got some conclusion after discussion with media team: > - Setting the canvans width/height to 0 will clear the buffer 'immediately' > - Setting canvas to null might help a little bit that make this canvas > object gc as early as possible > > So in this case, we definitely need to reset the canvas dimension to free > the resource instead of relying on gc scheduling. Doing both thing would be > the best even the canvas object might be trivial > after buffer cleared. BTW setting the image src to empty string also has the > same effct that help buffer released ASAP. great, let's do that ! > > > > ::: apps/sms/js/utils.js > > @@ +386,5 @@ > > > // We will resize the blob if image quality = 0.25 still exceed > > > // size limitation. > > > + canvas = null; > > > + if (mozSamplesize && mozSamplesize < 4) { > > > + obj.mozSamplesize *= 2; > > > > I'm sorry, I still don't understand this. Why do you want to increase > > sampleSize without increasing the ratio? > > sampleSize is for downsampling, and ratio is for original size calculation > method. Here is an example: > 1) A jpg image enters resizing 1st round with sampleSize applied > 2) After downsampling, if the size still exceed, double the sampleSize and > try again(if sampleSize not over the threshold) > 3) If the size still exceed and sampleSize over threshold, use the original > ratio for resizing (original canvas size calculation method). So ratio > should not be adjusted before this step. That's where I don't understand; why not use only the ratio (and calculate a samplesize from it)? What does the sample size alone (without ratio) give us ? In my opinion, the only thing that using the sample size alone without a ratio will give us is a ugly jpg (normal resolution jpg, but decoded as if it has low resolution), and I don't see the reason.

Adding the cavas size reset and sample size threshold adjustment Hi Julien, please ping me if you still concern about the implementation, thanks.

Hi Steve, noted. let's land the patch and fix Jpeg first. i tried 1MP 1024x1024px *.png, file size ~ 2MB and attached to MMS, works fine. no crash. we still need to justify boundary for *.png, *.bmp and *.gif.

btw, on build 20140422164001.

Comment on attachment 8412413 [details] Link to github I left a comment on github along with a sample code that I didn't test: https://github.com/mozilla-b2g/gaia/pull/18688

Hi SC, is it possible that platfrom failed to create a down sampled buffer with a normal jpeg image?

We're especially interested to know if this can happen on B2G builds.

I'm not sure about what kind of platform failure you guys are interested in, so I list all the failures I can think of: 1. Fail as crash: Not possible via -moz-samplesize. Gecko will ignore any non-integer value and libjpeg will adjust it to a feasible scaling factor. 2. Fail as no image: Not really possible if the original jpeg can be loaded successfully. The only scenario is that we are under low memory, either malloc() fail or hit the image buffer limit (65MB on master). 3. Fail as returning the original image without downsampling: This will only happen if the value of -moz-samplesize is 0 or non-integer.

Sorry that unassigned myself because of pto Basically I could agree julien's WIP patch if there is no edge case for jpeg image applied moz samplesize(or we can apply it first and open another follow-up if this exception does happen). Thanks for the quick feedback and we just need some clean up and test cases.

Julian, I am told by Steve that you will continue work on this bug?

yep, I'll have a patch ready for review today

So, not today, but tomorrow it will.

In the mean time, can QA re-test the new build with STR given? I suspect gonk/gecko improvement have already solve this bug.

Re-test on build 20140428014001. Tried 3 times, and each time attached 10 pictures, and cannot reproduce this issue. Build Info ----------------------------------------------------- Gaia 8895b180ed636069473703d0e7b73086989601ce Gecko https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/7caf4b5abfce BuildID 20140428014001 Version 28.1 ro.build.version.incremental=eng.cltbld.20140428.052137 ro.build.date=Mon Apr 28 05:21:44 EDT 2014

Hey Oleg, do you think you would be able to review this patch? I added the patch for bug 1001422 because I need it for running the test successfully (I think), but obviously it will need to land separately. So after applying the patch you'll need to remake your profile and restart your Firefox that runs the Test-agent. The original PR at https://github.com/mozilla-b2g/gaia/pull/18667 contains comments that could help you getting some history. Otherwise, ping me on IRC or on the bug if you have any question. Thanks !

This is the same patch for v1.3t. Nearly the same code, except we take also some code from Bug 929027 in compose.js.

(In reply to Julien Wajsberg [:julienw] from comment #55) > So after applying the patch you'll need to remake your profile and restart > your Firefox that runs the Test-agent. Alternative is to enable image.mozsamplesize.enabled using about:config

I was unable to reproduce this issue on the latest Tarako 1.3t build 1.3t Environmental Variables: Device: Tarako 1.3t BuildID: 20140429014002 Gaia: b5adc5a943d3abbd6ab070a47c847f2c24891cc5 Gecko: e9890f5d4709 Version: 28.1 Firmware Version: sp6821

WFM per comment 58.

(In reply to Julien Wajsberg [:julienw] from comment #55) > Created attachment 8414562 [details] > master github PR > > Hey Oleg, do you think you would be able to review this patch? > > I added the patch for bug 1001422 because I need it for running the test > successfully (I think), but obviously it will need to land separately. > > So after applying the patch you'll need to remake your profile and restart > your Firefox that runs the Test-agent. > > The original PR at https://github.com/mozilla-b2g/gaia/pull/18667 contains > comments that could help you getting some history. Otherwise, ping me on IRC > or on the bug if you have any question. > > Thanks ! Hey Julien, this bug is marked as resolved now, do you still need a review for your patches?

The changes from the patches are still useful for performance reason and I'd like them to go in. Let's just forget the 1.3t-specific patch then.

James, this bug is no longer the cause of your specific issue, since we cannot reproduce it on Tarako anymore (see comment 58). Remove reference to Tarako on summary and whiteboard.

Comment on attachment 8414562 [details] master github PR Looks good! Hopefully we'll make "_resizeImageBlobWithRatio" simpler some day :) Just left few nits at GitHub. Thanks!

Comment on attachment 8414562 [details] master github PR Hey Oleg, it's ready for you now :) I used images from http://mayang.com/textures/Plants/html/Leaves/ to test the code. I attached 5 images from gallery, and I was using this command to check the memory consumption: while adb shell b2g-ps ; do echo; sleep .3 ; done | grep Messages The meaningful number is just before "ffffffff". On master, I get about 70MB as peak consumption, while with the patch the highest is around 52MB. So it's a clear win. In the future we'll want to use the clean code in https://github.com/mozilla-b2g/gaia/blob/v1.3t/shared/js/media/downsample.js (not yet in master) along with the result of bug 983172 to have a cleaner code, but I think this is already good enough like this. We can also move all the image related utility functions in another file.

Note that I've put all the changes in its own commit so that it's easier to review. I'll need to squash obviously :)

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #64) > Comment on attachment 8414562 [details] > master github PR > > Hey Oleg, it's ready for you now :) > > I used images from http://mayang.com/textures/Plants/html/Leaves/ to test > the code. > > I attached 5 images from gallery, and I was using this command to check the > memory consumption: > > while adb shell b2g-ps ; do echo; sleep .3 ; done | grep Messages > > The meaningful number is just before "ffffffff". > > On master, I get about 70MB as peak consumption, while with the patch the > highest is around 52MB. So it's a clear win. Yep, memory consumption difference is pretty noticeable on my devices, great work! > In the future we'll want to use the clean code in > https://github.com/mozilla-b2g/gaia/blob/v1.3t/shared/js/media/downsample.js > (not yet in master) along with the result of bug 983172 to have a cleaner > code, but I think this is already good enough like this. We can also move > all the image related utility functions in another file. It's nice that downsample.js is in shared folder, I think we need to do our best to make the most of our current\future image processing code as generic\shareable as possible, so that more developers can use\test\improve it. > Note that I've put all the changes in its own commit so that it's easier to > review. I'll need to squash obviously :) Looks good, r+! Just check what is wrong with failing unit tests and make Travis happy again :) Thanks!

Thanks, I just pushed the unit test fixes + your suggested changes, waiting for a green travis now!

master: eb6ee83804ea79e65b716385a46a86e256bbda4c

BTW, I learned this morning that instead of : while adb shell b2g-ps ; do echo; sleep .3 ; done | grep Messages we can do watch -n 0.3 adb shell b2g-procrank Seems like the "Uss" column is what is really important.

Mmm except watch clear the screen between each execution so maybe it's not perfect to see the evolution of the command.

uplifted in v1.3t as part of bug 1008071