[libcamera-devel] [PATCH v2 1/3] android: CameraDevice: Validate crop_rotate_scale_degrees in configuration

[Laurent Pinchart]

Hi Hiro,

Thank you for the patch.

On Mon, Mar 29, 2021 at 07:45:26AM +0900, Hirokazu Honda wrote:
> Libcamera doesn't handle |crop_rotate_scale_degrees| in

s/Libcamera/libcamera/ (the name is always written in lowercase)

> camera3_stream at all. This adds the validation of the requested
> |crop_rotate_scale_degrees| in configuration, but still not
> handle the specified values.
> 
> Signed-off-by: Hirokazu Honda <hiroh at chromium.org>
> ---
>  src/android/camera_device.cpp | 39 +++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index ae693664..c5e55a18 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -256,6 +256,39 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
>  	unsortedConfigs = sortedConfigs;
>  }
> 
> +/*
> + * Returns where crop_rotate_scale_degrees in all camera3_stream in stream_list

s/where/whether/ ?

> + * are valid.

I'd expand this a little bit to document that we're not validating the
values against what libcamera support, but against what the Chrome OS
API defines.

 * Check whether the crop_rotate_scale_degrees values for all streams in
 * the list are valid according to the Chrome OS camera HAL API.

> + */
> +bool validateCropRotate(const camera3_stream_configuration_t &stream_list)

s/stream_list/streamList/

> +{
> +	ASSERT(stream_list.num_streams > 0);
> +
> +	const int cropRotateScaleDegrees =
> +		stream_list.streams[0]->crop_rotate_scale_degrees;
> +	for (unsigned int i = 0; i < stream_list.num_streams; ++i) {
> +		const camera3_stream_t &stream = *stream_list.streams[i];
> +		if (CAMERA3_STREAM_ROTATION_0 > stream.crop_rotate_scale_degrees ||
> +		    CAMERA3_STREAM_ROTATION_270 < stream.crop_rotate_scale_degrees) {
> +			LOG(HAL, Error) << "Invalid crop_rotate_scale_degrees: "
> +					<< stream.crop_rotate_scale_degrees;
> +			return false;
> +		}
> +		if (stream.crop_rotate_scale_degrees == CAMERA3_STREAM_ROTATION_180) {
> +			LOG(HAL, Error) << "crop_rotate_scale_degrees should "
> +					<< "not be CAMERA3_STREAM_ROTATION_180";

A comment to explain why this is the case would be useful.

> +			return false;
> +		}

How about simplifying this to

		switch (stream.crop_rotate_scale_degrees) {
		/* 180° rotation is specified by Chrome OS as invalid. */
		case CAMERA3_STREAM_ROTATION_180:
		default:
			LOG(HAL, Error) << "Invalid crop_rotate_scale_degrees: "
					<< stream.crop_rotate_scale_degrees;
			return false;

		case CAMERA3_STREAM_ROTATION_0:
		case CAMERA3_STREAM_ROTATION_90:
		case CAMERA3_STREAM_ROTATION_270:
			break;
		}

> +		if (cropRotateScaleDegrees != stream.crop_rotate_scale_degrees) {
> +			LOG(HAL, Error) << "crop_rotate_scale_degrees in all "
> +					<< "streams are not identical";
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
>  } /* namespace */
> 
>  /*
> @@ -1552,6 +1585,12 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		running_ = false;
>  	}
> 
> +	if (stream_list->num_streams == 0)
> +		return -EINVAL;
> +
> +	if (!validateCropRotate(*stream_list))
> +		return -EINVAL;

This should only be enabled when the 'android_platform' meson option is
to to 'cros', as it's not available on Android. In the Android camera
HAL v3.4, the first reserved field has been used for physical_camera_id,
so there's a risk the value wouldn't be zero.

> +
>  	/*
>  	 * Generate an empty configuration, and construct a StreamConfiguration
>  	 * for each camera3_stream to add to it.

-- 
Regards,

Laurent Pinchart