[{"id":36539,"web_url":"https://patchwork.libcamera.org/comment/36539/","msgid":"<176175175005.121034.12430575968261978585@isaac-ThinkPad-T16-Gen-2>","date":"2025-10-29T15:29:10","subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","submitter":{"id":215,"url":"https://patchwork.libcamera.org/api/people/215/","name":"Isaac Scott","email":"isaac.scott@ideasonboard.com"},"content":"Hi Stefan,\n\nThank you for the patch!\n\nQuoting Stefan Klug (2025-10-24 09:50:51)\n> The current code creates FrameInfo objects that tie together params,\n> stats and image buffers and expect that these always stay in these\n> groupings. However there are cases where these sequences get out of sync\n> (e.g. a scratch buffer is used for an image or a params buffer was too\n> late and therefore gets applied one frame later). As these situations\n> are timing related and cpu dependent it is impossible to guarantee the\n> initial grouping of buffers.\n> \n> Split the buffers into separate queues for stats, images and params.\n> Resynchronize on image buffers as soon as they are dequeued from V4L2 as\n> these are the only buffers tied to the libcamera requests coming from\n> the user application.\n> \n> Now handle the other buffer types according to their specific properties:\n> \n> Stats buffers only need to be tracked after dequing and can be tied to\n> the corresponding request after the corresponding image buffer was\n> dequeued.\n> \n> If params buffers get out of sync we can either inject the same set of\n> parameters twice or skip one set of params.\n> \n> If image buffers get out of sync we need to update the expected sensor\n> sequence, so that the next request is assigned the correct sensor\n> sequence.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v0.6:\n> - Fixed multiple assertions on start/stop and a few corner cases\n> - Fixed crash in dewarpRequestReady on stop\n> \n> Changes in v0.5\n> - Fixed possible use-after-free in RequestInfo\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 733 +++++++++++++++--------\n> 1 file changed, 475 insertions(+), 258 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index d83f7d787892..cd9364cb8950 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -6,6 +6,8 @@\n> */\n> \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> #include \n> @@ -61,44 +63,14 @@ LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> class PipelineHandlerRkISP1;\n> class RkISP1CameraData;\n> \n> -struct RkISP1FrameInfo {\n> - unsigned int frame;\n> - Request *request;\n> \n> - FrameBuffer *paramBuffer;\n> - FrameBuffer *statBuffer;\n> - FrameBuffer *mainPathBuffer;\n> - FrameBuffer *selfPathBuffer;\n> -\n> - bool paramDequeued;\n> - bool metadataProcessed;\n> -};\n> -\n> -class RkISP1Frames\n> -{\n> -public:\n> - RkISP1Frames(PipelineHandler *pipe);\n\n*snip*\n\n> + if (info.request == buffer->request()) {\n> + reqInfo = &info;\n> + }\n> + }\n> + }\n> \n> - const FrameMetadata &metadata = buffer->metadata();\n> - Request *request = info->request;\n> + if (!reqInfo) {\n> + if (data->usesDewarper_) {\n> + LOG(RkISP1Schedule, Info)\n> + << \"Image buffer ready, but no corresponding request\";\n> + availableMainPathBuffers_.push(buffer);\n\nOut of curiosity, is it possible here that if there is no request on the\nfirst frame, no frames will ever come through? What if theres never a\nrequest for the first image buffer?\n\nBest wishes,\nIsaac\n\n> + return;\n> + }\n> +\n> + LOG(RkISP1Schedule, Fatal)\n> + << \"Image buffer ready, but no corresponding request\";\n> + }\n> +\n> + Request *request = reqInfo->request;\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Image buffer ready: \" << buffer\n> + << \" Expected sequence: \" << reqInfo->sequence\n> + << \" got: \" << metadata.sequence;\n> +\n> + uint32_t sequence = metadata.sequence;\n> +\n> + /*\n> + * If the frame was cancelled, the metadata sequnce is usually wrong and\n> + * we assume that our guess was right.\n> + */\n> + if (metadata.status == FrameMetadata::FrameCancelled)\n> + sequence = reqInfo->sequence;\n> +\n> + /* We now know the buffer sequence that belongs to this request */\n> + int droppedFrames = imageSyncHelper_.gotFrame(reqInfo->sequence, sequence);\n> + if (droppedFrames != 0)\n> + LOG(RkISP1Schedule, Warning)\n> + << \"Frame \" << reqInfo->sequence << \": Dropped \"\n> + << droppedFrames << \" frames\";\n> +\n> + reqInfo->sequence = sequence;\n> + reqInfo->sequenceValid = true;\n> +\n> + if (sensorFrameInfos_[sequence].metadataProcessed) {\n> + LOG(RkISP1Schedule, Debug)\n> + << \"Apply stored metadata \" << reqInfo->sequence;\n> + request->metadata().merge(sensorFrameInfos_[sequence].metadata);\n> + }\n> \n> if (metadata.status != FrameMetadata::FrameCancelled) {\n> /*\n> * Record the sensor's timestamp in the request metadata.\n> *\n> - * \\todo The sensor timestamp should be better estimated by connecting\n> - * to the V4L2Device::frameStart signal.\n> + * \\todo The sensor timestamp should be better estimated by\n> + * connecting to the V4L2Device::frameStart signal.\n> */\n> request->metadata().set(controls::SensorTimestamp,\n> metadata.timestamp);\n> \n> + /* In raw mode call processStats() to fill the metadata */\n> if (isRaw_) {\n> const ControlList &ctrls =\n> data->delayedCtrls_->get(metadata.sequence);\n> - data->ipa_->processStats(info->frame, 0, ctrls);\n> + data->ipa_->processStats(reqInfo->sequence, 0, ctrls);\n> }\n> } else {\n> - if (isRaw_)\n> - info->metadataProcessed = true;\n> + /* No need to block waiting for metedata on that frame. */\n> + sensorFrameInfos_[sequence].metadataProcessed = true;\n> }\n> \n> if (!data->usesDewarper_) {\n> - completeBuffer(request, buffer);\n> - tryCompleteRequest(info);\n> + completeBuffer(reqInfo->request, buffer);\n> + tryCompleteRequests();\n> \n> return;\n> }\n> \n> - /* Do not queue cancelled frames to dewarper. */\n> + /* Do not queue cancelled frames to the dewarper. */\n> if (metadata.status == FrameMetadata::FrameCancelled) {\n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> return;\n> }\n> \n> @@ -1869,10 +2026,14 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> dewarper_->applyVertexMap(&data->mainPathStream_, dewarpRequest);\n> \n> /*\n> - * Queue input and output buffers to the dewarper. The output\n> - * buffers for the dewarper are the buffers of the request, supplied\n> - * by the application.\n> + * Queue input and output buffers to the dewarper. The output buffers\n> + * for the dewarper are the buffers of the request, supplied by the\n> + * application.\n> */\n> + DewarpBufferInfo dewarpInfo{ buffer, reqInfo->request->findBuffer(&data->mainPathStream_) };\n> + queuedDewarpBuffers_.push_back(dewarpInfo);\n> + LOG(RkISP1Schedule, Debug) << \"Queue dewarper \" << dewarpInfo.inputBuffer\n> + << \" \" << dewarpInfo.outputBuffer;\n> int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);\n> if (ret < 0) {\n> LOG(RkISP1, Error) << \"Failed to queue buffers to dewarper: -\"\n> @@ -1882,7 +2043,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> if (dewarpRequest)\n> dewarpRequestReady(dewarpRequest);\n> \n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> \n> return;\n> }\n> @@ -1895,7 +2056,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> /* Push it back into the queue. */\n> dewarpRequestReady(dewarpRequest);\n> \n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> }\n> }\n> \n> @@ -1919,29 +2080,59 @@ void PipelineHandlerRkISP1::dewarpRequestReady(V4L2Request *request)\n> \n> void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> {\n> - ASSERT(activeCamera_);\n> - RkISP1CameraData *data = cameraData(activeCamera_);\n> Request *request = buffer->request();\n> + const FrameMetadata &metadata = buffer->metadata();\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> - if (!info)\n> - return;\n> + /*\n> + * After stopping the dewarper, the buffers are returned out of order.\n> + * Search the list for the corresponding info and handle it. In regular\n> + * operation it will always be the first entry.\n> + */\n> + for (DewarpBufferInfo &dwInfo : queuedDewarpBuffers_) {\n> + if (dwInfo.outputBuffer != buffer)\n> + continue;\n> \n> - completeBuffer(request, buffer);\n> - tryCompleteRequest(info);\n> + availableMainPathBuffers_.push(dwInfo.inputBuffer);\n> + dwInfo.inputBuffer = nullptr;\n> + dwInfo.outputBuffer = nullptr;\n> +\n> + if (metadata.status == FrameMetadata::FrameCancelled)\n> + buffer->_d()->cancel();\n> +\n> + completeBuffer(request, buffer);\n> + }\n> +\n> + while (!queuedDewarpBuffers_.empty() &&\n> + queuedDewarpBuffers_.front().inputBuffer == nullptr)\n> + queuedDewarpBuffers_.pop_front();\n> +\n> + tryCompleteRequests();\n> + queueInternalBuffers();\n> }\n> \n> void PipelineHandlerRkISP1::paramBufferReady(FrameBuffer *buffer)\n> {\n> - ASSERT(activeCamera_);\n> - RkISP1CameraData *data = cameraData(activeCamera_);\n> + LOG(RkISP1Schedule, Debug) << \"Param buffer ready \" << buffer;\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> - if (!info)\n> + /* After stream off, the buffers are returned out of order, so\n> + * we don't care about the rest.\n> + */\n> + if (!running_) {\n> + availableParamBuffers_.push(buffer);\n> return;\n> + }\n> \n> - info->paramDequeued = true;\n> - tryCompleteRequest(info);\n> + ParamBufferInfo pInfo = queuedParamBuffers_.front();\n> + queuedParamBuffers_.pop();\n> +\n> + ASSERT(pInfo.buffer == buffer);\n> +\n> + size_t metaSequence = buffer->metadata().sequence;\n> + LOG(RkISP1Schedule, Debug) << \"Params buffer ready \"\n> + << \" Expected: \" << pInfo.expectedSequence\n> + << \" got: \" << metaSequence;\n> + paramsSyncHelper_.gotFrame(pInfo.expectedSequence, metaSequence);\n> + availableParamBuffers_.push(buffer);\n> }\n> \n> void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> @@ -1949,21 +2140,47 @@ void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> ASSERT(activeCamera_);\n> RkISP1CameraData *data = cameraData(activeCamera_);\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> - if (!info)\n> - return;\n> + size_t sequence = buffer->metadata().sequence;\n> \n> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> - info->metadataProcessed = true;\n> - tryCompleteRequest(info);\n> + LOG(RkISP1Schedule, Warning) << \"Stats cancelled \" << sequence;\n> + /*\n> + * We can't assume that the sequence of the stat buffer is valid,\n> + * so there is nothing left to do.\n> + */\n> + availableStatBuffers_.push(buffer);\n> + return;\n> + }\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Stats ready \" << sequence;\n> +\n> + if (nextStatsToProcess_ != sequence)\n> + LOG(RkISP1Schedule, Warning) << \"Stats sequence out of sync.\"\n> + << \" Expected: \" << nextStatsToProcess_\n> + << \" got: \" << sequence;\n> +\n> + if (nextStatsToProcess_ > sequence) {\n> + LOG(RkISP1Schedule, Warning) << \"Stats were too late. Ignored\";\n> + availableStatBuffers_.push(buffer);\n> return;\n> }\n> \n> - if (data->frame_ <= buffer->metadata().sequence)\n> - data->frame_ = buffer->metadata().sequence + 1;\n> + /* Send empty stats to ensure metadata gets created*/\n> + while (nextStatsToProcess_ < sequence) {\n> + LOG(RkISP1Schedule, Warning) << \"Send empty stats to fill metadata\";\n> + data->ipa_->processStats(nextStatsToProcess_, 0,\n> + data->delayedCtrls_->get(nextStatsToProcess_));\n> +\n> + nextStatsToProcess_++;\n> + }\n> +\n> + nextStatsToProcess_++;\n> +\n> + sensorFrameInfos_[sequence].statsBuffer = buffer;\n> \n> - data->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> - data->delayedCtrls_->get(buffer->metadata().sequence));\n> + LOG(RkISP1Schedule, Debug) << \"Process stats \" << sequence;\n> + data->ipa_->processStats(sequence, buffer->cookie(),\n> + data->delayedCtrls_->get(sequence));\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n> -- \n> 2.48.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 94915C3259\n\tfor ;\n\tWed, 29 Oct 2025 15:29:18 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 89F4A60867;\n\tWed, 29 Oct 2025 16:29:17 +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 561F560453\n\tfor ;\n\tWed, 29 Oct 2025 16:29:15 +0100 (CET)","from thinkpad.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust6594.18-1.cable.virginm.net [86.31.185.195])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6665F1E27;\n\tWed, 29 Oct 2025 16:27:24 +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=\"CZHtJ5a8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1761751644;\n\tbh=bgkgrCPCz2t2EDtV0KvOeU+ih2XuhwDBWOvTRrAVe8I=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=CZHtJ5a8y7lvEkmLqHmdzInk4HHMIS83lNShlrSk+jOClPWsF86rtmKieyI3kIA2k\n\tyXRsDAQwh4rX98h/cfDlW1F7vHEOofEpyABVFPRDyIJgbs2WXGjVJJr1hNRLQm5qj6\n\tZiA7lzTSOv31zf8YuFB3RDo5H5hkZutScH9M+k6M=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251024085130.995967-28-stefan.klug@ideasonboard.com>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-28-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","From":"Isaac Scott ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 29 Oct 2025 15:29:10 +0000","Message-ID":"<176175175005.121034.12430575968261978585@isaac-ThinkPad-T16-Gen-2>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":36752,"web_url":"https://patchwork.libcamera.org/comment/36752/","msgid":"<176253247820.509848.2486493253618729803@localhost>","date":"2025-11-07T16:21:18","subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","submitter":{"id":184,"url":"https://patchwork.libcamera.org/api/people/184/","name":"Stefan Klug","email":"stefan.klug@ideasonboard.com"},"content":"Hi Isaac,\n\nThanks for diving into this.\n\nQuoting Isaac Scott (2025-10-29 16:29:10)\n> Hi Stefan,\n> \n> Thank you for the patch!\n> \n> Quoting Stefan Klug (2025-10-24 09:50:51)\n> > The current code creates FrameInfo objects that tie together params,\n> > stats and image buffers and expect that these always stay in these\n> > groupings. However there are cases where these sequences get out of sync\n> > (e.g. a scratch buffer is used for an image or a params buffer was too\n> > late and therefore gets applied one frame later). As these situations\n> > are timing related and cpu dependent it is impossible to guarantee the\n> > initial grouping of buffers.\n> > \n> > Split the buffers into separate queues for stats, images and params.\n> > Resynchronize on image buffers as soon as they are dequeued from V4L2 as\n> > these are the only buffers tied to the libcamera requests coming from\n> > the user application.\n> > \n> > Now handle the other buffer types according to their specific properties:\n> > \n> > Stats buffers only need to be tracked after dequing and can be tied to\n> > the corresponding request after the corresponding image buffer was\n> > dequeued.\n> > \n> > If params buffers get out of sync we can either inject the same set of\n> > parameters twice or skip one set of params.\n> > \n> > If image buffers get out of sync we need to update the expected sensor\n> > sequence, so that the next request is assigned the correct sensor\n> > sequence.\n> > \n> > Signed-off-by: Stefan Klug \n> > \n> > ---\n> > \n> > Changes in v0.6:\n> > - Fixed multiple assertions on start/stop and a few corner cases\n> > - Fixed crash in dewarpRequestReady on stop\n> > \n> > Changes in v0.5\n> > - Fixed possible use-after-free in RequestInfo\n> > ---\n> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 733 +++++++++++++++--------\n> > 1 file changed, 475 insertions(+), 258 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index d83f7d787892..cd9364cb8950 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -6,6 +6,8 @@\n> > */\n> > \n> > #include \n> > +#include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -61,44 +63,14 @@ LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> > class PipelineHandlerRkISP1;\n> > class RkISP1CameraData;\n> > \n> > -struct RkISP1FrameInfo {\n> > - unsigned int frame;\n> > - Request *request;\n> > \n> > - FrameBuffer *paramBuffer;\n> > - FrameBuffer *statBuffer;\n> > - FrameBuffer *mainPathBuffer;\n> > - FrameBuffer *selfPathBuffer;\n> > -\n> > - bool paramDequeued;\n> > - bool metadataProcessed;\n> > -};\n> > -\n> > -class RkISP1Frames\n> > -{\n> > -public:\n> > - RkISP1Frames(PipelineHandler *pipe);\n> \n> *snip*\n> \n> > + if (info.request == buffer->request()) {\n> > + reqInfo = &info;\n> > + }\n> > + }\n> > + }\n> > \n> > - const FrameMetadata &metadata = buffer->metadata();\n> > - Request *request = info->request;\n> > + if (!reqInfo) {\n> > + if (data->usesDewarper_) {\n> > + LOG(RkISP1Schedule, Info)\n> > + << \"Image buffer ready, but no corresponding request\";\n> > + availableMainPathBuffers_.push(buffer);\n> \n> Out of curiosity, is it possible here that if there is no request on the\n> first frame, no frames will ever come through? What if theres never a\n> request for the first image buffer?\n\nI don't see how. If no requests get queued after starting the camera,\nthe buffer loop will start and you'll see this warning. When the first\nrequest is then queued, reqInfo will not be null, and the request will\nbe handled.\n\nCould you elaborate a bit more, where it would break?\n\nBest regards,\nStefan\n\n> \n> Best wishes,\n> Isaac\n> \n> > + return;\n> > + }\n> > +\n> > + LOG(RkISP1Schedule, Fatal)\n> > + << \"Image buffer ready, but no corresponding request\";\n> > + }\n> > +\n> > + Request *request = reqInfo->request;\n> > +\n> > + LOG(RkISP1Schedule, Debug) << \"Image buffer ready: \" << buffer\n> > + << \" Expected sequence: \" << reqInfo->sequence\n> > + << \" got: \" << metadata.sequence;\n> > +\n> > + uint32_t sequence = metadata.sequence;\n> > +\n> > + /*\n> > + * If the frame was cancelled, the metadata sequnce is usually wrong and\n> > + * we assume that our guess was right.\n> > + */\n> > + if (metadata.status == FrameMetadata::FrameCancelled)\n> > + sequence = reqInfo->sequence;\n> > +\n> > + /* We now know the buffer sequence that belongs to this request */\n> > + int droppedFrames = imageSyncHelper_.gotFrame(reqInfo->sequence, sequence);\n> > + if (droppedFrames != 0)\n> > + LOG(RkISP1Schedule, Warning)\n> > + << \"Frame \" << reqInfo->sequence << \": Dropped \"\n> > + << droppedFrames << \" frames\";\n> > +\n> > + reqInfo->sequence = sequence;\n> > + reqInfo->sequenceValid = true;\n> > +\n> > + if (sensorFrameInfos_[sequence].metadataProcessed) {\n> > + LOG(RkISP1Schedule, Debug)\n> > + << \"Apply stored metadata \" << reqInfo->sequence;\n> > + request->metadata().merge(sensorFrameInfos_[sequence].metadata);\n> > + }\n> > \n> > if (metadata.status != FrameMetadata::FrameCancelled) {\n> > /*\n> > * Record the sensor's timestamp in the request metadata.\n> > *\n> > - * \\todo The sensor timestamp should be better estimated by connecting\n> > - * to the V4L2Device::frameStart signal.\n> > + * \\todo The sensor timestamp should be better estimated by\n> > + * connecting to the V4L2Device::frameStart signal.\n> > */\n> > request->metadata().set(controls::SensorTimestamp,\n> > metadata.timestamp);\n> > \n> > + /* In raw mode call processStats() to fill the metadata */\n> > if (isRaw_) {\n> > const ControlList &ctrls =\n> > data->delayedCtrls_->get(metadata.sequence);\n> > - data->ipa_->processStats(info->frame, 0, ctrls);\n> > + data->ipa_->processStats(reqInfo->sequence, 0, ctrls);\n> > }\n> > } else {\n> > - if (isRaw_)\n> > - info->metadataProcessed = true;\n> > + /* No need to block waiting for metedata on that frame. */\n> > + sensorFrameInfos_[sequence].metadataProcessed = true;\n> > }\n> > \n> > if (!data->usesDewarper_) {\n> > - completeBuffer(request, buffer);\n> > - tryCompleteRequest(info);\n> > + completeBuffer(reqInfo->request, buffer);\n> > + tryCompleteRequests();\n> > \n> > return;\n> > }\n> > \n> > - /* Do not queue cancelled frames to dewarper. */\n> > + /* Do not queue cancelled frames to the dewarper. */\n> > if (metadata.status == FrameMetadata::FrameCancelled) {\n> > - cancelDewarpRequest(info);\n> > + cancelDewarpRequest(reqInfo->request);\n> > return;\n> > }\n> > \n> > @@ -1869,10 +2026,14 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> > dewarper_->applyVertexMap(&data->mainPathStream_, dewarpRequest);\n> > \n> > /*\n> > - * Queue input and output buffers to the dewarper. The output\n> > - * buffers for the dewarper are the buffers of the request, supplied\n> > - * by the application.\n> > + * Queue input and output buffers to the dewarper. The output buffers\n> > + * for the dewarper are the buffers of the request, supplied by the\n> > + * application.\n> > */\n> > + DewarpBufferInfo dewarpInfo{ buffer, reqInfo->request->findBuffer(&data->mainPathStream_) };\n> > + queuedDewarpBuffers_.push_back(dewarpInfo);\n> > + LOG(RkISP1Schedule, Debug) << \"Queue dewarper \" << dewarpInfo.inputBuffer\n> > + << \" \" << dewarpInfo.outputBuffer;\n> > int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);\n> > if (ret < 0) {\n> > LOG(RkISP1, Error) << \"Failed to queue buffers to dewarper: -\"\n> > @@ -1882,7 +2043,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> > if (dewarpRequest)\n> > dewarpRequestReady(dewarpRequest);\n> > \n> > - cancelDewarpRequest(info);\n> > + cancelDewarpRequest(reqInfo->request);\n> > \n> > return;\n> > }\n> > @@ -1895,7 +2056,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> > /* Push it back into the queue. */\n> > dewarpRequestReady(dewarpRequest);\n> > \n> > - cancelDewarpRequest(info);\n> > + cancelDewarpRequest(reqInfo->request);\n> > }\n> > }\n> > \n> > @@ -1919,29 +2080,59 @@ void PipelineHandlerRkISP1::dewarpRequestReady(V4L2Request *request)\n> > \n> > void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> > {\n> > - ASSERT(activeCamera_);\n> > - RkISP1CameraData *data = cameraData(activeCamera_);\n> > Request *request = buffer->request();\n> > + const FrameMetadata &metadata = buffer->metadata();\n> > \n> > - RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> > - if (!info)\n> > - return;\n> > + /*\n> > + * After stopping the dewarper, the buffers are returned out of order.\n> > + * Search the list for the corresponding info and handle it. In regular\n> > + * operation it will always be the first entry.\n> > + */\n> > + for (DewarpBufferInfo &dwInfo : queuedDewarpBuffers_) {\n> > + if (dwInfo.outputBuffer != buffer)\n> > + continue;\n> > \n> > - completeBuffer(request, buffer);\n> > - tryCompleteRequest(info);\n> > + availableMainPathBuffers_.push(dwInfo.inputBuffer);\n> > + dwInfo.inputBuffer = nullptr;\n> > + dwInfo.outputBuffer = nullptr;\n> > +\n> > + if (metadata.status == FrameMetadata::FrameCancelled)\n> > + buffer->_d()->cancel();\n> > +\n> > + completeBuffer(request, buffer);\n> > + }\n> > +\n> > + while (!queuedDewarpBuffers_.empty() &&\n> > + queuedDewarpBuffers_.front().inputBuffer == nullptr)\n> > + queuedDewarpBuffers_.pop_front();\n> > +\n> > + tryCompleteRequests();\n> > + queueInternalBuffers();\n> > }\n> > \n> > void PipelineHandlerRkISP1::paramBufferReady(FrameBuffer *buffer)\n> > {\n> > - ASSERT(activeCamera_);\n> > - RkISP1CameraData *data = cameraData(activeCamera_);\n> > + LOG(RkISP1Schedule, Debug) << \"Param buffer ready \" << buffer;\n> > \n> > - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > - if (!info)\n> > + /* After stream off, the buffers are returned out of order, so\n> > + * we don't care about the rest.\n> > + */\n> > + if (!running_) {\n> > + availableParamBuffers_.push(buffer);\n> > return;\n> > + }\n> > \n> > - info->paramDequeued = true;\n> > - tryCompleteRequest(info);\n> > + ParamBufferInfo pInfo = queuedParamBuffers_.front();\n> > + queuedParamBuffers_.pop();\n> > +\n> > + ASSERT(pInfo.buffer == buffer);\n> > +\n> > + size_t metaSequence = buffer->metadata().sequence;\n> > + LOG(RkISP1Schedule, Debug) << \"Params buffer ready \"\n> > + << \" Expected: \" << pInfo.expectedSequence\n> > + << \" got: \" << metaSequence;\n> > + paramsSyncHelper_.gotFrame(pInfo.expectedSequence, metaSequence);\n> > + availableParamBuffers_.push(buffer);\n> > }\n> > \n> > void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> > @@ -1949,21 +2140,47 @@ void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> > ASSERT(activeCamera_);\n> > RkISP1CameraData *data = cameraData(activeCamera_);\n> > \n> > - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> > - if (!info)\n> > - return;\n> > + size_t sequence = buffer->metadata().sequence;\n> > \n> > if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> > - info->metadataProcessed = true;\n> > - tryCompleteRequest(info);\n> > + LOG(RkISP1Schedule, Warning) << \"Stats cancelled \" << sequence;\n> > + /*\n> > + * We can't assume that the sequence of the stat buffer is valid,\n> > + * so there is nothing left to do.\n> > + */\n> > + availableStatBuffers_.push(buffer);\n> > + return;\n> > + }\n> > +\n> > + LOG(RkISP1Schedule, Debug) << \"Stats ready \" << sequence;\n> > +\n> > + if (nextStatsToProcess_ != sequence)\n> > + LOG(RkISP1Schedule, Warning) << \"Stats sequence out of sync.\"\n> > + << \" Expected: \" << nextStatsToProcess_\n> > + << \" got: \" << sequence;\n> > +\n> > + if (nextStatsToProcess_ > sequence) {\n> > + LOG(RkISP1Schedule, Warning) << \"Stats were too late. Ignored\";\n> > + availableStatBuffers_.push(buffer);\n> > return;\n> > }\n> > \n> > - if (data->frame_ <= buffer->metadata().sequence)\n> > - data->frame_ = buffer->metadata().sequence + 1;\n> > + /* Send empty stats to ensure metadata gets created*/\n> > + while (nextStatsToProcess_ < sequence) {\n> > + LOG(RkISP1Schedule, Warning) << \"Send empty stats to fill metadata\";\n> > + data->ipa_->processStats(nextStatsToProcess_, 0,\n> > + data->delayedCtrls_->get(nextStatsToProcess_));\n> > +\n> > + nextStatsToProcess_++;\n> > + }\n> > +\n> > + nextStatsToProcess_++;\n> > +\n> > + sensorFrameInfos_[sequence].statsBuffer = buffer;\n> > \n> > - data->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> > - data->delayedCtrls_->get(buffer->metadata().sequence));\n> > + LOG(RkISP1Schedule, Debug) << \"Process stats \" << sequence;\n> > + data->ipa_->processStats(sequence, buffer->cookie(),\n> > + data->delayedCtrls_->get(sequence));\n> > }\n> > \n> > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n> > -- \n> > 2.48.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 1C271C3241\n\tfor ;\n\tFri, 7 Nov 2025 16:21:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61D32609D8;\n\tFri, 7 Nov 2025 17:21:23 +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 99174608CF\n\tfor ;\n\tFri, 7 Nov 2025 17:21:21 +0100 (CET)","from ideasonboard.com (unknown\n\t[IPv6:2a00:6020:448c:6c00:8809:eccd:216c:b9a0])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 923863C8;\n\tFri, 7 Nov 2025 17:19:25 +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=\"jtnrHLB8\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1762532365;\n\tbh=vS3iYkrMAEqsFZLXquP9GHjDBnOvxU0wkMyKHiPdvus=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=jtnrHLB8Y1ZY8VCHLlhyC+r7NDxQHt7TYiW9rXSDe6cjgJ5vN0nuWZ8hzzsTt+POT\n\t669fF9t7ZQ1OTSP/eoC/bIpn6fNrx2IJ3j7sIAlPikf/FmcDQyEh9W8TGete4repJL\n\tr3gZXWm3fC7i7jJ987oMBW5puaB15PdRw1NhBodU=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<176175175005.121034.12430575968261978585@isaac-ThinkPad-T16-Gen-2>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-28-stefan.klug@ideasonboard.com>\n\t<176175175005.121034.12430575968261978585@isaac-ThinkPad-T16-Gen-2>","Subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","From":"Stefan Klug ","Cc":"","To":"Isaac Scott ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Fri, 07 Nov 2025 17:21:18 +0100","Message-ID":"<176253247820.509848.2486493253618729803@localhost>","User-Agent":"alot/0.12.dev8+g2c003385c862.d20250602","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":37491,"web_url":"https://patchwork.libcamera.org/comment/37491/","msgid":"<176768722599.2470890.13611601803903542065@neptunite.rasen.tech>","date":"2026-01-06T08:13:45","subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Stefan,\n\nThanks for the patch.\n\nQuoting Stefan Klug (2025-10-24 17:50:51)\n> The current code creates FrameInfo objects that tie together params,\n> stats and image buffers and expect that these always stay in these\n> groupings. However there are cases where these sequences get out of sync\n> (e.g. a scratch buffer is used for an image or a params buffer was too\n> late and therefore gets applied one frame later). As these situations\n> are timing related and cpu dependent it is impossible to guarantee the\n> initial grouping of buffers.\n> \n> Split the buffers into separate queues for stats, images and params.\n> Resynchronize on image buffers as soon as they are dequeued from V4L2 as\n> these are the only buffers tied to the libcamera requests coming from\n> the user application.\n> \n> Now handle the other buffer types according to their specific properties:\n> \n> Stats buffers only need to be tracked after dequing and can be tied to\n> the corresponding request after the corresponding image buffer was\n> dequeued.\n> \n> If params buffers get out of sync we can either inject the same set of\n> parameters twice or skip one set of params.\n> \n> If image buffers get out of sync we need to update the expected sensor\n> sequence, so that the next request is assigned the correct sensor\n> sequence.\n> \n> Signed-off-by: Stefan Klug \n> \n> ---\n> \n> Changes in v0.6:\n> - Fixed multiple assertions on start/stop and a few corner cases\n> - Fixed crash in dewarpRequestReady on stop\n> \n> Changes in v0.5\n> - Fixed possible use-after-free in RequestInfo\n> ---\n> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 733 +++++++++++++++--------\n> 1 file changed, 475 insertions(+), 258 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index d83f7d787892..cd9364cb8950 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -6,6 +6,8 @@\n> */\n> \n> #include \n> +#include \n> +#include \n> #include \n> #include \n> #include \n> @@ -61,44 +63,14 @@ LOG_DEFINE_CATEGORY(RkISP1Schedule)\n> class PipelineHandlerRkISP1;\n> class RkISP1CameraData;\n> \n> -struct RkISP1FrameInfo {\n> - unsigned int frame;\n> - Request *request;\n> \n> - FrameBuffer *paramBuffer;\n> - FrameBuffer *statBuffer;\n> - FrameBuffer *mainPathBuffer;\n> - FrameBuffer *selfPathBuffer;\n> -\n> - bool paramDequeued;\n> - bool metadataProcessed;\n> -};\n> -\n> -class RkISP1Frames\n> -{\n> -public:\n> - RkISP1Frames(PipelineHandler *pipe);\n> -\n> - RkISP1FrameInfo *create(const RkISP1CameraData *data, Request *request,\n> - bool isRaw);\n> - int destroy(unsigned int frame);\n> - void clear();\n> -\n> - RkISP1FrameInfo *find(unsigned int frame);\n> - RkISP1FrameInfo *find(FrameBuffer *buffer);\n> - RkISP1FrameInfo *find(Request *request);\n> -\n> -private:\n> - PipelineHandlerRkISP1 *pipe_;\n> - std::map frameInfo_;\n> -};\n> \n> class RkISP1CameraData : public Camera::Private\n> {\n> public:\n> RkISP1CameraData(PipelineHandler *pipe, RkISP1MainPath *mainPath,\n> RkISP1SelfPath *selfPath)\n> - : Camera::Private(pipe), frame_(0), frameInfo_(pipe),\n> + : Camera::Private(pipe), frame_(0),\n> mainPath_(mainPath), selfPath_(selfPath)\n> {\n> }\n> @@ -111,9 +83,12 @@ public:\n> Stream selfPathStream_;\n> std::unique_ptr sensor_;\n> std::unique_ptr delayedCtrls_;\n> + /*\n> + * The sensor frame sequence of the last request queued to the pipeline\n> + * handler.\n> + */\n> unsigned int frame_;\n> std::vector ipaBuffers_;\n> - RkISP1Frames frameInfo_;\n> \n> RkISP1MainPath *mainPath_;\n> RkISP1SelfPath *selfPath_;\n> @@ -169,21 +144,54 @@ private:\n> Transform combinedTransform_;\n> };\n> \n> +struct SensorFrameInfo {\n> + Request *request = nullptr;\n> + FrameBuffer *statsBuffer = nullptr;\n> + ControlList metadata;\n> + bool metadataProcessed = false;\n> +};\n> +\n> +struct RequestInfo {\n> + Request *request = nullptr;\n> + /*\n> + * The estimated sensor sequence for this request. Only reliable when\n> + * sequenceValid is true\n> + */\n> + size_t sequence = 0;\n> + bool sequenceValid = false;\n> +};\n> +\n> +struct ParamBufferInfo {\n> + FrameBuffer *buffer = nullptr;\n> + size_t expectedSequence = 0;\n> +};\n> +\n> +struct DewarpBufferInfo {\n> + FrameBuffer *inputBuffer;\n> + FrameBuffer *outputBuffer;\n> +};\n> +\n> namespace {\n> \n> /*\n> - * Maximum number of requests that shall be queued into the pipeline to keep\n> - * the regulation fast.\n> - * \\todo This needs revisiting as soon as buffers got decoupled from requests\n> - * and/or a fast path for controls was implemented.\n> + * This many buffers ensures that the pipeline runs smoothly, without frame\n> + * drops.\n> */\n> -static constexpr unsigned int kRkISP1MaxQueuedRequests = 4;\n> +static constexpr unsigned int kRkISP1MinBufferCount = 6;\n> \n> /*\n> - * This many internal buffers (or rather parameter and statistics buffer\n> - * pairs) ensures that the pipeline runs smoothly, without frame drops.\n> + * This many internal buffers (params and stats) are needed for smooth operation\n> + * \\todo In high framerate or high cpu load situations it might be necessary to\n> + * increase this number. \\todo: This also relates to max sensor delay and must\n> + * always be >= maxSensor delay\n> */\n> -static constexpr unsigned int kRkISP1MinBufferCount = 4;\n> +static constexpr unsigned int kRkISP1InternalBufferCount = 4;\n> +\n> +/*\n> + * This many internal image buffers between ISP and dewarper are needed for\n> + * smooth operation.\n> + */\n> +static constexpr unsigned int kRkISP1DewarpImageBufferCount = 4;\n> \n> /*\n> * This flag allows to use dynamic dewarp maps to support pan, zoom, rotate when\n> @@ -223,12 +231,11 @@ private:\n> \n> friend RkISP1CameraData;\n> friend RkISP1CameraConfiguration;\n> - friend RkISP1Frames;\n> \n> int initLinks(Camera *camera, const RkISP1CameraConfiguration &config);\n> int createCamera(MediaEntity *sensor);\n> - void tryCompleteRequest(RkISP1FrameInfo *info);\n> - void cancelDewarpRequest(RkISP1FrameInfo *info);\n> + void tryCompleteRequests();\n> + void cancelDewarpRequest(Request *request);\n> void imageBufferReady(FrameBuffer *buffer);\n> void paramBufferReady(FrameBuffer *buffer);\n> void statBufferReady(FrameBuffer *buffer);\n> @@ -236,6 +243,9 @@ private:\n> void dewarpBufferReady(FrameBuffer *buffer);\n> void frameStart(uint32_t sequence);\n> \n> + void queueInternalBuffers();\n> + void computeParamBuffers(uint32_t maxSequence);\n> +\n> int allocateBuffers(Camera *camera);\n> int freeBuffers(Camera *camera);\n> \n> @@ -261,139 +271,32 @@ private:\n> std::vector> dewarpRequests_;\n> std::queue availableDewarpRequests_;\n> \n> + bool running_ = false;\n> +\n> std::vector> paramBuffers_;\n> std::vector> statBuffers_;\n> std::queue availableParamBuffers_;\n> std::queue availableStatBuffers_;\n\nIs there a reason why these aren't converted to using ParamBufferInfo (and\nSensorFrameInfo)? afaik it's parallel to computingParamBuffers_ and\nqueuedParamBuffers_ (and sensorFrameInfos_) below.\n\nComing back to this after having reviewed everything else, I think I kind of\nsee it now, that you unwrap the info-structs because you discard the sequence\nwhen you queue them, since you'll re-estimate the sequence number and zip it\nback up together with the buffer again after they're dequeued.\n\n> \n> - Camera *activeCamera_;\n> -};\n> + std::deque queuedRequests_;\n> \n> -RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)\n> - : pipe_(static_cast(pipe))\n> -{\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::create(const RkISP1CameraData *data, Request *request,\n> - bool isRaw)\n> -{\n> - unsigned int frame = data->frame_;\n> -\n> - FrameBuffer *paramBuffer = nullptr;\n> - FrameBuffer *statBuffer = nullptr;\n> - FrameBuffer *mainPathBuffer = nullptr;\n> - FrameBuffer *selfPathBuffer = nullptr;\n> + std::map sensorFrameInfos_;\n> \n> - if (!isRaw) {\n> - if (pipe_->availableParamBuffers_.empty()) {\n> - LOG(RkISP1, Error) << \"Parameters buffer underrun\";\n> - return nullptr;\n> - }\n> -\n> - if (pipe_->availableStatBuffers_.empty()) {\n> - LOG(RkISP1, Error) << \"Statistic buffer underrun\";\n> - return nullptr;\n> - }\n> -\n> - paramBuffer = pipe_->availableParamBuffers_.front();\n> - pipe_->availableParamBuffers_.pop();\n> -\n> - statBuffer = pipe_->availableStatBuffers_.front();\n> - pipe_->availableStatBuffers_.pop();\n> -\n> - if (data->usesDewarper_) {\n> - mainPathBuffer = pipe_->availableMainPathBuffers_.front();\n> - pipe_->availableMainPathBuffers_.pop();\n> - }\n> - }\n> + std::deque queuedDewarpBuffers_;\n> + SequenceSyncHelper paramsSyncHelper_;\n> + SequenceSyncHelper imageSyncHelper_;\n> \n> - if (!mainPathBuffer)\n> - mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> - selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> + std::queue computingParamBuffers_;\n> + std::queue queuedParamBuffers_;\n> \n> - auto [it, inserted] = frameInfo_.try_emplace(frame);\n> - ASSERT(inserted);\n> \n> - auto &info = it->second;\n> + uint32_t nextParamsSequence_;\n> + uint32_t nextStatsToProcess_;\n> \n> - info.frame = frame;\n> - info.request = request;\n> - info.paramBuffer = paramBuffer;\n> - info.mainPathBuffer = mainPathBuffer;\n> - info.selfPathBuffer = selfPathBuffer;\n> - info.statBuffer = statBuffer;\n> - info.paramDequeued = false;\n> - info.metadataProcessed = false;\n> -\n> - return &info;\n> -}\n> -\n> -int RkISP1Frames::destroy(unsigned int frame)\n> -{\n> - auto it = frameInfo_.find(frame);\n> - if (it == frameInfo_.end())\n> - return -ENOENT;\n> -\n> - auto &info = it->second;\n> -\n> - pipe_->availableParamBuffers_.push(info.paramBuffer);\n> - pipe_->availableStatBuffers_.push(info.statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> -\n> - frameInfo_.erase(it);\n> -\n> - return 0;\n> -}\n> -\n> -void RkISP1Frames::clear()\n> -{\n> - for (const auto &[frame, info] : frameInfo_) {\n> - pipe_->availableParamBuffers_.push(info.paramBuffer);\n> - pipe_->availableStatBuffers_.push(info.statBuffer);\n> - pipe_->availableMainPathBuffers_.push(info.mainPathBuffer);\n> - }\n> -\n> - frameInfo_.clear();\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(unsigned int frame)\n> -{\n> - auto itInfo = frameInfo_.find(frame);\n> -\n> - if (itInfo != frameInfo_.end())\n> - return &itInfo->second;\n> -\n> - LOG(RkISP1, Fatal) << \"Can't locate info from frame\";\n> -\n> - return nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)\n> -{\n> - for (auto &[frame, info] : frameInfo_) {\n> - if (info.paramBuffer == buffer ||\n> - info.statBuffer == buffer ||\n> - info.mainPathBuffer == buffer ||\n> - info.selfPathBuffer == buffer)\n> - return &info;\n> - }\n> -\n> - LOG(RkISP1, Fatal) << \"Can't locate info from buffer\";\n> -\n> - return nullptr;\n> -}\n> -\n> -RkISP1FrameInfo *RkISP1Frames::find(Request *request)\n> -{\n> - for (auto &[frame, info] : frameInfo_) {\n> - if (info.request == request)\n> - return &info;\n> - }\n> + Camera *activeCamera_;\n> +};\n\nMan, I wish git separated this hunk a bit more nicely. It didn't have to be\nthis hard to read...\n\n> \n> - LOG(RkISP1, Fatal) << \"Can't locate info from request\";\n> \n> - return nullptr;\n> -}\n> \n> PipelineHandlerRkISP1 *RkISP1CameraData::pipe()\n> {\n> @@ -504,44 +407,95 @@ int RkISP1CameraData::loadTuningFile(const std::string &path)\n> void RkISP1CameraData::paramsComputed(unsigned int frame, unsigned int bytesused)\n> {\n> PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> - RkISP1FrameInfo *info = frameInfo_.find(frame);\n> - if (!info)\n> - return;\n> + ParamBufferInfo &pInfo = pipe->computingParamBuffers_.front();\n> + pipe->computingParamBuffers_.pop();\n> +\n> + ASSERT(pInfo.expectedSequence == frame);\n> + FrameBuffer *buffer = pInfo.buffer;\n> \n> - info->paramBuffer->_d()->metadata().planes()[0].bytesused = bytesused;\n> + LOG(RkISP1Schedule, Debug) << \"Queue params for \" << frame << \" \" << buffer;\n> \n> - int ret = pipe->param_->queueBuffer(info->paramBuffer);\n> + buffer->_d()->metadata().planes()[0].bytesused = bytesused;\n> + int ret = pipe->param_->queueBuffer(buffer);\n> if (ret < 0) {\n> LOG(RkISP1, Error) << \"Failed to queue parameter buffer: \"\n> << strerror(-ret);\n> + pipe->availableParamBuffers_.push(buffer);\n> return;\n> }\n> \n> - pipe->stat_->queueBuffer(info->statBuffer);\n> -\n> - if (info->mainPathBuffer)\n> - mainPath_->queueBuffer(info->mainPathBuffer);\n> -\n> - if (selfPath_ && info->selfPathBuffer)\n> - selfPath_->queueBuffer(info->selfPathBuffer);\n> + pipe->queuedParamBuffers_.push({ buffer, frame });\n\nThinking out loud: ok so this is part of the params loop and so we no longer\nqueue to stats nor capture. I assume that's moved to request loop?\n\nOk stats is in queueInternalBuffers and main+self path (no dewarp) is\nqueueRequestDevice and main (yes dewarp) is queueInternalBuffers, got it.\n\n> }\n> \n> void RkISP1CameraData::setSensorControls(unsigned int frame,\n> const ControlList &sensorControls)\n> {\n> + /* We know delayed controls is prewarmed for frame 0 */\n> + if (frame == 0)\n> + return;\n> +\n> + LOG(RkISP1Schedule, Debug) << \"DelayedControls push \" << frame;\n> delayedCtrls_->push(frame, sensorControls);\n> }\n> \n> void RkISP1CameraData::metadataReady(unsigned int frame, const ControlList &metadata)\n> {\n> - RkISP1FrameInfo *info = frameInfo_.find(frame);\n> - if (!info)\n> - return;\n> + PipelineHandlerRkISP1 *pipe = RkISP1CameraData::pipe();\n> +\n> + LOG(RkISP1Schedule, Debug) << \" metadataReady \" << frame;\n> \n> - info->request->metadata().merge(metadata);\n> - info->metadataProcessed = true;\n> + auto &info = pipe->sensorFrameInfos_[frame];\n> \n> - pipe()->tryCompleteRequest(info);\n> + /*\n> + * We don't necessarily know the request for that sequence number,\n> + * as the dequeue of the image buffer might not have happened yet.\n> + * So we check all known requests and store the metadata otherwise.\n> + */\n> + for (auto &reqInfo : pipe->queuedRequests_) {\n> + if (!reqInfo.sequenceValid) {\n> + LOG(RkISP1Schedule, Debug)\n> + << \"Need to store metadata for later \" << frame;\n> + info.metadata = metadata;\n> + break;\n> + }\n> +\n> + if (frame > reqInfo.sequence) {\n\nOk afaiu since the sequence in initialized in queueRequest and is adjusted in\nimageBufferReady, the way that we can lose metadata below is if frames are\ndropped and the sequence number jumps forward.\n\n> + /*\n> + * We will never get stats for that request. Log an\n> + * error and return it.\n> + */\n\nThinking out loud: in this scenario, we have metadata for a relatively-future\nframe and the RequestInfo in the front of the queue is for a relatively-past\nframe. That means the request in the front of the queue will never receive its\nmetadata because it's already passed. Ok looks good to me.\n\n> + LOG(RkISP1, Warning)\n> + << \"Stats for frame \" << reqInfo.sequence\n> + << \" got lost\";\n> + auto &info2 = pipe->sensorFrameInfos_[reqInfo.sequence];\n\nI was wondering what happens if this doesn't exist but I see in statBufferReady\nwe make sure we have all the sequence numbers here so looks good to me.\n\n> + info2.metadataProcessed = true;\n> + ASSERT(info2.statsBuffer == nullptr);\n> + continue;\n> + }\n> +\n> + if (frame == reqInfo.sequence) {\n> + reqInfo.request->metadata().merge(metadata);\n> + break;\n> + }\n> +\n> + /* We should never end up here */\n\nIs this because it is the case that stats never drops buffers while image\nbuffers could be dropped?\n\n> + LOG(RkISP1, Error) << \"Request for sequence \" << frame\n> + << \" is already handled. Metadata was too late\";\n\nI guess there is indeed not much to be done if it does happen.\n\n> +\n> + break;\n> + }\n> +\n> + info.metadataProcessed = true;\n> + /*\n> + * info.statsBuffer can be null, if ipa->processStats() was called\n> + * without a buffer to just fill the metadata.\n> + */\n> + if (info.statsBuffer)\n> + pipe->availableStatBuffers_.push(info.statsBuffer);\n> + info.statsBuffer = nullptr;\n> +\n> + pipe->tryCompleteRequests();\n\nafaiu doesn't this need an if-not-dewarper check? Since if-dewarper then these\nhappen in dewarpBufferReady, and if-not-dewarper then it's in imageBufferReady.\nWait then we have two if-not-dewarper... the one in imageBufferReady must be\nfor raw then...? I'm a bit lost.\n\nI am now unlost now that I've reviewed the rest of the patch and then went back\nto queueInternalBuffers :)\n\n> + pipe->queueInternalBuffers();\n> }\n> \n> /* -----------------------------------------------------------------------------\n> @@ -810,7 +764,7 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate()\n> */\n> \n> PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)\n> - : PipelineHandler(manager, kRkISP1MaxQueuedRequests), hasSelfPath_(true)\n> + : PipelineHandler(manager), hasSelfPath_(true)\n> {\n> }\n> \n> @@ -1189,18 +1143,19 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> } };\n> \n> if (!isRaw_) {\n> - ret = param_->allocateBuffers(kRkISP1MinBufferCount, ¶mBuffers_);\n> + ret = param_->allocateBuffers(kRkISP1InternalBufferCount, ¶mBuffers_);\n> if (ret < 0)\n> return ret;\n> \n> - ret = stat_->allocateBuffers(kRkISP1MinBufferCount, &statBuffers_);\n> + ret = stat_->allocateBuffers(kRkISP1InternalBufferCount, &statBuffers_);\n> if (ret < 0)\n> return ret;\n> }\n> \n> /* If the dewarper is being used, allocate internal buffers for ISP. */\n> if (data->usesDewarper_) {\n> - ret = mainPath_.exportBuffers(kRkISP1MinBufferCount, &mainPathBuffers_);\n> + ret = mainPath_.exportBuffers(kRkISP1DewarpImageBufferCount,\n> + &mainPathBuffers_);\n> if (ret < 0)\n> return ret;\n> \n> @@ -1208,7 +1163,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)\n> availableMainPathBuffers_.push(buffer.get());\n> \n> if (dewarper_->supportsRequests()) {\n> - ret = dewarper_->allocateRequests(kRkISP1MinBufferCount,\n> + ret = dewarper_->allocateRequests(kRkISP1DewarpImageBufferCount + 1,\n> &dewarpRequests_);\n> if (ret < 0)\n> LOG(RkISP1, Error) << \"Failed to allocate requests.\";\n> @@ -1308,6 +1263,10 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> data->delayedCtrls_->reset();\n> \n> data->frame_ = 0;\n> + nextParamsSequence_ = 0;\n> + nextStatsToProcess_ = 0;\n> + paramsSyncHelper_.reset();\n> + imageSyncHelper_.reset();\n> \n> if (!isRaw_) {\n> ret = param_->streamOn();\n> @@ -1364,6 +1323,9 @@ int PipelineHandlerRkISP1::start(Camera *camera, [[maybe_unused]] const ControlL\n> isp_->setFrameStartEnabled(true);\n> \n> activeCamera_ = camera;\n> + running_ = true;\n> +\n> + queueInternalBuffers();\n> \n> actions.release();\n> return 0;\n> @@ -1373,6 +1335,9 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> {\n> RkISP1CameraData *data = cameraData(camera);\n> int ret;\n> + running_ = false;\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Stop device\";\n> \n> isp_->setFrameStartEnabled(false);\n> \n> @@ -1393,40 +1358,153 @@ void PipelineHandlerRkISP1::stopDevice(Camera *camera)\n> LOG(RkISP1, Warning)\n> << \"Failed to stop parameters for \" << camera->id();\n> \n> + /*\n> + * The param buffers are not returned in order, so the queue\n> + * becomes useless.\n> + */\n> + queuedParamBuffers_ = {};\n> +\n> if (data->usesDewarper_)\n> dewarper_->stop();\n> }\n> \n> - ASSERT(data->queuedRequests_.empty());\n> - data->frameInfo_.clear();\n> + tryCompleteRequests();\n> +\n> + /* There can still be requests that are either waiting for metadata\n> + or that contain buffers which were not yet queued at all. */\n\n/*\n * Multiline\n * comment\n */\n\n> + while (!queuedRequests_.empty()) {\n> + RequestInfo &reqInfo = queuedRequests_.front();\n> + cancelRequest(reqInfo.request);\n> + queuedRequests_.pop_front();\n> + }\n> + sensorFrameInfos_.clear();\n> +\n> + ASSERT(queuedDewarpBuffers_.empty());\n> + ASSERT(queuedParamBuffers_.empty());\n> + ASSERT(computingParamBuffers_.empty());\n> \n> freeBuffers(camera);\n> \n> activeCamera_ = nullptr;\n> }\n> \n> -int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> +void PipelineHandlerRkISP1::queueInternalBuffers()\n\nOk this one needs to be reviewed last...\n\nSo afaiu this queuest internal buffers for all the outputs, so it's stats and\nmain path (only if dewarp).\n\nAlthough, why is this called from both dewarpBufferReady and metadataReady? If\ndewarpBufferReady shouldn't we only be requeueing just the dewarp buffers, and\nfor metadataReady just the stats buffers? Or you're consolidating them because\nthe respective queues will be empty anyway so they'll nop?\n\n> {\n> - RkISP1CameraData *data = cameraData(camera);\n> + if (!running_)\n> + return;\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.create(data, request, isRaw_);\n> - if (!info)\n> - return -ENOENT;\n> + RkISP1CameraData *data = cameraData(activeCamera_);\n> +\n> + while (!availableStatBuffers_.empty()) {\n> + FrameBuffer *buf = availableStatBuffers_.front();\n> + availableStatBuffers_.pop();\n> + data->pipe()->stat_->queueBuffer(buf);\n> + }\n> \n> - data->ipa_->queueRequest(data->frame_, request->controls());\n> + /*\n> + * In case of the dewarper, there is a seperate buffer loop for the main\n> + * path\n> + */\n> + while (!availableMainPathBuffers_.empty()) {\n\nIt took me a while to realize that availableMainPathBuffers_ is only used with\ndewarper. That explains the comment (I was wondering why there was no\nif-dewarp when the comment mentioned it).\n\n> + FrameBuffer *buf = availableMainPathBuffers_.front();\n> + availableMainPathBuffers_.pop();\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Queue mainPath \" << buf;\n> + data->mainPath_->queueBuffer(buf);\n\nAnd then it just queues everything, ok.\n\nOk and we need to queue here instead of with in queueRequestDevice because of\nthe separate buffer loop, I see. The asymetry makes me a bit uncomfortable but\nehh.\n\n> + }\n> +}\n> +\n> +void PipelineHandlerRkISP1::computeParamBuffers(uint32_t maxSequence)\n> +{\n> + RkISP1CameraData *data = cameraData(activeCamera_);\n> if (isRaw_) {\n> - if (info->mainPathBuffer)\n> - data->mainPath_->queueBuffer(info->mainPathBuffer);\n> + /*\n> + * Call computeParams with an empty param buffer to trigger\n> + * the setSensorControls signal.\n> + */\n> + data->ipa_->computeParams(maxSequence, 0);\n> + return;\n> + }\n> \n> - if (data->selfPath_ && info->selfPathBuffer)\n> - data->selfPath_->queueBuffer(info->selfPathBuffer);\n> - } else {\n> - data->ipa_->computeParams(data->frame_,\n> - info->paramBuffer->cookie());\n> + while (nextParamsSequence_ <= maxSequence) {\n> + if (availableParamBuffers_.empty()) {\n> + LOG(RkISP1Schedule, Warning)\n> + << \"Ran out of parameter buffers\";\n> + return;\n> + }\n> +\n> + int correction = paramsSyncHelper_.correction();\n> + if (correction != 0)\n> + LOG(RkISP1Schedule, Warning)\n> + << \"Correcting params sequence \"\n> + << correction;\n> +\n> + uint32_t paramsSequence;\n> + if (correction >= 0) {\n> + nextParamsSequence_ += correction;\n> + paramsSyncHelper_.pushCorrection(correction);\n> + paramsSequence = nextParamsSequence_++;\n> + } else {\n> + /*\n> + * Inject the same sequence multiple times, to correct\n> + * for the offset.\n> + */\n> + paramsSyncHelper_.pushCorrection(-1);\n> + paramsSequence = nextParamsSequence_;\n> + }\n> +\n> + FrameBuffer *buf = availableParamBuffers_.front();\n> + availableParamBuffers_.pop();\n> + computingParamBuffers_.push({ buf, paramsSequence });\n> + LOG(RkISP1Schedule, Debug) << \"Request params for \" << paramsSequence;\n> + data->ipa_->computeParams(paramsSequence, buf->cookie());\n> }\n> +}\n\nOk this looks good to me.\n\n> \n> +int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera, Request *request)\n> +{\n> + RkISP1CameraData *data = cameraData(camera);\n> +\n> + RequestInfo info;\n> + info.request = request;\n> +\n> + int correction = imageSyncHelper_.correction();\n> + if (correction != 0)\n> + LOG(RkISP1Schedule, Debug)\n> + << \"Correcting image sequence \"\n> + << data->frame_ << \" to \" << data->frame_ + correction;\n> + data->frame_ += correction;\n> + imageSyncHelper_.pushCorrection(correction);\n> + info.sequence = data->frame_;\n> data->frame_++;\n> \n> + LOG(RkISP1Schedule, Debug) << \"Queue request. Request sequence: \"\n> + << request->sequence()\n> + << \" estimated sensor frame sequence: \" << info.sequence\n> + << \" queue size: \" << (queuedRequests_.size() + 1);\n> +\n> + data->ipa_->queueRequest(info.sequence, request->controls());\n> +\n> + /*\n> + * When the dewarper is used, the request buffers will be queued in\n> + * imageBufferReady()\n\nI don't understand the reason for this difference. Why can't we queue the\nrequest buffers here for dewarper? Is it because we want everything to be\ncompleted before we tie them all together? In which case why can't the\nnon-dewarper case be moved to imageBufferReady too for symmetry?\n\n> + */\n> + if (!data->usesDewarper_) {\n> + FrameBuffer *mainPathBuffer = request->findBuffer(&data->mainPathStream_);\n> + FrameBuffer *selfPathBuffer = request->findBuffer(&data->selfPathStream_);\n> + if (mainPathBuffer)\n> + data->mainPath_->queueBuffer(mainPathBuffer);\n> +\n> + if (data->selfPath_ && selfPathBuffer)\n> + data->selfPath_->queueBuffer(selfPathBuffer);\n> + }\n> +\n> + queuedRequests_.push_back(info);\n> +\n> + /* Kickstart computation of parameters. */\n> + if (info.sequence < kRkISP1InternalBufferCount)\n> + computeParamBuffers(info.sequence);\n> +\n> return 0;\n> }\n> \n> @@ -1626,12 +1704,11 @@ void PipelineHandlerRkISP1::frameStart(uint32_t sequence)\n> return;\n> \n> RkISP1CameraData *data = cameraData(activeCamera_);\n> + LOG(RkISP1Schedule, Debug) << \"frameStart \" << sequence;\n> uint32_t sequenceToApply = sequence + data->delayedCtrls_->maxDelay();\n> data->delayedCtrls_->applyControls(sequenceToApply);\n> \n> - if (isRaw_) {\n> - data->ipa_->computeParams(sequenceToApply + 1, 0);\n> - }\n> + computeParamBuffers(sequenceToApply + 1);\n> }\n\nOk that looks good.\n\n> \n> bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> @@ -1724,29 +1801,51 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)\n> * Buffer Handling\n> */\n> \n> -void PipelineHandlerRkISP1::tryCompleteRequest(RkISP1FrameInfo *info)\n> +void PipelineHandlerRkISP1::tryCompleteRequests()\n> {\n> - RkISP1CameraData *data = cameraData(activeCamera_);\n> - Request *request = info->request;\n> + std::optional lastDeletedSequence;\n> \n> - if (request->hasPendingBuffers())\n> - return;\n> + /* Complete finished requests */\n> + while (!queuedRequests_.empty()) {\n> + RequestInfo info = queuedRequests_.front();\n> \n> - if (!info->metadataProcessed)\n> - return;\n> + if (info.request->hasPendingBuffers())\n> + break;\n> +\n> + if (!info.sequenceValid)\n> + break;\n> +\n> + if (!sensorFrameInfos_[info.sequence].metadataProcessed)\n> + break;\n> +\n> + queuedRequests_.pop_front();\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Complete request \" << info.sequence;\n> + completeRequest(info.request);\n> +\n> + sensorFrameInfos_[info.sequence].request = nullptr;\n> + lastDeletedSequence = info.sequence;\n> + }\n> \n> - if (!isRaw_ && !info->paramDequeued)\n> + if (!lastDeletedSequence.has_value())\n> return;\n> \n> - data->frameInfo_.destroy(info->frame);\n> + /* Drop all outdated sensor frame infos. */\n> + while (!sensorFrameInfos_.empty()) {\n> + auto iter = sensorFrameInfos_.begin();\n> + if (iter->first > lastDeletedSequence.value())\n> + break;\n> +\n> + ASSERT(iter->second.request == nullptr);\n> + ASSERT(iter->second.statsBuffer == nullptr);\n> \n> - completeRequest(request);\n> + sensorFrameInfos_.erase(iter);\n> + }\n> }\n\nOk that looks good to me.\n\n> \n> -void PipelineHandlerRkISP1::cancelDewarpRequest(RkISP1FrameInfo *info)\n> +void PipelineHandlerRkISP1::cancelDewarpRequest(Request *request)\n> {\n> RkISP1CameraData *data = cameraData(activeCamera_);\n> - Request *request = info->request;\n> /*\n> * i.MX8MP is the only known platform with dewarper. It has\n> * no self path. Hence, only main path buffer completion is\n> @@ -1765,51 +1864,109 @@ void PipelineHandlerRkISP1::cancelDewarpRequest(RkISP1FrameInfo *info)\n> }\n> }\n> \n> - tryCompleteRequest(info);\n> + tryCompleteRequests();\n> }\n> \n> void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> {\n> ASSERT(activeCamera_);\n> RkISP1CameraData *data = cameraData(activeCamera_);\n> + const FrameMetadata &metadata = buffer->metadata();\n> + RequestInfo *reqInfo = nullptr;\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> - if (!info)\n> - return;\n> + /*\n> + * When the dewarper is used, the buffer is not yet tied to a request,\n> + * so find the first request without a valid sequence. Otherwise find\n> + * the request for that buffer. This is not necessarily the same,\n> + * because after streamoff the buffers are returned in arbitrary order.\n> + */\n> + for (auto &info : queuedRequests_) {\n> + if (data->usesDewarper_) {\n> + if (!info.sequenceValid) {\n> + reqInfo = &info;\n> + break;\n> + }\n> + } else {\n> + if (info.request == buffer->request()) {\n> + reqInfo = &info;\n\nNo break? :) I guess it's not strictly required.\n\n> + }\n> + }\n> + }\n> \n> - const FrameMetadata &metadata = buffer->metadata();\n> - Request *request = info->request;\n> + if (!reqInfo) {\n> + if (data->usesDewarper_) {\n> + LOG(RkISP1Schedule, Info)\n> + << \"Image buffer ready, but no corresponding request\";\n> + availableMainPathBuffers_.push(buffer);\n> + return;\n> + }\n> +\n> + LOG(RkISP1Schedule, Fatal)\n> + << \"Image buffer ready, but no corresponding request\";\n\nI wonder if it's worth it to drop buffers and keep the capture loop going. It\nsounds like it would be complex. Although we could also just say that any\napplication that doesn't keep the camera requests fed is just asking for\ntrouble. Or maybe we should be nice and help out an application that runs into\nsometing unexpected and crash gracefully instead of fatally. Not sure which is\nbetter.\n\nI'm guessing in the dewarper case there's still time for a request to be queued\nwhile the dewarper does processing? I wonder if that should be a Warning then.\n\n> + }\n> +\n> + Request *request = reqInfo->request;\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Image buffer ready: \" << buffer\n> + << \" Expected sequence: \" << reqInfo->sequence\n> + << \" got: \" << metadata.sequence;\n> +\n> + uint32_t sequence = metadata.sequence;\n> +\n> + /*\n> + * If the frame was cancelled, the metadata sequnce is usually wrong and\n> + * we assume that our guess was right.\n> + */\n> + if (metadata.status == FrameMetadata::FrameCancelled)\n> + sequence = reqInfo->sequence;\n> +\n> + /* We now know the buffer sequence that belongs to this request */\n> + int droppedFrames = imageSyncHelper_.gotFrame(reqInfo->sequence, sequence);\n> + if (droppedFrames != 0)\n> + LOG(RkISP1Schedule, Warning)\n> + << \"Frame \" << reqInfo->sequence << \": Dropped \"\n> + << droppedFrames << \" frames\";\n> +\n> + reqInfo->sequence = sequence;\n> + reqInfo->sequenceValid = true;\n> +\n> + if (sensorFrameInfos_[sequence].metadataProcessed) {\n> + LOG(RkISP1Schedule, Debug)\n> + << \"Apply stored metadata \" << reqInfo->sequence;\n> + request->metadata().merge(sensorFrameInfos_[sequence].metadata);\n> + }\n> \n> if (metadata.status != FrameMetadata::FrameCancelled) {\n> /*\n> * Record the sensor's timestamp in the request metadata.\n> *\n> - * \\todo The sensor timestamp should be better estimated by connecting\n> - * to the V4L2Device::frameStart signal.\n> + * \\todo The sensor timestamp should be better estimated by\n> + * connecting to the V4L2Device::frameStart signal.\n> */\n> request->metadata().set(controls::SensorTimestamp,\n> metadata.timestamp);\n> \n> + /* In raw mode call processStats() to fill the metadata */\n> if (isRaw_) {\n> const ControlList &ctrls =\n> data->delayedCtrls_->get(metadata.sequence);\n> - data->ipa_->processStats(info->frame, 0, ctrls);\n> + data->ipa_->processStats(reqInfo->sequence, 0, ctrls);\n> }\n> } else {\n> - if (isRaw_)\n> - info->metadataProcessed = true;\n> + /* No need to block waiting for metedata on that frame. */\n\ns/metedata/metadata/\n\n> + sensorFrameInfos_[sequence].metadataProcessed = true;\n> }\n> \n> if (!data->usesDewarper_) {\n> - completeBuffer(request, buffer);\n> - tryCompleteRequest(info);\n> + completeBuffer(reqInfo->request, buffer);\n> + tryCompleteRequests();\n> \n> return;\n> }\n> \n> - /* Do not queue cancelled frames to dewarper. */\n> + /* Do not queue cancelled frames to the dewarper. */\n> if (metadata.status == FrameMetadata::FrameCancelled) {\n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> return;\n> }\n> \n> @@ -1869,10 +2026,14 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> dewarper_->applyVertexMap(&data->mainPathStream_, dewarpRequest);\n> \n> /*\n> - * Queue input and output buffers to the dewarper. The output\n> - * buffers for the dewarper are the buffers of the request, supplied\n> - * by the application.\n> + * Queue input and output buffers to the dewarper. The output buffers\n> + * for the dewarper are the buffers of the request, supplied by the\n> + * application.\n> */\n> + DewarpBufferInfo dewarpInfo{ buffer, reqInfo->request->findBuffer(&data->mainPathStream_) };\n> + queuedDewarpBuffers_.push_back(dewarpInfo);\n> + LOG(RkISP1Schedule, Debug) << \"Queue dewarper \" << dewarpInfo.inputBuffer\n> + << \" \" << dewarpInfo.outputBuffer;\n> int ret = dewarper_->queueBuffers(buffer, request->buffers(), dewarpRequest);\n> if (ret < 0) {\n> LOG(RkISP1, Error) << \"Failed to queue buffers to dewarper: -\"\n> @@ -1882,7 +2043,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> if (dewarpRequest)\n> dewarpRequestReady(dewarpRequest);\n> \n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> \n> return;\n> }\n> @@ -1895,7 +2056,7 @@ void PipelineHandlerRkISP1::imageBufferReady(FrameBuffer *buffer)\n> /* Push it back into the queue. */\n> dewarpRequestReady(dewarpRequest);\n> \n> - cancelDewarpRequest(info);\n> + cancelDewarpRequest(reqInfo->request);\n> }\n> }\n> \n\nOk this part looks good to me.\n\n> @@ -1919,29 +2080,59 @@ void PipelineHandlerRkISP1::dewarpRequestReady(V4L2Request *request)\n> \n> void PipelineHandlerRkISP1::dewarpBufferReady(FrameBuffer *buffer)\n> {\n> - ASSERT(activeCamera_);\n> - RkISP1CameraData *data = cameraData(activeCamera_);\n> Request *request = buffer->request();\n> + const FrameMetadata &metadata = buffer->metadata();\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer->request());\n> - if (!info)\n> - return;\n> + /*\n> + * After stopping the dewarper, the buffers are returned out of order.\n> + * Search the list for the corresponding info and handle it. In regular\n> + * operation it will always be the first entry.\n> + */\n> + for (DewarpBufferInfo &dwInfo : queuedDewarpBuffers_) {\n> + if (dwInfo.outputBuffer != buffer)\n> + continue;\n> \n> - completeBuffer(request, buffer);\n> - tryCompleteRequest(info);\n> + availableMainPathBuffers_.push(dwInfo.inputBuffer);\n> + dwInfo.inputBuffer = nullptr;\n> + dwInfo.outputBuffer = nullptr;\n> +\n> + if (metadata.status == FrameMetadata::FrameCancelled)\n> + buffer->_d()->cancel();\n> +\n> + completeBuffer(request, buffer);\n> + }\n> +\n> + while (!queuedDewarpBuffers_.empty() &&\n> + queuedDewarpBuffers_.front().inputBuffer == nullptr)\n> + queuedDewarpBuffers_.pop_front();\n> +\n> + tryCompleteRequests();\n> + queueInternalBuffers();\n> }\n> \n> void PipelineHandlerRkISP1::paramBufferReady(FrameBuffer *buffer)\n> {\n> - ASSERT(activeCamera_);\n> - RkISP1CameraData *data = cameraData(activeCamera_);\n> + LOG(RkISP1Schedule, Debug) << \"Param buffer ready \" << buffer;\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> - if (!info)\n> + /* After stream off, the buffers are returned out of order, so\n> + * we don't care about the rest.\n> + */\n\n/*\n * Multiline\n * comment\n */\n\n> + if (!running_) {\n> + availableParamBuffers_.push(buffer);\n> return;\n> + }\n> \n> - info->paramDequeued = true;\n> - tryCompleteRequest(info);\n> + ParamBufferInfo pInfo = queuedParamBuffers_.front();\n> + queuedParamBuffers_.pop();\n> +\n> + ASSERT(pInfo.buffer == buffer);\n> +\n> + size_t metaSequence = buffer->metadata().sequence;\n> + LOG(RkISP1Schedule, Debug) << \"Params buffer ready \"\n> + << \" Expected: \" << pInfo.expectedSequence\n> + << \" got: \" << metaSequence;\n> + paramsSyncHelper_.gotFrame(pInfo.expectedSequence, metaSequence);\n> + availableParamBuffers_.push(buffer);\n\nOk looks good.\n\n> }\n> \n> void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> @@ -1949,21 +2140,47 @@ void PipelineHandlerRkISP1::statBufferReady(FrameBuffer *buffer)\n> ASSERT(activeCamera_);\n> RkISP1CameraData *data = cameraData(activeCamera_);\n> \n> - RkISP1FrameInfo *info = data->frameInfo_.find(buffer);\n> - if (!info)\n> - return;\n> + size_t sequence = buffer->metadata().sequence;\n> \n> if (buffer->metadata().status == FrameMetadata::FrameCancelled) {\n> - info->metadataProcessed = true;\n> - tryCompleteRequest(info);\n> + LOG(RkISP1Schedule, Warning) << \"Stats cancelled \" << sequence;\n> + /*\n> + * We can't assume that the sequence of the stat buffer is valid,\n> + * so there is nothing left to do.\n> + */\n> + availableStatBuffers_.push(buffer);\n> + return;\n> + }\n> +\n> + LOG(RkISP1Schedule, Debug) << \"Stats ready \" << sequence;\n> +\n> + if (nextStatsToProcess_ != sequence)\n> + LOG(RkISP1Schedule, Warning) << \"Stats sequence out of sync.\"\n> + << \" Expected: \" << nextStatsToProcess_\n> + << \" got: \" << sequence;\n> +\n> + if (nextStatsToProcess_ > sequence) {\n> + LOG(RkISP1Schedule, Warning) << \"Stats were too late. Ignored\";\n> + availableStatBuffers_.push(buffer);\n> return;\n> }\n> \n> - if (data->frame_ <= buffer->metadata().sequence)\n> - data->frame_ = buffer->metadata().sequence + 1;\n> + /* Send empty stats to ensure metadata gets created*/\n\ns/d\\*\\//d \\*\\//\n\n> + while (nextStatsToProcess_ < sequence) {\n> + LOG(RkISP1Schedule, Warning) << \"Send empty stats to fill metadata\";\n> + data->ipa_->processStats(nextStatsToProcess_, 0,\n> + data->delayedCtrls_->get(nextStatsToProcess_));\n> +\n> + nextStatsToProcess_++;\n> + }\n> +\n> + nextStatsToProcess_++;\n> +\n> + sensorFrameInfos_[sequence].statsBuffer = buffer;\n> \n> - data->ipa_->processStats(info->frame, info->statBuffer->cookie(),\n> - data->delayedCtrls_->get(buffer->metadata().sequence));\n> + LOG(RkISP1Schedule, Debug) << \"Process stats \" << sequence;\n> + data->ipa_->processStats(sequence, buffer->cookie(),\n> + data->delayedCtrls_->get(sequence));\n\nOk I see so this whole function ensures that we have proper sequence numbers on\nthe stats buffers, and we use that to sync the request and image buffer sequence numbers too.\n\nThis looks good to me.\n\nAlthough I do have a question (probably I missed it just because of how big\nthis patch is): what happens when we're in raw mode? I guess it doesn't really\nmatter since there's no other buffers to synchronize so we can just use the\nRequest?\n\n\nThanks,\n\nPaul\n\n> }\n> \n> REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1, \"rkisp1\")\n> -- \n> 2.48.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 734D7BDCBF\n\tfor ;\n\tTue, 6 Jan 2026 08:13:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 797F061F9F;\n\tTue, 6 Jan 2026 09:13:57 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 02569615B2\n\tfor ;\n\tTue, 6 Jan 2026 09:13:54 +0100 (CET)","from neptunite.rasen.tech (unknown\n\t[IPv6:2404:7a81:160:2100:2700:4fa5:c459:a9a7])\n\tby perceval.ideasonboard.com (Postfix) with UTF8SMTPSA id 4B91D591;\n\tTue, 6 Jan 2026 09:13:33 +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=\"JcEqgBY1\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1767687214;\n\tbh=wk0wyG4Hc6p+rtUVDXaikYKCyQroRkMbPpAZOM97+4k=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=JcEqgBY1lLaWyRKcOyVa22gOBss3e9sGmX9DvZ7fYZLNg4AMqhSBvho+4iKWhXRHK\n\tBcEbbOKCEdbSOOm5O1CIdm0Ih+9Xac0oha8AIC3Uu+renvTKd4TJYhdXdDAry6ae0G\n\tOW/PmXTLzlesGjThgGDiuFU+hweTXvLYUmPH+Nrs=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20251024085130.995967-28-stefan.klug@ideasonboard.com>","References":"<20251024085130.995967-1-stefan.klug@ideasonboard.com>\n\t<20251024085130.995967-28-stefan.klug@ideasonboard.com>","Subject":"Re: [PATCH v1 27/35] pipeline: rkisp1: Decouple image,\n\tstats and param buffers","From":"Paul Elder ","Cc":"Stefan Klug ","To":"Stefan Klug ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Tue, 06 Jan 2026 17:13:45 +0900","Message-ID":"<176768722599.2470890.13611601803903542065@neptunite.rasen.tech>","User-Agent":"alot/0.0.0","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\" "}}]