[libcamera-devel] [PATCH v2 2/2] android: CameraDevice: Implement HAL3 API flush

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Fri, Apr 23, 2021 at 01:07:38PM +0900, Hirokazu Honda wrote:
> This implements flush(). It is mostly the same as close()
> though the canceled events are reported with error messages.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 41 +++++++++++++++++++++++++++++------
>  src/android/camera_device.h   |  1 +
>  src/android/camera_ops.cpp    |  6 ++++-
>  3 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index c74dc0e7..22a2e13c 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -753,6 +753,15 @@ void CameraDevice::close()
>  	stop(/*releaseCamera=*/true);
>  }
>  
> +int CameraDevice::flush()
> +{
> +	std::scoped_lock<std::mutex> lock(mutex_);
> +
> +	stop();
> +
> +	return 0;
> +}
> +
>  void CameraDevice::stop(bool releaseCamera)
>  {
>  	streams_.clear();
> @@ -770,6 +779,23 @@ void CameraDevice::stop(bool releaseCamera)
>  	worker_.stop();
>  	camera_->stop();
>  
> +	for (auto &[cookie, descriptor] : descriptors_) {
> +		LOG(HAL, Debug) << "request is canceled: " << cookie;

		LOG(HAL, Debug) << "request " << cookie << " is canceled";

> +
> +		camera3_capture_result_t captureResult = {};
> +		captureResult.frame_number = descriptor.frameNumber_;
> +		captureResult.num_output_buffers = descriptor.buffers_.size();
> +		for (camera3_stream_buffer_t &buffer : descriptor.buffers_) {
> +			buffer.acquire_fence = -1;
> +			buffer.release_fence = -1;
> +			buffer.status = CAMERA3_BUFFER_STATUS_ERROR;
> +		}
> +		captureResult.output_buffers = descriptor.buffers_.data();
> +
> +		notifyError(descriptor.frameNumber_,
> +			    descriptor.buffers_[0].stream);
> +		callbacks_->process_capture_result(callbacks_, &captureResult);

This looks very similar to the code in CameraDevice::requestComplete(),
would it make sense to move it to a separate function (including the
call to notifyError) ?

> +	}
>  	descriptors_.clear();
>  
>  	running_ = false;
> @@ -2128,6 +2154,14 @@ void CameraDevice::requestComplete(Request *request)
>  	}
>  
>  	if (status == CAMERA3_BUFFER_STATUS_ERROR || !captureResult.result) {
> +		/*
> +		 * \todo Report and identify the stream number or configuration
> +		 * to clarify the stream that failed.
> +		 */
> +		LOG(HAL, Error)
> +			<< "Error occurred on frame " << descriptor.frameNumber_ << " ("
> +			<< toPixelFormat(descriptor.buffers_[0].stream->format).toString() << ")";
> +
>  		/* \todo Improve error handling. In case we notify an error
>  		 * because the metadata generation fails, a shutter event has
>  		 * already been notified for this frame number before the error
> @@ -2161,13 +2195,6 @@ void CameraDevice::notifyError(uint32_t frameNumber, camera3_stream_t *stream)
>  {
>  	camera3_notify_msg_t notify = {};
>  
> -	/*
> -	 * \todo Report and identify the stream number or configuration to
> -	 * clarify the stream that failed.
> -	 */
> -	LOG(HAL, Error) << "Error occurred on frame " << frameNumber << " ("
> -			<< toPixelFormat(stream->format).toString() << ")";
> -
>  	notify.type = CAMERA3_MSG_ERROR;
>  	notify.message.error.error_stream = stream;
>  	notify.message.error.frame_number = frameNumber;
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 08553584..7004c89a 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -41,6 +41,7 @@ public:
>  
>  	int open(const hw_module_t *hardwareModule);
>  	void close();
> +	int flush();
>  
>  	unsigned int id() const { return id_; }
>  	camera3_device_t *camera3Device() { return &camera3Device_; }
> diff --git a/src/android/camera_ops.cpp b/src/android/camera_ops.cpp
> index 696e8043..981a3d30 100644
> --- a/src/android/camera_ops.cpp
> +++ b/src/android/camera_ops.cpp
> @@ -68,7 +68,11 @@ static void hal_dev_dump([[maybe_unused]] const struct camera3_device *dev,
>  
>  static int hal_dev_flush([[maybe_unused]] const struct camera3_device *dev)

You can drop [[maybe_unused]].

It would be nice to have an [[unused]] that causes a warning when the
variable is used.

>  {
> -	return 0;
> +	if (!dev)
> +		return -EINVAL;
> +
> +	CameraDevice *camera = reinterpret_cast<CameraDevice *>(dev->priv);
> +	return camera->flush();
>  }
>  
>  int hal_dev_close(hw_device_t *hw_device)

-- 
Regards,

Laurent Pinchart