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

[Laurent Pinchart]

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 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 ?

> +	} 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

-- 
Regards,

Laurent Pinchart