1038172 - Camera app launch latency regressed in v2.0

Status: VERIFIED FIXED

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

Description

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Camera app launch latency has regressed compared to v1.3.

For v1.3:
gaia: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v1.3&id=e08beb0297aff472b5c84437e9d7eaf8c0400a29
gecko: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v1.3&id=e0bfab94fd20e4dcbb2fecf19d7c2dc606189f81




For v2.0:
gaia: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gaia/commit/?h=mozilla/v2.0&id=ef67af27dff3130d41a9139d6ae7ed640c34d922
gecko: https://www.codeaurora.org/cgit/quic/lf/b2g/mozilla/gecko/commit/?h=mozilla/v2.0&id=d3eae03cdf4e6944e646d05938a22dc1380a0d95


Test Steps:
1) Flush v1.3 and v2.0 build on two flame device (512MB) device. Launch Camera app and compare side by side.

2) launch latency = touchdown to displaying camera preview pic
   In v1.3 launch latency is : 1.3 secs
   In v2.0 launch latency is : 2.4 secs

Comment 1

[bhavana bajaj [:bajaj]]

:mlee, can you help here with further debugging and also confirm if we are able to see this in our datazilla dashboard ?

Comment 2

[Mike Lee [:mlee]]

Hema,
Can you have someone on the Camera app team look into this?

Comment 3

[David Flanagan [:djf]]

Mike,

Does your team's datazilla data confirm this much latency?  That is: does this bug reproduce on flame or on any other devices that we have, or is this QRD only?

Comment 4

[David Flanagan [:djf]]

I'd be interested to know if memory issues and zmem swapping is involved here.  Does this reproduce on a 1gb Flame?

Comment 5

[Hema Koka [:hema]]

Do we have the numbers from 1.4 as well? Wondering if the new homescreen in the background is attributing to the increase in 2.0

Comment 6

[David Flanagan [:djf]]

Tapas,

The camera app changed recently to cache its initial configuration. The first launch after a flash is expected to take longer than the second launch. Is your latency data based on multiple launches, or only on the first launch after flashing a build?

Comment 7

[Mike Lee [:mlee]]

David,

Here's the data we've gathered using Eideticker since Datazilla isn't useful for comparing timing across releases:


----- Forwarded Message -----
From: "William Lachance" <[e-mail]>
Subject: Re: Regressions in App startup time in 2.0

Ok did a run, results below. Videos, etc. at

http://people.mozilla.org/~wlachance/eideticker-startup-test/

(click through to the index page, then click on the bars for videos and an
option to dig into the detail of what we're measuring... e.g.

http://people.mozilla.org/~wlachance/eideticker-startup-test/14-results/contacts-startup-14/#/timetostableframe
->
http://people.mozilla.org/~wlachance/eideticker-startup-test/14-results/contacts-startup-14/detail.html?id=90d314340b8f11e4b65210ddb19eacac#/framediffsums)

Remember this is a comparison between 1.4 and 2.0, so not the same as
what Qualcomm was measuring.

Still:

1. We see a small regression with contacts
2. An *improvement* in settings
3. An *improvement* in camera.

So not what they were seeing.

Hope this helps and let me know if you need more help interpreting this
data,

Will

----

v2.0 aurora (build on 14 July):

=== Results on b2g-contacts-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [1.8833333333333333, 1.95, 1.9, 1.9833333333333334, 1.8666666666666667]

=== Results on b2g-messages-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [4.683333333333334, 4.466666666666667, 4.633333333333334,
4.466666666666667, 4.55]

=== Results on b2g-settings-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.316666666666667, 2.1333333333333333, 2.2666666666666666, 2.1,
2.283333333333333]

=== Results on b2g-camera-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.6333333333333333, 2.2333333333333334, 2.95, 2.966666666666667, 2.2]

v1.4 (build on 14 July):

=== Results on b2g-contacts-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [1.8, 1.65, 1.65, 1.8166666666666667, 1.5666666666666667]

=== Results on b2g-messages-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [4.016666666666667, 4.0, 4.133333333333334, 4.133333333333334,
3.9833333333333334]

=== Results on b2g-settings-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [2.6166666666666667, 2.5, 2.8666666666666667, 2.1333333333333333, 2.6]

=== Results on b2g-camera-startup for FirefoxOS ===
   Times to first stable frame (seconds):
   [3.3, 2.933333333333333, 3.0, 2.8833333333333333, 2.816666666666667]

Comment 8

[Mike Lee [:mlee]]

+ William & Geo. Guys I just added Will's 2.0 Eideticker launch findings in comment 7.

Comment 9

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to David Flanagan[OOO until July 14] [:djf] from comment #6)
> Tapas,
> 
> The camera app changed recently to cache its initial configuration. The
> first launch after a flash is expected to take longer than the second
> launch. Is your latency data based on multiple launches, or only on the
> first launch after flashing a build?

It is based on multiple launch. Could you please try the STR which I mentioned in Comment 0 ?

Comment 10

