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

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

Commit Message

Hirokazu Honda March 28, 2021, 10:45 p.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 | 39 +++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

--
2.31.0.291.g576ba9dcdaf-goog

Comments

Laurent Pinchart March 29, 2021, 4:27 a.m. UTC | #1 
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@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.
Hirokazu Honda March 29, 2021, 9:37 a.m. UTC | #2 
Hi Laurent, thanks for reviewing.

On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> 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@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.
>

Right. If you don't mind, can I submit this patch as-is and quickly
submit a patch of limiting this with android_platform='cros' later?

Regards,
-Hiro
> > +
> >       /*
> >        * Generate an empty configuration, and construct a StreamConfiguration
> >        * for each camera3_stream to add to it.
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart March 30, 2021, 3:55 a.m. UTC | #3 
Hi Hiro,

On Mon, Mar 29, 2021 at 06:37:15PM +0900, Hirokazu Honda wrote:
> On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart wrote:
> > 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@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.
> 
> Right. If you don't mind, can I submit this patch as-is and quickly
> submit a patch of limiting this with android_platform='cros' later?

libcamera is already used on Android by at least one project, and I'd
like to avoid breaking bisection for them. We introduce enough
regression on the Android side already without noticing, it would be
better to avoid at least the regressions that we are aware of :-) Would
you mind adding the android_platform option check already ?

> > > +
> > >       /*
> > >        * Generate an empty configuration, and construct a StreamConfiguration
> > >        * for each camera3_stream to add to it.
Hirokazu Honda March 30, 2021, 4:23 a.m. UTC | #4 
Hi Laurent,

On Tue, Mar 30, 2021 at 12:56 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Hiro,
>
> On Mon, Mar 29, 2021 at 06:37:15PM +0900, Hirokazu Honda wrote:
> > On Mon, Mar 29, 2021 at 1:28 PM Laurent Pinchart wrote:
> > > 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@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.
> >
> > Right. If you don't mind, can I submit this patch as-is and quickly
> > submit a patch of limiting this with android_platform='cros' later?
>
> libcamera is already used on Android by at least one project, and I'd
> like to avoid breaking bisection for them. We introduce enough
> regression on the Android side already without noticing, it would be
> better to avoid at least the regressions that we are aware of :-) Would
> you mind adding the android_platform option check already ?
>

Fair enough. I will do in the next patch series.

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

Patch
diff mbox series 

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
+ * are valid.
+ */
+bool validateCropRotate(const camera3_stream_configuration_t &stream_list)
+{
+	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";
+			return false;
+		}
+		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;
+
 	/*
 	 * Generate an empty configuration, and construct a StreamConfiguration
 	 * for each camera3_stream to add to it.