837909 - Brief freeze in Gallery when starting to pinch & zoom

Status: VERIFIED FIXED

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

Description

[Bas Schouten (:bas.schouten)]

The gallery app seems to jerk when every time you start pinching/zooming. This was worst on the LG phones I tried, the jerk there seemed to be over 500ms.

Comment 1

[[e-mail]]

Yes, I do not much intermediate updates while zoom. 
Pinch operation is taking around 635 ms. So we do not see the initial update till that time.

The following profiling could help us finding the root issues. 
PreShellPaint takes 303ms
-> EndTransactions  287ms
-> BasicThebesLayer::PaintThebes 289 (Multiple calls spending 3 + 7 + 4 + 272 + 3 ms)

Overall we see around 5 PreShellPaints that consumes 100+ms each during Zoom.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Where I would start here is by investigating whether the delay is coming from app code or gecko.  Applying attachment 706602 [details] [diff] [review] and grabbing a couple of profiles with the gecko profiler would let us start to tell.

My first guess is that we might have another instance of bug 796242 in the gesture detector; delaying zooming artificially.

My second guess is that when we start zooming, we make the image layer "active" and are re-decoding and painting the image again.  If so, we can hack around that in the gallery app.

Comment 3

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

It's hard to tell from the profile [1] if there's artificial delay from gesture detection.  Someone who knows the zooming code may need to manually inspect that.

The profile does clearly show an overhead from layerizing the transformed image though.  This appears to be 150-200ms in the profile.  Adding a |perspective: 1px| rule to the element that's being transformed should keep it active outside of the gecko timeout.

[1] http://people.mozilla.com/~bgirard/cleopatra/#report=74ef28fc3bbeecccd7a09f8742da053307869cb4

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(Setting blocking flag to get this through triage and hopefully find an owner.  Not suggesting we block on this.)

Comment 5

[David Flanagan [:djf]]

The freeze is image decoding time.

To save memory, gallery displays preview images instead of fullsize images.  It only loads the full image if you want to zoom into it.

The pause while it loads is (in my opinion) unavoidable. (The alternative is that other apps will exit with OOM while gallery is running.) The blue dashed border around the image appears when this happens to let the user know that something is happening.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can we start zooming the preview image while we're loading the full-sized one and then swap it in when it's ready?

Comment 7

