[{"id":20269,"web_url":"https://patchwork.libcamera.org/comment/20269/","msgid":"<20211018152952.qlqfvgpr7exhlm5o@uno.localdomain>","date":"2021-10-18T15:29:52","subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Oct 18, 2021 at 06:59:14PM +0530, Umang Jain wrote:\n> From: Laurent Pinchart \n>\n> The Camera3RequestDescriptor structure is growing into an object with\n> member functions. Turn it into a class, uninline the destructor to\n> reduce code size, explicitly disable copy as requests are not copyable,\n> and delete the default constructor to force all instances to be fully\n> constructed.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_device.h | 2 +-\n> src/android/camera_request.cpp | 8 +++++---\n> src/android/camera_request.h | 13 +++++++++----\n> 3 files changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 86224aa1..863cf414 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -33,7 +33,7 @@\n> #include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n>\n> -struct Camera3RequestDescriptor;\n> +class Camera3RequestDescriptor;\n> struct CameraConfigData;\n>\n> class CameraDevice : protected libcamera::Loggable\n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 93e546bf..16a632b3 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -10,10 +10,10 @@\n> using namespace libcamera;\n>\n> /*\n> - * \\struct Camera3RequestDescriptor\n> + * \\class Camera3RequestDescriptor\n> *\n> - * A utility structure that groups information about a capture request to be\n> - * later re-used at request complete time to notify the framework.\n> + * A utility class that groups information about a capture request to be later\n> + * reused at request complete time to notify the framework.\n> */\n>\n> Camera3RequestDescriptor::Camera3RequestDescriptor(\n> @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> \trequest_ = std::make_unique(camera,\n> \t\t\t\t\t\t reinterpret_cast(this));\n> }\n> +\n> +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n\nWe should have done this for most classes probably...\n\nAnyway,\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 1346f6fa..79dfdb58 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n> #include \n> #include \n>\n> @@ -18,18 +20,18 @@\n> #include \"camera_metadata.h\"\n> #include \"camera_worker.h\"\n>\n> -struct Camera3RequestDescriptor {\n> +class Camera3RequestDescriptor\n> +{\n> +public:\n> \tenum class Status {\n> \t\tPending,\n> \t\tSuccess,\n> \t\tError,\n> \t};\n>\n> -\tCamera3RequestDescriptor() = default;\n> -\t~Camera3RequestDescriptor() = default;\n> \tCamera3RequestDescriptor(libcamera::Camera *camera,\n> \t\t\t\t const camera3_capture_request_t *camera3Request);\n> -\tCamera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> +\t~Camera3RequestDescriptor();\n>\n> \tbool isPending() const { return status_ == Status::Pending; }\n>\n> @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {\n>\n> \tcamera3_capture_result_t captureResult_ = {};\n> \tStatus status_ = Status::Pending;\n> +\n> +private:\n> +\tLIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)\n> };\n>\n> #endif /* __ANDROID_CAMERA_REQUEST_H__ */\n> --\n> 2.31.0\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 DAF5AC323E\n\tfor ;\n\tMon, 18 Oct 2021 15:29:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 6A65E68F59;\n\tMon, 18 Oct 2021 17:29:05 +0200 (CEST)","from relay5-d.mail.gandi.net (relay5-d.mail.gandi.net\n\t[217.70.183.197])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8151968F56\n\tfor ;\n\tMon, 18 Oct 2021 17:29:04 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay5-d.mail.gandi.net (Postfix) with ESMTPSA id 05F4E1C0009;\n\tMon, 18 Oct 2021 15:29:03 +0000 (UTC)"],"Date":"Mon, 18 Oct 2021 17:29:52 +0200","From":"Jacopo Mondi ","To":"Umang Jain ","Message-ID":"<20211018152952.qlqfvgpr7exhlm5o@uno.localdomain>","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-3-umang.jain@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20211018132923.476242-3-umang.jain@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20288,"web_url":"https://patchwork.libcamera.org/comment/20288/","msgid":"","date":"2021-10-19T04:03:45","subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","submitter":{"id":63,"url":"https://patchwork.libcamera.org/api/people/63/","name":"Hirokazu Honda","email":"hiroh@chromium.org"},"content":"HI Umang and Laurent, thank you for the patch.\n\nOn Mon, Oct 18, 2021 at 10:29 PM Umang Jain wrote:\n>\n> From: Laurent Pinchart \n>\n> The Camera3RequestDescriptor structure is growing into an object with\n> member functions. Turn it into a class, uninline the destructor to\n> reduce code size, explicitly disable copy as requests are not copyable,\n> and delete the default constructor to force all instances to be fully\n> constructed.\n>\n> Signed-off-by: Laurent Pinchart \n> ---\n> src/android/camera_device.h | 2 +-\n> src/android/camera_request.cpp | 8 +++++---\n> src/android/camera_request.h | 13 +++++++++----\n> 3 files changed, 15 insertions(+), 8 deletions(-)\n>\n> diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> index 86224aa1..863cf414 100644\n> --- a/src/android/camera_device.h\n> +++ b/src/android/camera_device.h\n> @@ -33,7 +33,7 @@\n> #include \"camera_worker.h\"\n> #include \"jpeg/encoder.h\"\n>\n> -struct Camera3RequestDescriptor;\n> +class Camera3RequestDescriptor;\n> struct CameraConfigData;\n>\n> class CameraDevice : protected libcamera::Loggable\n> diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> index 93e546bf..16a632b3 100644\n> --- a/src/android/camera_request.cpp\n> +++ b/src/android/camera_request.cpp\n> @@ -10,10 +10,10 @@\n> using namespace libcamera;\n>\n> /*\n> - * \\struct Camera3RequestDescriptor\n> + * \\class Camera3RequestDescriptor\n> *\n> - * A utility structure that groups information about a capture request to be\n> - * later re-used at request complete time to notify the framework.\n> + * A utility class that groups information about a capture request to be later\n> + * reused at request complete time to notify the framework.\n> */\n>\n> Camera3RequestDescriptor::Camera3RequestDescriptor(\n> @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> request_ = std::make_unique(camera,\n> reinterpret_cast(this));\n> }\n> +\n> +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> index 1346f6fa..79dfdb58 100644\n> --- a/src/android/camera_request.h\n> +++ b/src/android/camera_request.h\n> @@ -10,6 +10,8 @@\n> #include \n> #include \n>\n> +#include \n> +\n> #include \n> #include \n>\n> @@ -18,18 +20,18 @@\n> #include \"camera_metadata.h\"\n> #include \"camera_worker.h\"\n>\n> -struct Camera3RequestDescriptor {\n> +class Camera3RequestDescriptor\n> +{\n> +public:\n> enum class Status {\n> Pending,\n> Success,\n> Error,\n> };\n>\n> - Camera3RequestDescriptor() = default;\n> - ~Camera3RequestDescriptor() = default;\n> Camera3RequestDescriptor(libcamera::Camera *camera,\n> const camera3_capture_request_t *camera3Request);\n> - Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> + ~Camera3RequestDescriptor();\n>\n> bool isPending() const { return status_ == Status::Pending; }\n>\n> @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {\n>\n> camera3_capture_result_t captureResult_ = {};\n> Status status_ = Status::Pending;\n> +\n> +private:\n\nThis private is unnecessary.\n\nReviewed-by: Hirokazu Honda \n\n> + LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)\n> };\n>\n> #endif /* __ANDROID_CAMERA_REQUEST_H__ */\n> --\n> 2.31.0\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 A483CC323E\n\tfor ;\n\tTue, 19 Oct 2021 04:03:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2830D68F58;\n\tTue, 19 Oct 2021 06:03:59 +0200 (CEST)","from mail-ed1-x536.google.com (mail-ed1-x536.google.com\n\t[IPv6:2a00:1450:4864:20::536])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 49D8460126\n\tfor ;\n\tTue, 19 Oct 2021 06:03:57 +0200 (CEST)","by mail-ed1-x536.google.com with SMTP id t16so7581593eds.9\n\tfor ;\n\tMon, 18 Oct 2021 21:03:57 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"fMgXjCwv\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=oYAmKGG1ivcHWtRsQYHpVqhw6zskycTpkGDFUQQRvvc=;\n\tb=fMgXjCwvj+3vCouSb36klKG6PyZGSb3+4U13TVWlVuMJ/A/4+5Xig8Bukcn3dTqVCJ\n\txOkP4N0aFvuccKrjVkJWKEhiVN8ANr8yynzOWhTWu14oBgspu8vddp/DplCF1Pm9kdBp\n\tbuSJCd4kbopA9MBn46pfYPwaDVGNba9/EhW9k=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=oYAmKGG1ivcHWtRsQYHpVqhw6zskycTpkGDFUQQRvvc=;\n\tb=YhhFBp4OLNPp/y9eAnmgnJaDh4mQfa43nvQ6WR82ItWkl5YtkObmsr0bsJg94Faix5\n\t4M6BInCTXRoH4+u8Z7DRzvlHUNeylyVh6b7BGQh15ykF/mx7FFmYSOyG+cOkPoAeN+Wk\n\tukeOHyKOflYG9c1ke3rb3fEwjZtGScO3LC2KGfHqS4Givi5zRN/agvwECxeO2trutd+b\n\tn19gsClAwc4HYSajBvbdiBc4qp9A+ouypRWLJNJGfWM7K7A6fx37jVqoSNh9vT32pqbv\n\t/cE0xXSg7MjBMsTXUR5uOoE/NPiE/PfeNEzOaAXi6DgTkolAOfPpsP+DagjYbeS7HNW/\n\theMA==","X-Gm-Message-State":"AOAM531SfZek5tKHUuIB58Po66rwY+4r+C7FyCLxwh5ICI9Yyd0ETs6m\n\tpsA+LjjqwAUpI59F5xC/on+gnbNipelwNm4iy9iyMhan4fs=","X-Google-Smtp-Source":"ABdhPJyMDiQAGPiLN4HgaXG+sold78y+PfBYRltkDXx5pKXY4SdkkGrAJj6J8bwiaNFqbO+s8yp8uvb0c2HfaftW6kc=","X-Received":"by 2002:a17:906:4fd6:: with SMTP id\n\ti22mr34478754ejw.92.1634616236913; \n\tMon, 18 Oct 2021 21:03:56 -0700 (PDT)","MIME-Version":"1.0","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-3-umang.jain@ideasonboard.com>","In-Reply-To":"<20211018132923.476242-3-umang.jain@ideasonboard.com>","From":"Hirokazu Honda ","Date":"Tue, 19 Oct 2021 13:03:45 +0900","Message-ID":"","To":"Umang Jain ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":20295,"web_url":"https://patchwork.libcamera.org/comment/20295/","msgid":"","date":"2021-10-19T06:44:13","subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Hiro,\n\nOn Tue, Oct 19, 2021 at 01:03:45PM +0900, Hirokazu Honda wrote:\n> On Mon, Oct 18, 2021 at 10:29 PM Umang Jain wrote:\n> >\n> > From: Laurent Pinchart \n> >\n> > The Camera3RequestDescriptor structure is growing into an object with\n> > member functions. Turn it into a class, uninline the destructor to\n> > reduce code size, explicitly disable copy as requests are not copyable,\n> > and delete the default constructor to force all instances to be fully\n> > constructed.\n> >\n> > Signed-off-by: Laurent Pinchart \n> > ---\n> > src/android/camera_device.h | 2 +-\n> > src/android/camera_request.cpp | 8 +++++---\n> > src/android/camera_request.h | 13 +++++++++----\n> > 3 files changed, 15 insertions(+), 8 deletions(-)\n> >\n> > diff --git a/src/android/camera_device.h b/src/android/camera_device.h\n> > index 86224aa1..863cf414 100644\n> > --- a/src/android/camera_device.h\n> > +++ b/src/android/camera_device.h\n> > @@ -33,7 +33,7 @@\n> > #include \"camera_worker.h\"\n> > #include \"jpeg/encoder.h\"\n> >\n> > -struct Camera3RequestDescriptor;\n> > +class Camera3RequestDescriptor;\n> > struct CameraConfigData;\n> >\n> > class CameraDevice : protected libcamera::Loggable\n> > diff --git a/src/android/camera_request.cpp b/src/android/camera_request.cpp\n> > index 93e546bf..16a632b3 100644\n> > --- a/src/android/camera_request.cpp\n> > +++ b/src/android/camera_request.cpp\n> > @@ -10,10 +10,10 @@\n> > using namespace libcamera;\n> >\n> > /*\n> > - * \\struct Camera3RequestDescriptor\n> > + * \\class Camera3RequestDescriptor\n> > *\n> > - * A utility structure that groups information about a capture request to be\n> > - * later re-used at request complete time to notify the framework.\n> > + * A utility class that groups information about a capture request to be later\n> > + * reused at request complete time to notify the framework.\n> > */\n> >\n> > Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > @@ -43,3 +43,5 @@ Camera3RequestDescriptor::Camera3RequestDescriptor(\n> > request_ = std::make_unique(camera,\n> > reinterpret_cast(this));\n> > }\n> > +\n> > +Camera3RequestDescriptor::~Camera3RequestDescriptor() = default;\n> > diff --git a/src/android/camera_request.h b/src/android/camera_request.h\n> > index 1346f6fa..79dfdb58 100644\n> > --- a/src/android/camera_request.h\n> > +++ b/src/android/camera_request.h\n> > @@ -10,6 +10,8 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +\n> > #include \n> > #include \n> >\n> > @@ -18,18 +20,18 @@\n> > #include \"camera_metadata.h\"\n> > #include \"camera_worker.h\"\n> >\n> > -struct Camera3RequestDescriptor {\n> > +class Camera3RequestDescriptor\n> > +{\n> > +public:\n> > enum class Status {\n> > Pending,\n> > Success,\n> > Error,\n> > };\n> >\n> > - Camera3RequestDescriptor() = default;\n> > - ~Camera3RequestDescriptor() = default;\n> > Camera3RequestDescriptor(libcamera::Camera *camera,\n> > const camera3_capture_request_t *camera3Request);\n> > - Camera3RequestDescriptor &operator=(Camera3RequestDescriptor &&) = default;\n> > + ~Camera3RequestDescriptor();\n> >\n> > bool isPending() const { return status_ == Status::Pending; }\n> >\n> > @@ -41,6 +43,9 @@ struct Camera3RequestDescriptor {\n> >\n> > camera3_capture_result_t captureResult_ = {};\n> > Status status_ = Status::Pending;\n> > +\n> > +private:\n> \n> This private is unnecessary.\n\nIt's not strictly necessary indeed, but we put the\nLIBCAMERA_DISABLE_COPY macro in a private block everywhere, mostly as a\ncoding convention.\n\n> Reviewed-by: Hirokazu Honda \n> \n> > + LIBCAMERA_DISABLE_COPY(Camera3RequestDescriptor)\n> > };\n> >\n> > #endif /* __ANDROID_CAMERA_REQUEST_H__ */","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 294E2C324C\n\tfor ;\n\tTue, 19 Oct 2021 06:44:34 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7D57A68F59;\n\tTue, 19 Oct 2021 08:44:33 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C4B1B60126\n\tfor ;\n\tTue, 19 Oct 2021 08:44:31 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3006912A;\n\tTue, 19 Oct 2021 08:44:31 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"jiUzhyYh\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1634625871;\n\tbh=k0BWusO1a1sjgdNE4nlYoezpBRVeBFj64XONHOUFbjs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=jiUzhyYhZ1A0SYPLwUFFzop6kT11Q4qQEmHj8xo0lBJBpgCvKLStJp8mnv/wmdWAX\n\tRm0HmKFxhGuEC/3IKQKHs7XTC9XVK6ZNMbCRYmdpYWsFmUY0w6FB2J+EAbMUhfh/ih\n\tKkdr2pRSdNl/qgz18aDliHEn75JtvjTVw7M4q37A=","Date":"Tue, 19 Oct 2021 09:44:13 +0300","From":"Laurent Pinchart ","To":"Hirokazu Honda ","Message-ID":"","References":"<20211018132923.476242-1-umang.jain@ideasonboard.com>\n\t<20211018132923.476242-3-umang.jain@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 02/11] android: camera_request: Turn\n\tstruct into a class","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]