949779 - Modify the SMS app to use the new downsample-and-decode image feature when resizing images

Status: RESOLVED FIXED

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

Description

[David Flanagan [:djf]]

When bug 854795 is fixed, we'll have a better way to reduce the size of large images, and the SMS app should be modified to use it because it will save a lot of memory and prevent background apps from being killed.

Comment 1

[David Flanagan [:djf]]

In case it is not clear from the description, I'm talking about image attachments for MMS messages.

Comment 2

[David Flanagan [:djf]]

Actually, the app should also use this new API for creating the thumbnail it uses for the attached image as well as for resizing the image itself.

Comment 3

[Julien Wajsberg [:julienw]]

Wow, great, we absolutely _need_ this :)

Comment 4

[Steve Chung [:steveck]]

We do need it for attaching image, especially the original image resizing method might cause the app crash when device memory is limited. Taking the bug since I could have more time to work with gecko side.

Comment 5

[Julien Wajsberg [:julienw]]

David, can you give a heads up on this? How do we use the new feature?

Comment 6

[David Flanagan [:djf]]

The gecko feature landed yesterday, so it should be ready for use.  You add #-moz-samplesize=n where n is a number between 2 and 8, I think.  If you specify a samplesize of 4, you're giving a hint that you only need 1/4th of the width and height of the image.  For jpeg images, it will work, and you'll get a smaller image.  If you are using HTMLImageElement, the naturalWidth and naturalHeight properties show the size that the image was decoded at.

The trick is that in order to use this, you need to know what the full size of the image is.  Gallery has code (gallery/js/imagesize.js and shared/js/media/jpeg_metadata_parser.js) that will determine the size of an image for you without decoding it.

For examples, take a look at the UI tests patch I attached to bug 854975, and also at my wip branch here: https://github.com/davidflanagan/gaia/tree/bug949755

Comment 7

[Nicholas Nethercote [inactive]]

Tracking [MemShrink] in the meta-bug.

Comment 8

[Julien Wajsberg [:julienw]]

(In reply to David Flanagan [:djf] from comment #6)
> The gecko feature landed yesterday, so it should be ready for use.  You add
> #-moz-samplesize=n where n is a number between 2 and 8, I think.  If you
> specify a samplesize of 4, you're giving a hint that you only need 1/4th of
> the width and height of the image.

Do you know whether this is 1/4th of the width/height or 1/4th of the area?

>  For jpeg images, it will work, and
> you'll get a smaller image.  If you are using HTMLImageElement, the
> naturalWidth and naturalHeight properties show the size that the image was
> decoded at.

> 
> The trick is that in order to use this, you need to know what the full size
> of the image is.  Gallery has code (gallery/js/imagesize.js and
> shared/js/media/jpeg_metadata_parser.js) that will determine the size of an
> image for you without decoding it.
> For examples, take a look at the UI tests patch I attached to bug 854975,
> and also at my wip branch here:
> https://github.com/davidflanagan/gaia/tree/bug949755

Thanks !

Comment 9

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #8)
> (In reply to David Flanagan [:djf] from comment #6)
> > The gecko feature landed yesterday, so it should be ready for use.  You add
> > #-moz-samplesize=n where n is a number between 2 and 8, I think.  If you
> > specify a samplesize of 4, you're giving a hint that you only need 1/4th of
> > the width and height of the image.
> 
> Do you know whether this is 1/4th of the width/height or 1/4th of the area?

It will take 1/4th of the width/height here, and please note the graphics lib will only resize to 1/2, 1/4 and 1/8 no matter the value you gave is odd number or float(it will take the closest one).

Comment 10

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

1.3T+ as this blocks partner testing on tarako

Comment 11

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

Steve, do you mind updating the status on this bug? Thanks

Comment 12

[Steve Chung [:steveck]]

(In reply to Joe Cheng [:jcheng] from comment #11)
> Steve, do you mind updating the status on this bug? Thanks

Message app has 2 parts could benifit from this patch:
- Resizing the image to avoid the size over limitation.
- Rendering image attachment thumbnails.

tarako might facing oom crash issue while doing these two step, and I will start from 2nd part because I notice the latency after the resizing image notification dismiss is quite long. Please note that gecko dev said that patch from backend only support jpg image, which means we might still facing oom issue on tarako device while handling other type of image.

Comment 13

[Julien Wajsberg [:julienw]]

(In reply to Steve Chung [:steveck] from comment #12)

> tarako might facing oom crash issue while doing these two step, and I will
> start from 2nd part because I notice the latency after the resizing image
> notification dismiss is quite long.

Please note that we use a fixed delay for this notification. The time the notification is displayed does not depend on the time it takes to resize.
That said, I've noticed a flickering when adding an image, so there could be a quick win here.

> Please note that gecko dev said that
> patch from backend only support jpg image, which means we might still facing
> oom issue on tarako device while handling other type of image.

I think .jpg images are by far the most common use case anyway.

Comment 14

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

Hi Steve, do you think you can provide an ETA on this bug? Thanks

Comment 15

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, this patch utilize the image downsampling gecko patch in Bug 854795. I only apply this for thumbnail rendering because I discover it takes long time for rendering part.It did reduce the duration for all process to less then 3 sec on tarako, and the oom didn't happen during my testing. I might apply this in mms image resizing in the future(within another bug).

Comment 16

[Julien Wajsberg [:julienw]]

Comment on attachment 8389092 [details] [review]
Link to github

I added comments on the pull request.

So either we keep it like this, or we redo a small lib to read the JPEG dimensions. My preference would be the second solution, but if we lack time, I would be fine with doing it later.

Comment 17

[Julien Wajsberg [:julienw]]

Comment on attachment 8389092 [details] [review]
Link to github

r=me with the small comments in the test and a follow-up to properly get the jpg dimensions.

Comment 18

[Steve Chung [:steveck]]

I agree we should have a better solution for this, I'll create a follow up with my WIP about parsing the jpg header for downsampling. Land this patch first to avoid the oom and performance first:

in master: 8e94c7e64c387bd66b42729ef14fde0fc20d0c87

Comment 20

[Julien Wajsberg [:julienw]]

Needinfo so that you don't forget the follow up when you come back ;)

Comment 21

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> Needinfo so that you don't forget the follow up when you come back ;)

How could I forget ;) (please ref bug 983172)

Comment 22

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

v1.3t: 445505c19e88395fe9808dabbe1b5e271580ce8d

I hope I resolved the conflict correctly.