[{"id":17988,"web_url":"https://patchwork.libcamera.org/comment/17988/","msgid":"<20210705154458.fabapieiagsygrc6@uno.localdomain>","date":"2021-07-05T15:44:58","subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:\n> Add helpers for setting android metadata from libcamera controls.\n>\n> There are two versions, for scalars and collections, both of which take\n> a default value to fill in the android control if the libcamera control\n> is not found. A version for scalars exists for no default, to not set\n> the android control at all if it is not found in libcamera. The\n> functions take two template parameters, the first for the android type,\n> and the second for the libcamera type of the control. They can be\n> different, for example, if the former is an enum and the latter is a\n> boolean, or if the former is an enum (uint8_t) and the latter is an enum\n> (int32_t).\n>\n> The versions that take a default value return the value that was set in\n> the android metadata.\n>\n> Signed-off-by: Paul Elder \n>\n> ---\n> Changes in v3:\n> - setMetadata for collection only works with vectors\n> - change enum to enum class\n> - add two template parameters for android type and libcamera type\n> - add docs\n>\n> New in v2\n>\n> TODO: make ControlList versions so that we can use them in result\n> metadata\n> ---\n> src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++\n> 1 file changed, 137 insertions(+)\n>\n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 54bd71da..1d4c44ce 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -124,6 +124,143 @@ hwLevelStrings = {\n> \t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> };\n>\n> +enum class ControlRange {\n> +\tMin,\n> +\tDef,\n> +\tMax,\n> +};\n> +\n> +/**\n\nWe don't parse the HAL with doxygen so you can spare the additional *\nBut it's more than fine to use the Doxygen syntax when documenting\nthings imho, mostly for consistency\n\n> + * \\brief Set android metadata from libcamera ControlInfo\n> + * \\tparam T Type of the control in android\n> + * \\tparam V Type of the control in libcamera\n> + * \\param[in] metadata Android metadata to add the control value to\n\ns/Android metadata/Android metadata pack/\n\n> + * \\param[in] tag Android metadata tag to add\n\ns/to add// ?\n\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n\nSet the android metadata \\a tag in the \\a metadata pack based on\nthe...\n\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the function returns without modifying anything.\n> + *\n> + * This function is for scalar values.\n> + */\n> +template +\t std::enable_if_t> * = nullptr,\n\nYou should include \n\nDo you need this to solve which overload ? I see three functions here\nwith the following prototypes\n\nvoid setMetadata(CameraMetadata *metadata, uint32_t tag,\n\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n\t\t enum ControlRange controlRange)\n\nT setMetadata(CameraMetadata *metadata, uint32_t tag,\n\t const ControlInfoMap &controlsInfo, const ControlId *control,\n\t enum ControlRange controlRange, const V defaultValue)\n\nstd::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n\t\t\t const ControlInfoMap &controlsInfo,\n\t\t\t const ControlId *control,\n\t\t\t const std::vector &defaultVector)\n\nThere doesn't seem to be any need to distinguish them on the template parameter\ntype.\n\n> +\t typename V>\n> +void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> +\t\t enum ControlRange controlRange)\n> +{\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info == controlsInfo.end())\n> +\t\treturn;\n> +\n> +\tT ret;\n> +\tswitch (controlRange) {\n> +\tcase ControlRange::Min:\n> +\t\tret = info->second.min().get();\n> +\t\tbreak;\n> +\tcase ControlRange::Def:\n> +\t\tret = info->second.def().get();\n> +\t\tbreak;\n> +\tcase ControlRange::Max:\n> +\t\tret = info->second.max().get();\n> +\t\tbreak;\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn;\n> +}\n> +\n> +/**\n> + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> + * \\tparam T Type of the control in android\n> + * \\tparam U Type of the control in libcamera\n> + * \\param[in] metadata Android metadata to add the control value to\n> + * \\param[in] tag Android metadata tag to add\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> + * \\param[in] defaultValue The value to set in \\a metadata if \\a control is not found\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the android metadata entry is set to \\a defaultValue.\n> + *\n> + * This function is for scalar values.\n> + */\n> +template +\t typename U,\n> +\t typename V,\n\nIsn't V the same type as T ?\n\n> +\t std::enable_if_t> * = nullptr>\n\nThis is probably functionally equivalent for now, but in the previous\nversion you were checking for the type T to be an arithmetic type.\n\n> +T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> +\t enum ControlRange controlRange, const V defaultValue)\n> +{\n> +\tT ret = defaultValue;\n> +\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info != controlsInfo.end()) {\n> +\t\tswitch (controlRange) {\n> +\t\tcase ControlRange::Min:\n> +\t\t\tret = info->second.min().get();\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Def:\n> +\t\t\tret = info->second.def().get();\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Max:\n> +\t\t\tret = info->second.max().get();\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> + * \\tparam T Type of the control in android\n> + * \\tparam V Type of the control in libcamera\n> + * \\param[in] metadata Android metadata to add the control value to\n> + * \\param[in] tag Android metadata tag to add\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] defaultVector The value to set in \\a metadata if \\a control is not found\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the android metadata entry is set to \\a defaultVector.\n> + *\n> + * This function is for vector values.\n> + */\n> +template +\t typename V>\n\nFits on one line\n\n> +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t\t\t const ControlInfoMap &controlsInfo,\n> +\t\t\t const ControlId *control,\n> +\t\t\t const std::vector &defaultVector)\n> +{\n> +\tstd::vector ret = {};\n> +\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info != controlsInfo.end()) {\n> +\t\tret.reserve(info->second.values().size());\n> +\t\tfor (const auto &value : info->second.values())\n> +\t\t\tret.push_back(value.get());\n> +\t} else {\n> +\t\tret = defaultVector;\n\nthis is a copy that could be avoided by calling addEntry() in each\nbranch. Something like\n\n\tconst auto &info = controlsInfo.find(control);\n\tif (info == controlsInfo.end()) {\n metadata->addEntry(tag, defaultVector);\n return defaultVector;\n }\n\n std::vector ret(info->second.values().size());\n for (const auto &value : info->second.values())\n ret.push_back(value.get());\n metadata->addEntry(tag, ret);\n\n return ret;\n\n\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn ret;\n> +}\n> +\n> } /* namespace */\n>\n> int CameraCapabilities::initialize(std::shared_ptr camera,\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 51AECC0100\n\tfor ;\n\tMon, 5 Jul 2021 15:44:11 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id AD37A68503;\n\tMon, 5 Jul 2021 17:44:10 +0200 (CEST)","from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8EDD86050A\n\tfor ;\n\tMon, 5 Jul 2021 17:44:09 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id ECB52E000B;\n\tMon, 5 Jul 2021 15:44:08 +0000 (UTC)"],"Date":"Mon, 5 Jul 2021 17:44:58 +0200","From":"Jacopo Mondi ","To":"Paul Elder ","Message-ID":"<20210705154458.fabapieiagsygrc6@uno.localdomain>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210702103800.41291-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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":18036,"web_url":"https://patchwork.libcamera.org/comment/18036/","msgid":"<20210709044210.GA1947@pyrite.rasen.tech>","date":"2021-07-09T04:43:06","subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Jul 05, 2021 at 05:44:58PM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:\n> > Add helpers for setting android metadata from libcamera controls.\n> >\n> > There are two versions, for scalars and collections, both of which take\n> > a default value to fill in the android control if the libcamera control\n> > is not found. A version for scalars exists for no default, to not set\n> > the android control at all if it is not found in libcamera. The\n> > functions take two template parameters, the first for the android type,\n> > and the second for the libcamera type of the control. They can be\n> > different, for example, if the former is an enum and the latter is a\n> > boolean, or if the former is an enum (uint8_t) and the latter is an enum\n> > (int32_t).\n> >\n> > The versions that take a default value return the value that was set in\n> > the android metadata.\n> >\n> > Signed-off-by: Paul Elder \n> >\n> > ---\n> > Changes in v3:\n> > - setMetadata for collection only works with vectors\n> > - change enum to enum class\n> > - add two template parameters for android type and libcamera type\n> > - add docs\n> >\n> > New in v2\n> >\n> > TODO: make ControlList versions so that we can use them in result\n> > metadata\n> > ---\n> > src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++\n> > 1 file changed, 137 insertions(+)\n> >\n> > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > index 54bd71da..1d4c44ce 100644\n> > --- a/src/android/camera_capabilities.cpp\n> > +++ b/src/android/camera_capabilities.cpp\n> > @@ -124,6 +124,143 @@ hwLevelStrings = {\n> > \t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > };\n> >\n> > +enum class ControlRange {\n> > +\tMin,\n> > +\tDef,\n> > +\tMax,\n> > +};\n> > +\n> > +/**\n> \n> We don't parse the HAL with doxygen so you can spare the additional *\n> But it's more than fine to use the Doxygen syntax when documenting\n> things imho, mostly for consistency\n> \n> > + * \\brief Set android metadata from libcamera ControlInfo\n> > + * \\tparam T Type of the control in android\n> > + * \\tparam V Type of the control in libcamera\n> > + * \\param[in] metadata Android metadata to add the control value to\n> \n> s/Android metadata/Android metadata pack/\n> \n> > + * \\param[in] tag Android metadata tag to add\n> \n> s/to add// ?\n> \n> > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > + *\n> > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> \n> Set the android metadata \\a tag in the \\a metadata pack based on\n> the...\n> \n> > + * control info found for the libcamera control \\a control in the libcamera\n> > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > + * the function returns without modifying anything.\n> > + *\n> > + * This function is for scalar values.\n> > + */\n> > +template > +\t std::enable_if_t> * = nullptr,\n> \n> You should include \n> \n> Do you need this to solve which overload ? I see three functions here\n> with the following prototypes\n> \n> void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> \t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> \t\t enum ControlRange controlRange)\n> \n> T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> \t const ControlInfoMap &controlsInfo, const ControlId *control,\n> \t enum ControlRange controlRange, const V defaultValue)\n> \n> std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> \t\t\t const ControlInfoMap &controlsInfo,\n> \t\t\t const ControlId *control,\n> \t\t\t const std::vector &defaultVector)\n> \n> There doesn't seem to be any need to distinguish them on the template parameter\n> type.\n\nI added the template parameters for consistency. The latter two V and T,\nrespectively, can be inferred, but the first one cannot. And the first\none cannot be merged into the second (with default value) as then we\nhave no way of differentiating between \"unspecified\" and zero.\n\n> \n> > +\t typename V>\n> > +void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > +\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > +\t\t enum ControlRange controlRange)\n> > +{\n> > +\tconst auto &info = controlsInfo.find(control);\n> > +\tif (info == controlsInfo.end())\n> > +\t\treturn;\n> > +\n> > +\tT ret;\n> > +\tswitch (controlRange) {\n> > +\tcase ControlRange::Min:\n> > +\t\tret = info->second.min().get();\n> > +\t\tbreak;\n> > +\tcase ControlRange::Def:\n> > +\t\tret = info->second.def().get();\n> > +\t\tbreak;\n> > +\tcase ControlRange::Max:\n> > +\t\tret = info->second.max().get();\n> > +\t\tbreak;\n> > +\t}\n> > +\n> > +\tmetadata->addEntry(tag, ret);\n> > +\treturn;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > + * \\tparam T Type of the control in android\n> > + * \\tparam U Type of the control in libcamera\n> > + * \\param[in] metadata Android metadata to add the control value to\n> > + * \\param[in] tag Android metadata tag to add\n> > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > + * \\param[in] defaultValue The value to set in \\a metadata if \\a control is not found\n> > + *\n> > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > + * control info found for the libcamera control \\a control in the libcamera\n> > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > + * the android metadata entry is set to \\a defaultValue.\n> > + *\n> > + * This function is for scalar values.\n> > + */\n> > +template > +\t typename U,\n> > +\t typename V,\n> \n> Isn't V the same type as T ?\n\nIt is, but then as I mentioned earlier, this call would then be missing\na template parameter while the other one won't.\n\n> \n> > +\t std::enable_if_t> * = nullptr>\n> \n> This is probably functionally equivalent for now, but in the previous\n> version you were checking for the type T to be an arithmetic type.\n\nI was wondering if the template parameters should be in alphabetical\norder or not...\n\n\nThanks,\n\nPaul\n\n> \n> > +T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > +\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > +\t enum ControlRange controlRange, const V defaultValue)\n> > +{\n> > +\tT ret = defaultValue;\n> > +\n> > +\tconst auto &info = controlsInfo.find(control);\n> > +\tif (info != controlsInfo.end()) {\n> > +\t\tswitch (controlRange) {\n> > +\t\tcase ControlRange::Min:\n> > +\t\t\tret = info->second.min().get();\n> > +\t\t\tbreak;\n> > +\t\tcase ControlRange::Def:\n> > +\t\t\tret = info->second.def().get();\n> > +\t\t\tbreak;\n> > +\t\tcase ControlRange::Max:\n> > +\t\t\tret = info->second.max().get();\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t}\n> > +\n> > +\tmetadata->addEntry(tag, ret);\n> > +\treturn ret;\n> > +}\n> > +\n> > +/**\n> > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > + * \\tparam T Type of the control in android\n> > + * \\tparam V Type of the control in libcamera\n> > + * \\param[in] metadata Android metadata to add the control value to\n> > + * \\param[in] tag Android metadata tag to add\n> > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > + * \\param[in] defaultVector The value to set in \\a metadata if \\a control is not found\n> > + *\n> > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > + * control info found for the libcamera control \\a control in the libcamera\n> > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > + * the android metadata entry is set to \\a defaultVector.\n> > + *\n> > + * This function is for vector values.\n> > + */\n> > +template > +\t typename V>\n> \n> Fits on one line\n> \n> > +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > +\t\t\t const ControlInfoMap &controlsInfo,\n> > +\t\t\t const ControlId *control,\n> > +\t\t\t const std::vector &defaultVector)\n> > +{\n> > +\tstd::vector ret = {};\n> > +\n> > +\tconst auto &info = controlsInfo.find(control);\n> > +\tif (info != controlsInfo.end()) {\n> > +\t\tret.reserve(info->second.values().size());\n> > +\t\tfor (const auto &value : info->second.values())\n> > +\t\t\tret.push_back(value.get());\n> > +\t} else {\n> > +\t\tret = defaultVector;\n> \n> this is a copy that could be avoided by calling addEntry() in each\n> branch. Something like\n> \n> \tconst auto &info = controlsInfo.find(control);\n> \tif (info == controlsInfo.end()) {\n> metadata->addEntry(tag, defaultVector);\n> return defaultVector;\n> }\n> \n> std::vector ret(info->second.values().size());\n> for (const auto &value : info->second.values())\n> ret.push_back(value.get());\n> metadata->addEntry(tag, ret);\n> \n> return ret;\n> \n> \n> > +\t}\n> > +\n> > +\tmetadata->addEntry(tag, ret);\n> > +\treturn ret;\n> > +}\n> > +\n> > } /* namespace */\n> >\n> > int CameraCapabilities::initialize(std::shared_ptr camera,\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 87E8FBD794\n\tfor ;\n\tFri, 9 Jul 2021 04:43:20 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E5C7368523;\n\tFri, 9 Jul 2021 06:43:19 +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 E6FEF6059F\n\tfor ;\n\tFri, 9 Jul 2021 06:43:14 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 040F4E7;\n\tFri, 9 Jul 2021 06:43:12 +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=\"hlegGNja\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625805794;\n\tbh=ZwFp7t9r9JiIZwFxo7cSM1ZtV2FAHG/90874FRWe2sw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hlegGNja3tnKCPE7mLNxdyuWIdeQIv89wsA7pRwAX4UP7UPBT2E3ge8nb0rdpfjY1\n\tzVLpjkbsqV5+IObNwjn+7ChIBTCNJy8syLMorDhgmDjfGvyMheoNU/yj3kF2vSnN8t\n\tOXaAv2y52i965lzoxNc/MBscN0ww+GqTSJekIzE4=","Date":"Fri, 9 Jul 2021 13:43:06 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210709044210.GA1947@pyrite.rasen.tech>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-4-paul.elder@ideasonboard.com>\n\t<20210705154458.fabapieiagsygrc6@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210705154458.fabapieiagsygrc6@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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":18039,"web_url":"https://patchwork.libcamera.org/comment/18039/","msgid":"<20210709075255.4jbqsgxbbdxwoarn@uno.localdomain>","date":"2021-07-09T07:52:55","subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Paul,\n\nOn Fri, Jul 09, 2021 at 01:43:06PM +0900, paul.elder@ideasonboard.com wrote:\n> Hi Jacopo,\n>\n> On Mon, Jul 05, 2021 at 05:44:58PM +0200, Jacopo Mondi wrote:\n> > Hi Paul,\n> >\n> > On Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:\n> > > Add helpers for setting android metadata from libcamera controls.\n> > >\n> > > There are two versions, for scalars and collections, both of which take\n> > > a default value to fill in the android control if the libcamera control\n> > > is not found. A version for scalars exists for no default, to not set\n> > > the android control at all if it is not found in libcamera. The\n> > > functions take two template parameters, the first for the android type,\n> > > and the second for the libcamera type of the control. They can be\n> > > different, for example, if the former is an enum and the latter is a\n> > > boolean, or if the former is an enum (uint8_t) and the latter is an enum\n> > > (int32_t).\n> > >\n> > > The versions that take a default value return the value that was set in\n> > > the android metadata.\n> > >\n> > > Signed-off-by: Paul Elder \n> > >\n> > > ---\n> > > Changes in v3:\n> > > - setMetadata for collection only works with vectors\n> > > - change enum to enum class\n> > > - add two template parameters for android type and libcamera type\n> > > - add docs\n> > >\n> > > New in v2\n> > >\n> > > TODO: make ControlList versions so that we can use them in result\n> > > metadata\n> > > ---\n> > > src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++\n> > > 1 file changed, 137 insertions(+)\n> > >\n> > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > index 54bd71da..1d4c44ce 100644\n> > > --- a/src/android/camera_capabilities.cpp\n> > > +++ b/src/android/camera_capabilities.cpp\n> > > @@ -124,6 +124,143 @@ hwLevelStrings = {\n> > > \t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > > };\n> > >\n> > > +enum class ControlRange {\n> > > +\tMin,\n> > > +\tDef,\n> > > +\tMax,\n> > > +};\n> > > +\n> > > +/**\n> >\n> > We don't parse the HAL with doxygen so you can spare the additional *\n> > But it's more than fine to use the Doxygen syntax when documenting\n> > things imho, mostly for consistency\n> >\n> > > + * \\brief Set android metadata from libcamera ControlInfo\n> > > + * \\tparam T Type of the control in android\n> > > + * \\tparam V Type of the control in libcamera\n> > > + * \\param[in] metadata Android metadata to add the control value to\n> >\n> > s/Android metadata/Android metadata pack/\n> >\n> > > + * \\param[in] tag Android metadata tag to add\n> >\n> > s/to add// ?\n> >\n> > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > > + *\n> > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> >\n> > Set the android metadata \\a tag in the \\a metadata pack based on\n> > the...\n> >\n> > > + * control info found for the libcamera control \\a control in the libcamera\n> > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > + * the function returns without modifying anything.\n> > > + *\n> > > + * This function is for scalar values.\n> > > + */\n> > > +template > > +\t std::enable_if_t> * = nullptr,\n> >\n> > You should include \n> >\n> > Do you need this to solve which overload ? I see three functions here\n> > with the following prototypes\n> >\n> > void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > \t\t enum ControlRange controlRange)\n> >\n> > T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > \t enum ControlRange controlRange, const V defaultValue)\n> >\n> > std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > \t\t\t const ControlInfoMap &controlsInfo,\n> > \t\t\t const ControlId *control,\n> > \t\t\t const std::vector &defaultVector)\n> >\n> > There doesn't seem to be any need to distinguish them on the template parameter\n> > type.\n>\n> I added the template parameters for consistency. The latter two V and T,\n> respectively, can be inferred, but the first one cannot. And the first\n> one cannot be merged into the second (with default value) as then we\n> have no way of differentiating between \"unspecified\" and zero.\n>\n\nWhat I meant was specificially regarding enable_if<> as it doesn't\nseem to me you need it. Couldn't you get to the same result with a regular\nfunction overload that does not involve template metaprogramming ?\n\n> >\n> > > +\t typename V>\n> > > +void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > +\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > +\t\t enum ControlRange controlRange)\n> > > +{\n> > > +\tconst auto &info = controlsInfo.find(control);\n> > > +\tif (info == controlsInfo.end())\n> > > +\t\treturn;\n> > > +\n> > > +\tT ret;\n> > > +\tswitch (controlRange) {\n> > > +\tcase ControlRange::Min:\n> > > +\t\tret = info->second.min().get();\n> > > +\t\tbreak;\n> > > +\tcase ControlRange::Def:\n> > > +\t\tret = info->second.def().get();\n> > > +\t\tbreak;\n> > > +\tcase ControlRange::Max:\n> > > +\t\tret = info->second.max().get();\n> > > +\t\tbreak;\n> > > +\t}\n> > > +\n> > > +\tmetadata->addEntry(tag, ret);\n> > > +\treturn;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > > + * \\tparam T Type of the control in android\n> > > + * \\tparam U Type of the control in libcamera\n> > > + * \\param[in] metadata Android metadata to add the control value to\n> > > + * \\param[in] tag Android metadata tag to add\n> > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > > + * \\param[in] defaultValue The value to set in \\a metadata if \\a control is not found\n> > > + *\n> > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > > + * control info found for the libcamera control \\a control in the libcamera\n> > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > + * the android metadata entry is set to \\a defaultValue.\n> > > + *\n> > > + * This function is for scalar values.\n> > > + */\n> > > +template > > +\t typename U,\n> > > +\t typename V,\n> >\n> > Isn't V the same type as T ?\n>\n> It is, but then as I mentioned earlier, this call would then be missing\n> a template parameter while the other one won't.\n>\n\nI see three template parameters here and 2 above.\nAlso\n \n T setMetadata(..., V defaultValue)\n {\n T ret = defaultValue;\n ...\n }\n\nAgain feels like T == V\n\n\n> >\n> > > +\t std::enable_if_t> * = nullptr>\n> >\n> > This is probably functionally equivalent for now, but in the previous\n> > version you were checking for the type T to be an arithmetic type.\n>\n> I was wondering if the template parameters should be in alphabetical\n> order or not...\n\nWhatever, as long as you do your type checks on the same type as you\nhave:\n\n\t std::enable_if_t> * = nullptr>\nin one version, and\n\t std::enable_if_t> * = nullptr>\nin this version\n\nThis seems to reinforce to me T==V, but if you could just drop\nenable_if<> I think it would be better (iirc I compiled this patch\nwith enable_if<> removed, the firs time I reviewed it)\n\nThanks\n j\n\n>\n>\n> Thanks,\n>\n> Paul\n>\n> >\n> > > +T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > +\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > +\t enum ControlRange controlRange, const V defaultValue)\n> > > +{\n> > > +\tT ret = defaultValue;\n> > > +\n> > > +\tconst auto &info = controlsInfo.find(control);\n> > > +\tif (info != controlsInfo.end()) {\n> > > +\t\tswitch (controlRange) {\n> > > +\t\tcase ControlRange::Min:\n> > > +\t\t\tret = info->second.min().get();\n> > > +\t\t\tbreak;\n> > > +\t\tcase ControlRange::Def:\n> > > +\t\t\tret = info->second.def().get();\n> > > +\t\t\tbreak;\n> > > +\t\tcase ControlRange::Max:\n> > > +\t\t\tret = info->second.max().get();\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t}\n> > > +\n> > > +\tmetadata->addEntry(tag, ret);\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > +/**\n> > > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > > + * \\tparam T Type of the control in android\n> > > + * \\tparam V Type of the control in libcamera\n> > > + * \\param[in] metadata Android metadata to add the control value to\n> > > + * \\param[in] tag Android metadata tag to add\n> > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > + * \\param[in] defaultVector The value to set in \\a metadata if \\a control is not found\n> > > + *\n> > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > > + * control info found for the libcamera control \\a control in the libcamera\n> > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > + * the android metadata entry is set to \\a defaultVector.\n> > > + *\n> > > + * This function is for vector values.\n> > > + */\n> > > +template > > +\t typename V>\n> >\n> > Fits on one line\n> >\n> > > +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > +\t\t\t const ControlInfoMap &controlsInfo,\n> > > +\t\t\t const ControlId *control,\n> > > +\t\t\t const std::vector &defaultVector)\n> > > +{\n> > > +\tstd::vector ret = {};\n> > > +\n> > > +\tconst auto &info = controlsInfo.find(control);\n> > > +\tif (info != controlsInfo.end()) {\n> > > +\t\tret.reserve(info->second.values().size());\n> > > +\t\tfor (const auto &value : info->second.values())\n> > > +\t\t\tret.push_back(value.get());\n> > > +\t} else {\n> > > +\t\tret = defaultVector;\n> >\n> > this is a copy that could be avoided by calling addEntry() in each\n> > branch. Something like\n> >\n> > \tconst auto &info = controlsInfo.find(control);\n> > \tif (info == controlsInfo.end()) {\n> > metadata->addEntry(tag, defaultVector);\n> > return defaultVector;\n> > }\n> >\n> > std::vector ret(info->second.values().size());\n> > for (const auto &value : info->second.values())\n> > ret.push_back(value.get());\n> > metadata->addEntry(tag, ret);\n> >\n> > return ret;\n> >\n> >\n> > > +\t}\n> > > +\n> > > +\tmetadata->addEntry(tag, ret);\n> > > +\treturn ret;\n> > > +}\n> > > +\n> > > } /* namespace */\n> > >\n> > > int CameraCapabilities::initialize(std::shared_ptr camera,\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 98221C3224\n\tfor ;\n\tFri, 9 Jul 2021 07:52:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F407B68523;\n\tFri, 9 Jul 2021 09:52:09 +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 84D3D684F3\n\tfor ;\n\tFri, 9 Jul 2021 09:52:08 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 9E3B124000A;\n\tFri, 9 Jul 2021 07:52:06 +0000 (UTC)"],"Date":"Fri, 9 Jul 2021 09:52:55 +0200","From":"Jacopo Mondi ","To":"paul.elder@ideasonboard.com","Message-ID":"<20210709075255.4jbqsgxbbdxwoarn@uno.localdomain>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-4-paul.elder@ideasonboard.com>\n\t<20210705154458.fabapieiagsygrc6@uno.localdomain>\n\t<20210709044210.GA1947@pyrite.rasen.tech>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210709044210.GA1947@pyrite.rasen.tech>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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":18040,"web_url":"https://patchwork.libcamera.org/comment/18040/","msgid":"<20210709080842.GB5937@pyrite.rasen.tech>","date":"2021-07-09T08:08:42","subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Jul 09, 2021 at 09:52:55AM +0200, Jacopo Mondi wrote:\n> Hi Paul,\n> \n> On Fri, Jul 09, 2021 at 01:43:06PM +0900, paul.elder@ideasonboard.com wrote:\n> > Hi Jacopo,\n> >\n> > On Mon, Jul 05, 2021 at 05:44:58PM +0200, Jacopo Mondi wrote:\n> > > Hi Paul,\n> > >\n> > > On Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:\n> > > > Add helpers for setting android metadata from libcamera controls.\n> > > >\n> > > > There are two versions, for scalars and collections, both of which take\n> > > > a default value to fill in the android control if the libcamera control\n> > > > is not found. A version for scalars exists for no default, to not set\n> > > > the android control at all if it is not found in libcamera. The\n> > > > functions take two template parameters, the first for the android type,\n> > > > and the second for the libcamera type of the control. They can be\n> > > > different, for example, if the former is an enum and the latter is a\n> > > > boolean, or if the former is an enum (uint8_t) and the latter is an enum\n> > > > (int32_t).\n> > > >\n> > > > The versions that take a default value return the value that was set in\n> > > > the android metadata.\n> > > >\n> > > > Signed-off-by: Paul Elder \n> > > >\n> > > > ---\n> > > > Changes in v3:\n> > > > - setMetadata for collection only works with vectors\n> > > > - change enum to enum class\n> > > > - add two template parameters for android type and libcamera type\n> > > > - add docs\n> > > >\n> > > > New in v2\n> > > >\n> > > > TODO: make ControlList versions so that we can use them in result\n> > > > metadata\n> > > > ---\n> > > > src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++\n> > > > 1 file changed, 137 insertions(+)\n> > > >\n> > > > diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> > > > index 54bd71da..1d4c44ce 100644\n> > > > --- a/src/android/camera_capabilities.cpp\n> > > > +++ b/src/android/camera_capabilities.cpp\n> > > > @@ -124,6 +124,143 @@ hwLevelStrings = {\n> > > > \t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> > > > };\n> > > >\n> > > > +enum class ControlRange {\n> > > > +\tMin,\n> > > > +\tDef,\n> > > > +\tMax,\n> > > > +};\n> > > > +\n> > > > +/**\n> > >\n> > > We don't parse the HAL with doxygen so you can spare the additional *\n> > > But it's more than fine to use the Doxygen syntax when documenting\n> > > things imho, mostly for consistency\n> > >\n> > > > + * \\brief Set android metadata from libcamera ControlInfo\n> > > > + * \\tparam T Type of the control in android\n> > > > + * \\tparam V Type of the control in libcamera\n> > > > + * \\param[in] metadata Android metadata to add the control value to\n> > >\n> > > s/Android metadata/Android metadata pack/\n> > >\n> > > > + * \\param[in] tag Android metadata tag to add\n> > >\n> > > s/to add// ?\n> > >\n> > > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > > > + *\n> > > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > >\n> > > Set the android metadata \\a tag in the \\a metadata pack based on\n> > > the...\n> > >\n> > > > + * control info found for the libcamera control \\a control in the libcamera\n> > > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > > + * the function returns without modifying anything.\n> > > > + *\n> > > > + * This function is for scalar values.\n> > > > + */\n> > > > +template > > > +\t std::enable_if_t> * = nullptr,\n> > >\n> > > You should include \n> > >\n> > > Do you need this to solve which overload ? I see three functions here\n> > > with the following prototypes\n> > >\n> > > void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > \t\t enum ControlRange controlRange)\n> > >\n> > > T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > \t enum ControlRange controlRange, const V defaultValue)\n> > >\n> > > std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > \t\t\t const ControlInfoMap &controlsInfo,\n> > > \t\t\t const ControlId *control,\n> > > \t\t\t const std::vector &defaultVector)\n> > >\n> > > There doesn't seem to be any need to distinguish them on the template parameter\n> > > type.\n> >\n> > I added the template parameters for consistency. The latter two V and T,\n> > respectively, can be inferred, but the first one cannot. And the first\n> > one cannot be merged into the second (with default value) as then we\n> > have no way of differentiating between \"unspecified\" and zero.\n> >\n> \n> What I meant was specificially regarding enable_if<> as it doesn't\n> seem to me you need it. Couldn't you get to the same result with a regular\n> function overload that does not involve template metaprogramming ?\n\nI guess the first one doesn't need it.\n\nThe second and third I think do? Otherwise we could get V = std::vector<*> ?\n\n> > >\n> > > > +\t typename V>\n> > > > +void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > > +\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > > +\t\t enum ControlRange controlRange)\n> > > > +{\n> > > > +\tconst auto &info = controlsInfo.find(control);\n> > > > +\tif (info == controlsInfo.end())\n> > > > +\t\treturn;\n> > > > +\n> > > > +\tT ret;\n> > > > +\tswitch (controlRange) {\n> > > > +\tcase ControlRange::Min:\n> > > > +\t\tret = info->second.min().get();\n> > > > +\t\tbreak;\n> > > > +\tcase ControlRange::Def:\n> > > > +\t\tret = info->second.def().get();\n> > > > +\t\tbreak;\n> > > > +\tcase ControlRange::Max:\n> > > > +\t\tret = info->second.max().get();\n> > > > +\t\tbreak;\n> > > > +\t}\n> > > > +\n> > > > +\tmetadata->addEntry(tag, ret);\n> > > > +\treturn;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > > > + * \\tparam T Type of the control in android\n> > > > + * \\tparam U Type of the control in libcamera\n> > > > + * \\param[in] metadata Android metadata to add the control value to\n> > > > + * \\param[in] tag Android metadata tag to add\n> > > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > > + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> > > > + * \\param[in] defaultValue The value to set in \\a metadata if \\a control is not found\n> > > > + *\n> > > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > > > + * control info found for the libcamera control \\a control in the libcamera\n> > > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > > + * the android metadata entry is set to \\a defaultValue.\n> > > > + *\n> > > > + * This function is for scalar values.\n> > > > + */\n> > > > +template > > > +\t typename U,\n> > > > +\t typename V,\n> > >\n> > > Isn't V the same type as T ?\n> >\n> > It is, but then as I mentioned earlier, this call would then be missing\n> > a template parameter while the other one won't.\n> >\n> \n> I see three template parameters here and 2 above.\n> Also\n> \n> T setMetadata(..., V defaultValue)\n> {\n> T ret = defaultValue;\n> ...\n> }\n> \n> Again feels like T == V\n\nMy intention was to allow T != V as long as they can be implicitly\nconverted, like enum -> uint8_t, or uint8_t -> bool. It's quite common\nthat the android uses and enum while libcamera uses int32_t, for\nexample.\n\nWhich does mean that the template parameter needs || std::is_enum_v.\n\n\nThanks,\n\nPaul\n\n> \n> > >\n> > > > +\t std::enable_if_t> * = nullptr>\n> > >\n> > > This is probably functionally equivalent for now, but in the previous\n> > > version you were checking for the type T to be an arithmetic type.\n> >\n> > I was wondering if the template parameters should be in alphabetical\n> > order or not...\n> \n> Whatever, as long as you do your type checks on the same type as you\n> have:\n> \n> \t std::enable_if_t> * = nullptr>\n> in one version, and\n> \t std::enable_if_t> * = nullptr>\n> in this version\n> \n> This seems to reinforce to me T==V, but if you could just drop\n> enable_if<> I think it would be better (iirc I compiled this patch\n> with enable_if<> removed, the firs time I reviewed it)\n> \n> Thanks\n> j\n> \n> >\n> >\n> > Thanks,\n> >\n> > Paul\n> >\n> > >\n> > > > +T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > > +\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> > > > +\t enum ControlRange controlRange, const V defaultValue)\n> > > > +{\n> > > > +\tT ret = defaultValue;\n> > > > +\n> > > > +\tconst auto &info = controlsInfo.find(control);\n> > > > +\tif (info != controlsInfo.end()) {\n> > > > +\t\tswitch (controlRange) {\n> > > > +\t\tcase ControlRange::Min:\n> > > > +\t\t\tret = info->second.min().get();\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase ControlRange::Def:\n> > > > +\t\t\tret = info->second.def().get();\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase ControlRange::Max:\n> > > > +\t\t\tret = info->second.max().get();\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t}\n> > > > +\n> > > > +\tmetadata->addEntry(tag, ret);\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > +/**\n> > > > + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> > > > + * \\tparam T Type of the control in android\n> > > > + * \\tparam V Type of the control in libcamera\n> > > > + * \\param[in] metadata Android metadata to add the control value to\n> > > > + * \\param[in] tag Android metadata tag to add\n> > > > + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> > > > + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> > > > + * \\param[in] defaultVector The value to set in \\a metadata if \\a control is not found\n> > > > + *\n> > > > + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> > > > + * control info found for the libcamera control \\a control in the libcamera\n> > > > + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> > > > + * the android metadata entry is set to \\a defaultVector.\n> > > > + *\n> > > > + * This function is for vector values.\n> > > > + */\n> > > > +template > > > +\t typename V>\n> > >\n> > > Fits on one line\n> > >\n> > > > +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> > > > +\t\t\t const ControlInfoMap &controlsInfo,\n> > > > +\t\t\t const ControlId *control,\n> > > > +\t\t\t const std::vector &defaultVector)\n> > > > +{\n> > > > +\tstd::vector ret = {};\n> > > > +\n> > > > +\tconst auto &info = controlsInfo.find(control);\n> > > > +\tif (info != controlsInfo.end()) {\n> > > > +\t\tret.reserve(info->second.values().size());\n> > > > +\t\tfor (const auto &value : info->second.values())\n> > > > +\t\t\tret.push_back(value.get());\n> > > > +\t} else {\n> > > > +\t\tret = defaultVector;\n> > >\n> > > this is a copy that could be avoided by calling addEntry() in each\n> > > branch. Something like\n> > >\n> > > \tconst auto &info = controlsInfo.find(control);\n> > > \tif (info == controlsInfo.end()) {\n> > > metadata->addEntry(tag, defaultVector);\n> > > return defaultVector;\n> > > }\n> > >\n> > > std::vector ret(info->second.values().size());\n> > > for (const auto &value : info->second.values())\n> > > ret.push_back(value.get());\n> > > metadata->addEntry(tag, ret);\n> > >\n> > > return ret;\n> > >\n> > >\n> > > > +\t}\n> > > > +\n> > > > +\tmetadata->addEntry(tag, ret);\n> > > > +\treturn ret;\n> > > > +}\n> > > > +\n> > > > } /* namespace */\n> > > >\n> > > > int CameraCapabilities::initialize(std::shared_ptr camera,\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 A34E7BD794\n\tfor ;\n\tFri, 9 Jul 2021 08:08:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2992B6851F;\n\tFri, 9 Jul 2021 10:08:52 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B1674684F3\n\tfor ;\n\tFri, 9 Jul 2021 10:08:50 +0200 (CEST)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EBC25E7;\n\tFri, 9 Jul 2021 10:08:48 +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=\"h71FR5sG\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1625818130;\n\tbh=kPKQNYK1MfqSapIDERQU0XQiPPKvqRzpXT23IOErd1M=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=h71FR5sGPIjcSd1jTUHz180h6XWYeUr8GByVVkzIg/9rJIWVL+k9NQzSrqWh+TpjT\n\tgcEGKzWWAR93ehBtI328YXwX89xhSqjEhYoYxjYIN51Qk0RBgmHZjudjbNUdpvgeJ1\n\tp2B0snwsfNpYwet7xMBF4d68JifSH8y2PQB8NChI=","Date":"Fri, 9 Jul 2021 17:08:42 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210709080842.GB5937@pyrite.rasen.tech>","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-4-paul.elder@ideasonboard.com>\n\t<20210705154458.fabapieiagsygrc6@uno.localdomain>\n\t<20210709044210.GA1947@pyrite.rasen.tech>\n\t<20210709075255.4jbqsgxbbdxwoarn@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<20210709075255.4jbqsgxbbdxwoarn@uno.localdomain>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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":18088,"web_url":"https://patchwork.libcamera.org/comment/18088/","msgid":"","date":"2021-07-11T21:26:19","subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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 Fri, Jul 02, 2021 at 07:37:47PM +0900, Paul Elder wrote:\n> Add helpers for setting android metadata from libcamera controls.\n> \n> There are two versions, for scalars and collections, both of which take\n> a default value to fill in the android control if the libcamera control\n> is not found. A version for scalars exists for no default, to not set\n> the android control at all if it is not found in libcamera. The\n> functions take two template parameters, the first for the android type,\n> and the second for the libcamera type of the control. They can be\n> different, for example, if the former is an enum and the latter is a\n> boolean, or if the former is an enum (uint8_t) and the latter is an enum\n> (int32_t).\n> \n> The versions that take a default value return the value that was set in\n> the android metadata.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v3:\n> - setMetadata for collection only works with vectors\n> - change enum to enum class\n> - add two template parameters for android type and libcamera type\n> - add docs\n> \n> New in v2\n> \n> TODO: make ControlList versions so that we can use them in result\n> metadata\n> ---\n> src/android/camera_capabilities.cpp | 137 ++++++++++++++++++++++++++++\n> 1 file changed, 137 insertions(+)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index 54bd71da..1d4c44ce 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -124,6 +124,143 @@ hwLevelStrings = {\n> \t{ ANDROID_INFO_SUPPORTED_HARDWARE_LEVEL_EXTERNAL, \"EXTERNAL\" },\n> };\n> \n> +enum class ControlRange {\n> +\tMin,\n> +\tDef,\n> +\tMax,\n> +};\n> +\n> +/**\n> + * \\brief Set android metadata from libcamera ControlInfo\n> + * \\tparam T Type of the control in android\n> + * \\tparam V Type of the control in libcamera\n> + * \\param[in] metadata Android metadata to add the control value to\n> + * \\param[in] tag Android metadata tag to add\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the function returns without modifying anything.\n> + *\n> + * This function is for scalar values.\n> + */\n> +template +\t std::enable_if_t> * = nullptr,\n> +\t typename V>\n\nThis really puzzles me. The second template argument has a default\nvalue, but the third doesn't. I thought it would generate a compilation\nerror. This function doesn't seem to be used, so maybe that's why the\ncompiler doesn't choke ?\n\nDo you have plans to use this function in the near future ? If not, I'd\ndrop it for now.\n\nI also agree with Jacopo that enable_if doesn't seem to be needed here,\nnor below (the second version of the function has a ControlRange\nargument that the third one doesn't have)..\n\n> +void setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> +\t\t enum ControlRange controlRange)\n\nShouldn't these be member functions of CameraMetadata ?\n\n> +{\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info == controlsInfo.end())\n> +\t\treturn;\n> +\n> +\tT ret;\n> +\tswitch (controlRange) {\n> +\tcase ControlRange::Min:\n> +\t\tret = info->second.min().get();\n> +\t\tbreak;\n> +\tcase ControlRange::Def:\n> +\t\tret = info->second.def().get();\n> +\t\tbreak;\n> +\tcase ControlRange::Max:\n> +\t\tret = info->second.max().get();\n> +\t\tbreak;\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn;\n> +}\n> +\n> +/**\n> + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> + * \\tparam T Type of the control in android\n> + * \\tparam U Type of the control in libcamera\n\nV is missing.\n\n> + * \\param[in] metadata Android metadata to add the control value to\n> + * \\param[in] tag Android metadata tag to add\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] controlRange Whether to use the min, def, or max value from the control info\n> + * \\param[in] defaultValue The value to set in \\a metadata if \\a control is not found\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the android metadata entry is set to \\a defaultValue.\n> + *\n> + * This function is for scalar values.\n> + */\n> +template +\t typename U,\n\nHaving to specify the libcamera type manually is error-prone. Could we\nhandle this dynamically, based on control->type() ?\n\n> +\t typename V,\n> +\t std::enable_if_t> * = nullptr>\n> +T setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t const ControlInfoMap &controlsInfo, const ControlId *control,\n> +\t enum ControlRange controlRange, const V defaultValue)\n> +{\n> +\tT ret = defaultValue;\n> +\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info != controlsInfo.end()) {\n> +\t\tswitch (controlRange) {\n> +\t\tcase ControlRange::Min:\n> +\t\t\tret = info->second.min().get();\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Def:\n> +\t\t\tret = info->second.def().get();\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Max:\n> +\t\t\tret = info->second.max().get();\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn ret;\n> +}\n> +\n> +/**\n> + * \\brief Set android metadata from libcamera ControlInfo or a default value\n> + * \\tparam T Type of the control in android\n> + * \\tparam V Type of the control in libcamera\n> + * \\param[in] metadata Android metadata to add the control value to\n> + * \\param[in] tag Android metadata tag to add\n> + * \\param[in] controlsInfo libcamera ControlInfoMap from which to find the control info\n> + * \\param[in] control libcamera ControlId to find from \\a controlsInfo\n> + * \\param[in] defaultVector The value to set in \\a metadata if \\a control is not found\n> + *\n> + * Set the android metadata entry in \\a metadata with tag \\a tag based on the\n> + * control info found for the libcamera control \\a control in the libcamera\n> + * ControlInfoMap \\a controlsInfo. If no libcamera ControlInfo is found, then\n> + * the android metadata entry is set to \\a defaultVector.\n> + *\n> + * This function is for vector values.\n> + */\n> +template +\t typename V>\n> +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t\t\t const ControlInfoMap &controlsInfo,\n> +\t\t\t const ControlId *control,\n> +\t\t\t const std::vector &defaultVector)\n> +{\n> +\tstd::vector ret = {};\n> +\n> +\tconst auto &info = controlsInfo.find(control);\n> +\tif (info != controlsInfo.end()) {\n> +\t\tret.reserve(info->second.values().size());\n> +\t\tfor (const auto &value : info->second.values())\n> +\t\t\tret.push_back(value.get());\n> +\t} else {\n> +\t\tret = defaultVector;\n> +\t}\n> +\n> +\tmetadata->addEntry(tag, ret);\n> +\treturn ret;\n> +}\n> +\n> } /* namespace */\n> \n> int CameraCapabilities::initialize(std::shared_ptr camera,","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 788FFC3224\n\tfor ;\n\tSun, 11 Jul 2021 21:27:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C5E756851D;\n\tSun, 11 Jul 2021 23:27:07 +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 1BBA268519\n\tfor ;\n\tSun, 11 Jul 2021 23:27:06 +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 7A7C2CC;\n\tSun, 11 Jul 2021 23:27:05 +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=\"PCXE9llq\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1626038825;\n\tbh=RVkAT0je1Q9sz1e6y8Mo2p6D9LVHMTplkWad0YBWscE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=PCXE9llqVjfHg32R26Str5Lxp+4hJN86/TT1coi0Sbemm0pk9zlowut6NCp77+oIr\n\tmx+6Pwl24FNseZ3bz7DWGe+v0ihV00QIUe8x2Hcer4BOokZAce+wAW2bn4xFlI6P1Q\n\t15r7F8p4KXe0y+SDbGhoYvpoIfbLJ/2+mw4bIPb0=","Date":"Mon, 12 Jul 2021 00:26:19 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210702103800.41291-1-paul.elder@ideasonboard.com>\n\t<20210702103800.41291-4-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210702103800.41291-4-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH v3 03/16] android: Add helpers for\n\tsetting android metadata from libcamera controls","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\" "}}]