995116 - [MMS] SMS/MMS exits and a white screen appears for about 2 seconds after attach a large pixel picture from gallery

Status: RESOLVED FIXED

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

Description

[Bingqing Li]

Attachment: pic for testing

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.

Comment 1

[Bingqing Li]

Attachment: slog

Comment 2

[Bingqing Li]

Attachment: logcat

Comment 3

[Bingqing Li]

Attachment: STR-Video

Comment 4

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

triage: 1.3T+ broken feature

Comment 5

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

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

Comment 6

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

ni? Steve/Julien

Comment 7

[Steve Chung [:steveck]]

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

Comment 8

[Julien Wajsberg [:julienw]]

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.

Comment 9

[Julien Wajsberg [:julienw]]

NI reporter for comment 8

Comment 10

[Jason Smith [:jsmith]]

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

Comment 11

[Bingqing Li]

Attachment: STR-Video-2

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

Comment 12

[Bingqing Li]

(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

Comment 13

[Bingqing Li]

Attachment: logcat-loading-pic

Here is the logcat

Comment 14

[Bingqing Li]

It seems similar to bug #995936.

Comment 15

[Julien Wajsberg [:julienw]]

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

Comment 16

[Julien Wajsberg [:julienw]]

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 17

[Steve Chung [:steveck]]

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

Comment 18

[yang.zhao]

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

Comment 19

[Matthew Vaughan]

(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

Comment 20

[Bingqing Li]

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

Comment 21

[Brogan Zumwalt [Inactive]]

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

Comment 22

[Julien Wajsberg [:julienw]]

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

Comment 23

[Steve Chung [:steveck]]

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

Comment 24

[Bingqing Li]

Attachment: pic_A.jpg

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

Comment 25

[Bingqing Li]

Attachment: pic_B.jpg

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

Comment 26

[Steve Chung [:steveck]]

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.

Comment 27

[Steve Chung [:steveck]]

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!

Comment 28

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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.

Comment 29

[Steve Chung [:steveck]]

Attachment: 0001-Downsampling-fix.patch

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 30

[Julien Wajsberg [:julienw]]

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

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 32

[Steve Chung [:steveck]]

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  :-/

Comment 33

[Julien Wajsberg [:julienw]]

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

Comment 34

[Julien Wajsberg [:julienw]]

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

Comment 35

[Steve Chung [:steveck]]

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.

Comment 36

[Steve Chung [:steveck]]

Attachment: Downsampling-fix v2

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 37

[Julien Wajsberg [:julienw]]

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?

Comment 38

[Steve Chung [:steveck]]

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

Comment 39

[Julien Wajsberg [:julienw]]

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

Comment 40

[Steve Chung [:steveck]]

Attachment: Link to github

Adding the cavas size reset and sample size threshold adjustment

Hi Julien, please ping me if you still concern about the implementation, thanks.

Comment 41

[Marvin Khoo [:Marvin_Khoo]]

Attachment: 1024px-ValleyTodaySign.png

Comment 42

[Marvin Khoo [:Marvin_Khoo]]

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.

Comment 43

[Marvin Khoo [:Marvin_Khoo]]

btw, on build 20140422164001.

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8412413 [details] [review]
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

Comment 45

[Steve Chung [:steveck]]

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

Comment 46

[Julien Wajsberg [:julienw]]

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

Comment 48

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

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.

Comment 49

[Steve Chung [:steveck]]

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.

Comment 50

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

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

Comment 51

[Julien Wajsberg [:julienw]]

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

Comment 52

[Julien Wajsberg [:julienw]]

So, not today, but tomorrow it will.

Comment 53

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

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

Comment 54

[Bingqing Li]

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

Comment 55

[Julien Wajsberg [:julienw]]

Attachment: 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 !

Comment 56

[Julien Wajsberg [:julienw]]

Attachment: v1.3t github PR

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

Comment 57

[Julien Wajsberg [:julienw]]

(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

Comment 58

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

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

Comment 59

[Jason Smith [:jsmith]]

WFM per comment 58.

Comment 60

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #55)
> Created attachment 8414562 [details] [review]
> 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?

Comment 61

[Julien Wajsberg [:julienw]]

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.

Comment 62

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

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 63

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8414562 [details] [review]
master github PR

Looks good! Hopefully we'll make "_resizeImageBlobWithRatio" simpler some day :) Just left few nits at GitHub.

Thanks!

Comment 64

[Julien Wajsberg [:julienw]]

Comment on attachment 8414562 [details] [review]
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.

Comment 65

[Julien Wajsberg [:julienw]]

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 :)

Comment 66

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] (away May 8th) from comment #64)
> Comment on attachment 8414562 [details] [review]
> 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!

Comment 67

[Julien Wajsberg [:julienw]]

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

Comment 68

[Julien Wajsberg [:julienw]]

master: eb6ee83804ea79e65b716385a46a86e256bbda4c

Comment 69

[Julien Wajsberg [:julienw]]

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.

Comment 70

[Julien Wajsberg [:julienw]]

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

Comment 71

[Julien Wajsberg [:julienw]]

uplifted in v1.3t as part of bug 1008071