972245 - Photos can appear distorted in the call log and in the SMS app

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect)

Description

[Jose Manuel Cantera]

Since landing the thumbnail bug the photos in the call log appear distorted. see attachment. This should be blocking as it is a clear regression.

Comment 1

[Jose Manuel Cantera]

Attachment: 2014-02-13-09-58-56.png

Comment 2

[Etienne Segonzac (:etienne)]

We should match what we do in the contacts app, use a div with a background image and background-size cover.
It will also prevent some reflows and help APZC.

Comment 3

[David Scravaglieri [:scravag]]

we wouldn't block 1.3 for this bug but we must fixe it in 1.4

Comment 4

[Jose Manuel Cantera]

I'm proposing a simple patch for the FB imported contacts, making the thumbnail square as we do with the whole photo. This fixes the problem and I strongly believe this should be in 1.3. as we are regressing even from 1.1. 

For v14 the call log owners can implement what Etienne is suggesting.

Comment 5

[Jose Manuel Cantera]

reducing the scope of the bug and renominating

Comment 6

[Jose Manuel Cantera]

Attachment: 16245.html

Comment 7

[Etienne Segonzac (:etienne)]

So basically all the time spent discussing on bug 967272 was for nothing?

We don't need to square images in the database, we just need to display them properly, we have the CSS feature for this, this is what we do in the building block. There is absolutely no reason to apply the building block strategy everywhere.

Comment 8

[Etienne Segonzac (:etienne)]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> There is absolutely no reason to apply the building block
> strategy everywhere.

*not* to apply

Comment 9

[Etienne Segonzac (:etienne)]

