[libcamera-devel] [PATCH] android: camera_device: Stop camera when re-configuring it

[Niklas Söderlund]

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 at 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 at 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 at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund