[PATCH v2 2/2] libcamera: software_isp: Add contrast control
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Oct 10 00:06:33 CEST 2024
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?
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 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