[David Flanagan [:djf]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #6)
> Can we start zooming the preview image while we're loading the full-sized
> one and then swap it in when it's ready?

Oh. Yes, that seems like the obvious solution. I think the pixellation might look bad, but responsiveness would improve. I don't think it is a trivial fix, but it shouldn't be too hard.

I'm the most logical one to work on this, so I'm assigning to myself. Help me understand how to prioritize it... I assume that a blocking 837218 bugs have lower priority than tef+, right?

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Correct.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

David, Chris, sounds like this is under control without the graphics team, but let me know if we can help.

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Now that there's an owner, as per comment 4, we're not blocking on this.

Comment 11

[David Flanagan [:djf]]

The relevant code is in shared/js/media/media_frame.js, specifically in the _displayImage(), _switchToFullSizeImage(), and zoom() functions.  I'm hoping I can just modify _displayImage() to take the small image and make it the size of the big image and call its callback right away.  But the animated zoom (on double tap) will probably need something more to make it work right.

Comment 12

[David Flanagan [:djf]]

Attachment: link to patch on github

As described in the bug, media_frame.js displays a preview image when it can. But if the user wants to zoom in, it needs to switch to the fullsize image.

Previously, it would wait for the fullsize image to load and then begin the zoom.

With this patch it starts zooming with the preview image, and then switches to the fullsize image when that is available.  Some pixellation is visible before the images switch, but there is no initial delay, which is what this bug is about.

The hard thing about this bug is that we don't have a good way to know when the full size image is decoded and ready to display, and the patch uses a setTimeout of 1000ms to give the device time to decode a 1200x1600 camera photo.  500ms was not long enough.  800ms was usually, but not always long enough. I'm sure this will vary by device.

This is a tricky module to review if you're not already familiar with it. The most important stuff is in _switchToFullSizeImage().  A bunch of code has been removed from _displayImage().

Comment 13

[David Flanagan [:djf]]

Comment on attachment 716857 [details]
link to patch on github

Asking Dale for a review.

Dale, note that in addition to the main gallery app, the gallery open activity (open.html/open.js) and the camera filmstrip use media_frame. I've tested those, and by inspection, you can verify that neither of them use the preview image feature that gallery relies on.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 716857 [details]
link to patch on github

Probably unnecessarily, let me say Huzzah! to a much better experience.

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to David Flanagan [:djf] from comment #12)
> 
> The hard thing about this bug is that we don't have a good way to know when
> the full size image is decoded and ready to display

Is this worth a follow-up bug?  Do we want the ability to know exactly when the full size image is decoded and ready to display?  Or is this good enough "forever"?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We really need a new API/notification for this.

Comment 17

[David Flanagan [:djf]]

Filed bug 844245 as a followup.

Comment 18

[Dale Harvey (:daleharvey)]

Comment on attachment 716857 [details]
link to patch on github

Much better experience, the temp low quality jog is fine.

Agree with the nit comments and would prefer .bind + addEventListener unless theres a reason not to, would also like to see a comment that points to the bug regarding the setTimeout

Comment 19

[David Flanagan [:djf]]

Nits fixed, and landed on gaia master:
https://github.com/mozilla-b2g/gaia/commit/945074923daa296c135de87f4215aedd4d88f4b8

Comment 20

[David Flanagan [:djf]]

Comment on attachment 716857 [details]
link to patch on 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 #): no regression; just never really implemented this right
User impact if declined: bad ux for zooming in on images 
Testing completed: locally, and in the MWC build, I think
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Comment 21

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 716857 [details]
link to patch on github

Approving for v1-train uplift.

Comment 22

[Michael Vines [:m1] [:evilmachines]]

Requesting uplift to v1.0.1.  This is one of the patches in the MWC build, and makes a big improvement to initial gallery zoom UX

Comment 23

[Lukas Blakk [:lsblakk] use ?needinfo]

Approving for uplift based on MWC demo build testing.

Comment 24

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

Uplifting commit 945074923daa296c135de87f4215aedd4d88f4b8:
v1-train: 306aded5000c9adc8d45715a975b793e54c4cb05
v1.0.1: 7c7f3d90eccc276d2bf911d8d578b7a565cd0f84

Comment 25

[Darren]

The issue still seems to be encountered in build:
Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/da523063aa7b
Gaia: a845be046c5d3cb077e3c78f963ca5c079e7ab3d

Kernel Date: Dec 5
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/e43e65ec89d2
Gaia: daea430624ec02f8d36a12d581fc4a3278c27cb7

Would you like me to reopen the issue, or write a new one?

Comment 26

[John Hammink]

Reopening this.   Darren, can you please include the Build ID also next time?  I'm assuming this is from a 4/4 nightly?  Commercial RIL or Mozilla RIL?

Comment 27

[David Flanagan [:djf]]

Darren,

Given how much the relevant code has changed since this bug was originally filed, it is hard to believe that you could be seeing the same bug (which involved a 500ms freeze) So please describe exactly what you're seeing and what Gaia branch you were testing on.  (I don't know what to do with gaia commit numbers, but I do need to know whether you were on master, v1-train or v1.0.1 when you saw this).

Comment 28

[Darren]

I was on both V1-train and v1.0.1. There seemed to be a noticeable delay when pinching/ zooming.
Also I was on Mozilla RIL and it was the 4/4 nightly build.

Comment 29

[Dietrich Ayala (:dietrich)]

I think we should open a new bug, if only because there's already code here that has landed. It gets *very* confusing once a bug has multiple patch cycles.

Jammink or Darren, can you please open a new bug?

Comment 30

[bov]

There are no appreciable delay when pinching/zooming in the gallery pictures.
Tested latest build Inari.
Git Commit Information: 
2013-04-12 12:53:16
3e7dda88950ae09166109783adea818...