885584 - [B2G][SMS/MMS] Thumbnails are invisible in the preview attachment with large files

Status: RESOLVED FIXED

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

Description

[sarsenyev]

Attachment: video_attachment

Description:
After attaching 3 large(more than 1mb) picture files to "MMS", the attached files are invisible in thumbnail view

Repro Steps:
1) Updated to Buri/Inari/Leo/Unagi Build ID: 20130610070206
2) Open "SMS" app from the home menu
3) Tap on a new message (paper and pen) icon
4) Tap on "Clip" icon in the left corner
5) Choose: 
6) Select from "Gallery"
7) Choose a picture after cropping (checkbox) icon
8) Repeat steps 3-7 two more times until the three files appear in the attachment

Actual:
Thumbnails are invisible in the preview attachment

Expected:
A user can see all pictures in preview mode.

Environmental Variables
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/8e3f39363c54
Gaia: ce3b99781d182ad550a325206990c249b0dbcf0e
Platform Version: 18.0

Notes:
Repro frequency: 100%
Q Analysts Team Priority: (Pri Level 2)
See the video attachment

See the attachment for video

Comment 1

[Alex Keybl [:akeybl]]

Adding qawanted to find out if this is caused by

1) Large images (as in images not taken with the phone's camera)
2) Multiple images being attached (how many?)
3) A combination of the two

Comment 2

[sarsenyev]

1) Yes, images are not taken with the camera, just transferred from PC
2) Three images being attached (see the video attachment)
3) Combination of the three

Comment 3

[Peter Dolanjski [:pdol]]

Kev/Maria, could you clarify what the expectation is for this scenario?

Comment 4

[Borja Salguero [:borjasalguero]]

Corey, do you know what is happening here? Is related with the size of the files? Thanks!

Comment 5

[Julien Wajsberg [:julienw]]

This is expected, we've put an hard limit of 400kB for generating thumbnails, because we don't want the app to crash.

But soon (bug 873758) we should recompress images before adding them as attachment, and the goal size is 300kB for this, which is less than 400kB (obviously ;) ), and therefore this should just work.

Comment 6

[Julien Wajsberg [:julienw]]

maybe the 400kB limit should be put in a setting like the other values ? Or be somewhow calculated like "size limit" * 1.2" ?

Comment 7

[Borja Salguero [:borjasalguero]]

Gabriele, Is this the bug you are working on? I've seen that is not assigned and I dont know if is the same. Thanks!

Comment 8

[Alex Keybl [:akeybl]]

Already blocking on bug 873758 and comment 6 doesn't seem critical for v1.1.

Comment 9

[Corey Frang [:gnarf]]

Alex - This becomes critical if the operator defined limit is say 1mb, 400k is easily one image and therefore won't have any previews.

operatorLimit * 1.2 makes sense to me julien, I'll write a quick patch for this hoping that it gets leo+'ed

Comment 10

[Corey Frang [:gnarf]]

Attachment: patch v1

Uses Settings.mmsSizeLimitation to determine when to render a thumbnail

Comment 11

[Corey Frang [:gnarf]]

I'm pretty sure this is not the bug that gsvelto is working on so I'm canceling the ni?

Comment 12

[Julien Wajsberg [:julienw]]

Comment on attachment 767552 [details] [diff] [review]
patch v1

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

looks good !

some requested changes still ;)

::: apps/sms/js/attachment.js
@@ +163,5 @@
>        // We special case audio to display an image of an audio attachment video
>        // currently falls through this path too, we should revisit this with
>        // Bug 869244 - [MMS] 'Thumbnail'/'Poster' in video attachment is needed.
> +      if (type === 'img' && (!Settings.mmsSizeLimitation ||
> +          this.size < Settings.mmsSizeLimitation * 1.2)) {

I'd say we should keep the MAX_THUMBNAIL_GENERATION_SIZE in case we don't have Settings.mmsSizeLimitation. Maybe rename that constant.

I also still think we should still have a sensible absolute maximum, but I guess this should be configurable per device too (because it's probably "available memory dependent"), so we might have this in another bug.

::: apps/sms/test/unit/attachment_test.js
@@ +113,5 @@
>        name: 'Image attachment'
>      });
>      var el = attachment.render(function() {
>        assert.ok(el.src, 'src set');
>        assert.include(el.classList, 'attachment');

I know newer chaijs don't like using include with classList (and that's part of why we can't upgrade to the newer chaijs). Can you use "assert.ok(el.classList.contains(...))" instead ? Its not less readable anyway.

@@ +129,5 @@
> +      name: 'Image attachment'
> +    });
> +    var el = attachment.render(function() {
> +      assert.ok(el.src, 'src set');
> +      assert.include(el.classList, 'attachment');

same here

Comment 13

[Wayne Chang [:wchang]]

triage - agreed to leo+ this per the user impact.

Comment 14

[Corey Frang [:gnarf]]

Attachment: patch v2

* Sets max thumbnail generation size to 1.5 megs, which should be big enough for any mms image (instead of using max mms size)
* Removes a var self = this;

Comment 15

[Julien Wajsberg [:julienw]]

Comment on attachment 768125 [details] [diff] [review]
patch v2

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

r=me

Comment 16

[Corey Frang [:gnarf]]

master: https://github.com/mozilla-b2g/gaia/commit/b17b09bf8526be355e2e56a31942c6d9b9d3adab
v1-train: https://github.com/mozilla-b2g/gaia/commit/508cfc8ca95cc5dbfde8dbe8d153ab9653c6ab84

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3b25e3842019

Comment 18

[John Ford [:jhford] CET/CEST Berlin Time]

v1.1.0hd: 508cfc8ca95cc5dbfde8dbe8d153ab9653c6ab84

Comment 19

[dkumar]

Varified,fixed on leo 1.1 Mozilla RIL 

Environmental Variables
Build ID: 20130717070237
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/582e3a7018b0
Gaia: c506c50adaaebcf729ac3c27887ba2931ab79040
Platform Version: 18.1

Issue does not reproduce user can see all pictures in preview mode.