934657 - [Camera] Restructuring code following model / view pattern

Status: RESOLVED FIXED

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

Comment 1

[Diego Marcos [:dmarcos]]

Introducing a model / view pattern to layout the code. This will improve current separation of concerns making it easier for multiple developers to simultaneously work on different features.

Comment 2

[James Lal [:lightsofapollo]]

I 100% support this effort but ensure to take into account start-up performance (maybe even improve it :)) camera startup time is very sensitive (potentially release blocking).

Comment 3

[Diego Marcos [:dmarcos]]

Do we have a standard test to measure start-up performance? I would like to keep an eye on the numbers as we make progress with this.

Comment 4

[James Lal [:lightsofapollo]]

There are some settings to turn this on in the developer settings (like where you turn on adb [remote debugging)] not 100% sure what they are called right now... I would suggest adding a perf team person to the review to be extra careful.

Comment 5

[James Lal [:lightsofapollo]]

I did a few different things when I profiled this back in Jan

- use console.log + time hacks
- use profiler and watch the time between process startup and video activity (probably best option)
- use built in "green number" app startup time (this is not the true startup time)

Partners use high speed cameras to test this kind of thing so its hard to reproduce the exact conditions but its something to be aware of.

When profiling is critical that the profile is built with GAIA_OPTIMIZE=1 so you can test under production like conditions. ( I have no idea if all this stuff is in a wiki now this is simply my brain dump)

Comment 6

[Kevin Grandon :kgrandon]

I think you want bug 915158 to be fixed so we have test-perf on a device again.

I support a cleaner architecture, but we should definitely not regress load times for a core app like camera. I left some notes on github and will continue to follow this issue.

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to James Lal [:lightsofapollo] from comment #5)
> 
> Partners use high speed cameras to test this kind of thing so its hard to
> reproduce the exact conditions but its something to be aware of.

Diego, if you look at bug 915083, I used a GoPro Hero 2 camera to capture the start-up times of the camera using its 640x480x240fps mode. The picture quality isn't great, but its capture rate is 3x faster than the refresh rate of the LCD on our current devices (inari, hamachi) so it's easy to make accurate measurements of when events occur.

The Hero 3 refresh is out now, so you can probably find a Hero 2 cheap.

Comment 8

[Diego Marcos [:dmarcos]]

I'm trying to come with a rigorous approach to measure the camera startup time. It would be nice if you can validate this method or suggest an alternative. 

Measuring protocol:

1. I'm using timestamps(new Date().getTime()) to measure time. 
2. I first record the timestamp when the function Page.tap is invoked. Page.tap handles tap events on the home screen.
3. I record a second timestamp when the callback of requirejs/LazyLoader in the camera app is called.
4. I substract both timestamps to get the number of miliseconds. This is what I consider startup time.

I've observed that startup times are not stable. They range from 4870ms to 3433ms. Almost 1.5 seconds difference between fastest and slowest. Do you know why?

I made repeated measurements and averaged the results for both cases: with LazyLoder and requirejs. I wanted to reproduce the same conditions for each camera start so I rebooted the phone before each measure. Below a link to the video of the sequence I followed for each measurement:

http://www.youtube.com/watch?v=OHfJDGVC17g

I took 10 measurements for each case. The averages for each scenario are:

LazyLoader: 4534.7 (4870, 4613, 4589, 3834, 4746, 4657, 4746, 4500, 4304, 4488)
Requirejs: 4531.3 (4776, 3522, 3433, 4771, 5112, 4784, 4682, 4631, 4760, 4842)

I don't see any startup time regression in the case of the Camera App.

Am I doing things right?

Comment 9

[Kevin Grandon :kgrandon]

That is a fairly big range, but the methodology seems good to me! I'm not entirely sure why you're seeing that kind of range, or if it's expected.

How long are you waiting before launching each time? The pre-allocated process takes ~5s to spin up after app launch, and if you're not waiting for that, it will impact the launch times you're seeing.

Comment 10

[James Burke [:jrburke]]

What I have measured before: the TTL number that you can display via the Developer preferences in the system app, "Show time to load", and a green box will show up in the upper right corner.

While that number is a bit coarse, it just measures a load event that may not actually reflect the user seeing something useful, so use with caution. But also, ideally we have something usable when that number is generated. I believe it is the one that is measured in this graph:

