876467 - [mms] store, and reuse thumbnails to display the images

Status: RESOLVED INVALID

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

Description

[Julien Wajsberg [:julienw]]

From Bug 875338 comment 10

* we'll need to generate and store thumbnail images, and use them here. We can't display all big images in a thread, that would use too much memory and impair performance.

* We should use "background-size:cover" style thumbnails, for the panorama case, but still have landscape and portrait thumbnail form factors

* Be careful to not generate thumbnail for too big images (see Bug 805114 for a similar issue in Gallery)

Asking leo? here because I can't let a phone go in the wild with this.

Comment 1

[Corey Frang [:gnarf]]

I'd like to speak in support of blocking leo+ - We really need to fix this performance issue before the phone is in the wild.

Comment 3

[Corey Frang [:gnarf]]

:kaze - If you're going to work on this, can you take a look at bug 878042 while you are digging into it?

Please ping me for review here! Thanks!

Comment 4

[Fabien Cazenave [:kaze]]

Will do for sure.

Comment 5

[Maria Angeles Oteo (:oteo)]

I am not sure if the patch that Kaze will create for this bug is going to resolve directly the scroll issue reported in bug 875338 (that was marked as Duplicated of this)

Kaze, please, let me know, in case it will not fix it I will reopen bug 875338 to ensure that need to be handled.

Comment 6

[Corey Frang [:gnarf]]

It should definitely handle this problem Maria --  If we are calculating the preview size before we render it, we will know the proper size as it is inserted into the DOM.

Comment 7

[Fabien Cazenave [:kaze]]

(In reply to Julien Wajsberg [:julienw] from comment #0)
> From Bug 875338 comment 10
> * We should use "background-size:cover" style thumbnails, for the panorama
> case, but still have landscape and portrait thumbnail form factors
> 
> * Be careful to not generate thumbnail for too big images (see Bug 805114
> for a similar issue in Gallery)
> 

This part has been done with the patch I wrote for bug 878042.

(In reply to Julien Wajsberg [:julienw] from comment #0)
> From Bug 875338 comment 10
> 
> * we'll need to generate and store thumbnail images, and use them here. We
> can't display all big images in a thread, that would use too much memory and
> impair performance.
> 

Note that we only compute thumbnails for “small” images (< 400 kB); so unless we’re dealing with an MMS thread that has a lot of small images, I don’t expect this caching to be a very significant performance improvement.

Comment 8

[Julien Wajsberg [:julienw]]

I suggest we wait for Bug 876736 completion to decide if we need more here.

Comment 9

[Dietrich Ayala (:dietrich)]

blocking- per comment #8. re-nominate when sure this blocks.

Comment 10

[Julien Wajsberg [:julienw]]

I'm quite confident we'll need this in leo after all. We checked some things with the workload Jon Hylands made and this is quite messy as soon as we have a few images, and I'd like to be able to lazy load them as we scroll, just like we do in other apps.

Plus, we'll change the generation limit to whatever the mms size limit is * 1.2 (bug 885584) so that users can at least see their images, and therefore I'm afraid we can quite easily exhaust the device's memory.

Also, we checked in Android, and Android does have a number limit for stored MMS messages, maybe we should do that as well, but that's really 2 different things.

Talked with Michal and he would like to focus on this.

Note: this bug is not about the lazy loading part, I'll file another bug for this. It's maybe possible to work in parallel on both bugs.

Comment 11

[Julien Wajsberg [:julienw]]

In this bug we'll also switch from data url to blob url, which I think gecko is handling better when it's low in memory.

Comment 12

[Mike Lee [:mlee]]

(In reply to Julien Wajsberg [:julienw] from comment #10)
> I'm quite confident we'll need this in leo after all. We checked some things
> with the workload Jon Hylands made and this is quite messy as soon as we
> have a few images, and I'd like to be able to lazy load them as we scroll,
> just like we do in other apps.
> 
> Plus, we'll change the generation limit to whatever the mms size limit is *
> 1.2 (bug 885584) so that users can at least see their images, and therefore
> I'm afraid we can quite easily exhaust the device's memory.

Julien,

We're inclined to leo+ this based on your comments but would be more confident if you provided memory load details, as in MBs of memory being used and if possible by how much your/Michal's changes would reduce that.

Thanks,
Triage

Comment 13

[Julien Wajsberg [:julienw]]

plan to profile this first thing tomorrow with :gsvelto, so that we can have actual figures.

Comment 14

[Julien Wajsberg [:julienw]]

So, after profiling with Gabriele, I'm not so sure that storing/reusing would be such an improvement. However, we need to investigate with using blobs instead of data url.

Also, Bug 887196 or Bug 887198 could be a better idea (even if bug 887198 is risky).

Basically, we found that when displaying a big thread (either SMS or MMS), a big part of the time is spent in applying CSS. When displaying a thread containing lots of MMS we spend a lot of time doing CSP stuff, and manipulating history. We didn't have a precise enough profile, but we guessed that CSP was done because of canvas and/or iframes (since we're displaying attachments in iframe) and the history manipulation was done because of iframes. The actual time spent in image manipulation was not big enough to warrant storing the thumbnail yet.

However, we noticed that the sms process was using a lot of memory while displaying the thread, and I'd like to see if the memory consumption is the same using blobs instead of data url here.

Comment 15

[Gabriele Svelto [:gsvelto]]

Just to add another datapoint, I took an about:memory dump of the large MMS thread and the results are... weird. I cannot attach the report because even after compression it's over 16MiB mostly due to the zillion of large data URLs that are captured by the memory reporter. That being said the report say that we have ~30MiB of unused GC things and ~20MiB of analysis temporary stuff. The first thing points to fragmentation in the GC heap but that's too much even for an extreme workload, the second thing is weird because it should go away after a GC, except it doesn't.

All in all it look pretty strange so to reinforce Julien's point before we do large changes to the MMS app it would be better if we can do proper CPU & memory profiling and figure out what's wrong. Doing otherwise would be taking a stab in the dark.

Comment 16

[Dietrich Ayala (:dietrich)]

Please renominate if investigation proves in favor of this (big scary) approach.

Comment 17

[Julien Wajsberg [:julienw]]

I have a feeling that this becomes obsolete with bug 995116.

Comment 18

[Julien Wajsberg [:julienw]]

Displaying thumbnails is not so expensive now that we use moz-sample-size. Let's close this.