[libcamera-devel] [PATCH v2 2/4] ipa: raspberrypi: Limit the calculated vblank based on the sensor mode
Naushir Patuck
naush at raspberrypi.com
Tue Jan 26 15:12:50 CET 2021
Hi Jacopo,
On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi <jacopo at jmondi.org> wrote:
> Hi Naush, Laurent,
>
> one question on the patch and one more input on the below
> discussion on where to retrieve vblank limits from
>
> On Tue, Jan 26, 2021 at 12:51:12PM +0000, Naushir Patuck wrote:
> > Hi Laurent,
> >
> > Thank you for your review feedback.
> >
> > On Tue, 26 Jan 2021 at 10:01, Laurent Pinchart <
> > laurent.pinchart at ideasonboard.com> wrote:
> >
> > > Hi Naush,
> > >
> > > Thank you for the patch.
> > >
> > > On Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:
> > > > The existing framerate/vblank calculations did not account for the
> > > > sensor mode limits. This commit extracts vblank limits from the
> sensor
> > > > v4l2 controls, and stores it in the camera modes structure.
> > > >
> > > > Exposure and vblank value calculations now use values clamped to the
> > > > sensor mode limits. The libcamera::controls::FrameDurations metadata
> > > > return values now reflect the minimum and maximum after clamping.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > Reviewed-by: David Plowman <david.plowman at raspberrypi.com>
> > > > ---
> > > > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---
> > > > src/ipa/raspberrypi/cam_helper.hpp | 5 +-
> > > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--
> > > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-
> > > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-
> > > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +
> > > > src/ipa/raspberrypi/raspberrypi.cpp | 51
> ++++++++++++++------
> > > > 7 files changed, 49 insertions(+), 39 deletions(-)
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp
> > > b/src/ipa/raspberrypi/cam_helper.cpp
> > > > index b7b8faf09c69..3e17255737b2 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.cpp
> > > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const
> > > &cam_name)
> > > > return nullptr;
> > > > }
> > > >
> > > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,
> > > > - unsigned int frameIntegrationDiff)
> > > > - : parser_(parser), initialized_(false),
> > > maxFrameLength_(maxFrameLength),
> > > > +CamHelper::CamHelper(MdParser *parser, unsigned int
> > > frameIntegrationDiff)
> > > > + : parser_(parser), initialized_(false),
> > > > frameIntegrationDiff_(frameIntegrationDiff)
> > > > {
> > > > }
> > > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double
> &exposure,
> > > double minFrameDuration,
> > > > assert(initialized_);
> > > >
> > > > /*
> > > > - * Clamp frame length by the frame duration range and the
> maximum
> > > allowable
> > > > - * value in the sensor, given by maxFrameLength_.
> > > > + * minFrameDuration and maxFrameDuration will have been
> clamped to
> > > the
> > >
> > > Maybe "minFrameDuration and maxFrameDuration are clamped by the caller
> > > based on the limits for the active sensor mode" to make clear who is
> > > responsible for clamping those values ?
> > >
> >
> > That wording sounds good to me.
> >
> >
> > >
> > > > + * maximum allowable values in the sensor for this mode.
> > > > */
> > > > - frameLengthMin = std::clamp<uint32_t>(1e3 * minFrameDuration /
> > > mode_.line_length,
> > > > - mode_.height,
> > > maxFrameLength_);
> > > > - frameLengthMax = std::clamp<uint32_t>(1e3 * maxFrameDuration /
> > > mode_.line_length,
> > > > - mode_.height,
> > > maxFrameLength_);
> > > > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;
> > > > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;
> > > > +
> > > > /*
> > > > * Limit the exposure to the maximum frame duration requested,
> and
> > > > * re-calculate if it has been clipped.
> > > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp
> > > b/src/ipa/raspberrypi/cam_helper.hpp
> > > > index b1739a57e342..14d70112cb62 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper.hpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper.hpp
> > > > @@ -62,8 +62,7 @@ class CamHelper
> > > > {
> > > > public:
> > > > static CamHelper *Create(std::string const &cam_name);
> > > > - CamHelper(MdParser *parser, unsigned int maxFrameLength,
> > > > - unsigned int frameIntegrationDiff);
> > > > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);
> > > > virtual ~CamHelper();
> > > > void SetCameraMode(const CameraMode &mode);
> > > > MdParser &Parser() const { return *parser_; }
> > > > @@ -86,8 +85,6 @@ protected:
> > > >
> > > > private:
> > > > bool initialized_;
> > > > - /* Largest possible frame length, in units of lines. */
> > > > - unsigned int maxFrameLength_;
> > > > /*
> > > > * Smallest difference between the frame length and integration
> > > time,
> > > > * in units of lines.
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > index 8688ec091c44..95b8e698fe3b 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp
> > > > @@ -56,15 +56,13 @@ private:
> > > > * in units of lines.
> > > > */
> > > > static constexpr int frameIntegrationDiff = 4;
> > > > - /* Largest possible frame length, in units of lines. */
> > > > - static constexpr int maxFrameLength = 0xffff;
> > > > };
> > > >
> > > > CamHelperImx219::CamHelperImx219()
> > > > #if ENABLE_EMBEDDED_DATA
> > > > - : CamHelper(new MdParserImx219(), maxFrameLength,
> > > frameIntegrationDiff)
> > > > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)
> > > > #else
> > > > - : CamHelper(new MdParserRPi(), maxFrameLength,
> > > frameIntegrationDiff)
> > > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> > > > #endif
> > > > {
> > > > }
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > index 5396131003f1..eaa7be16d20e 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp
> > > > @@ -45,12 +45,10 @@ private:
> > > > * in units of lines.
> > > > */
> > > > static constexpr int frameIntegrationDiff = 22;
> > > > - /* Largest possible frame length, in units of lines. */
> > > > - static constexpr int maxFrameLength = 0xffdc;
> > > > };
> > > >
> > > > CamHelperImx477::CamHelperImx477()
> > > > - : CamHelper(new MdParserImx477(), maxFrameLength,
> > > frameIntegrationDiff)
> > > > + : CamHelper(new MdParserImx477(), frameIntegrationDiff)
> > > > {
> > > > }
> > > >
> > > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > > index 2bd8a754f133..a7f417324048 100644
> > > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp
> > > > @@ -30,8 +30,6 @@ private:
> > > > * in units of lines.
> > > > */
> > > > static constexpr int frameIntegrationDiff = 4;
> > > > - /* Largest possible frame length, in units of lines. */
> > > > - static constexpr int maxFrameLength = 0xffff;
> > > > };
> > > >
> > > > /*
> > > > @@ -40,7 +38,7 @@ private:
> > > > */
> > > >
> > > > CamHelperOv5647::CamHelperOv5647()
> > > > - : CamHelper(new MdParserRPi(), maxFrameLength,
> > > frameIntegrationDiff)
> > > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)
> > > > {
> > > > }
> > > >
> > > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h
> > > b/src/ipa/raspberrypi/controller/camera_mode.h
> > > > index 920f11beb0b0..3baeadaca076 100644
> > > > --- a/src/ipa/raspberrypi/controller/camera_mode.h
> > > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h
> > > > @@ -37,6 +37,8 @@ struct CameraMode {
> > > > double line_length;
> > > > // any camera transform *not* reflected already in the camera
> > > tuning
> > > > libcamera::Transform transform;
> > > > + // minimum and maximum vblanking limits for this mode
> > > > + uint32_t vblank_min, vblank_max;
> > > > };
> > > >
> > > > #ifdef __cplusplus
> > > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp
> > > b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > index 5ccc7a6551f5..30ba9aef9d97 100644
> > > > --- a/src/ipa/raspberrypi/raspberrypi.cpp
> > > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> > > > @@ -102,6 +102,7 @@ private:
> > > > void reportMetadata();
> > > > bool parseEmbeddedData(unsigned int bufferId, struct
> DeviceStatus
> > > &deviceStatus);
> > > > void processStats(unsigned int bufferId);
> > > > + void applyFrameDurations(double minFrameDuration, double
> > > maxFrameDuration);
> > > > void applyAGC(const struct AgcStatus *agcStatus, ControlList
> > > &ctrls);
> > > > void applyAWB(const struct AwbStatus *awbStatus, ControlList
> > > &ctrls);
> > > > void applyDG(const struct AgcStatus *dgStatus, ControlList
> &ctrls);
> > > > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo
> > > &sensorInfo)
> > > > * the line length in pixels and the pixel rate.
> > > > */
> > > > mode_.line_length = 1e9 * sensorInfo.lineLength /
> > > sensorInfo.pixelRate;
> > > > +
> > > > + /*
> > > > + * Set the vertical blanking (frame duration) limits for the
> mode
> > > to
> > > > + * ensure exposure and framerate calculations are clipped
> > > appropriately.
> > > > + */
> > > > + 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.
>
>
> > > 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
> >
> >
> >
> > > > }
> > > >
> > > > 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.
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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20210126/0ebd85b3/attachment-0001.htm>
More information about the libcamera-devel
mailing list