[libcamera-devel] [PATCH 1/1] libcamera: camera_sensor: Fix frame lengths calculated by sensorInfo()
Jacopo Mondi
jacopo at jmondi.org
Thu Apr 1 17:42:19 CEST 2021
On Thu, Apr 01, 2021 at 10:52:27AM +0100, David Plowman wrote:
> 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)
> ?
>
It's just a syntactic sugar maybe, but I was wondering if it would be
safest in case application takes references to the Camera::controls_..
> >
> > 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.
>
Yup, through the CameraSensorInfo I meant..
> > 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.
>
Currently no pipeline does that afict, and I thought we had a todo for
"update this control when a mechanism to do so exists" but it's gone
through reviews of the frame durations series probably...
> >
> > 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?
>
that and the concern about apps taking pointers to them. But it's a
minor indeed.
> >
> > 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.
>
I didn't notice the method was public already, and I was not a 100%
sure making it public would be worth it. Although I'm not super-happy
even about my proposal, missing a call to refreshControl() might cause nasty
errors during pipeline development, if that operation could be done
automatically that would be better. Your proposal does that for
sensorInfo(), we could do the same in CameraSensor::controls(), but
that would cause unnecessary refreshes if the state is not kept
somewhere...
All in all I would prefer calling refresh() at setFormat() time as a
public method that ph that knows what they're doing have to call by
themselves.. Also, I would add a note in the CameraSensor::sensorInfo()
and controls() documentation that states that ph that do not use
setFormat() have to refresh controls before accessing them. But I'm
not thrilled, so just take this a suggestion..
> >
> > 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.
>
Not really, I was just mumbling on how hard it would be to have the state
flag kept at the video device level :) but your idea is good too!
> 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