| Message ID | 20220713084317.24268-5-dse@thaumatec.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Daniel, On Wed, Jul 13, 2022 at 10:43:10AM +0200, Daniel Semkowicz via libcamera-devel wrote: > This will expose the AF controls and will allow controlling them using > the top level API. > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 7ee80192..99d66b1d 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > std::unique_ptr<RkISP1CameraData> data = > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > - ControlInfoMap::Map ctrls; > - ctrls.emplace(std::piecewise_construct, > - std::forward_as_tuple(&controls::AeEnable), > - std::forward_as_tuple(false, true)); > + ControlInfoMap::Map ctrls({ > + { &controls::AeEnable, ControlInfo(false, true) }, > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) } Does the algorithm support continuous auto-focus ? Has it been tested ? I've been told by those who tried implement it (RPi) that without PDAF statistics from the sensor it's rather hard to get it right. If the algorithm does not support it I would drop AfPause as it's only useful for CAF (and the associated functions should probably be removed along the class hierarchy as well maybe) > + }); > > data->controlInfo_ = ControlInfoMap(std::move(ctrls), > controls::controls); > -- > 2.34.1 >
Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:30:03) > Hi Daniel, > > On Wed, Jul 13, 2022 at 10:43:10AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > This will expose the AF controls and will allow controlling them using > > the top level API. > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 7ee80192..99d66b1d 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > std::unique_ptr<RkISP1CameraData> data = > > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > > > - ControlInfoMap::Map ctrls; > > - ctrls.emplace(std::piecewise_construct, > > - std::forward_as_tuple(&controls::AeEnable), > > - std::forward_as_tuple(false, true)); > > + ControlInfoMap::Map ctrls({ > > + { &controls::AeEnable, ControlInfo(false, true) }, > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) } > > Does the algorithm support continuous auto-focus ? Has it been tested ? > I've been told by those who tried implement it (RPi) that without PDAF > statistics from the sensor it's rather hard to get it right. > > If the algorithm does not support it I would drop AfPause as it's > only useful for CAF (and the associated functions should probably be > removed along the class hierarchy as well maybe) As this algorithm implementation is going into libipa, I wonder if we can get it (the common algorithm implementation) to report the controls it supports and merge that into the ControlList of supported controls exposed by the IPA? That would help towards keeping the support modular, and choosing a different algorithm implementation (perhaps at runtime) could then correctly reflect the supported controls? -- Kieran > > > + }); > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrls), > > controls::controls); > > -- > > 2.34.1 > >
Hello, On Thu, Jul 14, 2022 at 11:41:29PM +0100, Kieran Bingham via libcamera-devel wrote: > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:30:03) > > On Wed, Jul 13, 2022 at 10:43:10AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > This will expose the AF controls and will allow controlling them using > > > the top level API. > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 7ee80192..99d66b1d 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > > std::unique_ptr<RkISP1CameraData> data = > > > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > > > > > - ControlInfoMap::Map ctrls; > > > - ctrls.emplace(std::piecewise_construct, > > > - std::forward_as_tuple(&controls::AeEnable), > > > - std::forward_as_tuple(false, true)); > > > + ControlInfoMap::Map ctrls({ > > > + { &controls::AeEnable, ControlInfo(false, true) }, > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) } > > > > Does the algorithm support continuous auto-focus ? Has it been tested ? > > I've been told by those who tried implement it (RPi) that without PDAF > > statistics from the sensor it's rather hard to get it right. > > > > If the algorithm does not support it I would drop AfPause as it's > > only useful for CAF (and the associated functions should probably be > > removed along the class hierarchy as well maybe) > > As this algorithm implementation is going into libipa, I wonder if we > can get it (the common algorithm implementation) to report the controls > it supports and merge that into the ControlList of supported controls > exposed by the IPA? > > That would help towards keeping the support modular, and choosing a > different algorithm implementation (perhaps at runtime) could then > correctly reflect the supported controls? Yes, that would be better. The RPi IPA module has recently gone that route, see commit 53ada24e63f4 ("pipeline: ipa: raspberrypi: Move ControlInfoMap to the IPA"). > > > + }); > > > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrls), > > > controls::controls);
Hi, On Fri, Jul 15, 2022 at 2:44 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hello, > > On Thu, Jul 14, 2022 at 11:41:29PM +0100, Kieran Bingham via libcamera-devel wrote: > > Quoting Jacopo Mondi via libcamera-devel (2022-07-14 19:30:03) > > > On Wed, Jul 13, 2022 at 10:43:10AM +0200, Daniel Semkowicz via libcamera-devel wrote: > > > > This will expose the AF controls and will allow controlling them using > > > > the top level API. > > > > > > > > Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> > > > > --- > > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++---- > > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > index 7ee80192..99d66b1d 100644 > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > > @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > > > std::unique_ptr<RkISP1CameraData> data = > > > > std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); > > > > > > > > - ControlInfoMap::Map ctrls; > > > > - ctrls.emplace(std::piecewise_construct, > > > > - std::forward_as_tuple(&controls::AeEnable), > > > > - std::forward_as_tuple(false, true)); > > > > + ControlInfoMap::Map ctrls({ > > > > + { &controls::AeEnable, ControlInfo(false, true) }, > > > > + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, > > > > + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, > > > > + { &controls::AfPause, ControlInfo(controls::AfPauseValues) } > > > > > > Does the algorithm support continuous auto-focus ? Has it been tested ? > > > I've been told by those who tried implement it (RPi) that without PDAF > > > statistics from the sensor it's rather hard to get it right. > > > > > > If the algorithm does not support it I would drop AfPause as it's > > > only useful for CAF (and the associated functions should probably be > > > removed along the class hierarchy as well maybe) Algorithm supports continuous autofocus, but this part requires more improvement, as currently this works exactly the same as for triggered AF, just the trigger is automatically generated by detecting major change in contrast value. And for manual trigger lens is always set to the initial position. While for manual trigger this is not so big problem, for the continuous AF, you certainly would like to have smooth lens control. > > > > As this algorithm implementation is going into libipa, I wonder if we > > can get it (the common algorithm implementation) to report the controls > > it supports and merge that into the ControlList of supported controls > > exposed by the IPA? > > > > That would help towards keeping the support modular, and choosing a > > different algorithm implementation (perhaps at runtime) could then > > correctly reflect the supported controls? > > Yes, that would be better. The RPi IPA module has recently gone that > route, see commit 53ada24e63f4 ("pipeline: ipa: raspberrypi: Move > ControlInfoMap to the IPA"). > I have seen these changes and I also think this is a good idea. Best regards Daniel > > > > + }); > > > > > > > > data->controlInfo_ = ControlInfoMap(std::move(ctrls), > > > > controls::controls); > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 7ee80192..99d66b1d 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -958,10 +958,12 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) std::unique_ptr<RkISP1CameraData> data = std::make_unique<RkISP1CameraData>(this, &mainPath_, &selfPath_); - ControlInfoMap::Map ctrls; - ctrls.emplace(std::piecewise_construct, - std::forward_as_tuple(&controls::AeEnable), - std::forward_as_tuple(false, true)); + ControlInfoMap::Map ctrls({ + { &controls::AeEnable, ControlInfo(false, true) }, + { &controls::AfMode, ControlInfo(controls::AfModeValues) }, + { &controls::AfTrigger, ControlInfo(controls::AfTriggerValues) }, + { &controls::AfPause, ControlInfo(controls::AfPauseValues) } + }); data->controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);
This will expose the AF controls and will allow controlling them using the top level API. Signed-off-by: Daniel Semkowicz <dse@thaumatec.com> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)