(In reply to Jose M. Cantera from comment #6)
> Created attachment 8375430 [details]
> 16245.html

Also why aren't you asking a Contacts app peer for the review?

Comment 10

[Cristian Rodriguez (:crdlc)]

LGTM, there are some comments, but minor changes

Comment 11

[Jose Manuel Cantera]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> So basically all the time spent discussing on bug 967272 was for nothing?
> 
> We don't need to square images in the database, we just need to display them
> properly, we have the CSS feature for this, this is what we do in the
> building block. There is absolutely no reason to apply the building block
> strategy everywhere.

I don't disagree and it is what should be done for v1.4, but for the moment I consider less risky to do what I'm doing than changing call log, and all the apps that might have the same problem. 

On the other hand I'm totally disappointed that patches like thumbnails break work that was done years ago and on the other hand (with my end-user hat on) make the phone appear as a low quality thing only for the sake of saving a few milisecs.

Comment 12

[Jose Manuel Cantera]

(In reply to Etienne Segonzac (:etienne) from comment #9)
> (In reply to Jose M. Cantera from comment #6)
> > Created attachment 8375430 [details]
> > 16245.html
> 
> Also why aren't you asking a Contacts app peer for the review?

Cristian is the usual reviewer of the FB integration thing and is more familiar than contacts peer in that code. Also he developed the square image algorithm.

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Hi folks,

I though we did agree in bug 967272 about not to square images:

https://bugzilla.mozilla.org/show_bug.cgi?id=967272#c47

@Jose, is there any technical reason we are missing to need to square the FB images?

Comment 14

[Jose Manuel Cantera]

(In reply to Francisco Jordano [:arcturus] from comment #13)
> Hi folks,
> 
> I though we did agree in bug 967272 about not to square images:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=967272#c47
> 
> @Jose, is there any technical reason we are missing to need to square the FB
> images?

if you read the bug comments you can see them.

Comment 15

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.calllog.patch

I made a small patch that change that makes a minimal change to the call log logic.

Jose does it solve the issue for you ?

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> Created attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> I made a small patch that change that makes a minimal change to the call log
> logic.
> 
> Jose does it solve the issue for you ?

Thinking about it we should probably revoke the blob too once it is loaded (which is independent of any of the 2 solutions).

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 8375502 [details] [diff] [review]
bug972245.calllog.patch

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

Vivien's patch is shorter, it fixes the issue for all the contacts (not just facebook), and has better performances (when rendering the call log and when importing contacts since we save ourselves a costly canvas operation).

I think we should take this one.

Comment 18

[Jose Manuel Cantera]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #15)
> Created attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> I made a small patch that change that makes a minimal change to the call log
> logic.
> 
> Jose does it solve the issue for you ?

In the call log yes, but we have the same issue in the SMS app (see attached screenshot). That's why I was proposing my patch, but if we can fix it in the SMS app as well, I would be happy. 

many thanks

Comment 19

[Jose Manuel Cantera]

Attachment: same issue in the SMS app

Comment 20

[Jose Manuel Cantera]

(In reply to Etienne Segonzac (:etienne) from comment #17)
> Comment on attachment 8375502 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8375502 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Vivien's patch is shorter, it fixes the issue for all the contacts (not just
> facebook), and has better performances (when rendering the call log and when
> importing contacts since we save ourselves a costly canvas operation).
> 
> I think we should take this one.

I agree, provided we can fix also the issue in the SMS app

Comment 21

[Jose Manuel Cantera]

Comment on attachment 8375502 [details] [diff] [review]
bug972245.calllog.patch

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

::: apps/communications/dialer/js/call_log.js
@@ +378,4 @@
>    //      <span></span>
>    //    </label>
>    //    <aside class="pack-end">
> +  //      <span data-type="img" src="" class="call-log-contact-photo">

nit: src is no longer valid

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.calllog.patch

Sorry Etienne the last patch was not really intended to be reviewed. This one is a bit longer but it remove an extra variable but also revoke blobs urls once they have been used. I'm pretty sure Tarako will love this extra attention.

Comment 23

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

More generally and this is a bit out of the scope of this bug but a better cropping algorithm, even if the current is smart in the sense that it relies on the fact that most people center the person on the picture and exploit that.

There is some code around to detect faces on a picture using JavaScript. It was not very fast on a desktop machine but that was 3 and a half years ago. I wonder if we can revive this code, use 'asm.js' and a worker, as well as fixing the contacts API in order to have some metadata to the photo fields. That will let us take a normal thumbnail and center it using background position using the metadata returns by this algorithm.

Jose let me know if you are interested and I can probably do some archeology in some of my old laptops to find it.

Comment 24

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Moving the component to Gaia to reflect the fact that it touch a few more things that just the call log.

Comment 25

[Jose Manuel Cantera]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #23)
> More generally and this is a bit out of the scope of this bug but a better
> cropping algorithm, even if the current is smart in the sense that it relies
> on the fact that most people center the person on the picture and exploit
> that.
> 
> There is some code around to detect faces on a picture using JavaScript. It
> was not very fast on a desktop machine but that was 3 and a half years ago.
> I wonder if we can revive this code, use 'asm.js' and a worker, as well as
> fixing the contacts API in order to have some metadata to the photo fields.
> That will let us take a normal thumbnail and center it using background
> position using the metadata returns by this algorithm.
> 
> Jose let me know if you are interested and I can probably do some archeology
> in some of my old laptops to find it.

Oh yes, that can be exploited for the future visual refresh for Contacts in which photos will put in a circle

Comment 26

[Preeti Raghunath(:Preeti)]

blocking on this for 1.3

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.messages.patch

Julien I'm not sure I cover all the code paths you showned me earlier. Asking f? in case you got a chance to look at that before I see you at the office tomorrow.

Comment 28

[Julien Wajsberg [:julienw]]

Yeah I won't look at this today, too tired, sorry, and tomorrow I'm in PTO. So you should ask Steve who can probably look at this while you'll be sleeping.

For Steve, I think the code paths that handle contact pictures are in (possibly) ThreadUI, ContactRenderer, ThreadListUI, Utils, (possibly) GroupView. I think this is more certainly ContactRenderer, ThreadListUI and Utils.

Comment 29

[Julien Wajsberg [:julienw]]

Comment on attachment 8375789 [details] [diff] [review]
bug972245.messages.patch

Forwarding the feedback request to Steve.

Comment 30

[Julien Wajsberg [:julienw]]

For context, this regressed with bug 967272.

Comment 31

[Etienne Segonzac (:etienne)]

Comment on attachment 8375534 [details] [diff] [review]
bug972245.calllog.patch

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

See comments, probably need a second round.

::: apps/communications/dialer/js/call_log.js
@@ +963,5 @@
> +      // Revoke the blob once it's ready.
> +      var image = new Image();
> +      image.src = url;
> +      image.onload = image.onerror = function() {
> +        URL.revokeObjectURL(url);

from my on-device testing I'm not sure this is working...
(don't see the picture, but if I remove the revoke it works)

@@ +968,5 @@
> +      };
> +    }
> +  },
> +
> +  unloadBackgrounImage: function cl_unloadBackgroundImage(element) {

I think you're missing a 'd'.
The tests would have told you so :)

Comment 32

[Steve Chung [:steveck]]

Comment on attachment 8375789 [details] [diff] [review]
bug972245.messages.patch

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

There is one img style need to be applied for new structure: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L1006.
and here https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L193

That's what I found currently, I will spend more time if there exist any image styling need to change or remove.

::: apps/sms/js/contact_renderer.js
@@ +246,4 @@
>        target.appendChild(element);
>  
>        // Revoke contact photo after image onload.
> +      var photo = element.querySelector('span[data-type=img"]');

A quotation mark is missed here.

Comment 33

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #28)
> For Steve, I think the code paths that handle contact pictures are in
> (possibly) ThreadUI, ContactRenderer, ThreadListUI, Utils, (possibly)
> GroupView. I think this is more certainly ContactRenderer, ThreadListUI and
> Utils.

I could only found some image related issue in ThreadListUI/ContactRenderer, are you sure Utils also has the image issue(we only use image for resizing only)?

Comment 34

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #31)
> Comment on attachment 8375534 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8375534 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See comments, probably need a second round.
> 
> ::: apps/communications/dialer/js/call_log.js
> @@ +963,5 @@
> > +      // Revoke the blob once it's ready.
> > +      var image = new Image();
> > +      image.src = url;
> > +      image.onload = image.onerror = function() {
> > +        URL.revokeObjectURL(url);
> 
> from my on-device testing I'm not sure this is working...
> (don't see the picture, but if I remove the revoke it works)
> 

Works for me but I believe it can be racy since there is simple way to revoke a blob for a background image. I guess I can be dirty and use a setTimeout :/

> @@ +968,5 @@
> > +      };
> > +    }
> > +  },
> > +
> > +  unloadBackgrounImage: function cl_unloadBackgroundImage(element) {
> 
> I think you're missing a 'd'.
> The tests would have told you so :)

I changed the name at the last minute :)

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Steve Chung [:steveck] from comment #32)
> Comment on attachment 8375789 [details] [diff] [review]
> bug972245.messages.patch
> 
> Review of attachment 8375789 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is one img style need to be applied for new structure:
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L1006.
> and here
> https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/style/sms.css#L193
> 
> That's what I found currently, I will spend more time if there exist any
> image styling need to change or remove.
> 

Thanks for the references. That's what I was looking for.

Comment 36

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.calllog.patch

And Travis is green!

Comment 37

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.messages.patch

Here is the messages patch I sent to Travis.

Comment 38

[Etienne Segonzac (:etienne)]

Comment on attachment 8376286 [details] [diff] [review]
bug972245.calllog.patch

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

All good :)

