[libcamera-devel] [PATCH v2 1/2] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()
David Plowman
david.plowman at raspberrypi.com
Tue May 4 10:19:22 CEST 2021
Hi again - can I give this one another little prod just so that it's
not forgotten?
Thanks!
David
On Wed, 21 Apr 2021 at 09:56, David Plowman
<david.plowman at raspberrypi.com> wrote:
>
> Hi everyone
>
> Just a little nudge on this one... what are folks thinking, is there
> more stuff to look at here?
>
> Thanks
> David
>
> On Tue, 13 Apr 2021 at 22:34, Kieran Bingham
> <kieran.bingham at ideasonboard.com> wrote:
> >
> > Hi David,
> >
> > On 07/04/2021 10:33, David Plowman wrote:
> > > 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!
> >
> > I've discovered today that it's best to build doxygen from source as you
> > need a very recent version to have a quiet build.
> >
> > For me that was as straightforward as:
> >
> > git clone https://github.com/doxygen/doxygen
> > cd doxygen
> > mkdir build
> > cd build
> > cmake -DCMAKE_INSTALL_PREFIX=/usr ../ && make -j 8 all install
> >
> >
> > > 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;
> > >>> + }
> >
> > I'm curious if 'all' controls need to be refreshed, but I think that's
> > simpler that trying to filter it with a list that would then bitrot.
> >
> > So...
> >
> > Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
> >
> >
> > >>> +
> > >>> + /* 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
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> >
> > --
> > Regards
> > --
> > Kieran
More information about the libcamera-devel
mailing list