Don't block camera preview when loading storage config details

VERIFIED FIXED

Status

VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: jlal, Assigned: daleharvey)

Tracking

({perf})

unspecified
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(blocking-b2g:tef+, b2g18 verified, b2g18-v1.0.0 fixed, b2g18-v1.0.1 verified)

Details

(Whiteboard: [fast:500ms], [FFOS_perf], UX-P2)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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+ )
(Reporter)

Updated

6 years ago
Blocks: 817051
(Reporter)

Updated

6 years ago
Whiteboard: [fast:500ms]
(Reporter)

Updated

6 years ago
Keywords: perf
(Assignee)

Comment 1

6 years ago
Created attachment 705234 [details]
Pointer to Github PR
Attachment #705234 - Flags: review?(dflanagan)
(Assignee)

Comment 2

6 years ago
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 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.
Attachment #705234 - Flags: review?(dflanagan) → review+
Whiteboard: [fast:500ms] → [fast:500ms], [perf_tsk]
Whiteboard: [fast:500ms], [perf_tsk] → [fast:500ms], [FFOS_perf]
blocking-b2g: leo? → tef?

Updated

6 years ago
Whiteboard: [fast:500ms], [FFOS_perf] → [fast:500ms], [FFOS_perf], UX-P2
Blocks: 835367
blocking-b2g: tef? → tef+
Dale, this got marked tef+, should we land on master as-is, or are you going to make  follow-up changes?
(Assignee)

Comment 5

6 years ago
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
Should we clear this issue or will you update the PR ?
Flags: needinfo?(dale)
(Assignee)

Comment 7

6 years ago
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
Flags: needinfo?(dale)
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
status-b2g18: --- → affected
status-b2g18-v1.0.0: --- → affected
(Assignee)

Comment 9

6 years ago
Created attachment 708553 [details] [diff] [review]
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.
Attachment #708553 - Flags: review?(dflanagan)
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+
Attachment #708553 - Flags: review?(dflanagan) → review+
(Assignee)

Comment 11

6 years ago
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
(Assignee)

Comment 12

6 years ago
merged in: https://github.com/mozilla-b2g/gaia/commit/2f0d50894cc87b9c9862717e2aa7f36b5d798742
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
v1-train: 469e3047b80cf17f875c98684794d3c87d31de52
v1.0.0: e2ce3484f609dcdba76ce017b7bf5242c27c41a2
status-b2g18: affected → fixed
status-b2g18-v1.0.0: affected → fixed
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.
status-b2g18-v1.0.1: --- → affected
My mistake, branching for v1.0.1 was 2/13 - carry on.
status-b2g18-v1.0.1: affected → fixed

Comment 16

6 years ago
No Test Case needed in Moztrap for this issue.
Flags: in-moztrap-

Comment 17

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

Updated

6 years ago
Flags: needinfo?(jlal)
Keywords: verifyme
(Reporter)

Comment 18

6 years ago
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.
Flags: needinfo?(jlal)

Comment 19

6 years ago
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.
Status: RESOLVED → VERIFIED
status-b2g18: fixed → verified
status-b2g18-v1.0.1: fixed → verified

Updated

6 years ago
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.