| Message ID | 20241009193318.2394762-3-mzamazal@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Quoting Milan Zamazal (2024-10-09 20:33:17) > This patch introduces support for applying runtime controls to software > ISP. It enables the contrast algorithm as the first control that can be > used. > > Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > --- > .../libcamera/internal/software_isp/software_isp.h | 3 ++- > include/libcamera/ipa/soft.mojom | 2 +- > src/ipa/simple/algorithms/contrast.cpp | 7 +++++++ > src/ipa/simple/algorithms/contrast.h | 2 ++ > src/ipa/simple/data/uncalibrated.yaml | 1 + > src/ipa/simple/ipa_context.h | 1 + > src/ipa/simple/soft_simple.cpp | 11 ++++++++--- > src/libcamera/pipeline/simple/simple.cpp | 2 +- > src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- > 9 files changed, 29 insertions(+), 8 deletions(-) > > diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > index a3e3a9da4..d51b03fd6 100644 > --- a/include/libcamera/internal/software_isp/software_isp.h > +++ b/include/libcamera/internal/software_isp/software_isp.h > @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp) > class SoftwareIsp > { > public: > - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor); > + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > + ControlInfoMap *ipaControls); > ~SoftwareIsp(); > > int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } > diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom > index 347fd69b4..16e6a7071 100644 > --- a/include/libcamera/ipa/soft.mojom > +++ b/include/libcamera/ipa/soft.mojom > @@ -17,7 +17,7 @@ interface IPASoftInterface { > libcamera.SharedFD fdStats, > libcamera.SharedFD fdParams, > libcamera.ControlInfoMap sensorCtrlInfoMap) > - => (int32 ret); > + => (int32 ret, libcamera.ControlInfoMap ipaControls); > start() => (int32 ret); > stop(); > configure(IPAConfigInfo configInfo) > diff --git a/src/ipa/simple/algorithms/contrast.cpp b/src/ipa/simple/algorithms/contrast.cpp > index 1a2c14cf9..8d9997d81 100644 > --- a/src/ipa/simple/algorithms/contrast.cpp > +++ b/src/ipa/simple/algorithms/contrast.cpp > @@ -19,6 +19,13 @@ LOG_DEFINE_CATEGORY(IPASoftContrast) > > namespace ipa::soft::algorithms { > > +int Contrast::init(IPAContext &context, > + [[maybe_unused]] const YamlObject &tuningData) > +{ > + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 10.0f, 1.0f); > + return 0; > +} > + > int Contrast::configure(typename Module::Context &context, > [[maybe_unused]] const typename Module::Config &configInfo) > { > diff --git a/src/ipa/simple/algorithms/contrast.h b/src/ipa/simple/algorithms/contrast.h > index 0b3933099..405345acc 100644 > --- a/src/ipa/simple/algorithms/contrast.h > +++ b/src/ipa/simple/algorithms/contrast.h > @@ -21,6 +21,8 @@ public: > Contrast() = default; > ~Contrast() = default; > > + int init(IPAContext &context, const YamlObject &tuningData) override; > + > int configure(typename Module::Context &context, > const typename Module::Config &configInfo) > override; > diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml > index 3f1471121..c091e30fd 100644 > --- a/src/ipa/simple/data/uncalibrated.yaml > +++ b/src/ipa/simple/data/uncalibrated.yaml > @@ -5,6 +5,7 @@ version: 1 > algorithms: > - BlackLevel: > - Awb: > + - Contrast: > - Lut: > - Agc: > ... > diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > index 5118d6abf..095c83800 100644 > --- a/src/ipa/simple/ipa_context.h > +++ b/src/ipa/simple/ipa_context.h > @@ -65,6 +65,7 @@ struct IPAContext { > IPASessionConfiguration configuration; > IPAActiveState activeState; > FCQueue<IPAFrameContext> frameContexts; > + ControlInfoMap::Map ctrlMap; > }; > > } /* namespace ipa::soft */ > diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > index b28c7039f..eb027d754 100644 > --- a/src/ipa/simple/soft_simple.cpp > +++ b/src/ipa/simple/soft_simple.cpp > @@ -41,7 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module > { > public: > IPASoftSimple() > - : context_({ {}, {}, { kMaxFrameContexts } }) > + : context_({ {}, {}, { kMaxFrameContexts }, {} }) Could you instead, preceed this patch with an equivalent of https://patchwork.libcamera.org/patch/21542/ for the simple pipeline handler IPA? Bonus points if you convert others too. > { > } > > @@ -50,7 +50,8 @@ public: > int init(const IPASettings &settings, > const SharedFD &fdStats, > const SharedFD &fdParams, > - const ControlInfoMap &sensorInfoMap) override; > + const ControlInfoMap &sensorInfoMap, > + ControlInfoMap *ipaControls) override; > int configure(const IPAConfigInfo &configInfo) override; > > int start() override; > @@ -87,7 +88,8 @@ IPASoftSimple::~IPASoftSimple() > int IPASoftSimple::init(const IPASettings &settings, > const SharedFD &fdStats, > const SharedFD &fdParams, > - const ControlInfoMap &sensorInfoMap) > + const ControlInfoMap &sensorInfoMap, > + ControlInfoMap *ipaControls) > { > camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > if (!camHelper_) { > @@ -158,6 +160,9 @@ int IPASoftSimple::init(const IPASettings &settings, > stats_ = static_cast<SwIspStats *>(mem); > } > > + ControlInfoMap::Map ctrlMap = context_.ctrlMap; > + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); Am I correct in reading that this is taking a copy of the ctrlMap first so that we can then construct the ControlInfoMap from it, without destroying or losing the context_.ctrlMap? I think that's a good thing to do - but makes me think we should have a ControlInfoMap(const &ControlInfoMap::Map, ...) version that wouldn't move and would make a copy internally. Would *ipaControls = ControlInfoMap(std::copy(context_ctrlMap), controls::controls); have been equivalent / expressive in the same way ? But I think that's just implementation detail so: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > + > /* > * Check if the sensor driver supports the controls required by the > * Soft IPA. > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > index 3ddce71d3..38868df66 100644 > --- a/src/libcamera/pipeline/simple/simple.cpp > +++ b/src/libcamera/pipeline/simple/simple.cpp > @@ -530,7 +530,7 @@ int SimpleCameraData::init() > * Instantiate Soft ISP if this is enabled for the given driver and no converter is used. > */ > if (!converter_ && pipe->swIspEnabled()) { > - swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get()); > + swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get(), &controlInfo_); > if (!swIsp_->isValid()) { > LOG(SimplePipeline, Warning) > << "Failed to create software ISP, disabling software debayering"; > diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > index 47677784d..3d3f956cb 100644 > --- a/src/libcamera/software_isp/software_isp.cpp > +++ b/src/libcamera/software_isp/software_isp.cpp > @@ -13,6 +13,7 @@ > #include <sys/types.h> > #include <unistd.h> > > +#include <libcamera/controls.h> > #include <libcamera/formats.h> > #include <libcamera/stream.h> > > @@ -60,9 +61,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > * \brief Constructs SoftwareIsp object > * \param[in] pipe The pipeline handler in use > * \param[in] sensor Pointer to the CameraSensor instance owned by the pipeline > + * \param[out] ipaControls The IPA controls to update > * handler > */ > -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) > +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > + ControlInfoMap *ipaControls) > : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > @@ -124,7 +127,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) > int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, > debayer_->getStatsFD(), > sharedParams_.fd(), > - sensor->controls()); > + sensor->controls(), > + ipaControls); > if (ret) { > LOG(SoftwareIsp, Error) << "IPA init failed"; > debayer_.reset(); > -- > 2.44.1 >
Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > Quoting Milan Zamazal (2024-10-09 20:33:17) >> This patch introduces support for applying runtime controls to software >> ISP. It enables the contrast algorithm as the first control that can be > >> used. >> >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> >> --- >> .../libcamera/internal/software_isp/software_isp.h | 3 ++- >> include/libcamera/ipa/soft.mojom | 2 +- >> src/ipa/simple/algorithms/contrast.cpp | 7 +++++++ >> src/ipa/simple/algorithms/contrast.h | 2 ++ >> src/ipa/simple/data/uncalibrated.yaml | 1 + >> src/ipa/simple/ipa_context.h | 1 + >> src/ipa/simple/soft_simple.cpp | 11 ++++++++--- >> src/libcamera/pipeline/simple/simple.cpp | 2 +- >> src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- >> 9 files changed, 29 insertions(+), 8 deletions(-) >> >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h >> index a3e3a9da4..d51b03fd6 100644 >> --- a/include/libcamera/internal/software_isp/software_isp.h >> +++ b/include/libcamera/internal/software_isp/software_isp.h >> @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp) >> class SoftwareIsp >> { >> public: >> - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor); >> + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> + ControlInfoMap *ipaControls); >> ~SoftwareIsp(); >> >> int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom >> index 347fd69b4..16e6a7071 100644 >> --- a/include/libcamera/ipa/soft.mojom >> +++ b/include/libcamera/ipa/soft.mojom >> @@ -17,7 +17,7 @@ interface IPASoftInterface { >> libcamera.SharedFD fdStats, >> libcamera.SharedFD fdParams, >> libcamera.ControlInfoMap sensorCtrlInfoMap) >> - => (int32 ret); >> + => (int32 ret, libcamera.ControlInfoMap ipaControls); >> start() => (int32 ret); >> stop(); >> configure(IPAConfigInfo configInfo) >> diff --git a/src/ipa/simple/algorithms/contrast.cpp b/src/ipa/simple/algorithms/contrast.cpp >> index 1a2c14cf9..8d9997d81 100644 >> --- a/src/ipa/simple/algorithms/contrast.cpp >> +++ b/src/ipa/simple/algorithms/contrast.cpp >> @@ -19,6 +19,13 @@ LOG_DEFINE_CATEGORY(IPASoftContrast) >> >> namespace ipa::soft::algorithms { >> >> +int Contrast::init(IPAContext &context, >> + [[maybe_unused]] const YamlObject &tuningData) >> +{ >> + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 10.0f, 1.0f); >> + return 0; >> +} >> + >> int Contrast::configure(typename Module::Context &context, >> [[maybe_unused]] const typename Module::Config &configInfo) >> { >> diff --git a/src/ipa/simple/algorithms/contrast.h b/src/ipa/simple/algorithms/contrast.h >> index 0b3933099..405345acc 100644 >> --- a/src/ipa/simple/algorithms/contrast.h >> +++ b/src/ipa/simple/algorithms/contrast.h >> @@ -21,6 +21,8 @@ public: >> Contrast() = default; >> ~Contrast() = default; >> >> + int init(IPAContext &context, const YamlObject &tuningData) override; >> + >> int configure(typename Module::Context &context, >> const typename Module::Config &configInfo) >> override; >> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml >> index 3f1471121..c091e30fd 100644 >> --- a/src/ipa/simple/data/uncalibrated.yaml >> +++ b/src/ipa/simple/data/uncalibrated.yaml >> @@ -5,6 +5,7 @@ version: 1 >> algorithms: >> - BlackLevel: >> - Awb: >> + - Contrast: >> - Lut: >> - Agc: >> ... >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h >> index 5118d6abf..095c83800 100644 >> --- a/src/ipa/simple/ipa_context.h >> +++ b/src/ipa/simple/ipa_context.h >> @@ -65,6 +65,7 @@ struct IPAContext { >> IPASessionConfiguration configuration; >> IPAActiveState activeState; >> FCQueue<IPAFrameContext> frameContexts; >> + ControlInfoMap::Map ctrlMap; >> }; >> >> } /* namespace ipa::soft */ >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp >> index b28c7039f..eb027d754 100644 >> --- a/src/ipa/simple/soft_simple.cpp >> +++ b/src/ipa/simple/soft_simple.cpp >> @@ -41,7 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module >> { >> public: >> IPASoftSimple() >> - : context_({ {}, {}, { kMaxFrameContexts } }) >> + : context_({ {}, {}, { kMaxFrameContexts }, {} }) > > Could you instead, preceed this patch with an equivalent of > https://patchwork.libcamera.org/patch/21542/ for the simple pipeline > handler IPA? Yes, good idea. > Bonus points if you convert others too. OK. :-) >> { >> } >> >> @@ -50,7 +50,8 @@ public: >> int init(const IPASettings &settings, >> const SharedFD &fdStats, >> const SharedFD &fdParams, >> - const ControlInfoMap &sensorInfoMap) override; >> + const ControlInfoMap &sensorInfoMap, >> + ControlInfoMap *ipaControls) override; >> int configure(const IPAConfigInfo &configInfo) override; >> >> int start() override; >> @@ -87,7 +88,8 @@ IPASoftSimple::~IPASoftSimple() >> int IPASoftSimple::init(const IPASettings &settings, >> const SharedFD &fdStats, >> const SharedFD &fdParams, >> - const ControlInfoMap &sensorInfoMap) >> + const ControlInfoMap &sensorInfoMap, >> + ControlInfoMap *ipaControls) >> { >> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); >> if (!camHelper_) { >> @@ -158,6 +160,9 @@ int IPASoftSimple::init(const IPASettings &settings, >> stats_ = static_cast<SwIspStats *>(mem); >> } >> >> + ControlInfoMap::Map ctrlMap = context_.ctrlMap; >> + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > Am I correct in reading that this is taking a copy of the ctrlMap first > so that we can then construct the ControlInfoMap from it, without > destroying or losing the context_.ctrlMap? Yes. > I think that's a good thing to do - but makes me think we should have a > ControlInfoMap(const &ControlInfoMap::Map, ...) version that wouldn't > move and would make a copy internally. > > > Would > *ipaControls = ControlInfoMap(std::copy(context_ctrlMap), > controls::controls); > > have been equivalent / expressive in the same way ? I don't think there is a single-argument std::copy but *ipaControls = ControlInfoMap(ControlInfoMap::Map(context_.ctrlMap), controls::controls); should work. The only question is whether the single line is better than the two original lines. > But I think that's just implementation detail so: > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > >> + >> /* >> * Check if the sensor driver supports the controls required by the >> * Soft IPA. >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp >> index 3ddce71d3..38868df66 100644 >> --- a/src/libcamera/pipeline/simple/simple.cpp >> +++ b/src/libcamera/pipeline/simple/simple.cpp >> @@ -530,7 +530,7 @@ int SimpleCameraData::init() >> * Instantiate Soft ISP if this is enabled for the given driver and no converter is used. >> */ >> if (!converter_ && pipe->swIspEnabled()) { >> - swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get()); >> + swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get(), &controlInfo_); >> if (!swIsp_->isValid()) { >> LOG(SimplePipeline, Warning) >> << "Failed to create software ISP, disabling software debayering"; >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp >> index 47677784d..3d3f956cb 100644 >> --- a/src/libcamera/software_isp/software_isp.cpp >> +++ b/src/libcamera/software_isp/software_isp.cpp >> @@ -13,6 +13,7 @@ >> #include <sys/types.h> >> #include <unistd.h> >> >> +#include <libcamera/controls.h> >> #include <libcamera/formats.h> >> #include <libcamera/stream.h> >> >> @@ -60,9 +61,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) >> * \brief Constructs SoftwareIsp object >> * \param[in] pipe The pipeline handler in use >> * \param[in] sensor Pointer to the CameraSensor instance owned by the pipeline >> + * \param[out] ipaControls The IPA controls to update >> * handler >> */ >> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) >> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, >> + ControlInfoMap *ipaControls) >> : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | >> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | >> DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) >> @@ -124,7 +127,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) >> int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, >> debayer_->getStatsFD(), >> sharedParams_.fd(), >> - sensor->controls()); >> + sensor->controls(), >> + ipaControls); >> if (ret) { >> LOG(SoftwareIsp, Error) << "IPA init failed"; >> debayer_.reset(); >> -- >> 2.44.1 >>
Quoting Milan Zamazal (2024-10-10 10:37:41) > Kieran Bingham <kieran.bingham@ideasonboard.com> writes: > > > Quoting Milan Zamazal (2024-10-09 20:33:17) > >> This patch introduces support for applying runtime controls to software > >> ISP. It enables the contrast algorithm as the first control that can be > > > >> used. > >> > >> Signed-off-by: Milan Zamazal <mzamazal@redhat.com> > >> --- > >> .../libcamera/internal/software_isp/software_isp.h | 3 ++- > >> include/libcamera/ipa/soft.mojom | 2 +- > >> src/ipa/simple/algorithms/contrast.cpp | 7 +++++++ > >> src/ipa/simple/algorithms/contrast.h | 2 ++ > >> src/ipa/simple/data/uncalibrated.yaml | 1 + > >> src/ipa/simple/ipa_context.h | 1 + > >> src/ipa/simple/soft_simple.cpp | 11 ++++++++--- > >> src/libcamera/pipeline/simple/simple.cpp | 2 +- > >> src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- > >> 9 files changed, 29 insertions(+), 8 deletions(-) > >> > >> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h > >> index a3e3a9da4..d51b03fd6 100644 > >> --- a/include/libcamera/internal/software_isp/software_isp.h > >> +++ b/include/libcamera/internal/software_isp/software_isp.h > >> @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp) > >> class SoftwareIsp > >> { > >> public: > >> - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor); > >> + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > >> + ControlInfoMap *ipaControls); > >> ~SoftwareIsp(); > >> > >> int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } > >> diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom > >> index 347fd69b4..16e6a7071 100644 > >> --- a/include/libcamera/ipa/soft.mojom > >> +++ b/include/libcamera/ipa/soft.mojom > >> @@ -17,7 +17,7 @@ interface IPASoftInterface { > >> libcamera.SharedFD fdStats, > >> libcamera.SharedFD fdParams, > >> libcamera.ControlInfoMap sensorCtrlInfoMap) > >> - => (int32 ret); > >> + => (int32 ret, libcamera.ControlInfoMap ipaControls); > >> start() => (int32 ret); > >> stop(); > >> configure(IPAConfigInfo configInfo) > >> diff --git a/src/ipa/simple/algorithms/contrast.cpp b/src/ipa/simple/algorithms/contrast.cpp > >> index 1a2c14cf9..8d9997d81 100644 > >> --- a/src/ipa/simple/algorithms/contrast.cpp > >> +++ b/src/ipa/simple/algorithms/contrast.cpp > >> @@ -19,6 +19,13 @@ LOG_DEFINE_CATEGORY(IPASoftContrast) > >> > >> namespace ipa::soft::algorithms { > >> > >> +int Contrast::init(IPAContext &context, > >> + [[maybe_unused]] const YamlObject &tuningData) > >> +{ > >> + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 10.0f, 1.0f); > >> + return 0; > >> +} > >> + > >> int Contrast::configure(typename Module::Context &context, > >> [[maybe_unused]] const typename Module::Config &configInfo) > >> { > >> diff --git a/src/ipa/simple/algorithms/contrast.h b/src/ipa/simple/algorithms/contrast.h > >> index 0b3933099..405345acc 100644 > >> --- a/src/ipa/simple/algorithms/contrast.h > >> +++ b/src/ipa/simple/algorithms/contrast.h > >> @@ -21,6 +21,8 @@ public: > >> Contrast() = default; > >> ~Contrast() = default; > >> > >> + int init(IPAContext &context, const YamlObject &tuningData) override; > >> + > >> int configure(typename Module::Context &context, > >> const typename Module::Config &configInfo) > >> override; > >> diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml > >> index 3f1471121..c091e30fd 100644 > >> --- a/src/ipa/simple/data/uncalibrated.yaml > >> +++ b/src/ipa/simple/data/uncalibrated.yaml > >> @@ -5,6 +5,7 @@ version: 1 > >> algorithms: > >> - BlackLevel: > >> - Awb: > >> + - Contrast: > >> - Lut: > >> - Agc: > >> ... > >> diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h > >> index 5118d6abf..095c83800 100644 > >> --- a/src/ipa/simple/ipa_context.h > >> +++ b/src/ipa/simple/ipa_context.h > >> @@ -65,6 +65,7 @@ struct IPAContext { > >> IPASessionConfiguration configuration; > >> IPAActiveState activeState; > >> FCQueue<IPAFrameContext> frameContexts; > >> + ControlInfoMap::Map ctrlMap; > >> }; > >> > >> } /* namespace ipa::soft */ > >> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp > >> index b28c7039f..eb027d754 100644 > >> --- a/src/ipa/simple/soft_simple.cpp > >> +++ b/src/ipa/simple/soft_simple.cpp > >> @@ -41,7 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module > >> { > >> public: > >> IPASoftSimple() > >> - : context_({ {}, {}, { kMaxFrameContexts } }) > >> + : context_({ {}, {}, { kMaxFrameContexts }, {} }) > > > > Could you instead, preceed this patch with an equivalent of > > https://patchwork.libcamera.org/patch/21542/ for the simple pipeline > > handler IPA? > > Yes, good idea. > > > Bonus points if you convert others too. > > OK. :-) > > >> { > >> } > >> > >> @@ -50,7 +50,8 @@ public: > >> int init(const IPASettings &settings, > >> const SharedFD &fdStats, > >> const SharedFD &fdParams, > >> - const ControlInfoMap &sensorInfoMap) override; > >> + const ControlInfoMap &sensorInfoMap, > >> + ControlInfoMap *ipaControls) override; > >> int configure(const IPAConfigInfo &configInfo) override; > >> > >> int start() override; > >> @@ -87,7 +88,8 @@ IPASoftSimple::~IPASoftSimple() > >> int IPASoftSimple::init(const IPASettings &settings, > >> const SharedFD &fdStats, > >> const SharedFD &fdParams, > >> - const ControlInfoMap &sensorInfoMap) > >> + const ControlInfoMap &sensorInfoMap, > >> + ControlInfoMap *ipaControls) > >> { > >> camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); > >> if (!camHelper_) { > >> @@ -158,6 +160,9 @@ int IPASoftSimple::init(const IPASettings &settings, > >> stats_ = static_cast<SwIspStats *>(mem); > >> } > >> > >> + ControlInfoMap::Map ctrlMap = context_.ctrlMap; > >> + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); > > > > Am I correct in reading that this is taking a copy of the ctrlMap first > > so that we can then construct the ControlInfoMap from it, without > > destroying or losing the context_.ctrlMap? > > Yes. > > > I think that's a good thing to do - but makes me think we should have a > > ControlInfoMap(const &ControlInfoMap::Map, ...) version that wouldn't > > move and would make a copy internally. > > > > > > Would > > *ipaControls = ControlInfoMap(std::copy(context_ctrlMap), > > controls::controls); > > > > have been equivalent / expressive in the same way ? > > I don't think there is a single-argument std::copy but > > *ipaControls = ControlInfoMap(ControlInfoMap::Map(context_.ctrlMap), controls::controls); > > should work. The only question is whether the single line is better > than the two original lines. > I don't object to the two lines - I thought the current constructor was forcing you to make a copy and then move that copy... If the current constructors can do the right thing too then there's no real action /change required here ... > > But I think that's just implementation detail so: > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > >> + > >> /* > >> * Check if the sensor driver supports the controls required by the > >> * Soft IPA. > >> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp > >> index 3ddce71d3..38868df66 100644 > >> --- a/src/libcamera/pipeline/simple/simple.cpp > >> +++ b/src/libcamera/pipeline/simple/simple.cpp > >> @@ -530,7 +530,7 @@ int SimpleCameraData::init() > >> * Instantiate Soft ISP if this is enabled for the given driver and no converter is used. > >> */ > >> if (!converter_ && pipe->swIspEnabled()) { > >> - swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get()); > >> + swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get(), &controlInfo_); > >> if (!swIsp_->isValid()) { > >> LOG(SimplePipeline, Warning) > >> << "Failed to create software ISP, disabling software debayering"; > >> diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp > >> index 47677784d..3d3f956cb 100644 > >> --- a/src/libcamera/software_isp/software_isp.cpp > >> +++ b/src/libcamera/software_isp/software_isp.cpp > >> @@ -13,6 +13,7 @@ > >> #include <sys/types.h> > >> #include <unistd.h> > >> > >> +#include <libcamera/controls.h> > >> #include <libcamera/formats.h> > >> #include <libcamera/stream.h> > >> > >> @@ -60,9 +61,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) > >> * \brief Constructs SoftwareIsp object > >> * \param[in] pipe The pipeline handler in use > >> * \param[in] sensor Pointer to the CameraSensor instance owned by the pipeline > >> + * \param[out] ipaControls The IPA controls to update > >> * handler > >> */ > >> -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) > >> +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, > >> + ControlInfoMap *ipaControls) > >> : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | > >> DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | > >> DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) > >> @@ -124,7 +127,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) > >> int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, > >> debayer_->getStatsFD(), > >> sharedParams_.fd(), > >> - sensor->controls()); > >> + sensor->controls(), > >> + ipaControls); > >> if (ret) { > >> LOG(SoftwareIsp, Error) << "IPA init failed"; > >> debayer_.reset(); > >> -- > >> 2.44.1 > >> >
diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h index a3e3a9da4..d51b03fd6 100644 --- a/include/libcamera/internal/software_isp/software_isp.h +++ b/include/libcamera/internal/software_isp/software_isp.h @@ -46,7 +46,8 @@ LOG_DECLARE_CATEGORY(SoftwareIsp) class SoftwareIsp { public: - SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor); + SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, + ControlInfoMap *ipaControls); ~SoftwareIsp(); int loadConfiguration([[maybe_unused]] const std::string &filename) { return 0; } diff --git a/include/libcamera/ipa/soft.mojom b/include/libcamera/ipa/soft.mojom index 347fd69b4..16e6a7071 100644 --- a/include/libcamera/ipa/soft.mojom +++ b/include/libcamera/ipa/soft.mojom @@ -17,7 +17,7 @@ interface IPASoftInterface { libcamera.SharedFD fdStats, libcamera.SharedFD fdParams, libcamera.ControlInfoMap sensorCtrlInfoMap) - => (int32 ret); + => (int32 ret, libcamera.ControlInfoMap ipaControls); start() => (int32 ret); stop(); configure(IPAConfigInfo configInfo) diff --git a/src/ipa/simple/algorithms/contrast.cpp b/src/ipa/simple/algorithms/contrast.cpp index 1a2c14cf9..8d9997d81 100644 --- a/src/ipa/simple/algorithms/contrast.cpp +++ b/src/ipa/simple/algorithms/contrast.cpp @@ -19,6 +19,13 @@ LOG_DEFINE_CATEGORY(IPASoftContrast) namespace ipa::soft::algorithms { +int Contrast::init(IPAContext &context, + [[maybe_unused]] const YamlObject &tuningData) +{ + context.ctrlMap[&controls::Contrast] = ControlInfo(0.0f, 10.0f, 1.0f); + return 0; +} + int Contrast::configure(typename Module::Context &context, [[maybe_unused]] const typename Module::Config &configInfo) { diff --git a/src/ipa/simple/algorithms/contrast.h b/src/ipa/simple/algorithms/contrast.h index 0b3933099..405345acc 100644 --- a/src/ipa/simple/algorithms/contrast.h +++ b/src/ipa/simple/algorithms/contrast.h @@ -21,6 +21,8 @@ public: Contrast() = default; ~Contrast() = default; + int init(IPAContext &context, const YamlObject &tuningData) override; + int configure(typename Module::Context &context, const typename Module::Config &configInfo) override; diff --git a/src/ipa/simple/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml index 3f1471121..c091e30fd 100644 --- a/src/ipa/simple/data/uncalibrated.yaml +++ b/src/ipa/simple/data/uncalibrated.yaml @@ -5,6 +5,7 @@ version: 1 algorithms: - BlackLevel: - Awb: + - Contrast: - Lut: - Agc: ... diff --git a/src/ipa/simple/ipa_context.h b/src/ipa/simple/ipa_context.h index 5118d6abf..095c83800 100644 --- a/src/ipa/simple/ipa_context.h +++ b/src/ipa/simple/ipa_context.h @@ -65,6 +65,7 @@ struct IPAContext { IPASessionConfiguration configuration; IPAActiveState activeState; FCQueue<IPAFrameContext> frameContexts; + ControlInfoMap::Map ctrlMap; }; } /* namespace ipa::soft */ diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp index b28c7039f..eb027d754 100644 --- a/src/ipa/simple/soft_simple.cpp +++ b/src/ipa/simple/soft_simple.cpp @@ -41,7 +41,7 @@ class IPASoftSimple : public ipa::soft::IPASoftInterface, public Module { public: IPASoftSimple() - : context_({ {}, {}, { kMaxFrameContexts } }) + : context_({ {}, {}, { kMaxFrameContexts }, {} }) { } @@ -50,7 +50,8 @@ public: int init(const IPASettings &settings, const SharedFD &fdStats, const SharedFD &fdParams, - const ControlInfoMap &sensorInfoMap) override; + const ControlInfoMap &sensorInfoMap, + ControlInfoMap *ipaControls) override; int configure(const IPAConfigInfo &configInfo) override; int start() override; @@ -87,7 +88,8 @@ IPASoftSimple::~IPASoftSimple() int IPASoftSimple::init(const IPASettings &settings, const SharedFD &fdStats, const SharedFD &fdParams, - const ControlInfoMap &sensorInfoMap) + const ControlInfoMap &sensorInfoMap, + ControlInfoMap *ipaControls) { camHelper_ = CameraSensorHelperFactoryBase::create(settings.sensorModel); if (!camHelper_) { @@ -158,6 +160,9 @@ int IPASoftSimple::init(const IPASettings &settings, stats_ = static_cast<SwIspStats *>(mem); } + ControlInfoMap::Map ctrlMap = context_.ctrlMap; + *ipaControls = ControlInfoMap(std::move(ctrlMap), controls::controls); + /* * Check if the sensor driver supports the controls required by the * Soft IPA. diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp index 3ddce71d3..38868df66 100644 --- a/src/libcamera/pipeline/simple/simple.cpp +++ b/src/libcamera/pipeline/simple/simple.cpp @@ -530,7 +530,7 @@ int SimpleCameraData::init() * Instantiate Soft ISP if this is enabled for the given driver and no converter is used. */ if (!converter_ && pipe->swIspEnabled()) { - swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get()); + swIsp_ = std::make_unique<SoftwareIsp>(pipe, sensor_.get(), &controlInfo_); if (!swIsp_->isValid()) { LOG(SimplePipeline, Warning) << "Failed to create software ISP, disabling software debayering"; diff --git a/src/libcamera/software_isp/software_isp.cpp b/src/libcamera/software_isp/software_isp.cpp index 47677784d..3d3f956cb 100644 --- a/src/libcamera/software_isp/software_isp.cpp +++ b/src/libcamera/software_isp/software_isp.cpp @@ -13,6 +13,7 @@ #include <sys/types.h> #include <unistd.h> +#include <libcamera/controls.h> #include <libcamera/formats.h> #include <libcamera/stream.h> @@ -60,9 +61,11 @@ LOG_DEFINE_CATEGORY(SoftwareIsp) * \brief Constructs SoftwareIsp object * \param[in] pipe The pipeline handler in use * \param[in] sensor Pointer to the CameraSensor instance owned by the pipeline + * \param[out] ipaControls The IPA controls to update * handler */ -SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) +SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor, + ControlInfoMap *ipaControls) : dmaHeap_(DmaBufAllocator::DmaBufAllocatorFlag::CmaHeap | DmaBufAllocator::DmaBufAllocatorFlag::SystemHeap | DmaBufAllocator::DmaBufAllocatorFlag::UDmaBuf) @@ -124,7 +127,8 @@ SoftwareIsp::SoftwareIsp(PipelineHandler *pipe, const CameraSensor *sensor) int ret = ipa_->init(IPASettings{ ipaTuningFile, sensor->model() }, debayer_->getStatsFD(), sharedParams_.fd(), - sensor->controls()); + sensor->controls(), + ipaControls); if (ret) { LOG(SoftwareIsp, Error) << "IPA init failed"; debayer_.reset();
This patch introduces support for applying runtime controls to software ISP. It enables the contrast algorithm as the first control that can be used. Signed-off-by: Milan Zamazal <mzamazal@redhat.com> --- .../libcamera/internal/software_isp/software_isp.h | 3 ++- include/libcamera/ipa/soft.mojom | 2 +- src/ipa/simple/algorithms/contrast.cpp | 7 +++++++ src/ipa/simple/algorithms/contrast.h | 2 ++ src/ipa/simple/data/uncalibrated.yaml | 1 + src/ipa/simple/ipa_context.h | 1 + src/ipa/simple/soft_simple.cpp | 11 ++++++++--- src/libcamera/pipeline/simple/simple.cpp | 2 +- src/libcamera/software_isp/software_isp.cpp | 8 ++++++-- 9 files changed, 29 insertions(+), 8 deletions(-)