Comment 39

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #38)
> Comment on attachment 8376286 [details] [diff] [review]
> bug972245.calllog.patch
> 
> Review of attachment 8376286 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> All good :)

Call log part landed.

https://github.com/mozilla-b2g/gaia/commit/a921e33b583e87402fec2b369ecca737b880a90b

Comment 40

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug972245.messages.patch

This is still red on Travis. As it is also unlikely that I can get a review and advices before tomorrow morning, where I will be off for one week, someone else would have to finish this patch on the sms side.

Comment 41

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Unassigning myself as someone will likely have to finish the sms related part of this patch while I will be in some cold part of the world without any laptop.

Comment 42

[Steve Chung [:steveck]]

take the message part.

Comment 43

[Steve Chung [:steveck]]

Attachment: Link to github

Hi Julien, this patch is based on Vivien's patch with addition style and test fixing. It seems fine on current build, but we still need your eagle eyes to verify this patch is safe on master/v1.3. Thanks!

Comment 44

[Julien Wajsberg [:julienw]]

Comment on attachment 8376973 [details] [review]
Link to github

r=me with a nit to extract the common code in a function.

makes me quite sad though, will try to find a better solution with platform team.

Comment 45

[Steve Chung [:steveck]]

Suggestion applied and landed in master: 7b37543888457b6cdf8cb8dc766bcd6fd3ba383f

Comment 46

[Steve Chung [:steveck]]

