David Scravaglieri [:scravag] Reporter
	Description • 12 years ago

gecko-beta: e33dbe505ee0
gaia: 879aa74e6b49d639129e301060de4d8f279fc4db

Steps to reproduce:
1 - In the Homescreen launch Camera
2 - Take a picture
3 - Tap Home button to go back to the Homescreen
4 - Press Power Button to lock the screen
5 - Take a picture from lockscreen

Expected:
Camera is launched and you can take a picture

Actual:
Camera is launched, the preview stays black, the App hangs.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 1 • 12 years ago

I'm pretty sure this is already fixed in bug 817141.

	Dale Harvey (:daleharvey)
	Comment 2 • 12 years ago

The case where a pincode wasnt set has been fixed, but this bug is visible on the latest HEAD (with the patch in 819275 to fix the camera) when the lock screen is enabled. Clarified the steps to reproduce, will attach a log to this, ok to assign to you mikeh?

Steps to reproduce:
1 - Add a pincode to your lockscreen
2 - In the Homescreen launch Camera
3 - Take a picture
4 - Tap Home button to go back to the Homescreen
5 - Press Power Button to lock the screen
6 - Take a picture from lockscreen

	Dale Harvey (:daleharvey)
	Comment 3 • 12 years ago

and when I say 'pincode' I mean 'passcode'

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 5 • 12 years ago

I just repo sync'd and I can't reproduce this problem--but I do see something else: I can't take a picture.  Pressing the shutter button (step 3) does nothing, and it looks like the camera is also no longer responding to orientation change events.  If I follow the rest of the steps, the camera resumes with a blank viewfinder.

After steps 1 and 2, the logcat is littered with:

I/Gecko   (  514): [Child 514] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file ../../dist/include/nsCOMPtr.h, line 505

...which is new.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 6 • 12 years ago

Check that: the picture does take eventually--it's just grossly laggy on a debug build (worse than usually--looks like "shutter latency" is about 30s.  That's new).

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 7 • 12 years ago

(Finally) able to reproduce issue as described in comment 2.  Relevant logcat snippet:

