[PATCH/RFC 16/32] libcamera: camera_sensor: Reorder functions
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 4 23:48:08 CET 2024
On Mon, Mar 04, 2024 at 06:37:15PM +0100, Jacopo Mondi wrote:
> Hi Laurent
>
> On Fri, Mar 01, 2024 at 11:21:05PM +0200, Laurent Pinchart wrote:
> > The CameraSensor class has grown a lot since its creation, with many
> > functions added for different types of purposes. They are not grouped by
> > categories in the class definition, generating confusion when reading
> > the header file. Improve readability by sorting functions by category:
> >
> > - Getters for static data (model, entity, focus lens, ...)
> > - Format and sensor configuration accessors
> > - Properties and controls (including test pattern mode)
> >
> > Update the .cpp file accordingly.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > include/libcamera/internal/camera_sensor.h | 29 +-
> > src/libcamera/sensor/camera_sensor.cpp | 328 ++++++++++-----------
> > 2 files changed, 179 insertions(+), 178 deletions(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index b2f077b9cc75..750d6d729cac 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -46,15 +46,15 @@ public:
> >
> > const std::string &model() const { return model_; }
> > const std::string &id() const { return id_; }
> > +
> > const MediaEntity *entity() const { return entity_; }
> > + V4L2Subdevice *device() { return subdev_.get(); }
> > +
> > + CameraLens *focusLens() { return focusLens_.get(); }
> > +
>
> Should you move
> const ControlList &properties() const { return properties_; }
> const ControlInfoMap &controls() const;
>
> here ?
>
> (I understand if you want to keep them grouped though)
I specifically decided to put the properties just before the controls
:-)
> This can be fast-tracked too!
>
> Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> > const std::vector<unsigned int> &mbusCodes() const { return mbusCodes_; }
> > std::vector<Size> sizes(unsigned int mbusCode) const;
> > Size resolution() const;
> > - const std::vector<controls::draft::TestPatternModeEnum> &testPatternModes() const
> > - {
> > - return testPatternModes_;
> > - }
> > - int setTestPatternMode(controls::draft::TestPatternModeEnum mode);
> >
> > V4L2SubdeviceFormat getFormat(const std::vector<unsigned int> &mbusCodes,
> > const Size &size) const;
> > @@ -66,18 +66,19 @@ public:
> > Transform transform = Transform::Identity,
> > V4L2SubdeviceFormat *sensorFormat = nullptr);
> >
> > + const ControlList &properties() const { return properties_; }
> > + int sensorInfo(IPACameraSensorInfo *info) const;
> > + Transform computeTransform(Orientation *orientation) const;
> > +
> > const ControlInfoMap &controls() const;
> > ControlList getControls(const std::vector<uint32_t> &ids);
> > int setControls(ControlList *ctrls);
> >
> > - V4L2Subdevice *device() { return subdev_.get(); }
> > -
> > - const ControlList &properties() const { return properties_; }
> > - int sensorInfo(IPACameraSensorInfo *info) const;
> > -
> > - CameraLens *focusLens() { return focusLens_.get(); }
> > -
> > - Transform computeTransform(Orientation *orientation) const;
> > + const std::vector<controls::draft::TestPatternModeEnum> &testPatternModes() const
> > + {
> > + return testPatternModes_;
> > + }
> > + int setTestPatternMode(controls::draft::TestPatternModeEnum mode);
> >
> > protected:
> > std::string logPrefix() const override;
> > @@ -91,8 +92,8 @@ private:
> > void initStaticProperties();
> > void initTestPatternModes();
> > int initProperties();
> > - int applyTestPatternMode(controls::draft::TestPatternModeEnum mode);
> > int discoverAncillaryDevices();
> > + int applyTestPatternMode(controls::draft::TestPatternModeEnum mode);
> >
> > const MediaEntity *entity_;
> > std::unique_ptr<V4L2Subdevice> subdev_;
> > diff --git a/src/libcamera/sensor/camera_sensor.cpp b/src/libcamera/sensor/camera_sensor.cpp
> > index 545f89d036df..86ad9a85371c 100644
> > --- a/src/libcamera/sensor/camera_sensor.cpp
> > +++ b/src/libcamera/sensor/camera_sensor.cpp
> > @@ -215,6 +215,30 @@ int CameraSensor::init()
> > return applyTestPatternMode(controls::draft::TestPatternModeEnum::TestPatternModeOff);
> > }
> >
> > +int CameraSensor::generateId()
> > +{
> > + const std::string devPath = subdev_->devicePath();
> > +
> > + /* Try to get ID from firmware description. */
> > + id_ = sysfs::firmwareNodePath(devPath);
> > + if (!id_.empty())
> > + return 0;
> > +
> > + /*
> > + * Virtual sensors not described in firmware
> > + *
> > + * Verify it's a platform device and construct ID from the device path
> > + * and model of sensor.
> > + */
> > + if (devPath.find("/sys/devices/platform/", 0) == 0) {
> > + id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
> > + return 0;
> > + }
> > +
> > + LOG(CameraSensor, Error) << "Can't generate sensor ID";
> > + return -EINVAL;
> > +}
> > +
> > int CameraSensor::validateSensorDriver()
> > {
> > int err = 0;
> > @@ -580,6 +604,21 @@ int CameraSensor::discoverAncillaryDevices()
> > * \return The sensor media entity
> > */
> >
> > +/**
> > + * \fn CameraSensor::device()
> > + * \brief Retrieve the camera sensor device
> > + * \todo Remove this function by integrating DelayedControl with CameraSensor
> > + * \return The camera sensor device
> > + */
> > +
> > +/**
> > + * \fn CameraSensor::focusLens()
> > + * \brief Retrieve the focus lens controller
> > + *
> > + * \return The focus lens controller. nullptr if no focus lens controller is
> > + * connected to the sensor
> > + */
> > +
> > /**
> > * \fn CameraSensor::mbusCodes()
> > * \brief Retrieve the media bus codes supported by the camera sensor
> > @@ -632,64 +671,6 @@ Size CameraSensor::resolution() const
> > return std::min(sizes_.back(), activeArea_.size());
> > }
> >
> > -/**
> > - * \fn CameraSensor::testPatternModes()
> > - * \brief Retrieve all the supported test pattern modes of the camera sensor
> > - * The test pattern mode values correspond to the controls::TestPattern control.
> > - *
> > - * \return The list of test pattern modes
> > - */
> > -
> > -/**
> > - * \brief Set the test pattern mode for the camera sensor
> > - * \param[in] mode The test pattern mode
> > - *
> > - * The new \a mode is applied to the sensor if it differs from the active test
> > - * pattern mode. Otherwise, this function is a no-op. Setting the same test
> > - * pattern mode for every frame thus incurs no performance penalty.
> > - */
> > -int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)
> > -{
> > - if (testPatternMode_ == mode)
> > - return 0;
> > -
> > - if (testPatternModes_.empty()) {
> > - LOG(CameraSensor, Error)
> > - << "Camera sensor does not support test pattern modes.";
> > - return -EINVAL;
> > - }
> > -
> > - return applyTestPatternMode(mode);
> > -}
> > -
> > -int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)
> > -{
> > - if (testPatternModes_.empty())
> > - return 0;
> > -
> > - auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > - mode);
> > - if (it == testPatternModes_.end()) {
> > - LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > - << mode;
> > - return -EINVAL;
> > - }
> > -
> > - LOG(CameraSensor, Debug) << "Apply test pattern mode " << mode;
> > -
> > - int32_t index = staticProps_->testPatternModes.at(mode);
> > - ControlList ctrls{ controls() };
> > - ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > -
> > - int ret = setControls(&ctrls);
> > - if (ret)
> > - return ret;
> > -
> > - testPatternMode_ = mode;
> > -
> > - return 0;
> > -}
> > -
> > /**
> > * \brief Retrieve the best sensor format for a desired output
> > * \param[in] mbusCodes The list of acceptable media bus codes
> > @@ -919,80 +900,6 @@ int CameraSensor::applyConfiguration(const SensorConfiguration &config,
> > return 0;
> > }
> >
> > -/**
> > - * \brief Retrieve the supported V4L2 controls and their information
> > - *
> > - * Control information is updated automatically to reflect the current sensor
> > - * configuration when the setFormat() function is called, without invalidating
> > - * any iterator on the ControlInfoMap.
> > - *
> > - * \return A map of the V4L2 controls supported by the sensor
> > - */
> > -const ControlInfoMap &CameraSensor::controls() const
> > -{
> > - return subdev_->controls();
> > -}
> > -
> > -/**
> > - * \brief Read V4L2 controls from the sensor
> > - * \param[in] ids The list of controls to read, specified by their ID
> > - *
> > - * This function reads the value of all controls contained in \a ids, and
> > - * returns their values as a ControlList. The control identifiers are defined by
> > - * the V4L2 specification (V4L2_CID_*).
> > - *
> > - * If any control in \a ids is not supported by the device, is disabled (i.e.
> > - * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> > - * during validation of the requested controls, no control is read and this
> > - * function returns an empty control list.
> > - *
> > - * \sa V4L2Device::getControls()
> > - *
> > - * \return The control values in a ControlList on success, or an empty list on
> > - * error
> > - */
> > -ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > -{
> > - return subdev_->getControls(ids);
> > -}
> > -
> > -/**
> > - * \brief Write V4L2 controls to the sensor
> > - * \param[in] ctrls The list of controls to write
> > - *
> > - * This function writes the value of all controls contained in \a ctrls, and
> > - * stores the values actually applied to the device in the corresponding \a
> > - * ctrls entry. The control identifiers are defined by the V4L2 specification
> > - * (V4L2_CID_*).
> > - *
> > - * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > - * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> > - * error occurs during validation of the requested controls, no control is
> > - * written and this function returns -EINVAL.
> > - *
> > - * If an error occurs while writing the controls, the index of the first
> > - * control that couldn't be written is returned. All controls below that index
> > - * are written and their values are updated in \a ctrls, while all other
> > - * controls are not written and their values are not changed.
> > - *
> > - * \sa V4L2Device::setControls()
> > - *
> > - * \return 0 on success or an error code otherwise
> > - * \retval -EINVAL One of the control is not supported or not accessible
> > - * \retval i The index of the control that failed
> > - */
> > -int CameraSensor::setControls(ControlList *ctrls)
> > -{
> > - return subdev_->setControls(ctrls);
> > -}
> > -
> > -/**
> > - * \fn CameraSensor::device()
> > - * \brief Retrieve the camera sensor device
> > - * \todo Remove this function by integrating DelayedControl with CameraSensor
> > - * \return The camera sensor device
> > - */
> > -
> > /**
> > * \fn CameraSensor::properties()
> > * \brief Retrieve the camera sensor properties
> > @@ -1088,14 +995,6 @@ int CameraSensor::sensorInfo(IPACameraSensorInfo *info) const
> > return 0;
> > }
> >
> > -/**
> > - * \fn CameraSensor::focusLens()
> > - * \brief Retrieve the focus lens controller
> > - *
> > - * \return The focus lens controller. nullptr if no focus lens controller is
> > - * connected to the sensor
> > - */
> > -
> > /**
> > * \brief Compute the Transform that gives the requested \a orientation
> > * \param[inout] orientation The desired image orientation
> > @@ -1155,33 +1054,134 @@ Transform CameraSensor::computeTransform(Orientation *orientation) const
> > return transform;
> > }
> >
> > +/**
> > + * \brief Retrieve the supported V4L2 controls and their information
> > + *
> > + * Control information is updated automatically to reflect the current sensor
> > + * configuration when the setFormat() function is called, without invalidating
> > + * any iterator on the ControlInfoMap.
> > + *
> > + * \return A map of the V4L2 controls supported by the sensor
> > + */
> > +const ControlInfoMap &CameraSensor::controls() const
> > +{
> > + return subdev_->controls();
> > +}
> > +
> > +/**
> > + * \brief Read V4L2 controls from the sensor
> > + * \param[in] ids The list of controls to read, specified by their ID
> > + *
> > + * This function reads the value of all controls contained in \a ids, and
> > + * returns their values as a ControlList. The control identifiers are defined by
> > + * the V4L2 specification (V4L2_CID_*).
> > + *
> > + * If any control in \a ids is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), or if any other error occurs
> > + * during validation of the requested controls, no control is read and this
> > + * function returns an empty control list.
> > + *
> > + * \sa V4L2Device::getControls()
> > + *
> > + * \return The control values in a ControlList on success, or an empty list on
> > + * error
> > + */
> > +ControlList CameraSensor::getControls(const std::vector<uint32_t> &ids)
> > +{
> > + return subdev_->getControls(ids);
> > +}
> > +
> > +/**
> > + * \brief Write V4L2 controls to the sensor
> > + * \param[in] ctrls The list of controls to write
> > + *
> > + * This function writes the value of all controls contained in \a ctrls, and
> > + * stores the values actually applied to the device in the corresponding \a
> > + * ctrls entry. The control identifiers are defined by the V4L2 specification
> > + * (V4L2_CID_*).
> > + *
> > + * If any control in \a ctrls is not supported by the device, is disabled (i.e.
> > + * has the V4L2_CTRL_FLAG_DISABLED flag set), is read-only, or if any other
> > + * error occurs during validation of the requested controls, no control is
> > + * written and this function returns -EINVAL.
> > + *
> > + * If an error occurs while writing the controls, the index of the first
> > + * control that couldn't be written is returned. All controls below that index
> > + * are written and their values are updated in \a ctrls, while all other
> > + * controls are not written and their values are not changed.
> > + *
> > + * \sa V4L2Device::setControls()
> > + *
> > + * \return 0 on success or an error code otherwise
> > + * \retval -EINVAL One of the control is not supported or not accessible
> > + * \retval i The index of the control that failed
> > + */
> > +int CameraSensor::setControls(ControlList *ctrls)
> > +{
> > + return subdev_->setControls(ctrls);
> > +}
> > +
> > +/**
> > + * \fn CameraSensor::testPatternModes()
> > + * \brief Retrieve all the supported test pattern modes of the camera sensor
> > + * The test pattern mode values correspond to the controls::TestPattern control.
> > + *
> > + * \return The list of test pattern modes
> > + */
> > +
> > +/**
> > + * \brief Set the test pattern mode for the camera sensor
> > + * \param[in] mode The test pattern mode
> > + *
> > + * The new \a mode is applied to the sensor if it differs from the active test
> > + * pattern mode. Otherwise, this function is a no-op. Setting the same test
> > + * pattern mode for every frame thus incurs no performance penalty.
> > + */
> > +int CameraSensor::setTestPatternMode(controls::draft::TestPatternModeEnum mode)
> > +{
> > + if (testPatternMode_ == mode)
> > + return 0;
> > +
> > + if (testPatternModes_.empty()) {
> > + LOG(CameraSensor, Error)
> > + << "Camera sensor does not support test pattern modes.";
> > + return -EINVAL;
> > + }
> > +
> > + return applyTestPatternMode(mode);
> > +}
> > +
> > +int CameraSensor::applyTestPatternMode(controls::draft::TestPatternModeEnum mode)
> > +{
> > + if (testPatternModes_.empty())
> > + return 0;
> > +
> > + auto it = std::find(testPatternModes_.begin(), testPatternModes_.end(),
> > + mode);
> > + if (it == testPatternModes_.end()) {
> > + LOG(CameraSensor, Error) << "Unsupported test pattern mode "
> > + << mode;
> > + return -EINVAL;
> > + }
> > +
> > + LOG(CameraSensor, Debug) << "Apply test pattern mode " << mode;
> > +
> > + int32_t index = staticProps_->testPatternModes.at(mode);
> > + ControlList ctrls{ controls() };
> > + ctrls.set(V4L2_CID_TEST_PATTERN, index);
> > +
> > + int ret = setControls(&ctrls);
> > + if (ret)
> > + return ret;
> > +
> > + testPatternMode_ = mode;
> > +
> > + return 0;
> > +}
> > +
> > std::string CameraSensor::logPrefix() const
> > {
> > return "'" + entity_->name() + "'";
> > }
> >
> > -int CameraSensor::generateId()
> > -{
> > - const std::string devPath = subdev_->devicePath();
> > -
> > - /* Try to get ID from firmware description. */
> > - id_ = sysfs::firmwareNodePath(devPath);
> > - if (!id_.empty())
> > - return 0;
> > -
> > - /*
> > - * Virtual sensors not described in firmware
> > - *
> > - * Verify it's a platform device and construct ID from the device path
> > - * and model of sensor.
> > - */
> > - if (devPath.find("/sys/devices/platform/", 0) == 0) {
> > - id_ = devPath.substr(strlen("/sys/devices/")) + " " + model();
> > - return 0;
> > - }
> > -
> > - LOG(CameraSensor, Error) << "Can't generate sensor ID";
> > - return -EINVAL;
> > -}
> > -
> > } /* namespace libcamera */
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list