[Justin D'Arcangelo [:justindarc]]

Assigning to Wilson to investigate startup time related to loading the font icons.

Comment 11

[David Flanagan [:djf]]

Mike,

Thanks for this data. Unfortunately, the launch time regression that CAF is concerned about is between 1.3 and 2.0. Your data shows that 2.0 is better than 1.4, but it doesn't show how we did in 1.3.

Comment 12

[David Flanagan [:djf]]

(In reply to Tapas Kumar Kundu from comment #9)
> (In reply to David Flanagan[OOO until July 14] [:djf] from comment #6)
> > Tapas,
> > 
> > The camera app changed recently to cache its initial configuration. The
> > first launch after a flash is expected to take longer than the second
> > launch. Is your latency data based on multiple launches, or only on the
> > first launch after flashing a build?
> 
> It is based on multiple launch. Could you please try the STR which I
> mentioned in Comment 0 ?

I did try that, and found that camera launch time in 2.1 was about 1.6s.  I have not tried 2.0 or tried comparing 1.3 to 2.0. But the fact that my 1.6s time was so different than the 2.4s time you were finding made me wonder how you were measuring.

Comment 13

[Hema Koka [:hema]]

Mike/William,

Can we please get the comparison between 1.3 and 2.0 using Eideticker? Your help is appreciated!

Thanks
Hema

Comment 14

[Mike Lee [:mlee]]

Dave & Hema,

We don't have FxOS 1.3 Flame builds that support the automation used to capture the data presented by Eideticker. 1.4 is the earliest version for which we have this support. William can elaborate on this but when we last discussed his feedback was that the work needed to support Eideticker on 1.3 was non-trivial and likely not the most effective use of his time.

Comment 15

[William Lachance (:wlach)]

(In reply to Mike Lee [:mlee] from comment #14)
> Dave & Hema,
> 
> We don't have FxOS 1.3 Flame builds that support the automation used to
> capture the data presented by Eideticker. 1.4 is the earliest version for
> which we have this support. William can elaborate on this but when we last
> discussed his feedback was that the work needed to support Eideticker on 1.3
> was non-trivial and likely not the most effective use of his time.

So this 90% right. Eideticker actually does support 1.3 just fine, but it has the hard requirement of an engineering build. It just can't work without one, since a bunch of stuff it uses requires root access to the device. But if we had an eng build for 1.3 on the Flame, we could test.

P.S. New 2.0 results for Eideticker are now being generated continuously here:

http://eideticker.mozilla.org/b2g/#/flame-512/2.0/b2g-camera-startup/timetostableframe

Comment 16

[Hema Koka [:hema]]

The font optimizations that Wilson is currently looking into will probably make improvements at 100ms - 200ms (not significant). 

Mike, We will need your team's help to see if there is any else that we can do to reduce the launch latency

Thanks
Hema

Comment 17

[Diego Marcos [:dmarcos]]

Attachment: cameraStartupFlameVSNexus.MOV

What target are we aiming at? Camera startup times are comparable between Flame and Nexus 4.

Comment 18

[Diego Marcos [:dmarcos]]

In the video attached Nexus 4 is running stock Android 4.4.4 (Kit Kat)

Comment 19

[Ravi Dandu]

Here is published info of app startup time goals at https://wiki.mozilla.org/FirefoxOS/Performance/Release_Acceptance. We want to be < 1 second app start time, and no regressions from previous release.

More explanation on product goals:  

1) No unexplainable regressions: For cases where there are regressions due to new features, we need to get agreement on why the worsened user experience is justified by the new mandatory functionality.

2) User Experience for 1 second startup time: 'Perception of progress' is a need for humans to contune being engaged. From UX research the following timelines are relevant:  
   2a) 140ms cause/effect timeline: Camera Launch animation should be within 140 ms for humans to see effect of pressing the button
   2b) 1s perception of progress:  User should perceive the app is ready within 1s (to 1.25 second) timeline. If an operation will take more than that, a progress indicator is needed, so the human engagement is maintained. 
      These prevents users tuning out and increases retention with FxOS. These user experience guidelines are listed in FxOS Performance Roadmap at https://docs.google.com/a/mozilla.com/document/d/1_8RehppYplSpYZZtALxNTstj8l-BoustidBMaxuazo4/edit#heading=h.krnlq1x1t90g

3) Competition: Apps on FxOS needs to have equal or better  app startup time than equivalent apps on competitive OSs. This fits into our strategy of being a flexible and scalable to various HW, and marketing as a responsive and faster OS. Since we are currently not regularly measuring the competition, this was not listed in the 2.0 release acceptance criteria.

Comment 20

[Wilson Page [:wilsonpage]]

I'm going to spend today experimenting with:

1. Identifying cost of icon-font.
2. Having all content ready in DOM without needing to render and inject on startup

Comment 21

[Wilson Page [:wilsonpage]]

[Flame] I found a potential 40ms gain by not using :before/:after pseudo elements,

w/ pseudo elements: https://pastebin.mozilla.org/5573823
w/o pseudo elements: https://pastebin.mozilla.org/5573801

(this could translate to more on lower end devices)

Comment 22

[Mike Lee [:mlee]]

(In reply to Hema Koka [:hema] from comment #16)
> The font optimizations that Wilson is currently looking into will probably
> make improvements at 100ms - 200ms (not significant). 
> 
> Mike, We will need your team's help to see if there is any else that we can
> do to reduce the launch latency
> 
> Thanks
> Hema

Okay Hema. Let us know how we can help. FYI, 100 - 200 ms is significant though less than the 1s delta reported ;-)

Comment 23

[Wilson Page [:wilsonpage]]

[Hamachi] 180ms difference:

w/ pseudo elements: https://pastebin.mozilla.org/5573932
w/o pseudo elements: https://pastebin.mozilla.org/5574006