E/QualcommCamera(  958): Qint android::get_number_of_cameras(): E
I/QualcommCameraHardware(  958): getCameraInfo: IN
I/QualcommCameraHardware(  958): getCameraInfo: loading libqcamera at 0xb00100c0
V/QualcommCameraHardware(  958):  Storing the current target type as 3 
I/QualcommCameraHardware(  958): getCameraInfo: numOfCameras = 1
I/QualcommCameraHardware(  958): Camera sensor 0 info:
I/QualcommCameraHardware(  958): camera_id: 0
I/QualcommCameraHardware(  958): modes_supported: 5
I/QualcommCameraHardware(  958): position: 0
I/QualcommCameraHardware(  958): sensor_mount_angle: 90
V/QualcommCameraHardware(  958): getCameraInfo: dlclose(libqcamera)
I/QualcommCameraHardware(  958): getCameraInfo: OUT
I/PRLog   (  958): 1074304248[40404160]: getListOfCameras : get_number_of_cameras() returned 1
E/QualcommCamera(  958): Qint android::get_camera_info(int, camera_info*): E
I/QualcommCameraHardware(  958): Found a matching camera info for ID 0
I/QualcommCameraHardware(  958): HAL_getCameraInfo: orientation = 90
I/QualcommCameraHardware(  958): HAL_getCameraInfo: modes supported = 5
I/PRLog   (  958): 1074304248[40404160]: virtual nsresult nsDOMCameraManager::GetCamera(const JS::Value&, nsICameraGetCameraCallback*, nsICameraErrorCallback*, JSContext*):126
I/PRLog   (  958): 1074304248[40404160]: mozilla::nsDOMCameraControl::nsDOMCameraControl(uint32_t, nsIThread*, nsICameraGetCameraCallback*, nsICameraErrorCallback*, uint64_t):128 : this=4bd556c0
I/PRLog   (  958): 1074304248[40404160]: mozilla::CameraControlImpl::CameraControlImpl(uint32_t, nsIThread*, uint64_t):32 : this=48b6db00
I/PRLog   (  958): 1074304248[40404160]: mozilla::nsGonkCameraControl::nsGonkCameraControl(uint32_t, nsIThread*, mozilla::nsDOMCameraControl*, nsICameraGetCameraCallback*, nsICameraErrorCallback*, uint64_t):201 : this=48b6db00
I/PRLog   (  958): 1074304248[40404160]: InitGonkCameraControl::InitGonkCameraControl(mozilla::nsGonkCameraControl*, mozilla::nsDOMCameraControl*, nsICameraGetCameraCallback*, nsICameraErrorCallback*, uint64_t):158 : this=48b97ee0
I/PRLog   (  958): 1074304248[40404160]: >>> Register( aDOMCameraControl = 4bd556c0 ) mWindowId = 0xd
I/PRLog   (  958): 27139512[4bbebf90]: static void mozilla::GonkCameraHardware::ReleaseHandle(uint32_t): aHwHandle = 0, hw = 0 (sHwHandle = 0)
I/PRLog   (  958): 27139512[4bbebf90]: mozilla::GonkCameraHardware::GonkCameraHardware(mozilla::GonkCamera*, uint32_t):58 : this=4beb7380 (aTarget=48b6db00)
I/PRLog   (  958): 27139512[4bbebf90]: void mozilla::GonkCameraHardware::Init(): this=4beb7380
I/        (  958): Opening camera 0
E/QualcommCamera(  958): Qint android::camera_device_open(const hw_module_t*, const char*, hw_device_t**): E
I/QualcommCameraHardware(  958): openCameraHardware: call createInstance
I/QualcommCameraHardware(  958): openCameraHardware:Valid camera ID 0
I/QualcommCameraHardware(  958): HAL_openCameraHardware: HAL_currentSnapshotMode = 4 HAL_currentCameraMode = 1
I/QualcommCameraHardware(  958): createInstance: E
I/QualcommCameraHardware(  958): QualcommCameraHardware constructor E
V/QualcommCameraHardware(  958): MMCameraDL: E
V/QualcommCameraHardware(  958): Open MM camera DL libeomcamera loaded at 0xb00100c0 
V/QualcommCameraHardware(  958): MMCameraDL: X
V/QualcommCameraHardware(  958):  Storing the current target type as 3 
V/QualcommCameraHardware(  958): constructor EX
I/QualcommCameraHardware(  958): createInstance: created hardware=0x48ef9000
V/QualcommCameraHardware(  958): startCamera E
V/QualcommCameraHardware(  958): loading liboemcamera at 0xb00100c0
V/QualcommCameraHardware(  958):  openCamera : E
E/CAM_FD  (  958): cam_conf open driver failed: device =/dev/msm_camera/config0: Device or resource busy!
E/CAM_FD  (  958): cam_conf: CAMERA_EXIT
E/mm-camera(  958): mm_camera_exec: config thread launch failed
E/QualcommCameraHardware(  958): startCamera: mm_camera_exec failed:
E/QualcommCameraHardware(  958): openCamera() failed
E/QualcommCameraHardware(  958): createInstance: startCamera failed!
E/        (  958): Could not open camera 0: -1
I/        (  958): Destroying camera 0
I/PRLog   (  958): 27139512[4bbebf90]: failed to initialize camera hardware
I/PRLog   (  958): 27139512[4bbebf90]: mozilla::GonkCameraHardware::~GonkCameraHardware():221 : this=4beb7380
I/PRLog   (  958): 27139512[4bbebf90]: Initializing camera 0 (this=48b6db00, mHwHandle=0)
I/PRLog   (  958): 27139512[4bbebf90]: Pulling camera parameters
I/PRLog   (  958): 27139512[4bbebf90]: Camera preview formats: (null)
I/PRLog   (  958): 27139512[4bbebf90]: Pushing camera parameters
I/PRLog   (  958): 27139512[4bbebf90]: Pulling camera parameters
I/PRLog   (  958): 27139512[4bbebf90]:  - minimum exposure compensation: -1.000000
I/PRLog   (  958): 27139512[4bbebf90]:  - exposure compensation step:    -1.000000
I/PRLog   (  958): 27139512[4bbebf90]:  - maximum metering areas:        -1
I/PRLog   (  958): 27139512[4bbebf90]:  - maximum focus areas:           -1
I/PRLog   (  958): 27139512[4bbebf90]: virtual InitGonkCameraControl::~InitGonkCameraControl():163 : this=48b97ee0

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 8 • 12 years ago

