[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 14:44:59 CET 2021
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 ?
> > 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)
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