[libcamera-devel] [PATCH v2 3/4] libcamera: camera_sensor: Add function to apply a config

[Jacopo Mondi]

Hi Kieran

On Thu, Sep 14, 2023 at 11:33:09PM +0100, Kieran Bingham wrote:
> In $SUBJECT :
> I would probably do:
>
> -libcamera: camera_sensor: Add function to apply a config
> +libcamera: camera_sensor: Support a SensorConfiguration
>
>
> Quoting Jacopo Mondi via libcamera-devel (2023-07-31 12:31:14)
> > Add to the CameraSensor class a function to apply to the sensor
> > a full configuration.
>
> Add a class function to the CameraSensor class to apply a full
> configuration to the sensor.
>
>
>
> >
> > The configuration shall be fully populated and shall apply without
> > modifications to the sensor.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > Reviewed-by: Naushir Patuck <naush at raspberrypi.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  5 ++
> >  src/libcamera/camera_sensor.cpp            | 86 ++++++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 02c77ab037da..06791c3c6425 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -29,6 +29,7 @@ namespace libcamera {
> >  class BayerFormat;
> >  class CameraLens;
> >  class MediaEntity;
> > +class SensorConfiguration;
> >
> >  struct CameraSensorProperties;
> >
> > @@ -58,6 +59,10 @@ public:
> >                       Transform transform = Transform::Identity);
> >         int tryFormat(V4L2SubdeviceFormat *format) const;
> >
> > +       int applyConfiguration(const SensorConfiguration &config,
> > +                              Transform transform = Transform::Identity,
> > +                              V4L2SubdeviceFormat *sensorFormat = nullptr);
> > +
> >         const ControlInfoMap &controls() const;
> >         ControlList getControls(const std::vector<uint32_t> &ids);
> >         int setControls(ControlList *ctrls);
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f3a5aa37149f..96b618572a3d 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -15,6 +15,7 @@
> >  #include <math.h>
> >  #include <string.h>
> >
> > +#include <libcamera/camera.h>
> >  #include <libcamera/property_ids.h>
> >
> >  #include <libcamera/base/utils.h>
> > @@ -822,6 +823,91 @@ int CameraSensor::tryFormat(V4L2SubdeviceFormat *format) const
> >                                   V4L2Subdevice::Whence::TryFormat);
> >  }
> >
> > +/**
> > + * \brief Apply a sensor configuration to the camera sensor
> > + * \param[in] config The sensor configuration
> > + * \param[in] transform The transform to be applied on the sensor.
> > + * Defaults to Identity
> > + * \param[out] sensorFormat Optional output parameter where the format
> > + * actually applied to the sensor is returned to the caller
> > + *
> > + * Apply to the camera sensor the configuration \a config.
> > + *
> > + * \todo The configuration shall be fully populated and if any of the fields
> > + * there specified cannot be applied as it is, an error code is returned.
>
> and if any of the fields specified cannot be applied exactly, an error
> code is returned.
>
> > + *
> > + * \return 0 if \a config is applied to the camera sensor, a negative error code
> > + * otherwise
> > + */
> > +int CameraSensor::applyConfiguration(const SensorConfiguration &config,
> > +                                    Transform transform,
> > +                                    V4L2SubdeviceFormat *sensorFormat)
> > +{
> > +       if (!config) {
> > +               LOG(CameraSensor, Error) << "Invalid sensor configuration";
> > +               return -EINVAL;
> > +       }
> > +
> > +       std::vector<unsigned int> filteredCodes;
> > +       std::copy_if(mbusCodes_.begin(), mbusCodes_.end(),
> > +                    std::back_inserter(filteredCodes),
> > +                    [&config](unsigned int mbusCode) {
> > +                            BayerFormat bayer = BayerFormat::fromMbusCode(mbusCode);
> > +                            if (bayer.bitDepth == config.bitDepth)
> > +                                    return true;
> > +                            return false;
> > +                    });
> > +       if (filteredCodes.empty()) {
> > +               LOG(CameraSensor, Error)
> > +                       << "Cannot find any format with bit depth "
> > +                       << config.bitDepth;
> > +               return -EINVAL;
> > +       }
> > +
> > +       /*
> > +        * Compute the sensor's data frame size by applying the cropping
> > +        * rectangle, subsampling and output crop to the sensor's pixel array
> > +        * size.
> > +        *
> > +        * \todo The actual size computation is for now ignored and only the
> > +        * output size is considered. This implies that resolutions obtained
> > +        * with two different cropping/subsampling will look identical and
> > +        * only the first found one will be considered.
> > +        */
> > +       V4L2SubdeviceFormat subdevFormat = {};
> > +       for (unsigned int code : filteredCodes) {
> > +               for (const Size &size : sizes(code)) {
> > +                       if (size.width != config.outputSize.width ||
> > +                           size.height != config.outputSize.height)
> > +                               continue;
> > +
> > +                       subdevFormat.mbus_code = code;
> > +                       subdevFormat.size = size;
> > +                       break;
> > +               }
> > +       }
> > +       if (!subdevFormat.mbus_code) {
> > +               LOG(CameraSensor, Error) << "Invalid output size in sensor configuration";
> > +               return -EINVAL;
> > +       }
> > +
> > +       int ret = setFormat(&subdevFormat, transform);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * Return to the caller the format actually applied to the sensor.
> > +        * This is relevant if transform has changed the bayer pattern order.
> > +        */
> > +       if (sensorFormat)
> > +               *sensorFormat = subdevFormat;
> > +
> > +       /* \todo Handle AnalogCrop. Most sensors do not support set_selection */
> > +       /* \todo Handle scaling in the digital domain. */
>
> Should these return an error if they are set as they are unhandled for
> now? Or are we happy to just silently ignore here for now?

An error might be too harsh to be honest. Actually, right now most of
the sensor configuration fields are ignored, except for bitdepth and
output size. Should we maybe WARN ?

>
> Except the known todos, nothing blocking here though so
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

Thanks
  j

>
>
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * \brief Retrieve the supported V4L2 controls and their information
> >   *
> > --
> > 2.40.1
> >