[{"id":18476,"web_url":"https://patchwork.libcamera.org/comment/18476/","msgid":"","date":"2021-08-02T01:42:26","subject":"Re: [libcamera-devel] [PATCH v6 6/9] 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 30, 2021 at 07:35:33PM +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. They both return the value that was set.\n> \n> Signed-off-by: Paul Elder \n> \n> ---\n> Changes in v6:\n> - remove unused scalar-no-default version\n> - remove explicit template parameters\n> - infer the template parameters from the Control type\n> \n> Changes in v4:\n> - remove vector copy from the vector setter\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 | 88 +++++++++++++++++++++++++++++\n> 1 file changed, 88 insertions(+)\n> \n> diff --git a/src/android/camera_capabilities.cpp b/src/android/camera_capabilities.cpp\n> index b59a854f..fa701843 100644\n> --- a/src/android/camera_capabilities.cpp\n> +++ b/src/android/camera_capabilities.cpp\n> @@ -11,6 +11,7 @@\n> #include \n> #include \n> #include \n> +#include \n> \n> #include \n> \n> @@ -125,6 +126,93 @@ 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 or a default value\n> + * \\tparam T Type of the control in libcamera\n> + * \\tparam U Type of the control in android\n\ns/control/metadata/\ns/android/Android/\n\n> + * \\param[in] metadata Android metadata pack to add the control value to\n> + * \\param[in] tag Android metadata tag\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\ns/android/Android/\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 android metadata entry is set to \\a defaultValue.\n\nHere too.\n\n> + *\n> + * This function is for scalar values.\n> + */\n> +template\n> +U setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t const ControlInfoMap &controlsInfo, const Control *control,\n\nAs control can't be null, I'd pass it by reference.\n\n> +\t enum ControlRange controlRange, const U defaultValue)\n> +{\n> +\tU ret = defaultValue;\n\nI'd name the variable value instead of ret.\n\n> +\n> +\tconst auto &info = controlsInfo.find(reinterpret_cast(control));\n\nYou don't need the cast here, Control inherits from ControlId.\n\n> +\tif (info != controlsInfo.end()) {\n> +\t\tswitch (controlRange) {\n> +\t\tcase ControlRange::Min:\n> +\t\t\tret = static_cast(info->second.min().get());\n\nAs a side effect, however, dropping the cast requires writing this as\n\n\t\t\tret = static_cast(info->second.min().template get());\n\nI have no idea why the cast makes a difference. Same below.\n\nAll these comments apply to the next function as well.\n\nReviewed-by: Laurent Pinchart \n\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Def:\n> +\t\t\tret = static_cast(info->second.def().get());\n> +\t\t\tbreak;\n> +\t\tcase ControlRange::Max:\n> +\t\t\tret = static_cast(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 libcamera\n> + * \\tparam U Type of the control in android\n> + * \\param[in] metadata Android metadata pack to add the control value to\n> + * \\param[in] tag Android metadata tag\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\n> +std::vector setMetadata(CameraMetadata *metadata, uint32_t tag,\n> +\t\t\t const ControlInfoMap &controlsInfo,\n> +\t\t\t const Control *control,\n> +\t\t\t const std::vector &defaultVector)\n> +{\n> +\tconst auto &info = controlsInfo.find(reinterpret_cast(control));\n> +\tif (info == controlsInfo.end()) {\n> +\t\tmetadata->addEntry(tag, defaultVector);\n> +\t\treturn defaultVector;\n> +\t}\n> +\n> +\tstd::vector ret(info->second.values().size());\n> +\tfor (const auto &value : info->second.values())\n> +\t\tret.push_back(static_cast(value.get()));\n> +\tmetadata->addEntry(tag, ret);\n> +\n> +\treturn ret;\n> +}\n> +\n> } /* namespace */\n> \n> bool CameraCapabilities::validateManualSensorCapability()","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 BA3D6C3232\n\tfor ;\n\tMon, 2 Aug 2021 01:42:37 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 26643687C5;\n\tMon, 2 Aug 2021 03:42:37 +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 40006687B6\n\tfor ;\n\tMon, 2 Aug 2021 03:42:36 +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 B48354A3;\n\tMon, 2 Aug 2021 03:42:35 +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=\"tah2WjUW\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1627868555;\n\tbh=2Bh61STBRDaAh+oVKUzb0cOMg+T+8Fi9EyRSoCzPcEI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tah2WjUWr0HeMmJ2lESn5r0upMI5TlxjW0uNMbjYuD8OlpGf4j+55StTxpNqQSDtW\n\ttn3QnNK8yNHYXwoh9noZc1f09ofg1QdDm4QNca5Gr8nb36IYOXoDUs0PkptNmfiL6i\n\tuAYOBI3yo+CUinMcEDcVjHMqixgFzBw3zUI5d/dE=","Date":"Mon, 2 Aug 2021 04:42:26 +0300","From":"Laurent Pinchart ","To":"Paul Elder ","Message-ID":"","References":"<20210730103536.81117-1-paul.elder@ideasonboard.com>\n\t<20210730103536.81117-7-paul.elder@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20210730103536.81117-7-paul.elder@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v6 6/9] 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\" "}}]