[libcamera-devel,v3,02/12] libcamera: camera: Introduce SensorConfiguration

19021 diff mbox series

Introduce SensorConfiguration in the libcamera API.

The SensorConfiguration is part of the CameraConfiguration class
and allows applications to control the sensor settings.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
 include/libcamera/camera.h |  43 +++++++++
 src/libcamera/camera.cpp   | 185 +++++++++++++++++++++++++++++++++++++
 2 files changed, 228 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Fri, Sep 15, 2023 at 03:06:40PM +0200, Jacopo Mondi via libcamera-devel wrote:
> Introduce SensorConfiguration in the libcamera API.
> 
> The SensorConfiguration is part of the CameraConfiguration class
> and allows applications to control the sensor settings.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
>  include/libcamera/camera.h |  43 +++++++++
>  src/libcamera/camera.cpp   | 185 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 228 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 004bc89455f5..b2aa8d467feb 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -19,6 +19,7 @@
>  #include <libcamera/base/signal.h>
>  
>  #include <libcamera/controls.h>
> +#include <libcamera/geometry.h>
>  #include <libcamera/request.h>
>  #include <libcamera/stream.h>
>  #include <libcamera/transform.h>
> @@ -30,6 +31,47 @@ class FrameBufferAllocator;
>  class PipelineHandler;
>  class Request;
>  
> +class SensorConfiguration
> +{
> +public:
> +	unsigned int bitDepth = 0;
> +
> +	Rectangle analogCrop;
> +
> +	struct {
> +		unsigned int binX = 1;
> +		unsigned int binY = 1;
> +	} binning;
> +
> +	struct {
> +		unsigned int xOddInc = 1;
> +		unsigned int xEvenInc = 1;
> +		unsigned int yOddInc = 1;
> +		unsigned int yEvenInc = 1;

What's the reason for exposing the odd and even increments separately,
instead of a skipping factor ?

> +	} skipping;
> +
> +	Size outputSize;
> +
> +	bool valid() const
> +	{
> +		return validate() != Invalid;
> +	}
> +
> +	explicit operator bool() const
> +	{
> +		return validate() == Populated;
> +	}
> +
> +private:
> +	enum Status {
> +		Unpopulated,
> +		Populated,
> +		Invalid,
> +	};
> +
> +	Status validate() const;
> +};
> +
>  class CameraConfiguration
>  {
>  public:
> @@ -66,6 +108,7 @@ public:
>  	bool empty() const;
>  	std::size_t size() const;
>  
> +	SensorConfiguration sensorConfig;
>  	Transform transform;
>  
>  protected:
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 0eecee766f00..f497a35c90fb 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -97,6 +97,21 @@
>   * implemented in the above order at the hardware level. The libcamera pipeline
>   * handlers translate the pipeline model to the real hardware configuration.
>   *
> + * \subsection camera-sensor-mode Camera Sensor Model

s/-mode/-model/

> + *
> + * libcamera allows applications to control the configuration of the camera
> + * sensor, particularly it allows to control the frame geometry and frame
> + * delivery rate of the sensor.

s/delivery //

I'd like to make it clear that this is optional:

 * By default, libcamera configures the camera sensor automatically based on the
 * configuration of the streams. Applications may instead specify a manual
 * configuration for the camera sensor. This allows precise control of the frame
 * geometry and frame rate delivered by the sensor.

> + *
> + * The camera sensor configuration applies to all streams produced by a camera
> + * from the same image source.

This paragraph should go to CameraConfiguration::sensorConfig below.

> + *
> + * More details about the camera sensor model implemented by libcamera are
> + * available in the libcamera camera-sensor-model documentation page.
> + *
> + * The sensor configuration is specified by applications by populating the
> + * CameraConfiguration::sensorConfig class member.

Drop this paragraph, it doesn't belong to the camera model introduction
(none of the other sections here go into the level of details).

> + *
>   * \subsection digital-zoom Digital Zoom
>   *
>   * Digital zoom is implemented as a combination of the cropping and scaling
> @@ -111,6 +126,166 @@ namespace libcamera {
>  
>  LOG_DECLARE_CATEGORY(Camera)
>  
> +/**
> + * \class SensorConfiguration
> + * \brief Camera sensor configuration
> + *
> + * The SensorConfiguration class collects parameters to control the operations
> + * of the camera sensor, accordingly to the abstract camera sensor model

s/according/accordingly/

> + * implemented by libcamera.
> + *
> + * \todo Applications shall fully populate all fields of the
> + * CameraConfiguration::sensorConfig class members before validating the
> + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if
> + * any of its parameters cannot be applied to the sensor in use, the
> + * CameraConfiguration validation process will fail and return
> + * CameraConfiguration::Status::Invalid.
> + *
> + * Applications that populate the SensorConfiguration class members are
> + * expected to be highly-specialized applications that know what sensor
> + * they are operating with and what parameters are valid for the sensor in use.
> + *
> + * A detailed description of the abstract camera sensor model implemented by
> + * libcamera and the description of its configuration parameters is available
> + * in the libcamera documentation camera-sensor-model file.
> + */
> +
> +/**
> + * \enum SensorConfiguration::Status
> + * \brief The sensor configuration validation status
> + */
> +
> +/**
> + * \var SensorConfiguration::bitDepth
> + * \brief The sensor image format bit depth
> + *
> + * The number of bits (resolution) used to represent a pixel sample.
> + */
> +
> +/**
> + * \var SensorConfiguration::analogCrop
> + * \brief The analog crop rectangle
> + *
> + * The selected portion of the active pixel array used to produce the image
> + * frame.
> + */
> +
> +/**
> + * \var SensorConfiguration::binning
> + * \brief Sensor binning configuration
> + *
> + * Refer to the camera-sensor-model documentation for an accurate description
> + * of the binning operations. Disabled by default.
> + */
> +
> +/**
> + * \var SensorConfiguration::binX
> + * \brief Horizontal binning factor
> + *
> + * The horizontal binning factor. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::binY
> + * \brief Vertical binning factor
> + *
> + * The vertical binning factor. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::skipping
> + * \brief The sensor skipping configuration
> + *
> + * Refer to the camera-sensor-model documentation for an accurate description
> + * of the skipping operations.
> + *
> + * If no skipping is performed, all the structure fields should be
> + * set to 1. Disabled by default.
> + */
> +
> +/**
> + * \var SensorConfiguration::xOddInc
> + * \brief Horizontal increment for odd rows. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::xEvenInc
> + * \brief Horizontal increment for even rows. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::yOddInc
> + * \brief Vertical increment for odd columns. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::yEvenInc
> + * \brief Vertical increment for even columns. Default to 1.
> + */
> +
> +/**
> + * \var SensorConfiguration::outputSize
> + * \brief The frame output (visible) size
> + *
> + * The size of the data frame as received by the host processor.
> + */
> +
> +/**
> + * \fn SensorConfiguration::valid() const
> + * \brief Validate the SensorConfiguration
> + *
> + * Validate the sensor configuration.
> + *
> + * \todo A sensor configuration is valid (or well-formed) if it's either
> + * completely un-populated or fully populated. For now allow applications to
> + * populate the bitDepth and the outputSize only.

As I've mentioned before (so it shouldn't come as a surprise), I think
this is a hack (and I think we agree :-)). I'm OK with it in the very
short term, but with IMX519 support that should arrive soon, we'll need
to do better. The IMX519 will likely be supported by the ccs kernel
driver, which isn't mode-based. Configuring the sensor solely through
the output resolution won't work. I plan to rework the CameraSensor
class to support ccs-based sensors, and as part of that work, I will
most likely address this todo item and probably require applications to
select a full sensor configuration.

This doesn't require any immediate change. I've CC'ed David and Naush as
RPi is the first user of this API, and I don't want changes in the near
future to come as a surprise.

> + *
> + * \return True if the SensorConfiguration is either fully populated or
> + * un-populated, false otherwise
> + */
> +
> +/**
> + * \fn SensorConfiguration::operator bool() const
> + * \brief Test if a SensorConfiguration is fully populated
> + * \return True if the SensorConfiguration is fully populated
> + */
> +
> +/**
> + * \brief Validate the sensor configuration
> + *
> + * \todo A sensor configuration is valid (or well-formed) if it's either
> + * completely un-populated or fully populated. For now allow applications to
> + * populate the bitDepth and the outputSize only.
> + *
> + * \return The sensor configuration status
> + * \retval Unpopulated The sensor configuration is fully unpopulated
> + * \retval Populated The sensor configuration is fully populated
> + * \retval Invalid The sensor configuration is invalid (not fully populated
> + * and not fully unpopulated)
> + */
> +SensorConfiguration::Status SensorConfiguration::validate() const
> +{
> +	if (bitDepth && binning.binX && binning.binY &&
> +	    skipping.xOddInc && skipping.yOddInc &&
> +	    skipping.xEvenInc && skipping.yEvenInc &&
> +	    !outputSize.isNull())
> +		return Populated;
> +
> +	/*
> +	 * By default the binning and skipping factors are initialized to 1, but
> +	 * a zero-initialized SensorConfiguration is considered unpopulated
> +	 * as well.
> +	 */
> +	if (!bitDepth &&
> +	    binning.binX <= 1 && binning.binY <= 1 &&
> +	    skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&
> +	    skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&
> +	    outputSize.isNull())
> +		return Unpopulated;
> +
> +	return Invalid;
> +}
> +
>  /**
>   * \class CameraConfiguration
>   * \brief Hold configuration for streams of the camera
> @@ -391,6 +566,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>  	return status;
>  }
>  
> +/**
> + * \var CameraConfiguration::sensorConfig
> + * \brief The camera sensor configuration
> + *
> + * The sensorConfig field allows control of the configuration of the camera
> + * sensor. Refer to the camera-sensor-model documentation and to the
> + * SensorConfiguration class documentation for details about the sensor
> + * configuration process.
> + */
> +
>  /**
>   * \var CameraConfiguration::transform
>   * \brief User-specified transform to be applied to the image

Another small comment.
G
On Fri, Sep 15, 2023 at 07:06:15PM +0300, Laurent Pinchart via libcamera-devel wrote:
> On Fri, Sep 15, 2023 at 03:06:40PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Introduce SensorConfiguration in the libcamera API.
> > 
> > The SensorConfiguration is part of the CameraConfiguration class
> > and allows applications to control the sensor settings.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h |  43 +++++++++
> >  src/libcamera/camera.cpp   | 185 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 228 insertions(+)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 004bc89455f5..b2aa8d467feb 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -19,6 +19,7 @@
> >  #include <libcamera/base/signal.h>
> >  
> >  #include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >  #include <libcamera/transform.h>
> > @@ -30,6 +31,47 @@ class FrameBufferAllocator;
> >  class PipelineHandler;
> >  class Request;
> >  
> > +class SensorConfiguration
> > +{
> > +public:
> > +	unsigned int bitDepth = 0;
> > +
> > +	Rectangle analogCrop;
> > +
> > +	struct {
> > +		unsigned int binX = 1;
> > +		unsigned int binY = 1;
> > +	} binning;
> > +
> > +	struct {
> > +		unsigned int xOddInc = 1;
> > +		unsigned int xEvenInc = 1;
> > +		unsigned int yOddInc = 1;
> > +		unsigned int yEvenInc = 1;
> 
> What's the reason for exposing the odd and even increments separately,
> instead of a skipping factor ?
> 
> > +	} skipping;
> > +
> > +	Size outputSize;
> > +
> > +	bool valid() const
> > +	{
> > +		return validate() != Invalid;
> > +	}
> > +
> > +	explicit operator bool() const
> > +	{
> > +		return validate() == Populated;
> > +	}
> > +
> > +private:
> > +	enum Status {
> > +		Unpopulated,
> > +		Populated,
> > +		Invalid,
> > +	};
> > +
> > +	Status validate() const;
> > +};
> > +
> >  class CameraConfiguration
> >  {
> >  public:
> > @@ -66,6 +108,7 @@ public:
> >  	bool empty() const;
> >  	std::size_t size() const;
> >  
> > +	SensorConfiguration sensorConfig;
> >  	Transform transform;
> >  
> >  protected:
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0eecee766f00..f497a35c90fb 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -97,6 +97,21 @@
> >   * implemented in the above order at the hardware level. The libcamera pipeline
> >   * handlers translate the pipeline model to the real hardware configuration.
> >   *
> > + * \subsection camera-sensor-mode Camera Sensor Model
> 
> s/-mode/-model/
> 
> > + *
> > + * libcamera allows applications to control the configuration of the camera
> > + * sensor, particularly it allows to control the frame geometry and frame
> > + * delivery rate of the sensor.
> 
> s/delivery //
> 
> I'd like to make it clear that this is optional:
> 
>  * By default, libcamera configures the camera sensor automatically based on the
>  * configuration of the streams. Applications may instead specify a manual
>  * configuration for the camera sensor. This allows precise control of the frame
>  * geometry and frame rate delivered by the sensor.
> 
> > + *
> > + * The camera sensor configuration applies to all streams produced by a camera
> > + * from the same image source.
> 
> This paragraph should go to CameraConfiguration::sensorConfig below.
> 
> > + *
> > + * More details about the camera sensor model implemented by libcamera are
> > + * available in the libcamera camera-sensor-model documentation page.
> > + *
> > + * The sensor configuration is specified by applications by populating the
> > + * CameraConfiguration::sensorConfig class member.
> 
> Drop this paragraph, it doesn't belong to the camera model introduction
> (none of the other sections here go into the level of details).
> 
> > + *
> >   * \subsection digital-zoom Digital Zoom
> >   *
> >   * Digital zoom is implemented as a combination of the cropping and scaling
> > @@ -111,6 +126,166 @@ namespace libcamera {
> >  
> >  LOG_DECLARE_CATEGORY(Camera)
> >  
> > +/**
> > + * \class SensorConfiguration
> > + * \brief Camera sensor configuration
> > + *
> > + * The SensorConfiguration class collects parameters to control the operations
> > + * of the camera sensor, accordingly to the abstract camera sensor model
> 
> s/according/accordingly/
> 
> > + * implemented by libcamera.
> > + *
> > + * \todo Applications shall fully populate all fields of the
> > + * CameraConfiguration::sensorConfig class members before validating the
> > + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if
> > + * any of its parameters cannot be applied to the sensor in use, the
> > + * CameraConfiguration validation process will fail and return
> > + * CameraConfiguration::Status::Invalid.
> > + *
> > + * Applications that populate the SensorConfiguration class members are
> > + * expected to be highly-specialized applications that know what sensor
> > + * they are operating with and what parameters are valid for the sensor in use.
> > + *
> > + * A detailed description of the abstract camera sensor model implemented by
> > + * libcamera and the description of its configuration parameters is available
> > + * in the libcamera documentation camera-sensor-model file.
> > + */
> > +
> > +/**
> > + * \enum SensorConfiguration::Status
> > + * \brief The sensor configuration validation status
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::bitDepth
> > + * \brief The sensor image format bit depth
> > + *
> > + * The number of bits (resolution) used to represent a pixel sample.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::analogCrop
> > + * \brief The analog crop rectangle
> > + *
> > + * The selected portion of the active pixel array used to produce the image
> > + * frame.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binning
> > + * \brief Sensor binning configuration
> > + *
> > + * Refer to the camera-sensor-model documentation for an accurate description
> > + * of the binning operations. Disabled by default.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binX
> > + * \brief Horizontal binning factor
> > + *
> > + * The horizontal binning factor. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binY
> > + * \brief Vertical binning factor
> > + *
> > + * The vertical binning factor. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::skipping
> > + * \brief The sensor skipping configuration
> > + *
> > + * Refer to the camera-sensor-model documentation for an accurate description
> > + * of the skipping operations.
> > + *
> > + * If no skipping is performed, all the structure fields should be
> > + * set to 1. Disabled by default.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::xOddInc
> > + * \brief Horizontal increment for odd rows. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::xEvenInc
> > + * \brief Horizontal increment for even rows. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::yOddInc
> > + * \brief Vertical increment for odd columns. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::yEvenInc
> > + * \brief Vertical increment for even columns. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::outputSize
> > + * \brief The frame output (visible) size
> > + *
> > + * The size of the data frame as received by the host processor.
> > + */
> > +
> > +/**
> > + * \fn SensorConfiguration::valid() const
> > + * \brief Validate the SensorConfiguration
> > + *
> > + * Validate the sensor configuration.
> > + *
> > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > + * completely un-populated or fully populated. For now allow applications to
> > + * populate the bitDepth and the outputSize only.
> 
> As I've mentioned before (so it shouldn't come as a surprise), I think
> this is a hack (and I think we agree :-)). I'm OK with it in the very
> short term, but with IMX519 support that should arrive soon, we'll need
> to do better. The IMX519 will likely be supported by the ccs kernel
> driver, which isn't mode-based. Configuring the sensor solely through
> the output resolution won't work. I plan to rework the CameraSensor
> class to support ccs-based sensors, and as part of that work, I will
> most likely address this todo item and probably require applications to
> select a full sensor configuration.
> 
> This doesn't require any immediate change. I've CC'ed David and Naush as
> RPi is the first user of this API, and I don't want changes in the near
> future to come as a surprise.
> 
> > + *
> > + * \return True if the SensorConfiguration is either fully populated or
> > + * un-populated, false otherwise
> > + */
> > +
> > +/**
> > + * \fn SensorConfiguration::operator bool() const

Would this be better turned into a function with an explicit name (such
as populated()) instead of a conversion operator ? Writing

	if (config)
		...

doesn't make it immediately clear to the reader what is being tested.

> > + * \brief Test if a SensorConfiguration is fully populated
> > + * \return True if the SensorConfiguration is fully populated
> > + */
> > +
> > +/**
> > + * \brief Validate the sensor configuration
> > + *
> > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > + * completely un-populated or fully populated. For now allow applications to
> > + * populate the bitDepth and the outputSize only.
> > + *
> > + * \return The sensor configuration status
> > + * \retval Unpopulated The sensor configuration is fully unpopulated
> > + * \retval Populated The sensor configuration is fully populated
> > + * \retval Invalid The sensor configuration is invalid (not fully populated
> > + * and not fully unpopulated)
> > + */
> > +SensorConfiguration::Status SensorConfiguration::validate() const
> > +{
> > +	if (bitDepth && binning.binX && binning.binY &&
> > +	    skipping.xOddInc && skipping.yOddInc &&
> > +	    skipping.xEvenInc && skipping.yEvenInc &&
> > +	    !outputSize.isNull())
> > +		return Populated;
> > +
> > +	/*
> > +	 * By default the binning and skipping factors are initialized to 1, but
> > +	 * a zero-initialized SensorConfiguration is considered unpopulated
> > +	 * as well.
> > +	 */
> > +	if (!bitDepth &&
> > +	    binning.binX <= 1 && binning.binY <= 1 &&
> > +	    skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&
> > +	    skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&
> > +	    outputSize.isNull())
> > +		return Unpopulated;
> > +
> > +	return Invalid;
> > +}
> > +
> >  /**
> >   * \class CameraConfiguration
> >   * \brief Hold configuration for streams of the camera
> > @@ -391,6 +566,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >  	return status;
> >  }
> >  
> > +/**
> > + * \var CameraConfiguration::sensorConfig
> > + * \brief The camera sensor configuration
> > + *
> > + * The sensorConfig field allows control of the configuration of the camera
> > + * sensor. Refer to the camera-sensor-model documentation and to the
> > + * SensorConfiguration class documentation for details about the sensor
> > + * configuration process.
> > + */
> > +
> >  /**
> >   * \var CameraConfiguration::transform
> >   * \brief User-specified transform to be applied to the image

Hi Laurent

On Fri, Sep 15, 2023 at 07:06:15PM +0300, Laurent Pinchart via libcamera-devel wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Fri, Sep 15, 2023 at 03:06:40PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > Introduce SensorConfiguration in the libcamera API.
> >
> > The SensorConfiguration is part of the CameraConfiguration class
> > and allows applications to control the sensor settings.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> >  include/libcamera/camera.h |  43 +++++++++
> >  src/libcamera/camera.cpp   | 185 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 228 insertions(+)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 004bc89455f5..b2aa8d467feb 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -19,6 +19,7 @@
> >  #include <libcamera/base/signal.h>
> >
> >  #include <libcamera/controls.h>
> > +#include <libcamera/geometry.h>
> >  #include <libcamera/request.h>
> >  #include <libcamera/stream.h>
> >  #include <libcamera/transform.h>
> > @@ -30,6 +31,47 @@ class FrameBufferAllocator;
> >  class PipelineHandler;
> >  class Request;
> >
> > +class SensorConfiguration
> > +{
> > +public:
> > +	unsigned int bitDepth = 0;
> > +
> > +	Rectangle analogCrop;
> > +
> > +	struct {
> > +		unsigned int binX = 1;
> > +		unsigned int binY = 1;
> > +	} binning;
> > +
> > +	struct {
> > +		unsigned int xOddInc = 1;
> > +		unsigned int xEvenInc = 1;
> > +		unsigned int yOddInc = 1;
> > +		unsigned int yEvenInc = 1;
>
> What's the reason for exposing the odd and even increments separately,
> instead of a skipping factor ?
>

CCS expresses the two skipping factors separately. I agree there are
not many meaningful ways this two parameters can be combined, but if
one knows how a sensor work it might be more natural expressing them
in this way instead of simply saying x_skip = 2 ( == x_odd_inc=3,
x_even_inc=2)

> > +	} skipping;
> > +
> > +	Size outputSize;
> > +
> > +	bool valid() const
> > +	{
> > +		return validate() != Invalid;
> > +	}
> > +
> > +	explicit operator bool() const
> > +	{
> > +		return validate() == Populated;
> > +	}
> > +
> > +private:
> > +	enum Status {
> > +		Unpopulated,
> > +		Populated,
> > +		Invalid,
> > +	};
> > +
> > +	Status validate() const;
> > +};
> > +
> >  class CameraConfiguration
> >  {
> >  public:
> > @@ -66,6 +108,7 @@ public:
> >  	bool empty() const;
> >  	std::size_t size() const;
> >
> > +	SensorConfiguration sensorConfig;
> >  	Transform transform;
> >
> >  protected:
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0eecee766f00..f497a35c90fb 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -97,6 +97,21 @@
> >   * implemented in the above order at the hardware level. The libcamera pipeline
> >   * handlers translate the pipeline model to the real hardware configuration.
> >   *
> > + * \subsection camera-sensor-mode Camera Sensor Model
>
> s/-mode/-model/
>
> > + *
> > + * libcamera allows applications to control the configuration of the camera
> > + * sensor, particularly it allows to control the frame geometry and frame
> > + * delivery rate of the sensor.
>
> s/delivery //
>
> I'd like to make it clear that this is optional:
>
>  * By default, libcamera configures the camera sensor automatically based on the
>  * configuration of the streams. Applications may instead specify a manual
>  * configuration for the camera sensor. This allows precise control of the frame
>  * geometry and frame rate delivered by the sensor.
>
> > + *
> > + * The camera sensor configuration applies to all streams produced by a camera
> > + * from the same image source.
>
> This paragraph should go to CameraConfiguration::sensorConfig below.
>
> > + *
> > + * More details about the camera sensor model implemented by libcamera are
> > + * available in the libcamera camera-sensor-model documentation page.
> > + *
> > + * The sensor configuration is specified by applications by populating the
> > + * CameraConfiguration::sensorConfig class member.
>
> Drop this paragraph, it doesn't belong to the camera model introduction
> (none of the other sections here go into the level of details).
>
> > + *
> >   * \subsection digital-zoom Digital Zoom
> >   *
> >   * Digital zoom is implemented as a combination of the cropping and scaling
> > @@ -111,6 +126,166 @@ namespace libcamera {
> >
> >  LOG_DECLARE_CATEGORY(Camera)
> >
> > +/**
> > + * \class SensorConfiguration
> > + * \brief Camera sensor configuration
> > + *
> > + * The SensorConfiguration class collects parameters to control the operations
> > + * of the camera sensor, accordingly to the abstract camera sensor model
>
> s/according/accordingly/

Isn't this the case already ? Or did you mean the other way around ?

>
> > + * implemented by libcamera.
> > + *
> > + * \todo Applications shall fully populate all fields of the
> > + * CameraConfiguration::sensorConfig class members before validating the
> > + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if
> > + * any of its parameters cannot be applied to the sensor in use, the
> > + * CameraConfiguration validation process will fail and return
> > + * CameraConfiguration::Status::Invalid.
> > + *
> > + * Applications that populate the SensorConfiguration class members are
> > + * expected to be highly-specialized applications that know what sensor
> > + * they are operating with and what parameters are valid for the sensor in use.
> > + *
> > + * A detailed description of the abstract camera sensor model implemented by
> > + * libcamera and the description of its configuration parameters is available
> > + * in the libcamera documentation camera-sensor-model file.
> > + */
> > +
> > +/**
> > + * \enum SensorConfiguration::Status
> > + * \brief The sensor configuration validation status
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::bitDepth
> > + * \brief The sensor image format bit depth
> > + *
> > + * The number of bits (resolution) used to represent a pixel sample.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::analogCrop
> > + * \brief The analog crop rectangle
> > + *
> > + * The selected portion of the active pixel array used to produce the image
> > + * frame.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binning
> > + * \brief Sensor binning configuration
> > + *
> > + * Refer to the camera-sensor-model documentation for an accurate description
> > + * of the binning operations. Disabled by default.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binX
> > + * \brief Horizontal binning factor
> > + *
> > + * The horizontal binning factor. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::binY
> > + * \brief Vertical binning factor
> > + *
> > + * The vertical binning factor. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::skipping
> > + * \brief The sensor skipping configuration
> > + *
> > + * Refer to the camera-sensor-model documentation for an accurate description
> > + * of the skipping operations.
> > + *
> > + * If no skipping is performed, all the structure fields should be
> > + * set to 1. Disabled by default.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::xOddInc
> > + * \brief Horizontal increment for odd rows. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::xEvenInc
> > + * \brief Horizontal increment for even rows. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::yOddInc
> > + * \brief Vertical increment for odd columns. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::yEvenInc
> > + * \brief Vertical increment for even columns. Default to 1.
> > + */
> > +
> > +/**
> > + * \var SensorConfiguration::outputSize
> > + * \brief The frame output (visible) size
> > + *
> > + * The size of the data frame as received by the host processor.
> > + */
> > +
> > +/**
> > + * \fn SensorConfiguration::valid() const
> > + * \brief Validate the SensorConfiguration
> > + *
> > + * Validate the sensor configuration.
> > + *
> > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > + * completely un-populated or fully populated. For now allow applications to
> > + * populate the bitDepth and the outputSize only.
>
> As I've mentioned before (so it shouldn't come as a surprise), I think
> this is a hack (and I think we agree :-)). I'm OK with it in the very
> short term, but with IMX519 support that should arrive soon, we'll need
> to do better. The IMX519 will likely be supported by the ccs kernel
> driver, which isn't mode-based. Configuring the sensor solely through
> the output resolution won't work. I plan to rework the CameraSensor
> class to support ccs-based sensors, and as part of that work, I will
> most likely address this todo item and probably require applications to
> select a full sensor configuration.
>
> This doesn't require any immediate change. I've CC'ed David and Naush as
> RPi is the first user of this API, and I don't want changes in the near
> future to come as a surprise.
>
> > + *
> > + * \return True if the SensorConfiguration is either fully populated or
> > + * un-populated, false otherwise
> > + */
> > +
> > +/**
> > + * \fn SensorConfiguration::operator bool() const
> > + * \brief Test if a SensorConfiguration is fully populated
> > + * \return True if the SensorConfiguration is fully populated
> > + */
> > +
> > +/**
> > + * \brief Validate the sensor configuration
> > + *
> > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > + * completely un-populated or fully populated. For now allow applications to
> > + * populate the bitDepth and the outputSize only.
> > + *
> > + * \return The sensor configuration status
> > + * \retval Unpopulated The sensor configuration is fully unpopulated
> > + * \retval Populated The sensor configuration is fully populated
> > + * \retval Invalid The sensor configuration is invalid (not fully populated
> > + * and not fully unpopulated)
> > + */
> > +SensorConfiguration::Status SensorConfiguration::validate() const
> > +{
> > +	if (bitDepth && binning.binX && binning.binY &&
> > +	    skipping.xOddInc && skipping.yOddInc &&
> > +	    skipping.xEvenInc && skipping.yEvenInc &&
> > +	    !outputSize.isNull())
> > +		return Populated;
> > +
> > +	/*
> > +	 * By default the binning and skipping factors are initialized to 1, but
> > +	 * a zero-initialized SensorConfiguration is considered unpopulated
> > +	 * as well.
> > +	 */
> > +	if (!bitDepth &&
> > +	    binning.binX <= 1 && binning.binY <= 1 &&
> > +	    skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&
> > +	    skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&
> > +	    outputSize.isNull())
> > +		return Unpopulated;
> > +
> > +	return Invalid;
> > +}
> > +
> >  /**
> >   * \class CameraConfiguration
> >   * \brief Hold configuration for streams of the camera
> > @@ -391,6 +566,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >  	return status;
> >  }
> >
> > +/**
> > + * \var CameraConfiguration::sensorConfig
> > + * \brief The camera sensor configuration
> > + *
> > + * The sensorConfig field allows control of the configuration of the camera
> > + * sensor. Refer to the camera-sensor-model documentation and to the
> > + * SensorConfiguration class documentation for details about the sensor
> > + * configuration process.
> > + */
> > +
> >  /**
> >   * \var CameraConfiguration::transform
> >   * \brief User-specified transform to be applied to the image
>
> --
> Regards,
>
> Laurent Pinchart

Hi Jacopo,

On Sat, Sep 16, 2023 at 12:15:26PM +0200, Jacopo Mondi wrote:
> On Fri, Sep 15, 2023 at 07:06:15PM +0300, Laurent Pinchart via libcamera-devel wrote:
> > On Fri, Sep 15, 2023 at 03:06:40PM +0200, Jacopo Mondi via libcamera-devel wrote:
> > > Introduce SensorConfiguration in the libcamera API.
> > >
> > > The SensorConfiguration is part of the CameraConfiguration class
> > > and allows applications to control the sensor settings.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > Reviewed-by: Naushir Patuck <naush@raspberrypi.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > > ---
> > >  include/libcamera/camera.h |  43 +++++++++
> > >  src/libcamera/camera.cpp   | 185 +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 228 insertions(+)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 004bc89455f5..b2aa8d467feb 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -19,6 +19,7 @@
> > >  #include <libcamera/base/signal.h>
> > >
> > >  #include <libcamera/controls.h>
> > > +#include <libcamera/geometry.h>
> > >  #include <libcamera/request.h>
> > >  #include <libcamera/stream.h>
> > >  #include <libcamera/transform.h>
> > > @@ -30,6 +31,47 @@ class FrameBufferAllocator;
> > >  class PipelineHandler;
> > >  class Request;
> > >
> > > +class SensorConfiguration
> > > +{
> > > +public:
> > > +	unsigned int bitDepth = 0;
> > > +
> > > +	Rectangle analogCrop;
> > > +
> > > +	struct {
> > > +		unsigned int binX = 1;
> > > +		unsigned int binY = 1;
> > > +	} binning;
> > > +
> > > +	struct {
> > > +		unsigned int xOddInc = 1;
> > > +		unsigned int xEvenInc = 1;
> > > +		unsigned int yOddInc = 1;
> > > +		unsigned int yEvenInc = 1;
> >
> > What's the reason for exposing the odd and even increments separately,
> > instead of a skipping factor ?
> 
> CCS expresses the two skipping factors separately. I agree there are
> not many meaningful ways this two parameters can be combined, but if
> one knows how a sensor work it might be more natural expressing them
> in this way instead of simply saying x_skip = 2 ( == x_odd_inc=3,
> x_even_inc=2)

Lots of sensors that support configuring skipping do no expose separate
odd and even skipping increments. That's why I think a simpler parameter
would be better.

Sakari, do you have an opinion on this ? Are there reasonable use cases
for controlling the odd and even increments separately in a way that
couldn't be expressed by a single skipping factor (for each direction) ?

> > > +	} skipping;
> > > +
> > > +	Size outputSize;
> > > +
> > > +	bool valid() const
> > > +	{
> > > +		return validate() != Invalid;
> > > +	}
> > > +
> > > +	explicit operator bool() const
> > > +	{
> > > +		return validate() == Populated;
> > > +	}
> > > +
> > > +private:
> > > +	enum Status {
> > > +		Unpopulated,
> > > +		Populated,
> > > +		Invalid,
> > > +	};
> > > +
> > > +	Status validate() const;
> > > +};
> > > +
> > >  class CameraConfiguration
> > >  {
> > >  public:
> > > @@ -66,6 +108,7 @@ public:
> > >  	bool empty() const;
> > >  	std::size_t size() const;
> > >
> > > +	SensorConfiguration sensorConfig;
> > >  	Transform transform;
> > >
> > >  protected:
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 0eecee766f00..f497a35c90fb 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -97,6 +97,21 @@
> > >   * implemented in the above order at the hardware level. The libcamera pipeline
> > >   * handlers translate the pipeline model to the real hardware configuration.
> > >   *
> > > + * \subsection camera-sensor-mode Camera Sensor Model
> >
> > s/-mode/-model/
> >
> > > + *
> > > + * libcamera allows applications to control the configuration of the camera
> > > + * sensor, particularly it allows to control the frame geometry and frame
> > > + * delivery rate of the sensor.
> >
> > s/delivery //
> >
> > I'd like to make it clear that this is optional:
> >
> >  * By default, libcamera configures the camera sensor automatically based on the
> >  * configuration of the streams. Applications may instead specify a manual
> >  * configuration for the camera sensor. This allows precise control of the frame
> >  * geometry and frame rate delivered by the sensor.
> >
> > > + *
> > > + * The camera sensor configuration applies to all streams produced by a camera
> > > + * from the same image source.
> >
> > This paragraph should go to CameraConfiguration::sensorConfig below.
> >
> > > + *
> > > + * More details about the camera sensor model implemented by libcamera are
> > > + * available in the libcamera camera-sensor-model documentation page.
> > > + *
> > > + * The sensor configuration is specified by applications by populating the
> > > + * CameraConfiguration::sensorConfig class member.
> >
> > Drop this paragraph, it doesn't belong to the camera model introduction
> > (none of the other sections here go into the level of details).
> >
> > > + *
> > >   * \subsection digital-zoom Digital Zoom
> > >   *
> > >   * Digital zoom is implemented as a combination of the cropping and scaling
> > > @@ -111,6 +126,166 @@ namespace libcamera {
> > >
> > >  LOG_DECLARE_CATEGORY(Camera)
> > >
> > > +/**
> > > + * \class SensorConfiguration
> > > + * \brief Camera sensor configuration
> > > + *
> > > + * The SensorConfiguration class collects parameters to control the operations
> > > + * of the camera sensor, accordingly to the abstract camera sensor model
> >
> > s/according/accordingly/
> 
> Isn't this the case already ? Or did you mean the other way around ?

Oops, yes, I meant the other way around.

> > > + * implemented by libcamera.
> > > + *
> > > + * \todo Applications shall fully populate all fields of the
> > > + * CameraConfiguration::sensorConfig class members before validating the
> > > + * CameraConfiguration. If the SensorConfiguration is not fully populated, or if
> > > + * any of its parameters cannot be applied to the sensor in use, the
> > > + * CameraConfiguration validation process will fail and return
> > > + * CameraConfiguration::Status::Invalid.
> > > + *
> > > + * Applications that populate the SensorConfiguration class members are
> > > + * expected to be highly-specialized applications that know what sensor
> > > + * they are operating with and what parameters are valid for the sensor in use.
> > > + *
> > > + * A detailed description of the abstract camera sensor model implemented by
> > > + * libcamera and the description of its configuration parameters is available
> > > + * in the libcamera documentation camera-sensor-model file.
> > > + */
> > > +
> > > +/**
> > > + * \enum SensorConfiguration::Status
> > > + * \brief The sensor configuration validation status
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::bitDepth
> > > + * \brief The sensor image format bit depth
> > > + *
> > > + * The number of bits (resolution) used to represent a pixel sample.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::analogCrop
> > > + * \brief The analog crop rectangle
> > > + *
> > > + * The selected portion of the active pixel array used to produce the image
> > > + * frame.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::binning
> > > + * \brief Sensor binning configuration
> > > + *
> > > + * Refer to the camera-sensor-model documentation for an accurate description
> > > + * of the binning operations. Disabled by default.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::binX
> > > + * \brief Horizontal binning factor
> > > + *
> > > + * The horizontal binning factor. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::binY
> > > + * \brief Vertical binning factor
> > > + *
> > > + * The vertical binning factor. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::skipping
> > > + * \brief The sensor skipping configuration
> > > + *
> > > + * Refer to the camera-sensor-model documentation for an accurate description
> > > + * of the skipping operations.
> > > + *
> > > + * If no skipping is performed, all the structure fields should be
> > > + * set to 1. Disabled by default.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::xOddInc
> > > + * \brief Horizontal increment for odd rows. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::xEvenInc
> > > + * \brief Horizontal increment for even rows. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::yOddInc
> > > + * \brief Vertical increment for odd columns. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::yEvenInc
> > > + * \brief Vertical increment for even columns. Default to 1.
> > > + */
> > > +
> > > +/**
> > > + * \var SensorConfiguration::outputSize
> > > + * \brief The frame output (visible) size
> > > + *
> > > + * The size of the data frame as received by the host processor.
> > > + */
> > > +
> > > +/**
> > > + * \fn SensorConfiguration::valid() const
> > > + * \brief Validate the SensorConfiguration
> > > + *
> > > + * Validate the sensor configuration.
> > > + *
> > > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > > + * completely un-populated or fully populated. For now allow applications to
> > > + * populate the bitDepth and the outputSize only.
> >
> > As I've mentioned before (so it shouldn't come as a surprise), I think
> > this is a hack (and I think we agree :-)). I'm OK with it in the very
> > short term, but with IMX519 support that should arrive soon, we'll need
> > to do better. The IMX519 will likely be supported by the ccs kernel
> > driver, which isn't mode-based. Configuring the sensor solely through
> > the output resolution won't work. I plan to rework the CameraSensor
> > class to support ccs-based sensors, and as part of that work, I will
> > most likely address this todo item and probably require applications to
> > select a full sensor configuration.
> >
> > This doesn't require any immediate change. I've CC'ed David and Naush as
> > RPi is the first user of this API, and I don't want changes in the near
> > future to come as a surprise.
> >
> > > + *
> > > + * \return True if the SensorConfiguration is either fully populated or
> > > + * un-populated, false otherwise
> > > + */
> > > +
> > > +/**
> > > + * \fn SensorConfiguration::operator bool() const
> > > + * \brief Test if a SensorConfiguration is fully populated
> > > + * \return True if the SensorConfiguration is fully populated
> > > + */
> > > +
> > > +/**
> > > + * \brief Validate the sensor configuration
> > > + *
> > > + * \todo A sensor configuration is valid (or well-formed) if it's either
> > > + * completely un-populated or fully populated. For now allow applications to
> > > + * populate the bitDepth and the outputSize only.
> > > + *
> > > + * \return The sensor configuration status
> > > + * \retval Unpopulated The sensor configuration is fully unpopulated
> > > + * \retval Populated The sensor configuration is fully populated
> > > + * \retval Invalid The sensor configuration is invalid (not fully populated
> > > + * and not fully unpopulated)
> > > + */
> > > +SensorConfiguration::Status SensorConfiguration::validate() const
> > > +{
> > > +	if (bitDepth && binning.binX && binning.binY &&
> > > +	    skipping.xOddInc && skipping.yOddInc &&
> > > +	    skipping.xEvenInc && skipping.yEvenInc &&
> > > +	    !outputSize.isNull())
> > > +		return Populated;
> > > +
> > > +	/*
> > > +	 * By default the binning and skipping factors are initialized to 1, but
> > > +	 * a zero-initialized SensorConfiguration is considered unpopulated
> > > +	 * as well.
> > > +	 */
> > > +	if (!bitDepth &&
> > > +	    binning.binX <= 1 && binning.binY <= 1 &&
> > > +	    skipping.xOddInc <= 1 && skipping.yOddInc <= 1 &&
> > > +	    skipping.xEvenInc <= 1 && skipping.yEvenInc <= 1 &&
> > > +	    outputSize.isNull())
> > > +		return Unpopulated;
> > > +
> > > +	return Invalid;
> > > +}
> > > +
> > >  /**
> > >   * \class CameraConfiguration
> > >   * \brief Hold configuration for streams of the camera
> > > @@ -391,6 +566,16 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> > >  	return status;
> > >  }
> > >
> > > +/**
> > > + * \var CameraConfiguration::sensorConfig
> > > + * \brief The camera sensor configuration
> > > + *
> > > + * The sensorConfig field allows control of the configuration of the camera
> > > + * sensor. Refer to the camera-sensor-model documentation and to the
> > > + * SensorConfiguration class documentation for details about the sensor
> > > + * configuration process.
> > > + */
> > > +
> > >  /**
> > >   * \var CameraConfiguration::transform
> > >   * \brief User-specified transform to be applied to the image