https://datazilla.mozilla.org/b2g#closedataset

I also have used a timing based on navigation.timing, and wrap it in a plog command:

https://github.com/mozilla-b2g/gaia/blob/b12d548835f49db65e476311522f1a927ac2ad5f/apps/email/js/html_cache_restore.js#L3

and then I plog() when I wanted to get timings related to the start of navigation.timing.

Comment 11

[Justin D'Arcangelo [:justindarc]]

Attachment: GitHub Pull Request

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

Please don't land this until I've had a chance to look at launch-time numbers.

Comment 13

[Diego Marcos [:dmarcos]]

Mike,

No worries. This has been pushed back to 1.4. I just removed the review flag of a pull request that doesn't exist anymore.

Comment 14

[Diego Marcos [:dmarcos]]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/c38d3842daa97cea360fce1af82f2d7b78134bc3

Comment 15

[James Lal [:lightsofapollo]]

Looks like this breaks the b2g build https://tbpl.mozilla.org/php/getParsedLog.php?id=32456669&tree=B2g-Inbound&full=1... The gaia profile is built into the b2g desktop build which can cause this kind of thing where it will only fail when building gecko (which happens after your travis run and is not reported to the same place).

Comment 16

[James Lal [:lightsofapollo]]

Reverted: https://github.com/mozilla-b2g/gaia/commit/e41e5f95f818b1079ad3dde1d2b90e219c11e5da

Comment 17

[Wilson Page [:wilsonpage]]

I'm not really sure where to start with this:

make[7]: *** [../../build_stage/camera/js/main.js] Error 1
make[6]: *** [app-makefiles] Error 1
make[5]: *** [libs] Error 2
make[4]: *** [b2g/gaia/libs] Error 2
make[3]: *** [libs] Error 2
make[2]: *** [default] Error 2
make[1]: *** [realbuild] Error 2
make: *** [build] Error 2

It doesn't indicate what the error actually is. Can point me in the right direction?

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Mike Habicher [:mikeh] from comment #12)
> Please don't land this until I've had a chance to look at launch-time
> numbers.

Did we get answers on the TTL ?

Also it seems like this refactoring use alameda, which is also use in clock and in email. Should it not move to shared/js/ in this case instead ? Same for all things that are used in multiple apps.

Comment 19

[Wilson Page [:wilsonpage]]

It seems as though the build is failing due to the camera Makefile's dependency on nodejs. The r.js optimizer step was previously being run using XPCSHELLSDK (same as clock and email), this was changed by dmarcos yesterday. We'll find out why this change was made, then hopefully fix and resubmit the patch.

Comment 20

[Diego Marcos [:dmarcos]]

I made our Makefile to use node to run the r.js optimizer instead of XPCSHELLSDK. I wanted to decouple our Makefile from the main Makefile and from XPCSHELLSDK. I think having the apps decoupled from the gaia build system would be a good thing. I see a near future where apps can live in its own repos. Any person can checkout those apps, build them using widespread tools and easily run them in any browser, desktop or mobile. Just like any other web app.

I've been very cautious and I only landed this after seeing green on Travis. I didn't know that this would break TBPL. 

https://github.com/mozilla-b2g/gaia/pull/13363

Unit tests depend on node. Are those tests not run in the same environment? Is it possible to have node available in TBPL?

Comment 21

[Diego Marcos [:dmarcos]]

(In reply to Vivien Nicolas (:vingtetun) (:21) from comment #18)
> (In reply to Mike Habicher [:mikeh] from comment #12)
> > Please don't land this until I've had a chance to look at launch-time
> > numbers.
> 
> Did we get answers on the TTL ?
> 
> Also it seems like this refactoring use alameda, which is also use in clock
> and in email. Should it not move to shared/js/ in this case instead ? Same
> for all things that are used in multiple apps.

I made a lot of measurements on startup times using b2gperf. I should have shared those numbers in this bug prior to landing the patch. There's a known small regression of 79ms average in the refactoring. We now concatenate and load all the scripts up front and before some of them were loaded lazily. These are the numbers:

REFACTOR

_time: median:699, mean:702, std: 22, max:782, min:672

MASTER PRIOR TO REFACTOR

Results for Camera, cold_load_time: median:623, mean:623, std: 14, max:664, min:593

This is going to be fixed soon but we want to land this patch now since there's other work depending on it. We postponed this bug to 1.4 so we could merge early in the cycle and have plenty of time to deal with possible regressions and optimizations later on and before 1.4 branches.

Comment 22

[Kevin Grandon :kgrandon]

FYI - Any performance regressions here will likely result in this patch being backed out.

Comment 23

[Wilson Page [:wilsonpage]]

(In reply to Diego Marcos from comment #20)
> I made our Makefile to use node to run the r.js optimizer instead of
> XPCSHELLSDK. I wanted to decouple our Makefile from the main Makefile and
> from XPCSHELLSDK. I think having the apps decoupled from the gaia build
> system would be a good thing. I see a near future where apps can live in its
> own repos. Any person can checkout those apps, build them using widespread
> tools and easily run them in any browser, desktop or mobile. Just like any
> other web app.
> 
> I've been very cautious and I only landed this after seeing green on Travis.
> I didn't know that this would break TBPL. 
> 
> https://github.com/mozilla-b2g/gaia/pull/13363
> 
> Unit tests depend on node. Are those tests not run in the same environment?
> Is it possible to have node available in TBPL?

We could prefer node and fallback to xpcshell so that we can support both cases.

Comment 24

[Wilson Page [:wilsonpage]]

(In reply to Kevin Grandon :kgrandon from comment #22)
> FYI - Any performance regressions here will likely result in this patch
> being backed out.

1.4 is bringing a lot of new features into the camera app, this was primary reason we needed to refactor in the first place; to give us a foundation we could scale from. Diego indicated there may be a ~79ms startup time regression, but I feel it would be foolish to fine tune this before we have landed the outstanding new features.

We are early in the release cycle. We need to land the refactored app, build in the new features, whilst continually reviewing performance. If we still have a 1.3->1.4 performance regression after we are feature complete, we must identify bottlenecks and fine-tune.

Alternatively we continue working in a 1.4 features branch and don't land on master until feature and performance complete, although this will require that all third parties are aware that in the meantime camera app development cannot be done on master.

Comment 25

[Kevin Grandon :kgrandon]

We have a hard policy of no performance regressions. If it's absolutely impossible to land without regressions, my recommendation would be to follow what we did with 'music2' and keyboard apps. Check-in a 'camera2' application into the test_apps folder, and install it for engineering builds. That will give it some time to bake on master, and folks can continue to develop against it on the master branch.

Comment 26

[Diego Marcos [:dmarcos]]

LG is going to devote resources to develop new camera features and they're going to be working on an obsolete code base. The cost of porting all those contribution to the new code base is going to be greater than temporary live with extra 79ms of startup time. We can mark the regression as a blocker for 1.4

Comment 27

[Dietrich Ayala (:dietrich)]

Diego and Wilson: Sorry, but please do the requisite profiling and resolve the regression before landing.

Like Kevin said, the policy for performance regressions is zero-tolerance.

Comment 28

[Diego Marcos [:dmarcos]]

Attachment: Pull Request

Comment 29

[Diego Marcos [:dmarcos]]

Changed Makefile to use xpcshell if available and use node as fall back. This way the camera Makefile is still decoupled from the main Makefile but still works in a tpcl environment.

Comment 30

[Diego Marcos [:dmarcos]]

Merged in master:

https://github.com/mozilla-b2g/gaia/commit/c238c7705453cbf8a3109f2e610adbf74b3f7555

Comment 31

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

Hi Diego, where do you require GAIA_DIR/build/camera-configuration.js?

Comment 32

[Diego Marcos [:dmarcos]]

I had factored out the camera configuration to decouple the camera Makefile from the main gaia Makefile. I ended up with a different script in the camera app GAIA_DIR/apps/camera/build/configure.js. 

camera-configuration.js is not needed anymore. We can get rid of it.

I created a bug to keep track of the change. Can you please review the patch?

https://bugzilla.mozilla.org/show_bug.cgi?id=958590

Comment 33

[Yuren [:yurenju] (AFK ~ Oct. 12) (volunteer mode)]

got it.

BTW you did a great job for refactoring camera app!