[libcamera-devel,1/3] android: CameraDevice: Validate crop_rotate_scale_degrees in configuration
diff mbox series

Message ID 20210325051931.3748204-2-hiroh@chromium.org
State Superseded
Headers show
Series
  • Regard crop_rotate_scale_degrees in configuration
Related show

Commit Message

Hirokazu Honda March 25, 2021, 5:19 a.m. UTC 
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@chromium.org>
---
 src/android/camera_device.cpp | 35 +++++++++++++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

--
2.31.0.291.g576ba9dcdaf-goog

Comments

Jacopo Mondi March 25, 2021, 10:45 a.m. UTC | #1 
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@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@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Hirokazu Honda March 28, 2021, 10:37 p.m. UTC | #2 
On Thu, Mar 25, 2021 at 7:44 PM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> 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@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
>

Yeah, since the type is int, it can be any value.
In fact, chromeos camera test has a test to set an invalid value and
checks if the configuration fails.
So we should keep this check.

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

My intuition is to flip (rotate) on a sensor, and cropping and scaling
is done in the YUV post-processor.
If a sensor is not reliable, all should be done in the YUV post processor.

Best Regards,
-Hiro

> > +                     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@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

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
+ * all camera3_stream in stream_list are valid.
+ */
+bool verifyCropRotateScaleDegrees(const camera3_stream_configuration_t &stream_list)
+{
+	if (stream_list.num_streams == 0)
+		return true;
+	const int crop_rotate_scale_degrees =
+		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";
+			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;
+		}
+	}
+	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.