[{"id":34434,"web_url":"https://patchwork.libcamera.org/comment/34434/","msgid":"","date":"2025-06-06T11:45:35","subject":"Re: [PATCH v3 3/6] pipeline: ipa: rpi: Split\n\tRPiCameraData::dropFrameCount_","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Barnabás,\n\nOn Fri, 6 Jun 2025 at 12:40, Barnabás Pőcze \nwrote:\n\n> Hi\n>\n>\n> 2025. 06. 06. 12:55 keltezéssel, Naushir Patuck írta:\n> > Split the pipeline handler drop frame tracking into startup frames and\n> > invalid frames, as reported by the IPA.\n> >\n> > Remove the drop buffer handling logic in the pipeline handler. Now all\n> > image buffers are returned out with the appropriate FrameStatus set\n> > for startup or invalid frames.\n> >\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Jacopo Mondi \n> > ---\n> > .../pipeline/rpi/common/pipeline_base.cpp | 98 ++++++++-----------\n> > .../pipeline/rpi/common/pipeline_base.h | 5 +-\n> > 2 files changed, 42 insertions(+), 61 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > index 5956485953a2..3f0b7abdc59a 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp\n> > @@ -659,9 +659,9 @@ int PipelineHandlerBase::start(Camera *camera, const\n> ControlList *controls)\n> > if (!result.controls.empty())\n> > data->setSensorControls(result.controls);\n> >\n> > - /* Configure the number of dropped frames required on startup. */\n> > - data->dropFrameCount_ = data->config_.disableStartupFrameDrops ?\n> > - 0 : result.startupFrameCount +\n> result.invalidFrameCount;\n> > + /* Configure the number of startup and invalid frames reported by\n> the IPA. */\n> > + data->startupFrameCount_ = result.startupFrameCount;\n> > + data->invalidFrameCount_ = result.invalidFrameCount;\n> >\n> > for (auto const stream : data->streams_)\n> > stream->resetBuffers();\n> > @@ -678,7 +678,6 @@ int PipelineHandlerBase::start(Camera *camera, const\n> ControlList *controls)\n> > data->buffersAllocated_ = true;\n> > }\n> >\n> > - /* We need to set the dropFrameCount_ before queueing buffers. */\n> > ret = queueAllBuffers(camera);\n> > if (ret) {\n> > LOG(RPI, Error) << \"Failed to queue buffers\";\n> > @@ -894,28 +893,12 @@ int PipelineHandlerBase::queueAllBuffers(Camera\n> *camera)\n> > int ret;\n> >\n> > for (auto const stream : data->streams_) {\n> > - if (!(stream->getFlags() & StreamFlag::External)) {\n> > - ret = stream->queueAllBuffers();\n> > - if (ret < 0)\n> > - return ret;\n> > - } else {\n> > - /*\n> > - * For external streams, we must queue up a set of\n> internal\n> > - * buffers to handle the number of drop frames\n> requested by\n> > - * the IPA. This is done by passing nullptr in\n> queueBuffer().\n> > - *\n> > - * The below queueBuffer() call will do nothing if\n> there\n> > - * are not enough internal buffers allocated, but\n> this will\n> > - * be handled by queuing the request for buffers\n> in the\n> > - * RPiStream object.\n> > - */\n> > - unsigned int i;\n> > - for (i = 0; i < data->dropFrameCount_; i++) {\n> > - ret = stream->queueBuffer(nullptr);\n> > - if (ret)\n> > - return ret;\n> > - }\n> > - }\n> > + if (stream->getFlags() & StreamFlag::External)\n> > + continue;\n> > +\n> > + ret = stream->queueAllBuffers();\n> > + if (ret < 0)\n> > + return ret;\n> > }\n> >\n> > return 0;\n> > @@ -1412,7 +1395,15 @@ void CameraData::handleStreamBuffer(FrameBuffer\n> *buffer, RPi::Stream *stream)\n> > * buffer back to the stream.\n> > */\n> > Request *request = requestQueue_.empty() ? nullptr :\n> requestQueue_.front();\n> > - if (!dropFrameCount_ && request && request->findBuffer(stream) ==\n> buffer) {\n> > + if (request && request->findBuffer(stream) == buffer) {\n> > + FrameMetadata &md = buffer->_d()->metadata();\n> > +\n> > + /* Mark the non-converged and invalid frames in the\n> metadata. */\n> > + if (invalidFrameCount_)\n> > + md.status = FrameMetadata::Status::FrameError;\n> > + else if (startupFrameCount_)\n> > + md.status = FrameMetadata::Status::FrameStartup;\n> > +\n> > /*\n> > * Tag the buffer as completed, returning it to the\n> > * application.\n> > @@ -1458,42 +1449,31 @@ void CameraData::handleState()\n> >\n> > void CameraData::checkRequestCompleted()\n> > {\n> > - bool requestCompleted = false;\n> > - /*\n> > - * If we are dropping this frame, do not touch the request, simply\n> > - * change the state to IDLE when ready.\n> > - */\n> > - if (!dropFrameCount_) {\n>\n> There are two `request->metadata().clear()` calls in\n> `{PiSP,Vc4}CameraData::tryRunPipeline()`:\n>\n> /*\n> * Clear the request metadata and fill it with some initial non-IPA\n> * related controls. We clear it first because the request metadata\n> * may have been populated if we have dropped the previous frame.\n> */\n> request->metadata().clear();\n>\n>\n> Are those still needed?\n>\n\nI believe this can be removed now. I can post a patch on top once this is\nmerged.\n.\nRegards,\nNaush\n\n\n\n>\n>\n> Regards,\n> Barnabás Pőcze\n>\n> > - Request *request = requestQueue_.front();\n> > - if (request->hasPendingBuffers())\n> > - return;\n> > + Request *request = requestQueue_.front();\n> > + if (request->hasPendingBuffers())\n> > + return;\n> >\n> > - /* Must wait for metadata to be filled in before\n> completing. */\n> > - if (state_ != State::IpaComplete)\n> > - return;\n> > + /* Must wait for metadata to be filled in before completing. */\n> > + if (state_ != State::IpaComplete)\n> > + return;\n> >\n> > - LOG(RPI, Debug) << \"Completing request sequence: \"\n> > - << request->sequence();\n> > + LOG(RPI, Debug) << \"Completing request sequence: \"\n> > + << request->sequence();\n> >\n> > - pipe()->completeRequest(request);\n> > - requestQueue_.pop();\n> > - requestCompleted = true;\n> > - }\n> > + pipe()->completeRequest(request);\n> > + requestQueue_.pop();\n> >\n> > - /*\n> > - * Make sure we have three outputs completed in the case of a\n> dropped\n> > - * frame.\n> > - */\n> > - if (state_ == State::IpaComplete &&\n> > - ((ispOutputCount_ == ispOutputTotal_ && dropFrameCount_) ||\n> > - requestCompleted)) {\n> > - LOG(RPI, Debug) << \"Going into Idle state\";\n> > - state_ = State::Idle;\n> > - if (dropFrameCount_) {\n> > - dropFrameCount_--;\n> > - LOG(RPI, Debug) << \"Dropping frame at the request\n> of the IPA (\"\n> > - << dropFrameCount_ << \" left)\";\n> > - }\n> > + LOG(RPI, Debug) << \"Going into Idle state\";\n> > + state_ = State::Idle;\n> > +\n> > + if (invalidFrameCount_) {\n> > + invalidFrameCount_--;\n> > + LOG(RPI, Debug) << \"Decrementing invalid frames to \"\n> > + << invalidFrameCount_;\n> > + } else if (startupFrameCount_) {\n> > + startupFrameCount_--;\n> > + LOG(RPI, Debug) << \"Decrementing startup frames to \"\n> > + << startupFrameCount_;\n> > }\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > index aae0c2f35888..6023f9f9d6b3 100644\n> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h\n> > @@ -48,7 +48,7 @@ class CameraData : public Camera::Private\n> > public:\n> > CameraData(PipelineHandler *pipe)\n> > : Camera::Private(pipe), state_(State::Stopped),\n> > - dropFrameCount_(0), buffersAllocated_(false),\n> > + startupFrameCount_(0), invalidFrameCount_(0),\n> buffersAllocated_(false),\n> > ispOutputCount_(0), ispOutputTotal_(0)\n> > {\n> > }\n> > @@ -151,7 +151,8 @@ public:\n> > /* Mapping of CropParams keyed by the output stream order in\n> CameraConfiguration */\n> > std::map cropParams_;\n> >\n> > - unsigned int dropFrameCount_;\n> > + unsigned int startupFrameCount_;\n> > + unsigned int invalidFrameCount_;\n> >\n> > /*\n> > * If set, this stores the value that represets a gain of one for\n>\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 9A748C3237\n\tfor ;\n\tFri, 6 Jun 2025 11:46:17 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0026C68DB3;\n\tFri, 6 Jun 2025 13:46:15 +0200 (CEST)","from mail-vk1-xa30.google.com (mail-vk1-xa30.google.com\n\t[IPv6:2607:f8b0:4864:20::a30])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 44CC368DB0\n\tfor ;\n\tFri, 6 Jun 2025 13:46:13 +0200 (CEST)","by mail-vk1-xa30.google.com with SMTP id\n\t71dfb90a1353d-52f28e83e13so134981e0c.2\n\tfor ;\n\tFri, 06 Jun 2025 04:46:13 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"ZaP81IM/\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1749210372; x=1749815172;\n\tdarn=lists.libcamera.org; \n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=c3aeZ6pUdRJjbF34+UucSsfHpifwY5QuuFPu6M9nAPY=;\n\tb=ZaP81IM/h4uaxrqt4HdsHGiCYVSeRgQfqyjhtDp6gUKKZ6bD7hAJO5dlquj3dtAYaV\n\tXgM4vlfeZPzmhF9yiCoYEN6pTOHJfI6+lZ/sGbYTC0TX4UYDRH7fkSJ29PHdTE8i4imv\n\twMOY7V1qBXdaQXmJmGTjAo49ygil+lsCmKRXKKcaevK4LOu2w+lwsBfiwrOKeMU7Ds2W\n\tZDozknTzVlznxuTTxZNaDgNVJoxilHv8uBenvI2b2cFHOvLF0y02jcSOJA32JF21HAjV\n\tg0WlSyhHEpeXwVFmc6Qq4aj6E067VEKTvIXXtyq663tFDQbzl+M55hbbRDGK96he808m\n\tZmSg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1749210372; x=1749815172;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=c3aeZ6pUdRJjbF34+UucSsfHpifwY5QuuFPu6M9nAPY=;\n\tb=WWHjwzVLLqIIEy4Nrk7LqoK0vqwzBE+0E1i1W+HrSdU7KFaKDzkU9k0W90XgZ2BJlQ\n\tzhoXvra2esGlDrP/go/5F6sSnNy9iQ6cPMqOECHDY9VGmv/ref3JllP8BVAdxLObFnx8\n\t3ZAnICPkzxsu+NWHWSB0nqt837objpcRV1vsM0E6xl5swZdSAD99O3uc4bJz2Kx48CD7\n\t6wlAuwVRl0b6thbpd4TMkw1cQjSzAK0Mu2R+hogTrpSCxNuwUvO/p7RdarYfoVckXFuT\n\tpGQeuruFebpLDjMtPfngY0bcXD2ardJjxbb9rP/BrpR1gyh3sD3EAbET4QwlKujRwnfX\n\tu9Wg==","X-Gm-Message-State":"AOJu0Yyi+s3abvJrqfOVXMzfbmsNIczkukfCe1vHNthxqjajfgn+Hr5D\n\tF5mGMVwyRAxVSsPcPAymOz6TX1O2WZkGc5ygdRuLi1ZQSdKv2oFlFpy0YjFOTuQA3j4HVT7Vbmx\n\tTl06RDrGWbJbR8rdeO1SuiSLydD/JEsg+7f7WyJoGSg==","X-Gm-Gg":"ASbGncuY5mI21DD9w4yX+GPRQQ2nyo9neal0kLPmY1B2vsu3nTuf+IemMjVeiNQNOaz\n\tYDxHvVEPQC8pwZF/YfLI2s6da8x26t8aacHFMzxdJMOs+/VSHTA16AYylbUntEELL4bEg6F/9ye\n\txJHekqkDOVWBaaedOGhV4MJLffhd4/PmrdD7qzXUes36u0R5rl16jh+ChBRt2f3Mc=","X-Google-Smtp-Source":"AGHT+IEAY0inANDerNEubE5ZO4eFroY5U4Sh6T+MXHSVkGxV+7I/RLb9XftC8rducTcAc9W+3n8ap3zIPBK90wkhb6E=","X-Received":"by 2002:ac5:c91b:0:b0:530:5f59:b5ad with SMTP id\n\t71dfb90a1353d-530e48b128cmr668910e0c.3.1749210371831; Fri, 06 Jun 2025\n\t04:46:11 -0700 (PDT)","MIME-Version":"1.0","References":"<20250606105651.1624640-1-naush@raspberrypi.com>\n\t<20250606105651.1624640-4-naush@raspberrypi.com>\n\t<93d0c802-3db1-4371-ae93-ac2f0cde571f@ideasonboard.com>","In-Reply-To":"<93d0c802-3db1-4371-ae93-ac2f0cde571f@ideasonboard.com>","From":"Naushir Patuck ","Date":"Fri, 6 Jun 2025 12:45:35 +0100","X-Gm-Features":"AX0GCFtsEsIgyZPepNN886NsyTwOIwpXhOBPOBbrNxX-H-c4LWdlzPtWpnvzpew","Message-ID":"","Subject":"Re: [PATCH v3 3/6] pipeline: ipa: rpi: Split\n\tRPiCameraData::dropFrameCount_","To":"=?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= ","Cc":"libcamera-devel@lists.libcamera.org, \n\tDavid Plowman , \n\tJacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000d9cfad0636e5c6f0\"","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\" "}}]