| Message ID | 20230915130650.35691-3-jacopo.mondi@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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
Hi Laurent, Jacopo, Sorry for the late reply. On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > + 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 (sub-sampling) is generally independent horizontally and vertically, binning is not. On some sensors reasonable output sizes may be impelemented a combination of the two.
On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > Hi Laurent, Jacopo, > > Sorry for the late reply. > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > + 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 (sub-sampling) is generally independent horizontally and > vertically, binning is not. On some sensors reasonable output sizes may be > impelemented a combination of the two. There's no disagreement that horizontal and vertical factors need to be expressed independently. The question was about independent even and odd increments.
Hi Laurent, On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > Hi Laurent, Jacopo, > > > > Sorry for the late reply. > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > vertically, binning is not. On some sensors reasonable output sizes may be > > impelemented a combination of the two. > > There's no disagreement that horizontal and vertical factors need to be > expressed independently. The question was about independent even and odd > increments. That's what I meant. CCS separates odd and even increments but in practice the odd increment should always be 1. I guess we could add support for these as well. I'd be inclined to support sub-sampling using controls, separately for each direction.
On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > impelemented a combination of the two. > > > > There's no disagreement that horizontal and vertical factors need to be > > expressed independently. The question was about independent even and odd > > increments. > > That's what I meant. > > CCS separates odd and even increments but in practice the odd increment > should always be 1. I guess we could add support for these as well. I'm not asking to add support for exposing the odd and even increments to userspace. Quite the opposite, I was proposing combining them into a single factor in the API that libcamera exposes to applications. The feedback I would like is whether or not there is a real need for applications to configure the increments separately. If so, why ? > I'd be > inclined to support sub-sampling using controls, separately for each > direction. Is this still related to the odd and even increments, or a separate topic ? If still related, could you please explain more clearly what you mean ?
On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > impelemented a combination of the two. > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > expressed independently. The question was about independent even and odd > > > increments. > > > > That's what I meant. > > > > CCS separates odd and even increments but in practice the odd increment > > should always be 1. I guess we could add support for these as well. > > I'm not asking to add support for exposing the odd and even increments > to userspace. Quite the opposite, I was proposing combining them into a > single factor in the API that libcamera exposes to applications. The > feedback I would like is whether or not there is a real need for > applications to configure the increments separately. If so, why ? The limits for horizontal and vertical sub-sampling are not connected in hardware. Beyond that, as I noted above, some reasonable output sizes are achieved as combination of sub-sampling and binning on some sensors. Therefore I don't think you can combine the two, at least without limiting what can be supported, which is why I think horizontal and vertical sub-sampling need to remain separate. > > > I'd be > > inclined to support sub-sampling using controls, separately for each > > direction. > > Is this still related to the odd and even increments, or a separate > topic ? If still related, could you please explain more clearly what you > mean ? What's required for sub-sampling, i.e. odd increments. The even increments could be added later on. In fact, even increments appear to be useful on monochrome sensors. I'll add this to the next version of the sensor interface patches.
On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > impelemented a combination of the two. > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > expressed independently. The question was about independent even and odd > > > > increments. > > > > > > That's what I meant. > > > > > > CCS separates odd and even increments but in practice the odd increment > > > should always be 1. I guess we could add support for these as well. > > > > I'm not asking to add support for exposing the odd and even increments > > to userspace. Quite the opposite, I was proposing combining them into a > > single factor in the API that libcamera exposes to applications. The > > feedback I would like is whether or not there is a real need for > > applications to configure the increments separately. If so, why ? > > The limits for horizontal and vertical sub-sampling are not connected in > hardware. Beyond that, as I noted above, some reasonable output sizes are > achieved as combination of sub-sampling and binning on some sensors. > Therefore I don't think you can combine the two, at least without limiting > what can be supported, which is why I think horizontal and vertical > sub-sampling need to remain separate. ... I don't think we understand each other. The odd and even increments exist both horizontally and vertically. Quoting myself from a previous e-mail, > > > > There's no disagreement that horizontal and vertical factors need to be > > > > expressed independently. The question was about independent even and odd > > > > increments. > > > I'd be > > > inclined to support sub-sampling using controls, separately for each > > > direction. > > > > Is this still related to the odd and even increments, or a separate > > topic ? If still related, could you please explain more clearly what you > > mean ? > > What's required for sub-sampling, i.e. odd increments. The even increments > could be added later on. Aren't both odd and even increments related to subsampling ? > In fact, even increments appear to be useful on monochrome sensors. > > I'll add this to the next version of the sensor interface patches.
On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > impelemented a combination of the two. > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > expressed independently. The question was about independent even and odd > > > > > increments. > > > > > > > > That's what I meant. > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > should always be 1. I guess we could add support for these as well. > > > > > > I'm not asking to add support for exposing the odd and even increments > > > to userspace. Quite the opposite, I was proposing combining them into a > > > single factor in the API that libcamera exposes to applications. The > > > feedback I would like is whether or not there is a real need for > > > applications to configure the increments separately. If so, why ? > > > > The limits for horizontal and vertical sub-sampling are not connected in > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > achieved as combination of sub-sampling and binning on some sensors. > > Therefore I don't think you can combine the two, at least without limiting > > what can be supported, which is why I think horizontal and vertical > > sub-sampling need to remain separate. > > ... I don't think we understand each other. > > The odd and even increments exist both horizontally and vertically. > Quoting myself from a previous e-mail, > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > expressed independently. The question was about independent even and odd > > > > > increments. Ah, right. As noted above, the even increment is almost always 1. In practice this could have other values on monochrome sensors. We could expose both (via controls) right from the begininning, I think that makes sense. > > > > > > I'd be > > > > inclined to support sub-sampling using controls, separately for each > > > > direction. > > > > > > Is this still related to the odd and even increments, or a separate > > > topic ? If still related, could you please explain more clearly what you > > > mean ? > > > > What's required for sub-sampling, i.e. odd increments. The even increments > > could be added later on. > > Aren't both odd and even increments related to subsampling ? Yes. > > > In fact, even increments appear to be useful on monochrome sensors. > > > > I'll add this to the next version of the sensor interface patches.
On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > expressed independently. The question was about independent even and odd > > > > > > increments. > > > > > > > > > > That's what I meant. > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > single factor in the API that libcamera exposes to applications. The > > > > feedback I would like is whether or not there is a real need for > > > > applications to configure the increments separately. If so, why ? > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > achieved as combination of sub-sampling and binning on some sensors. > > > Therefore I don't think you can combine the two, at least without limiting > > > what can be supported, which is why I think horizontal and vertical > > > sub-sampling need to remain separate. > > > > ... I don't think we understand each other. > > > > The odd and even increments exist both horizontally and vertically. > > Quoting myself from a previous e-mail, > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > expressed independently. The question was about independent even and odd > > > > > > increments. > > Ah, right. As noted above, the even increment is almost always 1. In > practice this could have other values on monochrome sensors. We could > expose both (via controls) right from the begininning, I think that makes > sense. What's the use case ? > > > > > I'd be > > > > > inclined to support sub-sampling using controls, separately for each > > > > > direction. > > > > > > > > Is this still related to the odd and even increments, or a separate > > > > topic ? If still related, could you please explain more clearly what you > > > > mean ? > > > > > > What's required for sub-sampling, i.e. odd increments. The even increments > > > could be added later on. > > > > Aren't both odd and even increments related to subsampling ? > > Yes. > > > > In fact, even increments appear to be useful on monochrome sensors. > > > > > > I'll add this to the next version of the sensor interface patches.
On Mon, Nov 11, 2024 at 11:27:02AM +0200, Laurent Pinchart wrote: > On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > increments. > > > > > > > > > > > > That's what I meant. > > > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > > single factor in the API that libcamera exposes to applications. The > > > > > feedback I would like is whether or not there is a real need for > > > > > applications to configure the increments separately. If so, why ? > > > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > > achieved as combination of sub-sampling and binning on some sensors. > > > > Therefore I don't think you can combine the two, at least without limiting > > > > what can be supported, which is why I think horizontal and vertical > > > > sub-sampling need to remain separate. > > > > > > ... I don't think we understand each other. > > > > > > The odd and even increments exist both horizontally and vertically. > > > Quoting myself from a previous e-mail, > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > increments. > > > > Ah, right. As noted above, the even increment is almost always 1. In > > practice this could have other values on monochrome sensors. We could > > expose both (via controls) right from the begininning, I think that makes > > sense. > > What's the use case ? To do sub-sampling on monochrome sensors that require even values (and the same values for odd and even increments). There are different options in how this can be implemented, see table 78 (page 145) and the examples on page 148 in CCS 1.1 spec.
On Mon, Nov 11, 2024 at 09:46:46AM +0000, Sakari Ailus wrote: > On Mon, Nov 11, 2024 at 11:27:02AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > > > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > increments. > > > > > > > > > > > > > > That's what I meant. > > > > > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > > > single factor in the API that libcamera exposes to applications. The > > > > > > feedback I would like is whether or not there is a real need for > > > > > > applications to configure the increments separately. If so, why ? > > > > > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > > > achieved as combination of sub-sampling and binning on some sensors. > > > > > Therefore I don't think you can combine the two, at least without limiting > > > > > what can be supported, which is why I think horizontal and vertical > > > > > sub-sampling need to remain separate. > > > > > > > > ... I don't think we understand each other. > > > > > > > > The odd and even increments exist both horizontally and vertically. > > > > Quoting myself from a previous e-mail, > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > increments. > > > > > > Ah, right. As noted above, the even increment is almost always 1. In > > > practice this could have other values on monochrome sensors. We could > > > expose both (via controls) right from the begininning, I think that makes > > > sense. > > > > What's the use case ? > > To do sub-sampling on monochrome sensors that require even values (and the > same values for odd and even increments). There are different options in > how this can be implemented, see table 78 (page 145) and the examples on > page 148 in CCS 1.1 spec. Does this have to be exposed to userspace, or can appropriate even and odd increments be computed by the kernel ?
On Mon, Nov 11, 2024 at 11:59:50AM +0200, Laurent Pinchart wrote: > On Mon, Nov 11, 2024 at 09:46:46AM +0000, Sakari Ailus wrote: > > On Mon, Nov 11, 2024 at 11:27:02AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > > > > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > > > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > increments. > > > > > > > > > > > > > > > > That's what I meant. > > > > > > > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > > > > single factor in the API that libcamera exposes to applications. The > > > > > > > feedback I would like is whether or not there is a real need for > > > > > > > applications to configure the increments separately. If so, why ? > > > > > > > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > > > > achieved as combination of sub-sampling and binning on some sensors. > > > > > > Therefore I don't think you can combine the two, at least without limiting > > > > > > what can be supported, which is why I think horizontal and vertical > > > > > > sub-sampling need to remain separate. > > > > > > > > > > ... I don't think we understand each other. > > > > > > > > > > The odd and even increments exist both horizontally and vertically. > > > > > Quoting myself from a previous e-mail, > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > increments. > > > > > > > > Ah, right. As noted above, the even increment is almost always 1. In > > > > practice this could have other values on monochrome sensors. We could > > > > expose both (via controls) right from the begininning, I think that makes > > > > sense. > > > > > > What's the use case ? > > > > To do sub-sampling on monochrome sensors that require even values (and the > > same values for odd and even increments). There are different options in > > how this can be implemented, see table 78 (page 145) and the examples on > > page 148 in CCS 1.1 spec. > > Does this have to be exposed to userspace, or can appropriate even and > odd increments be computed by the kernel ? It would seem like that they could, but if the API e.g. CCS provides is more expressive than what we provide towards the user space, there could be situations where there are more options with different image quality related properties. I'm not sure if we'll ever see that, though -- right now that's not the case at least. I guess we could do with ratios for now, and if something else is needed, then add support for that.
On Mon, Nov 11, 2024 at 10:06:14AM +0000, Sakari Ailus wrote: > On Mon, Nov 11, 2024 at 11:59:50AM +0200, Laurent Pinchart wrote: > > On Mon, Nov 11, 2024 at 09:46:46AM +0000, Sakari Ailus wrote: > > > On Mon, Nov 11, 2024 at 11:27:02AM +0200, Laurent Pinchart wrote: > > > > On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > > > > > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > > > > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > > > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > > increments. > > > > > > > > > > > > > > > > > > That's what I meant. > > > > > > > > > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > > > > > single factor in the API that libcamera exposes to applications. The > > > > > > > > feedback I would like is whether or not there is a real need for > > > > > > > > applications to configure the increments separately. If so, why ? > > > > > > > > > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > > > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > > > > > achieved as combination of sub-sampling and binning on some sensors. > > > > > > > Therefore I don't think you can combine the two, at least without limiting > > > > > > > what can be supported, which is why I think horizontal and vertical > > > > > > > sub-sampling need to remain separate. > > > > > > > > > > > > ... I don't think we understand each other. > > > > > > > > > > > > The odd and even increments exist both horizontally and vertically. > > > > > > Quoting myself from a previous e-mail, > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > > increments. > > > > > > > > > > Ah, right. As noted above, the even increment is almost always 1. In > > > > > practice this could have other values on monochrome sensors. We could > > > > > expose both (via controls) right from the begininning, I think that makes > > > > > sense. > > > > > > > > What's the use case ? > > > > > > To do sub-sampling on monochrome sensors that require even values (and the > > > same values for odd and even increments). There are different options in > > > how this can be implemented, see table 78 (page 145) and the examples on > > > page 148 in CCS 1.1 spec. > > > > Does this have to be exposed to userspace, or can appropriate even and > > odd increments be computed by the kernel ? > > It would seem like that they could, but if the API e.g. CCS provides is > more expressive than what we provide towards the user space, there could be > situations where there are more options with different image quality > related properties. I'm not sure if we'll ever see that, though -- right > now that's not the case at least. I'm not denying it can't happen, but I'd like to better understand the needs. If we decide to expose separate odd and even increments to userspace, we also need to explain to userspace how those need to be computed. My understanding of the odd/even increments is that they are meant to make the sub-sampling configuration of the sensor independent from the CFA pattern. Is there more to it ? > I guess we could do with ratios for now, and if something else is needed, > then add support for that.
Hi Laurent, On Tue, Nov 12, 2024 at 08:27:38AM +0200, Laurent Pinchart wrote: > On Mon, Nov 11, 2024 at 10:06:14AM +0000, Sakari Ailus wrote: > > On Mon, Nov 11, 2024 at 11:59:50AM +0200, Laurent Pinchart wrote: > > > On Mon, Nov 11, 2024 at 09:46:46AM +0000, Sakari Ailus wrote: > > > > On Mon, Nov 11, 2024 at 11:27:02AM +0200, Laurent Pinchart wrote: > > > > > On Mon, Nov 11, 2024 at 09:04:03AM +0000, Sakari Ailus wrote: > > > > > > On Mon, Nov 11, 2024 at 10:57:55AM +0200, Laurent Pinchart wrote: > > > > > > > On Mon, Nov 11, 2024 at 08:34:53AM +0000, Sakari Ailus wrote: > > > > > > > > On Mon, Nov 11, 2024 at 10:12:30AM +0200, Laurent Pinchart wrote: > > > > > > > > > On Mon, Nov 11, 2024 at 08:04:19AM +0000, Sakari Ailus wrote: > > > > > > > > > > On Sun, Nov 10, 2024 at 09:05:03PM +0200, Laurent Pinchart wrote: > > > > > > > > > > > On Sun, Nov 10, 2024 at 11:25:08AM +0000, Sakari Ailus wrote: > > > > > > > > > > > > On Sun, Sep 17, 2023 at 06:45:55PM +0300, Laurent Pinchart wrote: > > > > > > > > > > > > > > > > + 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 (sub-sampling) is generally independent horizontally and > > > > > > > > > > > > vertically, binning is not. On some sensors reasonable output sizes may be > > > > > > > > > > > > impelemented a combination of the two. > > > > > > > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > > > increments. > > > > > > > > > > > > > > > > > > > > That's what I meant. > > > > > > > > > > > > > > > > > > > > CCS separates odd and even increments but in practice the odd increment > > > > > > > > > > should always be 1. I guess we could add support for these as well. > > > > > > > > > > > > > > > > > > I'm not asking to add support for exposing the odd and even increments > > > > > > > > > to userspace. Quite the opposite, I was proposing combining them into a > > > > > > > > > single factor in the API that libcamera exposes to applications. The > > > > > > > > > feedback I would like is whether or not there is a real need for > > > > > > > > > applications to configure the increments separately. If so, why ? > > > > > > > > > > > > > > > > The limits for horizontal and vertical sub-sampling are not connected in > > > > > > > > hardware. Beyond that, as I noted above, some reasonable output sizes are > > > > > > > > achieved as combination of sub-sampling and binning on some sensors. > > > > > > > > Therefore I don't think you can combine the two, at least without limiting > > > > > > > > what can be supported, which is why I think horizontal and vertical > > > > > > > > sub-sampling need to remain separate. > > > > > > > > > > > > > > ... I don't think we understand each other. > > > > > > > > > > > > > > The odd and even increments exist both horizontally and vertically. > > > > > > > Quoting myself from a previous e-mail, > > > > > > > > > > > > > > > > > > There's no disagreement that horizontal and vertical factors need to be > > > > > > > > > > > expressed independently. The question was about independent even and odd > > > > > > > > > > > increments. > > > > > > > > > > > > Ah, right. As noted above, the even increment is almost always 1. In > > > > > > practice this could have other values on monochrome sensors. We could > > > > > > expose both (via controls) right from the begininning, I think that makes > > > > > > sense. > > > > > > > > > > What's the use case ? > > > > > > > > To do sub-sampling on monochrome sensors that require even values (and the > > > > same values for odd and even increments). There are different options in > > > > how this can be implemented, see table 78 (page 145) and the examples on > > > > page 148 in CCS 1.1 spec. > > > > > > Does this have to be exposed to userspace, or can appropriate even and > > > odd increments be computed by the kernel ? > > > > It would seem like that they could, but if the API e.g. CCS provides is > > more expressive than what we provide towards the user space, there could be > > situations where there are more options with different image quality > > related properties. I'm not sure if we'll ever see that, though -- right > > now that's not the case at least. > > I'm not denying it can't happen, but I'd like to better understand the > needs. If we decide to expose separate odd and even increments to > userspace, we also need to explain to userspace how those need to be > computed. > > My understanding of the odd/even increments is that they are meant to > make the sub-sampling configuration of the sensor independent from the > CFA pattern. Is there more to it ? There's that, and in addition it would allow e.g. skipping one in every three pixels, that could be interesting in monochrome sensors. I'm still not sure if anyone ever might want to do that. :-)
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; + } 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 + * + * 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. + * + * The camera sensor configuration applies to all streams produced by a camera + * from the same image source. + * + * 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. + * * \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 + * 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. + * + * \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