| Message ID | 20230324142908.64224-9-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi daniel On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index cd1fbae3..4b30844f 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -101,10 +101,16 @@ namespace { > /* List of controls handled by the RkISP1 IPA */ > const ControlInfoMap::Map rkisp1Controls{ > { &controls::AeEnable, ControlInfo(false, true) }, > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, AfPauseDeferred is not supported Otherwise Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, We should -really- move to instantiate control limits directly based on the sensor configuration... > { &controls::AwbEnable, ControlInfo(false, true) }, > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > -- > 2.39.2 >
On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > Hi daniel > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp >> index cd1fbae3..4b30844f 100644 >> --- a/src/ipa/rkisp1/rkisp1.cpp >> +++ b/src/ipa/rkisp1/rkisp1.cpp >> @@ -101,10 +101,16 @@ namespace { >> /* List of controls handled by the RkISP1 IPA */ >> const ControlInfoMap::Map rkisp1Controls{ >> { &controls::AeEnable, ControlInfo(false, true) }, >> + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, >> + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > AfPauseDeferred is not supported > What happens if there's no VCM though - Should we only expose the controls if a VCM is identified? (Though that requires reworking the creation of this rkisp1Controls list to be handled by the algorithms) -- Kieran > Otherwise > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > >> + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, >> + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, >> + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > We should -really- move to instantiate control limits directly based > on the sensor configuration... > >> { &controls::AwbEnable, ControlInfo(false, true) }, >> { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, >> { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, >> { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, >> + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, >> { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, >> { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, >> { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, >> -- >> 2.39.2 >>
Hi Kieran On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > Hi daniel > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > index cd1fbae3..4b30844f 100644 > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > @@ -101,10 +101,16 @@ namespace { > > > /* List of controls handled by the RkISP1 IPA */ > > > const ControlInfoMap::Map rkisp1Controls{ > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > AfPauseDeferred is not supported > > > > What happens if there's no VCM though - Should we only expose the controls You're very right, I overlooked that. > if a VCM is identified? (Though that requires reworking the creation of this > rkisp1Controls list to be handled by the algorithms) I'm not sure it needs to be done by algorithms, but the IPA should certainly be made aware of the lens presence at init() time. To be honest I would be fine with what RPi does in this regard. have a look at the usage of lensPresent_ in their IPA module init() function. This looks like the less invasive change. I presume this could even be made better by passing the lens controls' from the PH to the IPA at init() time so that the AF ControlInfo elements could be initialized with the proper limits and not with default values as it happens here and on RPi. > > -- > Kieran > > > > Otherwise > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > We should -really- move to instantiate control limits directly based > > on the sensor configuration... > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > -- > > > 2.39.2 > > >
Hi Jacopo, Hi Kieran, On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Kieran > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > > Hi daniel > > > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > > 1 file changed, 6 insertions(+) > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > index cd1fbae3..4b30844f 100644 > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > @@ -101,10 +101,16 @@ namespace { > > > > /* List of controls handled by the RkISP1 IPA */ > > > > const ControlInfoMap::Map rkisp1Controls{ > > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > > > AfPauseDeferred is not supported > > > > > > > What happens if there's no VCM though - Should we only expose the controls > > You're very right, I overlooked that. > > > if a VCM is identified? (Though that requires reworking the creation of this > > rkisp1Controls list to be handled by the algorithms) > > I'm not sure it needs to be done by algorithms, but the IPA should > certainly be made aware of the lens presence at init() time. This is indeed something that is needed, especially that number of algorithm implementations is increasing. The most appropriate solution seems for the algorithms to expose the supported control list or to populate the list. This would also fix the problem of checking outside the algorithm, if certain configuration is available, like lens. I see at least two problems with the current approach: 1. controls are exposed by the IPA, even if the algorithm is not enabled in the tuning file. This is confusing. 2. What if there are, for example, two AF algorithm implementations, with different supported controls? Which controls should be added to the control list? > > To be honest I would be fine with what RPi does in this regard. have a > look at the usage of lensPresent_ in their IPA module init() function. > This looks like the less invasive change. > > I presume this could even be made better by passing the lens controls' > from the PH to the IPA at init() time so that the AF ControlInfo > elements could be initialized with the proper limits and not with > default values as it happens here and on RPi. For me, moving it to init() is also a better solution. I would like to avoid intermediate steps and initialize IPA lens config in one place for clarity. This means the whole lens configuration that's currently done in the configure(), will need to be moved to init(). However, I see one problem. context_.configuration struct is reset in configure() for some reason. Even that context_.configuration.sensor.lineDuration is set in init(). It is reset and set again. What about making the IPASessionConfiguration::lens structure std::optional, so it will be explicitly stated in the session config if the lens is available? Then, AF can fail in init() if lens are not available. This will work for lens, but I am afraid We will not be able to evaluate all AF ControlInfo elements in the init() phase. For example, AF window size limits depend on the sensor output size, which is only available in the configure(). Best regards Daniel > > > > > -- > > Kieran > > > > > > > Otherwise > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > > > We should -really- move to instantiate control limits directly based > > > on the sensor configuration... > > > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > > -- > > > > 2.39.2 > > > >
Hi Daniel On Mon, Mar 27, 2023 at 12:13:51PM +0200, Daniel Semkowicz via libcamera-devel wrote: > Hi Jacopo, Hi Kieran, > > On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Kieran > > > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > > > > > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > > > Hi daniel > > > > > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > index cd1fbae3..4b30844f 100644 > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > @@ -101,10 +101,16 @@ namespace { > > > > > /* List of controls handled by the RkISP1 IPA */ > > > > > const ControlInfoMap::Map rkisp1Controls{ > > > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > > > > > AfPauseDeferred is not supported > > > > > > > > > > What happens if there's no VCM though - Should we only expose the controls > > > > You're very right, I overlooked that. > > > > > if a VCM is identified? (Though that requires reworking the creation of this > > > rkisp1Controls list to be handled by the algorithms) > > > > I'm not sure it needs to be done by algorithms, but the IPA should > > certainly be made aware of the lens presence at init() time. > > This is indeed something that is needed, especially that number > of algorithm implementations is increasing. The most appropriate > solution seems for the algorithms to expose the supported control list > or to populate the list. This would also fix the problem of checking > outside the algorithm, if certain configuration is available, like > lens. > > I see at least two problems with the current approach: > 1. controls are exposed by the IPA, even if the algorithm is not > enabled in the tuning file. This is confusing. Agreed > 2. What if there are, for example, two AF algorithm implementations, > with different supported controls? Which controls should be added > to the control list? Handling multiple algos for the same functions is not something I think there's a design for at the moment. If we defer registering ControlInfo to the single algorithms I guess the problem then reduces on how to run-time select which algorithm implementation to use. > > > > > To be honest I would be fine with what RPi does in this regard. have a > > look at the usage of lensPresent_ in their IPA module init() function. > > This looks like the less invasive change. > > > > I presume this could even be made better by passing the lens controls' > > from the PH to the IPA at init() time so that the AF ControlInfo > > elements could be initialized with the proper limits and not with > > default values as it happens here and on RPi. > > For me, moving it to init() is also a better solution. I would like to > avoid intermediate steps and initialize IPA lens config in one place > for clarity. This means the whole lens configuration that's currently > done in the configure(), will need to be moved to init(). The part about the lens in configure() intializes lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); As you said, the lens min/max position -shouldn't- change between configurations so this part could be moved at init() time > However, I see one problem. context_.configuration struct is reset > in configure() for some reason. Even that > context_.configuration.sensor.lineDuration is set in init(). It is > reset and set again. The other parameters (such as the gains and shutter speed) change depending on the currently configured sensor's mode, that's why they're re-calculated at each configure() call. > > What about making the IPASessionConfiguration::lens structure > std::optional, so it will be explicitly stated in the session config > if the lens is available? Then, AF can fail in init() if lens > are not available. > Let's take a step back for a second. We have an updateControls() function, that starting for the static map rkisp1Controls augments it with controls whose values are computed by inspecting the sensorControls. Can't we do the same for the lens controls ? Add them only conditionally to the validity of lensControls ? Then if we deem it better we can pass to each algorithm the sensor's and lens configuration and have them populate the list of controls they handle. > This will work for lens, but I am afraid We will not be able to > evaluate all AF ControlInfo elements in the init() phase. > For example, AF window size limits depend on the sensor output size, > which is only available in the configure(). updateControls() has access to sensorInfo which contains the sensor's output size > > Best regards > Daniel > > > > > > > > > -- > > > Kieran > > > > > > > > > > Otherwise > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > > > > > We should -really- move to instantiate control limits directly based > > > > on the sensor configuration... > > > > > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > > > -- > > > > > 2.39.2 > > > > >
On Mon, Mar 27, 2023 at 1:07 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Mon, Mar 27, 2023 at 12:13:51PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > Hi Jacopo, Hi Kieran, > > > > On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Kieran > > > > > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > > > > > > > > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > > > > Hi daniel > > > > > > > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > > index cd1fbae3..4b30844f 100644 > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > > @@ -101,10 +101,16 @@ namespace { > > > > > > /* List of controls handled by the RkISP1 IPA */ > > > > > > const ControlInfoMap::Map rkisp1Controls{ > > > > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > > > > > > > AfPauseDeferred is not supported > > > > > > > > > > > > > What happens if there's no VCM though - Should we only expose the controls > > > > > > You're very right, I overlooked that. > > > > > > > if a VCM is identified? (Though that requires reworking the creation of this > > > > rkisp1Controls list to be handled by the algorithms) > > > > > > I'm not sure it needs to be done by algorithms, but the IPA should > > > certainly be made aware of the lens presence at init() time. > > > > This is indeed something that is needed, especially that number > > of algorithm implementations is increasing. The most appropriate > > solution seems for the algorithms to expose the supported control list > > or to populate the list. This would also fix the problem of checking > > outside the algorithm, if certain configuration is available, like > > lens. > > > > I see at least two problems with the current approach: > > 1. controls are exposed by the IPA, even if the algorithm is not > > enabled in the tuning file. This is confusing. > > Agreed > > > 2. What if there are, for example, two AF algorithm implementations, > > with different supported controls? Which controls should be added > > to the control list? > > Handling multiple algos for the same functions is not something I > think there's a design for at the moment. > > If we defer registering ControlInfo to the single algorithms I guess > the problem then reduces on how to run-time select which algorithm > implementation to use. I meant the current situation with ControlInfo map populated in the IPA instead in algorithms itself. As you said, this should not be a problem with the controls handled in the algorithms. > > > > > > > > > To be honest I would be fine with what RPi does in this regard. have a > > > look at the usage of lensPresent_ in their IPA module init() function. > > > This looks like the less invasive change. > > > > > > I presume this could even be made better by passing the lens controls' > > > from the PH to the IPA at init() time so that the AF ControlInfo > > > elements could be initialized with the proper limits and not with > > > default values as it happens here and on RPi. > > > > For me, moving it to init() is also a better solution. I would like to > > avoid intermediate steps and initialize IPA lens config in one place > > for clarity. This means the whole lens configuration that's currently > > done in the configure(), will need to be moved to init(). > > The part about the lens in configure() intializes > > lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > As you said, the lens min/max position -shouldn't- change between > configurations so this part could be moved at init() time > > > However, I see one problem. context_.configuration struct is reset > > in configure() for some reason. Even that > > context_.configuration.sensor.lineDuration is set in init(). It is > > reset and set again. > > The other parameters (such as the gains and shutter speed) change > depending on the currently configured sensor's mode, that's why > they're re-calculated at each configure() call. > > > > > What about making the IPASessionConfiguration::lens structure > > std::optional, so it will be explicitly stated in the session config > > if the lens is available? Then, AF can fail in init() if lens > > are not available. > > > > Let's take a step back for a second. > > We have an updateControls() function, that starting for the static map > rkisp1Controls augments it with controls whose values are computed by > inspecting the sensorControls. > > Can't we do the same for the lens controls ? Add them only > conditionally to the validity of lensControls ? Then if we deem it > better we can pass to each algorithm the sensor's and lens > configuration and have them populate the list of controls they handle. > Right, this makes sense. I can rework the code like this. Also, don't you think it is worth to fail the AF init if it was enabled on camera without moveable lens? > > > This will work for lens, but I am afraid We will not be able to > > evaluate all AF ControlInfo elements in the init() phase. > > For example, AF window size limits depend on the sensor output size, > > which is only available in the configure(). > > updateControls() has access to sensorInfo which contains the sensor's > output size Ah, I didn't notice that... In such case this is not a problem :) > > > > > Best regards > > Daniel > > > > > > > > > > > > > -- > > > > Kieran > > > > > > > > > > > > > Otherwise > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > > > > > > > We should -really- move to instantiate control limits directly based > > > > > on the sensor configuration... > > > > > > > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > > > > -- > > > > > > 2.39.2 > > > > > >
Hi Daniel On Mon, Mar 27, 2023 at 01:48:28PM +0200, Daniel Semkowicz wrote: > On Mon, Mar 27, 2023 at 1:07 PM Jacopo Mondi > <jacopo.mondi@ideasonboard.com> wrote: > > > > Hi Daniel > > > > On Mon, Mar 27, 2023 at 12:13:51PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > Hi Jacopo, Hi Kieran, > > > > > > On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > Hi Kieran > > > > > > > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > > > > > > > > > > > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > > > > > Hi daniel > > > > > > > > > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > > > index cd1fbae3..4b30844f 100644 > > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > > > @@ -101,10 +101,16 @@ namespace { > > > > > > > /* List of controls handled by the RkISP1 IPA */ > > > > > > > const ControlInfoMap::Map rkisp1Controls{ > > > > > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > > > > > > > > > AfPauseDeferred is not supported > > > > > > > > > > > > > > > > What happens if there's no VCM though - Should we only expose the controls > > > > > > > > You're very right, I overlooked that. > > > > > > > > > if a VCM is identified? (Though that requires reworking the creation of this > > > > > rkisp1Controls list to be handled by the algorithms) > > > > > > > > I'm not sure it needs to be done by algorithms, but the IPA should > > > > certainly be made aware of the lens presence at init() time. > > > > > > This is indeed something that is needed, especially that number > > > of algorithm implementations is increasing. The most appropriate > > > solution seems for the algorithms to expose the supported control list > > > or to populate the list. This would also fix the problem of checking > > > outside the algorithm, if certain configuration is available, like > > > lens. > > > > > > I see at least two problems with the current approach: > > > 1. controls are exposed by the IPA, even if the algorithm is not > > > enabled in the tuning file. This is confusing. > > > > Agreed > > > > > 2. What if there are, for example, two AF algorithm implementations, > > > with different supported controls? Which controls should be added > > > to the control list? > > > > Handling multiple algos for the same functions is not something I > > think there's a design for at the moment. > > > > If we defer registering ControlInfo to the single algorithms I guess > > the problem then reduces on how to run-time select which algorithm > > implementation to use. > > I meant the current situation with ControlInfo map populated > in the IPA instead in algorithms itself. As you said, this should not > be a problem with the controls handled in the algorithms. > > > > > > > > > > > > > > To be honest I would be fine with what RPi does in this regard. have a > > > > look at the usage of lensPresent_ in their IPA module init() function. > > > > This looks like the less invasive change. > > > > > > > > I presume this could even be made better by passing the lens controls' > > > > from the PH to the IPA at init() time so that the AF ControlInfo > > > > elements could be initialized with the proper limits and not with > > > > default values as it happens here and on RPi. > > > > > > For me, moving it to init() is also a better solution. I would like to > > > avoid intermediate steps and initialize IPA lens config in one place > > > for clarity. This means the whole lens configuration that's currently > > > done in the configure(), will need to be moved to init(). > > > > The part about the lens in configure() intializes > > > > lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > > lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > > > As you said, the lens min/max position -shouldn't- change between > > configurations so this part could be moved at init() time > > > > > However, I see one problem. context_.configuration struct is reset > > > in configure() for some reason. Even that > > > context_.configuration.sensor.lineDuration is set in init(). It is > > > reset and set again. > > > > The other parameters (such as the gains and shutter speed) change > > depending on the currently configured sensor's mode, that's why > > they're re-calculated at each configure() call. > > > > > > > > What about making the IPASessionConfiguration::lens structure > > > std::optional, so it will be explicitly stated in the session config > > > if the lens is available? Then, AF can fail in init() if lens > > > are not available. > > > > > > > Let's take a step back for a second. > > > > We have an updateControls() function, that starting for the static map > > rkisp1Controls augments it with controls whose values are computed by > > inspecting the sensorControls. > > > > Can't we do the same for the lens controls ? Add them only > > conditionally to the validity of lensControls ? Then if we deem it > > better we can pass to each algorithm the sensor's and lens > > configuration and have them populate the list of controls they handle. > > > > Right, this makes sense. I can rework the code like this. > Also, don't you think it is worth to fail the AF init if it was > enabled on camera without moveable lens? > mmm, that would make the whole IPA initialization fail if I'm not mistaken. The algorithm is enabled by the sensor configuration file, which indeed shouldn't include it if the device doesn't have a lens. It also seems to me that if we don't fail, the AF algorithm implementation would need to protect agains a missing lens with if (!context.configuration.lens) return 0; in all its methods, as it will be part of the list of active algorithms, and I'm not sure it's worth it. If you agree with the above, then yes, I guess we should fail in init() if no lens is available. > > > > > This will work for lens, but I am afraid We will not be able to > > > evaluate all AF ControlInfo elements in the init() phase. > > > For example, AF window size limits depend on the sensor output size, > > > which is only available in the configure(). > > > > updateControls() has access to sensorInfo which contains the sensor's > > output size > > Ah, I didn't notice that... In such case this is not a problem :) > > > > > > > > > Best regards > > > Daniel > > > > > > > > > > > > > > > > > -- > > > > > Kieran > > > > > > > > > > > > > > > > Otherwise > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > > > > > > > > > We should -really- move to instantiate control limits directly based > > > > > > on the sensor configuration... > > > > > > > > > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > > > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > > > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > > > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > > > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > > > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > > > > > -- > > > > > > > 2.39.2 > > > > > > >
On Mon, Mar 27, 2023 at 2:37 PM Jacopo Mondi <jacopo.mondi@ideasonboard.com> wrote: > > Hi Daniel > > On Mon, Mar 27, 2023 at 01:48:28PM +0200, Daniel Semkowicz wrote: > > On Mon, Mar 27, 2023 at 1:07 PM Jacopo Mondi > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > Hi Daniel > > > > > > On Mon, Mar 27, 2023 at 12:13:51PM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > Hi Jacopo, Hi Kieran, > > > > > > > > On Sat, Mar 25, 2023 at 10:27 AM Jacopo Mondi > > > > <jacopo.mondi@ideasonboard.com> wrote: > > > > > > > > > > Hi Kieran > > > > > > > > > > On Fri, Mar 24, 2023 at 03:47:53PM +0000, Kieran Bingham wrote: > > > > > > > > > > > > > > > > > > On 24/03/2023 15:42, Jacopo Mondi via libcamera-devel wrote: > > > > > > > Hi daniel > > > > > > > > > > > > > > On Fri, Mar 24, 2023 at 03:29:06PM +0100, 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 | 6 ++++++ > > > > > > > > 1 file changed, 6 insertions(+) > > > > > > > > > > > > > > > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > > > > > > > index cd1fbae3..4b30844f 100644 > > > > > > > > --- a/src/ipa/rkisp1/rkisp1.cpp > > > > > > > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > > > > > > > @@ -101,10 +101,16 @@ namespace { > > > > > > > > /* List of controls handled by the RkISP1 IPA */ > > > > > > > > const ControlInfoMap::Map rkisp1Controls{ > > > > > > > > { &controls::AeEnable, ControlInfo(false, true) }, > > > > > > > > + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, > > > > > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > > > > > > > > > > > AfPauseDeferred is not supported > > > > > > > > > > > > > > > > > > > What happens if there's no VCM though - Should we only expose the controls > > > > > > > > > > You're very right, I overlooked that. > > > > > > > > > > > if a VCM is identified? (Though that requires reworking the creation of this > > > > > > rkisp1Controls list to be handled by the algorithms) > > > > > > > > > > I'm not sure it needs to be done by algorithms, but the IPA should > > > > > certainly be made aware of the lens presence at init() time. > > > > > > > > This is indeed something that is needed, especially that number > > > > of algorithm implementations is increasing. The most appropriate > > > > solution seems for the algorithms to expose the supported control list > > > > or to populate the list. This would also fix the problem of checking > > > > outside the algorithm, if certain configuration is available, like > > > > lens. > > > > > > > > I see at least two problems with the current approach: > > > > 1. controls are exposed by the IPA, even if the algorithm is not > > > > enabled in the tuning file. This is confusing. > > > > > > Agreed > > > > > > > 2. What if there are, for example, two AF algorithm implementations, > > > > with different supported controls? Which controls should be added > > > > to the control list? > > > > > > Handling multiple algos for the same functions is not something I > > > think there's a design for at the moment. > > > > > > If we defer registering ControlInfo to the single algorithms I guess > > > the problem then reduces on how to run-time select which algorithm > > > implementation to use. > > > > I meant the current situation with ControlInfo map populated > > in the IPA instead in algorithms itself. As you said, this should not > > be a problem with the controls handled in the algorithms. > > > > > > > > > > > > > > > > > > > To be honest I would be fine with what RPi does in this regard. have a > > > > > look at the usage of lensPresent_ in their IPA module init() function. > > > > > This looks like the less invasive change. > > > > > > > > > > I presume this could even be made better by passing the lens controls' > > > > > from the PH to the IPA at init() time so that the AF ControlInfo > > > > > elements could be initialized with the proper limits and not with > > > > > default values as it happens here and on RPi. > > > > > > > > For me, moving it to init() is also a better solution. I would like to > > > > avoid intermediate steps and initialize IPA lens config in one place > > > > for clarity. This means the whole lens configuration that's currently > > > > done in the configure(), will need to be moved to init(). > > > > > > The part about the lens in configure() intializes > > > > > > lensConfig.minFocusPosition = focusAbsolute.min().get<int32_t>(); > > > lensConfig.maxFocusPosition = focusAbsolute.max().get<int32_t>(); > > > > > > As you said, the lens min/max position -shouldn't- change between > > > configurations so this part could be moved at init() time > > > > > > > However, I see one problem. context_.configuration struct is reset > > > > in configure() for some reason. Even that > > > > context_.configuration.sensor.lineDuration is set in init(). It is > > > > reset and set again. > > > > > > The other parameters (such as the gains and shutter speed) change > > > depending on the currently configured sensor's mode, that's why > > > they're re-calculated at each configure() call. > > > > > > > > > > > What about making the IPASessionConfiguration::lens structure > > > > std::optional, so it will be explicitly stated in the session config > > > > if the lens is available? Then, AF can fail in init() if lens > > > > are not available. > > > > > > > > > > Let's take a step back for a second. > > > > > > We have an updateControls() function, that starting for the static map > > > rkisp1Controls augments it with controls whose values are computed by > > > inspecting the sensorControls. > > > > > > Can't we do the same for the lens controls ? Add them only > > > conditionally to the validity of lensControls ? Then if we deem it > > > better we can pass to each algorithm the sensor's and lens > > > configuration and have them populate the list of controls they handle. > > > > > > > Right, this makes sense. I can rework the code like this. > > Also, don't you think it is worth to fail the AF init if it was > > enabled on camera without moveable lens? > > > > mmm, that would make the whole IPA initialization fail if I'm not > mistaken. The algorithm is enabled by the sensor configuration file, > which indeed shouldn't include it if the device doesn't have a lens. Yes, but I think it is better to fail early on startup, than trying to make sure there will be no side effects when someone will try to use the AF with wrong configuration. > > It also seems to me that if we don't fail, the AF algorithm > implementation would need to protect agains a missing lens with > > if (!context.configuration.lens) > return 0; > > in all its methods, as it will be part of the list of active > algorithms, and I'm not sure it's worth it. > > If you agree with the above, then yes, I guess we should fail in > init() if no lens is available. Yes, I agree and it is simpler to do one check in the Af::init() > > > > > > > > > This will work for lens, but I am afraid We will not be able to > > > > evaluate all AF ControlInfo elements in the init() phase. > > > > For example, AF window size limits depend on the sensor output size, > > > > which is only available in the configure(). > > > > > > updateControls() has access to sensorInfo which contains the sensor's > > > output size > > > > Ah, I didn't notice that... In such case this is not a problem :) > > > > > > > > > > > > > Best regards > > > > Daniel > > > > > > > > > > > > > > > > > > > > > -- > > > > > > Kieran > > > > > > > > > > > > > > > > > > > Otherwise > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, > > > > > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > > > > > + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, > > > > > > > > > > > > > > We should -really- move to instantiate control limits directly based > > > > > > > on the sensor configuration... > > > > > > > > > > > > > > > { &controls::AwbEnable, ControlInfo(false, true) }, > > > > > > > > { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, > > > > > > > > { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, > > > > > > > > { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > > > + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, > > > > > > > > { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, > > > > > > > > { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, > > > > > > > > { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) }, > > > > > > > > -- > > > > > > > > 2.39.2 > > > > > > > >
diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index cd1fbae3..4b30844f 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -101,10 +101,16 @@ namespace { /* List of controls handled by the RkISP1 IPA */ const ControlInfoMap::Map rkisp1Controls{ { &controls::AeEnable, ControlInfo(false, true) }, + { &controls::AfMetering, ControlInfo(controls::AfMeteringValues) }, + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, + { &controls::AfPause, ControlInfo(controls::AfPauseValues) }, + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, + { &controls::AfWindows, ControlInfo(Rectangle{}, Rectangle(65535, 65535, 65535, 65535), Rectangle{}) }, { &controls::AwbEnable, ControlInfo(false, true) }, { &controls::ColourGains, ControlInfo(0.0f, 3.996f, 1.0f) }, { &controls::Brightness, ControlInfo(-1.0f, 0.993f, 0.0f) }, { &controls::Contrast, ControlInfo(0.0f, 1.993f, 1.0f) }, + { &controls::LensPosition, ControlInfo(0.0f, 2147483647.0f) }, { &controls::Saturation, ControlInfo(0.0f, 1.993f, 1.0f) }, { &controls::Sharpness, ControlInfo(0.0f, 10.0f, 1.0f) }, { &controls::draft::NoiseReductionMode, ControlInfo(controls::draft::NoiseReductionModeValues) },
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 | 6 ++++++ 1 file changed, 6 insertions(+)