[{"id":28652,"web_url":"https://patchwork.libcamera.org/comment/28652/","msgid":"","date":"2024-02-13T11:31:31","subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Dan\n\nOn Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n> The rkisp1 driver uses a couple of different methods of tracking the\n> number of the frame - standardise them on request->sequence() and\n> remove the frame_ member of RkISP1CameraData which was being used to\n> do the job.\n\nRequest::Private::sequence_ gets assigned to\nCamera::Private::requestSequence_ by the PipelineHandler base class.\n\nCamera::Private::requestSequence_ gets zeroed in\nPipelineHandler::stop()\n\nCamera::Private::requestSequence_ gets incremented in\nPipelineHandler::doQueueRequest()\n\nSo I think this change is legit (and should be done on IPU3 as well if\nit uses the same scheme that was implemented here)\n\nReviewed-by: Jacopo Mondi \n\n>\n> Signed-off-by: Daniel Scally \n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n> 1 file changed, 10 insertions(+), 15 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index 5460dc11..22e553fe 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n> public:\n> \tRkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> \t\t\t RkISP1SelfPath *selfPath)\n> -\t\t: Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> +\t\t: Camera::Private(pipe), frameInfo_(pipe),\n> \t\t mainPath_(mainPath), selfPath_(selfPath)\n> \t{\n> \t}\n> @@ -99,7 +99,6 @@ public:\n> \tStream selfPathStream_;\n> \tstd::unique_ptr sensor_;\n> \tstd::unique_ptr delayedCtrls_;\n> -\tunsigned int frame_;\n> \tstd::vector ipaBuffers_;\n> \tRkISP1Frames frameInfo_;\n>\n> @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> \t\t\t\t bool isRaw)\n> {\n> -\tunsigned int frame = data->frame_;\n> +\tunsigned int frame = request->sequence();\n>\n> \tFrameBuffer *paramBuffer = nullptr;\n> \tFrameBuffer *statBuffer = nullptr;\n> @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>\n> \t\tstatBuffer = pipe_->availableStatBuffers_.front();\n> \t\tpipe_->availableStatBuffers_.pop();\n> +\t\tstatBuffer->_d()->setRequest(request);\n> \t}\n>\n> \tFrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> \t\treturn ret;\n> \t}\n>\n> -\tdata->frame_ = 0;\n> -\n> \tif (!isRaw_) {\n> \t\tret = param_->streamOn();\n> \t\tif (ret) {\n> @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> \tif (!info)\n> \t\treturn -ENOENT;\n>\n> -\tdata->ipa_->queueRequest(data->frame_, request->controls());\n> +\tdata->ipa_->queueRequest(request->sequence(), request->controls());\n> \tif (isRaw_) {\n> \t\tif (info->mainPathBuffer)\n> \t\t\tdata->mainPath_->queueBuffer(info->mainPathBuffer);\n> @@ -1033,12 +1031,10 @@ 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\tdata->ipa_->fillParamsBuffer(request->sequence(),\n> \t\t\t\t\t info->paramBuffer->cookie());\n> \t}\n>\n> -\tdata->frame_++;\n> -\n> \treturn 0;\n> }\n>\n> @@ -1269,7 +1265,8 @@ 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_->processStatsBuffer(request->sequence(),\n> +\t\t\t\t\t\t 0, ctrls);\n> \t\t}\n> \t} else {\n> \t\tif (isRaw_)\n> @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> {\n> \tASSERT(activeCamera_);\n> \tRkISP1CameraData *data = cameraData(activeCamera_);\n> +\tRequest *request = buffer->request();\n>\n> \tRkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> \tif (!info)\n> @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> \t\treturn;\n> \t}\n>\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_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n> +\t\t\t\t data->delayedCtrls_->get(request->sequence()));\n> }\n>\n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> --\n> 2.34.1\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 3FA66BDE17\n\tfor ;\n\tTue, 13 Feb 2024 11:31:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D4DE62801;\n\tTue, 13 Feb 2024 12:31:36 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0863461CB8\n\tfor ;\n\tTue, 13 Feb 2024 12:31:35 +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 9902C83F;\n\tTue, 13 Feb 2024 12:31:32 +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=\"UsKB4hQS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707823892;\n\tbh=FYFv2hDFdH7q9WUK7NKtHk6a5KU3ocbGI33QBZW+RTU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=UsKB4hQSmZJFYOaPMCKcCfM+OFtFW7p8kbSmXCS2HqPLhA+HHPaPkaWiaNx54cr7o\n\t/hb4oEOpIs2bARK6whEUyHa+mQ1mAnie4GFOehQbtEmPs9nmRVkyk78C8IhDDfR6pU\n\tc2Vr9anrtP+oj4hafol3gh88Kv5IxSUCgL/402ww=","Date":"Tue, 13 Feb 2024 12:31:31 +0100","From":"Jacopo Mondi ","To":"Daniel Scally ","Subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","Message-ID":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240212153532.179283-3-dan.scally@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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28656,"web_url":"https://patchwork.libcamera.org/comment/28656/","msgid":"<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","date":"2024-02-13T13:24:08","subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-02-13 11:31:31)\n> Hi Dan\n> \n> On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n> > The rkisp1 driver uses a couple of different methods of tracking the\n> > number of the frame - standardise them on request->sequence() and\n> > remove the frame_ member of RkISP1CameraData which was being used to\n> > do the job.\n> \n> Request::Private::sequence_ gets assigned to\n> Camera::Private::requestSequence_ by the PipelineHandler base class.\n> \n> Camera::Private::requestSequence_ gets zeroed in\n> PipelineHandler::stop()\n> \n> Camera::Private::requestSequence_ gets incremented in\n> PipelineHandler::doQueueRequest()\n> \n> So I think this change is legit (and should be done on IPU3 as well if\n> it uses the same scheme that was implemented here)\n\nI agree there, if this series works well, then we should handle both\nIPU3 and RKISP1 together to keep things aligned, and do the same\ncleanups on both sides.\n\n\n> \n> Reviewed-by: Jacopo Mondi \n> \n> >\n> > Signed-off-by: Daniel Scally \n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n> > 1 file changed, 10 insertions(+), 15 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index 5460dc11..22e553fe 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n> > public:\n> > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > RkISP1SelfPath *selfPath)\n> > - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> > + : Camera::Private(pipe), frameInfo_(pipe),\n> > mainPath_(mainPath), selfPath_(selfPath)\n> > {\n> > }\n> > @@ -99,7 +99,6 @@ public:\n> > Stream selfPathStream_;\n> > std::unique_ptr sensor_;\n> > std::unique_ptr delayedCtrls_;\n> > - unsigned int frame_;\n> > std::vector ipaBuffers_;\n> > RkISP1Frames frameInfo_;\n> >\n> > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> > bool isRaw)\n> > {\n> > - unsigned int frame = data->frame_;\n> > + unsigned int frame = request->sequence();\n\nThis worries me a little when we start thinking about what might happen\nif frames are dropped/application slows down.\n\nWe probably need some tests in lc-compliance to validate what happens if\nthe applciation *doesn't* keep up with the camera. I think before that\nwould make the \n\n> >\n> > FrameBuffer *paramBuffer = nullptr;\n> > FrameBuffer *statBuffer = nullptr;\n> > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> >\n> > statBuffer = pipe_->availableStatBuffers_.front();\n> > pipe_->availableStatBuffers_.pop();\n> > + statBuffer->_d()->setRequest(request);\n> > }\n> >\n> > FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > return ret;\n> > }\n> >\n> > - data->frame_ = 0;\n> > -\n> > if (!isRaw_) {\n> > ret = param_->streamOn();\n> > if (ret) {\n> > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > if (!info)\n> > return -ENOENT;\n> >\n> > - data->ipa_->queueRequest(data->frame_, request->controls());\n> > + data->ipa_->queueRequest(request->sequence(), request->controls());\n> > if (isRaw_) {\n> > if (info->mainPathBuffer)\n> > data->mainPath_->queueBuffer(info->mainPathBuffer);\n> > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > if (data->selfPath_ && info->selfPathBuffer)\n> > data->selfPath_->queueBuffer(info->selfPathBuffer);\n> > } else {\n> > - data->ipa_->fillParamsBuffer(data->frame_,\n> > + data->ipa_->fillParamsBuffer(request->sequence(),\n> > info->paramBuffer->cookie());\n> > }\n> >\n> > - data->frame_++;\n> > -\n> > return 0;\n> > }\n> >\n> > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > if (isRaw_) {\n> > const ControlList &ctrls =\n> > data->delayedCtrls_->get(metadata.sequence);\n> > - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > + data->ipa_->processStatsBuffer(request->sequence(),\n> > + 0, ctrls);\n> > }\n> > } else {\n> > if (isRaw_)\n> > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > {\n> > ASSERT(activeCamera_);\n> > RkISP1CameraData *data = cameraData(activeCamera_);\n> > + Request *request = buffer->request();\n> >\n> > RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > if (!info)\n> > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > return;\n> > }\n> >\n> > - if (data->frame_ <= buffer->metadata().sequence)\n> > - data->frame_ = buffer->metadata().sequence + 1;\n\nfor instance, it looks like there is some handling here that is tracking\ndata->frame_ to match the sequence number from the image pipelines.\n\nNow the sequencing is all based around the request sequences queued.\nThat might be all fine, but I'm weary about how we might need to keep\nthings aligned somewhere to the real sequence number from the camera.\nEspecially when it comes to tracking what controls were trully applied\nthrough DelayedControls etc.\n\n\n\n> > -\n> > - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > - data->delayedCtrls_->get(buffer->metadata().sequence));\n> > + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n> > + data->delayedCtrls_->get(request->sequence()));\n> > }\n> >\n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> > --\n> > 2.34.1\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 78F6AC3257\n\tfor ;\n\tTue, 13 Feb 2024 13:24:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D4EBD62802;\n\tTue, 13 Feb 2024 14:24:13 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8981C61CB8\n\tfor ;\n\tTue, 13 Feb 2024 14:24:11 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2A84983F;\n\tTue, 13 Feb 2024 14:24:09 +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=\"iH2a6NgX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707830649;\n\tbh=OfDk36afiHAXad+GT9cBUuk4GdbR863nFLn7p6pKdjw=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=iH2a6NgXI+WZ8ctIMiVmr6AgOqqy2MF0q+RBoqUkfwDxW9Zpmt9ifiXfFXiW31gPS\n\t8rsn+zmIt8xcp4nJ6u44eXTdIz3U0lpWmU9V3Eu6kwRhlULrx5z8sVxyjhJUegvngN\n\tlIMkB8CMOmOmM0iT/yrKbCqQOByBTas+Hm79qWHg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>\n\t","Subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","From":"Kieran Bingham ","To":"Daniel Scally ,\n\tJacopo Mondi ","Date":"Tue, 13 Feb 2024 13:24:08 +0000","Message-ID":"<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28658,"web_url":"https://patchwork.libcamera.org/comment/28658/","msgid":"<170783451837.1676185.7559627720262191952@ping.linuxembedded.co.uk>","date":"2024-02-13T14:28:38","subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Kieran Bingham (2024-02-13 13:24:08)\n> Quoting Jacopo Mondi (2024-02-13 11:31:31)\n> > Hi Dan\n> > \n> > On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n> > > The rkisp1 driver uses a couple of different methods of tracking the\n> > > number of the frame - standardise them on request->sequence() and\n> > > remove the frame_ member of RkISP1CameraData which was being used to\n> > > do the job.\n> > \n> > Request::Private::sequence_ gets assigned to\n> > Camera::Private::requestSequence_ by the PipelineHandler base class.\n> > \n> > Camera::Private::requestSequence_ gets zeroed in\n> > PipelineHandler::stop()\n> > \n> > Camera::Private::requestSequence_ gets incremented in\n> > PipelineHandler::doQueueRequest()\n> > \n> > So I think this change is legit (and should be done on IPU3 as well if\n> > it uses the same scheme that was implemented here)\n> \n> I agree there, if this series works well, then we should handle both\n> IPU3 and RKISP1 together to keep things aligned, and do the same\n> cleanups on both sides.\n> \n> \n> > \n> > Reviewed-by: Jacopo Mondi \n> > \n> > >\n> > > Signed-off-by: Daniel Scally \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n> > > 1 file changed, 10 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5460dc11..22e553fe 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n> > > public:\n> > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > > RkISP1SelfPath *selfPath)\n> > > - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> > > + : Camera::Private(pipe), frameInfo_(pipe),\n> > > mainPath_(mainPath), selfPath_(selfPath)\n> > > {\n> > > }\n> > > @@ -99,7 +99,6 @@ public:\n> > > Stream selfPathStream_;\n> > > std::unique_ptr sensor_;\n> > > std::unique_ptr delayedCtrls_;\n> > > - unsigned int frame_;\n> > > std::vector ipaBuffers_;\n> > > RkISP1Frames frameInfo_;\n> > >\n> > > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > > RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> > > bool isRaw)\n> > > {\n> > > - unsigned int frame = data->frame_;\n> > > + unsigned int frame = request->sequence();\n> \n> This worries me a little when we start thinking about what might happen\n> if frames are dropped/application slows down.\n> \n> We probably need some tests in lc-compliance to validate what happens if\n> the applciation *doesn't* keep up with the camera. I think before that\n> would make the \n\nErr ... language failure? GPT overflow? Something else? I'm not sure\nwhat happened here.\n\nI think I was starting to talk here about the handling of the data\nsequence number likely being based aroudn the number of *frames*\ncontrasted with the number of requests on request->sequence() which are\ntwo separate sequence generators.\n\n> \n> > >\n> > > FrameBuffer *paramBuffer = nullptr;\n> > > FrameBuffer *statBuffer = nullptr;\n> > > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > >\n> > > statBuffer = pipe_->availableStatBuffers_.front();\n> > > pipe_->availableStatBuffers_.pop();\n> > > + statBuffer->_d()->setRequest(request);\n> > > }\n> > >\n> > > FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > return ret;\n> > > }\n> > >\n> > > - data->frame_ = 0;\n> > > -\n> > > if (!isRaw_) {\n> > > ret = param_->streamOn();\n> > > if (ret) {\n> > > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > if (!info)\n> > > return -ENOENT;\n> > >\n> > > - data->ipa_->queueRequest(data->frame_, request->controls());\n> > > + data->ipa_->queueRequest(request->sequence(), request->controls());\n> > > if (isRaw_) {\n> > > if (info->mainPathBuffer)\n> > > data->mainPath_->queueBuffer(info->mainPathBuffer);\n> > > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > if (data->selfPath_ && info->selfPathBuffer)\n> > > data->selfPath_->queueBuffer(info->selfPathBuffer);\n> > > } else {\n> > > - data->ipa_->fillParamsBuffer(data->frame_,\n> > > + data->ipa_->fillParamsBuffer(request->sequence(),\n> > > info->paramBuffer->cookie());\n> > > }\n> > >\n> > > - data->frame_++;\n> > > -\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > if (isRaw_) {\n> > > const ControlList &ctrls =\n> > > data->delayedCtrls_->get(metadata.sequence);\n> > > - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > + data->ipa_->processStatsBuffer(request->sequence(),\n> > > + 0, ctrls);\n> > > }\n> > > } else {\n> > > if (isRaw_)\n> > > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > {\n> > > ASSERT(activeCamera_);\n> > > RkISP1CameraData *data = cameraData(activeCamera_);\n> > > + Request *request = buffer->request();\n> > >\n> > > RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > > if (!info)\n> > > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > return;\n> > > }\n> > >\n> > > - if (data->frame_ <= buffer->metadata().sequence)\n> > > - data->frame_ = buffer->metadata().sequence + 1;\n> \n> for instance, it looks like there is some handling here that is tracking\n> data->frame_ to match the sequence number from the image pipelines.\n> \n> Now the sequencing is all based around the request sequences queued.\n> That might be all fine, but I'm weary about how we might need to keep\n> things aligned somewhere to the real sequence number from the camera.\n> Especially when it comes to tracking what controls were trully applied\n> through DelayedControls etc.\n> \n> \n> \n> > > -\n> > > - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > > - data->delayedCtrls_->get(buffer->metadata().sequence));\n> > > + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n> > > + data->delayedCtrls_->get(request->sequence()));\n> > > }\n> > >\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> > > --\n> > > 2.34.1\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 78CAAC3257\n\tfor ;\n\tTue, 13 Feb 2024 14:28:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9436D62805;\n\tTue, 13 Feb 2024 15:28:43 +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 98D0461CB8\n\tfor ;\n\tTue, 13 Feb 2024 15:28:41 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 28046675;\n\tTue, 13 Feb 2024 15:28:39 +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=\"As91gm9y\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707834519;\n\tbh=VgexL9AcSxcF6Nr4kmIlt8NayW7DZpAoeQHzoD/3X6g=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=As91gm9yujAi7YltfeVl+nSsITLfuzj8xNNGnvtsxizBlQzASjhpAL75qzdZz8MhT\n\t6hFKEjlT3oOkBzut3SbqKn2kcKGUilbPo6pQwGHEZzihyFx7VFkiQrbjZrPPTv0FuV\n\t0S6ts2cF4KS8OblV/EGjU1a+wvlB50q9I6E845vQ=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>\n\t\n\t<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","Subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","From":"Kieran Bingham ","To":"Daniel Scally ,\n\tJacopo Mondi ","Date":"Tue, 13 Feb 2024 14:28:38 +0000","Message-ID":"<170783451837.1676185.7559627720262191952@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28659,"web_url":"https://patchwork.libcamera.org/comment/28659/","msgid":"","date":"2024-02-13T15:09:57","subject":"Re: Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number\n\ttracking","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:\n> Quoting Jacopo Mondi (2024-02-13 11:31:31)\n> > Hi Dan\n> >\n> > On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n> > > The rkisp1 driver uses a couple of different methods of tracking the\n> > > number of the frame - standardise them on request->sequence() and\n> > > remove the frame_ member of RkISP1CameraData which was being used to\n> > > do the job.\n> >\n> > Request::Private::sequence_ gets assigned to\n> > Camera::Private::requestSequence_ by the PipelineHandler base class.\n> >\n> > Camera::Private::requestSequence_ gets zeroed in\n> > PipelineHandler::stop()\n> >\n> > Camera::Private::requestSequence_ gets incremented in\n> > PipelineHandler::doQueueRequest()\n> >\n> > So I think this change is legit (and should be done on IPU3 as well if\n> > it uses the same scheme that was implemented here)\n>\n> I agree there, if this series works well, then we should handle both\n> IPU3 and RKISP1 together to keep things aligned, and do the same\n> cleanups on both sides.\n>\n>\n> >\n> > Reviewed-by: Jacopo Mondi \n> >\n> > >\n> > > Signed-off-by: Daniel Scally \n> > > ---\n> > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n> > > 1 file changed, 10 insertions(+), 15 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index 5460dc11..22e553fe 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n> > > public:\n> > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > > RkISP1SelfPath *selfPath)\n> > > - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> > > + : Camera::Private(pipe), frameInfo_(pipe),\n> > > mainPath_(mainPath), selfPath_(selfPath)\n> > > {\n> > > }\n> > > @@ -99,7 +99,6 @@ public:\n> > > Stream selfPathStream_;\n> > > std::unique_ptr sensor_;\n> > > std::unique_ptr delayedCtrls_;\n> > > - unsigned int frame_;\n> > > std::vector ipaBuffers_;\n> > > RkISP1Frames frameInfo_;\n> > >\n> > > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > > RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> > > bool isRaw)\n> > > {\n> > > - unsigned int frame = data->frame_;\n> > > + unsigned int frame = request->sequence();\n>\n> This worries me a little when we start thinking about what might happen\n> if frames are dropped/application slows down.\n>\n> We probably need some tests in lc-compliance to validate what happens if\n> the applciation *doesn't* keep up with the camera. I think before that\n> would make the\n>\n\nmake the ? :)\n\nAnyway, I'm failing to see why this patch changes the model when it\ncomes to feeding the pipeline from the application.\n\nBefore this patch data->frame_ was increased at queueRequestDevice() time\nand request->sequence_, which is used now, is similarly incremented by\nthe pipeline handler base class before calling queueRequestDevice().\n\nThe pipeline feeds one parameters buffer to the IPA in\nqueueRequestDevice() (aka when application queues a request), and once\nthe IPA has filled in the parameters it calls\nRkISP1CameraData::paramFilled() which queues buffers to the capture\nvideo devices.\n\nIf the application does not queue any request it is then my\nunderstanding that the IPA doesn't get triggered and doesn't do any\nprocessing.\n\nThe only event that happens asynchronously from the request queueing\nis the frameStart event, on which we poke at delayed controls but do\nnot actually trigger any IPA processing. This has not changed as far\nas I can tell..\n\n> > >\n> > > FrameBuffer *paramBuffer = nullptr;\n> > > FrameBuffer *statBuffer = nullptr;\n> > > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > >\n> > > statBuffer = pipe_->availableStatBuffers_.front();\n> > > pipe_->availableStatBuffers_.pop();\n> > > + statBuffer->_d()->setRequest(request);\n> > > }\n> > >\n> > > FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > return ret;\n> > > }\n> > >\n> > > - data->frame_ = 0;\n> > > -\n> > > if (!isRaw_) {\n> > > ret = param_->streamOn();\n> > > if (ret) {\n> > > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > if (!info)\n> > > return -ENOENT;\n> > >\n> > > - data->ipa_->queueRequest(data->frame_, request->controls());\n> > > + data->ipa_->queueRequest(request->sequence(), request->controls());\n> > > if (isRaw_) {\n> > > if (info->mainPathBuffer)\n> > > data->mainPath_->queueBuffer(info->mainPathBuffer);\n> > > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > if (data->selfPath_ && info->selfPathBuffer)\n> > > data->selfPath_->queueBuffer(info->selfPathBuffer);\n> > > } else {\n> > > - data->ipa_->fillParamsBuffer(data->frame_,\n> > > + data->ipa_->fillParamsBuffer(request->sequence(),\n> > > info->paramBuffer->cookie());\n> > > }\n> > >\n> > > - data->frame_++;\n> > > -\n> > > return 0;\n> > > }\n> > >\n> > > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > if (isRaw_) {\n> > > const ControlList &ctrls =\n> > > data->delayedCtrls_->get(metadata.sequence);\n> > > - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > + data->ipa_->processStatsBuffer(request->sequence(),\n> > > + 0, ctrls);\n> > > }\n> > > } else {\n> > > if (isRaw_)\n> > > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > {\n> > > ASSERT(activeCamera_);\n> > > RkISP1CameraData *data = cameraData(activeCamera_);\n> > > + Request *request = buffer->request();\n> > >\n> > > RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > > if (!info)\n> > > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > return;\n> > > }\n> > >\n> > > - if (data->frame_ <= buffer->metadata().sequence)\n> > > - data->frame_ = buffer->metadata().sequence + 1;\n>\n> for instance, it looks like there is some handling here that is tracking\n> data->frame_ to match the sequence number from the image pipelines.\n>\n\nIt's rather easy to track this condition, just save frames to disk\nwhen capturing with 'cam' this makes the application stall long enough\nto have two (or more) stats buffers being dequeued in between two\ncapture requests being submitted.\n\n\n> Now the sequencing is all based around the request sequences queued.\n> That might be all fine, but I'm weary about how we might need to keep\n> things aligned somewhere to the real sequence number from the camera.\n> Especially when it comes to tracking what controls were trully applied\n> through DelayedControls etc.\n>\n\nAnd that could actually be the real issue here.\n\nWe now do\n\n data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),\n data->delayedCtrls_->get(request->sequence()));\n\nWhile before we had\n\n data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n data->delayedCtrls_->get(buffer->metadata().sequence));\n\nSo yes, it seems to me in this new method are not counting 'frames'\nanymore, which is something we should indeed be doing mostly to\nsynchronize the sensor and the ISP control values...\n\nIn fact, I see different behaviours between current master and this\nseries. I'm testing capturing 100 frames to disk (hence triggering\nsome stalls). With master I see the image being visibile for the first\nframes, then getting very dark then stabilizing. With this series the\nimage starts all black, then stabilizes. I've not yet found out why or\neven if it related, just leaving it here for Dan to see if he had\nnoticed anything similar in his testing..\n\n>\n>\n> > > -\n> > > - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > > - data->delayedCtrls_->get(buffer->metadata().sequence));\n> > > + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n> > > + data->delayedCtrls_->get(request->sequence()));\n> > > }\n> > >\n> > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> > > --\n> > > 2.34.1\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 CA312BDE17\n\tfor ;\n\tTue, 13 Feb 2024 15:10:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2270C62801;\n\tTue, 13 Feb 2024 16:10:02 +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 EB22761CB8\n\tfor ;\n\tTue, 13 Feb 2024 16:10:00 +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 5869783F;\n\tTue, 13 Feb 2024 16:09:58 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hBYNE3+C\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707836998;\n\tbh=VRobhKGm++x9/y4Rfbyie/raU5clljl9XcXy5rJw5/Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hBYNE3+CtCXYEjp647gxO+qsAgo7Gfybb27tmhM0063ThXCNl27UES4fdEkJLXFVm\n\tcVK5nSQEIzMhoCbIOI4unJY5VMNaOaeR5jTp/31aBfAi1gG6jSe6vj77c0FWxyd9Qv\n\tA5lk3FxJET2nLrab3IXJfNNa4mQF9OXPOvagF1sc=","Date":"Tue, 13 Feb 2024 16:09:57 +0100","From":"Jacopo Mondi ","To":"Kieran Bingham ","Subject":"Re: Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number\n\ttracking","Message-ID":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>\n\t\n\t<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>","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","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28660,"web_url":"https://patchwork.libcamera.org/comment/28660/","msgid":"<170783889728.1011926.18154789779467969678@ping.linuxembedded.co.uk>","date":"2024-02-13T15:41:37","subject":"Re: Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number\n\ttracking","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi (2024-02-13 15:09:57)\n> Hi Kieran\n> \n> On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi (2024-02-13 11:31:31)\n> > > Hi Dan\n> > >\n> > > On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n> > > > The rkisp1 driver uses a couple of different methods of tracking the\n> > > > number of the frame - standardise them on request->sequence() and\n> > > > remove the frame_ member of RkISP1CameraData which was being used to\n> > > > do the job.\n> > >\n> > > Request::Private::sequence_ gets assigned to\n> > > Camera::Private::requestSequence_ by the PipelineHandler base class.\n> > >\n> > > Camera::Private::requestSequence_ gets zeroed in\n> > > PipelineHandler::stop()\n> > >\n> > > Camera::Private::requestSequence_ gets incremented in\n> > > PipelineHandler::doQueueRequest()\n> > >\n> > > So I think this change is legit (and should be done on IPU3 as well if\n> > > it uses the same scheme that was implemented here)\n> >\n> > I agree there, if this series works well, then we should handle both\n> > IPU3 and RKISP1 together to keep things aligned, and do the same\n> > cleanups on both sides.\n> >\n> >\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> > >\n> > > >\n> > > > Signed-off-by: Daniel Scally \n> > > > ---\n> > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n> > > > 1 file changed, 10 insertions(+), 15 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index 5460dc11..22e553fe 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n> > > > public:\n> > > > RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> > > > RkISP1SelfPath *selfPath)\n> > > > - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> > > > + : Camera::Private(pipe), frameInfo_(pipe),\n> > > > mainPath_(mainPath), selfPath_(selfPath)\n> > > > {\n> > > > }\n> > > > @@ -99,7 +99,6 @@ public:\n> > > > Stream selfPathStream_;\n> > > > std::unique_ptr sensor_;\n> > > > std::unique_ptr delayedCtrls_;\n> > > > - unsigned int frame_;\n> > > > std::vector ipaBuffers_;\n> > > > RkISP1Frames frameInfo_;\n> > > >\n> > > > @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> > > > RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> > > > bool isRaw)\n> > > > {\n> > > > - unsigned int frame = data->frame_;\n> > > > + unsigned int frame = request->sequence();\n> >\n> > This worries me a little when we start thinking about what might happen\n> > if frames are dropped/application slows down.\n> >\n> > We probably need some tests in lc-compliance to validate what happens if\n> > the applciation *doesn't* keep up with the camera. I think before that\n> > would make the\n> >\n> \n> make the ? :)\n> \n> Anyway, I'm failing to see why this patch changes the model when it\n> comes to feeding the pipeline from the application.\n> \n> Before this patch data->frame_ was increased at queueRequestDevice() time\n> and request->sequence_, which is used now, is similarly incremented by\n> the pipeline handler base class before calling queueRequestDevice().\n\nAnd also increased if there was a gap identified in\nPipelineHandlerRkISP1::statReady()\n\n\n> The pipeline feeds one parameters buffer to the IPA in\n> queueRequestDevice() (aka when application queues a request), and once\n> the IPA has filled in the parameters it calls\n> RkISP1CameraData::paramFilled() which queues buffers to the capture\n> video devices.\n> \n> If the application does not queue any request it is then my\n> understanding that the IPA doesn't get triggered and doesn't do any\n> processing.\n\nI expect so. Though at some point, we might actually want to 'fix' that\nso that the IPA can run if the stats are produced. Otherwise we're\nlosing that data, and the stats buffers 'can' likely run without\nrequests.\n\n(Changing this behaviour is a big rabbit hole though, and likely\nrequires lots more synchronisations, and maybe isn't even likely to\nhappen/be possible if the ISP gets stalled when there's no image output\nbuffers....)\n\n\n\n> The only event that happens asynchronously from the request queueing\n> is the frameStart event, on which we poke at delayed controls but do\n> not actually trigger any IPA processing. This has not changed as far\n> as I can tell..\n> \n> > > >\n> > > > FrameBuffer *paramBuffer = nullptr;\n> > > > FrameBuffer *statBuffer = nullptr;\n> > > > @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n> > > >\n> > > > statBuffer = pipe_->availableStatBuffers_.front();\n> > > > pipe_->availableStatBuffers_.pop();\n> > > > + statBuffer->_d()->setRequest(request);\n> > > > }\n> > > >\n> > > > FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> > > > @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> > > > return ret;\n> > > > }\n> > > >\n> > > > - data->frame_ = 0;\n> > > > -\n> > > > if (!isRaw_) {\n> > > > ret = param_->streamOn();\n> > > > if (ret) {\n> > > > @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > > if (!info)\n> > > > return -ENOENT;\n> > > >\n> > > > - data->ipa_->queueRequest(data->frame_, request->controls());\n> > > > + data->ipa_->queueRequest(request->sequence(), request->controls());\n> > > > if (isRaw_) {\n> > > > if (info->mainPathBuffer)\n> > > > data->mainPath_->queueBuffer(info->mainPathBuffer);\n> > > > @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> > > > if (data->selfPath_ && info->selfPathBuffer)\n> > > > data->selfPath_->queueBuffer(info->selfPathBuffer);\n> > > > } else {\n> > > > - data->ipa_->fillParamsBuffer(data->frame_,\n> > > > + data->ipa_->fillParamsBuffer(request->sequence(),\n> > > > info->paramBuffer->cookie());\n> > > > }\n> > > >\n> > > > - data->frame_++;\n> > > > -\n> > > > return 0;\n> > > > }\n> > > >\n> > > > @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n> > > > if (isRaw_) {\n> > > > const ControlList &ctrls =\n> > > > data->delayedCtrls_->get(metadata.sequence);\n> > > > - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n> > > > + data->ipa_->processStatsBuffer(request->sequence(),\n> > > > + 0, ctrls);\n> > > > }\n> > > > } else {\n> > > > if (isRaw_)\n> > > > @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > > {\n> > > > ASSERT(activeCamera_);\n> > > > RkISP1CameraData *data = cameraData(activeCamera_);\n> > > > + Request *request = buffer->request();\n> > > >\n> > > > RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > > > if (!info)\n> > > > @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n> > > > return;\n> > > > }\n> > > >\n> > > > - if (data->frame_ <= buffer->metadata().sequence)\n> > > > - data->frame_ = buffer->metadata().sequence + 1;\n> >\n> > for instance, it looks like there is some handling here that is tracking\n> > data->frame_ to match the sequence number from the image pipelines.\n> >\n> \n> It's rather easy to track this condition, just save frames to disk\n> when capturing with 'cam' this makes the application stall long enough\n> to have two (or more) stats buffers being dequeued in between two\n> capture requests being submitted.\n> \n> \n> > Now the sequencing is all based around the request sequences queued.\n> > That might be all fine, but I'm weary about how we might need to keep\n> > things aligned somewhere to the real sequence number from the camera.\n> > Especially when it comes to tracking what controls were trully applied\n> > through DelayedControls etc.\n> >\n> \n> And that could actually be the real issue here.\n\nYes, that's my concern. Essentially we have two 'timing source'. The\nSensor - generating sensor sequence numbers, and the applications\n(implicitly) generating request sequence numbers.\n\n\n> We now do\n> \n> data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),\n> data->delayedCtrls_->get(request->sequence()));\n> \n> While before we had\n> \n> data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> data->delayedCtrls_->get(buffer->metadata().sequence));\n> \n> So yes, it seems to me in this new method are not counting 'frames'\n> anymore, which is something we should indeed be doing mostly to\n> synchronize the sensor and the ISP control values...\n\nYes, at some level we need to make sure we keep maintaining that\nrelationship.\n\n\n> In fact, I see different behaviours between current master and this\n> series. I'm testing capturing 100 frames to disk (hence triggering\n> some stalls). With master I see the image being visibile for the first\n> frames, then getting very dark then stabilizing. With this series the\n> image starts all black, then stabilizes. I've not yet found out why or\n> even if it related, just leaving it here for Dan to see if he had\n> noticed anything similar in his testing..\n> \n> >\n> >\n> > > > -\n> > > > - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> > > > - data->delayedCtrls_->get(buffer->metadata().sequence));\n> > > > + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n> > > > + data->delayedCtrls_->get(request->sequence()));\n> > > > }\n> > > >\n> > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n> > > > --\n> > > > 2.34.1\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 96126C3257\n\tfor ;\n\tTue, 13 Feb 2024 15:41:41 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DB9D062805;\n\tTue, 13 Feb 2024 16:41:40 +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 A040D61CB8\n\tfor ;\n\tTue, 13 Feb 2024 16:41:39 +0100 (CET)","from pendragon.ideasonboard.com\n\t(aztw-30-b2-v4wan-166917-cust845.vm26.cable.virginm.net\n\t[82.37.23.78])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1911F2A5;\n\tTue, 13 Feb 2024 16:41:37 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"GtWvYtqF\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1707838897;\n\tbh=VC8IjsMxnGkdimH066wsf8vfsVHx7154rcf36yk6Nt8=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=GtWvYtqFqzvhMOmcqdqiPBAQr2RPcoo6spR69lvHsK29WLdSUlYrwTniL7ce8WaOq\n\tKh8cDKjk7Z6BZm0sr5CABvq7mR5T1+79xOe+umwR5SDmmkzRLYTXPq1p3sKs8vGLOo\n\tesNQqmuQUmwImhKzS0prjk5h+jYg1ViWfe6f80ic=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>\n\t\n\t<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>\n\t","Subject":"Re: Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number\n\ttracking","From":"Kieran Bingham ","To":"Jacopo Mondi ","Date":"Tue, 13 Feb 2024 15:41:37 +0000","Message-ID":"<170783889728.1011926.18154789779467969678@ping.linuxembedded.co.uk>","User-Agent":"alot/0.10","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","Cc":"Jacopo Mondi ,\n\tlibcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":28688,"web_url":"https://patchwork.libcamera.org/comment/28688/","msgid":"","date":"2024-02-16T17:04:04","subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","submitter":{"id":156,"url":"https://patchwork.libcamera.org/api/people/156/","name":"Dan Scally","email":"dan.scally@ideasonboard.com"},"content":"Hi Kieran, Jacopo\n\nOn 13/02/2024 15:09, Jacopo Mondi wrote:\n> Hi Kieran\n>\n> On Tue, Feb 13, 2024 at 01:24:08PM +0000, Kieran Bingham wrote:\n>> Quoting Jacopo Mondi (2024-02-13 11:31:31)\n>>> Hi Dan\n>>>\n>>> On Mon, Feb 12, 2024 at 03:35:29PM +0000, Daniel Scally wrote:\n>>>> The rkisp1 driver uses a couple of different methods of tracking the\n>>>> number of the frame - standardise them on request->sequence() and\n>>>> remove the frame_ member of RkISP1CameraData which was being used to\n>>>> do the job.\n>>> Request::Private::sequence_ gets assigned to\n>>> Camera::Private::requestSequence_ by the PipelineHandler base class.\n>>>\n>>> Camera::Private::requestSequence_ gets zeroed in\n>>> PipelineHandler::stop()\n>>>\n>>> Camera::Private::requestSequence_ gets incremented in\n>>> PipelineHandler::doQueueRequest()\n>>>\n>>> So I think this change is legit (and should be done on IPU3 as well if\n>>> it uses the same scheme that was implemented here)\n>> I agree there, if this series works well, then we should handle both\n>> IPU3 and RKISP1 together to keep things aligned, and do the same\n>> cleanups on both sides.\n>>\n>>\n>>> Reviewed-by: Jacopo Mondi \n>>>\n>>>> Signed-off-by: Daniel Scally \n>>>> ---\n>>>> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 25 ++++++++++--------------\n>>>> 1 file changed, 10 insertions(+), 15 deletions(-)\n>>>>\n>>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> index 5460dc11..22e553fe 100644\n>>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n>>>> @@ -87,7 +87,7 @@ class RkISP1CameraData : public Camera::Private\n>>>> public:\n>>>> RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n>>>> RkISP1SelfPath *selfPath)\n>>>> - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n>>>> + : Camera::Private(pipe), frameInfo_(pipe),\n>>>> mainPath_(mainPath), selfPath_(selfPath)\n>>>> {\n>>>> }\n>>>> @@ -99,7 +99,6 @@ public:\n>>>> Stream selfPathStream_;\n>>>> std::unique_ptr sensor_;\n>>>> std::unique_ptr delayedCtrls_;\n>>>> - unsigned int frame_;\n>>>> std::vector ipaBuffers_;\n>>>> RkISP1Frames frameInfo_;\n>>>>\n>>>> @@ -212,7 +211,7 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n>>>> RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n>>>> bool isRaw)\n>>>> {\n>>>> - unsigned int frame = data->frame_;\n>>>> + unsigned int frame = request->sequence();\n>> This worries me a little when we start thinking about what might happen\n>> if frames are dropped/application slows down.\n>>\n>> We probably need some tests in lc-compliance to validate what happens if\n>> the applciation *doesn't* keep up with the camera. I think before that\n>> would make the\n>>\n> make the ? :)\n>\n> Anyway, I'm failing to see why this patch changes the model when it\n> comes to feeding the pipeline from the application.\n>\n> Before this patch data->frame_ was increased at queueRequestDevice() time\n> and request->sequence_, which is used now, is similarly incremented by\n> the pipeline handler base class before calling queueRequestDevice().\n>\n> The pipeline feeds one parameters buffer to the IPA in\n> queueRequestDevice() (aka when application queues a request), and once\n> the IPA has filled in the parameters it calls\n> RkISP1CameraData::paramFilled() which queues buffers to the capture\n> video devices.\n>\n> If the application does not queue any request it is then my\n> understanding that the IPA doesn't get triggered and doesn't do any\n> processing.\n>\n> The only event that happens asynchronously from the request queueing\n> is the frameStart event, on which we poke at delayed controls but do\n> not actually trigger any IPA processing. This has not changed as far\n> as I can tell..\n>\n>>>> FrameBuffer *paramBuffer = nullptr;\n>>>> FrameBuffer *statBuffer = nullptr;\n>>>> @@ -233,6 +232,7 @@ RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *req\n>>>>\n>>>> statBuffer = pipe_->availableStatBuffers_.front();\n>>>> pipe_->availableStatBuffers_.pop();\n>>>> + statBuffer->_d()->setRequest(request);\n>>>> }\n>>>>\n>>>> FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n>>>> @@ -932,8 +932,6 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n>>>> return ret;\n>>>> }\n>>>>\n>>>> - data->frame_ = 0;\n>>>> -\n>>>> if (!isRaw_) {\n>>>> ret = param_->streamOn();\n>>>> if (ret) {\n>>>> @@ -1025,7 +1023,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>>>> if (!info)\n>>>> return -ENOENT;\n>>>>\n>>>> - data->ipa_->queueRequest(data->frame_, request->controls());\n>>>> + data->ipa_->queueRequest(request->sequence(), request->controls());\n>>>> if (isRaw_) {\n>>>> if (info->mainPathBuffer)\n>>>> data->mainPath_->queueBuffer(info->mainPathBuffer);\n>>>> @@ -1033,12 +1031,10 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n>>>> if (data->selfPath_ && info->selfPathBuffer)\n>>>> data->selfPath_->queueBuffer(info->selfPathBuffer);\n>>>> } else {\n>>>> - data->ipa_->fillParamsBuffer(data->frame_,\n>>>> + data->ipa_->fillParamsBuffer(request->sequence(),\n>>>> info->paramBuffer->cookie());\n>>>> }\n>>>>\n>>>> - data->frame_++;\n>>>> -\n>>>> return 0;\n>>>> }\n>>>>\n>>>> @@ -1269,7 +1265,8 @@ void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer)\n>>>> if (isRaw_) {\n>>>> const ControlList &ctrls =\n>>>> data->delayedCtrls_->get(metadata.sequence);\n>>>> - data->ipa_->processStatsBuffer(info->frame, 0, ctrls);\n>>>> + data->ipa_->processStatsBuffer(request->sequence(),\n>>>> + 0, ctrls);\n>>>> }\n>>>> } else {\n>>>> if (isRaw_)\n>>>> @@ -1284,6 +1281,7 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>>>> {\n>>>> ASSERT(activeCamera_);\n>>>> RkISP1CameraData *data = cameraData(activeCamera_);\n>>>> + Request *request = buffer->request();\n>>>>\n>>>> RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n>>>> if (!info)\n>>>> @@ -1295,11 +1293,8 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)\n>>>> return;\n>>>> }\n>>>>\n>>>> - if (data->frame_ <= buffer->metadata().sequence)\n>>>> - data->frame_ = buffer->metadata().sequence + 1;\n>> for instance, it looks like there is some handling here that is tracking\n>> data->frame_ to match the sequence number from the image pipelines.\n>>\n> It's rather easy to track this condition, just save frames to disk\n> when capturing with 'cam' this makes the application stall long enough\n> to have two (or more) stats buffers being dequeued in between two\n> capture requests being submitted.\n>\n>\n>> Now the sequencing is all based around the request sequences queued.\n>> That might be all fine, but I'm weary about how we might need to keep\n>> things aligned somewhere to the real sequence number from the camera.\n>> Especially when it comes to tracking what controls were trully applied\n>> through DelayedControls etc.\n>>\n> And that could actually be the real issue here.\n>\n> We now do\n>\n> data->ipa_->processStatsBuffer(request->sequence(), buffer->cookie(),\n> data->delayedCtrls_->get(request->sequence()));\n>\n> While before we had\n>\n> data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n> data->delayedCtrls_->get(buffer->metadata().sequence));\n>\n> So yes, it seems to me in this new method are not counting 'frames'\n> anymore, which is something we should indeed be doing mostly to\n> synchronize the sensor and the ISP control values...\n>\n> In fact, I see different behaviours between current master and this\n> series. I'm testing capturing 100 frames to disk (hence triggering\n> some stalls). With master I see the image being visibile for the first\n> frames, then getting very dark then stabilizing. With this series the\n> image starts all black, then stabilizes. I've not yet found out why or\n> even if it related, just leaving it here for Dan to see if he had\n> noticed anything similar in his testing..\n\n\nSo I didn't notice any difference between master and my branch in my testing. I'll graph it as \nKieran suggested to try to have a more reproducible proof of equivalence. I couldn't see any reason \nto rely on the frame number coming from the driver instead of the request sequence, so it seemed ok \nto change. I've been working through it again today to see if I missed something, but as far as I \ncan tell I haven't. In queueRequestDevice() the FrameInfo->frame is set to data->frame_, and then \nthat value is used to index the IPA's frameContexts every other time they're accessed. Although \ndata->frame_ is corrected in PipelineHandlerRkISP1::statReady() to track the sequence from the \nkernel driver it's not actually **used** following that correction until the next queueRequestDevice \nwhere it's used to set the new FrameInfo's frame member. In that regard then as far as I can tell, \nall that's happening is that it's accessing a different one of the 16 FrameContexts stored in the \nIPA's FCQueue...but given the chosen context is cleared in FCQueue::alloc anyway, that doesn't seem \nlike a problem.\n\n\nAs for the delayed controls; it looks to me like the implementation doesn't really take account of \nthe passed-in frame number to the IPA module anyway. IPARkISP1::processStatsBuffer() calls \nsetControls(frame) - that function fills a ControlList with values from frameContext.agc and emits \nsetSensorControls(frame, ctrls) but the RkISP1CameraData::setSensorControls() slot connected to that \nsignal doesn't use the frame parameter - it just pushes the controls onto delayedCtrls_ with a 1 or \n2 frame delay depending on whether it's gain or exposure, and then the DelayedControls class \ninternally relies on those delay values and its writeCount_ (taken from the kernel driver's \nsequence) and queueCount_ members to decide which controls to set. The values in frameContext.agc \nwere filled during the Agc algorithm's ::prepare() with the values from the activeContext, \ncalculated during the last processStatsBuffer...in other words as far as I can tell the controls set \nvia the DelayedControls always take effect in 1/2 frames regardless of what frame number you use and \nalways use the values calculated from whatever the most recently processed statistics buffer is. The \nactual applied control values are directly passed in from the pipeline handler rather than fetched \nwith the frame/request number or anything, and they're fetched from DelayedControls without relying \non either the request->sequence() number or the data->frame_ number...\n\n\nThis has been painful to follow, but I think, **think**, it's safe.\n\n\n>>\n>>>> -\n>>>> - data->ipa_->processStatsBuffer(info->frame, info->statBuffer->cookie(),\n>>>> - data->delayedCtrls_->get(buffer->metadata().sequence));\n>>>> + data->ipa_->processStatsBuffer(request->sequence(), info->statBuffer->cookie(),\n>>>> + data->delayedCtrls_->get(request->sequence()));\n>>>> }\n>>>>\n>>>> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1)\n>>>> --\n>>>> 2.34.1\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 8891BBDE17\n\tfor ;\n\tFri, 16 Feb 2024 17:04:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id BE0F062801;\n\tFri, 16 Feb 2024 18:04:09 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2525B61CAE\n\tfor ;\n\tFri, 16 Feb 2024 18:04:08 +0100 (CET)","from [192.168.0.43]\n\t(cpc141996-chfd3-2-0-cust928.12-3.cable.virginm.net [86.13.91.161])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 43405D4A;\n\tFri, 16 Feb 2024 18:04:03 +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=\"YXJIkOrx\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1708103043;\n\tbh=31tsZg015prSeBmhntoFlvtE1Kyu840walK5dGqiHFs=;\n\th=Date:Subject:To:Cc:References:From:In-Reply-To:From;\n\tb=YXJIkOrxoHmWeZ3eBPsZajn5jZaENSQvDVznOm/p00a2VQgr0n9G9Ngn2CfdTevKo\n\tmdwJ9E1hyJqhPeepB7QqnGZju5m9wn0fJgMDM7ZpkdMIDB1/CMCo2Fe3WIhLWceSIc\n\tRHWIhl5KPYenDyE3PWztskrCwXzyWAq/X8AmVGNA=","Message-ID":"","Date":"Fri, 16 Feb 2024 17:04:04 +0000","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH 2/5] libcamera: rkisp1: Standardise frame number tracking","Content-Language":"en-US","To":"Jacopo Mondi ,\n\tKieran Bingham ","References":"<20240212153532.179283-1-dan.scally@ideasonboard.com>\n\t<20240212153532.179283-3-dan.scally@ideasonboard.com>\n\t\n\t<170783064841.2629073.1101096632744172672@ping.linuxembedded.co.uk>\n\t","From":"Dan Scally ","Autocrypt":"addr=dan.scally@ideasonboard.com; keydata=\n\txsFNBGLydlEBEADa5O2s0AbUguprfvXOQun/0a8y2Vk6BqkQALgeD6KnXSWwaoCULp18etYW\n\tB31bfgrdphXQ5kUQibB0ADK8DERB4wrzrUb5CMxLBFE7mQty+v5NsP0OFNK9XTaAOcmD+Ove\n\teIjYvqurAaro91jrRVrS1gBRxIFqyPgNvwwL+alMZhn3/2jU2uvBmuRrgnc/e9cHKiuT3Dtq\n\tMHGPKL2m+plk+7tjMoQFfexoQ1JKugHAjxAhJfrkXh6uS6rc01bYCyo7ybzg53m1HLFJdNGX\n\tsUKR+dQpBs3SY4s66tc1sREJqdYyTsSZf80HjIeJjU/hRunRo4NjRIJwhvnK1GyjOvvuCKVU\n\tRWpY8dNjNu5OeAfdrlvFJOxIE9M8JuYCQTMULqd1NuzbpFMjc9524U3Cngs589T7qUMPb1H1\n\tNTA81LmtJ6Y+IV5/kiTUANflpzBwhu18Ok7kGyCq2a2jsOcVmk8gZNs04gyjuj8JziYwwLbf\n\tvzABwpFVcS8aR+nHIZV1HtOzyw8CsL8OySc3K9y+Y0NRpziMRvutrppzgyMb9V+N31mK9Mxl\n\t1YkgaTl4ciNWpdfUe0yxH03OCuHi3922qhPLF4XX5LN+NaVw5Xz2o3eeWklXdouxwV7QlN33\n\tu4+u2FWzKxDqO6WLQGjxPE0mVB4Gh5Pa1Vb0ct9Ctg0qElvtGQARAQABzShEYW4gU2NhbGx5\n\tIDxkYW4uc2NhbGx5QGlkZWFzb25ib2FyZC5jb20+wsGNBBMBCAA3FiEEsdtt8OWP7+8SNfQe\n\tkiQuh/L+GMQFAmLydlIFCQWjmoACGwMECwkIBwUVCAkKCwUWAgMBAAAKCRCSJC6H8v4YxDI2\n\tEAC2Gz0iyaXJkPInyshrREEWbo0CA6v5KKf3I/HlMPqkZ48bmGoYm4mEQGFWZJAT3K4ir8bg\n\tcEfs9V54gpbrZvdwS4abXbUK4WjKwEs8HK3XJv1WXUN2bsz5oEJWZUImh9gD3naiLLI9QMMm\n\tw/aZkT+NbN5/2KvChRWhdcha7+2Te4foOY66nIM+pw2FZM6zIkInLLUik2zXOhaZtqdeJZQi\n\tHSPU9xu7TRYN4cvdZAnSpG7gQqmLm5/uGZN1/sB3kHTustQtSXKMaIcD/DMNI3JN/t+RJVS7\n\tc0Jh/ThzTmhHyhxx3DRnDIy7kwMI4CFvmhkVC2uNs9kWsj1DuX5kt8513mvfw2OcX9UnNKmZ\n\tnhNCuF6DxVrL8wjOPuIpiEj3V+K7DFF1Cxw1/yrLs8dYdYh8T8vCY2CHBMsqpESROnTazboh\n\tAiQ2xMN1cyXtX11Qwqm5U3sykpLbx2BcmUUUEAKNsM//Zn81QXKG8vOx0ZdMfnzsCaCzt8f6\n\t9dcDBBI3tJ0BI9ByiocqUoL6759LM8qm18x3FYlxvuOs4wSGPfRVaA4yh0pgI+ModVC2Pu3y\n\tejE/IxeatGqJHh6Y+iJzskdi27uFkRixl7YJZvPJAbEn7kzSi98u/5ReEA8Qhc8KO/B7wprj\n\txjNMZNYd0Eth8+WkixHYj752NT5qshKJXcyUU87BTQRi8nZSARAAx0BJayh1Fhwbf4zoY56x\n\txHEpT6DwdTAYAetd3yiKClLVJadYxOpuqyWa1bdfQWPb+h4MeXbWw/53PBgn7gI2EA7ebIRC\n\tPJJhAIkeym7hHZoxqDQTGDJjxFEL11qF+U3rhWiL2Zt0Pl+zFq0eWYYVNiXjsIS4FI2+4m16\n\ttPbDWZFJnSZ828VGtRDQdhXfx3zyVX21lVx1bX4/OZvIET7sVUufkE4hrbqrrufre7wsjD1t\n\t8MQKSapVrr1RltpzPpScdoxknOSBRwOvpp57pJJe5A0L7+WxJ+vQoQXj0j+5tmIWOAV1qBQp\n\thyoyUk9JpPfntk2EKnZHWaApFp5TcL6c5LhUvV7F6XwOjGPuGlZQCWXee9dr7zym8iR3irWT\n\t+49bIh5PMlqSLXJDYbuyFQHFxoiNdVvvf7etvGfqFYVMPVjipqfEQ38ST2nkzx+KBICz7uwj\n\tJwLBdTXzGFKHQNckGMl7F5QdO/35An/QcxBnHVMXqaSd12tkJmoRVWduwuuoFfkTY5mUV3uX\n\txGj3iVCK4V+ezOYA7c2YolfRCNMTza6vcK/P4tDjjsyBBZrCCzhBvd4VVsnnlZhVaIxoky4K\n\taL+AP+zcQrUZmXmgZjXOLryGnsaeoVrIFyrU6ly90s1y3KLoPsDaTBMtnOdwxPmo1xisH8oL\n\ta/VRgpFBfojLPxMAEQEAAcLBfAQYAQgAJhYhBLHbbfDlj+/vEjX0HpIkLofy/hjEBQJi8nZT\n\tBQkFo5qAAhsMAAoJEJIkLofy/hjEXPcQAMIPNqiWiz/HKu9W4QIf1OMUpKn3YkVIj3p3gvfM\n\tRes4fGX94Ji599uLNrPoxKyaytC4R6BTxVriTJjWK8mbo9jZIRM4vkwkZZ2bu98EweSucxbp\n\tvjESsvMXGgxniqV/RQ/3T7LABYRoIUutARYq58p5HwSP0frF0fdFHYdTa2g7MYZl1ur2JzOC\n\tFHRpGadlNzKDE3fEdoMobxHB3Lm6FDml5GyBAA8+dQYVI0oDwJ3gpZPZ0J5Vx9RbqXe8RDuR\n\tdu90hvCJkq7/tzSQ0GeD3BwXb9/R/A4dVXhaDd91Q1qQXidI+2jwhx8iqiYxbT+DoAUkQRQy\n\txBtoCM1CxH7u45URUgD//fxYr3D4B1SlonA6vdaEdHZOGwECnDpTxecENMbz/Bx7qfrmd901\n\tD+N9SjIwrbVhhSyUXYnSUb8F+9g2RDY42Sk7GcYxIeON4VzKqWM7hpkXZ47pkK0YodO+dRKM\n\tyMcoUWrTK0Uz6UzUGKoJVbxmSW/EJLEGoI5p3NWxWtScEVv8mO49gqQdrRIOheZycDmHnItt\n\t9Qjv00uFhEwv2YfiyGk6iGF2W40s2pH2t6oeuGgmiZ7g6d0MEK8Ql/4zPItvr1c1rpwpXUC1\n\tu1kQWgtnNjFHX3KiYdqjcZeRBiry1X0zY+4Y24wUU0KsEewJwjhmCKAsju1RpdlPg2kC","In-Reply-To":"","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]