[{"id":12923,"web_url":"https://patchwork.libcamera.org/comment/12923/","msgid":"<20201002003525.GO3722@pendragon.ideasonboard.com>","date":"2020-10-02T00:35:25","subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Wed, Sep 30, 2020 at 03:27:05PM +0200, Jacopo Mondi wrote:\n> Add two methods to the CameraDevice class to retrieve and return\n> frame buffers associated to a stream from the memory pool reserved\n> in libcamera.\n> \n> Protect accessing the vector of FrameBuffer pointers with a\n> per-pool mutex in the get and return buffer methods.\n> \n> Reviewed-by: Niklas Söderlund \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_device.cpp | 34 +++++++++++++++++++++++++++++++++-\n> src/android/camera_device.h | 11 +++++++++--\n> 2 files changed, 42 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> index 07041dd916d5..2ebc3fcc336e 100644\n> --- a/src/android/camera_device.cpp\n> +++ b/src/android/camera_device.cpp\n> @@ -8,6 +8,7 @@\n> #include \"camera_device.h\"\n> #include \"camera_ops.h\"\n> \n> +#include \n\nYou have that in camera_device.h already.\n\n> #include \n> #include \n> #include \n> @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(Stream *stream)\n> \t * the HAL.\n> \t */\n> \tfor (const auto &frameBuffer : allocator_.buffers(stream))\n> -\t\tbufferPool_[stream].push_back(frameBuffer.get());\n> +\t\tbufferPool_[stream].buffers.push_back(frameBuffer.get());\n> \n> \treturn 0;\n> }\n> \n> +libcamera::FrameBuffer *CameraDevice::getBuffer(libcamera::Stream *stream)\n> +{\n> +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> +\t\treturn nullptr;\n> +\n> +\tBufferPool *pool = &bufferPool_[stream];\n\n\tauto iter = bufferPool_.find(stream);\n\tif (iter == bufferPool_.end())\n\t\treturn nullptr;\n\n\tBufferPool *pool = &iter->second;\n\nto avoid a double lookup. Same below.\n\n> +\tstd::lock_guard locker(pool->mutex);\n> +\n> +\tif (pool->buffers.empty()) {\n> +\t\tLOG(HAL, Error) << \"Buffer underrun\";\n> +\t\treturn nullptr;\n> +\t}\n> +\n> +\tFrameBuffer *buffer = pool->buffers.front();\n> +\tpool->buffers.erase(pool->buffers.begin());\n\nErasing an element at the front isn't nice, as all the elements will\nhave to move. Wouldn't it be better to use a std::queue ? Alternatively\nyou can use use back() and pop_back(), as we don't need to cycle through\nbuffers in order.\n\n> +\n> +\treturn buffer;\n> +}\n> +\n> +void CameraDevice::returnBuffer(libcamera::Stream *stream,\n> +\t\t\t\tlibcamera::FrameBuffer *buffer)\n> +{\n> +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> +\t\treturn;\n> +\n> +\tBufferPool *pool = &bufferPool_[stream];\n> +\tstd::lock_guard locker(pool->mutex);\n> +\n> +\tpool->buffers.push_back(buffer);\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 c46610725e12..c91de367d7bd 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -9,6 +9,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> #include \n> \n> @@ -166,8 +167,11 @@ protected:\n> \tstd::string logPrefix() const override;\n> \n> private:\n> -\tusing FrameBufferPool = std::map -\t\t\t\t\t std::vector>;\n> +\tstruct BufferPool {\n> +\t\tstd::mutex mutex;\n> +\t\tstd::vector buffers;\n> +\t};\n> +\tusing FrameBufferPool = std::map;\n\nI think I'd squash this patch with the previous one.\n\nAnd wouldn't it be best to store the pool in the CameraStream class ?\nThe callers of allocateBufferPool(), getBuffer() and returnBuffer() all\nhave a pointer to the CameraStream.\n\n> \n> \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> \n> @@ -198,6 +202,9 @@ private:\n> \tstd::tuple calculateStaticMetadataSize();\n> \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> \tint allocateBufferPool(libcamera::Stream *stream);\n> +\tlibcamera::FrameBuffer *getBuffer(libcamera::Stream *stream);\n> +\tvoid returnBuffer(libcamera::Stream *stream,\n> +\t\t\t libcamera::FrameBuffer *buffer);\n> \n> \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);","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 E01BDC3B5B\n\tfor ;\n\tFri, 2 Oct 2020 00:36:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7195E6239C;\n\tFri, 2 Oct 2020 02:36:05 +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 D22FD60576\n\tfor ;\n\tFri, 2 Oct 2020 02:36:03 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3CD0660;\n\tFri, 2 Oct 2020 02:36:03 +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=\"l6izbYzW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601598963;\n\tbh=iHP3TbJWCign5wj0uU9T9vmXsvrNGM2HWk5lscDtlj4=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=l6izbYzWoo7sjrH2FvCnccil08SMqvl6ozIike2ckyGQcbufxU4NSpmth89cYYUkk\n\t1wS68L6eEWvL4XNk+NPXfr8mJTxtML5riVxgWSPZUOM9nvZ4f40+2OI3q9Tg+2K1HH\n\tx5hbcRv36mXemM5jkBpJ3YzMDe9FVwqHyChWz+WE=","Date":"Fri, 2 Oct 2020 03:35:25 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201002003525.GO3722@pendragon.ideasonboard.com>","References":"<20200930132707.19367-1-jacopo@jmondi.org>\n\t<20200930132707.19367-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200930132707.19367-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","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":12945,"web_url":"https://patchwork.libcamera.org/comment/12945/","msgid":"<20201002144622.6awgi3bliepkwqdh@uno.localdomain>","date":"2020-10-02T14:46:22","subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Fri, Oct 02, 2020 at 03:35:25AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Wed, Sep 30, 2020 at 03:27:05PM +0200, Jacopo Mondi wrote:\n> > Add two methods to the CameraDevice class to retrieve and return\n> > frame buffers associated to a stream from the memory pool reserved\n> > in libcamera.\n> >\n> > Protect accessing the vector of FrameBuffer pointers with a\n> > per-pool mutex in the get and return buffer methods.\n> >\n> > Reviewed-by: Niklas Söderlund \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_device.cpp | 34 +++++++++++++++++++++++++++++++++-\n> > src/android/camera_device.h | 11 +++++++++--\n> > 2 files changed, 42 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > index 07041dd916d5..2ebc3fcc336e 100644\n> > --- a/src/android/camera_device.cpp\n> > +++ b/src/android/camera_device.cpp\n> > @@ -8,6 +8,7 @@\n> > #include \"camera_device.h\"\n> > #include \"camera_ops.h\"\n> >\n> > +#include \n>\n> You have that in camera_device.h already.\n>\n\nFor sake of discussion: I tend to include all the headers I need in\nthe .cpp files too, mostly for clarity, as that's after all a no-op as\nall headers are guarded. Is this a bad practice I should avoid ?\n\n> > #include \n> > #include \n> > #include \n> > @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(Stream *stream)\n> > \t * the HAL.\n> > \t */\n> > \tfor (const auto &frameBuffer : allocator_.buffers(stream))\n> > -\t\tbufferPool_[stream].push_back(frameBuffer.get());\n> > +\t\tbufferPool_[stream].buffers.push_back(frameBuffer.get());\n> >\n> > \treturn 0;\n> > }\n> >\n> > +libcamera::FrameBuffer *CameraDevice::getBuffer(libcamera::Stream *stream)\n> > +{\n> > +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> > +\t\treturn nullptr;\n> > +\n> > +\tBufferPool *pool = &bufferPool_[stream];\n>\n> \tauto iter = bufferPool_.find(stream);\n> \tif (iter == bufferPool_.end())\n> \t\treturn nullptr;\n>\n> \tBufferPool *pool = &iter->second;\n>\n> to avoid a double lookup. Same below.\n>\n\nAck!\n\n> > +\tstd::lock_guard locker(pool->mutex);\n> > +\n> > +\tif (pool->buffers.empty()) {\n> > +\t\tLOG(HAL, Error) << \"Buffer underrun\";\n> > +\t\treturn nullptr;\n> > +\t}\n> > +\n> > +\tFrameBuffer *buffer = pool->buffers.front();\n> > +\tpool->buffers.erase(pool->buffers.begin());\n>\n> Erasing an element at the front isn't nice, as all the elements will\n> have to move. Wouldn't it be better to use a std::queue ? Alternatively\n> you can use use back() and pop_back(), as we don't need to cycle through\n> buffers in order.\n>\n\nI started with a queue, but seems to be quite expensive in terms of\nmemory. Hiro suggested to use the last entry of the vector, but we\nmight end up re-using the same buffer, potentially only a single one,\nand this seems to be a bad idea even if it doesn't have any practical\neffect.\n\nWhat's best? A queue, or potentially re-use the same buffer over and over ?\n\n> > +\n> > +\treturn buffer;\n> > +}\n> > +\n> > +void CameraDevice::returnBuffer(libcamera::Stream *stream,\n> > +\t\t\t\tlibcamera::FrameBuffer *buffer)\n> > +{\n> > +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> > +\t\treturn;\n> > +\n> > +\tBufferPool *pool = &bufferPool_[stream];\n> > +\tstd::lock_guard locker(pool->mutex);\n> > +\n> > +\tpool->buffers.push_back(buffer);\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 c46610725e12..c91de367d7bd 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -9,6 +9,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> > #include \n> >\n> > @@ -166,8 +167,11 @@ protected:\n> > \tstd::string logPrefix() const override;\n> >\n> > private:\n> > -\tusing FrameBufferPool = std::map > -\t\t\t\t\t std::vector>;\n> > +\tstruct BufferPool {\n> > +\t\tstd::mutex mutex;\n> > +\t\tstd::vector buffers;\n> > +\t};\n> > +\tusing FrameBufferPool = std::map;\n>\n> I think I'd squash this patch with the previous one.\n>\n\nI was afraid it would grow very large.. but Kieran had the same\ncomment on this type changing in two consecutive patches...\n\n> And wouldn't it be best to store the pool in the CameraStream class ?\n> The callers of allocateBufferPool(), getBuffer() and returnBuffer() all\n> have a pointer to the CameraStream.\n\nThat's the long discussion I had with Kieran. I agree it would be\nbetter placed in the CameraStream, but I think this would require a\nmajor re-design of that class, that needs to be broken out from\nthe camera_device.cpp file and made copy-constructable. Also adding\nyet another parameter to the constructor is not nice, it already has 5\nof them.\n\nIf I could tie a CameraConfiguration to the Camera that created it,\nfrom there get the StreamConfiguration at the right index, I could\nsave a few parameters now that I look at that again.\n\nI will have another try maybe, but I already think this is piling too\nmany things on top of this series and CameraStream will require a\nmajor re-design anyway. In case it gets too complex, I'll record with\na todo that we have to move allocation and framebuffer access there.\n\nThanks\n j\n\n>\n> >\n> > \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> >\n> > @@ -198,6 +202,9 @@ private:\n> > \tstd::tuple calculateStaticMetadataSize();\n> > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > \tint allocateBufferPool(libcamera::Stream *stream);\n> > +\tlibcamera::FrameBuffer *getBuffer(libcamera::Stream *stream);\n> > +\tvoid returnBuffer(libcamera::Stream *stream,\n> > +\t\t\t libcamera::FrameBuffer *buffer);\n> >\n> > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 987D1C3B5B\n\tfor ;\n\tFri, 2 Oct 2020 14:42:27 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2715B63BA7;\n\tFri, 2 Oct 2020 16:42:27 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id BF32563B59\n\tfor ;\n\tFri, 2 Oct 2020 16:42:25 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 277D3C0008;\n\tFri, 2 Oct 2020 14:42:24 +0000 (UTC)"],"X-Originating-IP":"93.34.118.233","Date":"Fri, 2 Oct 2020 16:46:22 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20201002144622.6awgi3bliepkwqdh@uno.localdomain>","References":"<20200930132707.19367-1-jacopo@jmondi.org>\n\t<20200930132707.19367-8-jacopo@jmondi.org>\n\t<20201002003525.GO3722@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002003525.GO3722@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","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":12951,"web_url":"https://patchwork.libcamera.org/comment/12951/","msgid":"<20201003035959.GA8848@pendragon.ideasonboard.com>","date":"2020-10-03T03:59:59","subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Oct 02, 2020 at 04:46:22PM +0200, Jacopo Mondi wrote:\n> On Fri, Oct 02, 2020 at 03:35:25AM +0300, Laurent Pinchart wrote:\n> > On Wed, Sep 30, 2020 at 03:27:05PM +0200, Jacopo Mondi wrote:\n> > > Add two methods to the CameraDevice class to retrieve and return\n> > > frame buffers associated to a stream from the memory pool reserved\n> > > in libcamera.\n> > >\n> > > Protect accessing the vector of FrameBuffer pointers with a\n> > > per-pool mutex in the get and return buffer methods.\n> > >\n> > > Reviewed-by: Niklas Söderlund \n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_device.cpp | 34 +++++++++++++++++++++++++++++++++-\n> > > src/android/camera_device.h | 11 +++++++++--\n> > > 2 files changed, 42 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/android/camera_device.cpp b/src/android/camera_device.cpp\n> > > index 07041dd916d5..2ebc3fcc336e 100644\n> > > --- a/src/android/camera_device.cpp\n> > > +++ b/src/android/camera_device.cpp\n> > > @@ -8,6 +8,7 @@\n> > > #include \"camera_device.h\"\n> > > #include \"camera_ops.h\"\n> > >\n> > > +#include \n> >\n> > You have that in camera_device.h already.\n> \n> For sake of discussion: I tend to include all the headers I need in\n> the .cpp files too, mostly for clarity, as that's after all a no-op as\n> all headers are guarded. Is this a bad practice I should avoid ?\n\nMaking sure we don't depend on implicit includes is a very good\npractice. I usually don't include the headers that are included by the\nheader corresponding to a source file though. The rationale is that if\nclass Foo embeds a Bar, foo.h will have to include bar.h, so usage of\nBar in foo.cpp comes for free.\n\n> > > #include \n> > > #include \n> > > #include \n> > > @@ -1402,11 +1403,42 @@ int CameraDevice::allocateBufferPool(Stream *stream)\n> > > \t * the HAL.\n> > > \t */\n> > > \tfor (const auto &frameBuffer : allocator_.buffers(stream))\n> > > -\t\tbufferPool_[stream].push_back(frameBuffer.get());\n> > > +\t\tbufferPool_[stream].buffers.push_back(frameBuffer.get());\n> > >\n> > > \treturn 0;\n> > > }\n> > >\n> > > +libcamera::FrameBuffer *CameraDevice::getBuffer(libcamera::Stream *stream)\n> > > +{\n> > > +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\tBufferPool *pool = &bufferPool_[stream];\n> >\n> > \tauto iter = bufferPool_.find(stream);\n> > \tif (iter == bufferPool_.end())\n> > \t\treturn nullptr;\n> >\n> > \tBufferPool *pool = &iter->second;\n> >\n> > to avoid a double lookup. Same below.\n> \n> Ack!\n> \n> > > +\tstd::lock_guard locker(pool->mutex);\n> > > +\n> > > +\tif (pool->buffers.empty()) {\n> > > +\t\tLOG(HAL, Error) << \"Buffer underrun\";\n> > > +\t\treturn nullptr;\n> > > +\t}\n> > > +\n> > > +\tFrameBuffer *buffer = pool->buffers.front();\n> > > +\tpool->buffers.erase(pool->buffers.begin());\n> >\n> > Erasing an element at the front isn't nice, as all the elements will\n> > have to move. Wouldn't it be better to use a std::queue ? Alternatively\n> > you can use use back() and pop_back(), as we don't need to cycle through\n> > buffers in order.\n> \n> I started with a queue, but seems to be quite expensive in terms of\n> memory. Hiro suggested to use the last entry of the vector, but we\n> might end up re-using the same buffer, potentially only a single one,\n> and this seems to be a bad idea even if it doesn't have any practical\n> effect.\n> \n> What's best? A queue, or potentially re-use the same buffer over and over ?\n\nReusing same buffer shouldn't be an issue. I don't think it will cause\nthat particular piece of RAM to wear out faster :-)\n\n> > > +\n> > > +\treturn buffer;\n> > > +}\n> > > +\n> > > +void CameraDevice::returnBuffer(libcamera::Stream *stream,\n> > > +\t\t\t\tlibcamera::FrameBuffer *buffer)\n> > > +{\n> > > +\tif (bufferPool_.find(stream) == bufferPool_.end())\n> > > +\t\treturn;\n> > > +\n> > > +\tBufferPool *pool = &bufferPool_[stream];\n> > > +\tstd::lock_guard locker(pool->mutex);\n> > > +\n> > > +\tpool->buffers.push_back(buffer);\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 c46610725e12..c91de367d7bd 100644\n> > > --- a/src/android/camera_device.h\n> > > +++ b/src/android/camera_device.h\n> > > @@ -9,6 +9,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > > #include \n> > >\n> > > @@ -166,8 +167,11 @@ protected:\n> > > \tstd::string logPrefix() const override;\n> > >\n> > > private:\n> > > -\tusing FrameBufferPool = std::map > > -\t\t\t\t\t std::vector>;\n> > > +\tstruct BufferPool {\n> > > +\t\tstd::mutex mutex;\n> > > +\t\tstd::vector buffers;\n> > > +\t};\n> > > +\tusing FrameBufferPool = std::map;\n> >\n> > I think I'd squash this patch with the previous one.\n> \n> I was afraid it would grow very large.. but Kieran had the same\n> comment on this type changing in two consecutive patches...\n> \n> > And wouldn't it be best to store the pool in the CameraStream class ?\n> > The callers of allocateBufferPool(), getBuffer() and returnBuffer() all\n> > have a pointer to the CameraStream.\n> \n> That's the long discussion I had with Kieran. I agree it would be\n> better placed in the CameraStream, but I think this would require a\n> major re-design of that class, that needs to be broken out from\n> the camera_device.cpp file and made copy-constructable. Also adding\n> yet another parameter to the constructor is not nice, it already has 5\n> of them.\n> \n> If I could tie a CameraConfiguration to the Camera that created it,\n> from there get the StreamConfiguration at the right index, I could\n> save a few parameters now that I look at that again.\n> \n> I will have another try maybe, but I already think this is piling too\n> many things on top of this series and CameraStream will require a\n> major re-design anyway. In case it gets too complex, I'll record with\n> a todo that we have to move allocation and framebuffer access there.\n\nWorks for me, thanks.\n\n> > >\n> > > \tCameraDevice(unsigned int id, const std::shared_ptr &camera);\n> > >\n> > > @@ -198,6 +202,9 @@ private:\n> > > \tstd::tuple calculateStaticMetadataSize();\n> > > \tlibcamera::FrameBuffer *createFrameBuffer(const buffer_handle_t camera3buffer);\n> > > \tint allocateBufferPool(libcamera::Stream *stream);\n> > > +\tlibcamera::FrameBuffer *getBuffer(libcamera::Stream *stream);\n> > > +\tvoid returnBuffer(libcamera::Stream *stream,\n> > > +\t\t\t libcamera::FrameBuffer *buffer);\n> > >\n> > > \tvoid notifyShutter(uint32_t frameNumber, uint64_t timestamp);\n> > > \tvoid notifyError(uint32_t frameNumber, camera3_stream_t *stream);","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 8672DC3B5B\n\tfor ;\n\tSat, 3 Oct 2020 04:00:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id DC49B622B5;\n\tSat, 3 Oct 2020 06:00:39 +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 E78BA609FF\n\tfor ;\n\tSat, 3 Oct 2020 06:00:37 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 584892A2;\n\tSat, 3 Oct 2020 06:00:37 +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=\"jItQA7Zq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1601697637;\n\tbh=GZhF0O8ztQxwDvWiV4y5tmKRG0bgoPsN8nwOeMEBvFk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jItQA7ZqPCGggvV1r2COdWGMSsjpl96rB1yhqRt7gFk+Cymn6x4hjpxtPUgC+kjPe\n\tvxKZdYaTx9ll2Q++IAIURZNjAORYmTziJoxUBTYW2jjJbapktj9jXwwjtWSn/OBlKf\n\tMmaV7txm2RGjmkDtOGYlf7f5SW0s6KJhrRC7CwZc=","Date":"Sat, 3 Oct 2020 06:59:59 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"<20201003035959.GA8848@pendragon.ideasonboard.com>","References":"<20200930132707.19367-1-jacopo@jmondi.org>\n\t<20200930132707.19367-8-jacopo@jmondi.org>\n\t<20201002003525.GO3722@pendragon.ideasonboard.com>\n\t<20201002144622.6awgi3bliepkwqdh@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20201002144622.6awgi3bliepkwqdh@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v4 7/9] android: camera_device: Add\n\tmethods to get and return buffers","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\" "}}]