[libcamera-devel,v5,04/10] libcamera: camera_sensor: Default analogue crop rectangle
diff mbox series

Message ID 20210105123128.617543-5-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: camera_sensor: Make validation more strict
Related show

Commit Message

Jacopo Mondi Jan. 5, 2021, 12:31 p.m. UTC 
As support for the V4L2_SEL_TGT_CROP selection target, used to read the
sensor analogue crop rectangle is schedule to become mandatory but is
still optional, cache a default value equal to the sensor's active area
size at sensor validation time to allow the creation of the
CameraSensorInfo in the case the driver does not support it.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 include/libcamera/internal/camera_sensor.h |  1 +
 src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++--------
 2 files changed, 19 insertions(+), 9 deletions(-)

Comments

Niklas Söderlund Jan. 6, 2021, 10:50 a.m. UTC | #1 
Hi Jacopo,

Thanks for your work.

On 2021-01-05 13:31:22 +0100, Jacopo Mondi wrote:
> As support for the V4L2_SEL_TGT_CROP selection target, used to read the
> sensor analogue crop rectangle is schedule to become mandatory but is
> still optional, cache a default value equal to the sensor's active area
> size at sensor validation time to allow the creation of the
> CameraSensorInfo in the case the driver does not support it.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  1 +
>  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++--------
>  2 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 86902b85ada8..de6a0202d19a 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -86,6 +86,7 @@ private:
>  
>  	Size pixelArraySize_;
>  	Rectangle activeArea_;
> +	Rectangle analogueCrop_;
>  
>  	ControlList properties_;
>  };
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index 92c90862215d..eda41980aa22 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -289,8 +289,10 @@ int CameraSensor::validateSensorDriver()
>  
>  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
>  	if (ret) {
> +		analogueCrop_ = activeArea_;
>  		LOG(CameraSensor, Warning)
> -			<< "Failed to retrieve the sensor crop rectangle";
> +			<< "The analogue crop rectangle has been defaulted to: "
> +			<< analogueCrop_.toString();
>  		err = -EINVAL;
>  	}
>  
> @@ -643,13 +645,20 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  	 */
>  	info->activeAreaSize = { activeArea_.width, activeArea_.height };
>  
> -	/* It's mandatory for the subdevice to report its crop rectangle. */
> -	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> -	if (ret) {
> -		LOG(CameraSensor, Error)
> -			<< "Failed to construct camera sensor info: "
> -			<< "the camera sensor does not report the crop rectangle";
> -		return ret;
> +	/*
> +	 * \todo Support for retreiving the crop rectangle is scheduled to
> +	 * become mandatory. For the time being use the default value if it has
> +	 * been initialized at sensor driver validation time.
> +	 */
> +	if (!analogueCrop_.isNull()) {
> +		info->analogCrop = analogueCrop_;
> +	} else {
> +		int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> +		if (ret) {
> +			LOG(CameraSensor, Error)
> +				<< "Failed to retrieve the sensor crop rectangle.";
> +			return -EINVAL;
> +		}

As I understand 3/10 activeArea_ is static right? Would it make more 
sens to use it here instead of adding and caching it in analogueCrop_?  
This would align the code here to always call getSelection() and 
huddling the defaulting in the if (ret) code block? I think this would 
make the code cleaner as the default as you point out is a temporary 
work around.

>  	}
>  
>  	/*
> @@ -664,7 +673,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
>  
>  	/* The bit depth and image size depend on the currently applied format. */
>  	V4L2SubdeviceFormat format{};
> -	ret = subdev_->getFormat(pad_, &format);
> +	int ret = subdev_->getFormat(pad_, &format);
>  	if (ret)
>  		return ret;
>  	info->bitsPerPixel = format.bitsPerPixel();
> -- 
> 2.29.2
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Jan. 7, 2021, 2:44 a.m. UTC | #2 
Hi Jacopo,

Thank you for the patch.

On Wed, Jan 06, 2021 at 11:50:41AM +0100, Niklas Söderlund wrote:
> On 2021-01-05 13:31:22 +0100, Jacopo Mondi wrote:
> > As support for the V4L2_SEL_TGT_CROP selection target, used to read the
> > sensor analogue crop rectangle is schedule to become mandatory but is
> > still optional, cache a default value equal to the sensor's active area
> > size at sensor validation time to allow the creation of the
> > CameraSensorInfo in the case the driver does not support it.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  1 +
> >  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++--------
> >  2 files changed, 19 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 86902b85ada8..de6a0202d19a 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -86,6 +86,7 @@ private:
> >  
> >  	Size pixelArraySize_;
> >  	Rectangle activeArea_;
> > +	Rectangle analogueCrop_;
> >  
> >  	ControlList properties_;
> >  };
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index 92c90862215d..eda41980aa22 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -289,8 +289,10 @@ int CameraSensor::validateSensorDriver()
> >  
> >  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> >  	if (ret) {
> > +		analogueCrop_ = activeArea_;
> >  		LOG(CameraSensor, Warning)
> > -			<< "Failed to retrieve the sensor crop rectangle";
> > +			<< "The analogue crop rectangle has been defaulted to: "
> > +			<< analogueCrop_.toString();
> >  		err = -EINVAL;
> >  	}
> >  
> > @@ -643,13 +645,20 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >  	 */
> >  	info->activeAreaSize = { activeArea_.width, activeArea_.height };
> >  
> > -	/* It's mandatory for the subdevice to report its crop rectangle. */
> > -	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > -	if (ret) {
> > -		LOG(CameraSensor, Error)
> > -			<< "Failed to construct camera sensor info: "
> > -			<< "the camera sensor does not report the crop rectangle";
> > -		return ret;
> > +	/*
> > +	 * \todo Support for retreiving the crop rectangle is scheduled to
> > +	 * become mandatory. For the time being use the default value if it has
> > +	 * been initialized at sensor driver validation time.
> > +	 */
> > +	if (!analogueCrop_.isNull()) {
> > +		info->analogCrop = analogueCrop_;
> > +	} else {
> > +		int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > +		if (ret) {
> > +			LOG(CameraSensor, Error)
> > +				<< "Failed to retrieve the sensor crop rectangle.";
> > +			return -EINVAL;
> > +		}
> 
> As I understand 3/10 activeArea_ is static right? Would it make more 
> sens to use it here instead of adding and caching it in analogueCrop_?  
> This would align the code here to always call getSelection() and 
> huddling the defaulting in the if (ret) code block? I think this would 
> make the code cleaner as the default as you point out is a temporary 
> work around.

I like this a bit better too. With if possible, or without this change,

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

> >  	}
> >  
> >  	/*
> > @@ -664,7 +673,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >  
> >  	/* The bit depth and image size depend on the currently applied format. */
> >  	V4L2SubdeviceFormat format{};
> > -	ret = subdev_->getFormat(pad_, &format);
> > +	int ret = subdev_->getFormat(pad_, &format);
> >  	if (ret)
> >  		return ret;
> >  	info->bitsPerPixel = format.bitsPerPixel();
Jacopo Mondi Jan. 7, 2021, 8:03 a.m. UTC | #3 
Hi Laurent, Niklas,

On Thu, Jan 07, 2021 at 04:44:59AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Jan 06, 2021 at 11:50:41AM +0100, Niklas Söderlund wrote:
> > On 2021-01-05 13:31:22 +0100, Jacopo Mondi wrote:
> > > As support for the V4L2_SEL_TGT_CROP selection target, used to read the
> > > sensor analogue crop rectangle is schedule to become mandatory but is
> > > still optional, cache a default value equal to the sensor's active area
> > > size at sensor validation time to allow the creation of the
> > > CameraSensorInfo in the case the driver does not support it.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  include/libcamera/internal/camera_sensor.h |  1 +
> > >  src/libcamera/camera_sensor.cpp            | 27 ++++++++++++++--------
> > >  2 files changed, 19 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > > index 86902b85ada8..de6a0202d19a 100644
> > > --- a/include/libcamera/internal/camera_sensor.h
> > > +++ b/include/libcamera/internal/camera_sensor.h
> > > @@ -86,6 +86,7 @@ private:
> > >
> > >  	Size pixelArraySize_;
> > >  	Rectangle activeArea_;
> > > +	Rectangle analogueCrop_;
> > >
> > >  	ControlList properties_;
> > >  };
> > > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > > index 92c90862215d..eda41980aa22 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > @@ -289,8 +289,10 @@ int CameraSensor::validateSensorDriver()
> > >
> > >  	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
> > >  	if (ret) {
> > > +		analogueCrop_ = activeArea_;
> > >  		LOG(CameraSensor, Warning)
> > > -			<< "Failed to retrieve the sensor crop rectangle";
> > > +			<< "The analogue crop rectangle has been defaulted to: "
> > > +			<< analogueCrop_.toString();
> > >  		err = -EINVAL;
> > >  	}
> > >
> > > @@ -643,13 +645,20 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > >  	 */
> > >  	info->activeAreaSize = { activeArea_.width, activeArea_.height };
> > >
> > > -	/* It's mandatory for the subdevice to report its crop rectangle. */
> > > -	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > > -	if (ret) {
> > > -		LOG(CameraSensor, Error)
> > > -			<< "Failed to construct camera sensor info: "
> > > -			<< "the camera sensor does not report the crop rectangle";
> > > -		return ret;
> > > +	/*
> > > +	 * \todo Support for retreiving the crop rectangle is scheduled to
> > > +	 * become mandatory. For the time being use the default value if it has
> > > +	 * been initialized at sensor driver validation time.
> > > +	 */
> > > +	if (!analogueCrop_.isNull()) {
> > > +		info->analogCrop = analogueCrop_;
> > > +	} else {
> > > +		int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
> > > +		if (ret) {
> > > +			LOG(CameraSensor, Error)
> > > +				<< "Failed to retrieve the sensor crop rectangle.";
> > > +			return -EINVAL;
> > > +		}
> >
> > As I understand 3/10 activeArea_ is static right? Would it make more
> > sens to use it here instead of adding and caching it in analogueCrop_?
> > This would align the code here to always call getSelection() and
> > huddling the defaulting in the if (ret) code block? I think this would
> > make the code cleaner as the default as you point out is a temporary
> > work around.
>
> I like this a bit better too. With if possible, or without this change,
>

Yeah, makes sense, I'll fixe

> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> > >  	}
> > >
> > >  	/*
> > > @@ -664,7 +673,7 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > >
> > >  	/* The bit depth and image size depend on the currently applied format. */
> > >  	V4L2SubdeviceFormat format{};
> > > -	ret = subdev_->getFormat(pad_, &format);
> > > +	int ret = subdev_->getFormat(pad_, &format);
> > >  	if (ret)
> > >  		return ret;
> > >  	info->bitsPerPixel = format.bitsPerPixel();
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 86902b85ada8..de6a0202d19a 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -86,6 +86,7 @@  private:
 
 	Size pixelArraySize_;
 	Rectangle activeArea_;
+	Rectangle analogueCrop_;
 
 	ControlList properties_;
 };
diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
index 92c90862215d..eda41980aa22 100644
--- a/src/libcamera/camera_sensor.cpp
+++ b/src/libcamera/camera_sensor.cpp
@@ -289,8 +289,10 @@  int CameraSensor::validateSensorDriver()
 
 	ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &rect);
 	if (ret) {
+		analogueCrop_ = activeArea_;
 		LOG(CameraSensor, Warning)
-			<< "Failed to retrieve the sensor crop rectangle";
+			<< "The analogue crop rectangle has been defaulted to: "
+			<< analogueCrop_.toString();
 		err = -EINVAL;
 	}
 
@@ -643,13 +645,20 @@  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
 	 */
 	info->activeAreaSize = { activeArea_.width, activeArea_.height };
 
-	/* It's mandatory for the subdevice to report its crop rectangle. */
-	int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
-	if (ret) {
-		LOG(CameraSensor, Error)
-			<< "Failed to construct camera sensor info: "
-			<< "the camera sensor does not report the crop rectangle";
-		return ret;
+	/*
+	 * \todo Support for retreiving the crop rectangle is scheduled to
+	 * become mandatory. For the time being use the default value if it has
+	 * been initialized at sensor driver validation time.
+	 */
+	if (!analogueCrop_.isNull()) {
+		info->analogCrop = analogueCrop_;
+	} else {
+		int ret = subdev_->getSelection(pad_, V4L2_SEL_TGT_CROP, &info->analogCrop);
+		if (ret) {
+			LOG(CameraSensor, Error)
+				<< "Failed to retrieve the sensor crop rectangle.";
+			return -EINVAL;
+		}
 	}
 
 	/*
@@ -664,7 +673,7 @@  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
 
 	/* The bit depth and image size depend on the currently applied format. */
 	V4L2SubdeviceFormat format{};
-	ret = subdev_->getFormat(pad_, &format);
+	int ret = subdev_->getFormat(pad_, &format);
 	if (ret)
 		return ret;
 	info->bitsPerPixel = format.bitsPerPixel();