833104 - Don't block camera preview when loading storage config details

Status: VERIFIED FIXED

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

Description

[James Lal [:lightsofapollo]]

We block camera preview while waiting for asyncStorage to return. This adds about 500-600ms of time to the overall time it takes for the user to see the first preview.

( Think this should be tef+ if not then at least leo+ )

Comment 1

[Dale Harvey (:daleharvey)]

Attachment: Pointer to Github PR

Comment 2

[Dale Harvey (:daleharvey)]

I am seeing around 300/400ms improvements with this, not quite 500/600 but a big improvement.

I dont like having a codepath that will be very very rarely executed, the deferring of createDCFFilename is unlikely to happen as we are unlikely to capture before asyncStorage returns, but it is possible. I tested by changing asyncStorage.getItem to setTimeout(fun(), 100000) and it behaves as expected (takes a photo and waits 10 seconds before actually saving)

Comment 3

[David Flanagan [:djf]]

Comment on attachment 705234 [details]
Pointer to Github PR

r+.

If you want to avoid the hack with saving the DCF args here's another way you could do it: in gotPreviewScreen() don't call enableButtons() unless dcfConfigLoaded is true. If it is not true, set a previewStreamReady flag. And in the asyncStorage callback, if previewStreamReady is set, then call enableButtons() from there.

Whichever of the two async operations completes last will enable the shutter button.

I don't know if it would make any significant difference, but you could also move the asyncStorage.getItem() up to the top of the init() function, or even to the top of the file, since it should be safe to call that even before the DOM is ready.

Comment 4

[Ben Francis [:benfrancis]]

Dale, this got marked tef+, should we land on master as-is, or are you going to make  follow-up changes?

Comment 5

[Dale Harvey (:daleharvey)]

djf, cheers for the comments, I didnt want to block actually taking the photo as it seemed that should also be fast. 

But ben yeh I am working on a bigger set of changes to improve the camera performance, so we shouldnt land this as it involves the same changes done in a different way. I will close the PR

Comment 6

[David Scravaglieri [:scravag]]

Should we clear this issue or will you update the PR ?

Comment 7

[Dale Harvey (:daleharvey)]

I am updating the PR, not entirely sure I will be able to split out the commits for one per bug (I have those done but it doesnt get the performance we need) trying another approach right now will update the meta bug when I have results

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

Marking this affected for status-b2g18 and status-b2g18-v1.0.0 --- please update these flags when the PR is landed to v1-train and v1.0.0

Comment 9

[Dale Harvey (:daleharvey)]

Attachment: Dont block the camera preview from loading during initialisation

Instead of juggling things around, this patch allows the camera to start initialising the hardware as soon as camera.js has loaded, and waits for the previewstream to catch up before initialising the rest as trying to load them asll asynchronously at the same time slows the initial previewStream from loading.

The dcf logic has been extracted as with the last patch I had it be allowed to be called before being initialised, I dont think we dont want to block taking photos either.

The only other change to the logic is the enableButtons logic which used assume activities would have init'd before gotPreviewStream (technically it could have raced, but almost impossible) I have tested in every configuration I can think of and think its more stable now.

I tried a few different ways of doing this patch and this is the one that was the least intrusive and had the largest performance gain.

Comment 10

[David Flanagan [:djf]]

Comment on attachment 708553 [details] [diff] [review]
Dont block the camera preview from loading during initialisation

This patch makes a noticeable difference in responsiveness.  Separately, it might be nice to have a spinner or other busy indicator while acquiring preview streams, but that is a different bug.

I have a number of comments on github, including one bit of unused code that you can take out. But nothing that would block you from getting this landed.

r+

Comment 11

[Dale Harvey (:daleharvey)]

Cheers David

Updated the commit based on a few of the comments, I didnt want to introduce a spinner in this commit to even more confuse the performance implications, but yeh we should have one if we cant get anything more out of hardware.

I wont land yet since I need to rush and havent tested thoroughly, but its there for any further feedback

Comment 12

[Dale Harvey (:daleharvey)]

merged in: https://github.com/mozilla-b2g/gaia/commit/2f0d50894cc87b9c9862717e2aa7f36b5d798742

Comment 13

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

v1-train: 469e3047b80cf17f875c98684794d3c87d31de52
v1.0.0: e2ce3484f609dcdba76ce017b7bf5242c27c41a2

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

This looks like it needs to be uplifted to v1.0.1 branch since it was landed to v1-train after the 1/25 merge.

Comment 15

[Lukas Blakk [:lsblakk] use ?needinfo]

My mistake, branching for v1.0.1 was 2/13 - carry on.

Comment 16

[amiller]

No Test Case needed in Moztrap for this issue.

Comment 17

[amiller]

Can you please provide steps to verify this fix - as we will blackbox test from the UI?

Comment 18

[James Lal [:lightsofapollo]]

This is purely a performance improvement to blackbox testing the normal functionality of camera is sufficient as this is not really a bug fix or feature.

Comment 19

[sarsenyev]

Inari device:
Environmental  Variables:
Inari Build ID: 20130429070204
Kernel Date: Feb 21
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/45aa5ba0ed53
Gaia: cf2d4136f0ebc66039637fdbeb72ed184dfbc0f2

Leo device:
Environmental  Variables:
Unagi Build ID: 20130426070204
Kernel Date: Mar 15
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18/rev/6c2493de1441
Gaia: c9046a7acef33328977840176ff5574720d2c74c

The issue no longer reproduces on "Leo", "Inari" device. The preview is available as soon as the application opens.