[PATCH 2/2] libcamera: software_isp: Add contrast control
Milan Zamazal
mzamazal at redhat.com
Mon Sep 16 17:12:49 CEST 2024
Hi Kieran,
thank you for review.
Kieran Bingham <kieran.bingham at ideasonboard.com> writes:
> Quoting Milan Zamazal (2024-09-04 08:44:59)
>> 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/data/uncalibrated.yaml | 1 +
>> src/ipa/simple/soft_simple.cpp | 13 +++++++++++--
>> src/libcamera/pipeline/simple/simple.cpp | 2 +-
>> src/libcamera/software_isp/software_isp.cpp | 8 ++++++--
>> 6 files changed, 22 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/libcamera/internal/software_isp/software_isp.h b/include/libcamera/internal/software_isp/software_isp.h
>> index a3e3a9da..d51b03fd 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 bc70d4e4..3b8a72a2 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/data/uncalibrated.yaml b/src/ipa/simple/data/uncalibrated.yaml
>> index 324f98f4..2ed24e8c 100644
>> --- a/src/ipa/simple/data/uncalibrated.yaml
>> +++ b/src/ipa/simple/data/uncalibrated.yaml
>> @@ -4,6 +4,7 @@
>> version: 1
>> algorithms:
>> - BlackLevel:
>> + - Contrast:
>> - Gamma:
>> - Awb:
>> - Lut:
>> diff --git a/src/ipa/simple/soft_simple.cpp b/src/ipa/simple/soft_simple.cpp
>> index ac7a22b7..d6c7ff7f 100644
>> --- a/src/ipa/simple/soft_simple.cpp
>> +++ b/src/ipa/simple/soft_simple.cpp
>> @@ -34,6 +34,10 @@ LOG_DEFINE_CATEGORY(IPASoft)
>>
>> namespace ipa::soft {
>>
>> +const ControlInfoMap::Map swispControls{
>> + { &controls::Contrast, ControlInfo(0.0f, 10.0f, 1.0f) },
>> +};
>
> I would prefer not to register all controls at the top level.
Well, I followed what the other pipelines do.
> Can the control be registered by the algorithm that supports it?
I don't think Algorithm class API allows that. But I can look again at
the other pipelines for possibly more inspiration.
> I'm not sure if 'contrast' is a full algorithm if it's all being handled
> through gamma ... but it's only a detail for now, I guess it's a way to
> enable or disable features if it's separated...
I don't have strong opinion about this. On one hand it's better to not
put many things on a single pile, on the other hand the separation may
not make much sense here. Another option is to modify the gamma table,
or at least provide the S-curve, in the contrast algorithm.
>> +
>> /* Maximum number of frame contexts to be held */
>> static constexpr uint32_t kMaxFrameContexts = 16;
>>
>> @@ -50,7 +54,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 +92,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 +164,9 @@ int IPASoftSimple::init(const IPASettings &settings,
>> stats_ = static_cast<SwIspStats *>(mem);
>> }
>>
>> + ControlInfoMap::Map ctrlMap = swispControls;
>> + *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 fc80e665..86c4447e 100644
>> --- a/src/libcamera/pipeline/simple/simple.cpp
>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>> @@ -531,7 +531,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 dbf27f31..78b78bab 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/request.h>
>> #include <libcamera/stream.h>
>> @@ -61,9 +62,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)
>> @@ -125,7 +128,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