Okay, here's what's going on: when the instance of the camera opened in comment 2 step 2 is sent to the background in step 4, the preview is stopped but the camera app continues to hold onto a reference to the camera hardware.  This keeps the camera device "open", and unavailable for use by other applications.

There is a low-level hardware manager in gecko that tries to detect extra calls to getCamera() and clean up previous references to the camera hardware, but this only works within a single memory space.

It looks like something has changed in the lockscreen that causes it to spawn a new camera app instance--instead of just bringing the existing one to the foreground--and that instance is seeing the camera device as busy.

Unfortunately, there is nothing we can do about this, without making the camera a managed resource (too big a change for v1).

The solution is that the camera app needs to release its reference to the camera hardware when it goes into the background; unfortunately this isn't as simple as setting |Camera._cameraObj=null|, since the camera object is a cycle participant and cycle collection is non-deterministic.  The camera API will need a new release() method that can be called in response to 'mozvisibilitychange' to release the hardware.

Another solution could be to make each camera object instance listen for something like a "camera-close-required" event, and calling NotifyObservers() to force them to release the camera hardware so that another client can acquire it.

	Dale Harvey (:daleharvey)
	Comment 9 • 12 years ago

The camera opened from the lock screen when a passcode is enabled is opened as the homescreen application. Someone was working on a patch to make sure that the camera is killed properly when it is existed inside the homescreen, trying to find out who now.

Hopefully that patch (it was for a geolocation bug) should fix this as well

	Dale Harvey (:daleharvey)
	Comment 10 • 12 years ago