Comment 24

[Wilson Page [:wilsonpage]]

(In reply to Mike Lee [:mlee] Paris: 2014.07.21 - 25 from comment #22)
> Okay Hema. Let us know how we can help. FYI, 100 - 200 ms is significant
> though less than the 1s delta reported ;-)

I think 100-200ms is ambitious. I would expect < 100ms after some tweaks.

Comment 25

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

This, like my other performance patches, has turned into a mammoth. It involved changes to many files to trim off milliseconds all over the place. I'll attempt to summarise my findings in descending importance.

- Migrating from Alameda to Requirejs appears to shave 200-300ms off startup depending on the device. We should find out more from jrburke on this at some point.

- Compressing JS after build. I'm quite surprised this wasn't being done already. I assumed there was an automated step that took care of this. On closer inspection I found we had been shipping uncompressed code to production. This should also have an impact on memory consumption, justindarc might be interested in this.

To build an optimized camera app now use the `GAIA_OPTMIZE=1` whenever you are using `make`. This is the flag that default Gaia build uses when building for production.

- Moving all mozSettings calls off the critical path. These seem to sometimes block for ~20ms despite the async API.

- Painting HUD display after app has loaded to avoid expensive opacity paints on critical-path. I decided to still paint the controls view as this gives the user a sense of control right away (even if they're not immediately usable).

- Lazy loading recording-timer, zoom-bar, and indicators. Which are all non-critical modules.

- Tidy up unused hardware-accelerated layers and make sure `will-change` CSS property is on all elements that transition.

- Provide more detailed performance logs on every startup.

- Added new app 'loaded' event, which triggers when all modules (inc. lazy) have loaded.

- The 'app-visually-complete' event was previously being fired too early. We now wait for the preview stream to actually start, before declaring the app 'visually-complete'. So when observing benchmarks it's important to be aware that previous 'app-visually-complete' figures were inaccurate, firing roughly 200-300ms too early!

- The 'moz-app-loaded' event was also being fired too early. It's now fired when all our lazy-loaded modules have finished loading, on the app 'loaded' event.

Comment 26

[Wilson Page [:wilsonpage]]

I have left the patch unsquashed to make it easier to review. We may also only decide to uplift parts to v2.0 branch. Has v2.0 branch diverged far from master? If so this may be a tricky uplift :-/

Comment 27

[David Flanagan [:djf]]

Comment on attachment 8459611 [details] [review]
pull-request (master)

Hema asked me to review this bug as well.  Setting r? for myself so it gets into my list.

Comment 28

[David Flanagan [:djf]]

Comment on attachment 8459611 [details] [review]
pull-request (master)

I read through each commit individually, and my review basically takes the form of a bunch of questions.  Mostly I'm asking you to double-check stuff that I wasn't sure about. Often that is probably because I didn't understand. And sometimes I had a question that was answered when I got to a later commit in the series.

I don't need answers to all my questions, but I hope you'll look them over and make sure that none are actually issues.  There were two actual issues, I think:

1) You moved one of the views so it was created by the controller, but still have a line referencing the view object in app.views.

2) You're not showing the hud until after you've sent the performance events that say the app chrome is visible.

There are also a couple of things I'm nervous about:

- The fade in animation that is delayed by 280ms is now triggered by a different event than it used to be, and I worry that is going to cause more fade ins than there used to be, and perhaps create a possibility of race conditions if the user switches to the preview gallery (for example) while the fade in timer is running.

- the startup time improvement you found for switching from alameda to requirejs seems too good to be true, and I wonder if we're going to pay for it somewhere else, like in memory use. I'd be more comfortable if we knew why it was so much more efficient and had memory profiles of the app with and without this patch to see what the memory impact is.

And finally, note that I have not actually run any of this code. We'll probably want to let this patch sit on master for a day or two before uplifting to make sure that it doesn't cause any regressions and to give QA some time to try it out.

With all that said, I'm setting r+

Comment 29

[:Eli Perelman]

(Quoting David Flanagan [:djf] from comment #28)
> Comment on attachment 8459611 [details] [review]
> pull-request (master)
> 
> 2) You're not showing the hud until after you've sent the performance events
> that say the app chrome is visible.

I'd like to see this resolved before landing. If we really mark these elements as visible or ready for interaction before they actually are, it will skew our performance results and have other possible implications as well, even though it will appear that the timings improved.

Comment 30

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

Please also send us a PR for v2.0 branch . Thanks a lot for your help :)

Comment 31

[Wilson Page [:wilsonpage]]

Sure will do, I'm just working on review comments from djf and dmarcos.

Comment 32

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

I also shared some profiling data via email which may help anyone to see gecko paint/layout delay happening in v2.0 . Thanks a lot for your help :)

Comment 33

[Hema Koka [:hema]]

Mike/Milan, 

I have forwarded the information that Tapas provided on the paint/layout delays that they are seeing on QRD. Would be great to check on the gecko front while Wilson is working on getting the UI side optimizations reviewed. 

Thanks
Hema

Comment 34

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

Mike, you may want to see if bug 1037220 helps there.

Comment 35

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

I did mean Mike Lee

Comment 36

[Hema Koka [:hema]]

Unlike 1.3 camera, when the camera app starts viewfinder is faded in. Looking at the videos, this probably is contributing to user perceived delay in comparison to 1.3 

