[libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()

[David Plowman]

Hi Jacopo

On Tue, 6 Apr 2021 at 13:09, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> On Tue, Apr 06, 2021 at 11:40:49AM +0100, David Plowman wrote:
> > The minimum and maximum vblanking can change when a new format is
> > applied to the sensor subdevice, so be sure to retrieve up-to-date
> > values.
> >
> > The V4L2Device acquires the new updateControlInfos() method to perform
> > this function, and which the CameraSensor calls automatically if its
> > setFormat method is used to update the sensor.
> >
> > However, not all pipeline handlers invoke the setFormat method
> > directly, so the new method must be made publicly available for
> > pipeline handlers to call if they need to.
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h |  2 ++
> >  include/libcamera/internal/v4l2_device.h   |  2 ++
> >  src/libcamera/camera_sensor.cpp            | 31 +++++++++++++++++++++-
> >  src/libcamera/v4l2_device.cpp              | 26 ++++++++++++++++++
> >  4 files changed, 60 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 3e98f71b..c94744f0 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -68,6 +68,8 @@ public:
> >       const ControlList &properties() const { return properties_; }
> >       int sensorInfo(CameraSensorInfo *info) const;
> >
> > +     void updateControlInfos();
> > +
> >  protected:
> >       std::string logPrefix() const override;
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index d006bf68..274cbe65 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -41,6 +41,8 @@ public:
> >       int setFrameStartEnabled(bool enable);
> >       Signal<uint32_t> frameStart;
> >
> > +     void updateControlInfos();
> > +
> >  protected:
> >       V4L2Device(const std::string &deviceNode);
> >       ~V4L2Device();
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f7ed91d9..5dbe47a7 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -656,15 +656,30 @@ V4L2SubdeviceFormat CameraSensor::getFormat(const std::vector<unsigned int> &mbu
> >  /**
> >   * \brief Set the sensor output format
> >   * \param[in] format The desired sensor output format
> > + *
> > + * The sensor output format is set. The ranges of any controls associated
> > + * with the sensor are also updated.
> > + *
> >   * \return 0 on success or a negative error code otherwise
> >   */
> >  int CameraSensor::setFormat(V4L2SubdeviceFormat *format)
> >  {
> > -     return subdev_->setFormat(pad_, format);
> > +     int ret = subdev_->setFormat(pad_, format);
> > +
> > +     if (ret == 0)
> > +             updateControlInfos();
> > +
> > +     return ret;
> >  }
> >
> >  /**
> >   * \brief Retrieve the supported V4L2 controls and their information
> > + *
> > + * Pipeline handlers that do not change the sensor format using the
> > + * CameraSensor::setFormat method may need to call
> > + * CameraSensor::updateControlInfos beforehand, to ensure all the control
> > + * ranges are up to date.
> > + *
> >   * \return A map of the V4L2 controls supported by the sensor
> >   */
> >  const ControlInfoMap &CameraSensor::controls() const
> > @@ -750,6 +765,11 @@ int CameraSensor::setControls(ControlList *ctrls)
> >   * Sensor information is only available for raw sensors. When called for a YUV
> >   * sensor, this function returns -EINVAL.
> >   *
> > + * Pipeline handlers that do not change the sensor format using the
> > + * CameraSensor::setFormat method may need to call
> > + * CameraSensor::updateControlInfos beforehand, to ensure all the control
> > + * ranges are up to date.
> > + *
> >   * \return 0 on success, a negative error code otherwise
> >   */
> >  int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > @@ -821,6 +841,15 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> >       return 0;
> >  }
> >
> > +/**
> > + * \fn void CameraSensor::updateControlInfos()
> > + * \brief Update the sensor's ControlInfos in case they have changed
> > + */
> > +void CameraSensor::updateControlInfos()
> > +{
> > +     subdev_->updateControlInfos();
> > +}
> > +
> >  std::string CameraSensor::logPrefix() const
> >  {
> >       return "'" + entity_->name() + "'";
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index decd19ef..9678962c 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -514,6 +514,32 @@ void V4L2Device::listControls()
> >       controls_ = std::move(ctrls);
> >  }
> >
> > +/*
>
> This is now a public method and needs /**
> Doesn't doxygen give you a warning here ?

Ah yes, the documentation seems to have stopped building some time
ago... there appear to be a couple more dependencies I have to install
for it to happen now. No problem!

Thanks
David

>
> The patch looks good otherwise!
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
>
> Thanks
>    j
>
> > + * \brief Update the control information that was previously stored by
> > + * listControls(). Some parts of this information, such as min and max
> > + * values of some controls, are liable to change when a new format is set.
> > + */
> > +void V4L2Device::updateControlInfos()
> > +{
> > +     for (auto &controlId : controlIds_) {
> > +             unsigned int id = controlId->id();
> > +
> > +             struct v4l2_query_ext_ctrl ctrl = {};
> > +             ctrl.id = id;
> > +
> > +             if (ioctl(VIDIOC_QUERY_EXT_CTRL, &ctrl)) {
> > +                     LOG(V4L2, Debug)
> > +                             << "Could not refresh control "
> > +                             << utils::hex(ctrl.id);
> > +                     continue;
> > +             }
> > +
> > +             /* Assume these are present - listControls() made them. */
> > +             controlInfo_[id] = ctrl;
> > +             controls_.at(controlId.get()) = V4L2ControlInfo(ctrl);
> > +     }
> > +}
> > +
> >  /*
> >   * \brief Update the value of the first \a count V4L2 controls in \a ctrls using
> >   * values in \a v4l2Ctrls
> > --
> > 2.20.1
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel