[{"id":723,"web_url":"https://patchwork.libcamera.org/comment/723/","msgid":"<20190131183435.esne5uyw3uunp5rf@uno.localdomain>","date":"2019-01-31T18:34:35","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran,\n   thanks for the patch\n\nOn Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:\n> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>\n> Provide classes that represent frame buffers and pools of frame buffers.\n>\n> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>\n> ---\n> Kieran\n>  - Fix checkstyle.py diff\n> ---\n>  include/libcamera/buffer.h    |  55 ++++++++++++++++++\n>  include/libcamera/libcamera.h |   1 +\n>  include/libcamera/meson.build |   1 +\n>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++\n>  src/libcamera/meson.build     |   1 +\n>  5 files changed, 160 insertions(+)\n>  create mode 100644 include/libcamera/buffer.h\n>  create mode 100644 src/libcamera/buffer.cpp\n>\n> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n> new file mode 100644\n> index 000000000000..97c8025d9e77\n> --- /dev/null\n> +++ b/include/libcamera/buffer.h\n> @@ -0,0 +1,55 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n\n2019 ?\n\n> + *\n> + * buffer.h - Buffer handling\n> + */\n> +#ifndef __LIBCAMERA_BUFFER_H__\n> +#define __LIBCAMERA_BUFFER_H__\n> +\n> +#include <vector>\n> +\n> +namespace libcamera {\n> +\n> +class Buffer\n> +{\n> +public:\n> +\tBuffer();\n> +\t~Buffer();\n> +\n> +\tint dmabuf() const { return fd_; };\n> +\tint setDmabuf(int fd);\n> +\n> +\tint mmap();\n> +\tvoid munmap();\n> +\n> +private:\n> +\tint fd_;\n> +};\n> +\n> +class BufferPool\n> +{\n> +public:\n> +\tenum Memory {\n> +\t\tInternal,\n> +\t\tExternal,\n> +\t};\n> +\n> +\tBufferPool(Memory memory);\n> +\tvirtual ~BufferPool();\n> +\n> +\tint allocate(unsigned int count);\n> +\tvoid free();\n> +\n> +\tunsigned int count() const { return buffers_.size(); };\n> +\n> +private:\n> +\tvirtual int allocateMemory() = 0;\n> +\n> +\tBufferPool::Memory memory_;\n> +\tstd::vector<Buffer *> buffers_;\n> +};\n> +\n> +} /* namespace libcamera */\n> +\n> +#endif /* __LIBCAMERA_BUFFER_H__ */\n> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n> index c0511cf6d662..6619e556ebbd 100644\n> --- a/include/libcamera/libcamera.h\n> +++ b/include/libcamera/libcamera.h\n> @@ -7,6 +7,7 @@\n>  #ifndef __LIBCAMERA_LIBCAMERA_H__\n>  #define __LIBCAMERA_LIBCAMERA_H__\n>\n> +#include <libcamera/buffer.h>\n>  #include <libcamera/camera.h>\n>  #include <libcamera/camera_manager.h>\n>  #include <libcamera/event_dispatcher.h>\n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index d7cb55ba4a49..8fcbbf7892ea 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n>  libcamera_api = files([\n> +    'buffer.h',\n>      'camera.h',\n>      'camera_manager.h',\n>      'event_dispatcher.h',\n> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> new file mode 100644\n> index 000000000000..6dfebfc6bb28\n> --- /dev/null\n> +++ b/src/libcamera/buffer.cpp\n> @@ -0,0 +1,102 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2018, Google Inc.\n\n2019 ?\n\n> + *\n> + * buffer.cpp - Buffer handling\n> + */\n> +\n> +#include <errno.h>\n> +#include <unistd.h>\n> +\n> +#include <libcamera/buffer.h>\n> +\n> +/**\n> + * \\file buffer.h\n> + * \\brief Buffer handling\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\class Buffer\n> + * \\brief A memory buffer to store a frame\n> + *\n> + * The Buffer class represents a memory buffer used to store a frame.\n> + */\n> +Buffer::Buffer()\n> +\t: fd_(-1)\n> +{\n> +}\n> +\n> +Buffer::~Buffer()\n> +{\n> +\tif (fd_ != -1)\n> +\t\tclose(fd_);\n> +}\n> +\n> +/**\n> + * \\fn Buffer::dmabuf()\n> + * \\brief Get the dmabuf file handle backing the buffer\n> + */\n> +\n> +/**\n> + * \\brief Set the dmabuf file handle backing the buffer\n> + *\n> + * The \\a fd dmabuf file handle is duplicated and stored.\n> + */\n> +int Buffer::setDmabuf(int fd)\n> +{\n> +\tif (fd_ != -1) {\n> +\t\tclose(fd_);\n> +\t\tfd_ = -1;\n> +\t}\n> +\n> +\tif (fd != -1)\n> +\t\treturn 0;\n\nI didn't get this... Shouldn't we exit if (fd == -1) ?\n\n> +\n> +\tfd_ = dup(fd);\n> +\tif (fd_ == -1)\n\nA more approriate error handling sequence might be considered here.\n\n> +\t\treturn -errno;\n> +\n> +\treturn 0;\n> +}\n> +\n> +/**\n> + * \\class BufferPool\n> + * \\brief A pool of buffers\n> + */\n> +BufferPool::BufferPool(BufferPool::Memory memory)\n> +\t: memory_(memory)\n> +{\n> +}\n> +\n> +BufferPool::~BufferPool()\n> +{\n> +\tfree();\n> +}\n> +\n> +int BufferPool::allocate(unsigned int count)\n> +{\n> +\tfor (unsigned int i = 0; i < count; ++i) {\n> +\t\tBuffer *buffer = new Buffer();\n> +\t\tbuffers_.push_back(buffer);\n> +\t}\n> +\n> +\tif (memory_ == Memory::Internal) {\n\nThe Internal vs External distinction matches the \"exporter\" vs\n\"importer\" one for the DMABUF use case, or has other purposes?\nShouldn't we use then use Importer vs Exporter not to add another\nterm?\n\n> +\t\tint ret = allocateMemory();\n\nThis is left to subclasses, which I do expect to use MMAP or DMABUF,\nand each subclass will have its own specific fields. I wonder if it is\nworth to provide a base class like Buffer which is not pure virtual.\n\nIn example, importer vs exporter does not apply to mmap buffers, am I\nright? Also, why would a DMABUF implementation expose a mmap() and\nunmap() method?\n\nHave you considered having Buffer as pure virtual, and use it keep the\nvector<Buffers *> in the pool, but move most of the details and\nimplementations in subclasses?\n\n> +\t\tif (ret < 0)\n> +\t\t\treturn ret;\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n> +void BufferPool::free()\n> +{\n> +\tfor (Buffer *buffer : buffers_)\n> +\t\tdelete buffer;\n> +\n> +\tbuffers_.clear();\n\nI think buffers_ will be destroyed anyway, so you could drop this.\n\nThanks\n  j\n\n> +}\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index f9f25c0ecf15..b5b25965fd12 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -1,4 +1,5 @@\n>  libcamera_sources = files([\n> +    'buffer.cpp',\n>      'camera.cpp',\n>      'camera_manager.cpp',\n>      'device_enumerator.cpp',\n> --\n> 2.19.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":"<jacopo@jmondi.org>","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3EA060C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 31 Jan 2019 19:34:18 +0100 (CET)","from uno.localdomain (host-109-88-34-5.dynamic.voo.be\n\t[109.88.34.5]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id 71D8D100003;\n\tThu, 31 Jan 2019 18:34:18 +0000 (UTC)"],"Date":"Thu, 31 Jan 2019 19:34:35 +0100","From":"Jacopo Mondi <jacopo@jmondi.org>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190131183435.esne5uyw3uunp5rf@uno.localdomain>","References":"<20190129135357.32339-1-kieran.bingham@ideasonboard.com>\n\t<20190129135357.32339-2-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"x52meptybl3ejlyf\"","Content-Disposition":"inline","In-Reply-To":"<20190129135357.32339-2-kieran.bingham@ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 31 Jan 2019 18:34:19 -0000"}},{"id":724,"web_url":"https://patchwork.libcamera.org/comment/724/","msgid":"<a7c59141-533a-d19c-d0f3-c3bf4f4e12de@ideasonboard.com>","date":"2019-01-31T19:00:31","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn 31/01/2019 18:34, Jacopo Mondi wrote:\n> Hi Kieran,\n>    thanks for the patch\n> \n> On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:\n>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>\n>> Provide classes that represent frame buffers and pools of frame buffers.\n>>\n>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>\n>> ---\n>> Kieran\n>>  - Fix checkstyle.py diff\n>> ---\n>>  include/libcamera/buffer.h    |  55 ++++++++++++++++++\n>>  include/libcamera/libcamera.h |   1 +\n>>  include/libcamera/meson.build |   1 +\n>>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++\n>>  src/libcamera/meson.build     |   1 +\n>>  5 files changed, 160 insertions(+)\n>>  create mode 100644 include/libcamera/buffer.h\n>>  create mode 100644 src/libcamera/buffer.cpp\n>>\n>> diff --git a/include/libcamera/buffer.h b/include/libcamera/buffer.h\n>> new file mode 100644\n>> index 000000000000..97c8025d9e77\n>> --- /dev/null\n>> +++ b/include/libcamera/buffer.h\n>> @@ -0,0 +1,55 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n> \n> 2019 ?\n\nLaurent created this patch, in 2018. I've just been adding on top of his\npatch. (except for one small issue I must have seen with checkstyle).\n\nI guess I can update the year too ...\n\n\n>> + *\n>> + * buffer.h - Buffer handling\n>> + */\n>> +#ifndef __LIBCAMERA_BUFFER_H__\n>> +#define __LIBCAMERA_BUFFER_H__\n>> +\n>> +#include <vector>\n>> +\n>> +namespace libcamera {\n>> +\n>> +class Buffer\n>> +{\n>> +public:\n>> +\tBuffer();\n>> +\t~Buffer();\n>> +\n>> +\tint dmabuf() const { return fd_; };\n>> +\tint setDmabuf(int fd);\n>> +\n>> +\tint mmap();\n>> +\tvoid munmap();\n>> +\n>> +private:\n>> +\tint fd_;\n>> +};\n>> +\n>> +class BufferPool\n>> +{\n>> +public:\n>> +\tenum Memory {\n>> +\t\tInternal,\n>> +\t\tExternal,\n>> +\t};\n>> +\n>> +\tBufferPool(Memory memory);\n>> +\tvirtual ~BufferPool();\n>> +\n>> +\tint allocate(unsigned int count);\n>> +\tvoid free();\n>> +\n>> +\tunsigned int count() const { return buffers_.size(); };\n>> +\n>> +private:\n>> +\tvirtual int allocateMemory() = 0;\n>> +\n>> +\tBufferPool::Memory memory_;\n>> +\tstd::vector<Buffer *> buffers_;\n>> +};\n>> +\n>> +} /* namespace libcamera */\n>> +\n>> +#endif /* __LIBCAMERA_BUFFER_H__ */\n>> diff --git a/include/libcamera/libcamera.h b/include/libcamera/libcamera.h\n>> index c0511cf6d662..6619e556ebbd 100644\n>> --- a/include/libcamera/libcamera.h\n>> +++ b/include/libcamera/libcamera.h\n>> @@ -7,6 +7,7 @@\n>>  #ifndef __LIBCAMERA_LIBCAMERA_H__\n>>  #define __LIBCAMERA_LIBCAMERA_H__\n>>\n>> +#include <libcamera/buffer.h>\n>>  #include <libcamera/camera.h>\n>>  #include <libcamera/camera_manager.h>\n>>  #include <libcamera/event_dispatcher.h>\n>> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n>> index d7cb55ba4a49..8fcbbf7892ea 100644\n>> --- a/include/libcamera/meson.build\n>> +++ b/include/libcamera/meson.build\n>> @@ -1,4 +1,5 @@\n>>  libcamera_api = files([\n>> +    'buffer.h',\n>>      'camera.h',\n>>      'camera_manager.h',\n>>      'event_dispatcher.h',\n>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>> new file mode 100644\n>> index 000000000000..6dfebfc6bb28\n>> --- /dev/null\n>> +++ b/src/libcamera/buffer.cpp\n>> @@ -0,0 +1,102 @@\n>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n>> +/*\n>> + * Copyright (C) 2018, Google Inc.\n> \n> 2019 ?\n> \n>> + *\n>> + * buffer.cpp - Buffer handling\n>> + */\n>> +\n>> +#include <errno.h>\n>> +#include <unistd.h>\n>> +\n>> +#include <libcamera/buffer.h>\n>> +\n>> +/**\n>> + * \\file buffer.h\n>> + * \\brief Buffer handling\n>> + */\n>> +\n>> +namespace libcamera {\n>> +\n>> +/**\n>> + * \\class Buffer\n>> + * \\brief A memory buffer to store a frame\n>> + *\n>> + * The Buffer class represents a memory buffer used to store a frame.\n>> + */\n>> +Buffer::Buffer()\n>> +\t: fd_(-1)\n>> +{\n>> +}\n>> +\n>> +Buffer::~Buffer()\n>> +{\n>> +\tif (fd_ != -1)\n>> +\t\tclose(fd_);\n>> +}\n>> +\n>> +/**\n>> + * \\fn Buffer::dmabuf()\n>> + * \\brief Get the dmabuf file handle backing the buffer\n>> + */\n>> +\n>> +/**\n>> + * \\brief Set the dmabuf file handle backing the buffer\n>> + *\n>> + * The \\a fd dmabuf file handle is duplicated and stored.\n>> + */\n>> +int Buffer::setDmabuf(int fd)\n>> +{\n>> +\tif (fd_ != -1) {\n>> +\t\tclose(fd_);\n>> +\t\tfd_ = -1;\n>> +\t}\n>> +\n>> +\tif (fd != -1)\n>> +\t\treturn 0;\n> \n> I didn't get this... Shouldn't we exit if (fd == -1) ?\n\nah yes - I spotted this before - I thought I fixed it.\nLooks like I must have dropped/lost the fix.\n\nI'll update now so it doesn't get lost.\n\n\n> \n>> +\n>> +\tfd_ = dup(fd);\n>> +\tif (fd_ == -1)\n> \n> A more approriate error handling sequence might be considered here.\n> \n>> +\t\treturn -errno;\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +/**\n>> + * \\class BufferPool\n>> + * \\brief A pool of buffers\n>> + */\n>> +BufferPool::BufferPool(BufferPool::Memory memory)\n>> +\t: memory_(memory)\n>> +{\n>> +}\n>> +\n>> +BufferPool::~BufferPool()\n>> +{\n>> +\tfree();\n>> +}\n>> +\n>> +int BufferPool::allocate(unsigned int count)\n>> +{\n>> +\tfor (unsigned int i = 0; i < count; ++i) {\n>> +\t\tBuffer *buffer = new Buffer();\n>> +\t\tbuffers_.push_back(buffer);\n>> +\t}\n>> +\n>> +\tif (memory_ == Memory::Internal) {\n> \n> The Internal vs External distinction matches the \"exporter\" vs\n> \"importer\" one for the DMABUF use case, or has other purposes?\n> Shouldn't we use then use Importer vs Exporter not to add another\n> term?\n\nPlaying this through - I'm becoming less convinced that we need a\nMemory::Internal.\n\nI'd like to be able restrict the creation of pools to the 'Device' which\nwill own them. And at that point, it can store a pointer of that device.\n\nThen - if Pool.device == Device Buffers are internal.\nElse Buffers are external.\n\n\nI haven't yet seen what benefit setting the memory_ provides - but I'm\nhaving issues creating the BufferPools anyway. It's a WIP. (see below)\n\n\n\n>> +\t\tint ret = allocateMemory();\n> \n> This is left to subclasses, which I do expect to use MMAP or DMABUF,\n> and each subclass will have its own specific fields. I wonder if it is\n> worth to provide a base class like Buffer which is not pure virtual.\n> \n> In example, importer vs exporter does not apply to mmap buffers, am I\n> right? Also, why would a DMABUF implementation expose a mmap() and\n> unmap() method?\n> \n> Have you considered having Buffer as pure virtual, and use it keep the\n> vector<Buffers *> in the pool, but move most of the details and\n> implementations in subclasses?\n\nI think that's sort of the way that my current branch is heading. I'm\ntrying to get the V4L2Device to create the objects directly. Having some\nissues with casting and storage though.\n\nI have a V4L2Buffer which allows instantiation of the Buffer object from\nthe V4L2 device.\n\nBut if I create a V4L2BufferPool - I'm hitting issues with how to\nexpress a vector of Buffer objects - that are also (or rather are in\nfact) V4L2Buffer objects.\n\n\nIn summary - I believe I agree with you here. The interface at Buffer.h\nshould be just that. The interface exposed to the application, with as\ngeneric implementation as possible.\n\n\n\n> \n>> +\t\tif (ret < 0)\n>> +\t\t\treturn ret;\n>> +\t}\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> +void BufferPool::free()\n>> +{\n>> +\tfor (Buffer *buffer : buffers_)\n>> +\t\tdelete buffer;\n>> +\n>> +\tbuffers_.clear();\n> \n> I think buffers_ will be destroyed anyway, so you could drop this.\n> \n\nThis function is named free(). It's not a destructor. So at this stage,\nI think this .clear() needs to be kept.\n\nGranted, it's only use is in the destructor - but if someone were to\ncall free() directly - it should clear() the vector.\n\n\n\n> Thanks\n>   j\n> \n>> +}\n>> +\n>> +} /* namespace libcamera */\n>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>> index f9f25c0ecf15..b5b25965fd12 100644\n>> --- a/src/libcamera/meson.build\n>> +++ b/src/libcamera/meson.build\n>> @@ -1,4 +1,5 @@\n>>  libcamera_sources = files([\n>> +    'buffer.cpp',\n>>      'camera.cpp',\n>>      'camera_manager.cpp',\n>>      'device_enumerator.cpp',\n>> --\n>> 2.19.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":"<kieran.bingham@ideasonboard.com>","Received":["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 BDA4160C6A\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tThu, 31 Jan 2019 20:00:34 +0100 (CET)","from [10.10.152.147] (218.182-78-194.adsl-static.isp.belgacom.be\n\t[194.78.182.218])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2EA3441;\n\tThu, 31 Jan 2019 20:00:34 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548961234;\n\tbh=sv0YKvtGDL+Z7i7SeyVZoWEQ5KXOm5fzO2T0I/jS6bE=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=JgDxMUdOATu3cyiWVPXpw54K5WRi3/3xtom7xrfdXshTtizd20zOERbqSYU61Cx6a\n\tKgp2fzqVc6UpQU5F4iEs8sriq/MNysROzBW1S+lZ9+z60lnM54ZYGXmZC8fxBxxhvi\n\tT80LSn3ed5ERBBfpGnp/lOPahb0dnOQ/6+bDX+dE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Jacopo Mondi <jacopo@jmondi.org>","Cc":"LibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190129135357.32339-1-kieran.bingham@ideasonboard.com>\n\t<20190129135357.32339-2-kieran.bingham@ideasonboard.com>\n\t<20190131183435.esne5uyw3uunp5rf@uno.localdomain>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<a7c59141-533a-d19c-d0f3-c3bf4f4e12de@ideasonboard.com>","Date":"Thu, 31 Jan 2019 19:00:31 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190131183435.esne5uyw3uunp5rf@uno.localdomain>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Thu, 31 Jan 2019 19:00:34 -0000"}},{"id":730,"web_url":"https://patchwork.libcamera.org/comment/730/","msgid":"<20190201005235.GC23060@pendragon.ideasonboard.com>","date":"2019-02-01T00:52:35","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Thu, Jan 31, 2019 at 07:00:31PM +0000, Kieran Bingham wrote:\n> On 31/01/2019 18:34, Jacopo Mondi wrote:\n> > On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:\n> >> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >>\n> >> Provide classes that represent frame buffers and pools of frame buffers.\n> >>\n> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> >>\n> >> ---\n> >> Kieran\n> >>  - Fix checkstyle.py diff\n> >> ---\n> >>  include/libcamera/buffer.h    |  55 ++++++++++++++++++\n> >>  include/libcamera/libcamera.h |   1 +\n> >>  include/libcamera/meson.build |   1 +\n> >>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++\n> >>  src/libcamera/meson.build     |   1 +\n> >>  5 files changed, 160 insertions(+)\n> >>  create mode 100644 include/libcamera/buffer.h\n> >>  create mode 100644 src/libcamera/buffer.cpp\n\n[snip]\n\n> >> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n> >> new file mode 100644\n> >> index 000000000000..6dfebfc6bb28\n> >> --- /dev/null\n> >> +++ b/src/libcamera/buffer.cpp\n\n[snip]\n\n> >> +int BufferPool::allocate(unsigned int count)\n> >> +{\n> >> +\tfor (unsigned int i = 0; i < count; ++i) {\n> >> +\t\tBuffer *buffer = new Buffer();\n> >> +\t\tbuffers_.push_back(buffer);\n> >> +\t}\n> >> +\n> >> +\tif (memory_ == Memory::Internal) {\n> > \n> > The Internal vs External distinction matches the \"exporter\" vs\n> > \"importer\" one for the DMABUF use case, or has other purposes?\n> > Shouldn't we use then use Importer vs Exporter not to add another\n> > term?\n> \n> Playing this through - I'm becoming less convinced that we need a\n> Memory::Internal.\n> \n> I'd like to be able restrict the creation of pools to the 'Device' which\n> will own them. And at that point, it can store a pointer of that device.\n> \n> Then - if Pool.device == Device Buffers are internal.\n> Else Buffers are external.\n> \n> \n> I haven't yet seen what benefit setting the memory_ provides - but I'm\n> having issues creating the BufferPools anyway. It's a WIP. (see below)\n\nLooking at this from an application point of view, the application needs\na pool of buffers per stream. The pool shall be provided by libcamera,\nnot by the application, but the memory backing the buffers could be\nprovided by either libcamera (internal allocation) or the application\n(external allocation). We thus need a way for an application to request\na pool with a given number of buffers, and to specify whether buffers\nshould be allocated by libcamera (in which case the pool will be\nassociated with a V4L2 device) or will be provided externally (in which\ncase the pool doesn't need to be associated with a V4L2 device).\n\nYou will obviously need, in both cases, to request V4L2 buffers of type\nMMAP or DMABUF, but that should be decoupled from the buffers pool\nitself. Please keep in mind that the buffers pool class will be used\ninternally to share buffers between devices (for instance between the\nIPU3 CIO2 and ImgU), where you will have a single buffers pool but two\nV4L2 buffers queues. The pool is thus distinct from the queues.\n\nThis being said, when an application requests a buffers pool with\ninternal allocation, the allocation of buffers will be delegated to the\nV4L2 device, so you may certainly implement a V4L2BufferPool class for\nthis case.\n\n> >> +\t\tint ret = allocateMemory();\n> > \n> > This is left to subclasses, which I do expect to use MMAP or DMABUF,\n> > and each subclass will have its own specific fields. I wonder if it is\n> > worth to provide a base class like Buffer which is not pure virtual.\n> > \n> > In example, importer vs exporter does not apply to mmap buffers, am I\n> > right? Also, why would a DMABUF implementation expose a mmap() and\n> > unmap() method?\n> > \n> > Have you considered having Buffer as pure virtual, and use it keep the\n> > vector<Buffers *> in the pool, but move most of the details and\n> > implementations in subclasses?\n> \n> I think that's sort of the way that my current branch is heading. I'm\n> trying to get the V4L2Device to create the objects directly. Having some\n> issues with casting and storage though.\n> \n> I have a V4L2Buffer which allows instantiation of the Buffer object from\n> the V4L2 device.\n> \n> But if I create a V4L2BufferPool - I'm hitting issues with how to\n> express a vector of Buffer objects - that are also (or rather are in\n> fact) V4L2Buffer objects.\n> \n> \n> In summary - I believe I agree with you here. The interface at Buffer.h\n> should be just that. The interface exposed to the application, with as\n> generic implementation as possible.\n> \n> >> +\t\tif (ret < 0)\n> >> +\t\t\treturn ret;\n> >> +\t}\n> >> +\n> >> +\treturn 0;\n> >> +}\n> >> +\n> >> +void BufferPool::free()\n> >> +{\n> >> +\tfor (Buffer *buffer : buffers_)\n> >> +\t\tdelete buffer;\n> >> +\n> >> +\tbuffers_.clear();\n> > \n> > I think buffers_ will be destroyed anyway, so you could drop this.\n> > \n> \n> This function is named free(). It's not a destructor. So at this stage,\n> I think this .clear() needs to be kept.\n> \n> Granted, it's only use is in the destructor - but if someone were to\n> call free() directly - it should clear() the vector.\n> \n> >> +}\n> >> +\n> >> +} /* namespace libcamera */\n> >> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> >> index f9f25c0ecf15..b5b25965fd12 100644\n> >> --- a/src/libcamera/meson.build\n> >> +++ b/src/libcamera/meson.build\n> >> @@ -1,4 +1,5 @@\n> >>  libcamera_sources = files([\n> >> +    'buffer.cpp',\n> >>      'camera.cpp',\n> >>      'camera_manager.cpp',\n> >>      'device_enumerator.cpp',","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 65DE660C78\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Feb 2019 01:52:39 +0100 (CET)","from pendragon.ideasonboard.com (85-76-34-136-nat.elisa-mobile.fi\n\t[85.76.34.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2361D41;\n\tFri,  1 Feb 2019 01:52:37 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548982358;\n\tbh=xekCMRpK+n+zyF2cksTmB7516jLEmaBuAgWXS6gECGk=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=t/hmjnsgy8a7DdT/EydldyLPwr4MSlCTexLuPs8SGUbDYpxTSEMLqMa1ilUUHm7JM\n\tBdEbmeBYgIdmBF5ia/kg+BCaInZn+TD0L6F6DQF7iIfroQuNdPrNncnBohS/lZOomG\n\tBjBmA7AnXVHbzxqOhwmNHJdpiCi2VZf0qq+V0ALg=","Date":"Fri, 1 Feb 2019 02:52:35 +0200","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo@jmondi.org>,\n\tLibCamera Devel <libcamera-devel@lists.libcamera.org>","Message-ID":"<20190201005235.GC23060@pendragon.ideasonboard.com>","References":"<20190129135357.32339-1-kieran.bingham@ideasonboard.com>\n\t<20190129135357.32339-2-kieran.bingham@ideasonboard.com>\n\t<20190131183435.esne5uyw3uunp5rf@uno.localdomain>\n\t<a7c59141-533a-d19c-d0f3-c3bf4f4e12de@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<a7c59141-533a-d19c-d0f3-c3bf4f4e12de@ideasonboard.com>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 Feb 2019 00:52:39 -0000"}},{"id":738,"web_url":"https://patchwork.libcamera.org/comment/738/","msgid":"<a3fcf475-beee-3782-5564-adb319188fce@ideasonboard.com>","date":"2019-02-01T15:15:47","subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\n\nOn 01/02/2019 01:52, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> On Thu, Jan 31, 2019 at 07:00:31PM +0000, Kieran Bingham wrote:\n>> On 31/01/2019 18:34, Jacopo Mondi wrote:\n>>> On Tue, Jan 29, 2019 at 01:53:54PM +0000, Kieran Bingham wrote:\n>>>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>>>\n>>>> Provide classes that represent frame buffers and pools of frame buffers.\n>>>>\n>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n>>>> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n>>>>\n>>>> ---\n>>>> Kieran\n>>>>  - Fix checkstyle.py diff\n>>>> ---\n>>>>  include/libcamera/buffer.h    |  55 ++++++++++++++++++\n>>>>  include/libcamera/libcamera.h |   1 +\n>>>>  include/libcamera/meson.build |   1 +\n>>>>  src/libcamera/buffer.cpp      | 102 ++++++++++++++++++++++++++++++++++\n>>>>  src/libcamera/meson.build     |   1 +\n>>>>  5 files changed, 160 insertions(+)\n>>>>  create mode 100644 include/libcamera/buffer.h\n>>>>  create mode 100644 src/libcamera/buffer.cpp\n> \n> [snip]\n> \n>>>> diff --git a/src/libcamera/buffer.cpp b/src/libcamera/buffer.cpp\n>>>> new file mode 100644\n>>>> index 000000000000..6dfebfc6bb28\n>>>> --- /dev/null\n>>>> +++ b/src/libcamera/buffer.cpp\n> \n> [snip]\n> \n>>>> +int BufferPool::allocate(unsigned int count)\n>>>> +{\n>>>> +\tfor (unsigned int i = 0; i < count; ++i) {\n>>>> +\t\tBuffer *buffer = new Buffer();\n>>>> +\t\tbuffers_.push_back(buffer);\n>>>> +\t}\n>>>> +\n>>>> +\tif (memory_ == Memory::Internal) {\n>>>\n>>> The Internal vs External distinction matches the \"exporter\" vs\n>>> \"importer\" one for the DMABUF use case, or has other purposes?\n>>> Shouldn't we use then use Importer vs Exporter not to add another\n>>> term?\n>>\n>> Playing this through - I'm becoming less convinced that we need a\n>> Memory::Internal.\n>>\n>> I'd like to be able restrict the creation of pools to the 'Device' which\n>> will own them. And at that point, it can store a pointer of that device.\n>>\n>> Then - if Pool.device == Device Buffers are internal.\n>> Else Buffers are external.\n>>\n>>\n>> I haven't yet seen what benefit setting the memory_ provides - but I'm\n>> having issues creating the BufferPools anyway. It's a WIP. (see below)\n> \n> Looking at this from an application point of view, the application needs\n> a pool of buffers per stream. The pool shall be provided by libcamera,\n> not by the application, but the memory backing the buffers could be\n> provided by either libcamera (internal allocation) or the application\n> (external allocation). We thus need a way for an application to request\n> a pool with a given number of buffers, and to specify whether buffers\n> should be allocated by libcamera (in which case the pool will be\n> associated with a V4L2 device) or will be provided externally (in which\n> case the pool doesn't need to be associated with a V4L2 device).\n> \n> You will obviously need, in both cases, to request V4L2 buffers of type\n> MMAP or DMABUF, but that should be decoupled from the buffers pool\n> itself. Please keep in mind that the buffers pool class will be used\n> internally to share buffers between devices (for instance between the\n> IPU3 CIO2 and ImgU), where you will have a single buffers pool but two\n> V4L2 buffers queues. The pool is thus distinct from the queues.\n> \n> This being said, when an application requests a buffers pool with\n> internal allocation, the allocation of buffers will be delegated to the\n> V4L2 device, so you may certainly implement a V4L2BufferPool class for\n> this case.\n\n\n\nAh yes - I was getting distracted by internal sharing of buffers between\nV4L2Devices inside the library (i.e. CIO2->IMGU)\n\nProviding buffers externally will need some design to be able to import\nthe buffers.\n\nDo we need that /now/ or is this a feature for later?\n\n\nAt the moment, my branch was progressing towards the V4L2 device being\nable to operate in either two modes.\n\nEither it creates the buffers - and exports a BufferPool, or it imports\na BufferPool. ...\n\n\n>>>> +\t\tint ret = allocateMemory();\n>>>\n>>> This is left to subclasses, which I do expect to use MMAP or DMABUF,\n>>> and each subclass will have its own specific fields. I wonder if it is\n>>> worth to provide a base class like Buffer which is not pure virtual.\n>>>\n>>> In example, importer vs exporter does not apply to mmap buffers, am I\n>>> right? Also, why would a DMABUF implementation expose a mmap() and\n>>> unmap() method?\n>>>\n>>> Have you considered having Buffer as pure virtual, and use it keep the\n>>> vector<Buffers *> in the pool, but move most of the details and\n>>> implementations in subclasses?\n>>\n>> I think that's sort of the way that my current branch is heading. I'm\n>> trying to get the V4L2Device to create the objects directly. Having some\n>> issues with casting and storage though.\n>>\n>> I have a V4L2Buffer which allows instantiation of the Buffer object from\n>> the V4L2 device.\n>>\n>> But if I create a V4L2BufferPool - I'm hitting issues with how to\n>> express a vector of Buffer objects - that are also (or rather are in\n>> fact) V4L2Buffer objects.\n>>\n>>\n>> In summary - I believe I agree with you here. The interface at Buffer.h\n>> should be just that. The interface exposed to the application, with as\n>> generic implementation as possible.\n>>\n>>>> +\t\tif (ret < 0)\n>>>> +\t\t\treturn ret;\n>>>> +\t}\n>>>> +\n>>>> +\treturn 0;\n>>>> +}\n>>>> +\n>>>> +void BufferPool::free()\n>>>> +{\n>>>> +\tfor (Buffer *buffer : buffers_)\n>>>> +\t\tdelete buffer;\n>>>> +\n>>>> +\tbuffers_.clear();\n>>>\n>>> I think buffers_ will be destroyed anyway, so you could drop this.\n>>>\n>>\n>> This function is named free(). It's not a destructor. So at this stage,\n>> I think this .clear() needs to be kept.\n>>\n>> Granted, it's only use is in the destructor - but if someone were to\n>> call free() directly - it should clear() the vector.\n>>\n>>>> +}\n>>>> +\n>>>> +} /* namespace libcamera */\n>>>> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n>>>> index f9f25c0ecf15..b5b25965fd12 100644\n>>>> --- a/src/libcamera/meson.build\n>>>> +++ b/src/libcamera/meson.build\n>>>> @@ -1,4 +1,5 @@\n>>>>  libcamera_sources = files([\n>>>> +    'buffer.cpp',\n>>>>      'camera.cpp',\n>>>>      'camera_manager.cpp',\n>>>>      'device_enumerator.cpp',\n>","headers":{"Return-Path":"<kieran.bingham@ideasonboard.com>","Received":["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 DC28760B10\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tFri,  1 Feb 2019 16:15:51 +0100 (CET)","from [10.66.121.167] (unknown [91.183.179.147])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B4EBA4F7;\n\tFri,  1 Feb 2019 16:15:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1549034149;\n\tbh=BGNaMGUyKq3QzYQNj7whS3xmPkrLDOWNy94mEqFOAj0=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=cu4CAm7I/AUVaTV5Hd63xRwSKbTf0o8jLLV261rWExJ7eSeNkYEVYY+Y57L7obtgL\n\ttyUv7h7CVp4/A74n4UOdpSDpE+0fGCOj501mJhj/DLFQfbM+DjNHWKRtOPFBWRlDTd\n\tO8BoYGCAAciKRcy9R+iiqsNFiiKImyuNYSFIRTdE=","Reply-To":"kieran.bingham@ideasonboard.com","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo@jmondi.org>,\n\tLibCamera Devel <libcamera-devel@lists.libcamera.org>","References":"<20190129135357.32339-1-kieran.bingham@ideasonboard.com>\n\t<20190129135357.32339-2-kieran.bingham@ideasonboard.com>\n\t<20190131183435.esne5uyw3uunp5rf@uno.localdomain>\n\t<a7c59141-533a-d19c-d0f3-c3bf4f4e12de@ideasonboard.com>\n\t<20190201005235.GC23060@pendragon.ideasonboard.com>","From":"Kieran Bingham <kieran.bingham@ideasonboard.com>","Openpgp":"preference=signencrypt","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\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAkAEEwEKACoCGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEFAlnDk/gFCQeA/YsACgkQoR5GchCkYf3X5w/9EaZ7\n\tcnUcT6dxjxrcmmMnfFPoQA1iQXr/MXQJBjFWfxRUWYzjvUJb2D/FpA8FY7y+vksoJP7pWDL7\n\tQTbksdwzagUEk7CU45iLWL/CZ/knYhj1I/+5LSLFmvZ/5Gf5xn2ZCsmg7C0MdW/GbJ8IjWA8\n\t/LKJSEYH8tefoiG6+9xSNp1p0Gesu3vhje/GdGX4wDsfAxx1rIYDYVoX4bDM+uBUQh7sQox/\n\tR1bS0AaVJzPNcjeC14MS226mQRUaUPc9250aj44WmDfcg44/kMsoLFEmQo2II9aOlxUDJ+x1\n\txohGbh9mgBoVawMO3RMBihcEjo/8ytW6v7xSF+xP4Oc+HOn7qebAkxhSWcRxQVaQYw3S9iZz\n\t2iA09AXAkbvPKuMSXi4uau5daXStfBnmOfalG0j+9Y6hOFjz5j0XzaoF6Pln0jisDtWltYhP\n\tX9LjFVhhLkTzPZB/xOeWGmsG4gv2V2ExbU3uAmb7t1VSD9+IO3Km4FtnYOKBWlxwEd8qOFpS\n\tjEqMXURKOiJvnw3OXe9MqG19XdeENA1KyhK5rqjpwdvPGfSn2V+SlsdJA0DFsobUScD9qXQw\n\tOvhapHe3XboK2+Rd7L+g/9Ud7ZKLQHAsMBXOVJbufA1AT+IaOt0ugMcFkAR5UbBg5+dZUYJj\n\t1QbPQcGmM3wfvuaWV5+SlJ+WeKIb8ta5Ag0EVgT9ZgEQAM4o5G/kmruIQJ3K9SYzmPishRHV\n\tDcUcvoakyXSX2mIoccmo9BHtD9MxIt+QmxOpYFNFM7YofX4lG0ld8H7FqoNVLd/+a0yru5Cx\n\tadeZBe3qr1eLns10Q90LuMo7/6zJhCW2w+HE7xgmCHejAwuNe3+7yt4QmwlSGUqdxl8cgtS1\n\tPlEK93xXDsgsJj/bw1EfSVdAUqhx8UQ3aVFxNug5OpoX9FdWJLKROUrfNeBE16RLrNrq2ROc\n\tiSFETpVjyC/oZtzRFnwD9Or7EFMi76/xrWzk+/b15RJ9WrpXGMrttHUUcYZEOoiC2lEXMSAF\n\tSSSj4vHbKDJ0vKQdEFtdgB1roqzxdIOg4rlHz5qwOTynueiBpaZI3PHDudZSMR5Fk6QjFooE\n\tXTw3sSl/km/lvUFiv9CYyHOLdygWohvDuMkV/Jpdkfq8XwFSjOle+vT/4VqERnYFDIGBxaRx\n\tkoBLfNDiiuR3lD8tnJ4A1F88K6ojOUs+jndKsOaQpDZV6iNFv8IaNIklTPvPkZsmNDhJMRHH\n\tIu60S7BpzNeQeT4yyY4dX9lC2JL/LOEpw8DGf5BNOP1KgjCvyp1/KcFxDAo89IeqljaRsCdP\n\t7WCIECWYem6pLwaw6IAL7oX+tEqIMPph/G/jwZcdS6Hkyt/esHPuHNwX4guqTbVEuRqbDzDI\n\t2DJO5FbxABEBAAGJAiUEGAEKAA8CGwwFAlnDlGsFCQeA/gIACgkQoR5GchCkYf1yYRAAq+Yo\n\tnbf9DGdK1kTAm2RTFg+w9oOp2Xjqfhds2PAhFFvrHQg1XfQR/UF/SjeUmaOmLSczM0s6XMeO\n\tVcE77UFtJ/+hLo4PRFKm5X1Pcar6g5m4xGqa+Xfzi9tRkwC29KMCoQOag1BhHChgqYaUH3yo\n\tUzaPwT/fY75iVI+yD0ih/e6j8qYvP8pvGwMQfrmN9YB0zB39YzCSdaUaNrWGD3iCBxg6lwSO\n\tLKeRhxxfiXCIYEf3vwOsP3YMx2JkD5doseXmWBGW1U0T/oJF+DVfKB6mv5UfsTzpVhJRgee7\n\t4jkjqFq4qsUGxcvF2xtRkfHFpZDbRgRlVmiWkqDkT4qMA+4q1y/dWwshSKi/uwVZNycuLsz+\n\t+OD8xPNCsMTqeUkAKfbD8xW4LCay3r/dD2ckoxRxtMD9eOAyu5wYzo/ydIPTh1QEj9SYyvp8\n\tO0g6CpxEwyHUQtF5oh15O018z3ZLztFJKR3RD42VKVsrnNDKnoY0f4U0z7eJv2NeF8xHMuiU\n\tRCIzqxX1GVYaNkKTnb/Qja8hnYnkUzY1Lc+OtwiGmXTwYsPZjjAaDX35J/RSKAoy5wGo/YFA\n\tJxB1gWThL4kOTbsqqXj9GLcyOImkW0lJGGR3o/fV91Zh63S5TKnf2YGGGzxki+ADdxVQAm+Q\n\tsbsRB8KNNvVXBOVNwko86rQqF9drZuw=","Organization":"Ideas on Board","Message-ID":"<a3fcf475-beee-3782-5564-adb319188fce@ideasonboard.com>","Date":"Fri, 1 Feb 2019 16:15:47 +0100","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101\n\tThunderbird/60.4.0","MIME-Version":"1.0","In-Reply-To":"<20190201005235.GC23060@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-GB","Content-Transfer-Encoding":"8bit","Subject":"Re: [libcamera-devel] [PATCH 1/4] libcamera: Add Buffer and\n\tBufferPool classes","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Fri, 01 Feb 2019 15:15:52 -0000"}}]