Mike Habicher can help get a comparison on Flame with and without Viewfinder Fade in. He does not have the camera gear to do this today, but can help get this information tomorrow. Mike Lee, if anyone from your team can help today, it would be great! 

We are also seeing delay in transitioning from homescreen which someone from the systems fe team should investigate. NI'ing gregor.

Milan mentioned that patch in this bug 1037220 would possibly help speed up the app transition animation. Can we get some help to try this out as well?

Thanks
Hema

Comment 37

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8459611 [details] [review]
pull-request (master)

Ok, so here are some videos I captured comparing v1.3 to the new Camera (with and without the patch). I was unable to test directly to v2.0 because the patch has not yet been uplifted for v2.0, but there is very little difference with the Camera on master so this should be very comparable. Please note that it took me a couple attempts in each video to get the apps to launch at the same time so be sure to watch for when I manage to actually get them in sync :-)

Also, it was noted earlier today that there is code for an intentional 280ms delay in showing the viewfinder in the app. However, upon closer inspection, this 280ms delay is not applied when launching the app and is only used when switching between picture and video modes to provide a smoother transition. Regardless, I also removed the delay and captured some videos with and without this delay for comparison (you should see that there is no difference).

NOTE: All videos are titled as LEFT vs. RIGHT when referring to the two phones depicted. Both phones were Flame reference devices configured with 512 MB memory.

v1.3 vs. master - http://youtu.be/jt4HycGzl-g
v1.3 vs. master+patch - http://youtu.be/XxeBrQVad0k
v1.3 vs. master+patch+nodelay - http://youtu.be/AMbApdIM_R8
master vs. master+patch - http://youtu.be/jO8Q6i8JX6g
master vs. master+patch+nodelay - http://youtu.be/AlvOREEenuY
master+patch vs. master+patch+nodelay - http://youtu.be/TKVipuxfVSk

My analysis: The patch provided here in this bug certainly helps reduce the startup time to be *almost* as fast as v1.3. Its possible that we fall short a little bit due to the overall code size increase and it might just be taking longer to load the scripts? (just a guess)

Comment 38

[Diego Marcos [:dmarcos]]

Great job Justin! 

It would be interesting to also collect numbers comparing different OEM builds. I would not discard regressions on the gonk/drivers layers.

Also, how much of the startup time is attributable to the camera controller API (navigator.mozCameras.getCamera)? Has the time to request the camera HW changed across gecko versions since v1.3?

Comment 39

[Wilson Page [:wilsonpage]]

This is awesome analysis Justin thanks! It looks as though my patch does make a smidgen of a difference. As expected between 200-300ms.

Comment 40

[Wilson Page [:wilsonpage]]

Attachment: pull-request (v2.0)

Was a tricky uplift, but it's there. Would be good the have some eyes on it to make sure I haven't missed anything.

Comment 41

[Hema Koka [:hema]]

Justin and Wilson: Thanks a bunch!

Wilson, please Ni Inder/Tapas for feedback/testing on the 2.0 patch before we uplift. 

Thanks
Hema

Comment 42

[Hema Koka [:hema]]

Mike Lee/Gregor, any update on app transition/homescreen delay?

Comment 43

[Wilson Page [:wilsonpage]]

Landed on 'master'

https://github.com/mozilla-b2g/gaia/commit/3d058c42adf6672c244f7e9ee58eba8b762d4dce

Comment 44

[Inder]

