[{"id":27603,"web_url":"https://patchwork.libcamera.org/comment/27603/","msgid":"","date":"2023-07-24T07:50:03","subject":"Re: [libcamera-devel] [PATCH v1 4/4] pipeline: rpi: Simplify buffer\n\tid generation","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Naush\n\nOn Fri, Jul 21, 2023 at 10:37:59AM +0100, Naushir Patuck via libcamera-devel wrote:\n> Replace the buffer id generation in RPi::Stream with a simple integer\n> counter since ids don't get recycled any more.\n>\n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/rpi/common/rpi_stream.cpp | 9 ++--\n> .../pipeline/rpi/common/rpi_stream.h | 42 +------------------\n> 2 files changed, 5 insertions(+), 46 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> index e24531e171c8..d32163d3fc0f 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> @@ -53,7 +53,7 @@ void Stream::resetBuffers()\n> void Stream::setExportedBuffers(std::vector> *buffers)\n> {\n> \tfor (auto const &buffer : *buffers)\n> -\t\tbufferMap_.emplace(id_.get(), buffer.get());\n> +\t\tbufferMap_.emplace(++id_, buffer.get());\n\nThis make id_ start from 1. Is 0 a special reserved value ?\n\n> }\n>\n> const BufferMap &Stream::getBuffers() const\n> @@ -78,7 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const\n>\n> void Stream::setExportedBuffer(FrameBuffer *buffer)\n> {\n> -\tbufferMap_.emplace(id_.get(), buffer);\n> +\tbufferMap_.emplace(++id_, buffer);\n> }\n>\n> int Stream::prepareBuffers(unsigned int count)\n> @@ -149,9 +149,6 @@ void Stream::returnBuffer(FrameBuffer *buffer)\n> \t/* Push this buffer back into the queue to be used again. */\n> \tavailableBuffers_.push(buffer);\n>\n> -\t/* Allow the buffer id to be reused. */\n> -\tid_.release(getBufferId(buffer));\n> -\n> \t/*\n> \t * Do we have any Request buffers that are waiting to be queued?\n> \t * If so, do it now as availableBuffers_ will not be empty.\n> @@ -210,7 +207,7 @@ void Stream::clearBuffers()\n> \trequestBuffers_ = std::queue{};\n> \tinternalBuffers_.clear();\n> \tbufferMap_.clear();\n> -\tid_.reset();\n> +\tid_ = 0;\n> }\n>\n> int Stream::queueToDevice(FrameBuffer *buffer)\n> diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> index d1289c4679b9..cc5d3f4afad4 100644\n> --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> @@ -60,7 +60,7 @@ public:\n>\n> \tStream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None)\n> \t\t: flags_(flags), name_(name),\n> -\t\t dev_(std::make_unique(dev)), id_(BufferMask::MaskID)\n> +\t\t dev_(std::make_unique(dev)), id_(0)\n\nShould you change the other constructor too ?\n\n\tStream()\n\t\t: flags_(StreamFlag::None), id_(BufferMask::MaskID)\n\t{\n\t}\n\n\tStream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None)\n\t\t: flags_(flags), name_(name),\n\t\t dev_(std::make_unique(dev)), id_(0)\n\t{\n\t}\n\nIt also makes me wonder if you still need the BufferMask enum..\n\n> \t{\n> \t}\n>\n> @@ -86,44 +86,6 @@ public:\n> \tvoid releaseBuffers();\n>\n> private:\n> -\tclass IdGenerator\n> -\t{\n> -\tpublic:\n> -\t\tIdGenerator(unsigned int max)\n> -\t\t\t: max_(max), id_(0)\n> -\t\t{\n> -\t\t}\n> -\n> -\t\tunsigned int get()\n> -\t\t{\n> -\t\t\tunsigned int id;\n> -\t\t\tif (!recycle_.empty()) {\n> -\t\t\t\tid = recycle_.front();\n> -\t\t\t\trecycle_.pop();\n> -\t\t\t} else {\n> -\t\t\t\tid = ++id_;\n> -\t\t\t\tASSERT(id_ <= max_);\n> -\t\t\t}\n> -\t\t\treturn id;\n> -\t\t}\n> -\n> -\t\tvoid release(unsigned int id)\n> -\t\t{\n> -\t\t\trecycle_.push(id);\n> -\t\t}\n> -\n> -\t\tvoid reset()\n> -\t\t{\n> -\t\t\tid_ = 0;\n> -\t\t\trecycle_ = {};\n> -\t\t}\n> -\n> -\tprivate:\n> -\t\tunsigned int max_;\n> -\t\tunsigned int id_;\n> -\t\tstd::queue recycle_;\n> -\t};\n> -\n> \tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n>\n> @@ -136,7 +98,7 @@ private:\n> \tstd::unique_ptr dev_;\n>\n> \t/* Tracks a unique id key for the bufferMap_ */\n> -\tIdGenerator id_;\n> +\tunsigned int id_;\n>\n> \t/* All frame buffers associated with this device stream. */\n> \tBufferMap bufferMap_;\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 7C749BDC71\n\tfor ;\n\tMon, 24 Jul 2023 07:50:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3554C628BF;\n\tMon, 24 Jul 2023 09:50:09 +0200 (CEST)","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 0B23A61E25\n\tfor ;\n\tMon, 24 Jul 2023 09:50:08 +0200 (CEST)","from ideasonboard.com (mob-5-91-20-233.net.vodafone.it\n\t[5.91.20.233])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 61B45735;\n\tMon, 24 Jul 2023 09:49:10 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690185009;\n\tbh=OkhyOyyFJneZqf4M8sz84GwLkhnD0IjyJfCbX5zPhr8=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PNDUCcrmc2GPT8VWAGgFofP30VCemHSGDbKD+aGjhf7kQ6UdqWD7rjEnvdOywIuIw\n\tfD3+1C7K6Q4wXh2SbcseYI/ZUjgYLJuBEEp8nIpHW8fhfBTaD1kj2zCiQrxKVY/657\n\tk+stWgSBOoZ5L0/so58TRzYn5XWENQLRPJXzk4KJekg+MFo4eBh/khF7RWcpQ5DNtB\n\t1iOjN4tlQ2T1g8bDuHqxxNA+q3eAviUgB+siQAhzziU8/yN+bwe84UFfQa4QWiu5e9\n\tYmIDBgS7XKO5DDDkFq/CNiy6o6EHiCcEEyzeVHAKPquGZ8p41ChxsCQLOrwJ0ishVD\n\tNo3earTC//qmw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1690184950;\n\tbh=OkhyOyyFJneZqf4M8sz84GwLkhnD0IjyJfCbX5zPhr8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MxJDFvT6Mj2kuhp8wvXk+op+BElKrPUZhj4AviWDf/+V9Wq49WcW///HaD0on2lz9\n\tvNCQI8AX/ptZHrzLSuJBeSF6LSf+APIZFX7s03Z6b6+dmTDhmluKUwmGAqHMr4r5Jw\n\t8cXsXFPYji8OHFflVJJ97MItHoqSFigq3uOHmRGY="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"MxJDFvT6\"; dkim-atps=neutral","Date":"Mon, 24 Jul 2023 09:50:03 +0200","To":"Naushir Patuck ","Message-ID":"","References":"<20230721093759.27700-1-naush@raspberrypi.com>\n\t<20230721093759.27700-5-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230721093759.27700-5-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] pipeline: rpi: Simplify buffer\n\tid generation","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27605,"web_url":"https://patchwork.libcamera.org/comment/27605/","msgid":"","date":"2023-07-25T08:01:04","subject":"Re: [libcamera-devel] [PATCH v1 4/4] pipeline: rpi: Simplify buffer\n\tid generation","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Jacopo,\n\nThank you for your review!\n\nOn Mon, 24 Jul 2023 at 08:50, Jacopo Mondi\n wrote:\n>\n> Hi Naush\n>\n> On Fri, Jul 21, 2023 at 10:37:59AM +0100, Naushir Patuck via libcamera-devel wrote:\n> > Replace the buffer id generation in RPi::Stream with a simple integer\n> > counter since ids don't get recycled any more.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/rpi/common/rpi_stream.cpp | 9 ++--\n> > .../pipeline/rpi/common/rpi_stream.h | 42 +------------------\n> > 2 files changed, 5 insertions(+), 46 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > index e24531e171c8..d32163d3fc0f 100644\n> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.cpp\n> > @@ -53,7 +53,7 @@ void Stream::resetBuffers()\n> > void Stream::setExportedBuffers(std::vector> *buffers)\n> > {\n> > for (auto const &buffer : *buffers)\n> > - bufferMap_.emplace(id_.get(), buffer.get());\n> > + bufferMap_.emplace(++id_, buffer.get());\n>\n> This make id_ start from 1. Is 0 a special reserved value ?\n\nThe 0 value is reserved for invalid id, so id_ does start from 1.\n\n>\n> > }\n> >\n> > const BufferMap &Stream::getBuffers() const\n> > @@ -78,7 +78,7 @@ unsigned int Stream::getBufferId(FrameBuffer *buffer) const\n> >\n> > void Stream::setExportedBuffer(FrameBuffer *buffer)\n> > {\n> > - bufferMap_.emplace(id_.get(), buffer);\n> > + bufferMap_.emplace(++id_, buffer);\n> > }\n> >\n> > int Stream::prepareBuffers(unsigned int count)\n> > @@ -149,9 +149,6 @@ void Stream::returnBuffer(FrameBuffer *buffer)\n> > /* Push this buffer back into the queue to be used again. */\n> > availableBuffers_.push(buffer);\n> >\n> > - /* Allow the buffer id to be reused. */\n> > - id_.release(getBufferId(buffer));\n> > -\n> > /*\n> > * Do we have any Request buffers that are waiting to be queued?\n> > * If so, do it now as availableBuffers_ will not be empty.\n> > @@ -210,7 +207,7 @@ void Stream::clearBuffers()\n> > requestBuffers_ = std::queue{};\n> > internalBuffers_.clear();\n> > bufferMap_.clear();\n> > - id_.reset();\n> > + id_ = 0;\n> > }\n> >\n> > int Stream::queueToDevice(FrameBuffer *buffer)\n> > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > index d1289c4679b9..cc5d3f4afad4 100644\n> > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h\n> > @@ -60,7 +60,7 @@ public:\n> >\n> > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None)\n> > : flags_(flags), name_(name),\n> > - dev_(std::make_unique(dev)), id_(BufferMask::MaskID)\n> > + dev_(std::make_unique(dev)), id_(0)\n>\n> Should you change the other constructor too ?\n\nYes - it's inconsequential, but it would be correct to change it as well.\n\n>\n> Stream()\n> : flags_(StreamFlag::None), id_(BufferMask::MaskID)\n> {\n> }\n>\n> Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None)\n> : flags_(flags), name_(name),\n> dev_(std::make_unique(dev)), id_(0)\n> {\n> }\n>\n> It also makes me wonder if you still need the BufferMask enum..\n\nI do still need this enum as it differentiates between stats and embedded data\nbuffer Ids that get passed from PH to the IPA. Buffer Ids are unique in a a\nsingle stream, but can be the same across different streams... if that\nmakes sense?\n\nRegards,\nNaush\n\n>\n> > {\n> > }\n> >\n> > @@ -86,44 +86,6 @@ public:\n> > void releaseBuffers();\n> >\n> > private:\n> > - class IdGenerator\n> > - {\n> > - public:\n> > - IdGenerator(unsigned int max)\n> > - : max_(max), id_(0)\n> > - {\n> > - }\n> > -\n> > - unsigned int get()\n> > - {\n> > - unsigned int id;\n> > - if (!recycle_.empty()) {\n> > - id = recycle_.front();\n> > - recycle_.pop();\n> > - } else {\n> > - id = ++id_;\n> > - ASSERT(id_ <= max_);\n> > - }\n> > - return id;\n> > - }\n> > -\n> > - void release(unsigned int id)\n> > - {\n> > - recycle_.push(id);\n> > - }\n> > -\n> > - void reset()\n> > - {\n> > - id_ = 0;\n> > - recycle_ = {};\n> > - }\n> > -\n> > - private:\n> > - unsigned int max_;\n> > - unsigned int id_;\n> > - std::queue recycle_;\n> > - };\n> > -\n> > void clearBuffers();\n> > int queueToDevice(FrameBuffer *buffer);\n> >\n> > @@ -136,7 +98,7 @@ private:\n> > std::unique_ptr dev_;\n> >\n> > /* Tracks a unique id key for the bufferMap_ */\n> > - IdGenerator id_;\n> > + unsigned int id_;\n> >\n> > /* All frame buffers associated with this device stream. */\n> > BufferMap bufferMap_;\n> > --\n> > 2.34.1\n> >","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9A912BDC71\n\tfor ;\n\tTue, 25 Jul 2023 08:01:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92AA1628BE;\n\tTue, 25 Jul 2023 10:01:13 +0200 (CEST)","from mail-yw1-x1131.google.com (mail-yw1-x1131.google.com\n\t[IPv6:2607:f8b0:4864:20::1131])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 55E14600F7\n\tfor ;\n\tTue, 25 Jul 2023 10:01:11 +0200 (CEST)","by mail-yw1-x1131.google.com with SMTP id\n\t00721157ae682-583f99641adso20841397b3.2\n\tfor ;\n\tTue, 25 Jul 2023 01:01:11 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1690272073;\n\tbh=kynhQ30KvDZz0FLdJM8WvynKOCjKjCkhe0aIK9I80y4=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=s3jxQi+JDxkhMGZi/1QcL3NbH2Tfi/xBta6sZZ8LnDp5WEWLqlVI5l7Ushxxttd1u\n\toKR3iStOafmeRbQzY7dZVpJvPo7dNU6xSTTYjZPo64EoHh7WYpon4ZV2ihGRpcNTGv\n\tFmgQgS/p1cLnCYjOqYlpdY1d8lKeCFQ/LfxnPft0ZKzoFcEHYBb8C66btol89fB69S\n\tcM3NPHUBeyjP9k2Gqw/TT/jgTPK9XR5EHDILQhgE8MjYTgDtCr9xRJ1fWRJlYBRtp7\n\tlVnwvNiQRGsOR5VDnxpiB0hxeXF3hUFNZKIn/69RUe9386wUMEnNP4FLVLvyQ3UF+7\n\tN/WFUWCPWH3FA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google; t=1690272070; x=1690876870;\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=2LqpZuQdxGZAoMnZzCUkN/yxMIyLfSlwQ75yCr2i2J4=;\n\tb=IwzB+RwdKQ0KZQclxHpDdT9ijfBNiekYPST6WdZ49jnOUoBVmfjoBshyq36ADtWtwG\n\tc7NkSbr5vqCqLjEnSzbo1FFb5jw5SR3BI1XSlmXiWFMpJel8oucwgFYVWiWZ6kpv+l+Y\n\trT6G8+rfQc8esWG28eMGOVwtro3lPfcWeYkdPBjmBOQBs0nJ4Hi+K2011x9fsPpCfGJR\n\tCo5hG4iuLdcsYuY44GqFjRLYMufh44IFSn9V4qaYyGeY4lWw/d7rCO/kn41pZKkUD3NC\n\tWtFZuzXGvD7+YYQq2a7Ugc5BbvsAj/KPJXMrxfjWbaZtXNgvYe8Xfo/f2j/Ll6C2gg3k\n\t9eHA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=raspberrypi.com\n\theader.i=@raspberrypi.com\n\theader.b=\"IwzB+Rwd\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1690272070; x=1690876870;\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=2LqpZuQdxGZAoMnZzCUkN/yxMIyLfSlwQ75yCr2i2J4=;\n\tb=J5ry02XHEIANwYOx/jPpMbyvntRpb5BWisgeiQ6bdPDYzEp4ZaCxEQqK67m1MYgaFq\n\tcu4gdiIPuqD1scYErXfK7MZi188KVTfRAj69Ah1p1m1EY8uhveJQq0qffTvWGSxFZcsZ\n\tXYOmdL3uMkPrbk0oFYMxgpSMG7EuiAz98upWR26/hOS+kY3f3VALogYQ8ss+bgv6+ExS\n\tACQdnJ1y2CIDewlaT4ENw4r4xCUMLTlhoZWGUF4myqWR5bNSOQibtsOmBGm835I80L1n\n\tMoUv0pM0iv2Dp3cr3827Kv7re3nsdvVnr0CFIN8V2S4ynHka0kgbW9fQQ9Z5EfGfe8Vl\n\tAXfQ==","X-Gm-Message-State":"ABy/qLZpbdWEH26I9Q+oW1hRR934Cqym762epwPRwqeTnlfvaXxcBdUg\n\tFpQZdoLoCqHE2I+TM4931z9LV1BQaHyt1darZcdcwQ==","X-Google-Smtp-Source":"APBJJlHldXShesLQSlbYwrJf9Md/ZfM+HI6VYHYixyC/OLvaKi73QWiNLIY/6V9kGYLyuakOvjK7g12zOTC4YGXnQQ0=","X-Received":"by 2002:a0d:ffc5:0:b0:55a:574f:327c with SMTP id\n\tp188-20020a0dffc5000000b0055a574f327cmr8010731ywf.13.1690272070023;\n\tTue, 25 Jul 2023 01:01:10 -0700 (PDT)","MIME-Version":"1.0","References":"<20230721093759.27700-1-naush@raspberrypi.com>\n\t<20230721093759.27700-5-naush@raspberrypi.com>\n\t","In-Reply-To":"","Date":"Tue, 25 Jul 2023 09:01:04 +0100","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH v1 4/4] pipeline: rpi: Simplify buffer\n\tid generation","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","From":"Naushir Patuck via libcamera-devel\n\t","Reply-To":"Naushir Patuck ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]