Closed Bug 972245 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox OS Graveyard :: Gaia, defect)

defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.2 wontfix, b2g-v1.3 verified, b2g-v1.3T verified, b2g-v1.4 verified)

VERIFIED FIXED
blocking-b2g 1.3+
Tracking Status
b2g-v1.2 --- wontfix
b2g-v1.3 --- verified
b2g-v1.3T --- verified
b2g-v1.4 --- verified

People

(Reporter: jmcf, Assigned: steveck)

References

Details

(Keywords: regression)

Attachments

(5 files, 6 obsolete files)

52.03 KB, image/png
Details
191 bytes, text/html
crdlc
: review+
Details
26.61 KB, image/png
Details
3.05 KB, patch
etienne
: review+
Details | Diff | Splinter Review
54 bytes, text/plain
steveck
: review+
Details
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.
Attached image 2014-02-13-09-58-56.png
Assignee: nobody → jmcf
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.
we wouldn't block 1.3 for this bug but we must fixe it in 1.4
blocking-b2g: 1.3? → 1.4?
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.
Summary: Photos appear distorted in the call log → Facebook Photos appear distorted in the call log
blocking-b2g: 1.4? → 1.3?
reducing the scope of the bug and renominating
Attached file 16245.html
Attachment #8375430 - Flags: review?(crdlc)
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.
(In reply to Etienne Segonzac (:etienne) from comment #7)
> There is absolutely no reason to apply the building block
> strategy everywhere.

*not* to apply
(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?
Attachment #8375430 - Flags: review?(crdlc) → review+
LGTM, there are some comments, but minor changes
(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.
(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.
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?
(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.
Attached patch bug972245.calllog.patch (obsolete) — Splinter Review
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 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 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.
Attachment #8375502 - Flags: review+
(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
(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 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
Attached patch bug972245.calllog.patch (obsolete) — Splinter Review
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.
Attachment #8375502 - Attachment is obsolete: true
Attachment #8375534 - Flags: review?(etienne)
Summary: Facebook Photos appear distorted in the call log → Photos can appear distorted in the call log and in the SMS app
Assignee: jmcf → 21
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.
Moving the component to Gaia to reflect the fact that it touch a few more things that just the call log.
Component: Gaia::Dialer → Gaia
OS: Mac OS X → All
Hardware: x86 → All
(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
blocking on this for 1.3
blocking-b2g: 1.3? → 1.3+
Attached patch bug972245.messages.patch (obsolete) — Splinter Review
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.
Attachment #8375789 - Flags: feedback?(felash)
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 on attachment 8375789 [details] [diff] [review]
bug972245.messages.patch

Forwarding the feedback request to Steve.
Attachment #8375789 - Flags: feedback?(felash) → feedback?(schung)
For context, this regressed with bug 967272.
Blocks: 967272
Keywords: regression
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 :)
Attachment #8375534 - Flags: review?(etienne)
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.
Attachment #8375789 - Flags: feedback?(schung)
(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)?
(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 :)
(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.
And Travis is green!
Attachment #8375534 - Attachment is obsolete: true
Attachment #8376286 - Flags: review?(etienne)
Attached patch bug972245.messages.patch (obsolete) — Splinter Review
Here is the messages patch I sent to Travis.
Attachment #8375789 - Attachment is obsolete: true
Comment on attachment 8376286 [details] [diff] [review]
bug972245.calllog.patch

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

All good :)
Attachment #8376286 - Flags: review?(etienne) → review+
(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
Attached patch bug972245.messages.patch (obsolete) — Splinter Review
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.
Attachment #8376318 - Attachment is obsolete: true
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.
Assignee: 21 → nobody
take the message part.
Assignee: nobody → schung
Attached file Link to github (obsolete) —
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!
Attachment #8376973 - Flags: review?(felash)
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.
Attachment #8376973 - Flags: review?(felash) → review+
Suggestion applied and landed in master: 7b37543888457b6cdf8cb8dc766bcd6fd3ba383f
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
Attachment #8376973 - Flags: approval-gaia-v1.3?(fabrice)
Steve, I think you changed the v1.3 status to "wontfix" by mistake.
(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 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.
Attachment #8376973 - Flags: approval-gaia-v1.3?(fabrice)
Revert on master: a5b3110d25510a952417ba98a502250f374ef210

Thanks to Tim Pillard [1] :)
[1] https://twitter.com/tpillard/status/435755817220849664
Attached file github PR
Attachment #8376973 - Attachment is obsolete: true
Attachment #8377557 - Flags: review?(schung)
Attachment #8376415 - Attachment is obsolete: true
Steve, please land yourself and request approval if this looks fine.
Comment on attachment 8377557 [details]
github PR

Thanks for discovering the bug, let's wait for tree reopen.
Attachment #8377557 - Flags: review?(schung) → review+
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
Attachment #8377557 - Flags: approval-gaia-v1.3?(fabrice)
Keywords: verifyme
Adding verifyme for QA to help test once this lands on master.
Steve,

This will need Fabrice's approval to land on 1.3
Flags: needinfo?(schung)
approval has been already asked to Fabrice.

I'm landing on master
master: b9a5859551ffbe17884de3343f82fc3d98233ec8
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(schung)
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Attachment #8377557 - Flags: approval-gaia-v1.3?(fabrice) → approval-gaia-v1.3+
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
Flags: needinfo?(schung)
call log part land in v1.3 : 6e883bde818d4d53aef7b5b075b4b34267918360
Flags: needinfo?(schung)
Please note we also need to uplift b9a5859551ffbe17884de3343f82fc3d98233ec8(message) for closing this bug, thanks!
Flags: needinfo?(jhford)
v1.3: 3da675a68d2e4592e0df4a6f470b658f808f7058
Flags: needinfo?(jhford)
Depends on: 986465
Depends on: 994553
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
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
You need to log in before you can comment on or make changes to this bug.