[PATCH] libcamera: rkisp1: Rationalize IPA and handlers names
Milan Zamazal
mzamazal at redhat.com
Tue Oct 29 10:28:22 CET 2024
Milan Zamazal <mzamazal at redhat.com> writes:
> Hi Jacopo,
>
> thank you for the patch. As somebody who came to libcamera and had to
> understand that stuff, I can confirm the original terminology is quite
> confusing and your patch is a good improvement.
(And now the question is who would make similar changes to the other
pipelines; I can take care of `simple'.)
> Jacopo Mondi <jacopo.mondi at ideasonboard.com> writes:
>
>> The names used by the IPA interface and the names used for buffer
>> completions handlers for the RkISP1 clash in the use of the term "buffer".
>>
>> For example the video device buffer completion handler is called
>> "bufferReady" and the IPA event to ask the IPA to compute parameters is
>> called "fillParamsBuffers". This makes it hard to recognize which
>> function handles video device completion signals and which ones handle
>> the IPA interface events.
>>
>> Rationalize the naming scheme in the IPA interface function and events
>> and the signal handlers in the RkISP1 support, according to the
>> following table. Remove the name "buffer" from the IPA interface events
>> and events handler and reserve it for the buffer completion handlers.
>> Rename the IPA interface events and function to use the 'params' and
>> 'stats' names and make the signal and the even names specular (ie
>> 'computeParams' : 'paramsComputed')
>>
>> IPA Interface:
>>
>> - fillParamsBuffer -> computeParams [FUNCTION]
>> - paramsBufferReady -> paramsComputed [EVENT]
>>
>> - processStatsBuffer -> processStats [FUNCTION]
>> - metadataReady -> statsProcessed [EVENT]
>>
>> Pipeline handler:
>>
>> - bufferReady -> videoBufferReady [BUFFER HANDLER]
>> - paramReady -> paramsBufferReady [BUFFER HANDLER]
>> - statReady -> statsBufferReady [BUFFER HANDLER]
>> - paramFilled -> paramsComputed [IPA EVENT HANDLER]
>> - metadataReady -> statsProcessed [IPA EVENT HANDLER]
>>
>> Cosmetic change only, no functional changes intended.
>>
>> Signed-off-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
>
> Reviewed-by: Milan Zamazal <mzamazal at redhat.com>
>
>> ---
>> include/libcamera/ipa/rkisp1.mojom | 10 +++---
>> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----
>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------
>> 3 files changed, 34 insertions(+), 34 deletions(-)
>>
>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom
>> index 80d54a03aa90..ced1526e18cd 100644
>> --- a/include/libcamera/ipa/rkisp1.mojom
>> +++ b/include/libcamera/ipa/rkisp1.mojom
>> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {
>> unmapBuffers(array<uint32> ids);
>>
>> [async] queueRequest(uint32 frame, libcamera.ControlList reqControls);
>> - [async] fillParamsBuffer(uint32 frame, uint32 bufferId);
>> - [async] processStatsBuffer(uint32 frame, uint32 bufferId,
>> - libcamera.ControlList sensorControls);
>> + [async] computeParams(uint32 frame, uint32 bufferId);
>> + [async] processStats(uint32 frame, uint32 bufferId,
>> + libcamera.ControlList sensorControls);
>> };
>>
>> interface IPARkISP1EventInterface {
>> - paramsBufferReady(uint32 frame, uint32 bytesused);
>> + paramsComputed(uint32 frame, uint32 bytesused);
>> setSensorControls(uint32 frame, libcamera.ControlList sensorControls);
>> - metadataReady(uint32 frame, libcamera.ControlList metadata);
>> + statsProcessed(uint32 frame, libcamera.ControlList metadata);
>> };
>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp
>> index 9e161cabdea4..6d6f8392a70f 100644
>> --- a/src/ipa/rkisp1/rkisp1.cpp
>> +++ b/src/ipa/rkisp1/rkisp1.cpp
>> @@ -65,9 +65,9 @@ public:
>> void unmapBuffers(const std::vector<unsigned int> &ids) override;
>>
>> void queueRequest(const uint32_t frame, const ControlList &controls) override;
>> - void fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;
>> - void processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>> - const ControlList &sensorControls) override;
>> + void computeParams(const uint32_t frame, const uint32_t bufferId) override;
>> + void processStats(const uint32_t frame, const uint32_t bufferId,
>> + const ControlList &sensorControls) override;
>>
>> protected:
>> std::string logPrefix() const override;
>> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)
>> }
>> }
>>
>> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)
>> {
>> IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>
>> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)
>> for (auto const &algo : algorithms())
>> algo->prepare(context_, frame, frameContext, ¶ms);
>>
>> - paramsBufferReady.emit(frame, params.size());
>> + paramsComputed.emit(frame, params.size());
>> }
>>
>> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,
>> - const ControlList &sensorControls)
>> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,
>> + const ControlList &sensorControls)
>> {
>> IPAFrameContext &frameContext = context_.frameContexts.get(frame);
>>
>> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId
>>
>> setControls(frame);
>>
>> - metadataReady.emit(frame, metadata);
>> + statsProcessed.emit(frame, metadata);
>> }
>>
>> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,
>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> index c7b0b3927da1..83b74b27652f 100644
>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>> @@ -114,11 +114,11 @@ public:
>> ControlInfoMap ipaControls_;
>>
>> private:
>> - void paramFilled(unsigned int frame, unsigned int bytesused);
>> + void paramsComputed(unsigned int frame, unsigned int bytesused);
>> void setSensorControls(unsigned int frame,
>> const ControlList &sensorControls);
>>
>> - void metadataReady(unsigned int frame, const ControlList &metadata);
>> + void statsProcessed(unsigned int frame, const ControlList &metadata);
>> };
>>
>> class RkISP1CameraConfiguration : public CameraConfiguration
>> @@ -180,9 +180,9 @@ private:
>> const RkISP1CameraConfiguration &config);
>> int createCamera(MediaEntity *sensor);
>> void tryCompleteRequest(RkISP1FrameInfo *info);
>> - void bufferReady(FrameBuffer *buffer);
>> - void paramReady(FrameBuffer *buffer);
>> - void statReady(FrameBuffer *buffer);
>> + void videoBufferReady(FrameBuffer *buffer);
>> + void paramsBufferReady(FrameBuffer *buffer);
>> + void statsBufferReady(FrameBuffer *buffer);
>> void dewarpBufferReady(FrameBuffer *buffer);
>> void frameStart(uint32_t sequence);
>>
>> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>> return -ENOENT;
>>
>> ipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);
>> - ipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);
>> - ipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);
>> + ipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);
>> + ipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);
>>
>> /*
>> * The API tuning file is made from the sensor name unless the
>> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)
>> return 0;
>> }
>>
>> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)
>> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)
>> {
>> PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();
>> RkISP1FrameInfo *info = frameInfo_.find(frame);
>> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,
>> delayedCtrls_->push(sensorControls);
>> }
>>
>> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)
>> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)
>> {
>> RkISP1FrameInfo *info = frameInfo_.find(frame);
>> if (!info)
>> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)
>> if (data->selfPath_ && info->selfPathBuffer)
>> data->selfPath_->queueBuffer(info->selfPathBuffer);
>> } else {
>> - data->ipa_->fillParamsBuffer(data->frame_,
>> - info->paramBuffer->cookie());
>> + data->ipa_->computeParams(data->frame_,
>> + info->paramBuffer->cookie());
>> }
>>
>> data->frame_++;
>> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>> if (hasSelfPath_ && !selfPath_.init(media_))
>> return false;
>>
>> - mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>> + mainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);
>> if (hasSelfPath_)
>> - selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);
>> - stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
>> - param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>> + selfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);
>> + stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);
>> + param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);
>>
>> /* If dewarper is present, create its instance. */
>> DeviceMatch dwp("dw100");
>> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)
>> completeRequest(request);
>> }
>>
>> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)
>> {
>> ASSERT(activeCamera_);
>> RkISP1CameraData *data = cameraData(activeCamera_);
>> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)
>> if (isRaw_) {
>> const ControlList &ctrls =
>> data->delayedCtrls_->get(metadata.sequence);
>> - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);
>> + data->ipa_->processStats(info->frame, 0, ctrls);
>> }
>> } else {
>> if (isRaw_)
>> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)
>> tryCompleteRequest(info);
>> }
>>
>> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)
>> {
>> ASSERT(activeCamera_);
>> RkISP1CameraData *data = cameraData(activeCamera_);
>> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>> tryCompleteRequest(info);
>> }
>>
>> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)
>> {
>> ASSERT(activeCamera_);
>> RkISP1CameraData *data = cameraData(activeCamera_);
>> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>> if (data->frame_ <= buffer->metadata().sequence)
>> data->frame_ = buffer->metadata().sequence + 1;
>>
>> - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),
>> - data->delayedCtrls_->get(buffer->metadata().sequence));
>> + data->ipa_->processStats(info->frame, info->statBuffer->cookie(),
>> + data->delayedCtrls_->get(buffer->metadata().sequence));
>> }
>>
>> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, "rkisp1")
More information about the libcamera-devel
mailing list