[PATCH v2 2/2] libcamera: software_isp: Add contrast control
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 10 13:15:32 CEST 2024
Quoting Milan Zamazal (2024-10-10 10:37:41)
> Kieran Bingham <kieran.bingham at 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 at 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 at 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
> >>
>
More information about the libcamera-devel
mailing list