[PATCH 2/2] libcamera: software_isp: Add contrast control
Milan Zamazal
mzamazal at redhat.com
Mon Sep 16 18:05:05 CEST 2024
Milan Zamazal <mzamazal at redhat.com> writes:
> 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 can see there has been a similar change posted for rkisp1, I'll try to
follow it.
>> 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