[{"id":31954,"web_url":"https://patchwork.libcamera.org/comment/31954/","msgid":"<87v7xbntqh.fsf@redhat.com>","date":"2024-10-29T09:28:22","subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Milan Zamazal writes:\n\n> Hi Jacopo,\n>\n> thank you for the patch. As somebody who came to libcamera and had to\n> understand that stuff, I can confirm the original terminology is quite\n> confusing and your patch is a good improvement.\n\n(And now the question is who would make similar changes to the other\npipelines; I can take care of `simple'.)\n\n> Jacopo Mondi writes:\n>\n>> The names used by the IPA interface and the names used for buffer\n>> completions handlers for the RkISP1 clash in the use of the term \"buffer\".\n>>\n>> For example the video device buffer completion handler is called\n>> \"bufferReady\" and the IPA event to ask the IPA to compute parameters is\n>> called \"fillParamsBuffers\". This makes it hard to recognize which\n>> function handles video device completion signals and which ones handle\n>> the IPA interface events.\n>>\n>> Rationalize the naming scheme in the IPA interface function and events\n>> and the signal handlers in the RkISP1 support, according to the\n>> following table. Remove the name \"buffer\" from the IPA interface events\n>> and events handler and reserve it for the buffer completion handlers.\n>> Rename the IPA interface events and function to use the 'params' and\n>> 'stats' names and make the signal and the even names specular (ie\n>> 'computeParams' : 'paramsComputed')\n>>\n>> IPA Interface:\n>>\n>> - fillParamsBuffer -> computeParams [FUNCTION]\n>> - paramsBufferReady -> paramsComputed [EVENT]\n>>\n>> - processStatsBuffer -> processStats [FUNCTION]\n>> - metadataReady -> statsProcessed [EVENT]\n>>\n>> Pipeline handler:\n>>\n>> - bufferReady -> videoBufferReady [BUFFER HANDLER]\n>> - paramReady -> paramsBufferReady [BUFFER HANDLER]\n>> - statReady -> statsBufferReady [BUFFER HANDLER]\n>> - paramFilled -> paramsComputed [IPA EVENT HANDLER]\n>> - metadataReady -> statsProcessed [IPA EVENT HANDLER]\n>>\n>> Cosmetic change only, no functional changes intended.\n>>\n>> Signed-off-by: Jacopo Mondi \n>\n> Reviewed-by: Milan Zamazal \n>\n>> ---\n>> include/libcamera/ipa/rkisp1.mojom | 10 +++---\n>> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----\n>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------\n>> 3 files changed, 34 insertions(+), 34 deletions(-)\n>>\n>> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n>> index 80d54a03aa90..ced1526e18cd 100644\n>> --- a/include/libcamera/ipa/rkisp1.mojom\n>> +++ b/include/libcamera/ipa/rkisp1.mojom\n>> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {\n>> \tunmapBuffers(array ids);\n>> \n>> \t[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);\n>> -\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> -\t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n>> -\t\t\t\t libcamera.ControlList sensorControls);\n>> +\t[async] computeParams(uint32 frame, uint32 bufferId);\n>> +\t[async] processStats(uint32 frame, uint32 bufferId,\n>> +\t\t\t libcamera.ControlList sensorControls);\n>> };\n>> \n>> interface IPARkISP1EventInterface {\n>> -\tparamsBufferReady(uint32 frame, uint32 bytesused);\n>> +\tparamsComputed(uint32 frame, uint32 bytesused);\n>> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n>> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>> +\tstatsProcessed(uint32 frame, libcamera.ControlList metadata);\n>> };\n>> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> index 9e161cabdea4..6d6f8392a70f 100644\n>> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> @@ -65,9 +65,9 @@ public:\n>> \tvoid unmapBuffers(const std::vector &ids) override;\n>> \n>> \tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> -\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> -\tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n>> -\t\t\t\tconst ControlList &sensorControls) override;\n>> +\tvoid computeParams(const uint32_t frame, const uint32_t bufferId) override;\n>> +\tvoid processStats(const uint32_t frame, const uint32_t bufferId,\n>> +\t\t\t const ControlList &sensorControls) override;\n>> \n>> protected:\n>> \tstd::string logPrefix() const override;\n>> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n>> \t}\n>> }\n>> \n>> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n>> {\n>> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>> \n>> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> \tfor (auto const &algo : algorithms())\n>> \t\talgo->prepare(context_, frame, frameContext, ¶ms);\n>> \n>> -\tparamsBufferReady.emit(frame, params.size());\n>> +\tparamsComputed.emit(frame, params.size());\n>> }\n>> \n>> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n>> -\t\t\t\t const ControlList &sensorControls)\n>> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n>> +\t\t\t const ControlList &sensorControls)\n>> {\n>> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>> \n>> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>> \n>> \tsetControls(frame);\n>> \n>> -\tmetadataReady.emit(frame, metadata);\n>> +\tstatsProcessed.emit(frame, metadata);\n>> }\n>> \n>> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> index c7b0b3927da1..83b74b27652f 100644\n>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> @@ -114,11 +114,11 @@ public:\n>> \tControlInfoMap ipaControls_;\n>> \n>> private:\n>> -\tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n>> +\tvoid paramsComputed(unsigned int frame, unsigned int bytesused);\n>> \tvoid setSensorControls(unsigned int frame,\n>> \t\t\t const ControlList &sensorControls);\n>> \n>> -\tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n>> +\tvoid statsProcessed(unsigned int frame, const ControlList &metadata);\n>> };\n>> \n>> class RkISP1CameraConfiguration : public CameraConfiguration\n>> @@ -180,9 +180,9 @@ private:\n>> \t\t const RkISP1CameraConfiguration &config);\n>> \tint createCamera(MediaEntity *sensor);\n>> \tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n>> -\tvoid bufferReady(FrameBuffer *buffer);\n>> -\tvoid paramReady(FrameBuffer *buffer);\n>> -\tvoid statReady(FrameBuffer *buffer);\n>> +\tvoid videoBufferReady(FrameBuffer *buffer);\n>> +\tvoid paramsBufferReady(FrameBuffer *buffer);\n>> +\tvoid statsBufferReady(FrameBuffer *buffer);\n>> \tvoid dewarpBufferReady(FrameBuffer *buffer);\n>> \tvoid frameStart(uint32_t sequence);\n>> \n>> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> \t\treturn -ENOENT;\n>> \n>> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n>> -\tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n>> -\tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n>> +\tipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);\n>> +\tipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);\n>> \n>> \t/*\n>> \t * The API tuning file is made from the sensor name unless the\n>> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> \treturn 0;\n>> }\n>> \n>> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n>> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n>> {\n>> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n>> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n>> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n>> \tdelayedCtrls_->push(sensorControls);\n>> }\n>> \n>> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n>> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)\n>> {\n>> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n>> \tif (!info)\n>> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>> \t\tif (data->selfPath_ && info->selfPathBuffer)\n>> \t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n>> \t} else {\n>> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n>> -\t\t\t\t\t info->paramBuffer->cookie());\n>> +\t\tdata->ipa_->computeParams(data->frame_,\n>> +\t\t\t\t\t info->paramBuffer->cookie());\n>> \t}\n>> \n>> \tdata->frame_++;\n>> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> \tif (hasSelfPath_ && !selfPath_.init(media_))\n>> \t\treturn false;\n>> \n>> -\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n>> +\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n>> \tif (hasSelfPath_)\n>> -\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n>> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n>> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);\n>> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);\n>> \n>> \t/* If dewarper is present, create its instance. */\n>> \tDeviceMatch dwp(\"dw100\");\n>> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n>> \tcompleteRequest(request);\n>> }\n>> \n>> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)\n>> {\n>> \tASSERT(activeCamera_);\n>> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> \t\tif (isRaw_) {\n>> \t\t\tconst ControlList &ctrls =\n>> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n>> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n>> +\t\t\tdata->ipa_->processStats(info->frame, 0, ctrls);\n>> \t\t}\n>> \t} else {\n>> \t\tif (isRaw_)\n>> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n>> \ttryCompleteRequest(info);\n>> }\n>> \n>> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n>> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)\n>> {\n>> \tASSERT(activeCamera_);\n>> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n>> \ttryCompleteRequest(info);\n>> }\n>> \n>> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)\n>> {\n>> \tASSERT(activeCamera_);\n>> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>> \tif (data->frame_ <= buffer->metadata().sequence)\n>> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n>> \n>> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n>> -\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n>> +\tdata->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n>> +\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n>> }\n>> \n>> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 88D39C31E9\n\tfor ;\n\tTue, 29 Oct 2024 09:28:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9E0396539F;\n\tTue, 29 Oct 2024 10:28:31 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.133.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EBCE360366\n\tfor ;\n\tTue, 29 Oct 2024 10:28:29 +0100 (CET)","from mail-wr1-f70.google.com (mail-wr1-f70.google.com\n\t[209.85.221.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-680-EqvPDVMePVKX3m0Zpv_iFQ-1; Tue, 29 Oct 2024 05:28:26 -0400","by mail-wr1-f70.google.com with SMTP id\n\tffacd0b85a97d-37d458087c0so3691675f8f.1\n\tfor ;\n\tTue, 29 Oct 2024 02:28:26 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\t5b1f17b1804b1-43193572910sm137297135e9.9.2024.10.29.02.28.23\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 29 Oct 2024 02:28:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"KcUx4k0+\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730194108;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=ReA48A9tTQIUyio0YqRncqgTRpF7rl1JH1uroa5YrFw=;\n\tb=KcUx4k0+ywPcDrF5r5dE9/9ZMawikkMvPpaDXBcdSjMvENp6YC3Yhee+6pipKHhvMkS3F5\n\tX3x1VrRWUq6EgmutrqMBe0SzkNWbh3837rE3eKyJ6+mWq5V/vznj2lp4cnwoZ6wV0Yg92w\n\tpuEK5Kt6T9DWU90znwoJZ27ThYiA56c=","X-MC-Unique":"EqvPDVMePVKX3m0Zpv_iFQ-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730194105; x=1730798905;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=ReA48A9tTQIUyio0YqRncqgTRpF7rl1JH1uroa5YrFw=;\n\tb=Pg0Q3cGxAE7kvbuWVz9yzIPN0geY4pC/ptTf8+n8SGO+chUQVlnyvElIPENx0F1/kg\n\tt3fT4vkMYXYli+DNLTklWXiASb4oG/AU9CMWFAQpjya+1G72UY6oZ2yKh4WFEcmyCRxo\n\tnyP0/reVwUpN3td3fyMSBuX3IslM/dMydQmkhT4sntX7MQhoB8DMTewU6O9FjId61cBx\n\tnxdk6KYseBYORJ/KUMaLkgqggtcNzt5B+PBViQFg3BE3IEmLS8j7uaGjPkI4G5uNAaOM\n\t4yfH+Hg5ZYcTyjn83Tu34hlp+4eYbjv6Fx8THmmDsnWh1b0/NrfnW4+0qERa+bQhdlD9\n\tDmUg==","X-Gm-Message-State":"AOJu0YwQh5amk5gunntXRE/k2bT55EZ1YNxZ4dEMAFrBzH/9VQFKHPLa\n\tHrlH2Uu3ryyz1orHXr1mZhx0ploRUUk7w5feVZACcBbxcKnUcCslvCrSOQgEKXBWVle9GHFVj8j\n\t/bhHE+fnajKfFQ7/CjgEE1Ct6J+DnbxeloIOwXOetO+Gl6MXllXC9/j1j3L43BLShG7faN0tlnr\n\tlk7Gz6bKmHvQZW4tm0LnDB0pG9H6xx5sWOYmHWIwnsk9FwTSas3YXZbSE=","X-Received":["by 2002:a5d:5707:0:b0:374:c33d:377d with SMTP id\n\tffacd0b85a97d-38162922b96mr1006883f8f.28.1730194105032; \n\tTue, 29 Oct 2024 02:28:25 -0700 (PDT)","by 2002:a5d:5707:0:b0:374:c33d:377d with SMTP id\n\tffacd0b85a97d-38162922b96mr1006857f8f.28.1730194104320; \n\tTue, 29 Oct 2024 02:28:24 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IGEQUBlfzgoq/ejeGMs6tpGoykegp2VrUz6FVZTUB111qIR9EWulel+Gb8NpORhu0s0937gFg==","From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","In-Reply-To":"<87zfmnntuq.fsf@redhat.com> (Milan Zamazal's message of \"Tue, 29\n\tOct 2024 10:25:49 +0100\")","References":"<20241028091249.16473-1-jacopo.mondi@ideasonboard.com>\n\t<87zfmnntuq.fsf@redhat.com>","Date":"Tue, 29 Oct 2024 10:28:22 +0100","Message-ID":"<87v7xbntqh.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31966,"web_url":"https://patchwork.libcamera.org/comment/31966/","msgid":"","date":"2024-10-30T08:32:44","subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Milan\n thanks for reviw\n\nOn Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:\n> Milan Zamazal writes:\n>\n> > Hi Jacopo,\n> >\n> > thank you for the patch. As somebody who came to libcamera and had to\n> > understand that stuff, I can confirm the original terminology is quite\n> > confusing and your patch is a good improvement.\n>\n> (And now the question is who would make similar changes to the other\n> pipelines; I can take care of `simple'.)\n>\n\nWell, the first one who gets there and is annoyed by the existing\nnames I would say :)\n\nI only count vimc and ipu3, soft-isp apart.\n\nLet's start with this and simple if you like, then we'll get to the\nother ones eventually ? After all, the IPA interface names doesn't\nneed to be standardized across all platforms.\n\nOn the other hand, true, for consistency I should rather bite the\nbullet and convert all of them in one go ?\n\n> > Jacopo Mondi writes:\n> >\n> >> The names used by the IPA interface and the names used for buffer\n> >> completions handlers for the RkISP1 clash in the use of the term \"buffer\".\n> >>\n> >> For example the video device buffer completion handler is called\n> >> \"bufferReady\" and the IPA event to ask the IPA to compute parameters is\n> >> called \"fillParamsBuffers\". This makes it hard to recognize which\n> >> function handles video device completion signals and which ones handle\n> >> the IPA interface events.\n> >>\n> >> Rationalize the naming scheme in the IPA interface function and events\n> >> and the signal handlers in the RkISP1 support, according to the\n> >> following table. Remove the name \"buffer\" from the IPA interface events\n> >> and events handler and reserve it for the buffer completion handlers.\n> >> Rename the IPA interface events and function to use the 'params' and\n> >> 'stats' names and make the signal and the even names specular (ie\n> >> 'computeParams' : 'paramsComputed')\n> >>\n> >> IPA Interface:\n> >>\n> >> - fillParamsBuffer -> computeParams [FUNCTION]\n> >> - paramsBufferReady -> paramsComputed [EVENT]\n> >>\n> >> - processStatsBuffer -> processStats [FUNCTION]\n> >> - metadataReady -> statsProcessed [EVENT]\n> >>\n> >> Pipeline handler:\n> >>\n> >> - bufferReady -> videoBufferReady [BUFFER HANDLER]\n> >> - paramReady -> paramsBufferReady [BUFFER HANDLER]\n> >> - statReady -> statsBufferReady [BUFFER HANDLER]\n> >> - paramFilled -> paramsComputed [IPA EVENT HANDLER]\n> >> - metadataReady -> statsProcessed [IPA EVENT HANDLER]\n> >>\n> >> Cosmetic change only, no functional changes intended.\n> >>\n> >> Signed-off-by: Jacopo Mondi \n> >\n> > Reviewed-by: Milan Zamazal \n> >\n> >> ---\n> >> include/libcamera/ipa/rkisp1.mojom | 10 +++---\n> >> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----\n> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------\n> >> 3 files changed, 34 insertions(+), 34 deletions(-)\n> >>\n> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> >> index 80d54a03aa90..ced1526e18cd 100644\n> >> --- a/include/libcamera/ipa/rkisp1.mojom\n> >> +++ b/include/libcamera/ipa/rkisp1.mojom\n> >> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {\n> >> \tunmapBuffers(array ids);\n> >>\n> >> \t[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);\n> >> -\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> >> -\t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n> >> -\t\t\t\t libcamera.ControlList sensorControls);\n> >> +\t[async] computeParams(uint32 frame, uint32 bufferId);\n> >> +\t[async] processStats(uint32 frame, uint32 bufferId,\n> >> +\t\t\t libcamera.ControlList sensorControls);\n> >> };\n> >>\n> >> interface IPARkISP1EventInterface {\n> >> -\tparamsBufferReady(uint32 frame, uint32 bytesused);\n> >> +\tparamsComputed(uint32 frame, uint32 bytesused);\n> >> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> >> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> >> +\tstatsProcessed(uint32 frame, libcamera.ControlList metadata);\n> >> };\n> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> >> index 9e161cabdea4..6d6f8392a70f 100644\n> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> >> @@ -65,9 +65,9 @@ public:\n> >> \tvoid unmapBuffers(const std::vector &ids) override;\n> >>\n> >> \tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> >> -\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> >> -\tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> >> -\t\t\t\tconst ControlList &sensorControls) override;\n> >> +\tvoid computeParams(const uint32_t frame, const uint32_t bufferId) override;\n> >> +\tvoid processStats(const uint32_t frame, const uint32_t bufferId,\n> >> +\t\t\t const ControlList &sensorControls) override;\n> >>\n> >> protected:\n> >> \tstd::string logPrefix() const override;\n> >> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> >> \t}\n> >> }\n> >>\n> >> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> >> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n> >> {\n> >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >>\n> >> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> >> \tfor (auto const &algo : algorithms())\n> >> \t\talgo->prepare(context_, frame, frameContext, ¶ms);\n> >>\n> >> -\tparamsBufferReady.emit(frame, params.size());\n> >> +\tparamsComputed.emit(frame, params.size());\n> >> }\n> >>\n> >> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> >> -\t\t\t\t const ControlList &sensorControls)\n> >> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> >> +\t\t\t const ControlList &sensorControls)\n> >> {\n> >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> >>\n> >> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> >>\n> >> \tsetControls(frame);\n> >>\n> >> -\tmetadataReady.emit(frame, metadata);\n> >> +\tstatsProcessed.emit(frame, metadata);\n> >> }\n> >>\n> >> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> index c7b0b3927da1..83b74b27652f 100644\n> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> >> @@ -114,11 +114,11 @@ public:\n> >> \tControlInfoMap ipaControls_;\n> >>\n> >> private:\n> >> -\tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> >> +\tvoid paramsComputed(unsigned int frame, unsigned int bytesused);\n> >> \tvoid setSensorControls(unsigned int frame,\n> >> \t\t\t const ControlList &sensorControls);\n> >>\n> >> -\tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n> >> +\tvoid statsProcessed(unsigned int frame, const ControlList &metadata);\n> >> };\n> >>\n> >> class RkISP1CameraConfiguration : public CameraConfiguration\n> >> @@ -180,9 +180,9 @@ private:\n> >> \t\t const RkISP1CameraConfiguration &config);\n> >> \tint createCamera(MediaEntity *sensor);\n> >> \tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> >> -\tvoid bufferReady(FrameBuffer *buffer);\n> >> -\tvoid paramReady(FrameBuffer *buffer);\n> >> -\tvoid statReady(FrameBuffer *buffer);\n> >> +\tvoid videoBufferReady(FrameBuffer *buffer);\n> >> +\tvoid paramsBufferReady(FrameBuffer *buffer);\n> >> +\tvoid statsBufferReady(FrameBuffer *buffer);\n> >> \tvoid dewarpBufferReady(FrameBuffer *buffer);\n> >> \tvoid frameStart(uint32_t sequence);\n> >>\n> >> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> >> \t\treturn -ENOENT;\n> >>\n> >> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n> >> -\tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n> >> -\tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n> >> +\tipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);\n> >> +\tipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);\n> >>\n> >> \t/*\n> >> \t * The API tuning file is made from the sensor name unless the\n> >> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> >> \treturn 0;\n> >> }\n> >>\n> >> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> >> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n> >> {\n> >> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> >> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> >> \tdelayedCtrls_->push(sensorControls);\n> >> }\n> >>\n> >> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> >> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)\n> >> {\n> >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> >> \tif (!info)\n> >> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> >> \t\tif (data->selfPath_ && info->selfPathBuffer)\n> >> \t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> >> \t} else {\n> >> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> >> -\t\t\t\t\t info->paramBuffer->cookie());\n> >> +\t\tdata->ipa_->computeParams(data->frame_,\n> >> +\t\t\t\t\t info->paramBuffer->cookie());\n> >> \t}\n> >>\n> >> \tdata->frame_++;\n> >> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> >> \tif (hasSelfPath_ && !selfPath_.init(media_))\n> >> \t\treturn false;\n> >>\n> >> -\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> >> +\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> >> \tif (hasSelfPath_)\n> >> -\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> >> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> >> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> >> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> >> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);\n> >> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);\n> >>\n> >> \t/* If dewarper is present, create its instance. */\n> >> \tDeviceMatch dwp(\"dw100\");\n> >> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> >> \tcompleteRequest(request);\n> >> }\n> >>\n> >> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> >> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)\n> >> {\n> >> \tASSERT(activeCamera_);\n> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> >> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> >> \t\tif (isRaw_) {\n> >> \t\t\tconst ControlList &ctrls =\n> >> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> >> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> >> +\t\t\tdata->ipa_->processStats(info->frame, 0, ctrls);\n> >> \t\t}\n> >> \t} else {\n> >> \t\tif (isRaw_)\n> >> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> >> \ttryCompleteRequest(info);\n> >> }\n> >>\n> >> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> >> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)\n> >> {\n> >> \tASSERT(activeCamera_);\n> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> >> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> >> \ttryCompleteRequest(info);\n> >> }\n> >>\n> >> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> >> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)\n> >> {\n> >> \tASSERT(activeCamera_);\n> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> >> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> >> \tif (data->frame_ <= buffer->metadata().sequence)\n> >> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> >>\n> >> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> >> -\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> >> +\tdata->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> >> +\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> >> }\n> >>\n> >> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id D4A7BC31E9\n\tfor ;\n\tWed, 30 Oct 2024 08:32:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9C13E65399;\n\tWed, 30 Oct 2024 09:32:49 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 63FB260365\n\tfor ;\n\tWed, 30 Oct 2024 09:32:47 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 36073A8F;\n\tWed, 30 Oct 2024 09:32:44 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"YKHfDii9\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730277164;\n\tbh=qPR3MAHeKQHtEA9itiWVQ9Bcc7KXKfrFASOb9Il4FHA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YKHfDii9AHOABY2B0GSDciTqQlptqwqyh+fLrCWVi1zg8FTv1BdfVoaRLt1uUnah6\n\tC4ffa1tbxQvxsx8T75omWJEzZOHYkMDoenhRV/eNiUS+F3wSpRnLTft9U3GPEvNyyz\n\tKWfKJ4zCeqhTI40fQae0ONVJKLIJeXaCdUOKrO68=","Date":"Wed, 30 Oct 2024 09:32:44 +0100","From":"Jacopo Mondi ","To":"Milan Zamazal ","Cc":"Jacopo Mondi , \n\tlibcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","Message-ID":"","References":"<20241028091249.16473-1-jacopo.mondi@ideasonboard.com>\n\t<87zfmnntuq.fsf@redhat.com> <87v7xbntqh.fsf@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<87v7xbntqh.fsf@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31967,"web_url":"https://patchwork.libcamera.org/comment/31967/","msgid":"<87zfmm7wxk.fsf@redhat.com>","date":"2024-10-30T09:38:15","subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","submitter":{"id":177,"url":"https://patchwork.libcamera.org/api/people/177/","name":"Milan Zamazal","email":"mzamazal@redhat.com"},"content":"Jacopo Mondi writes:\n\n> Hi Milan\n> thanks for reviw\n>\n> On Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:\n>> Milan Zamazal writes:\n>>\n>> > Hi Jacopo,\n>> >\n>> > thank you for the patch. As somebody who came to libcamera and had to\n>> > understand that stuff, I can confirm the original terminology is quite\n>> > confusing and your patch is a good improvement.\n>>\n>> (And now the question is who would make similar changes to the other\n>> pipelines; I can take care of `simple'.)\n>>\n>\n> Well, the first one who gets there and is annoyed by the existing\n> names I would say :)\n>\n> I only count vimc and ipu3, soft-isp apart.\n>\n> Let's start with this and simple if you like, then we'll get to the\n> other ones eventually ? \n\nI'd say this would be generally easier.\n\n> After all, the IPA interface names doesn't need to be standardized\n> across all platforms.\n>\n> On the other hand, true, for consistency I should rather bite the\n> bullet and convert all of them in one go ?\n>\n>> > Jacopo Mondi writes:\n>> >\n>> >> The names used by the IPA interface and the names used for buffer\n>> >> completions handlers for the RkISP1 clash in the use of the term \"buffer\".\n>> >>\n>> >> For example the video device buffer completion handler is called\n>> >> \"bufferReady\" and the IPA event to ask the IPA to compute parameters is\n>> >> called \"fillParamsBuffers\". This makes it hard to recognize which\n>> >> function handles video device completion signals and which ones handle\n>> >> the IPA interface events.\n>> >>\n>> >> Rationalize the naming scheme in the IPA interface function and events\n>> >> and the signal handlers in the RkISP1 support, according to the\n>> >> following table. Remove the name \"buffer\" from the IPA interface events\n>> >> and events handler and reserve it for the buffer completion handlers.\n>> >> Rename the IPA interface events and function to use the 'params' and\n>> >> 'stats' names and make the signal and the even names specular (ie\n>> >> 'computeParams' : 'paramsComputed')\n>> >>\n>> >> IPA Interface:\n>> >>\n>> >> - fillParamsBuffer -> computeParams [FUNCTION]\n>> >> - paramsBufferReady -> paramsComputed [EVENT]\n>> >>\n>> >> - processStatsBuffer -> processStats [FUNCTION]\n>> >> - metadataReady -> statsProcessed [EVENT]\n>> >>\n>> >> Pipeline handler:\n>> >>\n>> >> - bufferReady -> videoBufferReady [BUFFER HANDLER]\n>> >> - paramReady -> paramsBufferReady [BUFFER HANDLER]\n>> >> - statReady -> statsBufferReady [BUFFER HANDLER]\n>> >> - paramFilled -> paramsComputed [IPA EVENT HANDLER]\n>> >> - metadataReady -> statsProcessed [IPA EVENT HANDLER]\n>> >>\n>> >> Cosmetic change only, no functional changes intended.\n>> >>\n>> >> Signed-off-by: Jacopo Mondi \n>> >\n>> > Reviewed-by: Milan Zamazal \n>> >\n>> >> ---\n>> >> include/libcamera/ipa/rkisp1.mojom | 10 +++---\n>> >> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----\n>> >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------\n>> >> 3 files changed, 34 insertions(+), 34 deletions(-)\n>> >>\n>> >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n>> >> index 80d54a03aa90..ced1526e18cd 100644\n>> >> --- a/include/libcamera/ipa/rkisp1.mojom\n>> >> +++ b/include/libcamera/ipa/rkisp1.mojom\n>> >> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {\n>> >> \tunmapBuffers(array ids);\n>> >>\n>> >> \t[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);\n>> >> -\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n>> >> -\t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n>> >> -\t\t\t\t libcamera.ControlList sensorControls);\n>> >> +\t[async] computeParams(uint32 frame, uint32 bufferId);\n>> >> +\t[async] processStats(uint32 frame, uint32 bufferId,\n>> >> +\t\t\t libcamera.ControlList sensorControls);\n>> >> };\n>> >>\n>> >> interface IPARkISP1EventInterface {\n>> >> -\tparamsBufferReady(uint32 frame, uint32 bytesused);\n>> >> +\tparamsComputed(uint32 frame, uint32 bytesused);\n>> >> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n>> >> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n>> >> +\tstatsProcessed(uint32 frame, libcamera.ControlList metadata);\n>> >> };\n>> >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n>> >> index 9e161cabdea4..6d6f8392a70f 100644\n>> >> --- a/src/ipa/rkisp1/rkisp1.cpp\n>> >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n>> >> @@ -65,9 +65,9 @@ public:\n>> >> \tvoid unmapBuffers(const std::vector &ids) override;\n>> >>\n>> >> \tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n>> >> -\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n>> >> -\tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n>> >> -\t\t\t\tconst ControlList &sensorControls) override;\n>> >> +\tvoid computeParams(const uint32_t frame, const uint32_t bufferId) override;\n>> >> +\tvoid processStats(const uint32_t frame, const uint32_t bufferId,\n>> >> +\t\t\t const ControlList &sensorControls) override;\n>> >>\n>> >> protected:\n>> >> \tstd::string logPrefix() const override;\n>> >> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n>> >> \t}\n>> >> }\n>> >>\n>> >> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> >> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n>> >> {\n>> >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>> >>\n>> >> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n>> >> \tfor (auto const &algo : algorithms())\n>> >> \t\talgo->prepare(context_, frame, frameContext, ¶ms);\n>> >>\n>> >> -\tparamsBufferReady.emit(frame, params.size());\n>> >> +\tparamsComputed.emit(frame, params.size());\n>> >> }\n>> >>\n>> >> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n>> >> -\t\t\t\t const ControlList &sensorControls)\n>> >> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n>> >> +\t\t\t const ControlList &sensorControls)\n>> >> {\n>> >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n>> >>\n>> >> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n>> >>\n>> >> \tsetControls(frame);\n>> >>\n>> >> -\tmetadataReady.emit(frame, metadata);\n>> >> +\tstatsProcessed.emit(frame, metadata);\n>> >> }\n>> >>\n>> >> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n>> >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> >> index c7b0b3927da1..83b74b27652f 100644\n>> >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>> >> @@ -114,11 +114,11 @@ public:\n>> >> \tControlInfoMap ipaControls_;\n>> >>\n>> >> private:\n>> >> -\tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n>> >> +\tvoid paramsComputed(unsigned int frame, unsigned int bytesused);\n>> >> \tvoid setSensorControls(unsigned int frame,\n>> >> \t\t\t const ControlList &sensorControls);\n>> >>\n>> >> -\tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n>> >> +\tvoid statsProcessed(unsigned int frame, const ControlList &metadata);\n>> >> };\n>> >>\n>> >> class RkISP1CameraConfiguration : public CameraConfiguration\n>> >> @@ -180,9 +180,9 @@ private:\n>> >> \t\t const RkISP1CameraConfiguration &config);\n>> >> \tint createCamera(MediaEntity *sensor);\n>> >> \tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n>> >> -\tvoid bufferReady(FrameBuffer *buffer);\n>> >> -\tvoid paramReady(FrameBuffer *buffer);\n>> >> -\tvoid statReady(FrameBuffer *buffer);\n>> >> +\tvoid videoBufferReady(FrameBuffer *buffer);\n>> >> +\tvoid paramsBufferReady(FrameBuffer *buffer);\n>> >> +\tvoid statsBufferReady(FrameBuffer *buffer);\n>> >> \tvoid dewarpBufferReady(FrameBuffer *buffer);\n>> >> \tvoid frameStart(uint32_t sequence);\n>> >>\n>> >> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> >> \t\treturn -ENOENT;\n>> >>\n>> >> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n>> >> -\tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n>> >> -\tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n>> >> +\tipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);\n>> >> +\tipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);\n>> >>\n>> >> \t/*\n>> >> \t * The API tuning file is made from the sensor name unless the\n>> >> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n>> >> \treturn 0;\n>> >> }\n>> >>\n>> >> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n>> >> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n>> >> {\n>> >> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n>> >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n>> >> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n>> >> \tdelayedCtrls_->push(sensorControls);\n>> >> }\n>> >>\n>> >> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n>> >> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)\n>> >> {\n>> >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n>> >> \tif (!info)\n>> >> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>> >> \t\tif (data->selfPath_ && info->selfPathBuffer)\n>> >> \t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n>> >> \t} else {\n>> >> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n>> >> -\t\t\t\t\t info->paramBuffer->cookie());\n>> >> +\t\tdata->ipa_->computeParams(data->frame_,\n>> >> +\t\t\t\t\t info->paramBuffer->cookie());\n>> >> \t}\n>> >>\n>> >> \tdata->frame_++;\n>> >> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n>> >> \tif (hasSelfPath_ && !selfPath_.init(media_))\n>> >> \t\treturn false;\n>> >>\n>> >> -\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n>> >> +\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n>> >> \tif (hasSelfPath_)\n>> >> -\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n>> >> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n>> >> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n>> >> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n>> >> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);\n>> >> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);\n>> >>\n>> >> \t/* If dewarper is present, create its instance. */\n>> >> \tDeviceMatch dwp(\"dw100\");\n>> >> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n>> >> \tcompleteRequest(request);\n>> >> }\n>> >>\n>> >> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> >> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)\n>> >> {\n>> >> \tASSERT(activeCamera_);\n>> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> >> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>> >> \t\tif (isRaw_) {\n>> >> \t\t\tconst ControlList &ctrls =\n>> >> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n>> >> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n>> >> +\t\t\tdata->ipa_->processStats(info->frame, 0, ctrls);\n>> >> \t\t}\n>> >> \t} else {\n>> >> \t\tif (isRaw_)\n>> >> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n>> >> \ttryCompleteRequest(info);\n>> >> }\n>> >>\n>> >> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n>> >> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)\n>> >> {\n>> >> \tASSERT(activeCamera_);\n>> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> >> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n>> >> \ttryCompleteRequest(info);\n>> >> }\n>> >>\n>> >> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>> >> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)\n>> >> {\n>> >> \tASSERT(activeCamera_);\n>> >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n>> >> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>> >> \tif (data->frame_ <= buffer->metadata().sequence)\n>> >> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n>> >>\n>> >> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n>> >> -\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n>> >> +\tdata->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n>> >> +\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n>> >> }\n>> >>\n>> >> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n>>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 2720EC3237\n\tfor ;\n\tWed, 30 Oct 2024 09:38:31 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADD7A65399;\n\tWed, 30 Oct 2024 10:38:29 +0100 (CET)","from us-smtp-delivery-124.mimecast.com\n\t(us-smtp-delivery-124.mimecast.com [170.10.129.124])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8F9E60365\n\tfor ;\n\tWed, 30 Oct 2024 10:38:27 +0100 (CET)","from mail-ej1-f70.google.com (mail-ej1-f70.google.com\n\t[209.85.218.70]) by relay.mimecast.com with ESMTP with STARTTLS\n\t(version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id\n\tus-mta-497-CpzCCQZrMk-_n4Dd-xZbRw-1; Wed, 30 Oct 2024 05:38:25 -0400","by mail-ej1-f70.google.com with SMTP id\n\ta640c23a62f3a-a9a004bfc1cso405754466b.1\n\tfor ;\n\tWed, 30 Oct 2024 02:38:25 -0700 (PDT)","from nuthatch (ip-77-48-47-2.net.vodafone.cz. [77.48.47.2])\n\tby smtp.gmail.com with ESMTPSA id\n\ta640c23a62f3a-a9b3a0881d4sm552781166b.216.2024.10.30.02.38.16\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 30 Oct 2024 02:38:16 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=redhat.com header.i=@redhat.com\n\theader.b=\"MF6XT6wn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;\n\ts=mimecast20190719; t=1730281106;\n\th=from:from:reply-to:subject:subject:date:date:message-id:message-id:\n\tto:to:cc:cc:mime-version:mime-version:content-type:content-type:\n\tin-reply-to:in-reply-to:references:references;\n\tbh=gzD41x3/16NZ+BarUNBSMu2zT2RIyfIExaEm2IcxN48=;\n\tb=MF6XT6wn8JognnJz1ZV6NT1fadYNVId1t9EjhaTzf9ZqlCzVbsDVAkujG5bxdZ+LP1Qmte\n\t75RllRxlu78tXroBQpVIUP/Ck4VYofAwaGR2/IBHGM+lB2/7xbDBwC+pqhqywVrNNbbqKH\n\t+qXCBSQiIt6lArTevNr1M0jDxEURDQ8=","X-MC-Unique":"CpzCCQZrMk-_n4Dd-xZbRw-1","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1730281104; x=1730885904;\n\th=mime-version:user-agent:message-id:date:references:in-reply-to\n\t:subject:cc:to:from:x-gm-message-state:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=gzD41x3/16NZ+BarUNBSMu2zT2RIyfIExaEm2IcxN48=;\n\tb=Q+u/A3BL40eMyBBhPMokpBfuuTSVHAR3E7CnMh6e8LZPMxKgEVrw6iXa/avjWPvsM+\n\tcA0t0YkdM7RGXPUrN/+PkZFz6a2xL6ksxOO50UNJUnE6v2keDRqrd/5rToz5WcT6ME5D\n\tNz9vpuD1UMFSekKTqocwc/jBac+7aNYmJr1beDo1mRZVx5wwUHxm1ecwH8nh6vp7LaPc\n\thmyrV0cBVHee55pLJCpFgotK3g5qrEplLRto4HyNroS2PUuv39SvCLWGpqScBj80WhBt\n\tpu+URUdYVPElXZMpKexrnLASH0ah/kTwU0aK9KTuJI7brAxbZ/0BdLb+A7PwoQcBNO8o\n\tdFnA==","X-Gm-Message-State":"AOJu0YzpJHWoF2ldsAppI1A0mJAGzNe2ewe5GTPM6Qs9GA2Wm4NuAgHe\n\t+IGurZ32pW5k7JzePBz3MedEsdpfLJIjbi3OIcAO6tqTqkMdq1ttKn/60r8w3HY2lFHZMYrj4ZN\n\tSun0f5pdrf2n3Q8PhfwM5oc2Z2nFkqQSasXicfRWglJDVObRPgMxSntfgN7kiIUhOSB0qrEWptW\n\tdttdZqw50grdUc5DSEctpa/9/pcAnr+QzG1WglPapOvE3qjNA0wnNZaFE=","X-Received":["by 2002:a17:907:9281:b0:a99:f283:8147 with SMTP id\n\ta640c23a62f3a-a9de5f49d74mr1321436566b.27.1730281100348; \n\tWed, 30 Oct 2024 02:38:20 -0700 (PDT)","by 2002:a17:907:9281:b0:a99:f283:8147 with SMTP id\n\ta640c23a62f3a-a9de5f49d74mr1321419266b.27.1730281097392; \n\tWed, 30 Oct 2024 02:38:17 -0700 (PDT)"],"X-Google-Smtp-Source":"AGHT+IFncHQQS7ji/fSehrKWpUrEMIIv76JPvaNkEdIH6NAwnofK8lTT1ZvJaPgkDHEWpMV0ZuWBOg==","From":"Milan Zamazal ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","In-Reply-To":"\n\t(Jacopo Mondi's message of \"Wed, 30 Oct 2024 09:32:44 +0100\")","References":"<20241028091249.16473-1-jacopo.mondi@ideasonboard.com>\n\t<87zfmnntuq.fsf@redhat.com> <87v7xbntqh.fsf@redhat.com>\n\t","Date":"Wed, 30 Oct 2024 10:38:15 +0100","Message-ID":"<87zfmm7wxk.fsf@redhat.com>","User-Agent":"Gnus/5.13 (Gnus v5.13)","MIME-Version":"1.0","X-Mimecast-Spam-Score":"0","X-Mimecast-Originator":"redhat.com","Content-Type":"text/plain","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31968,"web_url":"https://patchwork.libcamera.org/comment/31968/","msgid":"<20241030095224.GU22600@pendragon.ideasonboard.com>","date":"2024-10-30T09:52:24","subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Wed, Oct 30, 2024 at 09:32:44AM +0100, Jacopo Mondi wrote:\n> Hi Milan\n> thanks for reviw\n> \n> On Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:\n> > Milan Zamazal writes:\n> >\n> > > Hi Jacopo,\n> > >\n> > > thank you for the patch. As somebody who came to libcamera and had to\n> > > understand that stuff, I can confirm the original terminology is quite\n> > > confusing and your patch is a good improvement.\n> >\n> > (And now the question is who would make similar changes to the other\n> > pipelines; I can take care of `simple'.)\n> >\n> \n> Well, the first one who gets there and is annoyed by the existing\n> names I would say :)\n> \n> I only count vimc and ipu3, soft-isp apart.\n> \n> Let's start with this and simple if you like, then we'll get to the\n> other ones eventually ? After all, the IPA interface names doesn't\n> need to be standardized across all platforms.\n> \n> On the other hand, true, for consistency I should rather bite the\n> bullet and convert all of them in one go ?\n\nIt should be easy to convert the other pipeline handlers, given that\nonly the first one should be subject to bikeshedding of the names. I\nwould prefer addressing them all in one go.\n\n> > > Jacopo Mondi writes:\n> > >\n> > >> The names used by the IPA interface and the names used for buffer\n> > >> completions handlers for the RkISP1 clash in the use of the term \"buffer\".\n> > >>\n> > >> For example the video device buffer completion handler is called\n> > >> \"bufferReady\" and the IPA event to ask the IPA to compute parameters is\n> > >> called \"fillParamsBuffers\". This makes it hard to recognize which\n> > >> function handles video device completion signals and which ones handle\n> > >> the IPA interface events.\n> > >>\n> > >> Rationalize the naming scheme in the IPA interface function and events\n> > >> and the signal handlers in the RkISP1 support, according to the\n> > >> following table. Remove the name \"buffer\" from the IPA interface events\n> > >> and events handler and reserve it for the buffer completion handlers.\n> > >> Rename the IPA interface events and function to use the 'params' and\n> > >> 'stats' names and make the signal and the even names specular (ie\n> > >> 'computeParams' : 'paramsComputed')\n> > >>\n> > >> IPA Interface:\n> > >>\n> > >> - fillParamsBuffer -> computeParams [FUNCTION]\n> > >> - paramsBufferReady -> paramsComputed [EVENT]\n> > >>\n> > >> - processStatsBuffer -> processStats [FUNCTION]\n> > >> - metadataReady -> statsProcessed [EVENT]\n\nThis is the only change I don't like. The metadataReady event is really\nabout signalling that metadata is ready, not that stats have been\nprocessed.\n\n> > >>\n> > >> Pipeline handler:\n> > >>\n> > >> - bufferReady -> videoBufferReady [BUFFER HANDLER]\n\nI think we use the term \"image\" instead of \"video\" preferentially.\n\n> > >> - paramReady -> paramsBufferReady [BUFFER HANDLER]\n> > >> - statReady -> statsBufferReady [BUFFER HANDLER]\n> > >> - paramFilled -> paramsComputed [IPA EVENT HANDLER]\n> > >> - metadataReady -> statsProcessed [IPA EVENT HANDLER]\n\nThis depends on if we rename the metadataReady event, and what we rename\nit to.\n\nThe rest looks good.\n\n> > >>\n> > >> Cosmetic change only, no functional changes intended.\n> > >>\n> > >> Signed-off-by: Jacopo Mondi \n> > >\n> > > Reviewed-by: Milan Zamazal \n> > >\n> > >> ---\n> > >> include/libcamera/ipa/rkisp1.mojom | 10 +++---\n> > >> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----\n> > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------\n> > >> 3 files changed, 34 insertions(+), 34 deletions(-)\n> > >>\n> > >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > >> index 80d54a03aa90..ced1526e18cd 100644\n> > >> --- a/include/libcamera/ipa/rkisp1.mojom\n> > >> +++ b/include/libcamera/ipa/rkisp1.mojom\n> > >> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {\n> > >> \tunmapBuffers(array ids);\n> > >>\n> > >> \t[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);\n> > >> -\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > >> -\t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n> > >> -\t\t\t\t libcamera.ControlList sensorControls);\n> > >> +\t[async] computeParams(uint32 frame, uint32 bufferId);\n> > >> +\t[async] processStats(uint32 frame, uint32 bufferId,\n> > >> +\t\t\t libcamera.ControlList sensorControls);\n> > >> };\n> > >>\n> > >> interface IPARkISP1EventInterface {\n> > >> -\tparamsBufferReady(uint32 frame, uint32 bytesused);\n> > >> +\tparamsComputed(uint32 frame, uint32 bytesused);\n> > >> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > >> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> > >> +\tstatsProcessed(uint32 frame, libcamera.ControlList metadata);\n> > >> };\n> > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > >> index 9e161cabdea4..6d6f8392a70f 100644\n> > >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> > >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > >> @@ -65,9 +65,9 @@ public:\n> > >> \tvoid unmapBuffers(const std::vector &ids) override;\n> > >>\n> > >> \tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > >> -\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > >> -\tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> > >> -\t\t\t\tconst ControlList &sensorControls) override;\n> > >> +\tvoid computeParams(const uint32_t frame, const uint32_t bufferId) override;\n> > >> +\tvoid processStats(const uint32_t frame, const uint32_t bufferId,\n> > >> +\t\t\t const ControlList &sensorControls) override;\n> > >>\n> > >> protected:\n> > >> \tstd::string logPrefix() const override;\n> > >> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> > >> \t}\n> > >> }\n> > >>\n> > >> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > >> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n> > >> {\n> > >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >>\n> > >> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > >> \tfor (auto const &algo : algorithms())\n> > >> \t\talgo->prepare(context_, frame, frameContext, ¶ms);\n> > >>\n> > >> -\tparamsBufferReady.emit(frame, params.size());\n> > >> +\tparamsComputed.emit(frame, params.size());\n> > >> }\n> > >>\n> > >> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> > >> -\t\t\t\t const ControlList &sensorControls)\n> > >> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> > >> +\t\t\t const ControlList &sensorControls)\n> > >> {\n> > >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > >>\n> > >> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > >>\n> > >> \tsetControls(frame);\n> > >>\n> > >> -\tmetadataReady.emit(frame, metadata);\n> > >> +\tstatsProcessed.emit(frame, metadata);\n> > >> }\n> > >>\n> > >> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> index c7b0b3927da1..83b74b27652f 100644\n> > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > >> @@ -114,11 +114,11 @@ public:\n> > >> \tControlInfoMap ipaControls_;\n> > >>\n> > >> private:\n> > >> -\tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> > >> +\tvoid paramsComputed(unsigned int frame, unsigned int bytesused);\n> > >> \tvoid setSensorControls(unsigned int frame,\n> > >> \t\t\t const ControlList &sensorControls);\n> > >>\n> > >> -\tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n> > >> +\tvoid statsProcessed(unsigned int frame, const ControlList &metadata);\n> > >> };\n> > >>\n> > >> class RkISP1CameraConfiguration : public CameraConfiguration\n> > >> @@ -180,9 +180,9 @@ private:\n> > >> \t\t const RkISP1CameraConfiguration &config);\n> > >> \tint createCamera(MediaEntity *sensor);\n> > >> \tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> > >> -\tvoid bufferReady(FrameBuffer *buffer);\n> > >> -\tvoid paramReady(FrameBuffer *buffer);\n> > >> -\tvoid statReady(FrameBuffer *buffer);\n> > >> +\tvoid videoBufferReady(FrameBuffer *buffer);\n> > >> +\tvoid paramsBufferReady(FrameBuffer *buffer);\n> > >> +\tvoid statsBufferReady(FrameBuffer *buffer);\n> > >> \tvoid dewarpBufferReady(FrameBuffer *buffer);\n> > >> \tvoid frameStart(uint32_t sequence);\n> > >>\n> > >> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > >> \t\treturn -ENOENT;\n> > >>\n> > >> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n> > >> -\tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n> > >> -\tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n> > >> +\tipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);\n> > >> +\tipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);\n> > >>\n> > >> \t/*\n> > >> \t * The API tuning file is made from the sensor name unless the\n> > >> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > >> \treturn 0;\n> > >> }\n> > >>\n> > >> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > >> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n> > >> {\n> > >> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> > >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > >> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > >> \tdelayedCtrls_->push(sensorControls);\n> > >> }\n> > >>\n> > >> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > >> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)\n> > >> {\n> > >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > >> \tif (!info)\n> > >> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > >> \t\tif (data->selfPath_ && info->selfPathBuffer)\n> > >> \t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> > >> \t} else {\n> > >> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> > >> -\t\t\t\t\t info->paramBuffer->cookie());\n> > >> +\t\tdata->ipa_->computeParams(data->frame_,\n> > >> +\t\t\t\t\t info->paramBuffer->cookie());\n> > >> \t}\n> > >>\n> > >> \tdata->frame_++;\n> > >> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > >> \tif (hasSelfPath_ && !selfPath_.init(media_))\n> > >> \t\treturn false;\n> > >>\n> > >> -\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > >> +\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> > >> \tif (hasSelfPath_)\n> > >> -\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > >> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > >> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > >> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> > >> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);\n> > >> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);\n> > >>\n> > >> \t/* If dewarper is present, create its instance. */\n> > >> \tDeviceMatch dwp(\"dw100\");\n> > >> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > >> \tcompleteRequest(request);\n> > >> }\n> > >>\n> > >> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > >> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)\n> > >> {\n> > >> \tASSERT(activeCamera_);\n> > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > >> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > >> \t\tif (isRaw_) {\n> > >> \t\t\tconst ControlList &ctrls =\n> > >> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> > >> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > >> +\t\t\tdata->ipa_->processStats(info->frame, 0, ctrls);\n> > >> \t\t}\n> > >> \t} else {\n> > >> \t\tif (isRaw_)\n> > >> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> > >> \ttryCompleteRequest(info);\n> > >> }\n> > >>\n> > >> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> > >> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)\n> > >> {\n> > >> \tASSERT(activeCamera_);\n> > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > >> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> > >> \ttryCompleteRequest(info);\n> > >> }\n> > >>\n> > >> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > >> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)\n> > >> {\n> > >> \tASSERT(activeCamera_);\n> > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > >> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > >> \tif (data->frame_ <= buffer->metadata().sequence)\n> > >> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> > >>\n> > >> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > >> -\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> > >> +\tdata->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> > >> +\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> > >> }\n> > >>\n> > >> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EBFE7C31E9\n\tfor ;\n\tWed, 30 Oct 2024 09:52:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F32C065399;\n\tWed, 30 Oct 2024 10:52:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 140D360365\n\tfor ;\n\tWed, 30 Oct 2024 10:52:32 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 54C90B7E;\n\tWed, 30 Oct 2024 10:52:28 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"tA/FRsPt\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730281948;\n\tbh=lVffG6JNNpQU2QH5/J2K+U0n3mcMyPexjT0DhUFlrGo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tA/FRsPtmC9FS7pte9gT04Kj2m88gwKQQF8YeMn8h2UhGReoRREvnOaZDbyteOJtF\n\tZVeyqK1OLnHBrBuZ3JVskIiHjBmG8F38zWBoUXdah6tS75KRGQKpVP2bvffxkwTxdi\n\t7iOfPEgC2m9QN0cNQHh8BJuRMcGYgSRlfUw+JuLE=","Date":"Wed, 30 Oct 2024 11:52:24 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","Message-ID":"<20241030095224.GU22600@pendragon.ideasonboard.com>","References":"<20241028091249.16473-1-jacopo.mondi@ideasonboard.com>\n\t<87zfmnntuq.fsf@redhat.com> <87v7xbntqh.fsf@redhat.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":31969,"web_url":"https://patchwork.libcamera.org/comment/31969/","msgid":"","date":"2024-10-30T10:48:48","subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Wed, Oct 30, 2024 at 11:52:24AM +0200, Laurent Pinchart wrote:\n> On Wed, Oct 30, 2024 at 09:32:44AM +0100, Jacopo Mondi wrote:\n> > Hi Milan\n> > thanks for reviw\n> >\n> > On Tue, Oct 29, 2024 at 10:28:22AM +0100, Milan Zamazal wrote:\n> > > Milan Zamazal writes:\n> > >\n> > > > Hi Jacopo,\n> > > >\n> > > > thank you for the patch. As somebody who came to libcamera and had to\n> > > > understand that stuff, I can confirm the original terminology is quite\n> > > > confusing and your patch is a good improvement.\n> > >\n> > > (And now the question is who would make similar changes to the other\n> > > pipelines; I can take care of `simple'.)\n> > >\n> >\n> > Well, the first one who gets there and is annoyed by the existing\n> > names I would say :)\n> >\n> > I only count vimc and ipu3, soft-isp apart.\n> >\n> > Let's start with this and simple if you like, then we'll get to the\n> > other ones eventually ? After all, the IPA interface names doesn't\n> > need to be standardized across all platforms.\n> >\n> > On the other hand, true, for consistency I should rather bite the\n> > bullet and convert all of them in one go ?\n>\n> It should be easy to convert the other pipeline handlers, given that\n> only the first one should be subject to bikeshedding of the names. I\n> would prefer addressing them all in one go.\n>\n\nI knew I was going to ask for troubles :)\n\n> > > > Jacopo Mondi writes:\n> > > >\n> > > >> The names used by the IPA interface and the names used for buffer\n> > > >> completions handlers for the RkISP1 clash in the use of the term \"buffer\".\n> > > >>\n> > > >> For example the video device buffer completion handler is called\n> > > >> \"bufferReady\" and the IPA event to ask the IPA to compute parameters is\n> > > >> called \"fillParamsBuffers\". This makes it hard to recognize which\n> > > >> function handles video device completion signals and which ones handle\n> > > >> the IPA interface events.\n> > > >>\n> > > >> Rationalize the naming scheme in the IPA interface function and events\n> > > >> and the signal handlers in the RkISP1 support, according to the\n> > > >> following table. Remove the name \"buffer\" from the IPA interface events\n> > > >> and events handler and reserve it for the buffer completion handlers.\n> > > >> Rename the IPA interface events and function to use the 'params' and\n> > > >> 'stats' names and make the signal and the even names specular (ie\n> > > >> 'computeParams' : 'paramsComputed')\n> > > >>\n> > > >> IPA Interface:\n> > > >>\n> > > >> - fillParamsBuffer -> computeParams [FUNCTION]\n> > > >> - paramsBufferReady -> paramsComputed [EVENT]\n> > > >>\n> > > >> - processStatsBuffer -> processStats [FUNCTION]\n> > > >> - metadataReady -> statsProcessed [EVENT]\n>\n> This is the only change I don't like. The metadataReady event is really\n> about signalling that metadata is ready, not that stats have been\n> processed.\n>\n\nI value symmetry as it makes things easier to follow, but I would be\nfine with keeping metadataReady if that's preferred\n\n> > > >>\n> > > >> Pipeline handler:\n> > > >>\n> > > >> - bufferReady -> videoBufferReady [BUFFER HANDLER]\n>\n> I think we use the term \"image\" instead of \"video\" preferentially.\n>\n\nThat would work too\n\n> > > >> - paramReady -> paramsBufferReady [BUFFER HANDLER]\n> > > >> - statReady -> statsBufferReady [BUFFER HANDLER]\n> > > >> - paramFilled -> paramsComputed [IPA EVENT HANDLER]\n> > > >> - metadataReady -> statsProcessed [IPA EVENT HANDLER]\n>\n> This depends on if we rename the metadataReady event, and what we rename\n> it to.\n>\n> The rest looks good.\n>\n\nI can keep metadataReady if it makes easier to land this patch :)\n\n> > > >>\n> > > >> Cosmetic change only, no functional changes intended.\n> > > >>\n> > > >> Signed-off-by: Jacopo Mondi \n> > > >\n> > > > Reviewed-by: Milan Zamazal \n> > > >\n> > > >> ---\n> > > >> include/libcamera/ipa/rkisp1.mojom | 10 +++---\n> > > >> src/ipa/rkisp1/rkisp1.cpp | 16 ++++-----\n> > > >> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 42 ++++++++++++------------\n> > > >> 3 files changed, 34 insertions(+), 34 deletions(-)\n> > > >>\n> > > >> diff --git a/include/libcamera/ipa/rkisp1.mojom b/include/libcamera/ipa/rkisp1.mojom\n> > > >> index 80d54a03aa90..ced1526e18cd 100644\n> > > >> --- a/include/libcamera/ipa/rkisp1.mojom\n> > > >> +++ b/include/libcamera/ipa/rkisp1.mojom\n> > > >> @@ -31,13 +31,13 @@ interface IPARkISP1Interface {\n> > > >> \tunmapBuffers(array ids);\n> > > >>\n> > > >> \t[async] queueRequest(uint32 frame, libcamera.ControlList reqControls);\n> > > >> -\t[async] fillParamsBuffer(uint32 frame, uint32 bufferId);\n> > > >> -\t[async] processStatsBuffer(uint32 frame, uint32 bufferId,\n> > > >> -\t\t\t\t libcamera.ControlList sensorControls);\n> > > >> +\t[async] computeParams(uint32 frame, uint32 bufferId);\n> > > >> +\t[async] processStats(uint32 frame, uint32 bufferId,\n> > > >> +\t\t\t libcamera.ControlList sensorControls);\n> > > >> };\n> > > >>\n> > > >> interface IPARkISP1EventInterface {\n> > > >> -\tparamsBufferReady(uint32 frame, uint32 bytesused);\n> > > >> +\tparamsComputed(uint32 frame, uint32 bytesused);\n> > > >> \tsetSensorControls(uint32 frame, libcamera.ControlList sensorControls);\n> > > >> -\tmetadataReady(uint32 frame, libcamera.ControlList metadata);\n> > > >> +\tstatsProcessed(uint32 frame, libcamera.ControlList metadata);\n> > > >> };\n> > > >> diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp\n> > > >> index 9e161cabdea4..6d6f8392a70f 100644\n> > > >> --- a/src/ipa/rkisp1/rkisp1.cpp\n> > > >> +++ b/src/ipa/rkisp1/rkisp1.cpp\n> > > >> @@ -65,9 +65,9 @@ public:\n> > > >> \tvoid unmapBuffers(const std::vector &ids) override;\n> > > >>\n> > > >> \tvoid queueRequest(const uint32_t frame, const ControlList &controls) override;\n> > > >> -\tvoid fillParamsBuffer(const uint32_t frame, const uint32_t bufferId) override;\n> > > >> -\tvoid processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> > > >> -\t\t\t\tconst ControlList &sensorControls) override;\n> > > >> +\tvoid computeParams(const uint32_t frame, const uint32_t bufferId) override;\n> > > >> +\tvoid processStats(const uint32_t frame, const uint32_t bufferId,\n> > > >> +\t\t\t const ControlList &sensorControls) override;\n> > > >>\n> > > >> protected:\n> > > >> \tstd::string logPrefix() const override;\n> > > >> @@ -335,7 +335,7 @@ void IPARkISP1::queueRequest(const uint32_t frame, const ControlList &controls)\n> > > >> \t}\n> > > >> }\n> > > >>\n> > > >> -void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > > >> +void IPARkISP1::computeParams(const uint32_t frame, const uint32_t bufferId)\n> > > >> {\n> > > >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > >>\n> > > >> @@ -345,11 +345,11 @@ void IPARkISP1::fillParamsBuffer(const uint32_t frame, const uint32_t bufferId)\n> > > >> \tfor (auto const &algo : algorithms())\n> > > >> \t\talgo->prepare(context_, frame, frameContext, ¶ms);\n> > > >>\n> > > >> -\tparamsBufferReady.emit(frame, params.size());\n> > > >> +\tparamsComputed.emit(frame, params.size());\n> > > >> }\n> > > >>\n> > > >> -void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId,\n> > > >> -\t\t\t\t const ControlList &sensorControls)\n> > > >> +void IPARkISP1::processStats(const uint32_t frame, const uint32_t bufferId,\n> > > >> +\t\t\t const ControlList &sensorControls)\n> > > >> {\n> > > >> \tIPAFrameContext &frameContext = context_.frameContexts.get(frame);\n> > > >>\n> > > >> @@ -378,7 +378,7 @@ void IPARkISP1::processStatsBuffer(const uint32_t frame, const uint32_t bufferId\n> > > >>\n> > > >> \tsetControls(frame);\n> > > >>\n> > > >> -\tmetadataReady.emit(frame, metadata);\n> > > >> +\tstatsProcessed.emit(frame, metadata);\n> > > >> }\n> > > >>\n> > > >> void IPARkISP1::updateControls(const IPACameraSensorInfo &sensorInfo,\n> > > >> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> index c7b0b3927da1..83b74b27652f 100644\n> > > >> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > >> @@ -114,11 +114,11 @@ public:\n> > > >> \tControlInfoMap ipaControls_;\n> > > >>\n> > > >> private:\n> > > >> -\tvoid paramFilled(unsigned int frame, unsigned int bytesused);\n> > > >> +\tvoid paramsComputed(unsigned int frame, unsigned int bytesused);\n> > > >> \tvoid setSensorControls(unsigned int frame,\n> > > >> \t\t\t const ControlList &sensorControls);\n> > > >>\n> > > >> -\tvoid metadataReady(unsigned int frame, const ControlList &metadata);\n> > > >> +\tvoid statsProcessed(unsigned int frame, const ControlList &metadata);\n> > > >> };\n> > > >>\n> > > >> class RkISP1CameraConfiguration : public CameraConfiguration\n> > > >> @@ -180,9 +180,9 @@ private:\n> > > >> \t\t const RkISP1CameraConfiguration &config);\n> > > >> \tint createCamera(MediaEntity *sensor);\n> > > >> \tvoid tryCompleteRequest(RkISP1FrameInfo *info);\n> > > >> -\tvoid bufferReady(FrameBuffer *buffer);\n> > > >> -\tvoid paramReady(FrameBuffer *buffer);\n> > > >> -\tvoid statReady(FrameBuffer *buffer);\n> > > >> +\tvoid videoBufferReady(FrameBuffer *buffer);\n> > > >> +\tvoid paramsBufferReady(FrameBuffer *buffer);\n> > > >> +\tvoid statsBufferReady(FrameBuffer *buffer);\n> > > >> \tvoid dewarpBufferReady(FrameBuffer *buffer);\n> > > >> \tvoid frameStart(uint32_t sequence);\n> > > >>\n> > > >> @@ -367,8 +367,8 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > >> \t\treturn -ENOENT;\n> > > >>\n> > > >> \tipa_->setSensorControls.connect(this, &RkISP1CameraData::setSensorControls);\n> > > >> -\tipa_->paramsBufferReady.connect(this, &RkISP1CameraData::paramFilled);\n> > > >> -\tipa_->metadataReady.connect(this, &RkISP1CameraData::metadataReady);\n> > > >> +\tipa_->paramsComputed.connect(this, &RkISP1CameraData::paramsComputed);\n> > > >> +\tipa_->statsProcessed.connect(this, &RkISP1CameraData::statsProcessed);\n> > > >>\n> > > >> \t/*\n> > > >> \t * The API tuning file is made from the sensor name unless the\n> > > >> @@ -400,7 +400,7 @@ int RkISP1CameraData::loadIPA(unsigned int hwRevision)\n> > > >> \treturn 0;\n> > > >> }\n> > > >>\n> > > >> -void RkISP1CameraData::paramFilled(unsigned int frame, unsigned int bytesused)\n> > > >> +void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n> > > >> {\n> > > >> \tPipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> > > >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > >> @@ -424,7 +424,7 @@ void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int frame,\n> > > >> \tdelayedCtrls_->push(sensorControls);\n> > > >> }\n> > > >>\n> > > >> -void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> > > >> +void RkISP1CameraData::statsProcessed(unsigned int frame, const ControlList &metadata)\n> > > >> {\n> > > >> \tRkISP1FrameInfo *info = frameInfo_.find(frame);\n> > > >> \tif (!info)\n> > > >> @@ -1120,8 +1120,8 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > >> \t\tif (data->selfPath_ && info->selfPathBuffer)\n> > > >> \t\t\tdata->selfPath_->queueBuffer(info->selfPathBuffer);\n> > > >> \t} else {\n> > > >> -\t\tdata->ipa_->fillParamsBuffer(data->frame_,\n> > > >> -\t\t\t\t\t info->paramBuffer->cookie());\n> > > >> +\t\tdata->ipa_->computeParams(data->frame_,\n> > > >> +\t\t\t\t\t info->paramBuffer->cookie());\n> > > >> \t}\n> > > >>\n> > > >> \tdata->frame_++;\n> > > >> @@ -1334,11 +1334,11 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> > > >> \tif (hasSelfPath_ && !selfPath_.init(media_))\n> > > >> \t\treturn false;\n> > > >>\n> > > >> -\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > >> +\tmainPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> > > >> \tif (hasSelfPath_)\n> > > >> -\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::bufferReady);\n> > > >> -\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);\n> > > >> -\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);\n> > > >> +\t\tselfPath_.bufferReady().connect(this, &PipelineHandlerRkISP1::videoBufferReady);\n> > > >> +\tstat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statsBufferReady);\n> > > >> +\tparam_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramsBufferReady);\n> > > >>\n> > > >> \t/* If dewarper is present, create its instance. */\n> > > >> \tDeviceMatch dwp(\"dw100\");\n> > > >> @@ -1399,7 +1399,7 @@ void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> > > >> \tcompleteRequest(request);\n> > > >> }\n> > > >>\n> > > >> -void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > >> +void PipelineHandlerRkISP1::videoBufferReady(FrameBuffer *buffer)\n> > > >> {\n> > > >> \tASSERT(activeCamera_);\n> > > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > >> @@ -1424,7 +1424,7 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > >> \t\tif (isRaw_) {\n> > > >> \t\t\tconst ControlList &ctrls =\n> > > >> \t\t\t\tdata->delayedCtrls_->get(metadata.sequence);\n> > > >> -\t\t\tdata->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > >> +\t\t\tdata->ipa_->processStats(info->frame, 0, ctrls);\n> > > >> \t\t}\n> > > >> \t} else {\n> > > >> \t\tif (isRaw_)\n> > > >> @@ -1508,7 +1508,7 @@ void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> > > >> \ttryCompleteRequest(info);\n> > > >> }\n> > > >>\n> > > >> -void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> > > >> +void PipelineHandlerRkISP1::paramsBufferReady(FrameBuffer *buffer)\n> > > >> {\n> > > >> \tASSERT(activeCamera_);\n> > > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > >> @@ -1521,7 +1521,7 @@ void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)\n> > > >> \ttryCompleteRequest(info);\n> > > >> }\n> > > >>\n> > > >> -void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > >> +void PipelineHandlerRkISP1::statsBufferReady(FrameBuffer *buffer)\n> > > >> {\n> > > >> \tASSERT(activeCamera_);\n> > > >> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> > > >> @@ -1539,8 +1539,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > >> \tif (data->frame_ <= buffer->metadata().sequence)\n> > > >> \t\tdata->frame_ = buffer->metadata().sequence + 1;\n> > > >>\n> > > >> -\tdata->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > > >> -\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> > > >> +\tdata->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> > > >> +\t\t\t\t data->delayedCtrls_->get(buffer->metadata().sequence));\n> > > >> }\n> > > >>\n> > > >> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id EC085C3237\n\tfor ;\n\tWed, 30 Oct 2024 10:48:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 970DE65399;\n\tWed, 30 Oct 2024 11:48:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8584260365\n\tfor ;\n\tWed, 30 Oct 2024 11:48:52 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 341F9A8F;\n\tWed, 30 Oct 2024 11:48:49 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"HtftsvVE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1730285329;\n\tbh=YHXXPwU0TUUmj9xd2Z0NlFiCMmqEdzu/G47f5BJ+oH0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=HtftsvVEa4NDmPZfR4Xc4bEcjFwVtmI8hrrn53ho3lF2lFSLA+NAw5pCISEc1d45y\n\tSONpiCpQGAnFngkY7m+mN8ztGOqDNFXsSiNtfyXX9LHQ4JhBm9f/aWQJBq3uRHM8hr\n\tRgoAG2e9/8KLZp2M8B5I/m3mp7b4JJaFTtG3RFRA=","Date":"Wed, 30 Oct 2024 11:48:48 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Jacopo Mondi , \n\tMilan Zamazal , libcamera-devel@lists.libcamera.org","Subject":"Re: [PATCH] libcamera: rkisp1: Rationalize IPA and handlers names","Message-ID":"","References":"<20241028091249.16473-1-jacopo.mondi@ideasonboard.com>\n\t<87zfmnntuq.fsf@redhat.com> <87v7xbntqh.fsf@redhat.com>\n\t\n\t<20241030095224.GU22600@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20241030095224.GU22600@pendragon.ideasonboard.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]