[libcamera-devel] [PATCH] android: camera_device: Check if a request is configured on processCaptureRequest()

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Jun 14, 2021 at 03:03:03PM +0900, Hirokazu Honda wrote:
> Adds a check on processCaptureRequest() if a given capture

s/Adds/Add/

> request contains a camera stream that has been configured.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 111 +++++++++++++++++++++-------------
>  src/android/camera_device.h   |   1 +
>  2 files changed, 70 insertions(+), 42 deletions(-)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 3adb657b..5b182257 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -260,48 +260,6 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
>  	unsortedConfigs = sortedConfigs;
>  }
>  
> -bool isValidRequest(camera3_capture_request_t *camera3Request)
> -{
> -	if (!camera3Request) {
> -		LOG(HAL, Error) << "No capture request provided";
> -		return false;
> -	}
> -
> -	if (!camera3Request->num_output_buffers ||
> -	    !camera3Request->output_buffers) {
> -		LOG(HAL, Error) << "No output buffers provided";
> -		return false;
> -	}
> -
> -	for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
> -		const camera3_stream_buffer_t &outputBuffer =
> -			camera3Request->output_buffers[i];
> -		if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {
> -			LOG(HAL, Error) << "Invalid native handle";
> -			return false;
> -		}
> -
> -		const native_handle_t *handle = *outputBuffer.buffer;
> -		constexpr int kNativeHandleMaxFds = 1024;
> -		if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
> -			LOG(HAL, Error)
> -				<< "Invalid number of fds (" << handle->numFds
> -				<< ") in buffer " << i;
> -			return false;
> -		}
> -
> -		constexpr int kNativeHandleMaxInts = 1024;
> -		if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
> -			LOG(HAL, Error)
> -				<< "Invalid number of ints (" << handle->numInts
> -				<< ") in buffer " << i;
> -			return false;
> -		}
> -	}
> -
> -	return true;
> -}
> -
>  const char *rotationToString(int rotation)
>  {
>  	switch (rotation) {
> @@ -1934,6 +1892,75 @@ void CameraDevice::abortRequest(camera3_capture_request_t *request)
>  	callbacks_->process_capture_result(callbacks_, &result);
>  }
>  
> +bool CameraDevice::isValidRequest(camera3_capture_request_t *camera3Request) const
> +{
> +	if (!camera3Request) {
> +		LOG(HAL, Error) << "No capture request provided";
> +		return false;
> +	}
> +
> +	if (!camera3Request->num_output_buffers ||
> +	    !camera3Request->output_buffers) {
> +		LOG(HAL, Error) << "No output buffers provided";
> +		return false;
> +	}
> +
> +	// configureStreams() is not called or fails.

C-style comments please. I'd also write it as

	/* configureStreams() hasn't been called or has failed. */

> +	if (streams_.empty() || !config_) {
> +		LOG(HAL, Error) << "No stream is configured";
> +		return false;
> +	}
> +
> +	for (uint32_t i = 0; i < camera3Request->num_output_buffers; i++) {
> +		const camera3_stream_buffer_t &outputBuffer =
> +			camera3Request->output_buffers[i];
> +		if (!outputBuffer.buffer || !(*outputBuffer.buffer)) {
> +			LOG(HAL, Error) << "Invalid native handle";
> +			return false;
> +		}
> +
> +		const native_handle_t *handle = *outputBuffer.buffer;
> +		constexpr int kNativeHandleMaxFds = 1024;
> +		if (handle->numFds < 0 || handle->numFds > kNativeHandleMaxFds) {
> +			LOG(HAL, Error)
> +				<< "Invalid number of fds (" << handle->numFds
> +				<< ") in buffer " << i;
> +			return false;
> +		}
> +
> +		constexpr int kNativeHandleMaxInts = 1024;
> +		if (handle->numInts < 0 || handle->numInts > kNativeHandleMaxInts) {
> +			LOG(HAL, Error)
> +				<< "Invalid number of ints (" << handle->numInts
> +				<< ") in buffer " << i;
> +			return false;
> +		}
> +
> +		const camera3_stream *camera3Stream = outputBuffer.stream;
> +		if (!camera3Stream)
> +			return false;
> +
> +		const CameraStream *cameraStream =
> +			static_cast<CameraStream *>(camera3Stream->priv);
> +
> +		bool found = false;
> +		for (const CameraStream &stream : streams_) {
> +			if (&stream == cameraStream) {
> +				found = true;
> +				break;
> +			}
> +		}

Another option could be

                auto found = std::find_if(streams_.begin(), streams_.end(),
                                          [&](const CameraStream &stream) {
                                                  return &stream == cameraStream;
                                          });
                if (found == streams_.end()) {
			LOG(HAL, Error)
				<< "No corresponding configured stream found";
			return false;
		};

(with a new #include <algorithm>). Up to you.

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

> +
> +		if (!found) {
> +			LOG(HAL, Error)
> +				<< "No corresponding configured stream found";
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)
>  {
>  	if (!isValidRequest(camera3Request))
> diff --git a/src/android/camera_device.h b/src/android/camera_device.h
> index 4aadb27c..4d9c904d 100644
> --- a/src/android/camera_device.h
> +++ b/src/android/camera_device.h
> @@ -109,6 +109,7 @@ private:
>  
>  	libcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);
>  	void abortRequest(camera3_capture_request_t *request);
> +	bool isValidRequest(camera3_capture_request_t *request) const;
>  	void notifyShutter(uint32_t frameNumber, uint64_t timestamp);
>  	void notifyError(uint32_t frameNumber, camera3_stream_t *stream,
>  			 camera3_error_msg_code code);

-- 
Regards,

Laurent Pinchart