Comment on attachment 8376973 [details] [review]
Link to github

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):bug 972245
[User impact] if declined: Contacts thumbnails will be distorted
[Testing completed]:manual
[Risk to taking this patch] (and alternatives if risky):low risk ,no alternatives
[String changes made]: none

Comment 47

[Julien Wajsberg [:julienw]]

Steve, I think you changed the v1.3 status to "wontfix" by mistake.

Comment 48

[Steve Chung [:steveck]]

(In reply to Julien Wajsberg [:julienw] from comment #47)
> Steve, I think you changed the v1.3 status to "wontfix" by mistake.

Oh! thanks for the reminder

Comment 49

[Julien Wajsberg [:julienw]]

Comment on attachment 8376973 [details] [review]
Link to github

Removing the approval request because the patch has a bug. Gonna revert, steal, and ask review again.

Comment 50

[Julien Wajsberg [:julienw]]

Revert on master: a5b3110d25510a952417ba98a502250f374ef210

Thanks to Tim Pillard [1] :)
[1] https://twitter.com/tpillard/status/435755817220849664

Comment 51

[Julien Wajsberg [:julienw]]

Attachment: github PR

Comment 52

[Julien Wajsberg [:julienw]]

Steve, please land yourself and request approval if this looks fine.

Comment 53

[Steve Chung [:steveck]]

Comment on attachment 8377557 [details]
github PR

Thanks for discovering the bug, let's wait for tree reopen.

Comment 54

[Steve Chung [:steveck]]

Comment on attachment 8377557 [details]
github PR

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):bug 972245
[User impact] if declined: Contacts thumbnails will be distorted
[Testing completed]:manual
[Risk to taking this patch] (and alternatives if risky):low risk ,no alternatives
[String changes made]: none

Comment 55

[bhavana bajaj [:bajaj]]

Adding verifyme for QA to help test once this lands on master.

Comment 56

[Preeti Raghunath(:Preeti)]

Steve,

This will need Fabrice's approval to land on 1.3

Comment 57

[Julien Wajsberg [:julienw]]

approval has been already asked to Fabrice.

I'm landing on master
master: b9a5859551ffbe17884de3343f82fc3d98233ec8

Comment 58

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b4e3bf7c62b5

Comment 59

[Salem Elkabule]

This issue is fixed on buri v 1.4 Mozilla RIL

Environmental Variables
Device: Buri 1.4 MOZ RIL
Build ID: 20140224040201
Gecko: https://hg.mozilla.org/mozilla-central/rev/31113754db3b
Gaia: ffb527b84594396ed611edf0a8a5a130d60a742f
Platform Version: 30.0a1
Firmware Version: V1.2-device.cfg

The Images display correctly in the Call log and the Message application.

Comment 60

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

I was not able to uplift this bug to v1.3.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1.3, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1.3
  git cherry-pick -x -m1 a921e33b583e87402fec2b369ecca737b880a90b
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 61

[Steve Chung [:steveck]]

call log part land in v1.3 : 6e883bde818d4d53aef7b5b075b4b34267918360

Comment 62

[Steve Chung [:steveck]]

Please note we also need to uplift b9a5859551ffbe17884de3343f82fc3d98233ec8(message) for closing this bug, thanks!

Comment 63

[Ryan VanderMeulen [:RyanVM]]

v1.3: 3da675a68d2e4592e0df4a6f470b658f808f7058

Comment 64

[Mike Livingston]

Photos appear correctly without distortion in 1.3 and 1.4

1.3 Environmental Variables:
Device: Buri 1.3 MOZ
BuildID: 20140525024003
Gaia: 0ce948e378cab7ed3db20231281dd7ca2eb99779
Gecko: 94e367cf6c94
Version: 28.0
Firmware Version: v1.2-device.cfg

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg

Comment 65

[Montoya Clemmons (MClemmons)]

Photos appear non-distorted on latest 1.3Tarako build.
1.3T Environmental Variables:
Device: Tarako 1.3T
BuildID: 20140530014002
Gaia: e68858693b71d917c9c5ee7e215f7ceea04635f7
Gecko: 1945abae19ff
Version: 28.1
Firmware Version: sp6821a-gonk-4.0-5-12
User Agent: Mozilla/5.0 (Mobile; rv:28.1) Gecko/28.1 Firefox/28.1