[{"id":12672,"web_url":"https://patchwork.libcamera.org/comment/12672/","msgid":"<457135c0-d445-d0d3-3504-02f692310f95@ideasonboard.com>","date":"2020-09-23T12:28:26","subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 22/09/2020 10:47, Jacopo Mondi wrote:\n> After the Camera has been configured, walk the list of collected\n> CameraStream instances and allocate memory for the ones that needs\n> intermediate buffers reserved by the libcamera FrameBufferAllocator.\n> \n> Maintain a map between each Stream and a vector of pointers to the\n> associated buffers.\n> \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++\n> src/android/camera_device.h | 6 ++++++\n> 2 files changed, 42 insertions(+)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 42fb9ea4e113..f96ea7321a67 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \tstreams_.clear();\n> \tstreams_.reserve(stream_list->num_streams);\n> \tallocator_.clear();\n> +\tbufferPool_.clear();\n> \n> \t/* First handle all non-MJPEG streams. */\n> \tcamera3_stream_t *jpegStream = nullptr;\n> @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> \t\treturn ret;\n> \t}\n> \n> +\t/*\n> +\t * Now that the camera has been configured allocate buffers for\n> +\t * the streams that need memory reserved by libcamera.\n> +\t */\n> +\tfor (const CameraStream &cameraStream : streams_) {\n> +\t\tconst StreamConfiguration &cfg = config_->at(cameraStream.index());\n> +\t\tStream *stream = cfg.stream();\n> +\n> +\t\tif (cameraStream.type() != CameraStream::Type::Internal)\n> +\t\t\tcontinue;\n> +\n> +\t\tret = allocateBuffersPool(stream);\n> +\t\tif (ret) {\n> +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers for stream \"\n> +\t\t\t\t\t<< cameraStream.index();\n> +\t\t\treturn ret;\n> +\t\t}\n> +\t}\n> +\n\nSome how I thought I envisaged the ownership of these buffers being 'in'\nthe CameraStream class ... but I think I can see that it might be\ndifficult to map that.\n\nI'll try to think some more, and I'm sure things will be more clear as I\ngo through the other patches.\n\n\n> \treturn 0;\n> }\n> \n> @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n> \treturn new FrameBuffer(std::move(planes));\n> }\n> \n> +int CameraDevice::allocateBuffersPool(Stream *stream)\n\ns/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to\nhave a Buffers Pool.\n\n(you can have a pool of buffers, but not a buffers pool - It's just a\nbuffer pool).\n\n\n> +{\n> +\tint ret = allocator_.allocate(stream);\n> +\tif (ret < 0)\n> +\t\treturn ret;\n> +\n> +\t/*\n> +\t * Save a pointer to the reserved frame buffer for usage in\n> +\t * the HAL.\n> +\t */\n> +\tfor (const auto &frameBuffer : allocator_.buffers(stream))\n> +\t\tbufferPool_[stream].push_back(frameBuffer.get());\n> +\n> +\treturn 0;\n> +}\n> +\n> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> {\n> \tif (!camera3Request->num_output_buffers) {\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 84f636f7a93c..4cef34c01a49 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -166,6 +166,9 @@ protected:\n> \tstd::string logPrefix() const override;\n> \n> private:\n> +\tusing FrameBufferPool = std::map +\t\t\t\t\t std::vector>;\n> +\n\nPutting these 'in' the CameraStream would mean we don't need to keep a\nmapping of it?\n\nHrm ... time to read further forwards...\n\n\n> \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> \n> \tstruct Camera3RequestDescriptor {\n> @@ -194,6 +197,8 @@ private:\n> \n> \tstd::tuple calculateStaticMetadataSize();\n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> +\tint allocateBuffersPool(libcamera::Stream *stream);\n> +\n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> \tCameraMetadata *requestTemplatePreview();\n> @@ -208,6 +213,7 @@ private:\n> \tstd::shared_ptr camera_;\n> \tstd::unique_ptr config_;\n> \tlibcamera::FrameBufferAllocator allocator_;\n> +\tFrameBufferPool bufferPool_;\n> \n> \tCameraMetadata *staticMetadata_;\n> \tstd::map requestTemplates_;\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 32D7AC3B5C\n\tfor ;\n\tWed, 23 Sep 2020 12:28:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 00D7E60576;\n\tWed, 23 Sep 2020 14:28:32 +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 AD08160576\n\tfor ;\n\tWed, 23 Sep 2020 14:28:29 +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 2A5ED2FD;\n\tWed, 23 Sep 2020 14:28:29 +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=\"RiS5xWJj\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600864109;\n\tbh=GdFXuOui2RYa6PKYy/yeyiRK3Hy+HxHviWaW0Z0oKkM=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=RiS5xWJjjKyfsiXJkrMKgunS7aiIKASXHFsVIXnRNCUVqBefPrsTSu36Ked0RqgqM\n\tPGq0jCJHBEL4gCLsVmEQtGP5NIRS4I61lS9LXeUP393gb/V6BgU3Adv88n/fj19LWz\n\tCGkZwExTVmBq4Tr5mWQCf1h/uS4/gfet9LCQQD7c=","To":"Jacopo Mondi , libcamera-devel@lists.libcamera.org","References":"<20200922094738.5327-1-jacopo@jmondi.org>\n\t<20200922094738.5327-6-jacopo@jmondi.org>","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":"<457135c0-d445-d0d3-3504-02f692310f95@ideasonboard.com>","Date":"Wed, 23 Sep 2020 13:28:26 +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":"<20200922094738.5327-6-jacopo@jmondi.org>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","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":"hanlinchen@chromium.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12674,"web_url":"https://patchwork.libcamera.org/comment/12674/","msgid":"<20200923132134.l6cy27j56ap4dujt@uno.localdomain>","date":"2020-09-23T13:21:34","subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Wed, Sep 23, 2020 at 01:28:26PM +0100, Kieran Bingham wrote:\n> Hi Jacopo,\n>\n> On 22/09/2020 10:47, Jacopo Mondi wrote:\n> > After the Camera has been configured, walk the list of collected\n> > CameraStream instances and allocate memory for the ones that needs\n> > intermediate buffers reserved by the libcamera FrameBufferAllocator.\n> >\n> > Maintain a map between each Stream and a vector of pointers to the\n> > associated buffers.\n> >\n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++\n> > src/android/camera_device.h | 6 ++++++\n> > 2 files changed, 42 insertions(+)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 42fb9ea4e113..f96ea7321a67 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > \tstreams_.clear();\n> > \tstreams_.reserve(stream_list->num_streams);\n> > \tallocator_.clear();\n> > +\tbufferPool_.clear();\n> >\n> > \t/* First handle all non-MJPEG streams. */\n> > \tcamera3_stream_t *jpegStream = nullptr;\n> > @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n> > \t\treturn ret;\n> > \t}\n> >\n> > +\t/*\n> > +\t * Now that the camera has been configured allocate buffers for\n> > +\t * the streams that need memory reserved by libcamera.\n> > +\t */\n> > +\tfor (const CameraStream &cameraStream : streams_) {\n> > +\t\tconst StreamConfiguration &cfg = config_->at(cameraStream.index());\n> > +\t\tStream *stream = cfg.stream();\n> > +\n> > +\t\tif (cameraStream.type() != CameraStream::Type::Internal)\n> > +\t\t\tcontinue;\n> > +\n> > +\t\tret = allocateBuffersPool(stream);\n> > +\t\tif (ret) {\n> > +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers for stream \"\n> > +\t\t\t\t\t<< cameraStream.index();\n> > +\t\t\treturn ret;\n> > +\t\t}\n> > +\t}\n> > +\n>\n> Some how I thought I envisaged the ownership of these buffers being 'in'\n> the CameraStream class ... but I think I can see that it might be\n> difficult to map that.\n>\n> I'll try to think some more, and I'm sure things will be more clear as I\n> go through the other patches.\n\nI considered that. However, the allocator is global to the camera\ndevice which manages its creation and celeanup, so it kind of made\nsense to me to make the pool a camera device feature. I also would\nhave had to pass the allocator around, but that's probably not that\nbad, as it is required for allocation only.\n\n>\n>\n> > \treturn 0;\n> > }\n> >\n> > @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n> > \treturn new FrameBuffer(std::move(planes));\n> > }\n> >\n> > +int CameraDevice::allocateBuffersPool(Stream *stream)\n>\n> s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to\n> have a Buffers Pool.\n>\n> (you can have a pool of buffers, but not a buffers pool - It's just a\n> buffer pool).\n>\n\nAh thanks, I was actually not sure about this\n\n>\n> > +{\n> > +\tint ret = allocator_.allocate(stream);\n> > +\tif (ret < 0)\n> > +\t\treturn ret;\n> > +\n> > +\t/*\n> > +\t * Save a pointer to the reserved frame buffer for usage in\n> > +\t * the HAL.\n> > +\t */\n> > +\tfor (const auto &frameBuffer : allocator_.buffers(stream))\n> > +\t\tbufferPool_[stream].push_back(frameBuffer.get());\n> > +\n> > +\treturn 0;\n> > +}\n> > +\n> > int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n> > {\n> > \tif (!camera3Request->num_output_buffers) {\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 84f636f7a93c..4cef34c01a49 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -166,6 +166,9 @@ protected:\n> > \tstd::string logPrefix() const override;\n> >\n> > private:\n> > +\tusing FrameBufferPool = std::map > +\t\t\t\t\t std::vector>;\n> > +\n>\n> Putting these 'in' the CameraStream would mean we don't need to keep a\n> mapping of it?\n\nWe would save a map and store the vectors in each CameraStream, yes.\n\n>\n> Hrm ... time to read further forwards...\n>\n>\n> > \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> >\n> > \tstruct Camera3RequestDescriptor {\n> > @@ -194,6 +197,8 @@ private:\n> >\n> > \tstd::tuple calculateStaticMetadataSize();\n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > +\tint allocateBuffersPool(libcamera::Stream *stream);\n> > +\n> > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n> > \tCameraMetadata *requestTemplatePreview();\n> > @@ -208,6 +213,7 @@ private:\n> > \tstd::shared_ptr camera_;\n> > \tstd::unique_ptr config_;\n> > \tlibcamera::FrameBufferAllocator allocator_;\n> > +\tFrameBufferPool bufferPool_;\n> >\n> > \tCameraMetadata *staticMetadata_;\n> > \tstd::map requestTemplates_;\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 126C9C3B5C\n\tfor ;\n\tWed, 23 Sep 2020 13:17:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6AAEB62FDE;\n\tWed, 23 Sep 2020 15:17:44 +0200 (CEST)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 37A7760576\n\tfor ;\n\tWed, 23 Sep 2020 15:17:43 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 79B92240009;\n\tWed, 23 Sep 2020 13:17:42 +0000 (UTC)"],"Date":"Wed, 23 Sep 2020 15:21:34 +0200","From":"Jacopo Mondi ","To":"Kieran Bingham ","Message-ID":"<20200923132134.l6cy27j56ap4dujt@uno.localdomain>","References":"<20200922094738.5327-1-jacopo@jmondi.org>\n\t<20200922094738.5327-6-jacopo@jmondi.org>\n\t<457135c0-d445-d0d3-3504-02f692310f95@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<457135c0-d445-d0d3-3504-02f692310f95@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","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":"hanlinchen@chromium.org, libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12750,"web_url":"https://patchwork.libcamera.org/comment/12750/","msgid":"<59462df8-5bff-6c3a-9ccc-ae16e5576e0d@ideasonboard.com>","date":"2020-09-24T16:25:48","subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 23/09/2020 14:21, Jacopo Mondi wrote:\n> Hi Kieran\n> \n> On Wed, Sep 23, 2020 at 01:28:26PM +0100, Kieran Bingham wrote:\n>> Hi Jacopo,\n>>\n>> On 22/09/2020 10:47, Jacopo Mondi wrote:\n>>> After the Camera has been configured, walk the list of collected\n>>> CameraStream instances and allocate memory for the ones that needs\n>>> intermediate buffers reserved by the libcamera FrameBufferAllocator.\n>>>\n>>> Maintain a map between each Stream and a vector of pointers to the\n>>> associated buffers.\n>>>\n>>> Reviewed-by: Niklas Söderlund \n>>> Signed-off-by: Jacopo Mondi \n>>> ---\n>>> src/android/camera_device.cpp | 36 +++++++++++++++++++++++++++++++++++\n>>> src/android/camera_device.h | 6 ++++++\n>>> 2 files changed, 42 insertions(+)\n>>>\n>>> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n>>> index 42fb9ea4e113..f96ea7321a67 100644\n>>> --- a/src/android/camera_device.cpp\n>>> +++ b/src/android/camera_device.cpp\n>>> @@ -1189,6 +1189,7 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>>> \tstreams_.clear();\n>>> \tstreams_.reserve(stream_list->num_streams);\n>>> \tallocator_.clear();\n>>> +\tbufferPool_.clear();\n>>>\n>>> \t/* First handle all non-MJPEG streams. */\n>>> \tcamera3_stream_t *jpegStream = nullptr;\n>>> @@ -1336,6 +1337,25 @@ int CameraDevice::configureStreams(camera3_stream_configuration_t *stream_list)\n>>> \t\treturn ret;\n>>> \t}\n>>>\n>>> +\t/*\n>>> +\t * Now that the camera has been configured allocate buffers for\n>>> +\t * the streams that need memory reserved by libcamera.\n>>> +\t */\n>>> +\tfor (const CameraStream &cameraStream : streams_) {\n>>> +\t\tconst StreamConfiguration &cfg = config_->at(cameraStream.index());\n>>> +\t\tStream *stream = cfg.stream();\n>>> +\n>>> +\t\tif (cameraStream.type() != CameraStream::Type::Internal)\n>>> +\t\t\tcontinue;\n>>> +\n>>> +\t\tret = allocateBuffersPool(stream);\n>>> +\t\tif (ret) {\n>>> +\t\t\tLOG(HAL, Error) << \"Failed to allocate buffers for stream \"\n>>> +\t\t\t\t\t<< cameraStream.index();\n>>> +\t\t\treturn ret;\n>>> +\t\t}\n>>> +\t}\n>>> +\n>>\n>> Some how I thought I envisaged the ownership of these buffers being 'in'\n>> the CameraStream class ... but I think I can see that it might be\n>> difficult to map that.\n>>\n>> I'll try to think some more, and I'm sure things will be more clear as I\n>> go through the other patches.\n> \n> I considered that. However, the allocator is global to the camera\n> device which manages its creation and celeanup, so it kind of made\n> sense to me to make the pool a camera device feature. I also would\n> have had to pass the allocator around, but that's probably not that\n> bad, as it is required for allocation only.\n\nNot necessarily, The code here could push the buffers directly into the\nCameraStream buffer pool or buffer vector or such (transferring ownership).\n\nOr the future bufferPool could also store a pointer to the Allocator\nindeed, to be able to return / free them when destroyed etc..\n\n>>\n>>> \treturn 0;\n>>> }\n>>>\n>>> @@ -1369,6 +1389,22 @@ FrameBuffer *CameraDevice::createFrameBuffer(const buffer_handle_t camera3buffer\n>>> \treturn new FrameBuffer(std::move(planes));\n>>> }\n>>>\n>>> +int CameraDevice::allocateBuffersPool(Stream *stream)\n>>\n>> s/Buffers/Buffer/ ... Pool is already plural, so it sounds really odd to\n>> have a Buffers Pool.\n>>\n>> (you can have a pool of buffers, but not a buffers pool - It's just a\n>> buffer pool).\n>>\n> \n> Ah thanks, I was actually not sure about this\n> \n>>\n>>> +{\n>>> +\tint ret = allocator_.allocate(stream);\n>>> +\tif (ret < 0)\n>>> +\t\treturn ret;\n>>> +\n>>> +\t/*\n>>> +\t * Save a pointer to the reserved frame buffer for usage in\n>>> +\t * the HAL.\n>>> +\t */\n>>> +\tfor (const auto &frameBuffer : allocator_.buffers(stream))\n>>> +\t\tbufferPool_[stream].push_back(frameBuffer.get());\n>>> +\n>>> +\treturn 0;\n>>> +}\n>>> +\n>>> int CameraDevice::processCaptureRequest(camera3_capture_request_t *camera3Request)\n>>> {\n>>> \tif (!camera3Request->num_output_buffers) {\n>>> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n>>> index 84f636f7a93c..4cef34c01a49 100644\n>>> --- a/src/android/camera_device.h\n>>> +++ b/src/android/camera_device.h\n>>> @@ -166,6 +166,9 @@ protected:\n>>> \tstd::string logPrefix() const override;\n>>>\n>>> private:\n>>> +\tusing FrameBufferPool = std::map>> +\t\t\t\t\t std::vector>;\n>>> +\n>>\n>> Putting these 'in' the CameraStream would mean we don't need to keep a\n>> mapping of it?\n> \n> We would save a map and store the vectors in each CameraStream, yes.\n> \n>>\n>> Hrm ... time to read further forwards...\n>>\n>>\n>>> \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n>>>\n>>> \tstruct Camera3RequestDescriptor {\n>>> @@ -194,6 +197,8 @@ private:\n>>>\n>>> \tstd::tuple calculateStaticMetadataSize();\n>>> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n>>> +\tint allocateBuffersPool(libcamera::Stream *stream);\n>>> +\n>>> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n>>> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n>>> \tCameraMetadata *requestTemplatePreview();\n>>> @@ -208,6 +213,7 @@ private:\n>>> \tstd::shared_ptr camera_;\n>>> \tstd::unique_ptr config_;\n>>> \tlibcamera::FrameBufferAllocator allocator_;\n>>> +\tFrameBufferPool bufferPool_;\n>>>\n>>> \tCameraMetadata *staticMetadata_;\n>>> \tstd::map requestTemplates_;\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 5EBECC3B5B\n\tfor ;\n\tThu, 24 Sep 2020 16:25:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D96CF62FE3;\n\tThu, 24 Sep 2020 18:25:53 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 61D7260363\n\tfor ;\n\tThu, 24 Sep 2020 18:25:52 +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 A7C242FD;\n\tThu, 24 Sep 2020 18:25:50 +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=\"Gz2RPTpg\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1600964750;\n\tbh=q8JpEolptpqtPCu+Ndnaz6jZ4CRpjOOwjH3An0WC1h0=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=Gz2RPTpgNQkoR+Zy96o8h6emLh+YJ7c+EFPYukT9Z0Wd2PM/VsXbSzkOyv0Xq3dEe\n\tHLJ7GTrHVLFN/HbKyB76I/VQuL7wkif+kKDEFz2/2wxEfE6mKqP2tY5vk0pI9eOETh\n\tp+TZ5kVxPUZSsdT2gdI91lrAwkn8/SAKR/Rt70SA=","To":"Jacopo Mondi ","References":"<20200922094738.5327-1-jacopo@jmondi.org>\n\t<20200922094738.5327-6-jacopo@jmondi.org>\n\t<457135c0-d445-d0d3-3504-02f692310f95@ideasonboard.com>\n\t<20200923132134.l6cy27j56ap4dujt@uno.localdomain>","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":"<59462df8-5bff-6c3a-9ccc-ae16e5576e0d@ideasonboard.com>","Date":"Thu, 24 Sep 2020 17:25:48 +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":"<20200923132134.l6cy27j56ap4dujt@uno.localdomain>","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH v3 5/8] android: camera_device:\n\tAllocate buffer pools","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":"hanlinchen@chromium.org, libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]