| Message ID | 20230331081930.19289-9-dse@thaumatec.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel On Fri, Mar 31, 2023 at 10:19:28AM +0200, Daniel Semkowicz via libcamera-devel wrote: > Add controls supported by the AF algorithm to the list of controls > supported by the RkISP1 IPA. This exposes the AF controls to the user > and allows controlling the AF algorithm using the top level API. > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > --- > src/ipa/rkisp1/rkisp1.cpp | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index 292768cf..9c8b4a82 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -456,6 +456,28 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > frameDurations[1], > frameDurations[2]); > > + if (lensControls_) { > + const ControlInfo &focusAbsolute = > + lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE); > + > + using namespace controls; > + > + ctrlMap[&AfMetering] = ControlInfo(AfMeteringValues); > + ctrlMap[&AfMode] = ControlInfo(AfModeValues); > + ctrlMap[&AfPause] = ControlInfo( > + Span<const ControlValue>{ > + { static_cast<int32_t>(AfPauseImmediate), > + static_cast<int32_t>(AfPauseResume) } }); > + ctrlMap[&AfTrigger] = ControlInfo(AfTriggerValues); > + ctrlMap[&AfWindows] = ControlInfo( > + Rectangle(), Rectangle(sensorInfo.outputSize), I wonder if the ISP doesn't have a minimum valid window size.. > + Rectangle()); > + ctrlMap[&LensPosition] = ControlInfo( > + static_cast<float>(focusAbsolute.min().get<int32_t>()), > + static_cast<float>(focusAbsolute.max().get<int32_t>()), > + static_cast<float>(focusAbsolute.def().get<int32_t>())); We're here exposing the values as they come from the v4l2 control interface. We should get to a point where we have a CameraLensHelper like we have CameraSensorHelpers to translate the platform-specific value to a generic value. I can add a \todo when applying if you agree with this. The rest looks good Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > + } > + > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > } > > -- > 2.39.2 >
Hi Jacopo, On Mon, Apr 3, 2023 at 11:37 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Fri, Mar 31, 2023 at 10:19:28AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Add controls supported by the AF algorithm to the list of controls > > supported by the RkISP1 IPA. This exposes the AF controls to the user > > and allows controlling the AF algorithm using the top level API. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > src/ipa/rkisp1/rkisp1.cpp | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index 292768cf..9c8b4a82 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -456,6 +456,28 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > frameDurations[1], > > frameDurations[2]); > > > > + if (lensControls_) { > > + const ControlInfo &focusAbsolute = > > + lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE); > > + > > + using namespace controls; > > + > > + ctrlMap[&AfMetering] = ControlInfo(AfMeteringValues); > > + ctrlMap[&AfMode] = ControlInfo(AfModeValues); > > + ctrlMap[&AfPause] = ControlInfo( > > + Span<const ControlValue>{ > > + { static_cast<int32_t>(AfPauseImmediate), > > + static_cast<int32_t>(AfPauseResume) } }); > > + ctrlMap[&AfTrigger] = ControlInfo(AfTriggerValues); > > + ctrlMap[&AfWindows] = ControlInfo( > > + Rectangle(), Rectangle(sensorInfo.outputSize), > > I wonder if the ISP doesn't have a minimum valid window size.. Unfortunately, I did not find any documentation on that. Only the window margins. > > > + Rectangle()); > > + ctrlMap[&LensPosition] = ControlInfo( > > + static_cast<float>(focusAbsolute.min().get<int32_t>()), > > + static_cast<float>(focusAbsolute.max().get<int32_t>()), > > + static_cast<float>(focusAbsolute.def().get<int32_t>())); > > We're here exposing the values as they come from the v4l2 control > interface. We should get to a point where we have a CameraLensHelper > like we have CameraSensorHelpers to translate the platform-specific > value to a generic value. I can add a \todo when applying if you agree > with this. Yes, I agree. For now, let's start with something basic that works :) > > The rest looks good > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > > > > + } > > + > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > } > > > > -- > > 2.39.2 > > Best regards Daniel
Hello Laurent, On Wed, Apr 26, 2023 at 5:58 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Daniel, > > Thank you for the patch. > > On Tue, Apr 04, 2023 at 11:49:59AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > On Mon, Apr 3, 2023 at 11:37 AM Jacopo Mondi wrote: > > > On Fri, Mar 31, 2023 at 10:19:28AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Add controls supported by the AF algorithm to the list of controls > > > > supported by the RkISP1 IPA. This exposes the AF controls to the user > > > > and allows controlling the AF algorithm using the top level API. > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > --- > > > > src/ipa/rkisp1/rkisp1.cpp | 22 ++++++++++++++++++++++ > > > > 1 file changed, 22 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index 292768cf..9c8b4a82 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -456,6 +456,28 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > > > frameDurations[1], > > > > frameDurations[2]); > > > > > > > > + if (lensControls_) { > > > > + const ControlInfo &focusAbsolute = > > > > + lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE); > > > > + > > > > + using namespace controls; > > > > + > > > > + ctrlMap[&AfMetering] = ControlInfo(AfMeteringValues); > > > > + ctrlMap[&AfMode] = ControlInfo(AfModeValues); > > > > + ctrlMap[&AfPause] = ControlInfo( > > > > + Span<const ControlValue>{ > > > > + { static_cast<int32_t>(AfPauseImmediate), > > > > + static_cast<int32_t>(AfPauseResume) } }); > > > > + ctrlMap[&AfTrigger] = ControlInfo(AfTriggerValues); > > > > + ctrlMap[&AfWindows] = ControlInfo( > > > > + Rectangle(), Rectangle(sensorInfo.outputSize), > > > > > > I wonder if the ISP doesn't have a minimum valid window size.. > > > > Unfortunately, I did not find any documentation on that. > > Only the window margins. > > There doesn't seem to be a minimum size other than (1, 1), as it makes > no sense to compute AF stats on an empty window. I'd set the minimum to > that value. > > > > > + Rectangle()); > > The default should match the 3/4 of the image used by default by the AF > algorithm. > > > > > + ctrlMap[&LensPosition] = ControlInfo( > > > > + static_cast<float>(focusAbsolute.min().get<int32_t>()), > > > > + static_cast<float>(focusAbsolute.max().get<int32_t>()), > > > > + static_cast<float>(focusAbsolute.def().get<int32_t>())); > > > > > > We're here exposing the values as they come from the v4l2 control > > > interface. We should get to a point where we have a CameraLensHelper > > > like we have CameraSensorHelpers to translate the platform-specific > > > value to a generic value. I can add a \todo when applying if you agree > > > with this. > > > > Yes, I agree. For now, let's start with something basic that works :) > > We don't have to calibrate the values yet, but they should already be > expressed in 1/distance units. > > > > The rest looks good > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > + } > > > > + > > > > *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > > > } > > > > > > -- > Regards, > > Laurent Pinchart Thank you for the review. These comments are reasonable. I will correct the code according to the comments. Best regards Daniel
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 292768cf..9c8b4a82 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -456,6 +456,28 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, frameDurations[1], frameDurations[2]); + if (lensControls_) { + const ControlInfo &focusAbsolute = + lensControls_->at(V4L2_CID_FOCUS_ABSOLUTE); + + using namespace controls; + + ctrlMap[&AfMetering] = ControlInfo(AfMeteringValues); + ctrlMap[&AfMode] = ControlInfo(AfModeValues); + ctrlMap[&AfPause] = ControlInfo( + Span<const ControlValue>{ + { static_cast<int32_t>(AfPauseImmediate), + static_cast<int32_t>(AfPauseResume) } }); + ctrlMap[&AfTrigger] = ControlInfo(AfTriggerValues); + ctrlMap[&AfWindows] = ControlInfo( + Rectangle(), Rectangle(sensorInfo.outputSize), + Rectangle()); + ctrlMap[&LensPosition] = ControlInfo( + static_cast<float>(focusAbsolute.min().get<int32_t>()), + static_cast<float>(focusAbsolute.max().get<int32_t>()), + static_cast<float>(focusAbsolute.def().get<int32_t>())); + } + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); }
Add controls supported by the AF algorithm to the list of controls supported by the RkISP1 IPA. This exposes the AF controls to the user and allows controlling the AF algorithm using the top level API. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- src/ipa/rkisp1/rkisp1.cpp | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)