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

[Jacopo Mondi]

Hi Hiro,

On Thu, Mar 25, 2021 at 02:19:29PM +0900, Hirokazu Honda wrote:
> Libcamera doesn't handle |crop_rotate_scale_degrees| in
> 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 | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp
> index 72a89258..1414bfef 100644
> --- a/src/android/camera_device.cpp
> +++ b/src/android/camera_device.cpp
> @@ -256,6 +256,38 @@ void sortCamera3StreamConfigs(std::vector<Camera3StreamConfig> &unsortedConfigs,
>  	unsortedConfigs = sortedConfigs;
>  }
>
> +/*
> + *  verifyCropRotateScaleDegrees returns where |crop_rotate_scale_degrees| in

double space at beginning of the line
> + * all camera3_stream in stream_list are valid.

And usage of |variable| is a bit alien to libcamera :)

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

just verifyCropRotate() ?
Or better validateCropRotate() ?

> +{
> +	if (stream_list.num_streams == 0)
> +		return true;

Emtpy line ?
Also, is this worth being checked in configureStreams() ?

> +	const int crop_rotate_scale_degrees =

We use camelCase for variables

> +		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) {

These are the possible values for crop_rotate_scale_degrees

/**
 * camera3_stream_rotation_t:
 *
 * The required counterclockwise rotation of camera stream.
 */
typedef enum camera3_stream_rotation {
    /* No rotation */
    CAMERA3_STREAM_ROTATION_0 = 0,

    /* Rotate by 90 degree counterclockwise */
    CAMERA3_STREAM_ROTATION_90 = 1,

    /* Rotate by 180 degree counterclockwise */
    CAMERA3_STREAM_ROTATION_180 = 2,

    /* Rotate by 270 degree counterclockwise */
    CAMERA3_STREAM_ROTATION_270 = 3
} camera3_stream_rotation_t;

The filed is documented as

   /**
     * This should be one of the camera3_stream_rotation_t values except for
     * CAMERA3_STREAM_ROTATION_180.
     * When setting to CAMERA3_STREAM_ROTATION_90 or CAMERA3_STREAM_ROTATION_270, HAL would crop,
     * rotate the frame by the specified degrees clockwise and scale it up to original size.
     * In Chrome OS, it's possible to have a portrait activity run in a landscape screen with
     * landscape-mounted camera. The activity would show stretched or rotated preview because it
     * does not expect to receive landscape preview frames. To solve this problem, we ask HAL to
     * crop, rotate and scale the frames and modify CameraCharacteristics.SENSOR_ORIENTATION
     * accordingly to imitate a portrait camera.
     * Setting it to CAMERA3_STREAM_ROTATION_0 means no crop-rotate-scale would be performed.
     * |cros_rotate_scale_degrees| in all camera3_stream_t of a configure_streams() call must be
     * identical. The HAL should return -EINVAL if the degrees are not the same for all the streams.
     */

I think checking for != ROTATION_180 would be enough, but in
camera3_stream_t the field as type int, not
camera3_stream_rotation_t, so do we need to make sure it is 'valid'
in the [0, 3] interval ? It doesn't hurt though

Handling rotation properly will be quite an headache as we'll have to
compensate the properties::Rotation value from the Camera. I admit
it's not yet clear in my mind how that will have to be done, but we'll
see. I guess we cannot rely on the sensor's flips but this will have
to go through the YUV post-processor (also to handle the crop-scale
part)

> +			LOG(HAL, Error) << "invalid crop_rotate_scale_degrees: "
> +					<< stream.crop_rotate_scale_degrees;

Invalid

> +			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";
> +			return false;
> +		}
> +		if (crop_rotate_scale_degrees != stream.crop_rotate_scale_degrees) {
> +			LOG(HAL, Error) << "crop_rotate_scale_degrees in all "
> +					<< "streams are not identical";
> +			return false;
> +		}
> +	}

Empty line ?

> +	return true;
> +}
> +
>  } /* namespace */
>
>  /*
> @@ -1566,6 +1598,9 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)
>  		running_ = false;
>  	}
>
> +	if (!verifyCropRotateScaleDegrees(*stream_list))
> +		return -EINVAL;
> +
>  	/*
>  	 * Generate an empty configuration, and construct a StreamConfiguration
>  	 * for each camera3_stream to add to it.
> --
> 2.31.0.291.g576ba9dcdaf-goog
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel