[libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Naushir Patuck
naush at raspberrypi.com
Wed Jan 27 12:38:55 CET 2021
Hi Laurent and Jacopo,
On Wed, 27 Jan 2021 at 01:34, Laurent Pinchart <
laurent.pinchart at ideasonboard.com> wrote:
> Hi Naush and Jacopo,
>
> On Tue, Jan 26, 2021 at 05:20:45PM +0100, Jacopo Mondi wrote:
> > On Tue, Jan 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:
> > > On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi wrote:
> > > > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:
> > > > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi wrote:
> > > > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:
> > > > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi wrote:
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > > > > > + */
> > > > > > > > > > > + const auto itVblank =
> sensorCtrls_.find(V4L2_CID_VBLANK);
> > > > > > > > > > > + mode_.vblank_min =
> itVblank->second.min().get<int32_t>();
> > > > > > > > > > > + mode_.vblank_max =
> itVblank->second.max().get<int32_t>();
> > > > > > > > > >
> > > > > > > > > > The trouble here is that sensorCtrls_ will not be
> updated after the
> > > > > > > > > > sensor configuration has changed. It comes from the
> > > > > > > > > > V4L2Device::controls() call in
> RPiCameraData::configureIPA(), and that
> > > > > > > > > > function returns a cached copy.
> > > > > > > > >
> > > > > > > > > I don't exactly know all of this in very much detail, so
> apologies if the
> > > > > > > > > following statements sound basic, or don't make much sense
> :-)
> > > > > > > > >
> > > > > > > > > In RPiCameraData::configureIPA(), would it be possible to
> "update" the
> > > > > > > > > cached values of V4L2Device::controls_ by
> > > > > > > > > calling V4L2Device::listControls(), or maybe a new method
> such
> > > > > > > > > as V4L2Device::updateControls(). This only needs to be
> called once
> > > > > > > > > in PipelineHandlerRPi::configure(), after the sensor mode
> has been set.
> > > > > > > > > This way, the VBLANK control will have up-to-date numbers
> for the IPA to
> > > > > > > > > use.
>
> I believe that should be technically feasible. Whether it is desired is
> a different question :-) More on that below.
>
> > > > > > > > > Alternatively, CameraSensorInfo gets populated in
> > > > > > > > > RPiCameraData::configureIPA() with a call
> > > > > > > > > to CameraSensor::sensorInfo(). Looks like this directly
> queries the device
> > > > > > > > > for controls via V4L2Device::getControls(). Perhaps we
> should re-visit
> > > > > > > > > Jacopo's suggestion and add the min/max vblank values to
> the mode?
>
> Regardless of how we retrieve the information (directly from the V4L2
> device or from the CameraSensor class), the problem stays the same.
> However, I believe CameraSensor is a better match, as it will help
> abstracting V4L2 from pipeline handlers. We don't have to switch right
> now, but I'm pretty sure we will down the line, so if it makes sense in
> the grand scheme of things to already make the transition, I'd welcome
> that.
>
> > > > > > > > > > I really dislike the fact that V4L2 provides us with a
> control whose
> > > > > > > > > > limits change when the analog crop rectangle height
> changes, while the
> > > > > > > > > > sensors usually have a fixed vertical total size limit.
> Wouldn't it be
> > > > > > > > > > better to base the code on the vertical total size limit
> in libcamera,
> > > > > > > > > > instead of vertical blanking ?
> > > > > > > > >
> > > > > > > > > By vertical total size limit, I assume you mean what
> sensors generally call
> > > > > > > > > vts or frame length? If so, then yes, it does make
> sense. In fact, you
> > > > > > > > > can see from my patch, the first thing I do is convert
> from vblank to frame
> > > > > > > > > length for all of my calculations.
>
> Yes, that's what I meant, the total of active lines and blanking lines.
> Blanking lines is a bit of a loosely defined concept here as it includes
> the embedded data lines, optical black lines, ... It's really the frame
> time expressed as a number of lines (which leads to interesting
> questions if we consider sensors that would natively express the frame
> time in a different way).
>
> > > > > > > > Let me summarize your understandings:
> > > > > > > > - You need the VBLANK limits to clamp frame durations in the
> sensor
> > > > > > > > limits
> > > > > > > > - VBLANK min and max are combined with the current mode
> height and
> > > > > > > > line length to get the min and max frame sizes, from where
> the min
> > > > > > > > and max frame duration limits are calculated
> > > > > > > > - min frame size = (mode_.height + frameIntegrationDiff) *
> mode_.line_length
> > > > > > > > - max frame size = (mode_.height + max vblank) *
> mode_.line_length
>
> As we assume everywhere that frames are made of an integral number of
> lines, I would express the minimum and maximum frame size as a number of
> lines, not a number of pixels.
>
>From the above calculations, I think Jacopo did use lines as the units, but
labelled it as size :-)
>
> > > > > > > > From the CameraSensorInfo you already know the visibile
> height and the
> > > > > > > > line_length. Wouldn't be enough to add to that structure
> > > > > > > > - maxFrameHeight = (mode_.height + max vblank)
> > > > > > > > - frameIntegrationDiff
> > > > > > > > This is sensor property and will be made available
> in the
> > > > > > > > sensor database. You can keep it in the IPA as
> long as no
> > > > > > > > sensor database is available
> > > > > > > >
> > > > > > > > To sum it up: is it enough for you to pass to the IPA the
> maximum
> > > > > > > > frame length (mode_.height + max vblank) in the
> CameraSensorClass ?
> > > > > > >
> > > > > > > This is *mostly* correct. max frame size as calculated above
> is fine.
> > > > > > > However, I do also need min frame size. Your calculation
> above assumes
> > > > > > > min_vblank = 0. This may not be the case always, so min frame
> size must be
> > > > > > > computed and presented in CameraSensorInfo.
> > > > > >
> > > > > > Correct, I had assumed min_vblank = frameIntegrationDiff, which
> might
> > > > > > not always be the case.
> > > > > >
> > > > > > Considering the requirement for 'updates' and the fact the
> > > > > > CameraSensorInfo is guaranteed to be fresh at IPA::Configure()
> time,
> > > > > > I would be happy to see minFrameSize (or Height) and
> maxFrameSize (or
> > > > > > Height) added there an computed once for all IPAs in the
> CameraSensor
> > > > > > class.
> > > > >
> > > > > Great, I can put together a change for this and submit for review
> as part
> > > > > of this series. One question, what name would you and Laurent
> prefer for
> > > > > this field - maxFrameSize or maxFrameLength (and equivalent for
> min)?
> > > > > Generally, I think sensors prefer using frame length in their
> terminology,
> > > > > but v4l2 probably prefers frame size. I am ok with either.
>
> I'd use *Length or *Height here, as we usually use *Size for
> two-dimensional sizes.
>
*Length it is!
>
> > > > This and also if we want to report only the frame heights limits
> > > > and let the IPA compute the frame size, or report the actual frame
> > > > size. It's a detail, I know, you can easily get from one to the other
> > > > and viceversa..
> > >
> > > So just to confirm before I make a submission, is something like this
> what
> > > you had in mind?
> > >
> > > diff --git a/src/libcamera/camera_sensor.cpp
> b/src/libcamera/camera_sensor.cpp
> > > index 251691aedfa3..6037f5f72897 100644
> > > --- a/src/libcamera/camera_sensor.cpp
> > > +++ b/src/libcamera/camera_sensor.cpp
> > > /**
> > > * \class CameraSensor
> > > * \brief A camera sensor based on V4L2 subdevices
> > > @@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo
> *info) const
> > >
> > > /*
> > > * Retrieve the pixel rate and the line length through V4L2
> controls.
> > > - * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK
> controls is
> > > - * mandatory.
> > > + * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and
> > > + * V4L2_CID_VBLANK controls is mandatory.
> > > */
> > > ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> > > - V4L2_CID_HBLANK });
> > > + V4L2_CID_HBLANK,
> > > + V4L2_CID_VBLANK });
> >
> > Please be aware of
> > https://patchwork.libcamera.org/patch/10937/
> >
> > You can rebase on that patch or do something similar and the first
> > that lands it wins :)
> >
> > > if (ctrls.empty()) {
> > > LOG(CameraSensor, Error)
> > > << "Failed to retrieve camera info controls";
> > > @@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo
> *info) const
> > > info->lineLength = info->outputSize.width + hblank;
> > > info->pixelRate =
> ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();
> > >
> > > + const ControlInfo vblank =
> ctrls.infoMap()->at(V4L2_CID_VBLANK);
> > > + info->minFrameLength = info->outputSize.height +
> vblank.min().get<int32_t>();
> > > + info->maxFrameLength = info->outputSize.height +
> vblank.max().get<int32_t>();
> > > +
> >
> > That's what I had in mind, yes!
> >
> > Works for me, let's see what Laurent thinks
>
> This looks good to me.
>
> > > return 0;
> > > }
> > >
> > > Let me know if you think that is suitable or not.
> > >
> > > > > > > > > > We would still have the issue of
> > > > > > > > > > retrieving this value, and I can see multiple options:
> > > > > > > > > >
> > > > > > > > > > - Compute it once at initialization from vblank limits +
> analog crop
> > > > > > > > >
> > > > > > > > > This is assuming that vblank + crop height is always going
> to be equal to
> > > > > > > > > frame length? We need to think if that is strictly true
> for all sensors,
> > > > > > > > > I'm not sure.
>
> It depends on how we define vblank. Its current purpose in libcamera is
> to control the frame rate, so from that point of view isn't it
> effectively always the frame length minus the active height ?
>
Yes, this is how I have always treated it in the calculations.
>
> > > > > > > > > > - Extend V4L2 with a new vertical total size control and
> use it (that
> > > > > > > > > > will be more work as we need to synchronize with the
> kernel changes)
> > > > > > > > >
> > > > > > > > > I expect this would have the longest lead time :-)
>
> Yes, I'm not looking forward to that :-S
>
> > > > > > > > > > - Hardcode the value in a sensor database in libcamera,
> bypassing the
> > > > > > > > > > kernel completly
> > > > > > > > >
> > > > > > > > > This is possible, but...
> > > > > > > > >
> > > > > > > > > > One question that I'm not sure I can answer by myself is
> whether we can
> > > > > > > > > > rely on the vertical total size limits being constant.
> > > > > > > > >
> > > > > > > > > ... I don't think you can assume total vertical size
> (frame length) is
> > > > > > > > > going to be the same for all modes a sensor advertises.
>
> That's my worry as well. Do you have any example of a sensor that
> wouldn't guarantee the maximum frame length being a constant ?
>
There is one example I can think of, but it's a pretty weak one. For the
imx477, we have a long exposure mode (hence very very long frame length)
applied to the full resolution mode. This long exposure mode is not
enabled for binned modes, so the frame length is advertised as much
shorter. I say weak example because I did not see much need to run long
exposures in binned mode, and thereby artificially forced the user to use
full res mode instead.
>
> > > > > > > > > In this case, the
> > > > > > > > > sensor database would have to know about every mode
> available in the kernel
> > > > > > > > > driver. This is something we moved away from with the
> Raspberry Pi
> > > > > > > > > CamHelper, so I doubt you want to re-introduce that link.
>
> Fully agreed, I don't want that either. If we decide to solve this
> through a camera sensor database, we would need to encode how the sensor
> handles the frame length constraints, not their numerical values for
> specific modes. For sensors that have a fixed maximum frame length it
> would just be a number, for other sensors it may be a few numerical
> constants for a fixed formula, or even sensor-specific code that would
> take as input the sensor mode.
>
> > > > > > > > > My feeling is that we could work something out by forcing
> a "refresh" of
> > > > > > > > > the cached controls on every configure, and I think that
> should cover our
> > > > > > > > > (or at least Raspberry Pi's in this particular instance)
> usage.
> > > > > > > > >
> > > > > > > > > Let me know your thoughts.
>
> Let's start with a refresh, through the CameraSensor class, as discussed
> above. This will move the problem away from pipeline handlers and IPA
> modules, and will allow us to switch to a different method to retrieve
> (or compute) the information later if desired.
>
Sure, I will prepare a change to the CameraSensor class and submit as part
of the next version.
Regards,
Naush
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210127/a82c2f07/attachment-0001.htm>
More information about the libcamera-devel
mailing list