946497 - [B2G][SMS][MMS] Thumbnail previews for cropped images in MMS message appear stretched causing not all of the image to be visible

Status: RESOLVED FIXED

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

Description

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - Thumbnails 1

Description:
Cropped images sent in MMS messages have thumbnail previews that do not show entirety of edited image. These stretched thumbnails appear on both senders and recipients phones.

Repro Steps:
1) Updated Buri to Build ID: 20131204115608
2) Open Messages app
3) Tap compose icon to begin new message
4) Enter recipient information
5) Select paperclip icon to attach media
6) Press gallery
7) Tap existing image
8) Drag crop selection so that only rightmost portion of image is selected and press "Done"
9) Send message

Actual:
Cropped image preview thumbnail is stretched and doesn't display entire image.

Expected:
Image preview thumbnails accurately display small versions of full sized image.

Environmental Variables
Device: Buri v 1.3 Mozilla
Build ID: 20131204115608
Gecko: http://hg.mozilla.org/mozilla-central/rev/526e12792fc8
Gaia: 324c467fc6b202fd09efe4b16cd83960fd2901eb
Platform Version: 28.0a1
Firmware Version: V1.2_US_20131115


Notes:
Repro frequency: 3/3, 100%
See attached: screenshots

Comment 1

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - Full 1

Comment 2

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - Thumbnails 2

Comment 3

[Brogan Zumwalt [Inactive]]

Attachment: Screenshot - Full 2

Comment 4

[Brogan Zumwalt [Inactive]]

Attachment: Comparison_Screenshot

This screenshot makes the bug more immediately apparent the previous screenshots

Comment 5

[Jason Smith [:jsmith]]

Does this reproduce on 1.1 or 1.2?

Comment 6

[Julien Wajsberg [:julienw]]

I think we haven't changed how we do this since it was first done, but maybe the Canvas behavior changed.

Comment 7

[Roland Kunkel [:RolandK]]

(In reply to Jason Smith [:jsmith] from comment #5)
> Does this reproduce on 1.1 or 1.2?

This reproduces for both the v1.1 and v1.2  builds on the Buri device.

Buri 1.2
Environmental Variables:
Device: Buri v1.2 Com RIL
BuildID: 20131205004003
Gaia: 0659f16b9790b1cf9eba4d80743fcc774d2ffe3a
Gecko: af2c7ebb5967
Version: 26.0
Firmware Version: V1.2_US_20131115

Buri 1.1
Environmental Variables:
Device: Buri v1.1 Com RIL
BuildID: 20131205041342
Gaia: 6ff3a607f873320d00cb036fa76117f6fadd010f
Gecko: 05117f42088f
Version: 18.0
Firmware Version: V1.2_US_20131115

Comment 8

[Julien Wajsberg [:julienw]]

It's somewhat reassuring to see this happens in 1.1.

Let's fix this in 1.4!

Comment 9

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

hi Kaze, what do you think of the effort / risk / eta to fix this bug? this will help the triage
 thanks

Comment 10

[Fabien Cazenave [:kaze]]

I expect this to be a rather easy and non-risky patch. If it gets an 1.4+ flag I should be able to land it next week.

Comment 11

[Noemí Freire (:noemi)]

Triage: not a blocker since we have already shipped with it but improving quality so it would be fine to have it as part of v1.4, leave it the reviewer to take a decision about landing on master. Thanks!

Comment 12

[Oleg Zasypkin [:azasypkin]]

Attachment: Pull request URL

Hey Fabien, while working on other bug I've noticed that problem and came up with one-liner solution. So this bug is more appropriate place for this patch.

Comment 13

[Julien Wajsberg [:julienw]]

Comment on attachment 8403356 [details] [review]
Pull request URL

Hey Oleg, Fabien is not a peer of the MMS app, so I'm redirecting the review to Steve who should know this part quite well.

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8403356 [details] [review]
Pull request URL

Hi Oleg, I think this solution is good but it seems safer to have a test for it. Could you give a try to write a test for panorama image, by spying CanvasRenderingContext2D.prototype.drawImage and verify the drawing size parameter is no bigger than attachment div size? Thanks.

Comment 15

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8403356 [details] [review]
Pull request URL

(In reply to Steve Chung [:steveck] from comment #14)
> Comment on attachment 8403356 [details] [review]
> Pull request URL
> 
> Hi Oleg, I think this solution is good but it seems safer to have a test for
> it. Could you give a try to write a test for panorama image, by spying
> CanvasRenderingContext2D.prototype.drawImage and verify the drawing size
> parameter is no bigger than attachment div size? Thanks.

Thanks for review! I've noticed that there was no any specific test for 'getThumbnails' method, so I added several to test cases that I can think of. If you know more cases that we can test - let me know, I'd be happy to add more tests.

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8403356 [details] [review]
Pull request URL

Only one nit so r=me, thanks for these great test cases!

Comment 17

[Oleg Zasypkin [:azasypkin]]

(In reply to Steve Chung [:steveck] from comment #16)
> Comment on attachment 8403356 [details] [review]
> Pull request URL
> 
> Only one nit so r=me, thanks for these great test cases!

Thanks for review! I've updated PR with the nit you suggested and also rebased it. Could you please help to land it as I don't have enough rights yet to do it by myself?

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/70f9c4e784d0e003886a6b898f79506b2046f4db

Comment 19

[Steve Chung [:steveck]]

(In reply to Oleg Zasypkin [:azasypkin] from comment #17)
> (In reply to Steve Chung [:steveck] from comment #16)
> > Comment on attachment 8403356 [details] [review]
> > Pull request URL
> > 
> > Only one nit so r=me, thanks for these great test cases!
> 
> Thanks for review! I've updated PR with the nit you suggested and also
> rebased it. Could you please help to land it as I don't have enough rights
> yet to do it by myself?

Oh Sorry I thought you've got the permission to land in master :( I'll remember that next time.

Comment 20

[Julien Wajsberg [:julienw]]

The "checkin-needed" magic keyword works fine too ;) Thanks Ryan !

Comment 21

[Oleg Zasypkin [:azasypkin]]

(In reply to Julien Wajsberg [:julienw] from comment #20)
> The "checkin-needed" magic keyword works fine too ;) Thanks Ryan !

Yay, I thought it only works for mozilla-central repo :) Thanks!