Message ID | 20240701144122.3418955-3-stefan.klug@ideasonboard.com |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Stefan On Mon, Jul 01, 2024 at 04:38:25PM GMT, Stefan Klug wrote: > To be able to query the black levels, the black level correction > algorithm needs access to the camera sensor helper. Allow this by moving > the camHelper_ member from IPARkISP1 into IPAContext. In general, I think it's useful to have helpers in the context for algorithms to access it > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > --- > src/ipa/rkisp1/ipa_context.h | 4 ++++ > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++-------------- > 2 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 8602b408870e..d2dd6a904b59 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -16,6 +16,7 @@ > #include <libcamera/controls.h> > #include <libcamera/geometry.h> > > +#include <libipa/camera_sensor_helper.h> > #include <libipa/fc_queue.h> > #include <libipa/matrix.h> > > @@ -178,6 +179,9 @@ struct IPAContext { > FCQueue<IPAFrameContext> frameContexts; > > ControlInfoMap::Map ctrlMap; > + > + /* Interface to the Camera Helper */ > + std::unique_ptr<CameraSensorHelper> camHelper; > }; > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index d31cdbab020b..23e0826cc335 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -29,7 +29,6 @@ > #include "libcamera/internal/yaml_parser.h" > > #include "algorithms/algorithm.h" > -#include "libipa/camera_sensor_helper.h" > > #include "ipa_context.h" > > @@ -81,9 +80,6 @@ private: > > ControlInfoMap sensorControls_; > > - /* Interface to the Camera Helper */ > - std::unique_ptr<CameraSensorHelper> camHelper_; > - > /* Local parameter storage */ > struct IPAContext context_; > }; > @@ -115,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{ > } /* namespace */ > > IPARkISP1::IPARkISP1() > - : context_({ {}, {}, {}, { kMaxFrameContexts }, {} }) > + : context_({ {}, {}, {}, { kMaxFrameContexts }, {}, {} }) Do we need a constructor for ipa:rkisp1:IPAContext ? > { > } > > @@ -147,8 +143,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > > - camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > - if (!camHelper_) { > + context_.camHelper = CameraSensorHelperFactoryBase::create(settings.sensorModel); > + if (!context_.camHelper) { > LOG(IPARkISP1, Error) > << "Failed to create camera sensor helper for " > << settings.sensorModel; > @@ -250,8 +246,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > minExposure * context_.configuration.sensor.lineDuration; > context_.configuration.sensor.maxShutterSpeed = > maxExposure * context_.configuration.sensor.lineDuration; > - context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain); > - context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain); > + context_.configuration.sensor.minAnalogueGain = > + context_.camHelper->gain(minGain); > + context_.configuration.sensor.maxAnalogueGain = > + context_.camHelper->gain(maxGain); > > context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), > [](auto &cfg) -> bool { > @@ -352,7 +350,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > frameContext.sensor.exposure = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > frameContext.sensor.gain = > - camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > + context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > ControlList metadata(controls::controls); > > @@ -389,9 +387,9 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > /* Compute the analogue gain limits. */ > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > - float minGain = camHelper_->gain(v4l2Gain.min().get<int32_t>()); > - float maxGain = camHelper_->gain(v4l2Gain.max().get<int32_t>()); > - float defGain = camHelper_->gain(v4l2Gain.def().get<int32_t>()); > + float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); > + float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); > + float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); > ctrlMap.emplace(std::piecewise_construct, > std::forward_as_tuple(&controls::AnalogueGain), > std::forward_as_tuple(minGain, maxGain, defGain)); > @@ -436,7 +434,7 @@ void IPARkISP1::setControls(unsigned int frame) > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > uint32_t exposure = frameContext.agc.exposure; > - uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); > + uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > ControlList ctrls(sensorControls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > -- > 2.43.0 >
On Tue, Jul 02, 2024 at 09:55:45AM +0200, Jacopo Mondi wrote: > Hi Stefan > > On Mon, Jul 01, 2024 at 04:38:25PM GMT, Stefan Klug wrote: > > To be able to query the black levels, the black level correction > > algorithm needs access to the camera sensor helper. Allow this by moving > > the camHelper_ member from IPARkISP1 into IPAContext. > > In general, I think it's useful to have helpers in the context for > algorithms to access it I think so too. > > > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> > > --- > > src/ipa/rkisp1/ipa_context.h | 4 ++++ > > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++-------------- > > 2 files changed, 16 insertions(+), 14 deletions(-) > > > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > > index 8602b408870e..d2dd6a904b59 100644 > > --- a/src/ipa/rkisp1/ipa_context.h > > +++ b/src/ipa/rkisp1/ipa_context.h > > @@ -16,6 +16,7 @@ > > #include <libcamera/controls.h> > > #include <libcamera/geometry.h> > > > > +#include <libipa/camera_sensor_helper.h> > > #include <libipa/fc_queue.h> > > #include <libipa/matrix.h> > > > > @@ -178,6 +179,9 @@ struct IPAContext { > > FCQueue<IPAFrameContext> frameContexts; > > > > ControlInfoMap::Map ctrlMap; > > + > > + /* Interface to the Camera Helper */ > > + std::unique_ptr<CameraSensorHelper> camHelper; #include <memory> > > }; > > > > } /* namespace ipa::rkisp1 */ > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index d31cdbab020b..23e0826cc335 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -29,7 +29,6 @@ > > #include "libcamera/internal/yaml_parser.h" > > > > #include "algorithms/algorithm.h" > > -#include "libipa/camera_sensor_helper.h" > > > > #include "ipa_context.h" > > > > @@ -81,9 +80,6 @@ private: > > > > ControlInfoMap sensorControls_; > > > > - /* Interface to the Camera Helper */ > > - std::unique_ptr<CameraSensorHelper> camHelper_; > > - > > /* Local parameter storage */ > > struct IPAContext context_; > > }; > > @@ -115,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{ > > } /* namespace */ > > > > IPARkISP1::IPARkISP1() > > - : context_({ {}, {}, {}, { kMaxFrameContexts }, {} }) > > + : context_({ {}, {}, {}, { kMaxFrameContexts }, {}, {} }) > > Do we need a constructor for ipa:rkisp1:IPAContext ? Eventually, when we'll rework the context implementation, this is something we should improve. > > { > > } > > > > @@ -147,8 +143,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > > > LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > > > > - camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > > - if (!camHelper_) { > > + context_.camHelper = CameraSensorHelperFactoryBase::create(settings.sensorModel); > > + if (!context_.camHelper) { > > LOG(IPARkISP1, Error) > > << "Failed to create camera sensor helper for " > > << settings.sensorModel; > > @@ -250,8 +246,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > > minExposure * context_.configuration.sensor.lineDuration; > > context_.configuration.sensor.maxShutterSpeed = > > maxExposure * context_.configuration.sensor.lineDuration; > > - context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain); > > - context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain); > > + context_.configuration.sensor.minAnalogueGain = > > + context_.camHelper->gain(minGain); > > + context_.configuration.sensor.maxAnalogueGain = > > + context_.camHelper->gain(maxGain); > > > > context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), > > [](auto &cfg) -> bool { > > @@ -352,7 +350,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > > frameContext.sensor.exposure = > > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > > frameContext.sensor.gain = > > - camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > + context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > > > ControlList metadata(controls::controls); > > > > @@ -389,9 +387,9 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > > > /* Compute the analogue gain limits. */ > > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > > - float minGain = camHelper_->gain(v4l2Gain.min().get<int32_t>()); > > - float maxGain = camHelper_->gain(v4l2Gain.max().get<int32_t>()); > > - float defGain = camHelper_->gain(v4l2Gain.def().get<int32_t>()); > > + float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); > > + float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); > > + float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); > > ctrlMap.emplace(std::piecewise_construct, > > std::forward_as_tuple(&controls::AnalogueGain), > > std::forward_as_tuple(minGain, maxGain, defGain)); > > @@ -436,7 +434,7 @@ void IPARkISP1::setControls(unsigned int frame) > > > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > > uint32_t exposure = frameContext.agc.exposure; > > - uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); > > + uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > > > ControlList ctrls(sensorControls_); > > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
On Mon, Jul 01, 2024 at 04:38:25PM +0200, Stefan Klug wrote: > To be able to query the black levels, the black level correction > algorithm needs access to the camera sensor helper. Allow this by moving > the camHelper_ member from IPARkISP1 into IPAContext. > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com> > --- > src/ipa/rkisp1/ipa_context.h | 4 ++++ > src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++-------------- > 2 files changed, 16 insertions(+), 14 deletions(-) > > diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h > index 8602b408870e..d2dd6a904b59 100644 > --- a/src/ipa/rkisp1/ipa_context.h > +++ b/src/ipa/rkisp1/ipa_context.h > @@ -16,6 +16,7 @@ > #include <libcamera/controls.h> > #include <libcamera/geometry.h> > > +#include <libipa/camera_sensor_helper.h> > #include <libipa/fc_queue.h> > #include <libipa/matrix.h> > > @@ -178,6 +179,9 @@ struct IPAContext { > FCQueue<IPAFrameContext> frameContexts; > > ControlInfoMap::Map ctrlMap; > + > + /* Interface to the Camera Helper */ > + std::unique_ptr<CameraSensorHelper> camHelper; > }; > > } /* namespace ipa::rkisp1 */ > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index d31cdbab020b..23e0826cc335 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -29,7 +29,6 @@ > #include "libcamera/internal/yaml_parser.h" > > #include "algorithms/algorithm.h" > -#include "libipa/camera_sensor_helper.h" > > #include "ipa_context.h" > > @@ -81,9 +80,6 @@ private: > > ControlInfoMap sensorControls_; > > - /* Interface to the Camera Helper */ > - std::unique_ptr<CameraSensorHelper> camHelper_; > - > /* Local parameter storage */ > struct IPAContext context_; > }; > @@ -115,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{ > } /* namespace */ > > IPARkISP1::IPARkISP1() > - : context_({ {}, {}, {}, { kMaxFrameContexts }, {} }) > + : context_({ {}, {}, {}, { kMaxFrameContexts }, {}, {} }) > { > } > > @@ -147,8 +143,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, > > LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; > > - camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > - if (!camHelper_) { > + context_.camHelper = CameraSensorHelperFactoryBase::create(settings.sensorModel); > + if (!context_.camHelper) { > LOG(IPARkISP1, Error) > << "Failed to create camera sensor helper for " > << settings.sensorModel; > @@ -250,8 +246,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, > minExposure * context_.configuration.sensor.lineDuration; > context_.configuration.sensor.maxShutterSpeed = > maxExposure * context_.configuration.sensor.lineDuration; > - context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain); > - context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain); > + context_.configuration.sensor.minAnalogueGain = > + context_.camHelper->gain(minGain); > + context_.configuration.sensor.maxAnalogueGain = > + context_.camHelper->gain(maxGain); > > context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), > [](auto &cfg) -> bool { > @@ -352,7 +350,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId > frameContext.sensor.exposure = > sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); > frameContext.sensor.gain = > - camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > + context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); > > ControlList metadata(controls::controls); > > @@ -389,9 +387,9 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, > > /* Compute the analogue gain limits. */ > const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; > - float minGain = camHelper_->gain(v4l2Gain.min().get<int32_t>()); > - float maxGain = camHelper_->gain(v4l2Gain.max().get<int32_t>()); > - float defGain = camHelper_->gain(v4l2Gain.def().get<int32_t>()); > + float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); > + float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); > + float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); > ctrlMap.emplace(std::piecewise_construct, > std::forward_as_tuple(&controls::AnalogueGain), > std::forward_as_tuple(minGain, maxGain, defGain)); > @@ -436,7 +434,7 @@ void IPARkISP1::setControls(unsigned int frame) > > IPAFrameContext &frameContext = context_.frameContexts.get(frame); > uint32_t exposure = frameContext.agc.exposure; > - uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); > + uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); > > ControlList ctrls(sensorControls_); > ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure)); > -- > 2.43.0 >
diff --git a/src/ipa/rkisp1/ipa_context.h b/src/ipa/rkisp1/ipa_context.h index 8602b408870e..d2dd6a904b59 100644 --- a/src/ipa/rkisp1/ipa_context.h +++ b/src/ipa/rkisp1/ipa_context.h @@ -16,6 +16,7 @@ #include <libcamera/controls.h> #include <libcamera/geometry.h> +#include <libipa/camera_sensor_helper.h> #include <libipa/fc_queue.h> #include <libipa/matrix.h> @@ -178,6 +179,9 @@ struct IPAContext { FCQueue<IPAFrameContext> frameContexts; ControlInfoMap::Map ctrlMap; + + /* Interface to the Camera Helper */ + std::unique_ptr<CameraSensorHelper> camHelper; }; } /* namespace ipa::rkisp1 */ diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index d31cdbab020b..23e0826cc335 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -29,7 +29,6 @@ #include "libcamera/internal/yaml_parser.h" #include "algorithms/algorithm.h" -#include "libipa/camera_sensor_helper.h" #include "ipa_context.h" @@ -81,9 +80,6 @@ private: ControlInfoMap sensorControls_; - /* Interface to the Camera Helper */ - std::unique_ptr<CameraSensorHelper> camHelper_; - /* Local parameter storage */ struct IPAContext context_; }; @@ -115,7 +111,7 @@ const ControlInfoMap::Map rkisp1Controls{ } /* namespace */ IPARkISP1::IPARkISP1() - : context_({ {}, {}, {}, { kMaxFrameContexts }, {} }) + : context_({ {}, {}, {}, { kMaxFrameContexts }, {}, {} }) { } @@ -147,8 +143,8 @@ int IPARkISP1::init(const IPASettings &settings, unsigned int hwRevision, LOG(IPARkISP1, Debug) << "Hardware revision is " << hwRevision; - camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); - if (!camHelper_) { + context_.camHelper = CameraSensorHelperFactoryBase::create(settings.sensorModel); + if (!context_.camHelper) { LOG(IPARkISP1, Error) << "Failed to create camera sensor helper for " << settings.sensorModel; @@ -250,8 +246,10 @@ int IPARkISP1::configure(const IPAConfigInfo &ipaConfig, minExposure * context_.configuration.sensor.lineDuration; context_.configuration.sensor.maxShutterSpeed = maxExposure * context_.configuration.sensor.lineDuration; - context_.configuration.sensor.minAnalogueGain = camHelper_->gain(minGain); - context_.configuration.sensor.maxAnalogueGain = camHelper_->gain(maxGain); + context_.configuration.sensor.minAnalogueGain = + context_.camHelper->gain(minGain); + context_.configuration.sensor.maxAnalogueGain = + context_.camHelper->gain(maxGain); context_.configuration.raw = std::any_of(streamConfig.begin(), streamConfig.end(), [](auto &cfg) -> bool { @@ -352,7 +350,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId frameContext.sensor.exposure = sensorControls.get(V4L2_CID_EXPOSURE).get<int32_t>(); frameContext.sensor.gain = - camHelper_->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); + context_.camHelper->gain(sensorControls.get(V4L2_CID_ANALOGUE_GAIN).get<int32_t>()); ControlList metadata(controls::controls); @@ -389,9 +387,9 @@ void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo, /* Compute the analogue gain limits. */ const ControlInfo &v4l2Gain = sensorControls.find(V4L2_CID_ANALOGUE_GAIN)->second; - float minGain = camHelper_->gain(v4l2Gain.min().get<int32_t>()); - float maxGain = camHelper_->gain(v4l2Gain.max().get<int32_t>()); - float defGain = camHelper_->gain(v4l2Gain.def().get<int32_t>()); + float minGain = context_.camHelper->gain(v4l2Gain.min().get<int32_t>()); + float maxGain = context_.camHelper->gain(v4l2Gain.max().get<int32_t>()); + float defGain = context_.camHelper->gain(v4l2Gain.def().get<int32_t>()); ctrlMap.emplace(std::piecewise_construct, std::forward_as_tuple(&controls::AnalogueGain), std::forward_as_tuple(minGain, maxGain, defGain)); @@ -436,7 +434,7 @@ void IPARkISP1::setControls(unsigned int frame) IPAFrameContext &frameContext = context_.frameContexts.get(frame); uint32_t exposure = frameContext.agc.exposure; - uint32_t gain = camHelper_->gainCode(frameContext.agc.gain); + uint32_t gain = context_.camHelper->gainCode(frameContext.agc.gain); ControlList ctrls(sensorControls_); ctrls.set(V4L2_CID_EXPOSURE, static_cast<int32_t>(exposure));
To be able to query the black levels, the black level correction algorithm needs access to the camera sensor helper. Allow this by moving the camHelper_ member from IPARkISP1 into IPAContext. Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com> --- src/ipa/rkisp1/ipa_context.h | 4 ++++ src/ipa/rkisp1/rkisp1.cpp | 26 ++++++++++++-------------- 2 files changed, 16 insertions(+), 14 deletions(-)