[{"id":12568,"web_url":"https://patchwork.libcamera.org/comment/12568/","msgid":"","date":"2020-09-18T12:53:52","subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 18/09/2020 10:42, Naushir Patuck wrote:\n> By using a map container, we can easily insert/remove buffers from the\n> buffer list. This will be required to track buffers allocated externally\n> and passed to the pipeline handler through a Request.\n> \n> Replace the buffer index tracking with an id generated internally by the\n> stream object.\n> \n> Signed-off-by: Naushir Patuck \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++-----\n> .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++--\n> 3 files changed, 82 insertions(+), 35 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7d91188c..51544233 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> -\tunsigned int index;\n> \tint ret;\n> \n> \t/*\n> @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * This will allow us to identify buffers passed between the pipeline\n> \t * handler and the IPA.\n> \t */\n> -\tindex = 0;\n> \tfor (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> -\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,\n> -\t\t\t\t\t .planes = b->planes() });\n> -\t\tindex++;\n> +\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first,\n> +\t\t\t\t\t .planes = b.second->planes() });\n> \t}\n> \n> -\tindex = 0;\n> \tfor (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> -\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,\n> -\t\t\t\t\t .planes = b->planes() });\n> -\t\tindex++;\n> +\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first,\n> +\t\t\t\t\t .planes = b.second->planes() });\n> \t}\n> \n> \tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tfor (RPi::RPiStream &s : unicam_) {\n> -\t\tindex = s.getBufferIndex(buffer);\n> +\t\tindex = s.getBufferId(buffer);\n> \t\tif (index != -1) {\n> \t\t\tstream = &s;\n> \t\t\tbreak;\n> @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tLOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> -\t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferIndex(buffer)\n> +\t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> \t/* The ISP input buffer gets re-queued into Unicam. */\n> @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tfor (RPi::RPiStream &s : isp_) {\n> -\t\tindex = s.getBufferIndex(buffer);\n> +\t\tindex = s.getBufferId(buffer);\n> \t\tif (index != -1) {\n> \t\t\tstream = &s;\n> \t\t\tbreak;\n> @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline()\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n> \n> -\tunsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);\n> -\tunsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);\n> +\tunsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);\n> +\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> \n> \tLOG(RPI, Debug) << \"Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:\"\n> -\t\t\t<< \" Bayer buffer id: \" << bayerIndex\n> -\t\t\t<< \" Embedded buffer id: \" << embeddedIndex;\n> +\t\t\t<< \" Bayer buffer id: \" << bayerId\n> +\t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \n> \top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> -\t\t RPiIpaMask::BAYER_DATA | bayerIndex };\n> +\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId,\n> +\t\t RPiIpaMask::BAYER_DATA | bayerId };\n> \tipa_->processEvent(op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 80170d64..aee0aa2d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const\n> \n> void RPiStream::setExportedBuffers(std::vector> *buffers)\n> {\n> -\tstd::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> -\t\t [](std::unique_ptr &b) { return b.get(); });\n> +\tfor (auto const &buffer : *buffers)\n> +\t\tbufferMap_.emplace(id_.get(), buffer.get());\n> }\n> \n> -const std::vector &RPiStream::getBuffers() const\n> +const BufferMap &RPiStream::getBuffers() const\n> {\n> -\treturn bufferList_;\n> +\treturn bufferMap_;\n> }\n> \n> -int RPiStream::getBufferIndex(FrameBuffer *buffer) const\n> +int RPiStream::getBufferId(FrameBuffer *buffer) const\n> {\n> \tif (importOnly_)\n> \t\treturn -1;\n> \n> -\t/* Find the buffer in the list, and return the index position. */\n> -\tauto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);\n> -\tif (it != bufferList_.end())\n> -\t\treturn std::distance(bufferList_.begin(), it);\n> +\t/* Find the buffer in the map, and return the buffer id. */\n> +\tauto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),\n> +\t\t\t [&buffer](auto const &p) { return p.second == buffer; });\n> \n> -\treturn -1;\n> +\tif (it == bufferMap_.end())\n> +\t\treturn -1;\n> +\n> +\treturn it->first;\n> }\n> \n> int RPiStream::prepareBuffers(unsigned int count)\n> @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count)\n> \t\t}\n> \n> \t\t/* We must import all internal/external exported buffers. */\n> -\t\tcount = bufferList_.size();\n> +\t\tcount = bufferMap_.size();\n> \t}\n> \n> \treturn dev_->importBuffers(count);\n> @@ -139,6 +141,9 @@ void RPiStream::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> @@ -196,12 +201,13 @@ void RPiStream::clearBuffers()\n> \tavailableBuffers_ = std::queue{};\n> \trequestBuffers_ = std::queue{};\n> \tinternalBuffers_.clear();\n> -\tbufferList_.clear();\n> +\tbufferMap_.clear();\n> +\tid_.reset();\n> }\n> \n> int RPiStream::queueToDevice(FrameBuffer *buffer)\n> {\n> -\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferIndex(buffer)\n> +\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> \t\t\t << \" for \" << name_;\n> \n> \tint ret = dev_->queueBuffer(buffer);\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 959e9147..df986367 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -9,8 +9,10 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> @@ -19,6 +21,8 @@ namespace libcamera {\n> \n> namespace RPi {\n> \n> +using BufferMap = std::unordered_map;\n> +\n> /*\n> * Device stream abstraction for either an internal or external stream.\n> * Used for both Unicam and the ISP.\n> @@ -27,12 +31,13 @@ class RPiStream : public Stream\n> {\n> public:\n> \tRPiStream()\n> +\t\t: id_(RPiIpaMask::ID)\n> \t{\n> \t}\n> \n> \tRPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n> \t\t: external_(false), importOnly_(importOnly), name_(name),\n> -\t\t dev_(std::make_unique(dev))\n> +\t\t dev_(std::make_unique(dev)), id_(RPiIpaMask::ID)\n> \t{\n> \t}\n> \n> @@ -45,8 +50,8 @@ public:\n> \tbool isExternal() const;\n> \n> \tvoid setExportedBuffers(std::vector> *buffers);\n> -\tconst std::vector &getBuffers() const;\n> -\tint getBufferIndex(FrameBuffer *buffer) const;\n> +\tconst BufferMap &getBuffers() const;\n> +\tint getBufferId(FrameBuffer *buffer) const;\n> \n> \tint prepareBuffers(unsigned int count);\n> \tint queueBuffer(FrameBuffer *buffer);\n> @@ -56,6 +61,44 @@ public:\n> \tvoid releaseBuffers();\n> \n> private:\n> +\tclass IdGenerator\n> +\t{\n> +\tpublic:\n> +\t\tIdGenerator(int max)\n\nShould this provide a default value for max?\n\nPerhaps int max = 32?\n\n(See below).\n\n> +\t\t\t: max_(max), id_(0)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tint get()\n> +\t\t{\n> +\t\t\tint 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(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\tint max_;\n> +\t\tint id_;\n> +\t\tstd::queue recycle_;\n> +\t};\n> +\n> \tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n> \n> @@ -74,8 +117,11 @@ private:\n> \t/* The actual device stream. */\n> \tstd::unique_ptr dev_;\n> \n> +\t/* Tracks a unique id key for the bufferMap_ */\n> +\tIdGenerator id_;\n\nThe only constructor for IdGenerator takes an int 'max' value, to assign\nto max_ which has the assertion check.\n\nThat sounds useful to me for validation, but I can't see how this works\n(I'm probably missing something).\n\nWith no default parameter, and nothing declared in the class definition\nas a default value, what is max_ set to ?\n(Some how I assume it would be zero?, but I would expect the compiler to\ncomplain too)\n\nActually, I think what probably happens is it uses a 'default\nconstructor' and leaves the id_ and max_ values uninitialised, so it's\nprobably works by chance ?\n\nOther than that, I think this is neat. A quick and fast solution to the\nproblem, and we never expect recycle queue to grow beyond the maximum\nquantity of unique buffers running in the system at any one time.\n\nWith the max_ issue cleared:\n\nReviewed-by: Kieran Bingham \n\nThere shouldn't be any need to resend the whole series for this.\nIf it does need fixing, either send an 8.1 of just this patch, or we can\nfix while applying perhaps, if it's just providing the default value.\n--\nKieran\n\n\n\n\n\n> +\n> \t/* All frame buffers associated with this device stream. */\n> -\tstd::vector bufferList_;\n> +\tBufferMap bufferMap_;\n> \n> \t/*\n> \t * List of frame buffers that we can use if none have been provided by\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 22334BF01C\n\tfor ;\n\tFri, 18 Sep 2020 12:53:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D0F762FB5;\n\tFri, 18 Sep 2020 14:53:57 +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 33E9D60576\n\tfor ;\n\tFri, 18 Sep 2020 14:53:56 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 943D52D7;\n\tFri, 18 Sep 2020 14:53:55 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"mAHvBulB\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600433635;\n\tbh=nLr7LS/C4PfRiO5LslqRzmdRlrtOpJAXojuf6pTWfbM=;\n\th=Reply-To:Subject:To:References:From:Date:In-Reply-To:From;\n\tb=mAHvBulBDKycS5pXDwCQX82/pLKfvpHFhfzPU5EvSSts+95+on/4TR3B7LRphiQhC\n\tzrRWhVEcIMIJzA18WW7AOgqIjS3ao1h7eYHR68Yhk9zPZboJKOYpooS4ZyC1iztzTJ\n\tdL0mRbBYzB9piJsowYgphRh/U8TxiulSDI5JwrTE=","To":"Naushir Patuck ,\n\tlibcamera-devel@lists.libcamera.org","References":"<20200918094233.5273-1-naush@raspberrypi.com>\n\t<20200918094233.5273-11-naush@raspberrypi.com>","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"","Date":"Fri, 18 Sep 2020 13:53:52 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"<20200918094233.5273-11-naush@raspberrypi.com>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","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","Reply-To":"kieran.bingham@ideasonboard.com","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12570,"web_url":"https://patchwork.libcamera.org/comment/12570/","msgid":"","date":"2020-09-18T13:00:52","subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Kieran,\n\nThank you for your review.\n\nOn Fri, 18 Sep 2020 at 13:53, Kieran Bingham\n wrote:\n>\n> Hi Naush,\n>\n> On 18/09/2020 10:42, Naushir Patuck wrote:\n> > By using a map container, we can easily insert/remove buffers from the\n> > buffer list. This will be required to track buffers allocated externally\n> > and passed to the pipeline handler through a Request.\n> >\n> > Replace the buffer index tracking with an id generated internally by the\n> > stream object.\n> >\n> > Signed-off-by: Naushir Patuck \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------\n> > .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++-----\n> > .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++--\n> > 3 files changed, 82 insertions(+), 35 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index 7d91188c..51544233 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> > int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > {\n> > RPiCameraData *data = cameraData(camera);\n> > - unsigned int index;\n> > int ret;\n> >\n> > /*\n> > @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> > * This will allow us to identify buffers passed between the pipeline\n> > * handler and the IPA.\n> > */\n> > - index = 0;\n> > for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,\n> > - .planes = b->planes() });\n> > - index++;\n> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first,\n> > + .planes = b.second->planes() });\n> > }\n> >\n> > - index = 0;\n> > for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> > - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,\n> > - .planes = b->planes() });\n> > - index++;\n> > + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first,\n> > + .planes = b.second->planes() });\n> > }\n> >\n> > data->ipa_->mapBuffers(data->ipaBuffers_);\n> > @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> > return;\n> >\n> > for (RPi::RPiStream &s : unicam_) {\n> > - index = s.getBufferIndex(buffer);\n> > + index = s.getBufferId(buffer);\n> > if (index != -1) {\n> > stream = &s;\n> > break;\n> > @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n> > return;\n> >\n> > LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> > - << \", buffer id \" << unicam_[Unicam::Image].getBufferIndex(buffer)\n> > + << \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> > << \", timestamp: \" << buffer->metadata().timestamp;\n> >\n> > /* The ISP input buffer gets re-queued into Unicam. */\n> > @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> > return;\n> >\n> > for (RPi::RPiStream &s : isp_) {\n> > - index = s.getBufferIndex(buffer);\n> > + index = s.getBufferId(buffer);\n> > if (index != -1) {\n> > stream = &s;\n> > break;\n> > @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline()\n> > /* Set our state to say the pipeline is active. */\n> > state_ = State::Busy;\n> >\n> > - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);\n> > - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);\n> > + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);\n> > + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> >\n> > LOG(RPI, Debug) << \"Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:\"\n> > - << \" Bayer buffer id: \" << bayerIndex\n> > - << \" Embedded buffer id: \" << embeddedIndex;\n> > + << \" Bayer buffer id: \" << bayerId\n> > + << \" Embedded buffer id: \" << embeddedId;\n> >\n> > op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> > - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> > - RPiIpaMask::BAYER_DATA | bayerIndex };\n> > + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId,\n> > + RPiIpaMask::BAYER_DATA | bayerId };\n> > ipa_->processEvent(op);\n> > }\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > index 80170d64..aee0aa2d 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> > @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const\n> >\n> > void RPiStream::setExportedBuffers(std::vector> *buffers)\n> > {\n> > - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> > - [](std::unique_ptr &b) { return b.get(); });\n> > + for (auto const &buffer : *buffers)\n> > + bufferMap_.emplace(id_.get(), buffer.get());\n> > }\n> >\n> > -const std::vector &RPiStream::getBuffers() const\n> > +const BufferMap &RPiStream::getBuffers() const\n> > {\n> > - return bufferList_;\n> > + return bufferMap_;\n> > }\n> >\n> > -int RPiStream::getBufferIndex(FrameBuffer *buffer) const\n> > +int RPiStream::getBufferId(FrameBuffer *buffer) const\n> > {\n> > if (importOnly_)\n> > return -1;\n> >\n> > - /* Find the buffer in the list, and return the index position. */\n> > - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);\n> > - if (it != bufferList_.end())\n> > - return std::distance(bufferList_.begin(), it);\n> > + /* Find the buffer in the map, and return the buffer id. */\n> > + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),\n> > + [&buffer](auto const &p) { return p.second == buffer; });\n> >\n> > - return -1;\n> > + if (it == bufferMap_.end())\n> > + return -1;\n> > +\n> > + return it->first;\n> > }\n> >\n> > int RPiStream::prepareBuffers(unsigned int count)\n> > @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count)\n> > }\n> >\n> > /* We must import all internal/external exported buffers. */\n> > - count = bufferList_.size();\n> > + count = bufferMap_.size();\n> > }\n> >\n> > return dev_->importBuffers(count);\n> > @@ -139,6 +141,9 @@ void RPiStream::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> > @@ -196,12 +201,13 @@ void RPiStream::clearBuffers()\n> > availableBuffers_ = std::queue{};\n> > requestBuffers_ = std::queue{};\n> > internalBuffers_.clear();\n> > - bufferList_.clear();\n> > + bufferMap_.clear();\n> > + id_.reset();\n> > }\n> >\n> > int RPiStream::queueToDevice(FrameBuffer *buffer)\n> > {\n> > - LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferIndex(buffer)\n> > + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> > << \" for \" << name_;\n> >\n> > int ret = dev_->queueBuffer(buffer);\n> > diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > index 959e9147..df986367 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> > @@ -9,8 +9,10 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > +#include \n> > #include \n> >\n> > #include \"libcamera/internal/v4l2_videodevice.h\"\n> > @@ -19,6 +21,8 @@ namespace libcamera {\n> >\n> > namespace RPi {\n> >\n> > +using BufferMap = std::unordered_map;\n> > +\n> > /*\n> > * Device stream abstraction for either an internal or external stream.\n> > * Used for both Unicam and the ISP.\n> > @@ -27,12 +31,13 @@ class RPiStream : public Stream\n> > {\n> > public:\n> > RPiStream()\n> > + : id_(RPiIpaMask::ID)\n> > {\n> > }\n> >\n> > RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n> > : external_(false), importOnly_(importOnly), name_(name),\n> > - dev_(std::make_unique(dev))\n> > + dev_(std::make_unique(dev)), id_(RPiIpaMask::ID)\n> > {\n> > }\n> >\n> > @@ -45,8 +50,8 @@ public:\n> > bool isExternal() const;\n> >\n> > void setExportedBuffers(std::vector> *buffers);\n> > - const std::vector &getBuffers() const;\n> > - int getBufferIndex(FrameBuffer *buffer) const;\n> > + const BufferMap &getBuffers() const;\n> > + int getBufferId(FrameBuffer *buffer) const;\n> >\n> > int prepareBuffers(unsigned int count);\n> > int queueBuffer(FrameBuffer *buffer);\n> > @@ -56,6 +61,44 @@ public:\n> > void releaseBuffers();\n> >\n> > private:\n> > + class IdGenerator\n> > + {\n> > + public:\n> > + IdGenerator(int max)\n>\n> Should this provide a default value for max?\n>\n> Perhaps int max = 32?\n>\n> (See below).\n>\n> > + : max_(max), id_(0)\n> > + {\n> > + }\n> > +\n> > + int get()\n> > + {\n> > + 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(int id)\n> > + {\n> > + recycle_.push(id);\n> > + }\n> > +\n> > + void reset()\n> > + {\n> > + id_ = 0;\n> > + recycle_ = {};\n> > + }\n> > +\n> > + private:\n> > + int max_;\n> > + int id_;\n> > + std::queue recycle_;\n> > + };\n> > +\n> > void clearBuffers();\n> > int queueToDevice(FrameBuffer *buffer);\n> >\n> > @@ -74,8 +117,11 @@ private:\n> > /* The actual device stream. */\n> > std::unique_ptr dev_;\n> >\n> > + /* Tracks a unique id key for the bufferMap_ */\n> > + IdGenerator id_;\n>\n> The only constructor for IdGenerator takes an int 'max' value, to assign\n> to max_ which has the assertion check.\n>\n> That sounds useful to me for validation, but I can't see how this works\n> (I'm probably missing something).\n>\n> With no default parameter, and nothing declared in the class definition\n> as a default value, what is max_ set to ?\n> (Some how I assume it would be zero?, but I would expect the compiler to\n> complain too)\n\nUnless I misunderstood your comment above, the IdGenerator class is\ninitialised in the RPiStream contstructors:\n\nRPiStream()\n: id_(RPiBufferMask::ID)\n{\n}\n\nRPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n: external_(false), importOnly_(importOnly), name_(name),\n dev_(std::make_unique(dev)), id_(RPiBufferMask::ID)\n{\n}\n\nIdGenerator::max_ takes the value RPiBufferMask::ID (0xffff) in this\ncase. Given no default value is used, the compiler ought to shout\nloudly if I were to construct an IdGenerator instance without a max\nparameter. Does that answer your query?\n\nRegards,\nNaush\n\n\n\n>\n> Actually, I think what probably happens is it uses a 'default\n> constructor' and leaves the id_ and max_ values uninitialised, so it's\n> probably works by chance ?\n>\n> Other than that, I think this is neat. A quick and fast solution to the\n> problem, and we never expect recycle queue to grow beyond the maximum\n> quantity of unique buffers running in the system at any one time.\n>\n> With the max_ issue cleared:\n>\n> Reviewed-by: Kieran Bingham \n>\n> There shouldn't be any need to resend the whole series for this.\n> If it does need fixing, either send an 8.1 of just this patch, or we can\n> fix while applying perhaps, if it's just providing the default value.\n> --\n> Kieran\n>\n>\n>\n>\n>\n> > +\n> > /* All frame buffers associated with this device stream. */\n> > - std::vector bufferList_;\n> > + BufferMap bufferMap_;\n> >\n> > /*\n> > * List of frame buffers that we can use if none have been provided by\n> >\n>\n> --\n> Regards\n> --\n> Kieran","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 7904ABF01C\n\tfor ;\n\tFri, 18 Sep 2020 13:01:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 0F85B62FBD;\n\tFri, 18 Sep 2020 15:01:11 +0200 (CEST)","from mail-lf1-x12c.google.com (mail-lf1-x12c.google.com\n\t[IPv6:2a00:1450:4864:20::12c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8C43D60576\n\tfor ;\n\tFri, 18 Sep 2020 15:01:09 +0200 (CEST)","by mail-lf1-x12c.google.com with SMTP id m5so6029986lfp.7\n\tfor ;\n\tFri, 18 Sep 2020 06:01:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"hibzYAHK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=rDlRAKU1pSB3n+pzhncr4SQOiPfMFBaFh8NRLcxtK3o=;\n\tb=hibzYAHKfL+qDLs1gLccBh0P5XYvglTDV7riJQSSrGXbcPIk4XaY4YtYWBycJN3J00\n\tA8J4lUCEOBSVx6JbibXUJMviGvRbLQgFdNHuXAsftlA3h2goC1hM3QBY5P5CLC6cbxm/\n\t+bT1r+HizyMoX8bCiIrIiVHWauqg++uNPRf+eXLiQBfIsmG0sE57P5Tm4jJwY7wpwMRf\n\tx9xlR4/dR4K2Crs9o1SzrQi8LEZy1ZwEUWiLPbcCeJuf0FMTJ7BOqOVbm1CDEFOxO/MB\n\tthGdUTVS6cYXSaZQ3yAILrdS2j+d2ng0uAXDrkSmQ5gaNLT2cryulFAu659a86N44cxH\n\tOcnA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=rDlRAKU1pSB3n+pzhncr4SQOiPfMFBaFh8NRLcxtK3o=;\n\tb=fCld5gDAfBK23QlS23h4QztWGcnT9V0+gbGiQ5V++FxfYJEc6n/3ZG9jROQw4IU3se\n\ty04UuU2tQDdEvJQn1HWRUUOSR/D7A8lSy5mrp+GJ1lBQzPRJa3oJtOOz+FvcQF98sF5D\n\tFGWWZ9ShU9wDf/0hlL5WJzqJi1XFJJM8sPJY426J8QvbUEyPseCvNJGwCZXt2aQwDBVA\n\tCIC6QF0reCpN+0MUSZ+zHTw7mgVGBw5LC/WVSBgw+pB5XL65tN8gviy9Ko/QZ0aZ/GNv\n\tu0Nd74LpqfIyihiN87hk6qggTB1Qw24VYFQrhvniejPjwXV4NZhJsbsmmVuDXoRwOL3A\n\tE2yA==","X-Gm-Message-State":"AOAM531TgkQRac3t2zXE7vas5aK/rOK9KB+unteXER771Zly4dZkyg2O\n\tsiUlyQrVw0BINkGdahzbKpdd9KBQ2jFlS7P1Nz0wGQ==","X-Google-Smtp-Source":"ABdhPJybCZr9Wp33c4BV1j+R/zhyGhEG7luv2dhN1GkMvzYvVnEZtIKH02tY/fAKAmfVCd7OTofJZoiTBfzaasGaDfM=","X-Received":"by 2002:a19:820c:: with SMTP id\n\te12mr10392089lfd.215.1600434068414; \n\tFri, 18 Sep 2020 06:01:08 -0700 (PDT)","MIME-Version":"1.0","References":"<20200918094233.5273-1-naush@raspberrypi.com>\n\t<20200918094233.5273-11-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 18 Sep 2020 14:00:52 +0100","Message-ID":"","To":"Kieran Bingham ","Subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12571,"web_url":"https://patchwork.libcamera.org/comment/12571/","msgid":"<422a2a84-fcb3-c36a-be2a-fa5d82b053c1@ideasonboard.com>","date":"2020-09-18T13:17:43","subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Naush,\n\nOn 18/09/2020 14:00, Naushir Patuck wrote:\n> Hi Kieran,\n> \n> Thank you for your review.\n> \n> On Fri, 18 Sep 2020 at 13:53, Kieran Bingham\n> wrote:\n>>\n>> Hi Naush,\n>>\n>> On 18/09/2020 10:42, Naushir Patuck wrote:\n>>> By using a map container, we can easily insert/remove buffers from the\n>>> buffer list. This will be required to track buffers allocated externally\n>>> and passed to the pipeline handler through a Request.\n>>>\n>>> Replace the buffer index tracking with an id generated internally by the\n>>> stream object.\n>>>\n>>> Signed-off-by: Naushir Patuck \n>>> ---\n>>> .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------\n>>> .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++-----\n>>> .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++--\n>>> 3 files changed, 82 insertions(+), 35 deletions(-)\n>>>\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> index 7d91188c..51544233 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n>>> @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n>>> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> {\n>>> RPiCameraData *data = cameraData(camera);\n>>> - unsigned int index;\n>>> int ret;\n>>>\n>>> /*\n>>> @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n>>> * This will allow us to identify buffers passed between the pipeline\n>>> * handler and the IPA.\n>>> */\n>>> - index = 0;\n>>> for (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n>>> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,\n>>> - .planes = b->planes() });\n>>> - index++;\n>>> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first,\n>>> + .planes = b.second->planes() });\n>>> }\n>>>\n>>> - index = 0;\n>>> for (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n>>> - data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,\n>>> - .planes = b->planes() });\n>>> - index++;\n>>> + data->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first,\n>>> + .planes = b.second->planes() });\n>>> }\n>>>\n>>> data->ipa_->mapBuffers(data->ipaBuffers_);\n>>> @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n>>> return;\n>>>\n>>> for (RPi::RPiStream &s : unicam_) {\n>>> - index = s.getBufferIndex(buffer);\n>>> + index = s.getBufferId(buffer);\n>>> if (index != -1) {\n>>> stream = &s;\n>>> break;\n>>> @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n>>> return;\n>>>\n>>> LOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n>>> - << \", buffer id \" << unicam_[Unicam::Image].getBufferIndex(buffer)\n>>> + << \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n>>> << \", timestamp: \" << buffer->metadata().timestamp;\n>>>\n>>> /* The ISP input buffer gets re-queued into Unicam. */\n>>> @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n>>> return;\n>>>\n>>> for (RPi::RPiStream &s : isp_) {\n>>> - index = s.getBufferIndex(buffer);\n>>> + index = s.getBufferId(buffer);\n>>> if (index != -1) {\n>>> stream = &s;\n>>> break;\n>>> @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline()\n>>> /* Set our state to say the pipeline is active. */\n>>> state_ = State::Busy;\n>>>\n>>> - unsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);\n>>> - unsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);\n>>> + unsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);\n>>> + unsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n>>>\n>>> LOG(RPI, Debug) << \"Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:\"\n>>> - << \" Bayer buffer id: \" << bayerIndex\n>>> - << \" Embedded buffer id: \" << embeddedIndex;\n>>> + << \" Bayer buffer id: \" << bayerId\n>>> + << \" Embedded buffer id: \" << embeddedId;\n>>>\n>>> op.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n>>> - op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n>>> - RPiIpaMask::BAYER_DATA | bayerIndex };\n>>> + op.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId,\n>>> + RPiIpaMask::BAYER_DATA | bayerId };\n>>> ipa_->processEvent(op);\n>>> }\n>>>\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> index 80170d64..aee0aa2d 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n>>> @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const\n>>>\n>>> void RPiStream::setExportedBuffers(std::vector> *buffers)\n>>> {\n>>> - std::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n>>> - [](std::unique_ptr &b) { return b.get(); });\n>>> + for (auto const &buffer : *buffers)\n>>> + bufferMap_.emplace(id_.get(), buffer.get());\n>>> }\n>>>\n>>> -const std::vector &RPiStream::getBuffers() const\n>>> +const BufferMap &RPiStream::getBuffers() const\n>>> {\n>>> - return bufferList_;\n>>> + return bufferMap_;\n>>> }\n>>>\n>>> -int RPiStream::getBufferIndex(FrameBuffer *buffer) const\n>>> +int RPiStream::getBufferId(FrameBuffer *buffer) const\n>>> {\n>>> if (importOnly_)\n>>> return -1;\n>>>\n>>> - /* Find the buffer in the list, and return the index position. */\n>>> - auto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);\n>>> - if (it != bufferList_.end())\n>>> - return std::distance(bufferList_.begin(), it);\n>>> + /* Find the buffer in the map, and return the buffer id. */\n>>> + auto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),\n>>> + [&buffer](auto const &p) { return p.second == buffer; });\n>>>\n>>> - return -1;\n>>> + if (it == bufferMap_.end())\n>>> + return -1;\n>>> +\n>>> + return it->first;\n>>> }\n>>>\n>>> int RPiStream::prepareBuffers(unsigned int count)\n>>> @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count)\n>>> }\n>>>\n>>> /* We must import all internal/external exported buffers. */\n>>> - count = bufferList_.size();\n>>> + count = bufferMap_.size();\n>>> }\n>>>\n>>> return dev_->importBuffers(count);\n>>> @@ -139,6 +141,9 @@ void RPiStream::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>>> @@ -196,12 +201,13 @@ void RPiStream::clearBuffers()\n>>> availableBuffers_ = std::queue{};\n>>> requestBuffers_ = std::queue{};\n>>> internalBuffers_.clear();\n>>> - bufferList_.clear();\n>>> + bufferMap_.clear();\n>>> + id_.reset();\n>>> }\n>>>\n>>> int RPiStream::queueToDevice(FrameBuffer *buffer)\n>>> {\n>>> - LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferIndex(buffer)\n>>> + LOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n>>> << \" for \" << name_;\n>>>\n>>> int ret = dev_->queueBuffer(buffer);\n>>> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> index 959e9147..df986367 100644\n>>> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n>>> @@ -9,8 +9,10 @@\n>>>\n>>> #include \n>>> #include \n>>> +#include \n>>> #include \n>>>\n>>> +#include \n>>> #include \n>>>\n>>> #include \"libcamera/internal/v4l2_videodevice.h\"\n>>> @@ -19,6 +21,8 @@ namespace libcamera {\n>>>\n>>> namespace RPi {\n>>>\n>>> +using BufferMap = std::unordered_map;\n>>> +\n>>> /*\n>>> * Device stream abstraction for either an internal or external stream.\n>>> * Used for both Unicam and the ISP.\n>>> @@ -27,12 +31,13 @@ class RPiStream : public Stream\n>>> {\n>>> public:\n>>> RPiStream()\n>>> + : id_(RPiIpaMask::ID)\n>>> {\n>>> }\n>>>\n>>> RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n>>> : external_(false), importOnly_(importOnly), name_(name),\n>>> - dev_(std::make_unique(dev))\n>>> + dev_(std::make_unique(dev)), id_(RPiIpaMask::ID)\n>>> {\n>>> }\n>>>\n>>> @@ -45,8 +50,8 @@ public:\n>>> bool isExternal() const;\n>>>\n>>> void setExportedBuffers(std::vector> *buffers);\n>>> - const std::vector &getBuffers() const;\n>>> - int getBufferIndex(FrameBuffer *buffer) const;\n>>> + const BufferMap &getBuffers() const;\n>>> + int getBufferId(FrameBuffer *buffer) const;\n>>>\n>>> int prepareBuffers(unsigned int count);\n>>> int queueBuffer(FrameBuffer *buffer);\n>>> @@ -56,6 +61,44 @@ public:\n>>> void releaseBuffers();\n>>>\n>>> private:\n>>> + class IdGenerator\n>>> + {\n>>> + public:\n>>> + IdGenerator(int max)\n>>\n>> Should this provide a default value for max?\n>>\n>> Perhaps int max = 32?\n>>\n>> (See below).\n>>\n>>> + : max_(max), id_(0)\n>>> + {\n>>> + }\n>>> +\n>>> + int get()\n>>> + {\n>>> + 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(int id)\n>>> + {\n>>> + recycle_.push(id);\n>>> + }\n>>> +\n>>> + void reset()\n>>> + {\n>>> + id_ = 0;\n>>> + recycle_ = {};\n>>> + }\n>>> +\n>>> + private:\n>>> + int max_;\n>>> + int id_;\n>>> + std::queue recycle_;\n>>> + };\n>>> +\n>>> void clearBuffers();\n>>> int queueToDevice(FrameBuffer *buffer);\n>>>\n>>> @@ -74,8 +117,11 @@ private:\n>>> /* The actual device stream. */\n>>> std::unique_ptr dev_;\n>>>\n>>> + /* Tracks a unique id key for the bufferMap_ */\n>>> + IdGenerator id_;\n>>\n>> The only constructor for IdGenerator takes an int 'max' value, to assign\n>> to max_ which has the assertion check.\n>>\n>> That sounds useful to me for validation, but I can't see how this works\n>> (I'm probably missing something).\n>>\n>> With no default parameter, and nothing declared in the class definition\n>> as a default value, what is max_ set to ?\n>> (Some how I assume it would be zero?, but I would expect the compiler to\n>> complain too)\n> \n> Unless I misunderstood your comment above, the IdGenerator class is\n> initialised in the RPiStream contstructors:\n\nAha- that's what I'd missed, it *is* initialised ;-)\n\nI'm sorry for the noise.\n\n> \n> RPiStream()\n> : id_(RPiBufferMask::ID)\n> {\n> }\n> \n> RPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n> : external_(false), importOnly_(importOnly), name_(name),\n> dev_(std::make_unique(dev)), id_(RPiBufferMask::ID)\n> {\n> }\n> \n> IdGenerator::max_ takes the value RPiBufferMask::ID (0xffff) in this\n> case. Given no default value is used, the compiler ought to shout\n> loudly if I were to construct an IdGenerator instance without a max\n> parameter. Does that answer your query?\n\nYup - I had completely missed that the id_ was being intialised by the\nstream, and somehow had it in my head that it was using a default\ninitialiser.\n\nThanks,\n\nThis still stands,\n\nReviewed-by: Kieran Bingham \n\nSo I'll let Niklas take this on from here!\n\n--\nKieran\n\n\n\n> Regards,\n> Naush\n> \n> \n> \n>>\n>> Actually, I think what probably happens is it uses a 'default\n>> constructor' and leaves the id_ and max_ values uninitialised, so it's\n>> probably works by chance ?\n>>\n>> Other than that, I think this is neat. A quick and fast solution to the\n>> problem, and we never expect recycle queue to grow beyond the maximum\n>> quantity of unique buffers running in the system at any one time.\n>>\n>> With the max_ issue cleared:\n>>\n>> Reviewed-by: Kieran Bingham \n>>\n>> There shouldn't be any need to resend the whole series for this.\n>> If it does need fixing, either send an 8.1 of just this patch, or we can\n>> fix while applying perhaps, if it's just providing the default value.\n>> --\n>> Kieran\n>>\n>>\n>>\n>>\n>>\n>>> +\n>>> /* All frame buffers associated with this device stream. */\n>>> - std::vector bufferList_;\n>>> + BufferMap bufferMap_;\n>>>\n>>> /*\n>>> * List of frame buffers that we can use if none have been provided by\n>>>\n>>\n>> --\n>> Regards\n>> --\n>> Kieran","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 65924C3B5B\n\tfor ;\n\tFri, 18 Sep 2020 13:17:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EB89762FAB;\n\tFri, 18 Sep 2020 15:17:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A8E8962F4F\n\tfor ;\n\tFri, 18 Sep 2020 15:17:46 +0200 (CEST)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1F4932D7;\n\tFri, 18 Sep 2020 15:17:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"hyQhzk9N\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600435066;\n\tbh=fHOIziJiRb1/6b8lNpxVszYf5jnj4X18n2syiexISL8=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=hyQhzk9NhW59xW84KZ1rQ4gWhGWdqemZf6rb995uo6wI1dV1RldyL2G5NCHhjc5Cm\n\tmV6qC/FgnEDO7mLRGq3wype6hq1dGNIFFqsLGqCxZiDPu0MykiUc45vNwkamoxh6u3\n\tdBaZDzGQmamYwf+t+S8n8eibirtt1lV0X+VwW0wQ=","To":"Naushir Patuck ","References":"<20200918094233.5273-1-naush@raspberrypi.com>\n\t<20200918094233.5273-11-naush@raspberrypi.com>\n\t\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<422a2a84-fcb3-c36a-be2a-fa5d82b053c1@ideasonboard.com>","Date":"Fri, 18 Sep 2020 14:17:43 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","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","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12592,"web_url":"https://patchwork.libcamera.org/comment/12592/","msgid":"<20200919125808.GY1850958@oden.dyn.berto.se>","date":"2020-09-19T12:58:08","subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Naushir,\n\nThanks for your work.\n\nOn 2020-09-18 10:42:31 +0100, Naushir Patuck wrote:\n> By using a map container, we can easily insert/remove buffers from the\n> buffer list. This will be required to track buffers allocated externally\n> and passed to the pipeline handler through a Request.\n> \n> Replace the buffer index tracking with an id generated internally by the\n> stream object.\n> \n> Signed-off-by: Naushir Patuck \n\nReviewed-by: Niklas Söderlund \n\n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 31 +++++------\n> .../pipeline/raspberrypi/rpi_stream.cpp | 32 ++++++-----\n> .../pipeline/raspberrypi/rpi_stream.h | 54 +++++++++++++++++--\n> 3 files changed, 82 insertions(+), 35 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index 7d91188c..51544233 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -890,7 +890,6 @@ int PipelineHandlerRPi::queueAllBuffers(Camera *camera)\n> int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> {\n> \tRPiCameraData *data = cameraData(camera);\n> -\tunsigned int index;\n> \tint ret;\n> \n> \t/*\n> @@ -917,18 +916,14 @@ int PipelineHandlerRPi::prepareBuffers(Camera *camera)\n> \t * This will allow us to identify buffers passed between the pipeline\n> \t * handler and the IPA.\n> \t */\n> -\tindex = 0;\n> \tfor (auto const &b : data->isp_[Isp::Stats].getBuffers()) {\n> -\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | index,\n> -\t\t\t\t\t .planes = b->planes() });\n> -\t\tindex++;\n> +\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::STATS | b.first,\n> +\t\t\t\t\t .planes = b.second->planes() });\n> \t}\n> \n> -\tindex = 0;\n> \tfor (auto const &b : data->unicam_[Unicam::Embedded].getBuffers()) {\n> -\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | index,\n> -\t\t\t\t\t .planes = b->planes() });\n> -\t\tindex++;\n> +\t\tdata->ipaBuffers_.push_back({ .id = RPiIpaMask::EMBEDDED_DATA | b.first,\n> +\t\t\t\t\t .planes = b.second->planes() });\n> \t}\n> \n> \tdata->ipa_->mapBuffers(data->ipaBuffers_);\n> @@ -1127,7 +1122,7 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tfor (RPi::RPiStream &s : unicam_) {\n> -\t\tindex = s.getBufferIndex(buffer);\n> +\t\tindex = s.getBufferId(buffer);\n> \t\tif (index != -1) {\n> \t\t\tstream = &s;\n> \t\t\tbreak;\n> @@ -1178,7 +1173,7 @@ void RPiCameraData::ispInputDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tLOG(RPI, Debug) << \"Stream ISP Input buffer complete\"\n> -\t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferIndex(buffer)\n> +\t\t\t<< \", buffer id \" << unicam_[Unicam::Image].getBufferId(buffer)\n> \t\t\t<< \", timestamp: \" << buffer->metadata().timestamp;\n> \n> \t/* The ISP input buffer gets re-queued into Unicam. */\n> @@ -1195,7 +1190,7 @@ void RPiCameraData::ispOutputDequeue(FrameBuffer *buffer)\n> \t\treturn;\n> \n> \tfor (RPi::RPiStream &s : isp_) {\n> -\t\tindex = s.getBufferIndex(buffer);\n> +\t\tindex = s.getBufferId(buffer);\n> \t\tif (index != -1) {\n> \t\t\tstream = &s;\n> \t\t\tbreak;\n> @@ -1436,16 +1431,16 @@ void RPiCameraData::tryRunPipeline()\n> \t/* Set our state to say the pipeline is active. */\n> \tstate_ = State::Busy;\n> \n> -\tunsigned int bayerIndex = unicam_[Unicam::Image].getBufferIndex(bayerBuffer);\n> -\tunsigned int embeddedIndex = unicam_[Unicam::Embedded].getBufferIndex(embeddedBuffer);\n> +\tunsigned int bayerId = unicam_[Unicam::Image].getBufferId(bayerBuffer);\n> +\tunsigned int embeddedId = unicam_[Unicam::Embedded].getBufferId(embeddedBuffer);\n> \n> \tLOG(RPI, Debug) << \"Signalling RPI_IPA_EVENT_SIGNAL_ISP_PREPARE:\"\n> -\t\t\t<< \" Bayer buffer id: \" << bayerIndex\n> -\t\t\t<< \" Embedded buffer id: \" << embeddedIndex;\n> +\t\t\t<< \" Bayer buffer id: \" << bayerId\n> +\t\t\t<< \" Embedded buffer id: \" << embeddedId;\n> \n> \top.operation = RPI_IPA_EVENT_SIGNAL_ISP_PREPARE;\n> -\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedIndex,\n> -\t\t RPiIpaMask::BAYER_DATA | bayerIndex };\n> +\top.data = { RPiIpaMask::EMBEDDED_DATA | embeddedId,\n> +\t\t RPiIpaMask::BAYER_DATA | bayerId };\n> \tipa_->processEvent(op);\n> }\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> index 80170d64..aee0aa2d 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.cpp\n> @@ -44,26 +44,28 @@ bool RPiStream::isExternal() const\n> \n> void RPiStream::setExportedBuffers(std::vector> *buffers)\n> {\n> -\tstd::transform(buffers->begin(), buffers->end(), std::back_inserter(bufferList_),\n> -\t\t [](std::unique_ptr &b) { return b.get(); });\n> +\tfor (auto const &buffer : *buffers)\n> +\t\tbufferMap_.emplace(id_.get(), buffer.get());\n> }\n> \n> -const std::vector &RPiStream::getBuffers() const\n> +const BufferMap &RPiStream::getBuffers() const\n> {\n> -\treturn bufferList_;\n> +\treturn bufferMap_;\n> }\n> \n> -int RPiStream::getBufferIndex(FrameBuffer *buffer) const\n> +int RPiStream::getBufferId(FrameBuffer *buffer) const\n> {\n> \tif (importOnly_)\n> \t\treturn -1;\n> \n> -\t/* Find the buffer in the list, and return the index position. */\n> -\tauto it = std::find(bufferList_.begin(), bufferList_.end(), buffer);\n> -\tif (it != bufferList_.end())\n> -\t\treturn std::distance(bufferList_.begin(), it);\n> +\t/* Find the buffer in the map, and return the buffer id. */\n> +\tauto it = std::find_if(bufferMap_.begin(), bufferMap_.end(),\n> +\t\t\t [&buffer](auto const &p) { return p.second == buffer; });\n> \n> -\treturn -1;\n> +\tif (it == bufferMap_.end())\n> +\t\treturn -1;\n> +\n> +\treturn it->first;\n> }\n> \n> int RPiStream::prepareBuffers(unsigned int count)\n> @@ -86,7 +88,7 @@ int RPiStream::prepareBuffers(unsigned int count)\n> \t\t}\n> \n> \t\t/* We must import all internal/external exported buffers. */\n> -\t\tcount = bufferList_.size();\n> +\t\tcount = bufferMap_.size();\n> \t}\n> \n> \treturn dev_->importBuffers(count);\n> @@ -139,6 +141,9 @@ void RPiStream::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> @@ -196,12 +201,13 @@ void RPiStream::clearBuffers()\n> \tavailableBuffers_ = std::queue{};\n> \trequestBuffers_ = std::queue{};\n> \tinternalBuffers_.clear();\n> -\tbufferList_.clear();\n> +\tbufferMap_.clear();\n> +\tid_.reset();\n> }\n> \n> int RPiStream::queueToDevice(FrameBuffer *buffer)\n> {\n> -\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferIndex(buffer)\n> +\tLOG(RPISTREAM, Debug) << \"Queuing buffer \" << getBufferId(buffer)\n> \t\t\t << \" for \" << name_;\n> \n> \tint ret = dev_->queueBuffer(buffer);\n> diff --git a/src/libcamera/pipeline/raspberrypi/rpi_stream.h b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> index 959e9147..df986367 100644\n> --- a/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> +++ b/src/libcamera/pipeline/raspberrypi/rpi_stream.h\n> @@ -9,8 +9,10 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> +#include \n> #include \n> \n> #include \"libcamera/internal/v4l2_videodevice.h\"\n> @@ -19,6 +21,8 @@ namespace libcamera {\n> \n> namespace RPi {\n> \n> +using BufferMap = std::unordered_map;\n> +\n> /*\n> * Device stream abstraction for either an internal or external stream.\n> * Used for both Unicam and the ISP.\n> @@ -27,12 +31,13 @@ class RPiStream : public Stream\n> {\n> public:\n> \tRPiStream()\n> +\t\t: id_(RPiIpaMask::ID)\n> \t{\n> \t}\n> \n> \tRPiStream(const char *name, MediaEntity *dev, bool importOnly = false)\n> \t\t: external_(false), importOnly_(importOnly), name_(name),\n> -\t\t dev_(std::make_unique(dev))\n> +\t\t dev_(std::make_unique(dev)), id_(RPiIpaMask::ID)\n> \t{\n> \t}\n> \n> @@ -45,8 +50,8 @@ public:\n> \tbool isExternal() const;\n> \n> \tvoid setExportedBuffers(std::vector> *buffers);\n> -\tconst std::vector &getBuffers() const;\n> -\tint getBufferIndex(FrameBuffer *buffer) const;\n> +\tconst BufferMap &getBuffers() const;\n> +\tint getBufferId(FrameBuffer *buffer) const;\n> \n> \tint prepareBuffers(unsigned int count);\n> \tint queueBuffer(FrameBuffer *buffer);\n> @@ -56,6 +61,44 @@ public:\n> \tvoid releaseBuffers();\n> \n> private:\n> +\tclass IdGenerator\n> +\t{\n> +\tpublic:\n> +\t\tIdGenerator(int max)\n> +\t\t\t: max_(max), id_(0)\n> +\t\t{\n> +\t\t}\n> +\n> +\t\tint get()\n> +\t\t{\n> +\t\t\tint 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(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\tint max_;\n> +\t\tint id_;\n> +\t\tstd::queue recycle_;\n> +\t};\n> +\n> \tvoid clearBuffers();\n> \tint queueToDevice(FrameBuffer *buffer);\n> \n> @@ -74,8 +117,11 @@ private:\n> \t/* The actual device stream. */\n> \tstd::unique_ptr dev_;\n> \n> +\t/* Tracks a unique id key for the bufferMap_ */\n> +\tIdGenerator id_;\n> +\n> \t/* All frame buffers associated with this device stream. */\n> -\tstd::vector bufferList_;\n> +\tBufferMap bufferMap_;\n> \n> \t/*\n> \t * List of frame buffers that we can use if none have been provided by\n> -- \n> 2.25.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 03336BF01C\n\tfor ;\n\tSat, 19 Sep 2020 12:58:13 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 61E2B62FBF;\n\tSat, 19 Sep 2020 14:58:12 +0200 (CEST)","from mail-lj1-x22e.google.com (mail-lj1-x22e.google.com\n\t[IPv6:2a00:1450:4864:20::22e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CD6C460367\n\tfor ;\n\tSat, 19 Sep 2020 14:58:10 +0200 (CEST)","by mail-lj1-x22e.google.com with SMTP id s205so7263645lja.7\n\tfor ;\n\tSat, 19 Sep 2020 05:58:10 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tl8sm1267631lfc.124.2020.09.19.05.58.09\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tSat, 19 Sep 2020 05:58:09 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"nDWKFOxE\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=jRA9Q3JAidPGIe+LPDLFtOlnP9K9+l3ZBcjO6AfbBXU=;\n\tb=nDWKFOxEbnwskIOjYGOYZXh+YV5DUPNizcY42zU1de9R6bCBG1NlWj9P0ZU2MO8xb8\n\toTJ2NuurWsgnwzHpDtAVOVxgjVSQERtfeKZ/TtmAilRGDc3CSo3ch+phdqt0LMCQHJs0\n\t79njusMbBjX09Zo2DMey9UhrovxdHQSFXlao12Fgj4MqKBacaH55JVk6JNPk8f3RJjLE\n\trCHf4xuNOG5jvSZgxZ8o/rijGwrM37heVLKkBmr/MRLnxFcbbVtKBHL0N8tGz04n7Zh6\n\tLZJmc81k48UbwEOoUrEJNBV2wgbta0u1XJ2EGyKZ8KYXUhcXNjGDGgTcpQLjFGobOWRm\n\tiZ2Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=jRA9Q3JAidPGIe+LPDLFtOlnP9K9+l3ZBcjO6AfbBXU=;\n\tb=tgQfDfJatbJ8EqR/+MBtDQrRwxJ5xM7GoGb7IyPw8DZwaaChfYOE9kHIQjGYuegYES\n\tLRkC9FuI59TPZTY/SlHW2th5R2U3+NUHk69TXPLfBjpb+9Q+TrGkYYkEnV/z4HBcOXsj\n\tuyXIhq49p0fKq4w4LVU6nZVB4bOCGUB+/W91yWXp5zui6CDQMwRXWpvH2qTQQmgALxjo\n\t5jzPDGrffMFVYfLPRxPyJd62Aaj75sD0RFAZ/3ec2Ar6ynA6hG20LAWkbZy1eNUcyJB7\n\t56cr8rWQgTybrOJzfyiLgntI9Ikwwaa5KR7CGPurWRtOWSP1ryFcrYxjBeQFgyiGjciB\n\t2DFQ==","X-Gm-Message-State":"AOAM533x5mXooI9JgXt0WlNJQ/jaUqT5u/ROTYO5Pj5hF30r58JEbm8W\n\tcyX82z1zQJCyr80m7I/aPhCW4w==","X-Google-Smtp-Source":"ABdhPJyN2Y3oPBiAbJd31wCVTNpgcnyQ4Bug0KXSc4WCl1l2uoM0H9EbFxmclsN3vOkBemWf/+0Cyw==","X-Received":"by 2002:a2e:9988:: with SMTP id\n\tw8mr14451286lji.286.1600520290078; \n\tSat, 19 Sep 2020 05:58:10 -0700 (PDT)","Date":"Sat, 19 Sep 2020 14:58:08 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Naushir Patuck ","Message-ID":"<20200919125808.GY1850958@oden.dyn.berto.se>","References":"<20200918094233.5273-1-naush@raspberrypi.com>\n\t<20200918094233.5273-11-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200918094233.5273-11-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v8 10/12] pipeline: raspberrypi: Use\n\tan unordered_map for the stream buffer list","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]