Closed Bug 967272 Opened 10 years ago Closed 10 years ago

Contacts spends to much time decoding image: Should be using thumbnails

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

RESOLVED FIXED
1.4 S1 (14feb)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed

People

(Reporter: BenWa, Assigned: etienne)

References

Details

(Keywords: perf, Whiteboard: [c=effect p= s= u=1.3])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
julienw
: review+
rik
: review+
arcturus
: review+
jmcf
: feedback+
Details | Review
Profile:
http://people.mozilla.org/~bgirard/cleopatra/#report=a3ab371e7a8663d3f94e61ed172b2b213684fa17&search=blob

The above profile with the filter shows the extra time decoding. We need to store thumbnail instead of decoding the full image to downscale it.
Keywords: perf
blocking-b2g: --- → 1.3?
David, how are we doing those images in the contacts app?  Are we keeping the original images, and if so, are we decoding the full size or looking at the EXIF thumbnail (if there)?  Or do we scale it the first time and then keep the scaled down version around?
Flags: needinfo?(dflanagan)
The image source are 256x256 from the test workload.

I took a image from the camera and I see that we're using a 320x320 image. These aren't massive but they're 60x60 on the screen so we're loosing a ton of performance here.
Benoit, what's the end-user impact and frequency of them encountering its effect?
Flags: needinfo?(bgirard)
Whiteboard: [c= p= s= u=]
The end impact is scrolling the contacts list means the thumbnail take ~5 times longer to show up. This can make the difference between having them show up while you're scrolling your contacts or having to stop panning and wait for the contact photos to show up.
Flags: needinfo?(bgirard)
Outside of the images showing up "later", is there an impact on the text part checkerboarding because of this?
(In reply to Milan Sreckovic [:milan] from comment #5)
> Outside of the images showing up "later", is there an impact on the text
> part checkerboarding because of this?

Etienne is working on this. We already discussed how to do that and to solve it for call log, sms and contacts where I assume the issue is the same.
Thanks Benoit. I'd consider the text checkerboarding a blocker but not the image issue of needing to stop panning.

Benoit & Vivien,
Do both issues need to be handled at the same time or can we split them into separate bugs so we can block on only the checkerboarding piece?
Flags: needinfo?(bgirard)
(In reply to Milan Sreckovic [:milan] from comment #5)
> Outside of the images showing up "later", is there an impact on the text
> part checkerboarding because of this?

Yes, we can delay it by an extra ~5ms here and there. But fixing this and tweaking how we decode means we could load the thumbnails as we're scrolling with minimal scrolling impact giving a much better user experience.

(In reply to Mike Lee [:mlee] from comment #7)

It's all the same cause that the thumbnails are too big. I don't see a great way to break it down further.

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)

Awesome!
Flags: needinfo?(bgirard)
triage: 1.3+ blocks a blocker
blocking-b2g: 1.3? → 1.3+
(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> (In reply to Milan Sreckovic [:milan] from comment #5)
> > Outside of the images showing up "later", is there an impact on the text
> > part checkerboarding because of this?
> 
> Etienne is working on this. We already discussed how to do that and to solve
> it for call log, sms and contacts where I assume the issue is the same.

Yep, experimenting with saving a thumbnail when we save a contact picture.
Should have a solid proof of concept by the end of the day.

(https://github.com/etiennesegonzac/gaia/tree/contact-thumbnails)
Assignee: nobody → etienne
(In reply to Etienne Segonzac (:etienne) from comment #10)
> (In reply to Vivien Nicolas (:vingtetun) (:21) from comment #6)
> > (In reply to Milan Sreckovic [:milan] from comment #5)
> > > Outside of the images showing up "later", is there an impact on the text
> > > part checkerboarding because of this?
> > 
> > Etienne is working on this. We already discussed how to do that and to solve
> > it for call log, sms and contacts where I assume the issue is the same.
> 
> Yep, experimenting with saving a thumbnail when we save a contact picture.
> Should have a solid proof of concept by the end of the day.
> 
> (https://github.com/etiennesegonzac/gaia/tree/contact-thumbnails)

Nice, it's looking awesome, a pitty that we don't have a thumbnail field on the api, but the photo array will do the trick.

Thanks a lot Etienne!
Progress report: it's working, dialer, contacts and sms are updated, all tests are green!

https://github.com/etiennesegonzac/gaia/compare/contact-thumbnails?expand=1

Still need to add tests, but this is really to be tested/profiled.
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P1
Hardware: x86 → ARM
Whiteboard: [c= p= s= u=] → [c=effect p= s= u=1.3]
Target Milestone: --- → 1.4 S1 (14feb)
Attached file Pointer to gaia PR
Here we go!

This patch:
* generates thumbnails for new contacts
* introduces a new ContactPhotoHelper
* updates all the comms app to use this helper

Tried to optimize for upliftability without sacrificing the test coverage :)

r? Arcturus for the Contacts part (biggest one obviously)
r? Rik for the dialer part
r? Julien for the sms part 

We should maybe open a bug to track a possible migration of all the old contact pictures but not sure the risk/reward ratio looks good.
Attachment #8370836 - Flags: review?(francisco.jordano)
Attachment #8370836 - Flags: review?(felash)
Attachment #8370836 - Flags: review?(anthony)
Notes:

* would be cool to have this patch profiled to see how much it helps, but since the reference workload contacts don't have thumbnails it will require some manual contact entry...

* the current version of the patch should uplift to 1.3 pretty easily (conflicts are only in test files and pretty straightforward).
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

Ok, this is great, r=me
I added some small nits but none are mandatory.

As discussed, we need to know whether the photos from Facebook are also Blob. I think they are but Francisco or José will know better. If they're Blob, then we can remove the legacy data-url code (but maybe in another patch that will land in master only, so that we don't risk a regression).
Attachment #8370836 - Flags: review?(felash) → review+
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

We should open a bug to update the reference-workload to include a thumbnail.

Two comments in the pull request. They should be addressed but no need to ask for another review.
Attachment #8370836 - Flags: review?(anthony) → review+
(First rounds of comments addressed, no more sadpanda)
Blocks: 968372
Clearing the needinfo. Looks like this has been completely handled without me.
Flags: needinfo?(dflanagan)
Review ping
Flags: needinfo?(francisco.jordano)
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

Left some comments in github, we are almost there!

Thanks a lot for the hard work Etienne!
Attachment #8370836 - Flags: review?(francisco.jordano) → feedback?(jmcf)
Flags: needinfo?(francisco.jordano)
Flagging Jose for feedback just in case I missed some of the Fb stuff.
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

I don't like the changes made on the contacts merger module. I wouls suggest you revert those and only change what is needed for this patch and do not change parts of the code that have nothing to do with this patch.
Attachment #8370836 - Flags: feedback?(jmcf) → feedback-
and as a last comment I don't like how the problem has been approached. This patch should have been divided in smaller patches, one to save the contact thumbnail and n number of patches to align that new data model with the rest of affected modules, in contacts and in the rest of apps. The regression risk of this huge patch is high and the revert of it can be painful.
(In reply to Jose M. Cantera from comment #22)
> Comment on attachment 8370836 [details] [review]
> Pointer to gaia PR
> 
> I don't like the changes made on the contacts merger module. I wouls suggest
> you revert those and only change what is needed for this patch and do not
> change parts of the code that have nothing to do with this patch.

Can you be more specific?
The thumbnail generation is async, so I have to *move* (not change) some code in its callback. What other solution are you suggesting.

And as a side note, I'd really like some feedback on how to implement this in the facebook-related code paths.
Right, the diff viewer is not helping there, it almost happen to me as well. But as commented before is just moving code.
Might be easier to read with the the w=1 option to ignore whitespace changes:

  https://github.com/mozilla-b2g/gaia/pull/15990/files?w=1
(In reply to Etienne Segonzac (:etienne) from comment #24)
> (In reply to Jose M. Cantera from comment #22)
> > Comment on attachment 8370836 [details] [review]
> > Pointer to gaia PR
> > 
> > I don't like the changes made on the contacts merger module. I wouls suggest
> > you revert those and only change what is needed for this patch and do not
> > change parts of the code that have nothing to do with this patch.
> 
> Can you be more specific?
> The thumbnail generation is async, so I have to *move* (not change) some
> code in its callback. What other solution are you suggesting.
> 

ok, you are right. I retract from that comment, although the logic has been slightly changed, please see my latest comment on GH. 

> And as a side note, I'd really like some feedback on how to implement this
> in the facebook-related code paths.

Following the principle stated in comment #23 (that I do not retract), that should be done in another bug. If you open it I would be more than happy to contribute to it.
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

Pull request updated!

And tested with gmail and facebook imports :)
Attachment #8370836 - Flags: review?(francisco.jordano)
Hey Benoit,

The patch is pretty much done, but we'll probably need to quantify the gain it brings us in order to make an inform uplift decision.

Could you help us with profiling?

(Since the reference workloads don't have thumbnails, the best way to test this is to import your facebook contacts, at least if you're a facebook user :))
Flags: needinfo?(bgirard)
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

There we go, thanks a lot Etienne!
Attachment #8370836 - Flags: review?(francisco.jordano) → review+
Blocks: 969274
I left substantial comments on GH. I think the patch is far from ready to land and I would ask reviewers to reconsider the r+ given
(In reply to Jose M. Cantera from comment #31)
> I left substantial comments on GH. I think the patch is far from ready to
> land and I would ask reviewers to reconsider the r+ given

My r+ was only regarding the SMS part, so I'm keeping it.
Hi Etienne did you take a loot to Jose's comments regarding the algorithm to create the thumbnails, apparently we are loosing quality.
Flags: needinfo?(etienne)
(In reply to Jose M. Cantera from comment #31)
> I left substantial comments on GH. I think the patch is far from ready to
> land and I would ask reviewers to reconsider the r+ given

Sorry guys, don't have the time nor the energy to play politics with José.
The patch is here, I gave it my very best shot. Feel free to take it, split it, build on it, or throw it away.
Assignee: etienne → nobody
Flags: needinfo?(etienne)
Wilfred, can you give us your POV about the quality of the thumbnails?

If that's ok for you, let's land this and I'll open two follow ups:

- One for FB sync.
- Other for the device pixel ratio.

Cheers,
F.
Flags: needinfo?(wmathanaraj)
(In reply to Francisco Jordano [:arcturus] from comment #35)
> Wilfred, can you give us your POV about the quality of the thumbnails?
> 
> If that's ok for you, let's land this and I'll open two follow ups:
> 
> - One for FB sync.
> - Other for the device pixel ratio.
> 
> Cheers,
> F.

Fwiw Etienne has fixed some of the quality concerns as far as I know.
Given the fixes that go in I think we can live with the current quality of the thumbnail image.
Flags: needinfo?(wmathanaraj)
Thanks for being the mediator Francisco :)
And sorry for over-reacting.

(In reply to Francisco Jordano [:arcturus] from comment #35)
> If that's ok for you, let's land this and I'll open two follow ups:

The contacts app has still 5 open blockers as of today...
I think we're past opening follow ups.

> - One for FB sync.

Can we get a exhaustive feedback on this, with the goal of actually landing the patch?

I'm afraid

> Unfortunately this is not the only place where the thumbnail has to be created as there are other code paths, mainly related to synchronization 

isn't actionable :/

> - Other for the device pixel ratio.

I don't think there's anything left to be done here.
Assignee: nobody → etienne
:etienne,

thanks for fixing the DPI! and sorry if my messages led you to misinterpret them as playing politics, It was not my real intention honestly. But in the future, as I'm a Contacts App peer I would like to be informed of this kind of changes, as soon as possible. Also I strongly believe that cross-app are not a good idea in terms of code control and change management. But I can understand you could be in a hurry for the 1.3+ deadline. 

Nonetheless, before landing I think we need to fix two things. 

A/ Generating thumbnails when FB sync updates images. That's an easy fix

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/js/fb_sync.js#L130 

B/ I still think that the generated thumbnails should be square instead of preserving the aspect ratio. We already have code that makes a picture square

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/utilities/image_square.js

thank you very much!
(In reply to Jose M. Cantera from comment #40)
> Nonetheless, before landing I think we need to fix two things. 
> 
> A/ Generating thumbnails when FB sync updates images. That's an easy fix
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/
> js/fb_sync.js#L130 

done, how can I trigger a sync to manually test this?

> 
> B/ I still think that the generated thumbnails should be square instead of
> preserving the aspect ratio. We already have code that makes a picture square
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> js/utilities/image_square.js


We display only squared contact images in gaia, but the mozContacts database is meant to be shared with other third party apps. This is why we don't want to remove part of the image.

The current implementation lets us (gaia) display full squared image without stretching (only croping), while still giving the option for other apps to do differently.
(In reply to Etienne Segonzac (:etienne) from comment #41)
> (In reply to Jose M. Cantera from comment #40)
> > Nonetheless, before landing I think we need to fix two things. 
> > 
> > A/ Generating thumbnails when FB sync updates images. That's an easy fix
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/
> > js/fb_sync.js#L130 
> 
> done, how can I trigger a sync to manually test this?

trigger FB import, set airplane mode, import contacts, they will be imported without photo. Then, unset airplane mode, click on update imported friends and a sync will be triggered and the contacts photos will downloaded. 

> 
> > 
> > B/ I still think that the generated thumbnails should be square instead of
> > preserving the aspect ratio. We already have code that makes a picture square
> > 
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > js/utilities/image_square.js
> 
> 
> We display only squared contact images in gaia, but the mozContacts database
> is meant to be shared with other third party apps. This is why we don't want
> to remove part of the image.
> 

Yes, but if we follow the same rationale, why are you creating the thumbnail with Gaia in mind i.e. with the dimensions required by the Gaia apps? 

> The current implementation lets us (gaia) display full squared image without
> stretching (only croping), while still giving the option for other apps to
> do differently.
(In reply to Jose M. Cantera from comment #42)
> (In reply to Etienne Segonzac (:etienne) from comment #41)
> > (In reply to Jose M. Cantera from comment #40)
> > > Nonetheless, before landing I think we need to fix two things. 
> > > 
> > > A/ Generating thumbnails when FB sync updates images. That's an easy fix
> > > 
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/
> > > js/fb_sync.js#L130 
> > 
> > done, how can I trigger a sync to manually test this?
> 
> trigger FB import, set airplane mode, import contacts, they will be imported
> without photo. Then, unset airplane mode, click on update imported friends
> and a sync will be triggered and the contacts photos will downloaded. 
> 
> > 
> > > 
> > > B/ I still think that the generated thumbnails should be square instead of
> > > preserving the aspect ratio. We already have code that makes a picture square
> > > 
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > > js/utilities/image_square.js
> > 
> > 
> > We display only squared contact images in gaia, but the mozContacts database
> > is meant to be shared with other third party apps. This is why we don't want
> > to remove part of the image.
> > 
> 
> Yes, but if we follow the same rationale, why are you creating the thumbnail
> with Gaia in mind i.e. with the dimensions required by the Gaia apps? 
> 
> > The current implementation lets us (gaia) display full squared image without
> > stretching (only croping), while still giving the option for other apps to
> > do differently.

Well, the current implementation works with the images loaded manually because they are made square beforehand https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/views/form.js#L1058 but that's not true for all the photos as they could come from other sources.
(In reply to Jose M. Cantera from comment #42)
> > We display only squared contact images in gaia, but the mozContacts database
> > is meant to be shared with other third party apps. This is why we don't want
> > to remove part of the image.
> > 
> 
> Yes, but if we follow the same rationale, why are you creating the thumbnail
> with Gaia in mind i.e. with the dimensions required by the Gaia apps? 

We're creating thumbnails because that's the point of the bug...
I strongly believe that the thumbnail version of an image should be the same image but smaller, we need to pick a size, the current setting is my best guest for the current use cases, and since it does not arbitrarily removes part of the image it should work well for other use cases.
(In reply to Jose M. Cantera from comment #43)
> Well, the current implementation works with the images loaded manually
> because they are made square beforehand
> https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> js/views/form.js#L1058 but that's not true for all the photos as they could
> come from other sources.

Not sure I follow you.
We already make sure that the images will be displayed in squares in CSS [1], we don't need to crop every picture in the DB.

[1] https://github.com/mozilla-b2g/gaia/blob/master/shared/style/lists.css#L125-134
(In reply to Etienne Segonzac (:etienne) from comment #45)
> (In reply to Jose M. Cantera from comment #43)
> > Well, the current implementation works with the images loaded manually
> > because they are made square beforehand
> > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > js/views/form.js#L1058 but that's not true for all the photos as they could
> > come from other sources.
> 
> Not sure I follow you.
> We already make sure that the images will be displayed in squares in CSS
> [1], we don't need to crop every picture in the DB.
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/master/shared/style/lists.css#L125-
> 134

Yes, but if a picture is not square it could be stretched in a way that the aspect ratio is not kept and as a result you can get a distorted image.
(In reply to Jose M. Cantera from comment #46)
> (In reply to Etienne Segonzac (:etienne) from comment #45)
> > (In reply to Jose M. Cantera from comment #43)
> > > Well, the current implementation works with the images loaded manually
> > > because they are made square beforehand
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > > js/views/form.js#L1058 but that's not true for all the photos as they could
> > > come from other sources.
> > 
> > Not sure I follow you.
> > We already make sure that the images will be displayed in squares in CSS
> > [1], we don't need to crop every picture in the DB.
> > 
> > [1]
> > https://github.com/mozilla-b2g/gaia/blob/master/shared/style/lists.css#L125-
> > 134
> 
> Yes, but if a picture is not square it could be stretched in a way that the
> aspect ratio is not kept and as a result you can get a distorted image.

No, we use background-size: cover [1].

[1] https://developer.mozilla.org/en-US/docs/Web/CSS/background-size
> The image is rendered, preserving its intrinsic proportion
(In reply to Jose M. Cantera from comment #42)
> (In reply to Etienne Segonzac (:etienne) from comment #41)
> > (In reply to Jose M. Cantera from comment #40)
> > > Nonetheless, before landing I think we need to fix two things. 
> > > 
> > > A/ Generating thumbnails when FB sync updates images. That's an easy fix
> > > 
> > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/
> > > js/fb_sync.js#L130 
> > 
> > done, how can I trigger a sync to manually test this?
> 
> trigger FB import, set airplane mode, import contacts, they will be imported
> without photo. Then, unset airplane mode, click on update imported friends
> and a sync will be triggered and the contacts photos will downloaded. 
> 

Just confirmed this works properly now :)
(In reply to Etienne Segonzac (:etienne) from comment #47)
> (In reply to Jose M. Cantera from comment #46)
> > (In reply to Etienne Segonzac (:etienne) from comment #45)
> > > (In reply to Jose M. Cantera from comment #43)
> > > > Well, the current implementation works with the images loaded manually
> > > > because they are made square beforehand
> > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/
> > > > js/views/form.js#L1058 but that's not true for all the photos as they could
> > > > come from other sources.
> > > 
> > > Not sure I follow you.
> > > We already make sure that the images will be displayed in squares in CSS
> > > [1], we don't need to crop every picture in the DB.
> > > 
> > > [1]
> > > https://github.com/mozilla-b2g/gaia/blob/master/shared/style/lists.css#L125-
> > > 134
> > 
> > Yes, but if a picture is not square it could be stretched in a way that the
> > aspect ratio is not kept and as a result you can get a distorted image.
> 
> No, we use background-size: cover [1].
> 
> [1] https://developer.mozilla.org/en-US/docs/Web/CSS/background-size
> > The image is rendered, preserving its intrinsic proportion

ok, in that case I trust you :)

thanks!
(In reply to Etienne Segonzac (:etienne) from comment #48)
> (In reply to Jose M. Cantera from comment #42)
> > (In reply to Etienne Segonzac (:etienne) from comment #41)
> > > (In reply to Jose M. Cantera from comment #40)
> > > > Nonetheless, before landing I think we need to fix two things. 
> > > > 
> > > > A/ Generating thumbnails when FB sync updates images. That's an easy fix
> > > > 
> > > > https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/facebook/
> > > > js/fb_sync.js#L130 
> > > 
> > > done, how can I trigger a sync to manually test this?
> > 
> > trigger FB import, set airplane mode, import contacts, they will be imported
> > without photo. Then, unset airplane mode, click on update imported friends
> > and a sync will be triggered and the contacts photos will downloaded. 
> > 
> 
> Just confirmed this works properly now :)

I reviewed the code and it looks ok!

thanks!
Comment on attachment 8370836 [details] [review]
Pointer to gaia PR

the code looks good now.
Attachment #8370836 - Flags: feedback- → feedback+
https://github.com/mozilla-b2g/gaia/commit/9fc36dde3a4a3c5ca200275b68ffb56b4173bec3

I'll prepare a 1.3 version of the patch (I think only tests are conflicting)
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Uplifted on 1.3, with a green travis, after a manual smoke test covering contact creation, update, merge, import (gmail and facebook) and import from the ftu.

https://github.com/mozilla-b2g/gaia/commit/00cd1ae74dfe773a41b55275b0b468a62f35076d
Flags: needinfo?(bgirard)
Etienne, can you comment on a list of test areas that QA can focus around this patch?   other than smoketesting, give some tips here on highest areas of regression points, things that would be good to retest, etc..   

Massimo's team at Telefonica has volunteered to assist with QA on the next 1.3 build.  Thanks Massimo!
Flags: needinfo?(mbarone976)
Like I said:

> smoke test covering contact creation, update, merge, import (gmail and facebook) and import from the ftu.
This bugs have been tested (02/12/2014)
1.3
Gecko 3820ecd
Gaia 01e9da4
Thanks Loli!

(In reply to Loli from comment #56)
> Related to :
> 
> - Import contact to gmail and facebook, you can see this bugs:
> https://bugzilla.mozilla.org/show_bug.cgi?id=970832

Trying to reproduce it to see if it was caused by this patch.

> https://bugzilla.mozilla.org/show_bug.cgi?id=970874
> https://bugzilla.mozilla.org/show_bug.cgi?id=970899, duplicate of
> https://bugzilla.mozilla.org/show_bug.cgi?id=838605

Those _look_ unrelated.
Depends on: 972245
Depends on: 983740
Flags: needinfo?(mbarone976)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: