[libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Jacopo Mondi
jacopo at jmondi.org
Tue Jan 26 17:20:45 CET 2021
Hi Naush,
On Tue, Jan 26, 2021 at 04:08:41PM +0000, Naushir Patuck wrote:
> Hi Jacopo and Laurent,
>
>
> On Tue, 26 Jan 2021 at 14:39, Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Naush,
> >
> > On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:
> > > Hi Jacopo,
> > >
> > > On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi <jacopo at jmondi.org> wrote:
> > >
> > > > Hi Naush,
> > > >
> > > > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:
> > > > > Hi Jacopo,
> > > > >
> > > > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi <jacopo at jmondi.org>
> > 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.
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > >
> > > > > >
> > > > > > 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
> > > > > >
> > > > > > 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.
> > >
> >
> > 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
Thanks
j
> return 0;
> }
>
> Let me know if you think that is suitable or not.
>
>
>
> >
> > > Regards,
> > > Naush
> > >
> > >
> > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > >
> > > > > > > > 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.
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > - 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 :-)
> > > > > > >
> > > > > > >
> > > > > > > > - 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. 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.
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > Regards,
> > > > > > > Naush
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > > > }
> > > > > > > > >
> > > >
> > > > [snip]
> > > >
> > > > > > > > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> > > > > > > > > @@ -384,8 +393,8 @@ void IPARPi::configure(const
> > CameraSensorInfo
> > > > > > > > &sensorInfo,
> > > > > > > > > controller_.Initialise();
> > > > > > > > > controllerInit_ = true;
> > > > > > > > >
> > > > > > > > > - minFrameDuration_ = defaultMinFrameDuration;
> > > > > > > > > - maxFrameDuration_ = defaultMaxFrameDuration;
> > > > > > > > > + /* Supply initial values for frame durations.
> > */
> > > > > > > > > + applyFrameDurations(defaultMinFrameDuration,
> > > > > > > > defaultMaxFrameDuration);
> > > > > > > > >
> > > > > > > > > /* Supply initial values for gain and
> > exposure. */
> > > > > > > > > ControlList ctrls(sensorCtrls_);
> > > > > > > > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const
> > ControlList
> > > > > > > > &controls)
> > > > > > > > >
> > > > > > > > > case controls::FRAME_DURATIONS: {
> > > > > > > > > auto frameDurations =
> > > > > > ctrl.second.get<Span<const
> > > > > > > > int64_t>>();
> > > > > > > > > -
> > > > > > > > > - /* This will be applied once AGC
> > > > recalculations
> > > > > > > > occur. */
> > > > > > > > > - minFrameDuration_ = frameDurations[0] ?
> > > > > > > > frameDurations[0] : defaultMinFrameDuration;
> > > > > > > > > - maxFrameDuration_ = frameDurations[1] ?
> > > > > > > > frameDurations[1] : defaultMaxFrameDuration;
> > > > > > > > > - maxFrameDuration_ =
> > > > std::max(maxFrameDuration_,
> > > > > > > > minFrameDuration_);
> > > > > > > > > -
> > > > > > > > > - /*
> > > > > > > > > - * \todo The values returned in the
> > > > metadata
> > > > > > below
> > > > > > > > must be
> > > > > > > > > - * correctly clipped by what the sensor
> > > > mode
> > > > > > > > supports and
> > > > > > > > > - * what the AGC exposure mode or manual
> > > > shutter
> > > > > > > > speed limits
> > > > > > > > > - */
> > > > > > > > > -
> > > > > > libcameraMetadata_.set(controls::FrameDurations,
> > > > > > > > > - {
> > > > > > > > static_cast<int64_t>(minFrameDuration_),
> > > > > > > > > -
> > > > > > > > static_cast<int64_t>(maxFrameDuration_) });
> > > > > > > > > + applyFrameDurations(frameDurations[0],
> > > > > > > > frameDurations[1]);
> > > > > > > > > break;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct
> > AwbStatus
> > > > > > > > *awbStatus, ControlList &ctrls)
> > > > > > > > > static_cast<int32_t>(awbStatus->gain_b *
> > 1000));
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > +void IPARPi::applyFrameDurations(double minFrameDuration,
> > double
> > > > > > > > maxFrameDuration)
> > > > > > > > > +{
> > > > > > > > > + const double minSensorFrameDuration = 1e-3 *
> > > > (mode_.vblank_min
> > > > > > +
> > > > > > > > mode_.height) *
> > > > > > > > > +
> > mode_.line_length;
> > > > > > > > > + const double maxSensorFrameDuration = 1e-3 *
> > > > (mode_.vblank_max
> > > > > > +
> > > > > > > > mode_.height) *
> > > > > > > > > +
> > mode_.line_length;
> > > > > >
> > > > > > To transform the frame size expressed in pixels:
> > > > > > (mode_.vblank + mode_.height) * mode_.line_length
> > > > > >
> > > > > > in a duration expressed in seconds sub-units, don't you need to use
> > > > > > the pixel clock (which is available in CameraSensorInfo afair)
> > > > > > frameDuration (usec) = frame size (pixels)
> > > > > > / pixel rate (pixels/usec)
> > > > > >
> > > > >
> > > > > Yes we do. However, mode_.line_length already factors the pixel
> > rate:
> > > > > mode_.line_length = 1e9 * sensorInfo.lineLength /
> > sensorInfo.pixelRate
> > > > >
> > > > > So mode_.line_length is already converted into units of time.
> > > >
> > > > Oh sorry, I assumed CameraMode.line_lenght ==
> > CameraSensorInfo.lineLength
> > > >
> > > > I should have checked.
> > > >
> > > > Thanks
> > > > j
> > > >
> > > > >
> > > > > Regards,
> > > > > Naush
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > Thanks
> > > > > > j
> > > > > >
> > > > > > > > > + /*
> > > > > > > > > + * This will only be applied once AGC recalculations
> > occur.
> > > > > > > > > + * The values may be clamped based on the sensor mode
> > > > > > capabilities
> > > > > > > > as well.
> > > > > > > > > + */
> > > > > > > > > + minFrameDuration_ = minFrameDuration ?
> > minFrameDuration :
> > > > > > > > defaultMaxFrameDuration;
> > > > > > > > > + maxFrameDuration_ = maxFrameDuration ?
> > maxFrameDuration :
> > > > > > > > defaultMinFrameDuration;
> > > > > > > > > + minFrameDuration_ = std::clamp(minFrameDuration_,
> > > > > > > > > + minSensorFrameDuration,
> > > > > > > > maxSensorFrameDuration);
> > > > > > > > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,
> > > > > > > > > + minSensorFrameDuration,
> > > > > > > > maxSensorFrameDuration);
> > > > > > > > > +
> > > > > > > > > + /* Return the validated limits out though metadata. */
> > > > > > > > > + libcameraMetadata_.set(controls::FrameDurations,
> > > > > > > > > + {
> > > > > > static_cast<int64_t>(minFrameDuration_),
> > > > > > > > > +
> > > > > > static_cast<int64_t>(maxFrameDuration_)
> > > > > > > > });
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus,
> > > > ControlList
> > > > > > > > &ctrls)
> > > > > > > > > {
> > > > > > > > --
> > > > > > > > Regards,
> > > > > > > >
> > > > > > > > Laurent Pinchart
> > > > > > > >
> > > > > >
> > > > > >
> > > > > > > _______________________________________________
> > > > > > > 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