996516 - [MMS] Use less memory for creating attachment thumbnail

Status: RESOLVED DUPLICATE of bug 983172

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

Description

[Ting-Yu Chou [:ting] (away)]

Currently Attachment.getThumbnail() works as such:

  var img = new Image();
  img.src = Utils.getDownsamplingSrcUrl(url: window.URL.createObjectURL(messageData.blob), ...);
  img.onload = ... {
    var canvas = document.createElement('canvas');
    var context = canvas.getContext(...);
    context.drawImage(img, ...);
    callback({..., data: canvas.toDataURL(...);});
  }

The generated data url will then be applied to thumbnail's div for its style "background-image" property.

We'd like to lower the memory usage here, and come up two options:

  1. Reuse the canvas and context.
  2. Use the downsampling url for "background-image" property directly,
     eliminate image and canvas.

Prefer option 2 for now.

Comment 1

[Gabriele Svelto [:gsvelto] (PTO)]

Ting-Yu note that we already had bug 889902 for reducing the memory used by attachment; unfortunately at the time we tested this that approach didn't seem to give the benefits we expected. You might want to revisit that bug and see if it gives us an improvement.

Comment 2

[Thinker Li [:sinker]]

Kan-Ru is working on bug 996512 as a complementary feature to make sure image sources don't occupy memory if it is not used.  Once bug 996512 is finished, the bug 889902 should be benefited.

Comment 3

[Steve Chung [:steveck]]

Attachment: (WIP)Use-less-memory-for-creating-attachme.patch

Hey Ting, here is the simple patch that avoid using canvas for thumbnail creation. The reason why I didn't remove the temporary image is because we need to display the corrupt icon if the image loading error, and we also need the image size for attachment size calculation(maybe we can get the size from header instead of loading image).
But the the memory comsuption still get a huge improvement in this patch: Before the patch applied, USS will be increased from 15.X MB to 3X or sometimes 4X MB when enter a long thread(heavy reference workload). After patch applied, USS could only reach 25MB at most. Although both cases will resume to 19.X MB after load complete, user will still benifit from this change because less memory and cup comsuption while loading means less chance that apps been killed by oom.

Comment 4

[Ting-Yu Chou [:ting] (away)]

I guess that means we're on the right direction.

As you told me earlier, getting image size and timing of calling revokeObjectURL() for the downsampling url still need some work.

Comment 5

[Ting-Yu Chou [:ting] (away)]

Comment on attachment 8407345 [details] [diff] [review]
(WIP)Use-less-memory-for-creating-attachme.patch

Tested the WIP on Tarako:

  1. Install reference-workload-heavy
  2. Launch SMS, wait til all the threads are loaded
  3. Enter BIG-THREAD-MIXED, eyeballs the USS from b2g-info til finished loading

Tried 3 times:

  with WIP   : min=16,15,18 max=21,23,23
  without WIP: min=16,16,17 max=30,32,34

Comment 6

[Steve Chung [:steveck]]

Comment on attachment 8407345 [details] [diff] [review]
(WIP)Use-less-memory-for-creating-attachme.patch

Hi Julien, although this patch could reduce the memory usage while creating the thumbnail , I can't find a proper place for revoke url. I think revoke while element removal is not a good choise here because we clear the message list frequently and I'm not sure it's a proper way to select all the attachments and revoke every time before clear list, or you think set a static timeout(like 10 sec) to revoke is enough in this case?

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8407345 [details] [diff] [review]
(WIP)Use-less-memory-for-creating-attachme.patch

Review of attachment 8407345 [details] [diff] [review]:
-----------------------------------------------------------------

You should generate your patches using -U8 or configure your .gitconfig with:

  [diff]
    context = 8


About the width/height calculation, if I'm not wrong, the only reason we load the image is to calculate the correct width/height for a ratio. Maybe using "background-size: contain", we don't even need this, as this calculation is done by the platform? We would only need a 120x120 div, and the platform would calculate automatically.
The case we would lose is when we would send smaller images; in that case the images would be scaled up, but since it's not the common case (common case is sending a picture) I wouldn't worry about this.

What do you think?

About the revoke, I asked on dev.b2g but I don't have a better solution than using a setTimeout.

Comment 8

[Julien Wajsberg [:julienw]]

Also, I didn't try it on device, but please check that it still works inside an iframe; IIRC we had issues in the past.

For also some history you can have a look to the patch for bug 878042 which is the last time we changed this code. The "downsampling hash" feature really helps us undoing part of this work here!

