<div dir="ltr"><div dir="ltr">Hi Jacopo and Laurent,<div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, 26 Jan 2021 at 14:39, 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,<br>
<br>
On Tue, Jan 26, 2021 at 02:26:59PM +0000, Naushir Patuck wrote:<br>
> Hi Jacopo,<br>
><br>
> On Tue, 26 Jan 2021 at 14:21, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
><br>
> > Hi Naush,<br>
> ><br>
> > On Tue, Jan 26, 2021 at 02:12:50PM +0000, Naushir Patuck wrote:<br>
> > > Hi Jacopo,<br>
> > ><br>
> > > On Tue, 26 Jan 2021 at 13:44, Jacopo Mondi <<a href="mailto:jacopo@jmondi.org" target="_blank">jacopo@jmondi.org</a>> wrote:<br>
> > ><br>
> ><br>
> > [snip]<br>
> ><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<br>
> > that<br>
> > > > > > function returns a cached copy.<br>
> > > > > ><br>
> > > > ><br>
> > > > > I don't exactly know all of this in very much detail, so apologies<br>
> > if the<br>
> > > > > following statements sound basic, or don't make much sense :-)<br>
> > > > ><br>
> > > > > In  RPiCameraData::configureIPA(), would it be possible to "update"<br>
> > 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<br>
> > set.<br>
> > > > > This way, the VBLANK control will have up-to-date numbers for the<br>
> > 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<br>
> > > > device<br>
> > > > > for controls via V4L2Device::getControls().  Perhaps we should<br>
> > 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<br>
> > whose<br>
> > > > > > limits change when the analog crop rectangle height changes, while<br>
> > the<br>
> > > > > > sensors usually have a fixed vertical total size limit. Wouldn't<br>
> > it be<br>
> > > > > > better to base the code on the vertical total size limit in<br>
> > libcamera,<br>
> > > > > > instead of vertical blanking ?<br>
> > > > ><br>
> > > > ><br>
> > > > > By vertical total size limit, I assume you mean what sensors<br>
> > generally<br>
> > > > call<br>
> > > > > vts or frame length?  If so, then yes, it does make sense.  In fact,<br>
> > you<br>
> > > > > can see from my patch, the first thing I do is convert from vblank to<br>
> > > > 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) *<br>
> > > > 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>
> > > ><br>
> > ><br>
> > > This is *mostly* correct.  max frame size as calculated above is fine.<br>
> > > However, I do also need min frame size.  Your calculation above assumes<br>
> > > min_vblank = 0.  This may not be the case always, so min frame size must<br>
> > be<br>
> > > computed and presented in CameraSensorInfo.<br>
> ><br>
> > Correct, I had assumed min_vblank = frameIntegrationDiff, which might<br>
> > not always be the case.<br>
> ><br>
> > Considering the requirement for 'updates' and the fact the<br>
> > CameraSensorInfo is guaranteed to be fresh at IPA::Configure() time,<br>
> > I would be happy to see minFrameSize (or Height) and maxFrameSize (or<br>
> > Height) added there an computed once for all IPAs in the CameraSensor<br>
> > class.<br>
> ><br>
><br>
> Great, I can put together a change for this and submit for review as part<br>
> of this series.  One question, what name would you and Laurent prefer for<br>
> this field - maxFrameSize or maxFrameLength (and equivalent for min)?<br>
> Generally, I think sensors prefer using frame length in their terminology,<br>
> but v4l2 probably prefers frame size.  I am ok with either.<br>
><br>
<br>
This and also if we want to report only the frame heights limits<br>
and let the IPA compute the frame size, or report the actual frame<br>
size. It's a detail, I know, you can easily get from one to the other<br>
and viceversa..<br></blockquote><div><br></div><div>So just to confirm before I make a submission, is something like this what you had in mind?</div><div><br></div><div>diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp<br>index 251691aedfa3..6037f5f72897 100644<br>--- a/src/libcamera/camera_sensor.cpp<br>+++ b/src/libcamera/camera_sensor.cpp<br></div><div>/**<br>  * \class CameraSensor<br>  * \brief A camera sensor based on V4L2 subdevices<br>@@ -698,11 +728,12 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const<br> <br>        /*<br>         * Retrieve the pixel rate and the line length through V4L2 controls.<br>-        * Support for the V4L2_CID_PIXEL_RATE and V4L2_CID_HBLANK controls is<br>-        * mandatory.<br>+        * Support for the V4L2_CID_PIXEL_RATE, V4L2_CID_HBLANK and<br>+        * V4L2_CID_VBLANK controls is mandatory.<br>         */<br>        ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,<br>-                                                  V4L2_CID_HBLANK });<br>+                                                  V4L2_CID_HBLANK,<br>+                                                  V4L2_CID_VBLANK });<br>        if (ctrls.empty()) {<br>                LOG(CameraSensor, Error)<br>                        << "Failed to retrieve camera info controls";<br>@@ -713,6 +744,10 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const<br>        info->lineLength = info->outputSize.width + hblank;<br>        info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get<int64_t>();<br> <br>+       const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK);<br>+       info->minFrameLength = info->outputSize.height + vblank.min().get<int32_t>();<br>+       info->maxFrameLength = info->outputSize.height + vblank.max().get<int32_t>();<br>+<br>        return 0;<br> }<br></div><div><br></div><div>Let me know if you think that is suitable or not.</div><div><br></div><div><br></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>
> Regards,<br>
> Naush<br>
><br>
><br>
><br>
> > ><br>
> > ><br>
> > ><br>
> > > ><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<br>
> > crop<br>
> > > > > ><br>
> > > > ><br>
> > > > > This is assuming that vblank + crop height is always going to be<br>
> > equal to<br>
> > > > > frame length?  We need to think if that is strictly true for all<br>
> > sensors,<br>
> > > > > I'm not sure.<br>
> > > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > > - Extend V4L2 with a new vertical total size control and use it<br>
> > (that<br>
> > > > > >   will be more work as we need to synchronize with the kernel<br>
> > 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<br>
> > 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<br>
> > 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)<br>
> > is<br>
> > > > > going to be the same for all modes a sensor advertises.  In this<br>
> > case,<br>
> > > > the<br>
> > > > > sensor database would have to know about every mode available in the<br>
> > > > 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<br>
> > "refresh" of<br>
> > > > > the cached controls on every configure, and I think that should<br>
> > 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>
> ><br>
> > [snip]<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 =<br>
> > > > ctrl.second.get<Span<const<br>
> > > > > > int64_t>>();<br>
> > > > > > > -<br>
> > > > > > > -                     /* This will be applied once AGC<br>
> > recalculations<br>
> > > > > > occur. */<br>
> > > > > > > -                     minFrameDuration_ = frameDurations[0] ?<br>
> > > > > > frameDurations[0] : defaultMinFrameDuration;<br>
> > > > > > > -                     maxFrameDuration_ = frameDurations[1] ?<br>
> > > > > > frameDurations[1] : defaultMaxFrameDuration;<br>
> > > > > > > -                     maxFrameDuration_ =<br>
> > std::max(maxFrameDuration_,<br>
> > > > > > minFrameDuration_);<br>
> > > > > > > -<br>
> > > > > > > -                     /*<br>
> > > > > > > -                      * \todo The values returned in the<br>
> > metadata<br>
> > > > below<br>
> > > > > > must be<br>
> > > > > > > -                      * correctly clipped by what the sensor<br>
> > mode<br>
> > > > > > supports and<br>
> > > > > > > -                      * what the AGC exposure mode or manual<br>
> > shutter<br>
> > > > > > speed limits<br>
> > > > > > > -                      */<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 *<br>
> > (mode_.vblank_min<br>
> > > > +<br>
> > > > > > mode_.height) *<br>
> > > > > > > +                                           mode_.line_length;<br>
> > > > > > > +     const double maxSensorFrameDuration = 1e-3 *<br>
> > (mode_.vblank_max<br>
> > > > +<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>
> > > ><br>
> > ><br>
> > > Yes we do.  However, mode_.line_length already factors the pixel rate:<br>
> > > mode_.line_length = 1e9 * sensorInfo.lineLength / sensorInfo.pixelRate<br>
> > ><br>
> > > So mode_.line_length is already converted into units of time.<br>
> ><br>
> > Oh sorry, I assumed CameraMode.line_lenght == CameraSensorInfo.lineLength<br>
> ><br>
> > I should have checked.<br>
> ><br>
> > Thanks<br>
> >   j<br>
> ><br>
> > ><br>
> > > Regards,<br>
> > > Naush<br>
> > ><br>
> > ><br>
> > ><br>
> > > ><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<br>
> > > > 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>
> > > > > > > +                            {<br>
> > > > static_cast<int64_t>(minFrameDuration_),<br>
> > > > > > > +<br>
> > > > static_cast<int64_t>(maxFrameDuration_)<br>
> > > > > > });<br>
> > > > > > > +}<br>
> > > > > > > +<br>
> > > > > > >  void IPARPi::applyAGC(const struct AgcStatus *agcStatus,<br>
> > 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>
> > > ><br>
> ><br>
</blockquote></div></div>