That patch already landed :(

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

So as you said it must be holding the reference while it is still waiting for collection. the release method sounds sanest to me, but it seems like this should be explicit (the camera should be able to survive if multiple apps attempt to use it without releasing it correctly)

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 11 • 12 years ago

Why do we have multiple camera apps?

	Dale Harvey (:daleharvey)
	Comment 12 • 12 years ago

We cant access the 'Camera App' from the lockscreen while the user has a passcode (at that point we couldnt handle the home button etc)

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 13 • 12 years ago

(In reply to Dale Harvey (:daleharvey) from comment #10)
> 
> So as you said it must be holding the reference while it is still waiting
> for collection. the release method sounds sanest to me, but it seems like
> this should be explicit (the camera should be able to survive if multiple
> apps attempt to use it without releasing it correctly)

The alternative is I have to setup an IPC channel to an outside-of-content camera manager service that handles requests to shut down other camera instances on the device.  I've been digging through the docs for that, and it looks like a non-trivial amount of work.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 14 • 12 years ago

That's bug 785592, and it's not trivial.  It's what we should have done from the beginning but the ship sailed on that for v1.

(In reply to Dale Harvey (:daleharvey) from comment #12)
> We cant access the 'Camera App' from the lockscreen while the user has a
> passcode (at that point we couldnt handle the home button etc)

I don't quite follow that.  Why can't we load the camera app in a mode where it doesn't show user data?  How is the home button related?

	Dylan Oliver [:doliver]
	Comment 15 • 12 years ago

Can we get an update on the thoughts for resolving this one?

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 16 • 12 years ago

Mark as dup of bug 818955 given the fact they got the same STR.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 17 • 12 years ago

Reopening the earlier bug, closing the later.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 19 • 12 years ago

dale, try this out.  With this API, if I follow your STR, everything works.  (Will add my patched camera.js in a moment to show how to use it.)

	Chiajung Hung [:chiajung]
	Comment 21 • 12 years ago

Hi Mike,

After read your patch, I have some quick questions:

I think you have to Camera.setSource(Camera._camera) when visibility change from hide to visible. Since the CameraControl is in an unusable state as it do not really hold a hardware handle, I think the Camera.startPreview call may cause problems. 

Or you have to recover the handle when any function of CameraControl that need the hardware handle called since the hardware handle is null. However, because there are many function need the hardware handle to do its work (and any function can be the start point), you have to recover the handle in many case for safe.

That's the same problems I think of when I started to think about the issue in bug#818955.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 22 • 12 years ago

(In reply to Chiajung Hung [:chiajung] from comment #21)
> 
> I think you have to Camera.setSource(Camera._camera) when visibility change
> from hide to visible. Since the CameraControl is in an unusable state as it
> do not really hold a hardware handle, I think the Camera.startPreview call
> may cause problems. 

Take a look inside startPreview() and you'll see the setSource() call is already there.

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 23 • 12 years ago

try-server push: https://tbpl.mozilla.org/?tree=Try&rev=936a807f3ee9

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 24 • 12 years ago

kanru: review ping?

	Dale Harvey (:daleharvey)
	Comment 26 • 12 years ago

Comment on attachment 691933 [details] [diff] [review]
HOWTO: camera.js

Looks great to me, will do the gaia side soon as you get your review

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 27 • 12 years ago

r=me if adding a new release() method is acceptable. Couldn't we release the hardware internally while the camera is in background?

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 29 • 12 years ago

(In reply to Kan-Ru Chen [:kanru] from comment #27)
>
> r=me if adding a new release() method is acceptable. Couldn't we release the
> hardware internally while the camera is in background?

That's what's intended to happen with this patch: .release() is meant to be called in response to visibility changes in the app.  I'm not aware of any behind-the-scenes way of detecting when an app was gone into the background, and it's probably best if we coordinate it through the app anyway.

(v2 will have a proper resource manager so that things like this won't be required anymore.)

	Mike Habicher [:mikeh] (high bugzilla latency)
	Comment 31 • 12 years ago

r+ as per comment 27.

	Dale Harvey (:daleharvey)
	Comment 32 • 12 years ago

Stealing so I remember to land the gaia side when its in gecko

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 12 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/606f13fe356f

(Really not a fan of landing self-reviewed patches, by the way)

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 12 years ago

(In reply to Ryan VanderMeulen from comment #33)
> (Really not a fan of landing self-reviewed patches, by the way)

Sorry, dholbert pointed out to me that this had r=kanru in the message attachment, which I missed. I don't think it's worth backing out and relanding over, though. I'll correct it when I uplift it to Aurora/b2g18, though.

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 12 years ago

https://hg.mozilla.org/mozilla-central/rev/606f13fe356f

	Dale Harvey (:daleharvey)
	Comment 36 • 12 years ago

This isnt quite fixed yet, I need to land the gaia side to it as well, will do so tonight

	Ryan VanderMeulen [:RyanVM]
	Comment 37 • 12 years ago

For future reference, you can put [leave open] on the status whiteboard to avoid the bug being closed prematurely.

	Ryan VanderMeulen [:RyanVM]
	Comment 38 • 12 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/7fba63d647d5

	Ryan VanderMeulen [:RyanVM]
	Comment 39 • 12 years ago

FYI, this doesn't apply cleanly to b2g18. This is either going to need a branch-specific patch or uplift of other dependencies.

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 40 • 12 years ago

It doesn't apply cleanly because of bug 817141 and bug 803976. Hope this patch for b2g18 doesn't screw up.

	Kan-Ru Chen [:kanru] (UTC+9)
	Comment 41 • 12 years ago

https://hg.mozilla.org/releases/mozilla-b2g18/rev/5ebd72acdf52

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 42 • 12 years ago

Stealing the Gaia fix.

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 43 • 12 years ago

Dale, David, not sure which of you is available for a little review.

Instead of doing release in the visibility change event handling, I am doing it in the stopPreview(). There is also some checks added in the setSource() (called by startPreview() -- watch the symmetry) to make sure we don't overwrite a camera object without properly release it first.

	Dale Harvey (:daleharvey)
	Comment 44 • 12 years ago

Comment on attachment 695723 [details]
Github: https://github.com/mozilla-b2g/gaia/pull/7192

Theres a bug caused by a typo in there, it isnt actually triggered on the target device as it will only occur when we have the ability to switch cameras (like on the nexus s), however would prefer to not start committing bugs.

I can confirm it fixed the problem though, so r+ with the typo fixed

	Tim Guan-tin Chien [:timdream] (please needinfo) Assignee
	Comment 45 • 12 years ago

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

Gaia part merged.

	David Flanagan [:djf]
	Comment 46 • 12 years ago

Comment on attachment 695723 [details]
Github: https://github.com/mozilla-b2g/gaia/pull/7192

looks good to me.