[libcamera-devel,v5,02/13] libcamera: camera: Introduce SensorConfiguration

19066 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 |  27 +++++++
 src/libcamera/camera.cpp   | 144 +++++++++++++++++++++++++++++++++++++
 2 files changed, 171 insertions(+)

Hi Jacopo,

Thank you for the patch.

On Thu, Sep 21, 2023 at 06:55:39PM +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 |  27 +++++++
>  src/libcamera/camera.cpp   | 144 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 171 insertions(+)
> 
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 004bc89455f5..92a948e0f53b 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -9,6 +9,7 @@
>  
>  #include <initializer_list>
>  #include <memory>
> +#include <optional>
>  #include <set>
>  #include <stdint.h>
>  #include <string>
> @@ -19,6 +20,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 +32,30 @@ 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;
> +	} skipping;
> +
> +	Size outputSize;
> +
> +	bool valid() const;

We call those functions isValid() in libcamera.

> +};
> +
>  class CameraConfiguration
>  {
>  public:
> @@ -66,6 +92,7 @@ public:
>  	bool empty() const;
>  	std::size_t size() const;
>  
> +	std::optional<SensorConfiguration> sensorConfig;
>  	Transform transform;
>  
>  protected:
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 0eecee766f00..05a47444be89 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -97,6 +97,16 @@
>   * 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-model Camera Sensor Model
> + *
> + * 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.
> + *
> + * More details about the camera sensor model implemented by libcamera are
> + * available in the libcamera camera-sensor-model documentation page.
> + *
>   * \subsection digital-zoom Digital Zoom
>   *
>   * Digital zoom is implemented as a combination of the cropping and scaling
> @@ -111,6 +121,127 @@ 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/accordingly/according/

> + * 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.

I think we should implement this already, right away. To avoid delaying
this series I will submit a patch on top.

> + *
> + * 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.
> + */
> +
> +/**
> + * \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.
> + */
> +
> +/**
> + * \brief Validate the sensor configuration

* \brief Check if the sensor configuration is valid

to be consistent with the rest of the documentation.

> + *
> + * A sensor configuration is valid if it's fully populated.
> + *
> + * \todo For now allow applications to populate the bitDepth and the outputSize
> + * only as skipping and binnings factors are initialized to 1 and the analog
> + * crop is ignored.

I will fix this on top too.

> + *
> + * \return True if the sensor configuration is valid, false otherwise.

s/.$//

> + */
> +bool SensorConfiguration::valid() const
> +{
> +	if (bitDepth && binning.binX && binning.binY &&
> +	    skipping.xOddInc && skipping.yOddInc &&
> +	    skipping.xEvenInc && skipping.yEvenInc &&
> +	    !outputSize.isNull())
> +		return true;
> +
> +	return false;
> +}
> +
>  /**
>   * \class CameraConfiguration
>   * \brief Hold configuration for streams of the camera
> @@ -391,6 +522,19 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
>  	return status;
>  }
>  
> +/**
> + * \var CameraConfiguration::sensorConfig
> + * \brief The camera sensor configuration
> + *
> + * The sensorConfig member 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.
> + *
> + * The camera sensor configuration applies to all streams produced by a camera
> + * from the same image source.

I'd like to add

 * If sensorConfig is not set, the camera will configure the sensor
 * automatically based on the configuration of the streams.

We can rephrase the block as follows:

 * The sensorConfig member allows manual control of the configuration of the
 * camera sensor. By default, if sensorConfig is not set, the camera will
 * configure the sensor automatically based on the configuration of the streams.
 * Applications can override this by manually specifying the full sensor
 * configuration.
 *
 * Refer to the camera-sensor-model documentation and to the SensorConfiguration
 * class documentation for details about the sensor configuration process.
 *
 * The camera sensor configuration applies to all streams produced by a camera
 * from the same image source.

If those changes are fine with you, I can update the patch when pushing.

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

> + */
> +
>  /**
>   * \var CameraConfiguration::transform
>   * \brief User-specified transform to be applied to the image

Hi,

Le mercredi 27 septembre 2023 à 00:33 +0300, Laurent Pinchart via libcamera-
devel a écrit :
> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Thu, Sep 21, 2023 at 06:55:39PM +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 |  27 +++++++
> >  src/libcamera/camera.cpp   | 144 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 171 insertions(+)
> > 
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 004bc89455f5..92a948e0f53b 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -9,6 +9,7 @@
> >  
> >  #include <initializer_list>
> >  #include <memory>
> > +#include <optional>
> >  #include <set>
> >  #include <stdint.h>
> >  #include <string>
> > @@ -19,6 +20,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 +32,30 @@ 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;
> > +	} skipping;
> > +
> > +	Size outputSize;
> > +
> > +	bool valid() const;
> 
> We call those functions isValid() in libcamera.
> 
> > +};
> > +
> >  class CameraConfiguration
> >  {
> >  public:
> > @@ -66,6 +92,7 @@ public:
> >  	bool empty() const;
> >  	std::size_t size() const;
> >  
> > +	std::optional<SensorConfiguration> sensorConfig;
> >  	Transform transform;
> >  
> >  protected:
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 0eecee766f00..05a47444be89 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -97,6 +97,16 @@
> >   * 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-model Camera Sensor Model
> > + *
> > + * 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.
> > + *
> > + * More details about the camera sensor model implemented by libcamera are
> > + * available in the libcamera camera-sensor-model documentation page.
> > + *
> >   * \subsection digital-zoom Digital Zoom
> >   *
> >   * Digital zoom is implemented as a combination of the cropping and scaling
> > @@ -111,6 +121,127 @@ 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/accordingly/according/
> 
> > + * 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.
> 
> I think we should implement this already, right away. To avoid delaying
> this series I will submit a patch on top.
> 
> > + *
> > + * 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.
> > + */
> > +
> > +/**
> > + * \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.
> > + */
> > +
> > +/**
> > + * \brief Validate the sensor configuration
> 
> * \brief Check if the sensor configuration is valid
> 
> to be consistent with the rest of the documentation.
> 
> > + *
> > + * A sensor configuration is valid if it's fully populated.
> > + *
> > + * \todo For now allow applications to populate the bitDepth and the outputSize
> > + * only as skipping and binnings factors are initialized to 1 and the analog
> > + * crop is ignored.
> 
> I will fix this on top too.
> 
> > + *
> > + * \return True if the sensor configuration is valid, false otherwise.
> 
> s/.$//
> 
> > + */
> > +bool SensorConfiguration::valid() const
> > +{
> > +	if (bitDepth && binning.binX && binning.binY &&
> > +	    skipping.xOddInc && skipping.yOddInc &&
> > +	    skipping.xEvenInc && skipping.yEvenInc &&
> > +	    !outputSize.isNull())
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> >  /**
> >   * \class CameraConfiguration
> >   * \brief Hold configuration for streams of the camera
> > @@ -391,6 +522,19 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> >  	return status;
> >  }
> >  
> > +/**
> > + * \var CameraConfiguration::sensorConfig
> > + * \brief The camera sensor configuration
> > + *
> > + * The sensorConfig member 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.
> > + *
> > + * The camera sensor configuration applies to all streams produced by a camera
> > + * from the same image source.
> 
> I'd like to add
> 
>  * If sensorConfig is not set, the camera will configure the sensor
>  * automatically based on the configuration of the streams.
> 
> We can rephrase the block as follows:
> 
>  * The sensorConfig member allows manual control of the configuration of the
>  * camera sensor. By default, if sensorConfig is not set, the camera will
>  * configure the sensor automatically based on the configuration of the streams.
>  * Applications can override this by manually specifying the full sensor
>  * configuration.
>  *
>  * Refer to the camera-sensor-model documentation and to the SensorConfiguration
>  * class documentation for details about the sensor configuration process.
>  *
>  * The camera sensor configuration applies to all streams produced by a camera
>  * from the same image source.
> 
> If those changes are fine with you, I can update the patch when pushing.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Did my first read of this, with considering the libcamera-app and gstreamer RFC
for sensor configuration. I think in order to actually solve someone problem, we
need a mechanism to retrieve the IPA chosen configuration after validation with
automatic configuration.

This implementation only focus on hardware aware software, so does not really
address any publicly discussed use cases. My general advise if you want to make
new API that are clearly useful, is to take one of the integration you control
and demonstrate its usability. Just adding configuration is agreed valid, but is
it usable by anyone using libcamera in real ?

Nicolas

> 
> > + */
> > +
> >  /**
> >   * \var CameraConfiguration::transform
> >   * \brief User-specified transform to be applied to the image
>

Hi Nicolas,

On Thu, Oct 05, 2023 at 09:30:53AM -0400, Nicolas Dufresne wrote:
> Le mercredi 27 septembre 2023 à 00:33 +0300, Laurent Pinchart via libcamera-devel a écrit :
> > On Thu, Sep 21, 2023 at 06:55:39PM +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 |  27 +++++++
> > >  src/libcamera/camera.cpp   | 144 +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 171 insertions(+)
> > > 
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 004bc89455f5..92a948e0f53b 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -9,6 +9,7 @@
> > >  
> > >  #include <initializer_list>
> > >  #include <memory>
> > > +#include <optional>
> > >  #include <set>
> > >  #include <stdint.h>
> > >  #include <string>
> > > @@ -19,6 +20,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 +32,30 @@ 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;
> > > +	} skipping;
> > > +
> > > +	Size outputSize;
> > > +
> > > +	bool valid() const;
> > 
> > We call those functions isValid() in libcamera.
> > 
> > > +};
> > > +
> > >  class CameraConfiguration
> > >  {
> > >  public:
> > > @@ -66,6 +92,7 @@ public:
> > >  	bool empty() const;
> > >  	std::size_t size() const;
> > >  
> > > +	std::optional<SensorConfiguration> sensorConfig;
> > >  	Transform transform;
> > >  
> > >  protected:
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 0eecee766f00..05a47444be89 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -97,6 +97,16 @@
> > >   * 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-model Camera Sensor Model
> > > + *
> > > + * 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.
> > > + *
> > > + * More details about the camera sensor model implemented by libcamera are
> > > + * available in the libcamera camera-sensor-model documentation page.
> > > + *
> > >   * \subsection digital-zoom Digital Zoom
> > >   *
> > >   * Digital zoom is implemented as a combination of the cropping and scaling
> > > @@ -111,6 +121,127 @@ 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/accordingly/according/
> > 
> > > + * 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.
> > 
> > I think we should implement this already, right away. To avoid delaying
> > this series I will submit a patch on top.
> > 
> > > + *
> > > + * 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.
> > > + */
> > > +
> > > +/**
> > > + * \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.
> > > + */
> > > +
> > > +/**
> > > + * \brief Validate the sensor configuration
> > 
> > * \brief Check if the sensor configuration is valid
> > 
> > to be consistent with the rest of the documentation.
> > 
> > > + *
> > > + * A sensor configuration is valid if it's fully populated.
> > > + *
> > > + * \todo For now allow applications to populate the bitDepth and the outputSize
> > > + * only as skipping and binnings factors are initialized to 1 and the analog
> > > + * crop is ignored.
> > 
> > I will fix this on top too.
> > 
> > > + *
> > > + * \return True if the sensor configuration is valid, false otherwise.
> > 
> > s/.$//
> > 
> > > + */
> > > +bool SensorConfiguration::valid() const
> > > +{
> > > +	if (bitDepth && binning.binX && binning.binY &&
> > > +	    skipping.xOddInc && skipping.yOddInc &&
> > > +	    skipping.xEvenInc && skipping.yEvenInc &&
> > > +	    !outputSize.isNull())
> > > +		return true;
> > > +
> > > +	return false;
> > > +}
> > > +
> > >  /**
> > >   * \class CameraConfiguration
> > >   * \brief Hold configuration for streams of the camera
> > > @@ -391,6 +522,19 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF
> > >  	return status;
> > >  }
> > >  
> > > +/**
> > > + * \var CameraConfiguration::sensorConfig
> > > + * \brief The camera sensor configuration
> > > + *
> > > + * The sensorConfig member 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.
> > > + *
> > > + * The camera sensor configuration applies to all streams produced by a camera
> > > + * from the same image source.
> > 
> > I'd like to add
> > 
> >  * If sensorConfig is not set, the camera will configure the sensor
> >  * automatically based on the configuration of the streams.
> > 
> > We can rephrase the block as follows:
> > 
> >  * The sensorConfig member allows manual control of the configuration of the
> >  * camera sensor. By default, if sensorConfig is not set, the camera will
> >  * configure the sensor automatically based on the configuration of the streams.
> >  * Applications can override this by manually specifying the full sensor
> >  * configuration.
> >  *
> >  * Refer to the camera-sensor-model documentation and to the SensorConfiguration
> >  * class documentation for details about the sensor configuration process.
> >  *
> >  * The camera sensor configuration applies to all streams produced by a camera
> >  * from the same image source.
> > 
> > If those changes are fine with you, I can update the patch when pushing.
> > 
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> Did my first read of this, with considering the libcamera-app and gstreamer RFC
> for sensor configuration. I think in order to actually solve someone problem, we
> need a mechanism to retrieve the IPA chosen configuration after validation with
> automatic configuration.
> 
> This implementation only focus on hardware aware software, so does not really
> address any publicly discussed use cases. My general advise if you want to make
> new API that are clearly useful, is to take one of the integration you control
> and demonstrate its usability. Just adding configuration is agreed valid, but is
> it usable by anyone using libcamera in real ?

I don't like this API much (I don't think this is a surprise to anyone,
I've voiced that opinion already). I however think it's a step towards
solving the problem. We'll take further steps :-)

> > > + */
> > > +
> > >  /**
> > >   * \var CameraConfiguration::transform
> > >   * \brief User-specified transform to be applied to the image