[libcamera-devel] [PATCH] libcamera: camera_sensor: Cap resolution to max frame size

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Mar 04, 2021 at 02:17:09PM +0100, Jacopo Mondi wrote:
> Since commit 96aecfe36508 ("libcamera: camera_sensor: Use active area
> size as resolution") the CameraSensor::resolution() method returned the
> sensor's active pixel area size.
> 
> As the CameraSensor::resolution() method is widely used in the library
> code base to retrieve the maximum frame size the sensor can produce,
> in case it is smaller than the pixel area size the returned size cannot
> be used to configure the sensor correctly.
> 
> Fix this by returning the maximum frame resolution the sensor can
> produce, or the pixel area size in case the sensor embeds and ISP that
> can upscale and the supported maximum frame size is thus larger that
> the pixel array size.
> 
> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> ---
>  include/libcamera/internal/camera_sensor.h |  2 +-
>  src/libcamera/camera_sensor.cpp            | 14 +++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 71d012f795fe..3e98f71b5e7f 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -53,7 +53,7 @@ public:
>  	const MediaEntity *entity() const { return entity_; }
>  	const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
>  	const std::vector<Size> &sizes() const { return sizes_; }
> -	Size resolution() const { return activeArea_.size(); }
> +	Size resolution() const;
> 
>  	V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
>  				      const Size &size) const;
> diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> index a04982971616..edc2c27084ed 100644
> --- a/src/libcamera/camera_sensor.cpp
> +++ b/src/libcamera/camera_sensor.cpp
> @@ -551,10 +551,22 @@ int CameraSensor::initProperties()
>   */
> 
>  /**
> - * \fn CameraSensor::resolution()
>   * \brief Retrieve the camera sensor resolution
> + *
> + * The camera sensor resolution is the largest frame size the sensor can produce
> + * or, if the camera sensor embeds and ISP that can up-scale, the pixel area
> + * size.

Should we express it the other way around, with the active pixel area
being the default ?

 * The camera sensor resolution is the active pixel area size, clamped to the
 * maximum frame size the sensor can produce if it is smaller than the active
 * pixel area.

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

> + *
> + * \todo Consider if it desirable to distinguish between the maximum resolution
> + * the sensor can produce (also including upscaled ones) and the actual pixel
> + * array size by splitting this method in two.
> + *
>   * \return The camera sensor resolution in pixels
>   */
> +Size CameraSensor::resolution() const
> +{
> +	return std::min(sizes_.back(), activeArea_.size());
> +}
> 
>  /**
>   * \brief Retrieve the best sensor format for a desired output

-- 
Regards,

Laurent Pinchart