[{"id":33736,"web_url":"https://patchwork.libcamera.org/comment/33736/","msgid":"<174302122766.1701483.16825770630533265321@ping.linuxembedded.co.uk>","date":"2025-03-26T20:33:47","subject":"Re: [PATCH v5 1/6] libcamera: software_isp: Track frames and\n\trequests","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Milan Zamazal (2025-03-26 12:48:50)\n> Hardware pipelines track requests and other information related to\n> particular frames. This hasn't been needed in software ISP so far. But\n> in order to be able to track metadata corresponding to a given frame,\n> frame-request tracking mechanism starts being useful. This patch\n> introduces the basic tracking structure, actual metadata handling is\n> added in the following patch.\n> \n> Signed-off-by: Milan Zamazal \n\nIn v3, Barnabas clarified that the fix I was worried about was\nincorporated here already so:\n\nReviewed-by: Kieran Bingham \nTested-by: Kieran Bingham # Lenovo X13s\n\nConcerns about duplicating this framework concept in each pipeline\nhandler can be aligned on top.\n\n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 85 ++++++++++++++++++++++--\n> 1 file changed, 80 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 6e039bf3..2281c0a6 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> \n> class SimplePipelineHandler;\n> \n> +struct SimpleFrameInfo {\n> + uint32_t frame;\n> + Request *request;\n> +};\n> +\n> +class SimpleFrames\n> +{\n> +public:\n> + void create(Request *request);\n> + void destroy(uint32_t frame);\n> + void clear();\n> +\n> + SimpleFrameInfo *find(uint32_t frame);\n> + SimpleFrameInfo *find(Request *request);\n> +\n> +private:\n> + std::map frameInfo_;\n> +};\n> +\n> +void SimpleFrames::create(Request *request)\n> +{\n> + uint32_t frame = request->sequence();\n> + auto [it, inserted] = frameInfo_.try_emplace(request->sequence());\n> + ASSERT(inserted);\n> + it->second.frame = frame;\n> + it->second.request = request;\n> +}\n> +\n> +void SimpleFrames::destroy(uint32_t frame)\n> +{\n> + frameInfo_.erase(frame);\n> +}\n> +\n> +void SimpleFrames::clear()\n> +{\n> + frameInfo_.clear();\n> +}\n> +\n> +SimpleFrameInfo *SimpleFrames::find(uint32_t frame)\n> +{\n> + auto info = frameInfo_.find(frame);\n> + if (info == frameInfo_.end())\n> + return nullptr;\n> + return &info->second;\n> +}\n> +\n> +SimpleFrameInfo *SimpleFrames::find(Request *request)\n> +{\n> + for (auto &itInfo : frameInfo_) {\n> + SimpleFrameInfo *info = &itInfo.second;\n> +\n> + if (info->request == request)\n> + return info;\n> + }\n> +\n> + return nullptr;\n> +}\n> +\n> struct SimplePipelineInfo {\n> const char *driver;\n> /*\n> @@ -293,11 +351,13 @@ public:\n> \n> std::unique_ptr converter_;\n> std::unique_ptr swIsp_;\n> + SimpleFrames frameInfo_;\n> \n> private:\n> void tryPipeline(unsigned int code, const Size &size);\n> static std::vector routedSourcePads(MediaPad *sink);\n> \n> + void completeRequest(Request *request);\n> void conversionInputDone(FrameBuffer *buffer);\n> void conversionOutputDone(FrameBuffer *buffer);\n> \n> @@ -785,7 +845,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> /* No conversion, just complete the request. */\n> Request *request = buffer->request();\n> pipe->completeBuffer(request, buffer);\n> - pipe->completeRequest(request);\n> + completeRequest(request);\n> return;\n> }\n> \n> @@ -803,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> const RequestOutputs &outputs = conversionQueue_.front();\n> for (auto &[stream, buf] : outputs.outputs)\n> pipe->completeBuffer(outputs.request, buf);\n> - pipe->completeRequest(outputs.request);\n> + completeRequest(outputs.request);\n> conversionQueue_.pop();\n> \n> return;\n> @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \n> /* Otherwise simply complete the request. */\n> pipe->completeBuffer(request, buffer);\n> - pipe->completeRequest(request);\n> + completeRequest(request);\n> }\n> \n> void SimpleCameraData::clearIncompleteRequests()\n> @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests()\n> }\n> }\n> \n> +void SimpleCameraData::completeRequest(Request *request)\n> +{\n> + SimpleFrameInfo *info = frameInfo_.find(request);\n> + if (!info) {\n> + /* Something is really wrong, let's return. */\n> + return;\n> + }\n> +\n> + frameInfo_.destroy(info->frame);\n> + pipe()->completeRequest(request);\n> +}\n> +\n> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n> {\n> /* Queue the input buffer back for capture. */\n> @@ -885,7 +957,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> /* Complete the buffer and the request. */\n> Request *request = buffer->request();\n> if (pipe->completeBuffer(request, buffer))\n> - pipe->completeRequest(request);\n> + completeRequest(request);\n> }\n> \n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> \n> video->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);\n> \n> + data->frameInfo_.clear();\n> data->clearIncompleteRequests();\n> data->conversionBuffers_.clear();\n> \n> @@ -1428,8 +1501,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> \n> if (data->useConversion_) {\n> data->conversionQueue_.push({ request, std::move(buffers) });\n> - if (data->swIsp_)\n> + if (data->swIsp_) {\n> + data->frameInfo_.create(request);\n> data->swIsp_->queueRequest(request->sequence(), request->controls());\n> + }\n> }\n> \n> return 0;\n> -- \n> 2.49.0\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 CAD0CC323E\n\tfor ;\n\tWed, 26 Mar 2025 20:33:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7EB506896B;\n\tWed, 26 Mar 2025 21:33:53 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4354E68950\n\tfor ;\n\tWed, 26 Mar 2025 21:33:51 +0100 (CET)","from pendragon.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 022D2A57;\n\tWed, 26 Mar 2025 21:32:02 +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=\"OP6c5LVM\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743021123;\n\tbh=aD32Kh4CWI+TCyfz4RTJWIM6JFSi1Y0TI5KmDPdDIJ4=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OP6c5LVM8uh9T/E92xsZ3OfH17Ixk0k8e0dPQ22fp0ehTwcZ5sfydJXrtn8B5twYf\n\t6W2AUtYVSP7dJUu33myUEB0cMW88aRQmrXVNxPtWH3mlE/Qtax40zexam8BHeMIGre\n\tujR7MZ6ecolFwiQeiJ7CzPtFQFDyEEQakiHL7TWg=","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20250326124856.75709-2-mzamazal@redhat.com>","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-2-mzamazal@redhat.com>","Subject":"Re: [PATCH v5 1/6] libcamera: software_isp: Track frames and\n\trequests","From":"Kieran Bingham ","Cc":"Milan Zamazal ,\n\tLaurent Pinchart ","To":"Milan Zamazal , libcamera-devel@lists.libcamera.org","Date":"Wed, 26 Mar 2025 20:33:47 +0000","Message-ID":"<174302122766.1701483.16825770630533265321@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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":33740,"web_url":"https://patchwork.libcamera.org/comment/33740/","msgid":"<20250326223342.GB21053@pendragon.ideasonboard.com>","date":"2025-03-26T22:33:42","subject":"Re: [PATCH v5 1/6] libcamera: software_isp: Track frames and\n\trequests","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Wed, Mar 26, 2025 at 01:48:50PM +0100, Milan Zamazal wrote:\n> Hardware pipelines track requests and other information related to\n> particular frames. This hasn't been needed in software ISP so far. But\n> in order to be able to track metadata corresponding to a given frame,\n> frame-request tracking mechanism starts being useful. This patch\n> introduces the basic tracking structure, actual metadata handling is\n> added in the following patch.\n> \n> Signed-off-by: Milan Zamazal \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 85 ++++++++++++++++++++++--\n> 1 file changed, 80 insertions(+), 5 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 6e039bf3..2281c0a6 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -181,6 +181,64 @@ LOG_DEFINE_CATEGORY(SimplePipeline)\n> \n> class SimplePipelineHandler;\n> \n> +struct SimpleFrameInfo {\n\nIf you add a constructor here:\n\n\tSimpleFrameInfo(uint32_t f, Request *r)\n\t\t: frame(f), request(r)\n\t{\n\t}\n\n...\n\n> +\tuint32_t frame;\n> +\tRequest *request;\n> +};\n> +\n> +class SimpleFrames\n> +{\n> +public:\n> +\tvoid create(Request *request);\n> +\tvoid destroy(uint32_t frame);\n> +\tvoid clear();\n> +\n> +\tSimpleFrameInfo *find(uint32_t frame);\n> +\tSimpleFrameInfo *find(Request *request);\n> +\n> +private:\n> +\tstd::map frameInfo_;\n> +};\n> +\n> +void SimpleFrames::create(Request *request)\n> +{\n> +\tuint32_t frame = request->sequence();\n> +\tauto [it, inserted] = frameInfo_.try_emplace(request->sequence());\n\n\tconst uint32_t frame = request->sequence();\n\tauto [it, inserted] = frameInfo_.try_emplace(frame);\n\n> +\tASSERT(inserted);\n> +\tit->second.frame = frame;\n> +\tit->second.request = request;\n\n... then you can write\n\n\tauto [it, inserted] = frameInfo_.try_emplace(frame, frame, request);\n\tASSERT(inserted);\n\n> +}\n> +\n> +void SimpleFrames::destroy(uint32_t frame)\n> +{\n> +\tframeInfo_.erase(frame);\n> +}\n> +\n> +void SimpleFrames::clear()\n> +{\n> +\tframeInfo_.clear();\n> +}\n> +\n> +SimpleFrameInfo *SimpleFrames::find(uint32_t frame)\n> +{\n> +\tauto info = frameInfo_.find(frame);\n> +\tif (info == frameInfo_.end())\n> +\t\treturn nullptr;\n> +\treturn &info->second;\n> +}\n> +\n> +SimpleFrameInfo *SimpleFrames::find(Request *request)\n> +{\n> +\tfor (auto &itInfo : frameInfo_) {\n> +\t\tSimpleFrameInfo *info = &itInfo.second;\n> +\n> +\t\tif (info->request == request)\n> +\t\t\treturn info;\n> +\t}\n\nThat's fairly inefficient. Can't you drop this function and replace\n\n\tfind(request)\n\nwith\n\n\tfind(request->sequence())\n\n? Or keep it as a convenience and simply\n\n\treturn find(request->sequence());\n\nReviewed-by: Laurent Pinchart \n\n> +\n> +\treturn nullptr;\n> +}\n> +\n> struct SimplePipelineInfo {\n> \tconst char *driver;\n> \t/*\n> @@ -293,11 +351,13 @@ public:\n> \n> \tstd::unique_ptr converter_;\n> \tstd::unique_ptr swIsp_;\n> +\tSimpleFrames frameInfo_;\n> \n> private:\n> \tvoid tryPipeline(unsigned int code, const Size &size);\n> \tstatic std::vector routedSourcePads(MediaPad *sink);\n> \n> +\tvoid completeRequest(Request *request);\n> \tvoid conversionInputDone(FrameBuffer *buffer);\n> \tvoid conversionOutputDone(FrameBuffer *buffer);\n> \n> @@ -785,7 +845,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \t\t\t/* No conversion, just complete the request. */\n> \t\t\tRequest *request = buffer->request();\n> \t\t\tpipe->completeBuffer(request, buffer);\n> -\t\t\tpipe->completeRequest(request);\n> +\t\t\tcompleteRequest(request);\n> \t\t\treturn;\n> \t\t}\n> \n> @@ -803,7 +863,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \t\tconst RequestOutputs &outputs = conversionQueue_.front();\n> \t\tfor (auto &[stream, buf] : outputs.outputs)\n> \t\t\tpipe->completeBuffer(outputs.request, buf);\n> -\t\tpipe->completeRequest(outputs.request);\n> +\t\tcompleteRequest(outputs.request);\n> \t\tconversionQueue_.pop();\n> \n> \t\treturn;\n> @@ -861,7 +921,7 @@ void SimpleCameraData::imageBufferReady(FrameBuffer *buffer)\n> \n> \t/* Otherwise simply complete the request. */\n> \tpipe->completeBuffer(request, buffer);\n> -\tpipe->completeRequest(request);\n> +\tcompleteRequest(request);\n> }\n> \n> void SimpleCameraData::clearIncompleteRequests()\n> @@ -872,6 +932,18 @@ void SimpleCameraData::clearIncompleteRequests()\n> \t}\n> }\n> \n> +void SimpleCameraData::completeRequest(Request *request)\n> +{\n> +\tSimpleFrameInfo *info = frameInfo_.find(request);\n> +\tif (!info) {\n> +\t\t/* Something is really wrong, let's return. */\n> +\t\treturn;\n> +\t}\n> +\n> +\tframeInfo_.destroy(info->frame);\n> +\tpipe()->completeRequest(request);\n> +}\n> +\n> void SimpleCameraData::conversionInputDone(FrameBuffer *buffer)\n> {\n> \t/* Queue the input buffer back for capture. */\n> @@ -885,7 +957,7 @@ void SimpleCameraData::conversionOutputDone(FrameBuffer *buffer)\n> \t/* Complete the buffer and the request. */\n> \tRequest *request = buffer->request();\n> \tif (pipe->completeBuffer(request, buffer))\n> -\t\tpipe->completeRequest(request);\n> +\t\tcompleteRequest(request);\n> }\n> \n> void SimpleCameraData::ispStatsReady(uint32_t frame, uint32_t bufferId)\n> @@ -1398,6 +1470,7 @@ void SimplePipelineHandler::stopDevice(Camera *camera)\n> \n> \tvideo->bufferReady.disconnect(data, &SimpleCameraData::imageBufferReady);\n> \n> +\tdata->frameInfo_.clear();\n> \tdata->clearIncompleteRequests();\n> \tdata->conversionBuffers_.clear();\n> \n> @@ -1428,8 +1501,10 @@ int SimplePipelineHandler::queueRequestDevice(Camera *camera, Request *request)\n> \n> \tif (data->useConversion_) {\n> \t\tdata->conversionQueue_.push({ request, std::move(buffers) });\n> -\t\tif (data->swIsp_)\n> +\t\tif (data->swIsp_) {\n> +\t\t\tdata->frameInfo_.create(request);\n> \t\t\tdata->swIsp_->queueRequest(request->sequence(), request->controls());\n> +\t\t}\n> \t}\n> \n> \treturn 0;","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 E6487C3213\n\tfor ;\n\tWed, 26 Mar 2025 22:34:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0FC466896B;\n\tWed, 26 Mar 2025 23:34:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CA3FF68950\n\tfor ;\n\tWed, 26 Mar 2025 23:34:05 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 58D093A4;\n\tWed, 26 Mar 2025 23:32:17 +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=\"l9T+zyWz\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1743028337;\n\tbh=kREuS9/r/Ut4fLz3JGKC/da/qYAuduKY1N2DURbYqBQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l9T+zyWzfpsdiWyc1DHOzJw8OsR7qHWKifv+by4xTOD81kfKDFLBUqoyD3KD1UM1K\n\t+EAsS4fn9pTPnEgyWmNP50so416lumnmRy85alWLvej/lWNwKGaC/xDZtt+Ra9XPWc\n\tvKyHtHZl0zA5Je/kB7v8fSq49uy7e3HDkVoUpG5o=","Date":"Thu, 27 Mar 2025 00:33:42 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tKieran Bingham ","Subject":"Re: [PATCH v5 1/6] libcamera: software_isp: Track frames and\n\trequests","Message-ID":"<20250326223342.GB21053@pendragon.ideasonboard.com>","References":"<20250326124856.75709-1-mzamazal@redhat.com>\n\t<20250326124856.75709-2-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20250326124856.75709-2-mzamazal@redhat.com>","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]