1090501 - [Camera][Gecko][Perf] Pre-emptively open camera hardware when certified app has 'camera' permission

Status: RESOLVED FIXED

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

Comment 1

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

Attachment: Pre-emptively open camera hardware, v1

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4690d6c0669b

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 8512977 [details] [diff] [review]
Pre-emptively open camera hardware, v1

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

::: dom/camera/CameraPreferences.cpp
@@ +293,5 @@
> +  if (strcmp(aTopic, "init-camera-hw") == 0) {
> +    return PreinitCameraHardware();
> +  }
> +
> +  printf_stderr("Got unhandled topic '%s'\n", aTopic);

DOM_CAMERA_LOGE instead of printf_stderr?

::: dom/camera/DOMCameraManager.h
@@ +91,5 @@
>  
> +#ifdef MOZ_WIDGET_GONK
> +  static void PreinitCameraHardware();
> +#endif
> +  

nit: whitespace

::: dom/ipc/TabChild.cpp
@@ +1837,5 @@
> +        nsCOMPtr<nsIObserverService> observerService =
> +          mozilla::services::GetObserverService();
> +        observerService->NotifyObservers(nullptr, "init-camera-hw", nullptr);
> +      }
> +    }

This is needs to be improved a bit to:
- check if the app is certified
- add some checks on return values (secMan, NS_NewURI for instance).

Comment 3

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

Attachment: Pre-emptively open camera hardware, v2

Incorporate review feedback.

Comment 4

[[:fabrice] Fabrice Desré]

Comment on attachment 8513805 [details] [diff] [review]
Pre-emptively open camera hardware, v2

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

r=me with nit fixed.

::: dom/ipc/TabChild.cpp
@@ +1826,5 @@
> +
> +    uint16_t status = nsIPrincipal::APP_STATUS_NOT_INSTALLED;
> +    principal->GetAppStatus(&status);
> +    bool isCertified = status == nsIPrincipal::APP_STATUS_CERTIFIED;
> +    if (NS_WARN_IF(!isCertified)) {

No need to warn here - nothing bad happened.

Comment 5

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

(In reply to Fabrice Desré [:fabrice] from comment #4)

> No need to warn here - nothing bad happened.

Agreed. I was thinking I can probably remove the warning from the permissions check as well, for the same reason.

Comment 6

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

Attachment: Pre-emptively open camera hardware, v2.1 r=fabrice

Incorporate review feedback, carrying r+ forward.

Comment 7

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

https://hg.mozilla.org/integration/b2g-inbound/rev/506d33f3931e

Comment 8

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

And the missing headers that didn't break the unified build:
https://hg.mozilla.org/integration/b2g-inbound/rev/4cb1154b0190

Comment 9

[Ryan VanderMeulen [:RyanVM]]

This conflicted with the backout of bug 1020368, so I had to back this out too :(
https://hg.mozilla.org/integration/b2g-inbound/rev/940af94c4bc6

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Oh geez, this gets worse. Your bustage follow-up here fixed the non-unified bustage from that push too :(

So it sounds like you should roll the bustage fix here into whatever lands first and then we should be good to go. Sorry for the mess :(

Comment 11

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

Take 2, now with (hopefully) 100% less bustage:
https://hg.mozilla.org/integration/b2g-inbound/rev/036de9b00df6

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/036de9b00df6

Comment 13

[Robert Chira [:RobertC]]

This is breaking the camera app. The automation suite is failing because after opening the camera app a loading spinner is displayed and does not disappear. The issue is also reproduced manually 5 out of 5 attempts.

The tests first failed in the following Jenkins run:
http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk-319.b2g-inbound.ui.functional.smoke/984/HTML_Report/

Is there another patch that should have landed together with this?
If not, please back out this patch.

Comment 14

[Robert Chira [:RobertC]]

Attachment: logcat.txt

This was tested on Flame 319MB shallow flash on top of v188-1.

I have attached the logcat from when I tried opening the app manually.

Comment 15

[Carsten Book [:Tomcat]]

(In reply to Robert Chira [:RobertC] from comment #13)
> This is breaking the camera app. The automation suite is failing because
> after opening the camera app a loading spinner is displayed and does not
> disappear. The issue is also reproduced manually 5 out of 5 attempts.
> 
> The tests first failed in the following Jenkins run:
> http://jenkins1.qa.scl3.mozilla.com/view/UI/job/flame-kk-319.b2g-inbound.ui.
> functional.smoke/984/HTML_Report/
> 
> Is there another patch that should have landed together with this?
> If not, please back out this patch.

Per past discussions and agreements with QA, backout requests for regressions that don't impact our CI infrastructure should go first to the patch author and then another module peer. This is to minimize risks associated  with other dependencies that may be currently unknown to people on our team. If a module peer isn't reachable and this is urgent (i.e. backout and respin nightlies ASAP because the phone is unusable), please let us know.

Redirecting the backout request to mike.

Thanks!

Comment 16

[Wilson Page [:wilsonpage]]

Confirmed this has broken camera app, with symptoms described in comment 13. Please backout.

Comment 17

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

Investigating.

Comment 18

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

It looks like the hardware is opening, but there is no initial configuration:

11-06 08:27:19.310  1370  1394 I PRLog   : 1668288[b2679980]: CameraControlImpl::OnUserError : aContext='SetConfiguration' (8), aError=0x80004005
11-06 08:27:19.430  1370  1370 I PRLog   : -1225137836[b434a080]: Camera mode still unspecified, nothing to do
11-06 08:27:19.430  1370  1370 I PRLog   : -1225137836[b434a080]: DOM OnHardwareStateChange: open

This probably didn't show up in testing because the camera had already cached an initial configuration. Testing a fix now.

Comment 19

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master)

Comment 20

[Justin D'Arcangelo [:justindarc]]

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

LGTM. Ship it!

Comment 21

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

Attachment: Handle no initial configuration, v1

Comment 22

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

Attachment: Handle no initial configuration, v1 r=aosmond

Add header to patch file; carry r+ forward.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3c8491fb2572

Master: https://github.com/mozilla-b2g/gaia/commit/10ac5891c8e06e28dd7b1856032d118449ed611c

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3c8491fb2572