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

[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 at ideasonboard.com>
> > > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > > Reviewed-by: Kieran Bingham <kieran.bingham at 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

-- 
Regards,

Laurent Pinchart