<div dir="ltr"><div dir="ltr">Hi Jacopo,</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org">jacopo@jmondi.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Naush, Laurent,<br>
<br>
one question on the patch and one more input on the below<br>
discussion on where to retrieve vblank limits from<br>
<br>
On Tue, Jan 26, 2021 at 12:51:12PM +0000, Naushir Patuck wrote:<br>
> Hi Laurent,<br>
><br>
> Thank you for your review feedback.<br>
><br>
> On Tue, 26 Jan 2021 at 10:01, Laurent Pinchart <<br>
> <a href="mailto:laurent.pinchart@ideasonboard.com" target="_blank">laurent.pinchart@ideasonboard.com</a>> wrote:<br>
><br>
> > Hi Naush,<br>
> ><br>
> > Thank you for the patch.<br>
> ><br>
> > On Sun, Jan 24, 2021 at 02:05:04PM +0000, Naushir Patuck wrote:<br>
> > > The existing framerate/vblank calculations did not account for the<br>
> > > sensor mode limits. This commit extracts vblank limits from the sensor<br>
> > > v4l2 controls, and stores it in the camera modes structure.<br>
> > ><br>
> > > Exposure and vblank value calculations now use values clamped to the<br>
> > > sensor mode limits. The libcamera::controls::FrameDurations metadata<br>
> > > return values now reflect the minimum and maximum after clamping.<br>
> > ><br>
> > > Signed-off-by: Naushir Patuck <<a href="mailto:naush@raspberrypi.com" target="_blank">naush@raspberrypi.com</a>><br>
> > > Reviewed-by: David Plowman <<a href="mailto:david.plowman@raspberrypi.com" target="_blank">david.plowman@raspberrypi.com</a>><br>
> > > ---<br>
> > > src/ipa/raspberrypi/cam_helper.cpp | 16 +++---<br>
> > > src/ipa/raspberrypi/cam_helper.hpp | 5 +-<br>
> > > src/ipa/raspberrypi/cam_helper_imx219.cpp | 6 +--<br>
> > > src/ipa/raspberrypi/cam_helper_imx477.cpp | 4 +-<br>
> > > src/ipa/raspberrypi/cam_helper_ov5647.cpp | 4 +-<br>
> > > src/ipa/raspberrypi/controller/camera_mode.h | 2 +<br>
> > > src/ipa/raspberrypi/raspberrypi.cpp | 51 ++++++++++++++------<br>
> > > 7 files changed, 49 insertions(+), 39 deletions(-)<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > index b7b8faf09c69..3e17255737b2 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.cpp<br>
> > > @@ -34,9 +34,8 @@ CamHelper *CamHelper::Create(std::string const<br>
> > &cam_name)<br>
> > > return nullptr;<br>
> > > }<br>
> > ><br>
> > > -CamHelper::CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > - unsigned int frameIntegrationDiff)<br>
> > > - : parser_(parser), initialized_(false),<br>
> > maxFrameLength_(maxFrameLength),<br>
> > > +CamHelper::CamHelper(MdParser *parser, unsigned int<br>
> > frameIntegrationDiff)<br>
> > > + : parser_(parser), initialized_(false),<br>
> > > frameIntegrationDiff_(frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > > @@ -67,13 +66,12 @@ uint32_t CamHelper::GetVBlanking(double &exposure,<br>
> > double minFrameDuration,<br>
> > > assert(initialized_);<br>
> > ><br>
> > > /*<br>
> > > - * Clamp frame length by the frame duration range and the maximum<br>
> > allowable<br>
> > > - * value in the sensor, given by maxFrameLength_.<br>
> > > + * minFrameDuration and maxFrameDuration will have been clamped to<br>
> > the<br>
> ><br>
> > Maybe "minFrameDuration and maxFrameDuration are clamped by the caller<br>
> > based on the limits for the active sensor mode" to make clear who is<br>
> > responsible for clamping those values ?<br>
> ><br>
><br>
> That wording sounds good to me.<br>
><br>
><br>
> ><br>
> > > + * maximum allowable values in the sensor for this mode.<br>
> > > */<br>
> > > - frameLengthMin = std::clamp<uint32_t>(1e3 * minFrameDuration /<br>
> > mode_.line_length,<br>
> > > - mode_.height,<br>
> > maxFrameLength_);<br>
> > > - frameLengthMax = std::clamp<uint32_t>(1e3 * maxFrameDuration /<br>
> > mode_.line_length,<br>
> > > - mode_.height,<br>
> > maxFrameLength_);<br>
> > > + frameLengthMin = 1e3 * minFrameDuration / mode_.line_length;<br>
> > > + frameLengthMax = 1e3 * maxFrameDuration / mode_.line_length;<br>
> > > +<br>
> > > /*<br>
> > > * Limit the exposure to the maximum frame duration requested, and<br>
> > > * re-calculate if it has been clipped.<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > index b1739a57e342..14d70112cb62 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper.hpp<br>
> > > @@ -62,8 +62,7 @@ class CamHelper<br>
> > > {<br>
> > > public:<br>
> > > static CamHelper *Create(std::string const &cam_name);<br>
> > > - CamHelper(MdParser *parser, unsigned int maxFrameLength,<br>
> > > - unsigned int frameIntegrationDiff);<br>
> > > + CamHelper(MdParser *parser, unsigned int frameIntegrationDiff);<br>
> > > virtual ~CamHelper();<br>
> > > void SetCameraMode(const CameraMode &mode);<br>
> > > MdParser &Parser() const { return *parser_; }<br>
> > > @@ -86,8 +85,6 @@ protected:<br>
> > ><br>
> > > private:<br>
> > > bool initialized_;<br>
> > > - /* Largest possible frame length, in units of lines. */<br>
> > > - unsigned int maxFrameLength_;<br>
> > > /*<br>
> > > * Smallest difference between the frame length and integration<br>
> > time,<br>
> > > * in units of lines.<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > index 8688ec091c44..95b8e698fe3b 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx219.cpp<br>
> > > @@ -56,15 +56,13 @@ private:<br>
> > > * in units of lines.<br>
> > > */<br>
> > > static constexpr int frameIntegrationDiff = 4;<br>
> > > - /* Largest possible frame length, in units of lines. */<br>
> > > - static constexpr int maxFrameLength = 0xffff;<br>
> > > };<br>
> > ><br>
> > > CamHelperImx219::CamHelperImx219()<br>
> > > #if ENABLE_EMBEDDED_DATA<br>
> > > - : CamHelper(new MdParserImx219(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > + : CamHelper(new MdParserImx219(), frameIntegrationDiff)<br>
> > > #else<br>
> > > - : CamHelper(new MdParserRPi(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)<br>
> > > #endif<br>
> > > {<br>
> > > }<br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > index 5396131003f1..eaa7be16d20e 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_imx477.cpp<br>
> > > @@ -45,12 +45,10 @@ private:<br>
> > > * in units of lines.<br>
> > > */<br>
> > > static constexpr int frameIntegrationDiff = 22;<br>
> > > - /* Largest possible frame length, in units of lines. */<br>
> > > - static constexpr int maxFrameLength = 0xffdc;<br>
> > > };<br>
> > ><br>
> > > CamHelperImx477::CamHelperImx477()<br>
> > > - : CamHelper(new MdParserImx477(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > + : CamHelper(new MdParserImx477(), frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > index 2bd8a754f133..a7f417324048 100644<br>
> > > --- a/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > +++ b/src/ipa/raspberrypi/cam_helper_ov5647.cpp<br>
> > > @@ -30,8 +30,6 @@ private:<br>
> > > * in units of lines.<br>
> > > */<br>
> > > static constexpr int frameIntegrationDiff = 4;<br>
> > > - /* Largest possible frame length, in units of lines. */<br>
> > > - static constexpr int maxFrameLength = 0xffff;<br>
> > > };<br>
> > ><br>
> > > /*<br>
> > > @@ -40,7 +38,7 @@ private:<br>
> > > */<br>
> > ><br>
> > > CamHelperOv5647::CamHelperOv5647()<br>
> > > - : CamHelper(new MdParserRPi(), maxFrameLength,<br>
> > frameIntegrationDiff)<br>
> > > + : CamHelper(new MdParserRPi(), frameIntegrationDiff)<br>
> > > {<br>
> > > }<br>
> > ><br>
> > > diff --git a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > index 920f11beb0b0..3baeadaca076 100644<br>
> > > --- a/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > +++ b/src/ipa/raspberrypi/controller/camera_mode.h<br>
> > > @@ -37,6 +37,8 @@ struct CameraMode {<br>
> > > double line_length;<br>
> > > // any camera transform *not* reflected already in the camera<br>
> > tuning<br>
> > > libcamera::Transform transform;<br>
> > > + // minimum and maximum vblanking limits for this mode<br>
> > > + uint32_t vblank_min, vblank_max;<br>
> > > };<br>
> > ><br>
> > > #ifdef __cplusplus<br>
> > > diff --git a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > index 5ccc7a6551f5..30ba9aef9d97 100644<br>
> > > --- a/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > +++ b/src/ipa/raspberrypi/raspberrypi.cpp<br>
> > > @@ -102,6 +102,7 @@ private:<br>
> > > void reportMetadata();<br>
> > > bool parseEmbeddedData(unsigned int bufferId, struct DeviceStatus<br>
> > &deviceStatus);<br>
> > > void processStats(unsigned int bufferId);<br>
> > > + void applyFrameDurations(double minFrameDuration, double<br>
> > maxFrameDuration);<br>
> > > void applyAGC(const struct AgcStatus *agcStatus, ControlList<br>
> > &ctrls);<br>
> > > void applyAWB(const struct AwbStatus *awbStatus, ControlList<br>
> > &ctrls);<br>
> > > void applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls);<br>
> > > @@ -279,6 +280,14 @@ void IPARPi::setMode(const CameraSensorInfo<br>
> > &sensorInfo)<br>
> > > * the line length in pixels and the pixel rate.<br>
> > > */<br>
> > > mode_.line_length = 1e9 * sensorInfo.lineLength /<br>
> > sensorInfo.pixelRate;<br>
> > > +<br>
> > > + /*<br>
> > > + * Set the vertical blanking (frame duration) limits for the mode<br>
> > to<br>
> > > + * ensure exposure and framerate calculations are clipped<br>
> > appropriately.<br>
> > > + */<br>
> > > + const auto itVblank = sensorCtrls_.find(V4L2_CID_VBLANK);<br>
> > > + mode_.vblank_min = itVblank->second.min().get<int32_t>();<br>
> > > + mode_.vblank_max = itVblank->second.max().get<int32_t>();<br>
> ><br>
> > The trouble here is that sensorCtrls_ will not be updated after the<br>
> > sensor configuration has changed. It comes from the<br>
> > V4L2Device::controls() call in RPiCameraData::configureIPA(), and that<br>
> > function returns a cached copy.<br>
> ><br>
><br>
> I don't exactly know all of this in very much detail, so apologies if the<br>
> following statements sound basic, or don't make much sense :-)<br>
><br>
> In RPiCameraData::configureIPA(), would it be possible to "update" the<br>
> cached values of V4L2Device::controls_ by<br>
> calling V4L2Device::listControls(), or maybe a new method such<br>
> as V4L2Device::updateControls(). This only needs to be called once<br>
> in PipelineHandlerRPi::configure(), after the sensor mode has been set.<br>
> This way, the VBLANK control will have up-to-date numbers for the IPA to<br>
> use.<br>
><br>
> Alternatively, CameraSensorInfo gets populated in<br>
> RPiCameraData::configureIPA() with a call<br>
> to CameraSensor::sensorInfo(). Looks like this directly queries the device<br>
> for controls via V4L2Device::getControls(). Perhaps we should re-visit<br>
> Jacopo's suggestion and add the min/max vblank values to the mode?<br>
><br>
><br>
> ><br>
> > I really dislike the fact that V4L2 provides us with a control whose<br>
> > limits change when the analog crop rectangle height changes, while the<br>
> > sensors usually have a fixed vertical total size limit. Wouldn't it be<br>
> > better to base the code on the vertical total size limit in libcamera,<br>
> > instead of vertical blanking ?<br>
><br>
><br>
> By vertical total size limit, I assume you mean what sensors generally call<br>
> vts or frame length? If so, then yes, it does make sense. In fact, you<br>
> can see from my patch, the first thing I do is convert from vblank to frame<br>
> length for all of my calculations.<br>
><br>
><br>
<br>
Let me summarize your understandings:<br>
- You need the VBLANK limits to clamp frame durations in the sensor<br>
limits<br>
- VBLANK min and max are combined with the current mode height and<br>
line length to get the min and max frame sizes, from where the min<br>
and max frame duration limits are calculated<br>
- min frame size = (mode_.height + frameIntegrationDiff) * mode_.line_length<br>
- max frame size = (mode_.height + max vblank) * mode_.line_length<br>
<br>
>From the CameraSensorInfo you already know the visibile height and the<br>
line_length. Wouldn't be enough to add to that structure<br>
- maxFrameHeight = (mode_.height + max vblank)<br>
- frameIntegrationDiff<br>
This is sensor property and will be made available in the<br>
sensor database. You can keep it in the IPA as long as no<br>
sensor database is available<br>
<br>
To sum it up: is it enough for you to pass to the IPA the maximum<br>
frame length (mode_.height + max vblank) in the CameraSensorClass ?<br></blockquote><div><br></div><div>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.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> > We would still have the issue of<br>
> > retrieving this value, and I can see multiple options:<br>
> ><br>
> > - Compute it once at initialization from vblank limits + analog crop<br>
> ><br>
><br>
> This is assuming that vblank + crop height is always going to be equal to<br>
> frame length? We need to think if that is strictly true for all sensors,<br>
> I'm not sure.<br>
><br>
><br>
><br>
> > - Extend V4L2 with a new vertical total size control and use it (that<br>
> > will be more work as we need to synchronize with the kernel changes)<br>
> ><br>
><br>
> I expect this would have the longest lead time :-)<br>
><br>
><br>
> > - Hardcode the value in a sensor database in libcamera, bypassing the<br>
> > kernel completly<br>
> ><br>
><br>
> This is possible, but...<br>
><br>
><br>
> ><br>
> > One question that I'm not sure I can answer by myself is whether we can<br>
> > rely on the vertical total size limits being constant.<br>
> ><br>
><br>
> ... I don't think you can assume total vertical size (frame length) is<br>
> going to be the same for all modes a sensor advertises. In this case, the<br>
> sensor database would have to know about every mode available in the kernel<br>
> driver. This is something we moved away from with the Raspberry Pi<br>
> CamHelper, so I doubt you want to re-introduce that link.<br>
><br>
> My feeling is that we could work something out by forcing a "refresh" of<br>
> the cached controls on every configure, and I think that should cover our<br>
> (or at least Raspberry Pi's in this particular instance) usage.<br>
><br>
> Let me know your thoughts.<br>
><br>
> Regards,<br>
> Naush<br>
><br>
><br>
><br>
> > > }<br>
> > ><br>
> > > void IPARPi::configure(const CameraSensorInfo &sensorInfo,<br>
> > > @@ -384,8 +393,8 @@ void IPARPi::configure(const CameraSensorInfo<br>
> > &sensorInfo,<br>
> > > controller_.Initialise();<br>
> > > controllerInit_ = true;<br>
> > ><br>
> > > - minFrameDuration_ = defaultMinFrameDuration;<br>
> > > - maxFrameDuration_ = defaultMaxFrameDuration;<br>
> > > + /* Supply initial values for frame durations. */<br>
> > > + applyFrameDurations(defaultMinFrameDuration,<br>
> > defaultMaxFrameDuration);<br>
> > ><br>
> > > /* Supply initial values for gain and exposure. */<br>
> > > ControlList ctrls(sensorCtrls_);<br>
> > > @@ -819,20 +828,7 @@ void IPARPi::queueRequest(const ControlList<br>
> > &controls)<br>
> > ><br>
> > > case controls::FRAME_DURATIONS: {<br>
> > > auto frameDurations = ctrl.second.get<Span<const<br>
> > int64_t>>();<br>
> > > -<br>
> > > - /* This will be applied once AGC recalculations<br>
> > occur. */<br>
> > > - minFrameDuration_ = frameDurations[0] ?<br>
> > frameDurations[0] : defaultMinFrameDuration;<br>
> > > - maxFrameDuration_ = frameDurations[1] ?<br>
> > frameDurations[1] : defaultMaxFrameDuration;<br>
> > > - maxFrameDuration_ = std::max(maxFrameDuration_,<br>
> > minFrameDuration_);<br>
> > > -<br>
> > > - /*<br>
> > > - * \todo The values returned in the metadata below<br>
> > must be<br>
> > > - * correctly clipped by what the sensor mode<br>
> > supports and<br>
> > > - * what the AGC exposure mode or manual shutter<br>
> > speed limits<br>
> > > - */<br>
> > > - libcameraMetadata_.set(controls::FrameDurations,<br>
> > > - {<br>
> > static_cast<int64_t>(minFrameDuration_),<br>
> > > -<br>
> > static_cast<int64_t>(maxFrameDuration_) });<br>
> > > + applyFrameDurations(frameDurations[0],<br>
> > frameDurations[1]);<br>
> > > break;<br>
> > > }<br>
> > ><br>
> > > @@ -991,6 +987,29 @@ void IPARPi::applyAWB(const struct AwbStatus<br>
> > *awbStatus, ControlList &ctrls)<br>
> > > static_cast<int32_t>(awbStatus->gain_b * 1000));<br>
> > > }<br>
> > ><br>
> > > +void IPARPi::applyFrameDurations(double minFrameDuration, double<br>
> > maxFrameDuration)<br>
> > > +{<br>
> > > + const double minSensorFrameDuration = 1e-3 * (mode_.vblank_min +<br>
> > mode_.height) *<br>
> > > + mode_.line_length;<br>
> > > + const double maxSensorFrameDuration = 1e-3 * (mode_.vblank_max +<br>
> > mode_.height) *<br>
> > > + mode_.line_length;<br>
<br>
To transform the frame size expressed in pixels:<br>
(mode_.vblank + mode_.height) * mode_.line_length<br>
<br>
in a duration expressed in seconds sub-units, don't you need to use<br>
the pixel clock (which is available in CameraSensorInfo afair)<br>
frameDuration (usec) = frame size (pixels)<br>
/ pixel rate (pixels/usec)<br></blockquote><div><br></div><div>Yes we do. However, mode_.line_length already factors the pixel rate:</div><div>mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate<br></div><div><br></div><div>So
mode_.line_length is already converted into units of time.</div><div><br></div><div>Regards,<br>Naush</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks<br>
j<br>
<br>
> > > + /*<br>
> > > + * This will only be applied once AGC recalculations occur.<br>
> > > + * The values may be clamped based on the sensor mode capabilities<br>
> > as well.<br>
> > > + */<br>
> > > + minFrameDuration_ = minFrameDuration ? minFrameDuration :<br>
> > defaultMaxFrameDuration;<br>
> > > + maxFrameDuration_ = maxFrameDuration ? maxFrameDuration :<br>
> > defaultMinFrameDuration;<br>
> > > + minFrameDuration_ = std::clamp(minFrameDuration_,<br>
> > > + minSensorFrameDuration,<br>
> > maxSensorFrameDuration);<br>
> > > + maxFrameDuration_ = std::clamp(maxFrameDuration_,<br>
> > > + minSensorFrameDuration,<br>
> > maxSensorFrameDuration);<br>
> > > +<br>
> > > + /* Return the validated limits out though metadata. */<br>
> > > + libcameraMetadata_.set(controls::FrameDurations,<br>
> > > + { static_cast<int64_t>(minFrameDuration_),<br>
> > > + static_cast<int64_t>(maxFrameDuration_)<br>
> > });<br>
> > > +}<br>
> > > +<br>
> > > void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList<br>
> > &ctrls)<br>
> > > {<br>
> > --<br>
> > Regards,<br>
> ><br>
> > Laurent Pinchart<br>
> ><br>
<br>
<br>
> _______________________________________________<br>
> libcamera-devel mailing list<br>
> <a href="mailto:libcamera-devel@lists.libcamera.org" target="_blank">libcamera-devel@lists.libcamera.org</a><br>
> <a href="https://lists.libcamera.org/listinfo/libcamera-devel" rel="noreferrer" target="_blank">https://lists.libcamera.org/listinfo/libcamera-devel</a><br>
<br>
</blockquote></div></div>