[PATCH] libcamera: rkisp1: Rationalize IPA and handlers names
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Oct 30 10:52:24 CET 2024
On Wed, Oct 30, 2024 at 09:32:44AM +0100, Jacopo Mondi wrote:
> Hi Milan
> thanks for reviw
>
> On Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:
> > 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'.)
> >
>
> Well, the first one who gets there and is annoyed by the existing
> names I would say :)
>
> I only count vimc and ipu3, soft-isp apart.
>
> Let's start with this and simple if you like, then we'll get to the
> other ones eventually ? After all, the IPA interface names doesn't
> need to be standardized across all platforms.
>
> On the other hand, true, for consistency I should rather bite the
> bullet and convert all of them in one go ?
It should be easy to convert the other pipeline handlers, given that
only the first one should be subject to bikeshedding of the names. I
would prefer addressing them all in one go.
> > > 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]
This is the only change I don't like. The metadataReady event is really
about signalling that metadata is ready, not that stats have been
processed.
> > >>
> > >> Pipeline handler:
> > >>
> > >> - bufferReady -> videoBufferReady [BUFFER HANDLER]
I think we use the term "image" instead of "video" preferentially.
> > >> - paramReady -> paramsBufferReady [BUFFER HANDLER]
> > >> - statReady -> statsBufferReady [BUFFER HANDLER]
> > >> - paramFilled -> paramsComputed [IPA EVENT HANDLER]
> > >> - metadataReady -> statsProcessed [IPA EVENT HANDLER]
This depends on if we rename the metadataReady event, and what we rename
it to.
The rest looks good.
> > >>
> > >> 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")
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list