[{"id":15335,"web_url":"https://patchwork.libcamera.org/comment/15335/","msgid":"","date":"2021-02-28T18:36:21","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","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 Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> In order to prepare to support more memory backends, make the\n> CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> pattern.\n\nIs this the right design pattern for the job ? The pimpl/d-pointer\npattern is used to hide the implementation from the interface of the\nclass, mostly to help maintaining ABI compatibility. In this specific\ncase, what you need is likely just an overload, with a base class\ndefining virtual methods.\n\nAnother option, if you want to simplify the implementation (I'm actually\nnot entirely sure it would be simpler) is to define the interface in\ncamera_buffer.h, and multiple implementations in different .cpp files,\nall named CameraBuffer. The .cpp file corresponding to the platform\nwould then be selected at compilation time. The drawback is that you\nthen would need to duplicate all functions in all implementations, even\nthe ones that could be shared.\n\n> Define the CameraBuffer class interface whose actual implementation is\n> delegated to an inner CameraBufferImpl class.\n> \n> Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> class to maintain compatibility of the CameraStream::process() interface\n> that requires a MappedBuffer * as second argument and will be converted\n> to use a CameraBuffer in the next patch.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_buffer.h | 12 ++++\n> src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> 2 files changed, 91 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> index 0590cd84652b..2a91e6a3c9c1 100644\n> --- a/src/android/camera_buffer.h\n> +++ b/src/android/camera_buffer.h\n> @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> public:\n> \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> \t~CameraBuffer();\n> +\n> +\tbool isValid() const;\n> +\n> +\tunsigned int numPlanes() const;\n> +\tssize_t planeSize(unsigned int plane) const;\n> +\n> +\tconst uint8_t *plane(unsigned int plane) const;\n> +\tuint8_t *plane(unsigned int plane);\n> +\n> +private:\n> +\tclass CameraBufferImpl;\n> +\tCameraBufferImpl *impl_;\n> };\n> \n> #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> index 807304a9e42d..10a43a61bd4d 100644\n> --- a/src/android/mm/android_generic_buffer.cpp\n> +++ b/src/android/mm/android_generic_buffer.cpp\n> @@ -13,7 +13,21 @@ using namespace libcamera;\n> \n> LOG_DECLARE_CATEGORY(HAL)\n> \n> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> +{\n> +public:\n> +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> +\t~CameraBufferImpl();\n> +\n> +\tunsigned int numPlanes() const;\n> +\tssize_t planeSize(unsigned int plane) const;\n> +\n> +\tconst uint8_t *plane(unsigned int plane) const;\n> +\tuint8_t *plane(unsigned int plane);\n> +};\n> +\n> +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> +\t\t\t\t\t\t int flags)\n> {\n> \tmaps_.reserve(camera3Buffer->numFds);\n> \terror_ = 0;\n> @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> \t}\n> }\n> \n> +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> +{\n> +}\n> +\n> +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> +{\n> +\treturn maps_.size();\n> +}\n> +\n> +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> +{\n> +\tif (plane >= maps_.size())\n> +\t\treturn -EINVAL;\n> +\n> +\treturn maps_[plane].size();\n> +}\n> +\n> +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> +{\n> +\tif (plane >= maps_.size())\n> +\t\treturn nullptr;\n> +\n> +\treturn maps_[plane].data();\n> +}\n> +\n> +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> +{\n> +\tif (plane >= maps_.size())\n> +\t\treturn nullptr;\n> +\n> +\treturn maps_[plane].data();\n> +}\n> +\n> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> +{\n> +}\n> +\n> CameraBuffer::~CameraBuffer()\n> {\n> +\tdelete impl_;\n> +}\n> +\n> +bool CameraBuffer::isValid() const\n> +{\n> +\treturn impl_->isValid();\n> +}\n> +\n> +unsigned int CameraBuffer::numPlanes() const\n> +{\n> +\treturn impl_->numPlanes();\n> +}\n> +\n> +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> +{\n> +\treturn impl_->planeSize(plane);\n> +}\n> +\n> +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> +{\n> +\treturn impl_->plane(plane);\n> +}\n> +\n> +uint8_t *CameraBuffer::plane(unsigned int plane)\n> +{\n> +\treturn impl_->plane(plane);\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 4A4E3BD808\n\tfor ;\n\tSun, 28 Feb 2021 18:36:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B8E8A68A79;\n\tSun, 28 Feb 2021 19:36:51 +0100 (CET)","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 D578268A45\n\tfor ;\n\tSun, 28 Feb 2021 19:36:50 +0100 (CET)","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 41D5780F;\n\tSun, 28 Feb 2021 19:36:50 +0100 (CET)"],"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=\"Jg45OlSS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614537410;\n\tbh=6uq3Kh7LgEd0MbYy6+r9soyOwOKJfp2X10EO1x+dd9A=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Jg45OlSSl5SyQt+SM08ac2NzZE9ZncX7yDOk686j1qMkYyzOMtEoBb8fQ2WKYDBKY\n\tvnJgU84SbnUxYuKZWnnACqk+oDR6eesaUSM5nV4/hjmPaLR0eXdHJ0pyBjPe+5TQ07\n\tcVwxd6S1uuBCiGJF0Z90l2e1LJFPPKsDzI88a8pQ=","Date":"Sun, 28 Feb 2021 20:36:21 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210226132932.165484-8-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15340,"web_url":"https://patchwork.libcamera.org/comment/15340/","msgid":"","date":"2021-02-28T19:10:21","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:\n> On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> > In order to prepare to support more memory backends, make the\n> > CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> > pattern.\n> \n> Is this the right design pattern for the job ? The pimpl/d-pointer\n> pattern is used to hide the implementation from the interface of the\n> class, mostly to help maintaining ABI compatibility. In this specific\n> case, what you need is likely just an overload, with a base class\n> defining virtual methods.\n> \n> Another option, if you want to simplify the implementation (I'm actually\n> not entirely sure it would be simpler) is to define the interface in\n> camera_buffer.h, and multiple implementations in different .cpp files,\n> all named CameraBuffer. The .cpp file corresponding to the platform\n> would then be selected at compilation time. The drawback is that you\n> then would need to duplicate all functions in all implementations, even\n> the ones that could be shared.\n\nAfter reviewing the rest of the series I see that you can't expose the\nbackend-specific private members in CameraBuffer, which explains why\nyou're using this design pattern.\n\nYou could keep using this pattern, with the CameraBufferImpl only\nstoring the data, not duplicating every function. In that case I'd\ninherit from libcamera::Extensible instead of creating a manual\nimplementation.\n\nThe other option is to use a base class and multiple derived classes as\nmentioned above. We've discussed this previously and I agreed that we\ncould just select which file to compile as a simple implementation, but\nI didn't realize then that we would need private data. I'm wondering\nwhether the implementation wouldn't be simpler with inheritance than\nwith private data. Sorry for not thinking about it in the beginning.\n\n> > Define the CameraBuffer class interface whose actual implementation is\n> > delegated to an inner CameraBufferImpl class.\n> > \n> > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> > class to maintain compatibility of the CameraStream::process() interface\n> > that requires a MappedBuffer * as second argument and will be converted\n> > to use a CameraBuffer in the next patch.\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/android/camera_buffer.h | 12 ++++\n> > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> > 2 files changed, 91 insertions(+), 1 deletion(-)\n> > \n> > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > index 0590cd84652b..2a91e6a3c9c1 100644\n> > --- a/src/android/camera_buffer.h\n> > +++ b/src/android/camera_buffer.h\n> > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> > public:\n> > \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > \t~CameraBuffer();\n> > +\n> > +\tbool isValid() const;\n> > +\n> > +\tunsigned int numPlanes() const;\n> > +\tssize_t planeSize(unsigned int plane) const;\n> > +\n> > +\tconst uint8_t *plane(unsigned int plane) const;\n> > +\tuint8_t *plane(unsigned int plane);\n> > +\n> > +private:\n> > +\tclass CameraBufferImpl;\n> > +\tCameraBufferImpl *impl_;\n> > };\n> > \n> > #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> > index 807304a9e42d..10a43a61bd4d 100644\n> > --- a/src/android/mm/android_generic_buffer.cpp\n> > +++ b/src/android/mm/android_generic_buffer.cpp\n> > @@ -13,7 +13,21 @@ using namespace libcamera;\n> > \n> > LOG_DECLARE_CATEGORY(HAL)\n> > \n> > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> > +{\n> > +public:\n> > +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> > +\t~CameraBufferImpl();\n> > +\n> > +\tunsigned int numPlanes() const;\n> > +\tssize_t planeSize(unsigned int plane) const;\n> > +\n> > +\tconst uint8_t *plane(unsigned int plane) const;\n> > +\tuint8_t *plane(unsigned int plane);\n> > +};\n> > +\n> > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> > +\t\t\t\t\t\t int flags)\n> > {\n> > \tmaps_.reserve(camera3Buffer->numFds);\n> > \terror_ = 0;\n> > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > \t}\n> > }\n> > \n> > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> > +{\n> > +}\n> > +\n> > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> > +{\n> > +\treturn maps_.size();\n> > +}\n> > +\n> > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> > +{\n> > +\tif (plane >= maps_.size())\n> > +\t\treturn -EINVAL;\n> > +\n> > +\treturn maps_[plane].size();\n> > +}\n> > +\n> > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> > +{\n> > +\tif (plane >= maps_.size())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn maps_[plane].data();\n> > +}\n> > +\n> > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> > +{\n> > +\tif (plane >= maps_.size())\n> > +\t\treturn nullptr;\n> > +\n> > +\treturn maps_[plane].data();\n> > +}\n> > +\n> > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> > +{\n> > +}\n> > +\n> > CameraBuffer::~CameraBuffer()\n> > {\n> > +\tdelete impl_;\n> > +}\n> > +\n> > +bool CameraBuffer::isValid() const\n> > +{\n> > +\treturn impl_->isValid();\n> > +}\n> > +\n> > +unsigned int CameraBuffer::numPlanes() const\n> > +{\n> > +\treturn impl_->numPlanes();\n> > +}\n> > +\n> > +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> > +{\n> > +\treturn impl_->planeSize(plane);\n> > +}\n> > +\n> > +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> > +{\n> > +\treturn impl_->plane(plane);\n> > +}\n> > +\n> > +uint8_t *CameraBuffer::plane(unsigned int plane)\n> > +{\n> > +\treturn impl_->plane(plane);\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 8D98EBD1F1\n\tfor ;\n\tSun, 28 Feb 2021 19:10:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2716668A7E;\n\tSun, 28 Feb 2021 20:10:51 +0100 (CET)","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 54BAA68A45\n\tfor ;\n\tSun, 28 Feb 2021 20:10:50 +0100 (CET)","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 B5D4C80F;\n\tSun, 28 Feb 2021 20:10:49 +0100 (CET)"],"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=\"Iy17ivys\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614539449;\n\tbh=SxNjXxE9KFfIO1x7x7K+ex876yneNgjDNFnos9K7C7Q=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Iy17ivysCNp9WUO5jg4e86BJPRrMJXrTSCzYu/W4C1j5sxDJK/w22zgaLQ+BHzX8L\n\trjINYBCD6lJ4ymEoeHnv03VKjrlHuiVgpZgGKpJe2PgaXNZ36bO8aKgtX4uRwtlXDh\n\tmxOf9vQ0tG1CTLgksMds97eNcE2zvmjohXpUuTK8=","Date":"Sun, 28 Feb 2021 21:10:21 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15348,"web_url":"https://patchwork.libcamera.org/comment/15348/","msgid":"<20210301090208.pwfictniaxa4jhyz@uno.localdomain>","date":"2021-03-01T09:02:08","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:\n> > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> > > In order to prepare to support more memory backends, make the\n> > > CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> > > pattern.\n> >\n> > Is this the right design pattern for the job ? The pimpl/d-pointer\n> > pattern is used to hide the implementation from the interface of the\n> > class, mostly to help maintaining ABI compatibility. In this specific\n> > case, what you need is likely just an overload, with a base class\n> > defining virtual methods.\n> >\n> > Another option, if you want to simplify the implementation (I'm actually\n> > not entirely sure it would be simpler) is to define the interface in\n> > camera_buffer.h, and multiple implementations in different .cpp files,\n> > all named CameraBuffer. The .cpp file corresponding to the platform\n> > would then be selected at compilation time. The drawback is that you\n> > then would need to duplicate all functions in all implementations, even\n> > the ones that could be shared.\n>\n> After reviewing the rest of the series I see that you can't expose the\n> backend-specific private members in CameraBuffer, which explains why\n> you're using this design pattern.\n\nThat's exactly the reason why I went in that directin\n\n>\n> You could keep using this pattern, with the CameraBufferImpl only\n> storing the data, not duplicating every function. In that case I'd\n> inherit from libcamera::Extensible instead of creating a manual\n> implementation.\n>\n> The other option is to use a base class and multiple derived classes as\n> mentioned above. We've discussed this previously and I agreed that we\n> could just select which file to compile as a simple implementation, but\n> I didn't realize then that we would need private data. I'm wondering\n> whether the implementation wouldn't be simpler with inheritance than\n> with private data. Sorry for not thinking about it in the beginning.\n\nI didn't realize that in the beginning as well.\n\nFor the cros backend we could get away by getting an instance of the\ncros::CameraBufferManager through the static singleton constructor in\nevery function, by changing the interface and adding the current\nbuffer as parameter, but that would make the whole hierachy stateless,\nsomething I am not sure we can guarantee for all the backends we'll\nhave to implement. I would so refrain from tying our hands, so using a\npointer to the actual implementation seems the less risky way.\n\nI'll see how Extensible looks like and send a v2.\n\nThanks for review\n j\n>\n> > > Define the CameraBuffer class interface whose actual implementation is\n> > > delegated to an inner CameraBufferImpl class.\n> > >\n> > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> > > class to maintain compatibility of the CameraStream::process() interface\n> > > that requires a MappedBuffer * as second argument and will be converted\n> > > to use a CameraBuffer in the next patch.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/android/camera_buffer.h | 12 ++++\n> > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> > > 2 files changed, 91 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > index 0590cd84652b..2a91e6a3c9c1 100644\n> > > --- a/src/android/camera_buffer.h\n> > > +++ b/src/android/camera_buffer.h\n> > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> > > public:\n> > > \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > \t~CameraBuffer();\n> > > +\n> > > +\tbool isValid() const;\n> > > +\n> > > +\tunsigned int numPlanes() const;\n> > > +\tssize_t planeSize(unsigned int plane) const;\n> > > +\n> > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > +\tuint8_t *plane(unsigned int plane);\n> > > +\n> > > +private:\n> > > +\tclass CameraBufferImpl;\n> > > +\tCameraBufferImpl *impl_;\n> > > };\n> > >\n> > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> > > index 807304a9e42d..10a43a61bd4d 100644\n> > > --- a/src/android/mm/android_generic_buffer.cpp\n> > > +++ b/src/android/mm/android_generic_buffer.cpp\n> > > @@ -13,7 +13,21 @@ using namespace libcamera;\n> > >\n> > > LOG_DECLARE_CATEGORY(HAL)\n> > >\n> > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> > > +{\n> > > +public:\n> > > +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> > > +\t~CameraBufferImpl();\n> > > +\n> > > +\tunsigned int numPlanes() const;\n> > > +\tssize_t planeSize(unsigned int plane) const;\n> > > +\n> > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > +\tuint8_t *plane(unsigned int plane);\n> > > +};\n> > > +\n> > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> > > +\t\t\t\t\t\t int flags)\n> > > {\n> > > \tmaps_.reserve(camera3Buffer->numFds);\n> > > \terror_ = 0;\n> > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > \t}\n> > > }\n> > >\n> > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> > > +{\n> > > +}\n> > > +\n> > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> > > +{\n> > > +\treturn maps_.size();\n> > > +}\n> > > +\n> > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> > > +{\n> > > +\tif (plane >= maps_.size())\n> > > +\t\treturn -EINVAL;\n> > > +\n> > > +\treturn maps_[plane].size();\n> > > +}\n> > > +\n> > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> > > +{\n> > > +\tif (plane >= maps_.size())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn maps_[plane].data();\n> > > +}\n> > > +\n> > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> > > +{\n> > > +\tif (plane >= maps_.size())\n> > > +\t\treturn nullptr;\n> > > +\n> > > +\treturn maps_[plane].data();\n> > > +}\n> > > +\n> > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> > > +{\n> > > +}\n> > > +\n> > > CameraBuffer::~CameraBuffer()\n> > > {\n> > > +\tdelete impl_;\n> > > +}\n> > > +\n> > > +bool CameraBuffer::isValid() const\n> > > +{\n> > > +\treturn impl_->isValid();\n> > > +}\n> > > +\n> > > +unsigned int CameraBuffer::numPlanes() const\n> > > +{\n> > > +\treturn impl_->numPlanes();\n> > > +}\n> > > +\n> > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> > > +{\n> > > +\treturn impl_->planeSize(plane);\n> > > +}\n> > > +\n> > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> > > +{\n> > > +\treturn impl_->plane(plane);\n> > > +}\n> > > +\n> > > +uint8_t *CameraBuffer::plane(unsigned int plane)\n> > > +{\n> > > +\treturn impl_->plane(plane);\n> > > }\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 264FFBD1F1\n\tfor ;\n\tMon, 1 Mar 2021 09:01:43 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B21C9689DD;\n\tMon, 1 Mar 2021 10:01:42 +0100 (CET)","from relay10.mail.gandi.net (relay10.mail.gandi.net\n\t[217.70.178.230])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3AB4260521\n\tfor ;\n\tMon, 1 Mar 2021 10:01:41 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay10.mail.gandi.net (Postfix) with ESMTPSA id 96C09240011;\n\tMon, 1 Mar 2021 09:01:40 +0000 (UTC)"],"Date":"Mon, 1 Mar 2021 10:02:08 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210301090208.pwfictniaxa4jhyz@uno.localdomain>","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15349,"web_url":"https://patchwork.libcamera.org/comment/15349/","msgid":"","date":"2021-03-01T09:24:22","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote:\n> On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:\n> > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> > > > In order to prepare to support more memory backends, make the\n> > > > CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> > > > pattern.\n> > >\n> > > Is this the right design pattern for the job ? The pimpl/d-pointer\n> > > pattern is used to hide the implementation from the interface of the\n> > > class, mostly to help maintaining ABI compatibility. In this specific\n> > > case, what you need is likely just an overload, with a base class\n> > > defining virtual methods.\n> > >\n> > > Another option, if you want to simplify the implementation (I'm actually\n> > > not entirely sure it would be simpler) is to define the interface in\n> > > camera_buffer.h, and multiple implementations in different .cpp files,\n> > > all named CameraBuffer. The .cpp file corresponding to the platform\n> > > would then be selected at compilation time. The drawback is that you\n> > > then would need to duplicate all functions in all implementations, even\n> > > the ones that could be shared.\n> >\n> > After reviewing the rest of the series I see that you can't expose the\n> > backend-specific private members in CameraBuffer, which explains why\n> > you're using this design pattern.\n> \n> That's exactly the reason why I went in that directin\n> \n> > You could keep using this pattern, with the CameraBufferImpl only\n> > storing the data, not duplicating every function. In that case I'd\n> > inherit from libcamera::Extensible instead of creating a manual\n> > implementation.\n> >\n> > The other option is to use a base class and multiple derived classes as\n> > mentioned above. We've discussed this previously and I agreed that we\n> > could just select which file to compile as a simple implementation, but\n> > I didn't realize then that we would need private data. I'm wondering\n> > whether the implementation wouldn't be simpler with inheritance than\n> > with private data. Sorry for not thinking about it in the beginning.\n> \n> I didn't realize that in the beginning as well.\n> \n> For the cros backend we could get away by getting an instance of the\n> cros::CameraBufferManager through the static singleton constructor in\n> every function, by changing the interface and adding the current\n> buffer as parameter, but that would make the whole hierachy stateless,\n> something I am not sure we can guarantee for all the backends we'll\n> have to implement. I would so refrain from tying our hands,\n\nAgreed.\n\n> so using a pointer to the actual implementation seems the less risky\n> way.\n\nThat I'm not sure about :-) You can store any amount of custom data in a\nderived class, so a base abstract class for the interface plus derived\nclasses that implement that interface could work fine too.\n\nAn implementation based on Extensible could be fine too, please just\ndon't rule the base + derived option out yet in your investigations.\n\n> I'll see how Extensible looks like and send a v2.\n> \n> > > > Define the CameraBuffer class interface whose actual implementation is\n> > > > delegated to an inner CameraBufferImpl class.\n> > > >\n> > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> > > > class to maintain compatibility of the CameraStream::process() interface\n> > > > that requires a MappedBuffer * as second argument and will be converted\n> > > > to use a CameraBuffer in the next patch.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/android/camera_buffer.h | 12 ++++\n> > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> > > > 2 files changed, 91 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > index 0590cd84652b..2a91e6a3c9c1 100644\n> > > > --- a/src/android/camera_buffer.h\n> > > > +++ b/src/android/camera_buffer.h\n> > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> > > > public:\n> > > > \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > \t~CameraBuffer();\n> > > > +\n> > > > +\tbool isValid() const;\n> > > > +\n> > > > +\tunsigned int numPlanes() const;\n> > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > +\n> > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > +\tuint8_t *plane(unsigned int plane);\n> > > > +\n> > > > +private:\n> > > > +\tclass CameraBufferImpl;\n> > > > +\tCameraBufferImpl *impl_;\n> > > > };\n> > > >\n> > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> > > > index 807304a9e42d..10a43a61bd4d 100644\n> > > > --- a/src/android/mm/android_generic_buffer.cpp\n> > > > +++ b/src/android/mm/android_generic_buffer.cpp\n> > > > @@ -13,7 +13,21 @@ using namespace libcamera;\n> > > >\n> > > > LOG_DECLARE_CATEGORY(HAL)\n> > > >\n> > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> > > > +{\n> > > > +public:\n> > > > +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> > > > +\t~CameraBufferImpl();\n> > > > +\n> > > > +\tunsigned int numPlanes() const;\n> > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > +\n> > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > +\tuint8_t *plane(unsigned int plane);\n> > > > +};\n> > > > +\n> > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> > > > +\t\t\t\t\t\t int flags)\n> > > > {\n> > > > \tmaps_.reserve(camera3Buffer->numFds);\n> > > > \terror_ = 0;\n> > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > \t}\n> > > > }\n> > > >\n> > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> > > > +{\n> > > > +}\n> > > > +\n> > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> > > > +{\n> > > > +\treturn maps_.size();\n> > > > +}\n> > > > +\n> > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> > > > +{\n> > > > +\tif (plane >= maps_.size())\n> > > > +\t\treturn -EINVAL;\n> > > > +\n> > > > +\treturn maps_[plane].size();\n> > > > +}\n> > > > +\n> > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> > > > +{\n> > > > +\tif (plane >= maps_.size())\n> > > > +\t\treturn nullptr;\n> > > > +\n> > > > +\treturn maps_[plane].data();\n> > > > +}\n> > > > +\n> > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> > > > +{\n> > > > +\tif (plane >= maps_.size())\n> > > > +\t\treturn nullptr;\n> > > > +\n> > > > +\treturn maps_[plane].data();\n> > > > +}\n> > > > +\n> > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> > > > +{\n> > > > +}\n> > > > +\n> > > > CameraBuffer::~CameraBuffer()\n> > > > {\n> > > > +\tdelete impl_;\n> > > > +}\n> > > > +\n> > > > +bool CameraBuffer::isValid() const\n> > > > +{\n> > > > +\treturn impl_->isValid();\n> > > > +}\n> > > > +\n> > > > +unsigned int CameraBuffer::numPlanes() const\n> > > > +{\n> > > > +\treturn impl_->numPlanes();\n> > > > +}\n> > > > +\n> > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> > > > +{\n> > > > +\treturn impl_->planeSize(plane);\n> > > > +}\n> > > > +\n> > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> > > > +{\n> > > > +\treturn impl_->plane(plane);\n> > > > +}\n> > > > +\n> > > > +uint8_t *CameraBuffer::plane(unsigned int plane)\n> > > > +{\n> > > > +\treturn impl_->plane(plane);\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 0FDC7BD808\n\tfor ;\n\tMon, 1 Mar 2021 09:24:53 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 97733689DD;\n\tMon, 1 Mar 2021 10:24:52 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8BB1960521\n\tfor ;\n\tMon, 1 Mar 2021 10:24:50 +0100 (CET)","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 EF7B5332;\n\tMon, 1 Mar 2021 10:24:49 +0100 (CET)"],"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=\"b5tOTntS\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614590690;\n\tbh=O1ZfR4pkS7pDKKe5PGb7Ih18cXZTfvK32a6RUhWEfGA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=b5tOTntSo19FjEfakrK3iw7gsWV97cOM8kRbrbZx1jt3x+Jxf4X4Nf6A6ST3Z1Ovy\n\tFsr3M32cIPSfljokinTRS1G/JCW70zpkqWPkoAsnuZvrVDBlTluQ7UE9Rnmo00uCF9\n\tUoyCYLBlIrsIJWLZQsO+qFUmIce3EGwqzj9aqMDU=","Date":"Mon, 1 Mar 2021 11:24:22 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>\n\t\n\t\n\t<20210301090208.pwfictniaxa4jhyz@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301090208.pwfictniaxa4jhyz@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15353,"web_url":"https://patchwork.libcamera.org/comment/15353/","msgid":"<20210301094013.vsmwwicfzrly2ibd@uno.localdomain>","date":"2021-03-01T09:40:13","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Mar 01, 2021 at 11:24:22AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote:\n> > On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:\n> > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:\n> > > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> > > > > In order to prepare to support more memory backends, make the\n> > > > > CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> > > > > pattern.\n> > > >\n> > > > Is this the right design pattern for the job ? The pimpl/d-pointer\n> > > > pattern is used to hide the implementation from the interface of the\n> > > > class, mostly to help maintaining ABI compatibility. In this specific\n> > > > case, what you need is likely just an overload, with a base class\n> > > > defining virtual methods.\n> > > >\n> > > > Another option, if you want to simplify the implementation (I'm actually\n> > > > not entirely sure it would be simpler) is to define the interface in\n> > > > camera_buffer.h, and multiple implementations in different .cpp files,\n> > > > all named CameraBuffer. The .cpp file corresponding to the platform\n> > > > would then be selected at compilation time. The drawback is that you\n> > > > then would need to duplicate all functions in all implementations, even\n> > > > the ones that could be shared.\n> > >\n> > > After reviewing the rest of the series I see that you can't expose the\n> > > backend-specific private members in CameraBuffer, which explains why\n> > > you're using this design pattern.\n> >\n> > That's exactly the reason why I went in that directin\n> >\n> > > You could keep using this pattern, with the CameraBufferImpl only\n> > > storing the data, not duplicating every function. In that case I'd\n> > > inherit from libcamera::Extensible instead of creating a manual\n> > > implementation.\n> > >\n> > > The other option is to use a base class and multiple derived classes as\n> > > mentioned above. We've discussed this previously and I agreed that we\n> > > could just select which file to compile as a simple implementation, but\n> > > I didn't realize then that we would need private data. I'm wondering\n> > > whether the implementation wouldn't be simpler with inheritance than\n> > > with private data. Sorry for not thinking about it in the beginning.\n> >\n> > I didn't realize that in the beginning as well.\n> >\n> > For the cros backend we could get away by getting an instance of the\n> > cros::CameraBufferManager through the static singleton constructor in\n> > every function, by changing the interface and adding the current\n> > buffer as parameter, but that would make the whole hierachy stateless,\n> > something I am not sure we can guarantee for all the backends we'll\n> > have to implement. I would so refrain from tying our hands,\n>\n> Agreed.\n>\n> > so using a pointer to the actual implementation seems the less risky\n> > way.\n>\n> That I'm not sure about :-) You can store any amount of custom data in a\n> derived class, so a base abstract class for the interface plus derived\n> classes that implement that interface could work fine too.\n\nYes, but then what instantiating the right derived class using a\ncompile-time option would require a factory or a chain of #ifdef\n\nHowever, if we compile in a single dervide class at the time, we can\ninstantiate a CameraBuffer unconditionally ? I'll try this one as\nwell, it might work and be simpler than using Extensible.\n\n>\n> An implementation based on Extensible could be fine too, please just\n> don't rule the base + derived option out yet in your investigations.\n>\n> > I'll see how Extensible looks like and send a v2.\n> >\n> > > > > Define the CameraBuffer class interface whose actual implementation is\n> > > > > delegated to an inner CameraBufferImpl class.\n> > > > >\n> > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> > > > > class to maintain compatibility of the CameraStream::process() interface\n> > > > > that requires a MappedBuffer * as second argument and will be converted\n> > > > > to use a CameraBuffer in the next patch.\n> > > > >\n> > > > > Signed-off-by: Jacopo Mondi \n> > > > > ---\n> > > > > src/android/camera_buffer.h | 12 ++++\n> > > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> > > > > 2 files changed, 91 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > index 0590cd84652b..2a91e6a3c9c1 100644\n> > > > > --- a/src/android/camera_buffer.h\n> > > > > +++ b/src/android/camera_buffer.h\n> > > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> > > > > public:\n> > > > > \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > \t~CameraBuffer();\n> > > > > +\n> > > > > +\tbool isValid() const;\n> > > > > +\n> > > > > +\tunsigned int numPlanes() const;\n> > > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > > +\n> > > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > > +\tuint8_t *plane(unsigned int plane);\n> > > > > +\n> > > > > +private:\n> > > > > +\tclass CameraBufferImpl;\n> > > > > +\tCameraBufferImpl *impl_;\n> > > > > };\n> > > > >\n> > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> > > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> > > > > index 807304a9e42d..10a43a61bd4d 100644\n> > > > > --- a/src/android/mm/android_generic_buffer.cpp\n> > > > > +++ b/src/android/mm/android_generic_buffer.cpp\n> > > > > @@ -13,7 +13,21 @@ using namespace libcamera;\n> > > > >\n> > > > > LOG_DECLARE_CATEGORY(HAL)\n> > > > >\n> > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> > > > > +{\n> > > > > +public:\n> > > > > +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> > > > > +\t~CameraBufferImpl();\n> > > > > +\n> > > > > +\tunsigned int numPlanes() const;\n> > > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > > +\n> > > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > > +\tuint8_t *plane(unsigned int plane);\n> > > > > +};\n> > > > > +\n> > > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> > > > > +\t\t\t\t\t\t int flags)\n> > > > > {\n> > > > > \tmaps_.reserve(camera3Buffer->numFds);\n> > > > > \terror_ = 0;\n> > > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > \t}\n> > > > > }\n> > > > >\n> > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> > > > > +{\n> > > > > +\treturn maps_.size();\n> > > > > +}\n> > > > > +\n> > > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> > > > > +{\n> > > > > +\tif (plane >= maps_.size())\n> > > > > +\t\treturn -EINVAL;\n> > > > > +\n> > > > > +\treturn maps_[plane].size();\n> > > > > +}\n> > > > > +\n> > > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> > > > > +{\n> > > > > +\tif (plane >= maps_.size())\n> > > > > +\t\treturn nullptr;\n> > > > > +\n> > > > > +\treturn maps_[plane].data();\n> > > > > +}\n> > > > > +\n> > > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> > > > > +{\n> > > > > +\tif (plane >= maps_.size())\n> > > > > +\t\treturn nullptr;\n> > > > > +\n> > > > > +\treturn maps_[plane].data();\n> > > > > +}\n> > > > > +\n> > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> > > > > +{\n> > > > > +}\n> > > > > +\n> > > > > CameraBuffer::~CameraBuffer()\n> > > > > {\n> > > > > +\tdelete impl_;\n> > > > > +}\n> > > > > +\n> > > > > +bool CameraBuffer::isValid() const\n> > > > > +{\n> > > > > +\treturn impl_->isValid();\n> > > > > +}\n> > > > > +\n> > > > > +unsigned int CameraBuffer::numPlanes() const\n> > > > > +{\n> > > > > +\treturn impl_->numPlanes();\n> > > > > +}\n> > > > > +\n> > > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> > > > > +{\n> > > > > +\treturn impl_->planeSize(plane);\n> > > > > +}\n> > > > > +\n> > > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> > > > > +{\n> > > > > +\treturn impl_->plane(plane);\n> > > > > +}\n> > > > > +\n> > > > > +uint8_t *CameraBuffer::plane(unsigned int plane)\n> > > > > +{\n> > > > > +\treturn impl_->plane(plane);\n> > > > > }\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 A712EBD808\n\tfor ;\n\tMon, 1 Mar 2021 09:39:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 829FB68A81;\n\tMon, 1 Mar 2021 10:39:47 +0100 (CET)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE81560521\n\tfor ;\n\tMon, 1 Mar 2021 10:39:45 +0100 (CET)","from uno.localdomain (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 7B0691BF204;\n\tMon, 1 Mar 2021 09:39:45 +0000 (UTC)"],"X-Originating-IP":"93.61.96.190","Date":"Mon, 1 Mar 2021 10:40:13 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210301094013.vsmwwicfzrly2ibd@uno.localdomain>","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>\n\t\n\t\n\t<20210301090208.pwfictniaxa4jhyz@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15356,"web_url":"https://patchwork.libcamera.org/comment/15356/","msgid":"","date":"2021-03-01T09:47:31","subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 01, 2021 at 10:40:13AM +0100, Jacopo Mondi wrote:\n> On Mon, Mar 01, 2021 at 11:24:22AM +0200, Laurent Pinchart wrote:\n> > On Mon, Mar 01, 2021 at 10:02:08AM +0100, Jacopo Mondi wrote:\n> > > On Sun, Feb 28, 2021 at 09:10:21PM +0200, Laurent Pinchart wrote:\n> > > > On Sun, Feb 28, 2021 at 08:36:23PM +0200, Laurent Pinchart wrote:\n> > > > > On Fri, Feb 26, 2021 at 02:29:27PM +0100, Jacopo Mondi wrote:\n> > > > > > In order to prepare to support more memory backends, make the\n> > > > > > CameraBuffer class implement the PIMPL (pointer-to-implementation)\n> > > > > > pattern.\n> > > > >\n> > > > > Is this the right design pattern for the job ? The pimpl/d-pointer\n> > > > > pattern is used to hide the implementation from the interface of the\n> > > > > class, mostly to help maintaining ABI compatibility. In this specific\n> > > > > case, what you need is likely just an overload, with a base class\n> > > > > defining virtual methods.\n> > > > >\n> > > > > Another option, if you want to simplify the implementation (I'm actually\n> > > > > not entirely sure it would be simpler) is to define the interface in\n> > > > > camera_buffer.h, and multiple implementations in different .cpp files,\n> > > > > all named CameraBuffer. The .cpp file corresponding to the platform\n> > > > > would then be selected at compilation time. The drawback is that you\n> > > > > then would need to duplicate all functions in all implementations, even\n> > > > > the ones that could be shared.\n> > > >\n> > > > After reviewing the rest of the series I see that you can't expose the\n> > > > backend-specific private members in CameraBuffer, which explains why\n> > > > you're using this design pattern.\n> > >\n> > > That's exactly the reason why I went in that directin\n> > >\n> > > > You could keep using this pattern, with the CameraBufferImpl only\n> > > > storing the data, not duplicating every function. In that case I'd\n> > > > inherit from libcamera::Extensible instead of creating a manual\n> > > > implementation.\n> > > >\n> > > > The other option is to use a base class and multiple derived classes as\n> > > > mentioned above. We've discussed this previously and I agreed that we\n> > > > could just select which file to compile as a simple implementation, but\n> > > > I didn't realize then that we would need private data. I'm wondering\n> > > > whether the implementation wouldn't be simpler with inheritance than\n> > > > with private data. Sorry for not thinking about it in the beginning.\n> > >\n> > > I didn't realize that in the beginning as well.\n> > >\n> > > For the cros backend we could get away by getting an instance of the\n> > > cros::CameraBufferManager through the static singleton constructor in\n> > > every function, by changing the interface and adding the current\n> > > buffer as parameter, but that would make the whole hierachy stateless,\n> > > something I am not sure we can guarantee for all the backends we'll\n> > > have to implement. I would so refrain from tying our hands,\n> >\n> > Agreed.\n> >\n> > > so using a pointer to the actual implementation seems the less risky\n> > > way.\n> >\n> > That I'm not sure about :-) You can store any amount of custom data in a\n> > derived class, so a base abstract class for the interface plus derived\n> > classes that implement that interface could work fine too.\n> \n> Yes, but then what instantiating the right derived class using a\n> compile-time option would require a factory or a chain of #ifdef\n\nYes. I'd go for #ifdef (at least for now) for simplicity I think.\n\n> However, if we compile in a single dervide class at the time, we can\n> instantiate a CameraBuffer unconditionally ? I'll try this one as\n> well, it might work and be simpler than using Extensible.\n\nIf we have multiple implementations of CameraBuffer, without a base\nclass, then we can instantiate CameraBuffer unconditionally, but we need\nprivate data that would be different depending on the implementation,\nand so we'll need Extensible. Otherwise, with a base class, we need to\ninstantiate the right derived class (using #ifdef), but we won't need\nExtensible. Up to you :-)\n\n> > An implementation based on Extensible could be fine too, please just\n> > don't rule the base + derived option out yet in your investigations.\n> >\n> > > I'll see how Extensible looks like and send a v2.\n> > >\n> > > > > > Define the CameraBuffer class interface whose actual implementation is\n> > > > > > delegated to an inner CameraBufferImpl class.\n> > > > > >\n> > > > > > Temporary maintain libcamera::MappedBuffer as the CameraBuffer base\n> > > > > > class to maintain compatibility of the CameraStream::process() interface\n> > > > > > that requires a MappedBuffer * as second argument and will be converted\n> > > > > > to use a CameraBuffer in the next patch.\n> > > > > >\n> > > > > > Signed-off-by: Jacopo Mondi \n> > > > > > ---\n> > > > > > src/android/camera_buffer.h | 12 ++++\n> > > > > > src/android/mm/android_generic_buffer.cpp | 80 ++++++++++++++++++++++-\n> > > > > > 2 files changed, 91 insertions(+), 1 deletion(-)\n> > > > > >\n> > > > > > diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> > > > > > index 0590cd84652b..2a91e6a3c9c1 100644\n> > > > > > --- a/src/android/camera_buffer.h\n> > > > > > +++ b/src/android/camera_buffer.h\n> > > > > > @@ -16,6 +16,18 @@ class CameraBuffer : public libcamera::MappedBuffer\n> > > > > > public:\n> > > > > > \tCameraBuffer(buffer_handle_t camera3Buffer, int flags);\n> > > > > > \t~CameraBuffer();\n> > > > > > +\n> > > > > > +\tbool isValid() const;\n> > > > > > +\n> > > > > > +\tunsigned int numPlanes() const;\n> > > > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > > > +\n> > > > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > > > +\tuint8_t *plane(unsigned int plane);\n> > > > > > +\n> > > > > > +private:\n> > > > > > +\tclass CameraBufferImpl;\n> > > > > > +\tCameraBufferImpl *impl_;\n> > > > > > };\n> > > > > >\n> > > > > > #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> > > > > > diff --git a/src/android/mm/android_generic_buffer.cpp b/src/android/mm/android_generic_buffer.cpp\n> > > > > > index 807304a9e42d..10a43a61bd4d 100644\n> > > > > > --- a/src/android/mm/android_generic_buffer.cpp\n> > > > > > +++ b/src/android/mm/android_generic_buffer.cpp\n> > > > > > @@ -13,7 +13,21 @@ using namespace libcamera;\n> > > > > >\n> > > > > > LOG_DECLARE_CATEGORY(HAL)\n> > > > > >\n> > > > > > -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > > +class CameraBuffer::CameraBufferImpl : public libcamera::MappedBuffer\n> > > > > > +{\n> > > > > > +public:\n> > > > > > +\tCameraBufferImpl(buffer_handle_t camera3Buffer, int flags);\n> > > > > > +\t~CameraBufferImpl();\n> > > > > > +\n> > > > > > +\tunsigned int numPlanes() const;\n> > > > > > +\tssize_t planeSize(unsigned int plane) const;\n> > > > > > +\n> > > > > > +\tconst uint8_t *plane(unsigned int plane) const;\n> > > > > > +\tuint8_t *plane(unsigned int plane);\n> > > > > > +};\n> > > > > > +\n> > > > > > +CameraBuffer::CameraBufferImpl::CameraBufferImpl(buffer_handle_t camera3Buffer,\n> > > > > > +\t\t\t\t\t\t int flags)\n> > > > > > {\n> > > > > > \tmaps_.reserve(camera3Buffer->numFds);\n> > > > > > \terror_ = 0;\n> > > > > > @@ -42,6 +56,70 @@ CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > > \t}\n> > > > > > }\n> > > > > >\n> > > > > > +CameraBuffer::CameraBufferImpl::~CameraBufferImpl()\n> > > > > > +{\n> > > > > > +}\n> > > > > > +\n> > > > > > +unsigned int CameraBuffer::CameraBufferImpl::numPlanes() const\n> > > > > > +{\n> > > > > > +\treturn maps_.size();\n> > > > > > +}\n> > > > > > +\n> > > > > > +ssize_t CameraBuffer::CameraBufferImpl::planeSize(unsigned int plane) const\n> > > > > > +{\n> > > > > > +\tif (plane >= maps_.size())\n> > > > > > +\t\treturn -EINVAL;\n> > > > > > +\n> > > > > > +\treturn maps_[plane].size();\n> > > > > > +}\n> > > > > > +\n> > > > > > +const uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane) const\n> > > > > > +{\n> > > > > > +\tif (plane >= maps_.size())\n> > > > > > +\t\treturn nullptr;\n> > > > > > +\n> > > > > > +\treturn maps_[plane].data();\n> > > > > > +}\n> > > > > > +\n> > > > > > +uint8_t *CameraBuffer::CameraBufferImpl::plane(unsigned int plane)\n> > > > > > +{\n> > > > > > +\tif (plane >= maps_.size())\n> > > > > > +\t\treturn nullptr;\n> > > > > > +\n> > > > > > +\treturn maps_[plane].data();\n> > > > > > +}\n> > > > > > +\n> > > > > > +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> > > > > > +\t: impl_(new CameraBuffer::CameraBufferImpl(camera3Buffer, flags))\n> > > > > > +{\n> > > > > > +}\n> > > > > > +\n> > > > > > CameraBuffer::~CameraBuffer()\n> > > > > > {\n> > > > > > +\tdelete impl_;\n> > > > > > +}\n> > > > > > +\n> > > > > > +bool CameraBuffer::isValid() const\n> > > > > > +{\n> > > > > > +\treturn impl_->isValid();\n> > > > > > +}\n> > > > > > +\n> > > > > > +unsigned int CameraBuffer::numPlanes() const\n> > > > > > +{\n> > > > > > +\treturn impl_->numPlanes();\n> > > > > > +}\n> > > > > > +\n> > > > > > +ssize_t CameraBuffer::planeSize(unsigned int plane) const\n> > > > > > +{\n> > > > > > +\treturn impl_->planeSize(plane);\n> > > > > > +}\n> > > > > > +\n> > > > > > +const uint8_t *CameraBuffer::plane(unsigned int plane) const\n> > > > > > +{\n> > > > > > +\treturn impl_->plane(plane);\n> > > > > > +}\n> > > > > > +\n> > > > > > +uint8_t *CameraBuffer::plane(unsigned int plane)\n> > > > > > +{\n> > > > > > +\treturn impl_->plane(plane);\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 661F1BD1F1\n\tfor ;\n\tMon, 1 Mar 2021 09:48:01 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3611F68A81;\n\tMon, 1 Mar 2021 10:48:01 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 59B1460521\n\tfor ;\n\tMon, 1 Mar 2021 10:47:59 +0100 (CET)","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 E45AF332;\n\tMon, 1 Mar 2021 10:47:58 +0100 (CET)"],"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=\"YAW9oF6j\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614592079;\n\tbh=x6kIN+mXI6Q+x2ziT3DQyrsgmc+2jkL5VLO7hCTt1hE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=YAW9oF6j1NhawzQvnBZLu9pi58PFJ+QrT3sEbPn7uBG7Cpv/TvLwoOo0dwYEoi984\n\tUHZukzgpZJmgMml0VTWkoVenkyQQZDWq6xIaCPapRK8JjuvDfuwcMlD5nhWloD8Hp2\n\tEWSZ/6pKfTIaJwEGJ1n7wYBoZJhgdbxSy7uAOwew=","Date":"Mon, 1 Mar 2021 11:47:31 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210226132932.165484-1-jacopo@jmondi.org>\n\t<20210226132932.165484-8-jacopo@jmondi.org>\n\t\n\t\n\t<20210301090208.pwfictniaxa4jhyz@uno.localdomain>\n\t\n\t<20210301094013.vsmwwicfzrly2ibd@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301094013.vsmwwicfzrly2ibd@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH 07/12] android: camera_buffer:\n\tImplement PIMPL pattern","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]