Closed Bug 949779 Opened 6 years ago Closed 6 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

(blocking-b2g:1.3T+, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: djf, Assigned: steveck)

References

Details

(Whiteboard: [SI-testing-blocker])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
julienw
: review+
Details | Review
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.
In case it is not clear from the description, I'm talking about image attachments for MMS messages.
Blocks: 949748
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.
Wow, great, we absolutely _need_ this :)
Depends on: 854795
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.
Assignee: nobody → schung
Whiteboard: [MemShrink]
David, can you give a heads up on this? How do we use the new feature?
Flags: needinfo?(dflanagan)
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
Flags: needinfo?(dflanagan)
Tracking [MemShrink] in the meta-bug.
Whiteboard: [MemShrink]
(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 !
(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).
1.3T+ as this blocks partner testing on tarako
blocking-b2g: --- → 1.3T+
Steve, do you mind updating the status on this bug? Thanks
Flags: needinfo?(schung)
(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.
Flags: needinfo?(schung)
(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.
Hi Steve, do you think you can provide an ETA on this bug? Thanks
Flags: needinfo?(schung)
Attached file 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).
Attachment #8389092 - Flags: review?(felash)
Flags: needinfo?(schung)
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 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.
Attachment #8389092 - Flags: review?(felash) → review+
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
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 970759
Needinfo so that you don't forget the follow up when you come back ;)
Flags: needinfo?(schung)
Blocks: 983172
(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)
Flags: needinfo?(schung)
v1.3t: 445505c19e88395fe9808dabbe1b5e271580ce8d

I hope I resolved the conflict correctly.
Whiteboard: [SI-testing-blocker]
You need to log in before you can comment on or make changes to this bug.