(In reply to Wilson Page [:wilsonpage] from comment #40)
> Created attachment 8460933 [details] [review]
> pull-request (v2.0)
> 
> Was a tricky uplift, but it's there. Would be good the have some eyes on it
> to make sure I haven't missed anything.

Tapas -- can you please try out the patch and see what improvement we are seeing?

Comment 45

[Mike Lee [:mlee]]

(In reply to Milan Sreckovic [:milan] from comment #34)
> Mike, you may want to see if bug 1037220 helps there.

Dave, please help in any way that you can on this issue. Confirming what if any impact the changes in bug 1037220 have is a good first start.

(In reply to Hema Koka [:hema] from comment #42)
> Mike Lee/Gregor, any update on app transition/homescreen delay?
No. Let's see what Dave's able to add to this.

Comment 46

[Dave Huseby [:huseby]]

alright, let me take a look.

Comment 47

[Hema Koka [:hema]]

(In reply to Inder from comment #44)
> (In reply to Wilson Page [:wilsonpage] from comment #40)
> > Created attachment 8460933 [details] [review]
> > pull-request (v2.0)
> > 
> > Was a tricky uplift, but it's there. Would be good the have some eyes on it
> > to make sure I haven't missed anything.
> 
> Tapas -- can you please try out the patch and see what improvement we are
> seeing?

Thanks Inder and Tapas for helping with testing, did you try out the 2.0 patch?

Comment 48

[Hema Koka [:hema]]

Inder, we are waiting to uplift the 2.0 patch until we hear feedback from you. Thanks a lot for your help.

Comment 49

[Inder]

Hema -- sure, Tapas is currently looking into it and will respond back shortly with his findings.

Comment 50

[Dave Huseby [:huseby]]

In the datazilla dashboard, the new startup_>_moz-content-interactive test we're measuring a median of 2600 ms consistently.  The old b2gperf cold_load_time is showing a median of 630 ms.  Let me do some more testing with the profiler to see if there is something obvious that is causing the difference in time.

Comment 51

[Diego Marcos [:dmarcos]]

The old b2gperf cold_load_time was only measuring css + js parsing. The camera HW initialization which is the dominant cost was not considered.

Comment 52

[:Eli Perelman]

Instead of moz-content-interactive, we should be using moz-app-visually-complete for official launch regression searching. Values for Camera launch don't currently exist in Datazilla for the v2.0 branch (which I am actively working towards), but for the master branch this value is showing a range of 1550ms to 1700ms [1]. Just remember that the values measured by b2gperf and make test-perf are comparing 2 different metrics, so doing any comparison on the two would most likely be unfruitful. As soon as the patch from bug 1024753 lands we should be able to start seeing launch metrics for v2.0 for Camera.

[1] https://datazilla.mozilla.org/b2g/?branch=master&device=flame&range=7&test=startup_%3E_moz-app-visually-complete&app_list=camera&app=camera&gaia_rev=e5957489dc1a5bfb&gecko_rev=3a41db0b9d88&plot=avg

Comment 53

[:Eli Perelman]

To clarify my previous comment, I need to get the patch uplifted to v2.0, not get it to land. :)

Comment 54

[Hema Koka [:hema]]

(In reply to Inder from comment #49)
> Hema -- sure, Tapas is currently looking into it and will respond back
> shortly with his findings.

Inder/Tapas: Any update?

Comment 55

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Hema Koka [:hema] from comment #54)
> (In reply to Inder from comment #49)
> > Hema -- sure, Tapas is currently looking into it and will respond back
> > shortly with his findings.
> 
> Inder/Tapas: Any update?

I tried that patch but I didn't see any big improvements. I compared both v2.0 and v1.3 device side by side and I can still see more delay in v2.0.

Comment 56

[Diego Marcos [:dmarcos]]

As comment #24 the improvements are around 100ms which is not negligable. How did you measure the improvements?

Comment 57

[Diego Marcos [:dmarcos]]

I think we're ready to land this on 2.0

Comment 58

[Wilson Page [:wilsonpage]]

(In reply to Tapas Kumar Kundu from comment #55)
> I tried that patch but I didn't see any big improvements. I compared both
> v2.0 and v1.3 device side by side and I can still see more delay in v2.0.

We have said from the start that we would not reach v1.3 startup time. This patch bring improvements of 100-300ms depending on devices, which was what was estimated before work began.

Comment 59

[Inder]

Tapas -- when you get a chance please add the exact launch latency measurements after applying the patch.

Comment 60

[Jason Smith [:jsmith]]

Okay, sounds good.

Comment 61

[Hema Koka [:hema]]

> 
> I tried that patch but I didn't see any big improvements. I compared both
> v2.0 and v1.3 device side by side and I can still see more delay in v2.0.

Inder/Tapas: From Justin's comparison videos on flame the improvements seem to be around 200-300ms with the patch, which is still an improvement over previous numbers. Also are you seeing more delays with the patch? If we get this info for QRD as well it would be helpful. 

Since you have already tried out the patch, if you can help provide feedback by running camera tests with patch, we can uplift this into 2.0. I want to make sure there are more eyes on this since it is a patch that touches a bunch of files. 

Also there are couple of other camera blocker bugs that are fixed on top of this patch (waiting on this uplift). So your help is greatly appreciated!

Justin and Diego are also testing out today to ensure we have more folks testing before we uplift. 

Thanks a bunch!
Hema

Comment 62

[Justin D'Arcangelo [:justindarc]]

There is an issue with this patch on v2.0. If the Camera app is not already running and you do one of two things:

- Open Camera from Lockscreen
- Open Camera from Gallery

The viewfinder flickers for a second, then goes black. Toggling Photo/Video mode or Front/Rear camera will restore the viewfinder. But, initially, it is black. Setting NI? for Wilson to investigate.

Comment 63

[Justin D'Arcangelo [:justindarc]]

Wilson, also see GH pull request for comment about another regression that logged some warnings to the console. I already submitted patch for master here in Bug 1045914.

Comment 64

[Wilson Page [:wilsonpage]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #62)
> There is an issue with this patch on v2.0. If the Camera app is not already
> running and you do one of two things:
> 
> - Open Camera from Lockscreen
> - Open Camera from Gallery
> 
> The viewfinder flickers for a second, then goes black. Toggling Photo/Video
> mode or Front/Rear camera will restore the viewfinder. But, initially, it is
> black. Setting NI? for Wilson to investigate.

This is addressed by bug 1045214, we're waiting for this to land before uplifting to 'v2.0'.

(In reply to Justin D'Arcangelo [:justindarc] from comment #63)
> Wilson, also see GH pull request for comment about another regression that
> logged some warnings to the console. I already submitted patch for master
> here in Bug 1045914.

I noticed this too. I've failed to identify what it's actually complaining about.

Comment 65

[Ryan VanderMeulen [:RyanVM]]

I'm a bit confused - comment 43 says this landed on master but the bug is still open?

Comment 66

[Justin D'Arcangelo [:justindarc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #65)
> I'm a bit confused - comment 43 says this landed on master but the bug is
> still open?

Waiting for Inder/Tapas to provide feedback on the v2.0 version of the patch before landing as per Comment 61. As soon as they can give the "ok", this can be landed on v2.0 along with the handful of patches this is blocking.

Comment 67

[Ryan VanderMeulen [:RyanVM]]

But we're good on master, right? Which is what bug resolution tracks (status flags track uplifts).

Comment 68

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Hema Koka [:hema] from comment #61)
> > 
> > I tried that patch but I didn't see any big improvements. I compared both
> > v2.0 and v1.3 device side by side and I can still see more delay in v2.0.
> 
> Inder/Tapas: From Justin's comparison videos on flame the improvements seem
> to be around 200-300ms with the patch, which is still an improvement over
> previous numbers. Also are you seeing more delays with the patch? If we get
> this info for QRD as well it would be helpful. 
> 
> Since you have already tried out the patch, if you can help provide feedback
> by running camera tests with patch, we can uplift this into 2.0. I want to
> make sure there are more eyes on this since it is a patch that touches a
> bunch of files. 
> 
> Also there are couple of other camera blocker bugs that are fixed on top of
> this patch (waiting on this uplift). So your help is greatly appreciated!
> 
> Justin and Diego are also testing out today to ensure we have more folks
> testing before we uplift. 
> 
> Thanks a bunch!
> Hema


Here is the numbers for 256MB msm8610 launch latency (Please don't compare this with Comment 0. Comment 0 is for 512MB build only)
 
	
                          Camera_launch_latency without [1]          Camera_Launch latency with Camera Optimization from [1]
First Launch latency         3181 ms                                        3865 ms
Subsequent launch latency    1186 ms                                        1485 ms


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8460933

So [1] does not help us much here and it is making it worse. Please also note that we saw a big regression between v1.3 and v2.0 (512 MB build, Comment 0). But we are still doing it better in FFOS 2.0 compared to Android on same platform.

Comment 69

[Inder]

Hema -- considering that the patch is making the latency worse, i would suggest not landing it in 2.0 and maybe consider removing it from master.

Diego/Wilson -- why are we seeing the performance go bad with the patch which is supposed to improve it?

Comment 70

[Diego Marcos [:dmarcos]]

Attachment: cameraLaunchTime_v2_0.MOV

Manual camera startup measurement in 2.0

Comment 71

[Diego Marcos [:dmarcos]]

Attachment: cameraLaunchTime_v2_0withPatch.MOV

Manual measurement of camera launch time. gaia v2.0 with patch applied

Comment 72

[Diego Marcos [:dmarcos]]

Tapas, How are you measuring startup times? I cannot see the regression you are observing. I'm using a flame device.

A manual measurement is unreliable when we're talking about 200ms improvements but I've done my best using a stopwatch (videos attached). I'm launching the camera and waiting until the preview is shown. I see gains with the patch applied. These are the numbers:

2.0 without the patch [1]. Video[2]

2.87
2.83
2.74

2.0 with the patch [1] applied. Video [3]

2.60
2.53
2.78

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8460933
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8466910
[3] https://bugzilla.mozilla.org/attachment.cgi?id=8466911

Comment 73

[Hema Koka [:hema]]

(In reply to Inder from comment #69)
> Hema -- considering that the patch is making the latency worse, i would
> suggest not landing it in 2.0 and maybe consider removing it from master.
> 
> Diego/Wilson -- why are we seeing the performance go bad with the patch
> which is supposed to improve it?

Inder,

Is it possible for you to provide videos (similar to the ones that you had sent via email) with and without patch applied? Also let us know how you are measuring, so we can try replicating the same test on Flame too. As Diego, Wilson, Justin have mentioned in previous comments we are seeing a few hundred milliseconds improvement with the patch applied on Flame. 

Thanks
Hema

Comment 74

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Hema Koka [:hema] from comment #73)
> (In reply to Inder from comment #69)
> > Hema -- considering that the patch is making the latency worse, i would
> > suggest not landing it in 2.0 and maybe consider removing it from master.
> > 
> > Diego/Wilson -- why are we seeing the performance go bad with the patch
> > which is supposed to improve it?
> 
> Inder,
> 
> Is it possible for you to provide videos (similar to the ones that you had
> sent via email) with and without patch applied? Also let us know how you are
> measuring, so we can try replicating the same test on Flame too. As Diego,
> Wilson, Justin have mentioned in previous comments we are seeing a few
> hundred milliseconds improvement with the patch applied on Flame. 
> 
> Thanks
> Hema

I am working our internal team to provide you video. They may need to capture video again for you. Please expect some delay 3-4 days for video.

We measure launch latency as touch down to displaying full camera preview. I understand that this will be more clear from video and I am trying to provide video asap.

Comment 75

[Hema Koka [:hema]]

Mike Lee, 

Does your team have a high-speed camera setup to test the difference with and without Wilson's patch? Any help from your team is also appreciated. 

Inder/Tapas are also testing and providing videos of their testing in parallel. 

Thanks
Hema

Comment 76

[Mike Lee [:mlee]]

(In reply to Hema Koka [:hema] from comment #75)
> Mike Lee, 
> 
> Does your team have a high-speed camera setup to test the difference with
> and without Wilson's patch?

Will & Geo, can either of you use Eideticker to help with this?

Comment 77

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

(In reply to Mike Lee [:mlee] from comment #76)
> (In reply to Hema Koka [:hema] from comment #75)
> > Mike Lee, 
> > 
> > Does your team have a high-speed camera setup to test the difference with
> > and without Wilson's patch?
> 
> Will & Geo, can either of you use Eideticker to help with this?

Given the need to switch between two builds to A/B, etc., probably just as easy to run it manually under camera as do an Eideticker test. The results likely need manual review regardless. My first opportunity to do this would probably be Wednesday or Thursday (not currently in office).

Comment 78

[Dave Huseby [:huseby]]

Geo,
If you can confirm this...which I assume you will, ni? me again and I'll take a shot at profiling it and see if I can identify what is taking longer.  Do we have a list of bugs that changed the gecko camera handling code?  Do we think this might be due to a driver update?

Comment 79

[William Lachance (:wlach)]

Looks like Geo has this one.

Comment 80

[Hema Koka [:hema]]

We cannot realistically compare 1.3 and 2.0 camera because v1.3 camera is a bare-bone camera and v2.0 has several new features. At this point, Camera gaia devs have done what they can to optimize where possible and landed the fixes in master. If there are any further ideas for optimizations, please let us know.

As per comment 68, Tapas mentioned that, "...we are still doing it better in FFOS 2.0 compared to Android on same platform". Given this input, this bug can be removed as a blocker for 2.0 release? Inder: as we discussed yesterday can you provide your input here.

We have been holding up a few other bug fixes that we were fixed a week ago on top of this patch hoping this patch will be uplifted soon based on feedback from testing. Removing dependency of other bugs on this one and creating 2.0 specific patches for uplift so we can close out the other bugs that were fixed a while back. 

In either case (whether we make this particular bug a blocker or not), we need to:

1. Run the comparison of launch latency with and without Wilson's improvements (landed in 2.1) and get the test/high speed camera set-up internally that compares to CAF testing so we can efficiently reproduce/test/fix bugs raised by CAF. (Tapas mentioned that he will provide videos and more info in a few days). 

2. Milan mentioned in an earlier comment that bug 1037220 will help speed up the app transition from homescreen. If we can get help from mlee or gregor's test to test it out, it would be awesome. 


Mike, we need your team's help for the above (Perhaps we should track these in separate bugs). Thanks a bunch!

Hema

Comment 81

[:Eli Perelman]

I'd be happy to capture the launch latency comparisons necessary, I just need to know which two revisions I'd be comparing (e.g. v2.0 vs. master). Please confirm and I'll get this piece done.

Comment 82

[Justin D'Arcangelo [:justindarc]]

(In reply to :Eli Perelman from comment #81)
> I'd be happy to capture the launch latency comparisons necessary, I just
> need to know which two revisions I'd be comparing (e.g. v2.0 vs. master).
> Please confirm and I'll get this piece done.

Eli: I think what we're looking for is v2.0 vs. v2.0 + this patch.

Comment 83

[:Eli Perelman]

Sounds good, I'll report back with v2.0 timings compared with the applied "pull-request (v2.0)".

Comment 84

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

Re-adding my own needinfo for the camera tests. 

Eli, if you end up with ready-to-flash builds based off your testing, you might save me some time and trouble if I could use them.

Comment 85

[Inder]

Not a CAF blocker anymore. As per our understanding any changes discussed here will be evaluated for 2.1.

Comment 86

[Hema Koka [:hema]]

Based on the call with Inder on 8/7 moving this out of 2.0. 

Wilson's patch landed on master (2.1) and per conversation with Mike yesterday, Geo from his team is helping test per comment 80

Thanks
Hema

Comment 87

[Diego Marcos [:dmarcos]]

This has landed already on master (2.1). Can we close the bug then?

Comment 88

[Hema Koka [:hema]]

We can mark this resolved since patch landed on master but we need to ensure that testing is done to verify the fixes. Adding Geo as QA contact.

Thanks
Hema

Comment 89

[Geo Mealer [:geo] -- This account is inactive after 2015-07-07]

So it sounds like rather than doing custom builds, I should pick up something pre-landing and something post-landing and just verify the bug resolution. Sound right?

Comment 90

[:Eli Perelman]

The 2.0 patch has merge conflicts. Could we resolve this so we can test 2.0 against the patch?

Comment 91

[Wilson Page [:wilsonpage]]

Comment on attachment 8460933 [details] [review]
pull-request (v2.0)

Rebased against 'v2.0'

Comment 92

[David Flanagan [:djf]]

In CAFs testing they found that the patch for the 2.0 branch increases launch time rather than decreasing it. This is no longer a 2.0 blocker and we should probably not uplift it at this point

Comment 93

[Hema Koka [:hema]]

CAF testing numbers shows 238ms increase with fix. 

And yes this is tracked for 2.1 (not a 2.0 blocker). Adding NO_UPLIFT flag as well. 

Thanks
Hema

Comment 94

[:Eli Perelman]

Using:
  GAIA_OPTIMIZE=1
  APP=camera RUNS=20 make test-perf


Against v2.0:
  "mozPerfDurationsAverage": 1486.658941368421

Against v2.0 + patch:
  "mozPerfDurationsAverage": 1510.2246663

Comment 95

[Diego Marcos [:dmarcos]]

What device are you running on? KK or JB? What does test-perf exactly measure? QC's and our tests give all results over 2 seconds on launch time.

Comment 96

[:Eli Perelman]

I am assuming JB since the KK builds are pretty fresh and I haven't done anything special to update to it. These measurements were all taken using a 319MB Flame, and using `make test-perf` captures timing information from the moment the home screen receives the touch event and emits the launch method to moment the "moz-app-visually-complete" event is triggered by the application [1]. The app emits the "moz-app-visually-complete" event once it designates that it is visually loaded, e.g. the "above-the-fold" content exists in the DOM and it has been marked as ready to be display (not display: none; or other hiding functionality).

Important to note this measurement does not include approximately 125-150ms of latency from the touch driver to invocation of the "touchend/click" event.

Let me know if you have more questions or need further clarification.

[1] https://developer.mozilla.org/en-US/Apps/Build/Performance/Firefox_OS_app_responsiveness_guidelines

Comment 97

[Wilson Page [:wilsonpage]]

(In reply to :Eli Perelman from comment #94)
> Using:
>   GAIA_OPTIMIZE=1
>   APP=camera RUNS=20 make test-perf
> 
> 
> Against v2.0:
>   "mozPerfDurationsAverage": 1486.658941368421
> 
> Against v2.0 + patch:
>   "mozPerfDurationsAverage": 1510.2246663

As part of the patch I found that we were firing some performance-events prematurely, including "moz-app-visually-complete". We were not waiting for the camera preview stream to actually 'start' before firing the event, this meant that the app was 'visually-complete' about 200-300ms after claiming to be so.

My patch fixed this to fire the event once the preview stream has started, but would of course lead to the Camera app appearing to have regressed. My optimizations (namely minifying code and switching module loader lib) gave us gains of a similar amount, so the before and after numbers show little change.

Comment 98

[:Eli Perelman]

The important takeaway from Wilson's statement is that with the movement of the performance event locations, launch latency will have appeared to regress but actually may not have had an impact at all (which is why we stressed having these events in the correct location to begin with, but I digress). At this point, camera verification will be needed to see if actual user-impacting regression has occurred with the patch applied.

Comment 99

[Diego Marcos [:dmarcos]]

What Wilson means is that the patch actually improves the startup time. The manual measurements in comment #72 confirm the gains of around 200ms on Flame JB. 

Eli, having the event in the correct location is what we were trying to do but defining and agreeing on what we want to exactly measure has required a bit of trial and error. I'm sorry for the confusion.

Several independent measurements confirm that the patch on 2.0 actually brings gains in launch time.

Discrepancies between CAF's and Mozilla's numbers might be due to differences betweeen JB and KK or/and differences in the measurement strategy. Also we don't know if CAF has been measuring with the GAIA_OPTIMIZE flag enabled. Can CAF provide a detailed description of their measurement protocol so we can reproduce and verify results?

Comment 100

[Hema Koka [:hema]]

(In reply to Diego Marcos [:dmarcos] from comment #99)
> What Wilson means is that the patch actually improves the startup time. The
> manual measurements in comment #72 confirm the gains of around 200ms on
> Flame JB. 
> 
> Eli, having the event in the correct location is what we were trying to do
> but defining and agreeing on what we want to exactly measure has required a
> bit of trial and error. I'm sorry for the confusion.
> 
> Several independent measurements confirm that the patch on 2.0 actually
> brings gains in launch time.
> 
> Discrepancies between CAF's and Mozilla's numbers might be due to
> differences betweeen JB and KK or/and differences in the measurement
> strategy. Also we don't know if CAF has been measuring with the
> GAIA_OPTIMIZE flag enabled. Can CAF provide a detailed description of their
> measurement protocol so we can reproduce and verify results?

This patch *is not* going into 2.0. We have already landed in master. Lets mark this fixed and have QA run the perf measurements for master (2.1). 

Per Tapas's comment earlier, they are measuring latency as touch down to camera preview. We can talk to them offline to see if they can provide more details. The videos that were sent unfortunately does not provide details on when clock starts and stops for measuring latency. 

NI Tapas to clarify if the 2.0 measurements were taken with GAIA_OPTIMIZE flag enabled or not?

Thanks
Hema

Comment 101

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8459611 [details] [review]
pull-request (master)

Clearing r? since this was already r+'d.

Comment 102

[Tapas[:tkundu on #b2g/gaia/memshrink/gfx] (always NI me)]

(In reply to Hema Koka [:hema] [OOO 8/20 to 8/29] from comment #100)
> NI Tapas to clarify if the 2.0 measurements were taken with GAIA_OPTIMIZE
> flag enabled or not?
> 

We are using GAIA_OPTIMIZE flag for all kind of launch latency measurements.

Comment 103

[Ryan VanderMeulen [:RyanVM]]

So this and bug 1045214 are officially wontfix for v2.0 then?

Comment 104

[Hema Koka [:hema]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #103)
> So this and bug 1045214 are officially wontfix for v2.0 then?

Yes the work in this bug is not going into 2.0. It landed on master before 2.1 branch cut, so it is going out in 2.1 release. And yes bug 1045214 is also a wontfix in v2.0 because it is an issue only if patch in this bug lands. 

Thanks
Hema

Comment 105

[Shally Li（leave from Mozilla）]

Hi,
   On Flame v2.1,fLash ROM and then camera launch latency is : 2.40/2.42/2.34 secs
   1.Occurrence time：3/3
   2.Attch:logcat_1131.txt and 2.1_1131.mp4.

Flame 2.1 build:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 106

[Shally Li（leave from Mozilla）]

Attachment: logcat_1131.txt

Comment 107

[Shally Li（leave from Mozilla）]

Attachment: 2.1_1131.mp4