[libcamera-devel] [PATCH v5 8/8] android: Implement flush() camera operation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jun 08, 2021 at 05:16:33PM +0200, Jacopo Mondi wrote:
> Implement the flush() camera operation in the CameraDevice class
> and make it available to the camera framework by implementing the
> operation wrapper in camera_ops.cpp.
> 
> Introduce a new camera state State::Flushing to handle concurrent
> flush() and process_capture_request() calls.
> 
> As flush() can race with processCaptureRequest() protect it
> by introducing a new State::Flushing state that
> processCaptureRequest() inspects before queuing the Request to the
> Camera. If flush() is in progress while processCaptureRequest() is
> called, return the current Request immediately in error state. If
> flush() has completed and a new call to processCaptureRequest() is
> made just after, start the camera again before queuing the request.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> Reviewed-by: Hirokazu Honda <hiroh at chromium.org>
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
>  src/android/camera_device.cpp | 77 +++++++++++++++++++++++++++--------
>  src/android/camera_device.h   |  3 ++
>  src/android/camera_ops.cpp    |  8 +++-
>  3 files changed, 71 insertions(+), 17 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index b29c1edc9970..3adb657bfeca 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -798,6 +798,23 @@ void CameraDevice::close()
>  	camera_->release();
>  }
>  
> +void CameraDevice::flush()
> +{
> +	{
> +		MutexLocker stateLock(stateMutex_);
> +		if (state_ != State::Running)
> +			return;
> +
> +		state_ = State::Flushing;
> +	}
> +
> +	worker_.stop();
> +	camera_->stop();
> +
> +	MutexLocker stateLock(stateMutex_);
> +	state_ = State::Stopped;
> +}
> +
>  void CameraDevice::stop()
>  {
>  	MutexLocker stateLock(stateMutex_);
> @@ -1896,27 +1913,31 @@ int CameraDevice::processControls(Camera3RequestDescriptor *descriptor)
>  	return 0;
>  }
>  
> -int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +void CameraDevice::abortRequest(camera3_capture_request_t *request)
>  {
> -	if (!isValidRequest(camera3Request))
> -		return -EINVAL;
> +	notifyError(request->frame_number, nullptr, CAMERA3_MSG_ERROR_REQUEST);
>  
> -	{
> -		MutexLocker stateLock(stateMutex_);
> +	camera3_capture_result_t result = {};
> +	result.num_output_buffers = request->num_output_buffers;
> +	result.frame_number = request->frame_number;
> +	result.partial_result = 0;
>  
> -		/* Start the camera if that's the first request we handle. */
> -		if (state_ == State::Stopped) {
> -			worker_.start();
> +	std::vector<camera3_stream_buffer_t> resultBuffers(result.num_output_buffers);
> +	for (auto [i, buffer] : utils::enumerate(resultBuffers)) {
> +		buffer = request->output_buffers[i];
> +		buffer.release_fence = request->output_buffers[i].acquire_fence;
> +		buffer.acquire_fence = -1;
> +		buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +	}
> +	result.output_buffers = resultBuffers.data();
>  
> -			int ret = camera_->start();
> -			if (ret) {
> -				LOG(HAL, Error) << "Failed to start camera";
> -				return ret;
> -			}
> +	callbacks_->process_capture_result(callbacks_, &result);
> +}
>  
> -			state_ = State::Running;
> -		}
> -	}
> +int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
> +{
> +	if (!isValidRequest(camera3Request))
> +		return -EINVAL;
>  
>  	/*
>  	 * Save the request descriptors for use at completion time.
> @@ -2006,6 +2027,30 @@ int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Reques
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * Just before queuing the request, make sure flush() has not
> +	 * been called while this function was running. If flush is in progress
> +	 * abort the request. If flush has completed and has stopped the camera
> +	 * we have to re-start it to be able to process the request.
> +	 */
> +	MutexLocker stateLock(stateMutex_);

I'd add a blank line here.

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

> +	if (state_ == State::Flushing) {
> +		abortRequest(camera3Request);
> +		return 0;
> +	}
> +
> +	if (state_ == State::Stopped) {
> +		worker_.start();
> +
> +		ret = camera_->start();
> +		if (ret) {
> +			LOG(HAL, Error) << "Failed to start camera";
> +			return ret;
> +		}
> +
> +		state_ = State::Running;
> +	}
> +
>  	worker_.queueRequest(descriptor.request_.get());
>  
>  	{
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index c949fa509ca4..4aadb27c562c 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -43,6 +43,7 @@ public:
>  
>  	int open(const hw_module_t *hardwareModule);
>  	void close();
> +	void flush();
>  
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> @@ -92,6 +93,7 @@ private:
>  
>  	enum class State {
>  		Stopped,
> +		Flushing,
>  		Running,
>  	};
>  
> @@ -106,6 +108,7 @@ private:
>  	getRawResolutions(const libcamera::PixelFormat &pixelFormat);
>  
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
> +	void abortRequest(camera3_capture_request_t *request);
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code);
> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> index 696e80436821..8a3cfa175ff5 100644
> --- a/src/android/camera_ops.cpp
> +++ b/src/android/camera_ops.cpp
> @@ -66,8 +66,14 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
>  {
>  }
>  
> -static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)
> +static int hal_dev_flush(const struct camera3_device *dev)
>  {
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> +	camera->flush();
> +
>  	return 0;
>  }
>  

-- 
Regards,

Laurent Pinchart