Closed Bug 1037322 Opened 5 years ago Closed 5 years ago

[dolphin][flame][perf] improve the performance of switch cameras

Categories

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

Other
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.4+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
2.1 S2 (15aug)
blocking-b2g 1.4+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: angelc04, Assigned: mikeh, NeedInfo)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s=2014.08.15.t u=] [sprd332042][partner-blocker][perf])

Attachments

(3 files, 4 obsolete files)

Here is the avg time for switching cameras in Camera App:
Dolphin: avg 2.2s
7715 android: avg1.14
Flame v1.4: avg 2.2s

Buri does not have front camera thus unable to test.Following is an analysis from sprd developer.


log1: this.viewfinder.fadeOut(this.camera.load);
      It set 300ms and then run this.camera.load
log2: this.mozCamera.release(onSuccess, onError);
      It took 0.15s on return success .
log3: navigator.mozCameras.getCamera(camera, {}, onSuccess, onError);
      It took 0.41s on return success
log4: navigator.mozSettings.createLock().get('deviceinfo.hardware');
      took 0.2s on return onSuccess
log5: this.mozCamera.setConfiguration(options, success, error);
      It took 0.72s to return success

log1:
07-10 01:45:27.303 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:1953 in module.exports<.onCameraClick: fxh_onCameraClick_end
07-10 01:45:27.603 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3072 in Camera.prototype.load: fxh_Camera.prototype.load_begin

log2:
07-10 01:45:27.603 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3112 in Camera.prototype.load: fxh_Camera.prototype.load_end
07-10 01:45:27.753 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3300 in onSuccess: fxh_this.mozCamera.release_onSuccess_begin

log3:
07-10 01:45:27.753 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3305 in onSuccess: fxh_this.mozCamera.release_onSuccess_end
07-10 01:45:28.253 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3130 in onSuccess: fxh_onSuccess_mozCamera_begin1

log4:
07-10 01:45:28.423 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:12997 in CameraController.prototype.onSettingsConfigured: fxh_hardware_begin
07-10 01:45:28.623 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:13001 in CameraController.prototype.onSettingsConfigured/hardware.onsuccess: fxh_hardware.onsuccess_begin

log5:
07-10 01:45:28.403 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3203 in Camera.prototype.configure: fxh_mozcamera_setconfiguration_6
07-10 01:45:28.623 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:13017 in CameraController.prototype.onSettingsConfigured/hardware.onsuccess: fxh_hardware.onsuccess_end
07-10 01:45:29.123 E/GeckoConsole( 2503): Content JS LOG at app://camera.gaiamobile.org/js/main.js:3183 in Camera.prototype.configure/success: fxh_success_begin
Whiteboard: [sprd332042][partner-blocker][perf]
(In reply to pcheng from comment #0)
> Here is the avg time for switching cameras in Camera App:
> Dolphin: avg 2.2s
> 7715 android: avg1.14
> Flame v1.4: avg 2.2s
> 
> Buri does not have front camera thus unable to test.Following is an analysis
> from sprd developer.
> 
> 
> log1: this.viewfinder.fadeOut(this.camera.load);
>       It set 300ms and then run this.camera.load
> log2: this.mozCamera.release(onSuccess, onError);
>       It took 0.15s on return success .
> log3: navigator.mozCameras.getCamera(camera, {}, onSuccess, onError);
>       It took 0.41s on return success
> log4: navigator.mozSettings.createLock().get('deviceinfo.hardware');
>       took 0.2s on return onSuccess
> log5: this.mozCamera.setConfiguration(options, success, error);
>       It took 0.72s to return success
> 

Added log point in gaia to profile the performance, two major log points are listed to be the top two to consume performance.

1. Time consumes between this.mozCamera.setConfiguration and its callback. It costs about 1000~1200ms
2. Time consumes between Camera.prototype.requestCamera and its callback. It costs about 300~500ms.

I will than narrow down the top 1 case to check if I can see more info.
blocking-b2g: --- → 1.4?
Attached patch WIP-bug-1037322.patch (obsolete) — Splinter Review
Hi Mike,

From the profiling reset of Camera on Dolphin, I found StartImpl() and SetConfigurationImpl() both calling to StartPreviewImpl(). In more detail of observation, camera initialization and back/front camera switching both called to StartPreviewImpl(). It seems there is an redundant code flow we can improve it. I tried to remove StartPreviewImpl() in StartImpl() because I think it is better we calls it after configuration is done. After the WIP, it can save about 300~500ms.

Can you please give me any feedback for the modification? Maybe there is some other user cases I didn't consider. Thanks
Attachment #8458493 - Flags: feedback?(mhabicher)
Similar performance between Flame and Dolphin, not blocking for 1.4 but will be happy to see improvement.

Vincent, when your patch is good to go we can set a request for approval to 1.4
blocking-b2g: 1.4? → -
(In reply to Vincent Liu[:vliu] from comment #2)

> From the profiling reset of Camera on Dolphin, I found StartImpl() and
> SetConfigurationImpl() both calling to StartPreviewImpl(). In more detail of
> observation, camera initialization and back/front camera switching both
> called to StartPreviewImpl(). It seems there is an redundant code flow we
> can improve it. I tried to remove StartPreviewImpl() in StartImpl() because
> I think it is better we calls it after configuration is done. After the WIP,
> it can save about 300~500ms.

Vincent, just so I understand: are you saying that you're seeing StartImpl() make it here[1] and then subsequently seeing the same call here[2]? If so, that's a problem with the Camera app: it should either be passing in a (cached) initial configuration to getCamera(), xor not passing an initial configuration to getCamera(), and instead calling setConfiguration().

1. http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#125
2. http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.cpp#280
Flags: needinfo?(wilsonpage)
I did some poking around in the camera front-end and found everything to be as expected:

- Camera startup *without* cached config will make a request to `.getCamera()` and then a second request to `.setConfiguration()` once capabilities are known.

- Camera startup *with* cached config will make a single request to `.getCamera()` passing config.

- Switching cameras will always make a request to `.getCamera()` followed by a second to `.setConfiguration()`. This is perhaps something we could look to optimize by caching front-camera config too. But as this doesn't affect startup, is less critical.

- Viewfinder fade-in is delayed due to a bug whereby preview-stream still shows old stream for a few frames even after `previewStateChange` has fired. This means that when we fade viewfinder back in, we see a glitchy flicker. I'm afraid I don't know the bug number associated with this, but I believe (hope) mikeh might :) If this is fixed, we can instantly get a 300ms improvement on mode and camera switching.
Flags: needinfo?(wilsonpage)
(In reply to Mike Habicher [:mikeh] from comment #4)
> (In reply to Vincent Liu[:vliu] from comment #2)
>  
> Vincent, just so I understand: are you saying that you're seeing StartImpl()
> make it here[1] and then subsequently seeing the same call here[2]? If so,
> that's a problem with the Camera app: it should either be passing in a
> (cached) initial configuration to getCamera(), xor not passing an initial
> configuration to getCamera(), and instead calling setConfiguration().
> 
> 1.
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> cpp#125
> 2.
> http://dxr.mozilla.org/mozilla-central/source/dom/camera/GonkCameraControl.
> cpp#280

Yes, I can [1] runs first and then subsequently seeing the same call here[2]. It always do when launching the camera app and switching front/back camera. 

(In reply to Wilson Page [:wilsonpage] from comment #5)
> I did some poking around in the camera front-end and found everything to be
> as expected:
> 
> - Camera startup *without* cached config will make a request to
> `.getCamera()` and then a second request to `.setConfiguration()` once
> capabilities are known.
> 
> - Camera startup *with* cached config will make a single request to
> `.getCamera()` passing config.
> 
> - Switching cameras will always make a request to `.getCamera()` followed by
> a second to `.setConfiguration()`. This is perhaps something we could look
> to optimize by caching front-camera config too. But as this doesn't affect
> startup, is less critical.
> 
> - Viewfinder fade-in is delayed due to a bug whereby preview-stream still
> shows old stream for a few frames even after `previewStateChange` has fired.
> This means that when we fade viewfinder back in, we see a glitchy flicker.
> I'm afraid I don't know the bug number associated with this, but I believe
> (hope) mikeh might :) If this is fixed, we can instantly get a 300ms
> improvement on mode and camera switching.

Does this code behavior also for v1.4 branch? Actually I found this issue on v1.4 branch.
Flags: needinfo?(wilsonpage)
(In reply to Vincent Liu[:vliu] from comment #6)
> Does this code behavior also for v1.4 branch? Actually I found this issue on
> v1.4 branch.

Ah sorry I'm referring to master only. The performance tuning (which involved cached config) landed in v2.0.
Flags: needinfo?(wilsonpage)
(In reply to Wilson Page [:wilsonpage] from comment #7)
> (In reply to Vincent Liu[:vliu] from comment #6)
> > Does this code behavior also for v1.4 branch? Actually I found this issue on
> > v1.4 branch.
> 
> Ah sorry I'm referring to master only. The performance tuning (which
> involved cached config) landed in v2.0.

Is it possible to uplift it to V1.4? I believe it is an common issue and can improve the performance as Comment 2 said.
The performance patch that landed in v2.0 was massive, it touched almost every file in the app. If you need this perf boost in v1.4 I would suggest uplifting the entire v2.0 camera. But there may be reasons why this is not a good idea.
[Blocking Requested - why for this release]:
blocking-b2g: - → 1.4?
Steven, can you try this patch?
Flags: needinfo?(steven.ouyang)
Keywords: perf
Priority: -- → P3
Whiteboard: [sprd332042][partner-blocker][perf] → [sprd332042][partner-blocker][perf][c=progress p= s= u=]
blocking-b2g: 1.4? → 1.4+
(In reply to Wilson Page [:wilsonpage] from comment #9)

> The performance patch that landed in v2.0 was massive, it touched almost
> every file in the app. If you need this perf boost in v1.4 I would suggest
> uplifting the entire v2.0 camera. But there may be reasons why this is not a
> good idea.

One option: we could apply attachment 8458593 [details] [diff] [review] to gecko_1.4 *ONLY*.
(In reply to Mike Habicher [:mikeh] from comment #12)

> One option: we could apply attachment 8458593 [details] [diff] [review] to gecko_1.4 *ONLY*.

That should, of course, read attachment 8458493 [details] [diff] [review]. According to comment 9, in 1.4, we always wind up calling setConfiguration(), the patch shouldn't break anything. Of course, it will break the API as described in the WebIDL file.

I would rather we fix the app, somehow.
Flags: needinfo?(wilsonpage)
(In reply to Mike Habicher [:mikeh] from comment #13)
> (In reply to Mike Habicher [:mikeh] from comment #12)
> 
> > One option: we could apply attachment 8458593 [details] [diff] [review] to gecko_1.4 *ONLY*.
> 
> That should, of course, read attachment 8458493 [details] [diff] [review].
> According to comment 9, in 1.4, we always wind up calling
> setConfiguration(), the patch shouldn't break anything. Of course, it will
> break the API as described in the WebIDL file.
> 
> I would rather we fix the app, somehow.

Are we talking about startup here?
Flags: needinfo?(wilsonpage)
See Also: → 982230
Comment on attachment 8458493 [details] [diff] [review]
WIP-bug-1037322.patch

Review of attachment 8458493 [details] [diff] [review]:
-----------------------------------------------------------------

f-
I'm going to post a patch that fixes this across all branches.
Attachment #8458493 - Flags: feedback?(mhabicher) → feedback-
Assignee: nobody → mhabicher
Oops--add a missing file.
Attachment #8458493 - Attachment is obsolete: true
Attachment #8465713 - Attachment is obsolete: true
Attachment #8465713 - Flags: feedback?(jdarcangelo)
Attachment #8465742 - Flags: feedback?(jdarcangelo)
Comment on attachment 8465742 [details] [diff] [review]
Don't assume 'picture' mode when no initial config is given, v1.1

Review of attachment 8465742 [details] [diff] [review]:
-----------------------------------------------------------------

This patch yields about a ~200ms improvement for the v1.4 Camera app when switching between front/rear cameras. Note, I didn't do any sort of accurate measurement here, just eyeballed it between 2 identical Flame devices side by side (one with/one without the patch). There also seems to be an improvement when applying this patch to master (v2.1) Camera app, although not as significant (maybe ~100ms).
Attachment #8465742 - Flags: feedback?(jdarcangelo) → feedback+
khuey, the DOM-facing changes are in CameraManager.webidl and DOMCameraControl.cpp.

dhylands, can you look over the changes in GonkCameraControl.cpp?

This patch includes a new (passing :) test.

tl;dr: without this change, a call to getCamera() with no initial configuration defaults to picture mode. With this change, it defaults to 'unspecified', and doesn't try to start the viewfinder until setConfiguration() is called.

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=83461e56528d
Attachment #8465742 - Attachment is obsolete: true
Attachment #8465767 - Flags: review?(khuey)
Attachment #8465767 - Flags: review?(dhylands)
Attachment #8465767 - Flags: feedback?(jdarcangelo)
Comment on attachment 8465767 [details] [diff] [review]
Don't assume 'picture' mode when no initial config is given, v1.2

Review of attachment 8465767 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me (except for my one paranoidish comment)

::: dom/camera/DOMCameraControl.cpp
@@ +167,5 @@
>      new DOMCameraConfiguration(aInitialConfig);
>  
>    // Create and initialize the underlying camera.
>    ICameraControl::Configuration config;
> +  bool haveInitialConfig = true;

Shouldn't this be initialized to false and then set to true when you get a config?

If it received something that hits the default on the release build then it will fall through with haveInitialConfig set to true.
Attachment #8465767 - Flags: review?(dhylands) → review+
Comment on attachment 8465767 [details] [diff] [review]
Don't assume 'picture' mode when no initial config is given, v1.2

Review of attachment 8465767 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the webidl
Attachment #8465767 - Flags: review?(khuey) → review+
Flags: needinfo?(renfeng.mei)
The latest test result base on FIREFOXOS_V1.4_W14.31.4_SP7715:
2.01   1.69   1.79   AVG:1.83s

The android7715 time is 1.14s.
Hi Jason and Renfeng,the latest test result revealed lots of improvement compared to the first trial statistics(below) :
Dolphin: avg 2.2s-->  0.37 sec improvement
7715 android: avg1.14
Flame v1.4: avg 2.2s

Besides, please also provide the data of Buri for reference.
(In reply to Rachelle Yang [:ryang][ryang@mozilla.com] from comment #23)
> Hi Jason and Renfeng,the latest test result revealed lots of improvement
> compared to the first trial statistics(below) :
> Dolphin: avg 2.2s-->  0.37 sec improvement
> 7715 android: avg1.14
> Flame v1.4: avg 2.2s
> 
> Besides, please also provide the data of Buri for reference.

Rachelle, buri does not have front camera, thus unable to test.
Final version of patch for b2g-inbound, incorporating review feedback; carrying r+ forward.

Versions for 2.0 and 1.4 coming soon.
Attachment #8465767 - Attachment is obsolete: true
Attachment #8465767 - Flags: feedback?(jdarcangelo)
Attachment #8467897 - Flags: review+
[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 909542
User impact if declined: camera launch can take longer than necessary.
Testing completed: automated test (included in patch) + testing as mentioned in the bug comments.
Risk to taking this patch (and alternatives if risky): low-risk, no change in Camera app required.
String or UUID changes made by this patch: none.
Attachment #8467904 - Flags: approval-mozilla-b2g30?
Comment on attachment 8467904 [details] [diff] [review]
Patch rebased to b2g32_v2_0

Actually request approval for b2g32 instead of b2g30. Previous request for approval comments apply.
Attachment #8467904 - Flags: approval-mozilla-b2g30? → approval-mozilla-b2g32?
This is the version of the patch specifically for dolphin.
https://hg.mozilla.org/mozilla-central/rev/a278dc70b5f8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
Mike,

What improvements are we seeing on Flame 2.0 with your patch?

Thanks
Hema
Flags: needinfo?(mhabicher)
Justin made the actual measurements.
Flags: needinfo?(mhabicher) → needinfo?(jdarcangelo)
(In reply to Hema Koka [:hema] from comment #32)
> Mike,
> 
> What improvements are we seeing on Flame 2.0 with your patch?
> 
> Thanks
> Hema

IIRC, I did a side-by-side comparison of two Flames and found the one with the patch applied was able to switch approx 200ms faster than the one without the patch applied. If you want a more accurate timing comparison, let me know and I can try and capture a video.
Flags: needinfo?(jdarcangelo)
Whiteboard: [sprd332042][partner-blocker][perf][c=progress p= s= u=] → [c=progress p= s=2014.08.15.t u=] [sprd332042][partner-blocker][perf]
NI : Hema, here as I had poked her offline to confirm we need this change in 2.0 given the CAF improvements that are being worked on for camera.
Flags: needinfo?(hkoka)
Inder/Tapas are planning to test it out after the p1 issues are resolved. I just spoke to Inder and leaving a NI on him in this bug for a 2.0 uplift. This is a low-risk change: https://bugzilla.mozilla.org/show_bug.cgi?id=1037322#c27

Justin, 

Is it possible for you to do a quick comparison video for 2.0 with and without the patch and add it here. We are keeping the gates stricter at the point for 2.0 for any changes going in.

Thanks
Hema
Flags: needinfo?(jdarcangelo)
Flags: needinfo?(ikumar)
Flags: needinfo?(hkoka)
(In reply to Hema Koka [:hema] [OOO 8/20 to 8/27] from comment #36)
> Inder/Tapas are planning to test it out after the p1 issues are resolved. I
> just spoke to Inder and leaving a NI on him in this bug for a 2.0 uplift.
> This is a low-risk change:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1037322#c27
> 
> Justin, 
> 
> Is it possible for you to do a quick comparison video for 2.0 with and
> without the patch and add it here. We are keeping the gates stricter at the
> point for 2.0 for any changes going in.
> 
> Thanks
> Hema

I compared side by side two msm8610 devices with and without with patch. I can see that device is switching faster from front camera to back camera with this patch.

Thanks for good work :).
Flags: needinfo?(ikumar)
Comment on attachment 8467904 [details] [diff] [review]
Patch rebased to b2g32_v2_0

Approving for landing in 2.0 based on test results
Attachment #8467904 - Flags: approval-mozilla-b2g32? → approval-mozilla-b2g32+
Flags: needinfo?(jdarcangelo)
You need to log in before you can comment on or make changes to this bug.