Comment 9

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #7)
> Comment on attachment 8407345 [details] [diff] [review]
> (WIP)Use-less-memory-for-creating-attachme.patch
> 
> Review of attachment 8407345 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You should generate your patches using -U8 or configure your .gitconfig with:
> 
>   [diff]
>     context = 8
> 
> 
> About the width/height calculation, if I'm not wrong, the only reason we
> load the image is to calculate the correct width/height for a ratio. Maybe
> using "background-size: contain", we don't even need this, as this
> calculation is done by the platform? We would only need a 120x120 div, and
> the platform would calculate automatically.
> The case we would lose is when we would send smaller images; in that case
> the images would be scaled up, but since it's not the common case (common
> case is sending a picture) I wouldn't worry about this.
> 
> What do you think?
> 
The reason why I didn't simplify the size calculation because the fixed size container will give different visual effect to user(they might saw more place holder background depend on the ratio) and that's why I still keep this. Maybe apply max width and height to 120 px could make this container looks better(and the best thing to remove size calculation is we don't even need the temp image tag !). I think we might need ux suggestion here.

> Also, I didn't try it on device, but please check that it still works inside an iframe; IIRC we had > issues in the past.

I tried it for composer part and I didn't see any regression there.

Comment 10

[Julien Wajsberg [:julienw]]

I don't see why a fixed container would do something differently to what we do "manually" currently. Can you provide screenshots of what would happen with my proposal ? That would be easier, both for me and Omega :)

Comment 11

[Omega Feng [:Omega] [:馮於懋]]

Steve, IMO we can crop all attached images into squares (1:1), but I think Vicky can help decide this.

Comment 12

[Etienne Segonzac (:etienne)]

Not sure which information is required from me here...

Comment 13

[Victoria Gerchinhoren [:vicky]]

We have discussed this issue with Omega an Steve, and the fixed container is a not elegant solution for showing variable ratio images, so we should stick to this spec and use option 3 for the extreme ratio cases: https://www.dropbox.com/s/pv4w7k8nki8rnuo/MMSPreviewThumbnailSpec_Landscape_2.0.png

If we set a fix ratio for images, we will all the time have some extra space surrounding the image and that adds -information- noise to the layout.

Comment 14

[Julien Wajsberg [:julienw]]

Steve, sorry, see comment 10, I missed my needinfo..

Comment 15

[Steve Chung [:steveck]]

I think visual didn't support the fixed container in comment 13... For now we show the container background only when aspect ratio is wider than 3:2. I will try some css solution that could still fit into current behavior.

Comment 16

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

triage: let's put this to backlog

Comment 17

[Julien Wajsberg [:julienw]]

Wondering how this bug works with bug 995116...

Comment 18

[James Zhang (Spreadtrum)]

(In reply to Joe Cheng [:jcheng] from comment #16)
> triage: let's put this to backlog

Why? I think it's time to workaround tarako bug.

Comment 19

[Julien Wajsberg [:julienw]]

James, do you still see issues on Tarako?

Comment 20

[Julien Wajsberg [:julienw]]

(In reply to Victoria Gerchinhoren [:vicky] from comment #13)
> We have discussed this issue with Omega an Steve, and the fixed container is
> a not elegant solution for showing variable ratio images, so we should stick
> to this spec and use option 3 for the extreme ratio cases:
> https://www.dropbox.com/s/pv4w7k8nki8rnuo/
> MMSPreviewThumbnailSpec_Landscape_2.0.png

Victoria, the image is not available anymore, can you put it somewhere?

> 
> If we set a fix ratio for images, we will all the time have some extra space
> surrounding the image and that adds -information- noise to the layout.

I don't understand: I think that's what we do now. Can you be clearer about what is wrong in my proposal, especially comparing with the existing behavior?

IMO the most usual case is the user takes a picture and wants to share it using about 4/3 or 3/4 ratio. We need to optimize for this instead of trying to accomodate all the possible cases.

Comment 21

[Victoria Gerchinhoren [:vicky]]

(In reply to Julien Wajsberg [:julienw] (away until 5th May) from comment #20)
> (In reply to Victoria Gerchinhoren [:vicky] from comment #13)
> > We have discussed this issue with Omega an Steve, and the fixed container is
> > a not elegant solution for showing variable ratio images, so we should stick
> > to this spec and use option 3 for the extreme ratio cases:
> > https://www.dropbox.com/s/pv4w7k8nki8rnuo/
> > MMSPreviewThumbnailSpec_Landscape_2.0.png
> 
> Victoria, the image is not available anymore, can you put it somewhere?

> Sure: https://mozilla.box.com/s/0v50sxn1cweyr3z6apy1
> > 
> > If we set a fix ratio for images, we will all the time have some extra space
> > surrounding the image and that adds -information- noise to the layout.
> 
> I don't understand: I think that's what we do now. Can you be clearer about
> what is wrong in my proposal, especially comparing with the existing
> behavior?
> 
> IMO the most usual case is the user takes a picture and wants to share it
> using about 4/3 or 3/4 ratio. We need to optimize for this instead of trying
> to accomodate all the possible cases.

I think for this is better to have a skype chat or call. Can you reach me in @vixvixy tomorrow wednesday morning and we discuss this? Thanks!

Comment 22

[Julien Wajsberg [:julienw]]

Victoria, sorry, today has been a busy day. I'll catch you Friday if that's ok.

Comment 23

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

P1 for the perf improvement proposal.
It should work generally, not limited to specific device only.

Comment 24

[Julien Wajsberg [:julienw]]

I thought we already use the option 2 as described in comment 0, will need to check this.

Comment 25

[Julien Wajsberg [:julienw]]

Yes, I think that's been done in bug 983172.

Steve, can you double-check in case I missed something? And then dupe to bug 983172 if that's really the case?

Thanks !

Comment 26

[Steve Chung [:steveck]]

Yes we already implemented opt 2 in bug 98317. Close it as dup and thanks for the checking first!