[{"id":18265,"web_url":"https://patchwork.libcamera.org/comment/18265/","msgid":"<20210722134201.24fqlyte46uvep4d@uno.localdomain>","date":"2021-07-22T13:42:01","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul.\n\nOn Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> Add convenience functions for checking if an entry is present in a\n> CameraMetadata instance, and to check if an array entry includes a\n> specific value.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v5:\n> - refactor template code to reduce the footprint of specialized template\n> code\n\nThe actual size of the generated binaries might actually be larger\nthan specializing the whole function.\n\nHowever, I think it's more readable, and we're really talking details,\nso\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> New in v4\n> ---\n> src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> src/android/camera_metadata.h | 14 ++++++++++++++\n> 2 files changed, 32 insertions(+)\n>\n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 3fc7cf27..ac86c5fc 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> \treturn true;\n> }\n>\n> +template<>\n> +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> +\t\t\t\t\t uint8_t value) const\n> +{\n> +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> +\t\tif (entry.data.u8[i] == value)\n> +\t\t\treturn true;\n> +\t}\n> +\n> +\treturn false;\n> +}\n> +\n> +bool CameraMetadata::hasEntry(uint32_t tag) const\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\treturn getEntry(tag, &entry);\n> +}\n> +\n> bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> \t\t\t size_t elementSize)\n> {\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index 3b7c9e24..72ba4db2 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -29,6 +29,20 @@ public:\n> \tbool isValid() const { return valid_; }\n> \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n>\n> +\ttemplate\n> +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n> +\n> +\ttemplate bool entryContains(uint32_t tag, T value) const\n> +\t{\n> +\t\tcamera_metadata_ro_entry_t entry;\n> +\t\tif (!getEntry(tag, &entry))\n> +\t\t\treturn false;\n> +\n> +\t\treturn entryContainsOne(entry, value);\n> +\t}\n> +\n> +\tbool hasEntry(uint32_t tag) const;\n> +\n> \ttemplate \t\t std::enable_if_t> * = nullptr>\n> \tbool addEntry(uint32_t tag, const T &data)\n> --\n> 2.27.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 6816AC322B\n\tfor ;\n\tThu, 22 Jul 2021 13:41:16 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C16F56027F;\n\tThu, 22 Jul 2021 15:41:15 +0200 (CEST)","from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net\n\t[217.70.183.195])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 53B1A6027A\n\tfor ;\n\tThu, 22 Jul 2021 15:41:14 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay3-d.mail.gandi.net (Postfix) with ESMTPSA id BDC0260003;\n\tThu, 22 Jul 2021 13:41:13 +0000 (UTC)"],"Date":"Thu, 22 Jul 2021 15:42:01 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210722134201.24fqlyte46uvep4d@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210720101307.26010-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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":18326,"web_url":"https://patchwork.libcamera.org/comment/18326/","msgid":"","date":"2021-07-25T00:37:42","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Paul,\n\nThank you for the patch.\n\nOn Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> Add convenience functions for checking if an entry is present in a\n> CameraMetadata instance, and to check if an array entry includes a\n> specific value.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v5:\n> - refactor template code to reduce the footprint of specialized template\n> code\n> \n> New in v4\n> ---\n> src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> src/android/camera_metadata.h | 14 ++++++++++++++\n> 2 files changed, 32 insertions(+)\n> \n> diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> index 3fc7cf27..ac86c5fc 100644\n> --- a/src/android/camera_metadata.cpp\n> +++ b/src/android/camera_metadata.cpp\n> @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> \treturn true;\n> }\n> \n> +template<>\n> +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> +\t\t\t\t\t uint8_t value) const\n> +{\n> +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> +\t\tif (entry.data.u8[i] == value)\n> +\t\t\treturn true;\n> +\t}\n> +\n> +\treturn false;\n> +}\n\nYou'll get a linker error if you call entryContains() with a type\ndifferent than uint8_t. Is there a use case for that ?\n\n> +\n> +bool CameraMetadata::hasEntry(uint32_t tag) const\n> +{\n> +\tcamera_metadata_ro_entry_t entry;\n> +\treturn getEntry(tag, &entry);\n> +}\n> +\n> bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> \t\t\t size_t elementSize)\n> {\n> diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> index 3b7c9e24..72ba4db2 100644\n> --- a/src/android/camera_metadata.h\n> +++ b/src/android/camera_metadata.h\n> @@ -29,6 +29,20 @@ public:\n> \tbool isValid() const { return valid_; }\n> \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> \n> +\ttemplate\n> +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n\nUnless you intend for this to be called directly, let's make it private.\n\n> +\n> +\ttemplate bool entryContains(uint32_t tag, T value) const\n> +\t{\n> +\t\tcamera_metadata_ro_entry_t entry;\n> +\t\tif (!getEntry(tag, &entry))\n> +\t\t\treturn false;\n> +\n> +\t\treturn entryContainsOne(entry, value);\n> +\t}\n\nSo the part you inline is the one that doesn't depend on the type T, and\nthe part you don't inline is that one that does ? That seems a bit\nbackwards, I'd move merge the two functions and move all the code to the\n.cpp file.\n\n> +\n> +\tbool hasEntry(uint32_t tag) const;\n> +\n> \ttemplate \t\t std::enable_if_t> * = nullptr>\n> \tbool addEntry(uint32_t tag, const T &data)","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 10FF1C0109\n\tfor ;\n\tSun, 25 Jul 2021 00:37:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 19B21687B5;\n\tSun, 25 Jul 2021 02:37:48 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id A169B60274\n\tfor ;\n\tSun, 25 Jul 2021 02:37:46 +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 1844D255;\n\tSun, 25 Jul 2021 02:37:46 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"PpcdgvxV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627173466;\n\tbh=HvYOOASUzIRG2nqwjeGQLLklzXCVh4lmN2YC45qQQoQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PpcdgvxVzxnZqsytOOZSb8fK3zKuwovLLRVGdD5y5ozYW9dGNS1McHQXMCcEYmLWz\n\tMeEAxkxKC4TKtaEV6hbKkqNHKikRRzj9F/uEzKMtGyCzoKt6t/MjdAXaCDoWGSr1FF\n\tFSozNUEsdAv5drWvj1BynL+SDTuYIykVThG3dysw=","Date":"Sun, 25 Jul 2021 03:37:42 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210720101307.26010-3-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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":18338,"web_url":"https://patchwork.libcamera.org/comment/18338/","msgid":"<20210726081707.kmqooch44psam43g@uno.localdomain>","date":"2021-07-26T08:17:07","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi LAurent,\n\nOn Sun, Jul 25, 2021 at 03:37:42AM +0300, Laurent Pinchart wrote:\n> Hi Paul,\n>\n> Thank you for the patch.\n>\n> On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> > Add convenience functions for checking if an entry is present in a\n> > CameraMetadata instance, and to check if an array entry includes a\n> > specific value.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v5:\n> > - refactor template code to reduce the footprint of specialized template\n> > code\n> >\n> > New in v4\n> > ---\n> > src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> > src/android/camera_metadata.h | 14 ++++++++++++++\n> > 2 files changed, 32 insertions(+)\n> >\n> > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > index 3fc7cf27..ac86c5fc 100644\n> > --- a/src/android/camera_metadata.cpp\n> > +++ b/src/android/camera_metadata.cpp\n> > @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> > \treturn true;\n> > }\n> >\n> > +template<>\n> > +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > +\t\t\t\t\t uint8_t value) const\n> > +{\n> > +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> > +\t\tif (entry.data.u8[i] == value)\n> > +\t\t\treturn true;\n> > +\t}\n> > +\n> > +\treturn false;\n> > +}\n>\n> You'll get a linker error if you call entryContains() with a type\n> different than uint8_t. Is there a use case for that ?\n>\n\nWhy is this an issue ? As one needs more specialization they will be\nadded, otherwise should the function be specialized for all types even\nif not used ? There are not many many types so that could be done\nalready, but I don't think it's strictly necessary.\n\n> > +\n> > +bool CameraMetadata::hasEntry(uint32_t tag) const\n> > +{\n> > +\tcamera_metadata_ro_entry_t entry;\n> > +\treturn getEntry(tag, &entry);\n> > +}\n> > +\n> > bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > \t\t\t size_t elementSize)\n> > {\n> > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > index 3b7c9e24..72ba4db2 100644\n> > --- a/src/android/camera_metadata.h\n> > +++ b/src/android/camera_metadata.h\n> > @@ -29,6 +29,20 @@ public:\n> > \tbool isValid() const { return valid_; }\n> > \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> >\n> > +\ttemplate\n> > +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n>\n> Unless you intend for this to be called directly, let's make it private.\n>\n> > +\n> > +\ttemplate bool entryContains(uint32_t tag, T value) const\n> > +\t{\n> > +\t\tcamera_metadata_ro_entry_t entry;\n> > +\t\tif (!getEntry(tag, &entry))\n> > +\t\t\treturn false;\n> > +\n> > +\t\treturn entryContainsOne(entry, value);\n> > +\t}\n>\n> So the part you inline is the one that doesn't depend on the type T, and\n> the part you don't inline is that one that does ? That seems a bit\n> backwards, I'd move merge the two functions and move all the code to the\n> .cpp file.\n>\n\nI suggested Paul to inline the common part and only specialize the\ntype-dependent part. That's why I was asking about the memory\nfootprint.\n\nHow would you do it backwards ?\n\nOtherwise yes, merge all in the .cpp file but be aware that the\nentryContains() function content will be replicated in each\nspecialization ?\n\n> > +\n> > +\tbool hasEntry(uint32_t tag) const;\n> > +\n> > \ttemplate > \t\t std::enable_if_t> * = nullptr>\n> > \tbool addEntry(uint32_t tag, const T &data)\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 647D1C322C\n\tfor ;\n\tMon, 26 Jul 2021 08:16:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D28E2687BA;\n\tMon, 26 Jul 2021 10:16:22 +0200 (CEST)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1787A687B3\n\tfor ;\n\tMon, 26 Jul 2021 10:16:21 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id F18ECC000D;\n\tMon, 26 Jul 2021 08:16:19 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 10:17:07 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726081707.kmqooch44psam43g@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@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 v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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":18341,"web_url":"https://patchwork.libcamera.org/comment/18341/","msgid":"","date":"2021-07-26T08:42:41","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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, Jul 26, 2021 at 10:17:07AM +0200, Jacopo Mondi wrote:\n> On Sun, Jul 25, 2021 at 03:37:42AM +0300, Laurent Pinchart wrote:\n> > On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> > > Add convenience functions for checking if an entry is present in a\n> > > CameraMetadata instance, and to check if an array entry includes a\n> > > specific value.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v5:\n> > > - refactor template code to reduce the footprint of specialized template\n> > > code\n> > >\n> > > New in v4\n> > > ---\n> > > src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> > > src/android/camera_metadata.h | 14 ++++++++++++++\n> > > 2 files changed, 32 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > > index 3fc7cf27..ac86c5fc 100644\n> > > --- a/src/android/camera_metadata.cpp\n> > > +++ b/src/android/camera_metadata.cpp\n> > > @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> > > \treturn true;\n> > > }\n> > >\n> > > +template<>\n> > > +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > > +\t\t\t\t\t uint8_t value) const\n> > > +{\n> > > +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> > > +\t\tif (entry.data.u8[i] == value)\n> > > +\t\t\treturn true;\n> > > +\t}\n> > > +\n> > > +\treturn false;\n> > > +}\n> >\n> > You'll get a linker error if you call entryContains() with a type\n> > different than uint8_t. Is there a use case for that ?\n> \n> Why is this an issue ? As one needs more specialization they will be\n> added, otherwise should the function be specialized for all types even\n> if not used ? There are not many many types so that could be done\n> already, but I don't think it's strictly necessary.\n\nNot necessarily, but I'd like to make sure I understand the foreseen use\ncases in order to review this correctly.\n\n> > > +\n> > > +bool CameraMetadata::hasEntry(uint32_t tag) const\n> > > +{\n> > > +\tcamera_metadata_ro_entry_t entry;\n> > > +\treturn getEntry(tag, &entry);\n> > > +}\n> > > +\n> > > bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > > \t\t\t size_t elementSize)\n> > > {\n> > > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > > index 3b7c9e24..72ba4db2 100644\n> > > --- a/src/android/camera_metadata.h\n> > > +++ b/src/android/camera_metadata.h\n> > > @@ -29,6 +29,20 @@ public:\n> > > \tbool isValid() const { return valid_; }\n> > > \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> > >\n> > > +\ttemplate\n> > > +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n> >\n> > Unless you intend for this to be called directly, let's make it private.\n> >\n> > > +\n> > > +\ttemplate bool entryContains(uint32_t tag, T value) const\n> > > +\t{\n> > > +\t\tcamera_metadata_ro_entry_t entry;\n> > > +\t\tif (!getEntry(tag, &entry))\n> > > +\t\t\treturn false;\n> > > +\n> > > +\t\treturn entryContainsOne(entry, value);\n> > > +\t}\n> >\n> > So the part you inline is the one that doesn't depend on the type T, and\n> > the part you don't inline is that one that does ? That seems a bit\n> > backwards, I'd move merge the two functions and move all the code to the\n> > .cpp file.\n> \n> I suggested Paul to inline the common part and only specialize the\n> type-dependent part. That's why I was asking about the memory\n> footprint.\n> \n> How would you do it backwards ?\n> \n> Otherwise yes, merge all in the .cpp file but be aware that the\n> entryContains() function content will be replicated in each\n> specialization ?\n\nIsn't it better than replicating it in each call site in the binary ?\n:-) Especially if we don't expect further specializations.\n\n> > > +\n> > > +\tbool hasEntry(uint32_t tag) const;\n> > > +\n> > > \ttemplate > > \t\t std::enable_if_t> * = nullptr>\n> > > \tbool addEntry(uint32_t tag, const T &data)","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 824A7C322C\n\tfor ;\n\tMon, 26 Jul 2021 08:42:48 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F0030687B8;\n\tMon, 26 Jul 2021 10:42:47 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C36FA687B3\n\tfor ;\n\tMon, 26 Jul 2021 10:42:46 +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 33D663F;\n\tMon, 26 Jul 2021 10:42:46 +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=\"E05/mKF3\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627288966;\n\tbh=dSzfX0YcMsTCLSTjyeRMvSxdRG9nsqIirn3crGfdxOA=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=E05/mKF3NZYr+jodROIhS/sCW3p7gRM2JfJlzqhGhipmq/Ctah21k3+PHx2D5ftS0\n\tbSSRfuBR4jgQrZpwfzIMBW+095hmNF1zzO+JYe94LpWGIVsa7jDmIf7Fwb4LDmUysW\n\tJaw0vcLE9OkMD+t4YdJzyXe8NaGDWnPlIJlbKqvE=","Date":"Mon, 26 Jul 2021 11:42:41 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@ideasonboard.com>\n\t\n\t<20210726081707.kmqooch44psam43g@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210726081707.kmqooch44psam43g@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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":18360,"web_url":"https://patchwork.libcamera.org/comment/18360/","msgid":"<20210726123433.3gc2vbpvftyrqp3i@uno.localdomain>","date":"2021-07-26T12:34:33","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Mon, Jul 26, 2021 at 11:42:41AM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 26, 2021 at 10:17:07AM +0200, Jacopo Mondi wrote:\n> > On Sun, Jul 25, 2021 at 03:37:42AM +0300, Laurent Pinchart wrote:\n> > > On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> > > > Add convenience functions for checking if an entry is present in a\n> > > > CameraMetadata instance, and to check if an array entry includes a\n> > > > specific value.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v5:\n> > > > - refactor template code to reduce the footprint of specialized template\n> > > > code\n> > > >\n> > > > New in v4\n> > > > ---\n> > > > src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> > > > src/android/camera_metadata.h | 14 ++++++++++++++\n> > > > 2 files changed, 32 insertions(+)\n> > > >\n> > > > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > > > index 3fc7cf27..ac86c5fc 100644\n> > > > --- a/src/android/camera_metadata.cpp\n> > > > +++ b/src/android/camera_metadata.cpp\n> > > > @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> > > > \treturn true;\n> > > > }\n> > > >\n> > > > +template<>\n> > > > +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > > > +\t\t\t\t\t uint8_t value) const\n> > > > +{\n> > > > +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> > > > +\t\tif (entry.data.u8[i] == value)\n> > > > +\t\t\treturn true;\n> > > > +\t}\n> > > > +\n> > > > +\treturn false;\n> > > > +}\n> > >\n> > > You'll get a linker error if you call entryContains() with a type\n> > > different than uint8_t. Is there a use case for that ?\n> >\n> > Why is this an issue ? As one needs more specialization they will be\n> > added, otherwise should the function be specialized for all types even\n> > if not used ? There are not many many types so that could be done\n> > already, but I don't think it's strictly necessary.\n>\n> Not necessarily, but I'd like to make sure I understand the foreseen use\n> cases in order to review this correctly.\n>\n\nwell, it's just about using a different member of the\ncamera_metadata_ro_entry_t.data union. Or do you expect more ?\n\n> > > > +\n> > > > +bool CameraMetadata::hasEntry(uint32_t tag) const\n> > > > +{\n> > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > +\treturn getEntry(tag, &entry);\n> > > > +}\n> > > > +\n> > > > bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > > > \t\t\t size_t elementSize)\n> > > > {\n> > > > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > > > index 3b7c9e24..72ba4db2 100644\n> > > > --- a/src/android/camera_metadata.h\n> > > > +++ b/src/android/camera_metadata.h\n> > > > @@ -29,6 +29,20 @@ public:\n> > > > \tbool isValid() const { return valid_; }\n> > > > \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> > > >\n> > > > +\ttemplate\n> > > > +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n> > >\n> > > Unless you intend for this to be called directly, let's make it private.\n> > >\n> > > > +\n> > > > +\ttemplate bool entryContains(uint32_t tag, T value) const\n> > > > +\t{\n> > > > +\t\tcamera_metadata_ro_entry_t entry;\n> > > > +\t\tif (!getEntry(tag, &entry))\n> > > > +\t\t\treturn false;\n> > > > +\n> > > > +\t\treturn entryContainsOne(entry, value);\n> > > > +\t}\n> > >\n> > > So the part you inline is the one that doesn't depend on the type T, and\n> > > the part you don't inline is that one that does ? That seems a bit\n> > > backwards, I'd move merge the two functions and move all the code to the\n> > > .cpp file.\n> >\n> > I suggested Paul to inline the common part and only specialize the\n> > type-dependent part. That's why I was asking about the memory\n> > footprint.\n> >\n> > How would you do it backwards ?\n> >\n> > Otherwise yes, merge all in the .cpp file but be aware that the\n> > entryContains() function content will be replicated in each\n> > specialization ?\n>\n> Isn't it better than replicating it in each call site in the binary ?\n> :-) Especially if we don't expect further specializations.\n>\n\nConsidering how small that part is I thought having it in a way nicer\nto read was worth the extra code inserted in the calling place.\nAnyway, we're really talking about details, I'm just sorry for Paul\nwhich will have to backtrack due to being misleaded by my suggestion.\n\n> > > > +\n> > > > +\tbool hasEntry(uint32_t tag) const;\n> > > > +\n> > > > \ttemplate > > > \t\t std::enable_if_t> * = nullptr>\n> > > > \tbool addEntry(uint32_t tag, const T &data)\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 4A0ABC322C\n\tfor ;\n\tMon, 26 Jul 2021 12:33:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8FDB2687BE;\n\tMon, 26 Jul 2021 14:33:48 +0200 (CEST)","from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net\n\t[217.70.183.193])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5CBED687AF\n\tfor ;\n\tMon, 26 Jul 2021 14:33:47 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 6267524000B;\n\tMon, 26 Jul 2021 12:33:45 +0000 (UTC)"],"Date":"Mon, 26 Jul 2021 14:34:33 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210726123433.3gc2vbpvftyrqp3i@uno.localdomain>","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@ideasonboard.com>\n\t\n\t<20210726081707.kmqooch44psam43g@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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":18365,"web_url":"https://patchwork.libcamera.org/comment/18365/","msgid":"","date":"2021-07-26T16:28:52","subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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, Jul 26, 2021 at 02:34:33PM +0200, Jacopo Mondi wrote:\n> On Mon, Jul 26, 2021 at 11:42:41AM +0300, Laurent Pinchart wrote:\n> > On Mon, Jul 26, 2021 at 10:17:07AM +0200, Jacopo Mondi wrote:\n> > > On Sun, Jul 25, 2021 at 03:37:42AM +0300, Laurent Pinchart wrote:\n> > > > On Tue, Jul 20, 2021 at 07:13:00PM +0900, Paul Elder wrote:\n> > > > > Add convenience functions for checking if an entry is present in a\n> > > > > CameraMetadata instance, and to check if an array entry includes a\n> > > > > specific value.\n> > > > >\n> > > > > Signed-off-by: Paul Elder \n> > > > >\n> > > > > ---\n> > > > > Changes in v5:\n> > > > > - refactor template code to reduce the footprint of specialized template\n> > > > > code\n> > > > >\n> > > > > New in v4\n> > > > > ---\n> > > > > src/android/camera_metadata.cpp | 18 ++++++++++++++++++\n> > > > > src/android/camera_metadata.h | 14 ++++++++++++++\n> > > > > 2 files changed, 32 insertions(+)\n> > > > >\n> > > > > diff --git a/src/android/camera_metadata.cpp b/src/android/camera_metadata.cpp\n> > > > > index 3fc7cf27..ac86c5fc 100644\n> > > > > --- a/src/android/camera_metadata.cpp\n> > > > > +++ b/src/android/camera_metadata.cpp\n> > > > > @@ -121,6 +121,24 @@ bool CameraMetadata::resize(size_t count, size_t size)\n> > > > > \treturn true;\n> > > > > }\n> > > > >\n> > > > > +template<>\n> > > > > +bool CameraMetadata::entryContainsOne(const camera_metadata_ro_entry_t &entry,\n> > > > > +\t\t\t\t\t uint8_t value) const\n> > > > > +{\n> > > > > +\tfor (unsigned int i = 0; i < entry.count; i++) {\n> > > > > +\t\tif (entry.data.u8[i] == value)\n> > > > > +\t\t\treturn true;\n> > > > > +\t}\n> > > > > +\n> > > > > +\treturn false;\n> > > > > +}\n> > > >\n> > > > You'll get a linker error if you call entryContains() with a type\n> > > > different than uint8_t. Is there a use case for that ?\n> > >\n> > > Why is this an issue ? As one needs more specialization they will be\n> > > added, otherwise should the function be specialized for all types even\n> > > if not used ? There are not many many types so that could be done\n> > > already, but I don't think it's strictly necessary.\n> >\n> > Not necessarily, but I'd like to make sure I understand the foreseen use\n> > cases in order to review this correctly.\n> \n> well, it's just about using a different member of the\n> camera_metadata_ro_entry_t.data union. Or do you expect more ?\n\nI don't expect more, it's the contrary actually. I'm wondering if we'll\never have a use case for entryContains() on a tag that has a different\ntype than u8. If that's not foreseen, maybe we can make this a\nnon-template function, and return false if the entry type isn't u8.\n\n> > > > > +\n> > > > > +bool CameraMetadata::hasEntry(uint32_t tag) const\n> > > > > +{\n> > > > > +\tcamera_metadata_ro_entry_t entry;\n> > > > > +\treturn getEntry(tag, &entry);\n> > > > > +}\n> > > > > +\n> > > > > bool CameraMetadata::addEntry(uint32_t tag, const void *data, size_t count,\n> > > > > \t\t\t size_t elementSize)\n> > > > > {\n> > > > > diff --git a/src/android/camera_metadata.h b/src/android/camera_metadata.h\n> > > > > index 3b7c9e24..72ba4db2 100644\n> > > > > --- a/src/android/camera_metadata.h\n> > > > > +++ b/src/android/camera_metadata.h\n> > > > > @@ -29,6 +29,20 @@ public:\n> > > > > \tbool isValid() const { return valid_; }\n> > > > > \tbool getEntry(uint32_t tag, camera_metadata_ro_entry_t *entry) const;\n> > > > >\n> > > > > +\ttemplate\n> > > > > +\tbool entryContainsOne(const camera_metadata_ro_entry_t &entry, T value) const;\n> > > >\n> > > > Unless you intend for this to be called directly, let's make it private.\n> > > >\n> > > > > +\n> > > > > +\ttemplate bool entryContains(uint32_t tag, T value) const\n> > > > > +\t{\n> > > > > +\t\tcamera_metadata_ro_entry_t entry;\n> > > > > +\t\tif (!getEntry(tag, &entry))\n> > > > > +\t\t\treturn false;\n> > > > > +\n> > > > > +\t\treturn entryContainsOne(entry, value);\n> > > > > +\t}\n> > > >\n> > > > So the part you inline is the one that doesn't depend on the type T, and\n> > > > the part you don't inline is that one that does ? That seems a bit\n> > > > backwards, I'd move merge the two functions and move all the code to the\n> > > > .cpp file.\n> > >\n> > > I suggested Paul to inline the common part and only specialize the\n> > > type-dependent part. That's why I was asking about the memory\n> > > footprint.\n> > >\n> > > How would you do it backwards ?\n> > >\n> > > Otherwise yes, merge all in the .cpp file but be aware that the\n> > > entryContains() function content will be replicated in each\n> > > specialization ?\n> >\n> > Isn't it better than replicating it in each call site in the binary ?\n> > :-) Especially if we don't expect further specializations.\n> \n> Considering how small that part is I thought having it in a way nicer\n> to read was worth the extra code inserted in the calling place.\n> Anyway, we're really talking about details, I'm just sorry for Paul\n> which will have to backtrack due to being misleaded by my suggestion.\n\nPlease let me take the blame for not having followed up on the\ndiscussion in previous versions. I'm sorry.\n\n> > > > > +\n> > > > > +\tbool hasEntry(uint32_t tag) const;\n> > > > > +\n> > > > > \ttemplate > > > > \t\t std::enable_if_t> * = nullptr>\n> > > > > \tbool addEntry(uint32_t tag, const T &data)","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 481BDC0109\n\tfor ;\n\tMon, 26 Jul 2021 16:28:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD2CD687BE;\n\tMon, 26 Jul 2021 18:28:58 +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 26BD2687AF\n\tfor ;\n\tMon, 26 Jul 2021 18:28:57 +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 82FA3332;\n\tMon, 26 Jul 2021 18:28:56 +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=\"Qbozw36d\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627316936;\n\tbh=jsiQLnIqE4mJB5yncUkZ2YXx/fSWHQWHoA92tUogqKw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Qbozw36de37tuJ8069STxevgGzJdbKLKsppdz9TpGW0X5DNM1YEWDI2fLh0Euctum\n\tXLN/JA1QFTF/ODHeWHXMrTC/NHFV1Szzap6PWUvSTiAo1LRUNH+qc3rXfjh8mQcfYX\n\tawhAs+sb6ZQCB4EVZAzMEwrqBWsOtQxrb35kkFtI=","Date":"Mon, 26 Jul 2021 19:28:52 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210720101307.26010-1-paul.elder@ideasonboard.com>\n\t<20210720101307.26010-3-paul.elder@ideasonboard.com>\n\t\n\t<20210726081707.kmqooch44psam43g@uno.localdomain>\n\t\n\t<20210726123433.3gc2vbpvftyrqp3i@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210726123433.3gc2vbpvftyrqp3i@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v5 2/9] android: metadata: Add\n\thasEntry and entryContains","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\" "}}]