[libcamera-devel] [PATCH 1/1] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()
David Plowman
david.plowman at raspberrypi.com
Thu Apr 1 11:52:27 CEST 2021
Hi Jacopo
Thanks for joining this discussion!
On Wed, 31 Mar 2021 at 13:12, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> Hi David,
>
> I admit I have not followed closely the recent discussions on this
> topic, but this seems an acceptable solution to me, however I wonder
> if it would not be worth taking it a little step forward (I know...)
>
> The problem I see here is fundamentally one: we're missing a method
> to update a (V4L2)ControlInfo. The method itself could be quite
> trivial, a ControlInfo::update(ControlValue min, ...) and one
> V4L2ControlInfo::update(v4l2_query_ext_ctrl ctrl).
So to help me understand, would
controlInfo.update(min, max, default)
and
v4l2ControlInfo.update(ctrl)
be the same as
controlInfo = ControlInfo(min, max, default)
and
v4l2ControlInfo = V4L2ControlInfo(ctrl)
?
>
> The lack of a mechanism to update a control info has this
> consequences:
> 1) V4L2 controls passed by the ph to the IPA could be stale after a
> sensor format update
Yes - though in my case the stale v4l2 controls are actually within
the CameraSensor class itself.
> 2) the Camera::controls_ might be stale after a Camera::configure()
True. Though in the Raspberry Pi pipeline handler I don't think we
really do anything about this. Should we? We do pass min and max
framerates back out to the application in the metadata.
>
> If you introduce the above suggested update() (or whatever) methods in
> (V4L2)ControlInfo, then they could be used as you're doing here below
> to handle 1)
>
> In order to handle 2) pipeline handlers that register controls would
> have to get a freshened list of V4L2Controls from the CameraSensor (or
> use a freshened CameraSensorInfo) to update the controls they register
> as Camera::controls_, performing the opportune re-calculations (in
> example the IPU3::initControls() function would need to be split in
> calculateExposure(), calculateDurations() etc which will then be
> called at configure() time to update Camera::controls_.
>
> Do you think your proposed solution could work well with
> ControlInfo::update() ?
I think so, though - as per the above - I'm not really clear on the
difference between an update method and using assignment. Do we think
an update method is a bit tidier?
>
> A few minors on the patch
>
> On Tue, Mar 30, 2021 at 05:57:43PM +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 refreshControls() method to perform
> > this function. Note that not all pipeline handlers invoke the
> > subdevice's setFormat method directly, so the new method must be made
> > publicly available.
>
> I would however call refreshControls() in CameraSensor::setFormat()
Yes, that seems a reasonable thing to do.
>
> >
> > Signed-off-by: David Plowman <david.plowman at raspberrypi.com>
> > ---
> > include/libcamera/internal/v4l2_device.h | 2 ++
> > src/libcamera/camera_sensor.cpp | 7 +++++++
> > src/libcamera/v4l2_device.cpp | 26 ++++++++++++++++++++++++
> > 3 files changed, 35 insertions(+)
> >
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index d006bf68..6efcb0db 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 refreshControls();
> > +
> > protected:
> > V4L2Device(const std::string &deviceNode);
> > ~V4L2Device();
> > diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp
> > index f7ed91d9..9f99ce3e 100644
> > --- a/src/libcamera/camera_sensor.cpp
> > +++ b/src/libcamera/camera_sensor.cpp
> > @@ -796,6 +796,13 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const
> > info->bitsPerPixel = format.bitsPerPixel();
> > info->outputSize = format.size;
> >
> > + /*
> > + * Some controls, specifically the vblanking, may have different min
> > + * or max values if the sensor format has been changed since it was
> > + * opened, so be sure to update them first.
> > + */
> > + subdev_->refreshControls();
> > +
>
> I feel a bit like it's up to the ph to be aware if controls need to be
> refreshed or not. For 'regular' ph this would be transpared, calling
> CameraSensor::setFormat() would refresh controls and it is guaranteed
> that CameraSensorInfo is always up-to-date.
Agree.
>
> For ph like yours that use CameraSensor but do not set the format
> directly on it, I think it's up to the ph to refresh controls after
> you have successfully set a format on the Unicam::Image node (this
> would require adding a refreshControls() method to CameraSensor
> though, I'm not 100% sure it is worth it :/)
Yes, we could make it our pipeline handler's business to cause the
CameraSensor to refresh its V4L2 controls, and then not do it in the
sensorInfo() method. I think CameraSensor would need some kind of
public method for us to call, I don't see that there's another way for
us to get at the stuff within the CameraSensor class.
>
> This would allow unnecessary refresh of controls, as keeping the state
> in the V4L2Device class through a flag would require to tweak into the
> subclasses' setFormat() methods, and it might get nasty, if even
> doable in clean way.
Ah, are we suggesting a public CameraSensor::refresh() method which
merely sets a flag? CameraSensor::setFormat would call it, and indeed
so would our pipeline handler.
Then there's a private method which checks the flag, does the actual
work if it's set, and clears it. CameraSensor::sensorInfo would call
this, as indeed could other parts of the CameraSensor class if they
are using the ControlInfos and feeling a bit nervous. This approach
sounds like it would work too if we prefer it.
Anyway, please let me know what you think on these points and I can
produce a v2 to look at.
Thanks and best regards
David
>
> All in all, I would be happy with this patch if it would provide a way
> to update ControlInfo first, and then use it to update the
> V4L2Controls during a refresh. Do you think it would be worth it ?
>
> Thanks
> j
>
> > /*
> > * Retrieve the pixel rate, line length and minimum/maximum frame
> > * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index decd19ef..6d396f89 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -514,6 +514,32 @@ void V4L2Device::listControls()
> > controls_ = std::move(ctrls);
> > }
> >
> > +/*
> > + * \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::refreshControls()
> > +{
>
> And I would here add a flag to the class, something like needRefersh,
> to be set to true in
> > + 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
More information about the libcamera-devel
mailing list