[PATCH v2 2/2] libcamera: software_isp: Add contrast control
Milan Zamazal
mzamazal at redhat.com
Thu Oct 10 11:37:41 CEST 2024
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.
> 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