[libcamera-devel] android: camera_device: Stop camera when re-configuring it
diff mbox series

Message ID 20201204143955.294320-1-jacopo@jmondi.org
State Superseded, archived
Delegated to: Jacopo Mondi
Headers show
Series
  • [libcamera-devel] android: camera_device: Stop camera when re-configuring it
Related show

Commit Message

Jacopo Mondi Dec. 4, 2020, 2:39 p.m. UTC 
The Android camera device HAL3 specification does not require a
camera to go through any explicit close() call between configurations.
It is legit for a camera to be configured, a number of requests
processed and the re-configured again without any explicit stop.

The libcamera Android camera HAL starts the Camera at the first handled
request, and only stops it at camera close time. This mean, that two
camera configuration attempts part of the same streaming session are only
interleaved by capture requests handling.

The libcamera::Camera state machine requires the Camera to be stopped,
before any configuration take place, and this currently doesn't happen
in the camera HAL CameraDevice class.

Fix this by stopping the camera and the associated worker thread if
a configuration attempt is performed while the Camera is in running
state.

This patch fixes cros_camera_test:
Camera3PreviewTest/Camera3SinglePreviewTest.Camera3BasicPreviewTest/0

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

---
Alternatively, the camera can be stopped at the very beginning of the
configureStream() function. But in that case the camera gets stopped
even if the configuration attempt fails (ie at validation time).

As both behaviour seems to be compliant with what Android expects, I decided
to stop the camera only if the configuration is valid.
---

 src/android/camera_device.cpp | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--
2.29.1

Comments

Niklas Söderlund Dec. 4, 2020, 4:08 p.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2020-12-04 15:39:55 +0100, Jacopo Mondi wrote:
> The Android camera device HAL3 specification does not require a
> camera to go through any explicit close() call between configurations.
> It is legit for a camera to be configured, a number of requests
> processed and the re-configured again without any explicit stop.
> 
> The libcamera Android camera HAL starts the Camera at the first handled
> request, and only stops it at camera close time. This mean, that two
> camera configuration attempts part of the same streaming session are only
> interleaved by capture requests handling.
> 
> The libcamera::Camera state machine requires the Camera to be stopped,
> before any configuration take place, and this currently doesn't happen
> in the camera HAL CameraDevice class.
> 
> Fix this by stopping the camera and the associated worker thread if
> a configuration attempt is performed while the Camera is in running
> state.
> 
> This patch fixes cros_camera_test:
> Camera3PreviewTest/Camera3SinglePreviewTest.Camera3BasicPreviewTest/0
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> ---
> Alternatively, the camera can be stopped at the very beginning of the
> configureStream() function. But in that case the camera gets stopped
> even if the configuration attempt fails (ie at validation time).
> 
> As both behaviour seems to be compliant with what Android expects, I decided
> to stop the camera only if the configuration is valid.

I'm leaning a bit towards also stopping the camera if the configuration 
fails, from a power perspective it would it not make most sens? I'm head 
deep in other PM work so maybe I'm biased ;-) As our current goal is to 
pass CTS I don't think there is a need to bikesheed on this and this is 
clearly a step in the right direction,

Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>

> ---
> 
>  src/android/camera_device.cpp | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 675af5705055..2500994e2f6a 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -1341,8 +1341,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> 
>  	/*
>  	 * Once the CameraConfiguration has been adjusted/validated
> -	 * it can be applied to the camera.
> +	 * it can be applied to the camera, which has to be stopped if
> +	 * in running state.
>  	 */
> +	if (running_) {
> +		worker_.stop();
> +		camera_->stop();
> +		running_ = false;
> +	}
> +
>  	int ret = camera_->configure(config_.get());
>  	if (ret) {
>  		LOG(HAL, Error) << "Failed to configure camera '"
> --
> 2.29.1
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Dec. 4, 2020, 10:29 p.m. UTC | #2 
On Fri, Dec 04, 2020 at 05:08:06PM +0100, Niklas Söderlund wrote:
> On 2020-12-04 15:39:55 +0100, Jacopo Mondi wrote:
> > The Android camera device HAL3 specification does not require a
> > camera to go through any explicit close() call between configurations.
> > It is legit for a camera to be configured, a number of requests
> > processed and the re-configured again without any explicit stop.
> > 
> > The libcamera Android camera HAL starts the Camera at the first handled
> > request, and only stops it at camera close time. This mean, that two
> > camera configuration attempts part of the same streaming session are only
> > interleaved by capture requests handling.
> > 
> > The libcamera::Camera state machine requires the Camera to be stopped,
> > before any configuration take place, and this currently doesn't happen
> > in the camera HAL CameraDevice class.
> > 
> > Fix this by stopping the camera and the associated worker thread if
> > a configuration attempt is performed while the Camera is in running
> > state.
> > 
> > This patch fixes cros_camera_test:
> > Camera3PreviewTest/Camera3SinglePreviewTest.Camera3BasicPreviewTest/0
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > 
> > ---
> > Alternatively, the camera can be stopped at the very beginning of the
> > configureStream() function. But in that case the camera gets stopped
> > even if the configuration attempt fails (ie at validation time).
> > 
> > As both behaviour seems to be compliant with what Android expects, I decided
> > to stop the camera only if the configuration is valid.
> 
> I'm leaning a bit towards also stopping the camera if the configuration 
> fails, from a power perspective it would it not make most sens? I'm head 
> deep in other PM work so maybe I'm biased ;-) As our current goal is to 
> pass CTS I don't think there is a need to bikesheed on this and this is 
> clearly a step in the right direction,

I'm not sure if there's any test covering this specifically, but if an
invalid configuration is requested, I believe Android would still expect
the camera to be "stopped" and won't queue any further requests. I'm
thus also leaning towards stopping it earlier, even if it probably
doesn't matter too much.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> 
> > ---
> > 
> >  src/android/camera_device.cpp | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> > index 675af5705055..2500994e2f6a 100644
> > --- a/src/android/camera_device.cpp
> > +++ b/src/android/camera_device.cpp
> > @@ -1341,8 +1341,15 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
> > 
> >  	/*
> >  	 * Once the CameraConfiguration has been adjusted/validated
> > -	 * it can be applied to the camera.
> > +	 * it can be applied to the camera, which has to be stopped if
> > +	 * in running state.
> >  	 */
> > +	if (running_) {
> > +		worker_.stop();
> > +		camera_->stop();
> > +		running_ = false;
> > +	}
> > +
> >  	int ret = camera_->configure(config_.get());
> >  	if (ret) {
> >  		LOG(HAL, Error) << "Failed to configure camera '"

Patch
diff mbox series 

diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
index 675af5705055..2500994e2f6a 100644
--- a/src/android/camera_device.cpp
+++ b/src/android/camera_device.cpp
@@ -1341,8 +1341,15 @@  int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)

 	/*
 	 * Once the CameraConfiguration has been adjusted/validated
-	 * it can be applied to the camera.
+	 * it can be applied to the camera, which has to be stopped if
+	 * in running state.
 	 */
+	if (running_) {
+		worker_.stop();
+		camera_->stop();
+		running_ = false;
+	}
+
 	int ret = camera_->configure(config_.get());
 	if (ret) {
 		LOG(HAL, Error) << "Failed to configure camera '"