[{"id":15372,"web_url":"https://patchwork.libcamera.org/comment/15372/","msgid":"","date":"2021-03-02T00:07:05","subject":"Re: [libcamera-devel] [PATCH 09/10] android: mm: Provide helper\n\tmacro for PIMPL","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 Mon, Mar 01, 2021 at 04:01:10PM +0100, Jacopo Mondi wrote:\n> Each memory backend has to declare a CameraBuffer class implementation\n> that bridges the API calls to each CameraBufferImpl implementation.\n> \n> As the code is likely the same for most (if not all) backends, provide\n> a convenience macro that expands to the CameraBuffer class declaration.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/android/camera_buffer.h | 28 ++++++++++++++++++++\n> src/android/mm/generic_camera_buffer.cpp | 33 +-----------------------\n> 2 files changed, 29 insertions(+), 32 deletions(-)\n> \n> diff --git a/src/android/camera_buffer.h b/src/android/camera_buffer.h\n> index 2311cdaf96b2..ea73bf5b0ede 100644\n> --- a/src/android/camera_buffer.h\n> +++ b/src/android/camera_buffer.h\n> @@ -28,4 +28,32 @@ public:\n> \tlibcamera::Span plane(unsigned int plane);\n> };\n> \n> +#define PUBLIC_CAMERA_BUFFER\t\t\t\t\t\t\\\n\nMaybe PUBLIC_CAMERA_BUFFER_IMPLEMENTATION ? Up to you.\n\n> +CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\t\\\n> +\t: Extensible(new Private(this, camera3Buffer, flags))\t\t\\\n> +{ \\\n> +}\t\t\t\t\t\t\t\t\t\\\n> +CameraBuffer::~CameraBuffer() \\\n> +{\t\t\t\t\t\t\t\t\t\\\n> +}\t\t\t\t\t\t\t \\\n> +bool CameraBuffer::isValid() const\t\t\t\t \\\n> +{ \\\n> +\tconst Private *const d = LIBCAMERA_D_PTR(); \\\n> +\treturn d->isValid(); \\\n> +}\t\t\t\t\t\t\t\t\t\\\n> +unsigned int CameraBuffer::numPlanes() const \\\n> +{ \\\n> +\tconst Private *const d = LIBCAMERA_D_PTR(); \\\n> +\treturn d->numPlanes(); \\\n> +} \\\n> +Span CameraBuffer::plane(unsigned int plane) const \\\n\nI think indentation is incorrect here.\n\n> +{\t\t\t\t\t\t\t\t\t\\\n> +\tconst Private *const d = LIBCAMERA_D_PTR(); \\\n> +\treturn d->plane(plane); \\\n> +} \\\n> +Span CameraBuffer::plane(unsigned int plane) \\\n\nAnd here too.\n\nI would normally say it would be nice to avoid duplicating the\nimplementation, but we only compile one of the .cpp file, so there's\nactually no duplication in the binary.\n\nReviewed-by: Laurent Pinchart \n\n> +{ \\\n> +\tPrivate *const d = LIBCAMERA_D_PTR();\t\t\t\t\\\n> +\treturn d->plane(plane);\t\t\t\t\t\t\\\n> +}\n> #endif /* __ANDROID_CAMERA_BUFFER_H__ */\n> diff --git a/src/android/mm/generic_camera_buffer.cpp b/src/android/mm/generic_camera_buffer.cpp\n> index ea85be805260..5ea1339bc0ec 100644\n> --- a/src/android/mm/generic_camera_buffer.cpp\n> +++ b/src/android/mm/generic_camera_buffer.cpp\n> @@ -86,35 +86,4 @@ Span CameraBuffer::Private::plane(unsigned int plane)\n> \treturn maps_[plane];\n> }\n> \n> -CameraBuffer::CameraBuffer(buffer_handle_t camera3Buffer, int flags)\n> -\t: Extensible(new Private(this, camera3Buffer, flags))\n> -{\n> -}\n> -\n> -CameraBuffer::~CameraBuffer()\n> -{\n> -}\n> -\n> -bool CameraBuffer::isValid() const\n> -{\n> -\tconst Private *const d = LIBCAMERA_D_PTR();\n> -\treturn d->isValid();\n> -}\n> -\n> -unsigned int CameraBuffer::numPlanes() const\n> -{\n> -\tconst Private *const d = LIBCAMERA_D_PTR();\n> -\treturn d->numPlanes();\n> -}\n> -\n> -Span CameraBuffer::plane(unsigned int plane) const\n> -{\n> -\tconst Private *const d = LIBCAMERA_D_PTR();\n> -\treturn d->plane(plane);\n> -}\n> -\n> -Span CameraBuffer::plane(unsigned int plane)\n> -{\n> -\tPrivate *const d = LIBCAMERA_D_PTR();\n> -\treturn d->plane(plane);\n> -}\n> +PUBLIC_CAMERA_BUFFER","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 6C302BD808\n\tfor ;\n\tTue, 2 Mar 2021 00:07:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id EEF9068A8F;\n\tTue, 2 Mar 2021 01:07:35 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 57006602E9\n\tfor ;\n\tTue, 2 Mar 2021 01:07:34 +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 D31ED45D;\n\tTue, 2 Mar 2021 01:07:33 +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=\"MLwyLATX\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1614643654;\n\tbh=L29pHs65xekn/U82EuCHF9lTO8l0vKJD1lLoUfQ3Y84=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=MLwyLATXfev+0oG4xNo6MXAjyFxxBZ5E8jaCbVKMmP5wO3jfxrXzPwy6th8hdit2/\n\tfaGQe5qpa5WwGCA1mGHEJhS1N7V086Q4odxN3nafuQqDALuYB49nKMeXtn89v+u8NG\n\tnGyY1BI+NlmFOw4T+crdBtEQxyuiCApgnfwgaMQI=","Date":"Tue, 2 Mar 2021 02:07:05 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210301150111.61791-1-jacopo@jmondi.org>\n\t<20210301150111.61791-10-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210301150111.61791-10-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH 09/10] android: mm: Provide helper\n\tmacro for PIMPL","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":"Han-lin Chen ,\n\tlibcamera-devel@lists.libcamera.org, Daniel Hung-yu Wu ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]