[{"id":22174,"web_url":"https://patchwork.libcamera.org/comment/22174/","msgid":"","date":"2022-02-16T00:57:14","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-lin,\n\nOn Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:\n> Introduce option to customize behavior for a camera module, which might\n> alter the camera characteristic and should be set before inspecting it.\n> \n> Re-use the Control generation infrastructure to generate options and define\n> the first 'PipelineConfigFile' option, which allows user to specify\n> preference for pipeline configuration.\n\nWhen thinking about this, I wasn't envisioning passing the configuration\nfile to libcamera through the C++ API, but looking for it in standard\nlocations on the file system. Could you explain the reason why you think\nthis should be dynamic ?\n\n> Signed-off-by: Han-Lin Chen \n> ---\n> include/libcamera/ipa/ipa_controls.h | 1 +\n> include/libcamera/meson.build | 3 +-\n> include/libcamera/option_ids.h.in | 36 +++++++++++++++++\n> src/libcamera/control_serializer.cpp | 12 ++++++\n> src/libcamera/option_ids.cpp.in | 58 ++++++++++++++++++++++++++++\n> src/libcamera/option_ids.yaml | 16 ++++++++\n> 6 files changed, 125 insertions(+), 1 deletion(-)\n> create mode 100644 include/libcamera/option_ids.h.in\n> create mode 100644 src/libcamera/option_ids.cpp.in\n> create mode 100644 src/libcamera/option_ids.yaml\n> \n> diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> index da1a7596..8173a2e5 100644\n> --- a/include/libcamera/ipa/ipa_controls.h\n> +++ b/include/libcamera/ipa/ipa_controls.h\n> @@ -18,6 +18,7 @@ extern \"C\" {\n> enum ipa_controls_id_map_type {\n> \tIPA_CONTROL_ID_MAP_CONTROLS,\n> \tIPA_CONTROL_ID_MAP_PROPERTIES,\n> +\tIPA_CONTROL_ID_MAP_OPTIONS,\n\nDo you have use cases for passing a configuration file to the IPA\nmodules ? As IPA modules are sandboxed, they don't have unrestricted\naccess to the filesystem. I think this could create issues, especially\nif the path to the configuration is under the control of an application.\n\n> \tIPA_CONTROL_ID_MAP_V4L2,\n> };\n> \n> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> index 408b7acf..a9b71bc3 100644\n> --- a/include/libcamera/meson.build\n> +++ b/include/libcamera/meson.build\n> @@ -31,10 +31,11 @@ install_headers(libcamera_public_headers,\n> \n> libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir\n> \n> -# control_ids.h and property_ids.h\n> +# control_ids.h, property_ids.h, and option_ids.h\n> control_source_files = [\n> 'control_ids',\n> 'property_ids',\n> + 'option_ids',\n> ]\n> \n> control_headers = []\n> diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in\n> new file mode 100644\n> index 00000000..53058a4b\n> --- /dev/null\n> +++ b/include/libcamera/option_ids.h.in\n> @@ -0,0 +1,36 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * option_ids.h - Option ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace options {\n> +\n> +enum {\n> +${ids}\n> +};\n> +\n> +${controls}\n> +\n> +namespace draft {\n> +\n> +${draft_controls}\n> +\n> +} /* namespace draft */\n> +\n> +extern const ControlIdMap options;\n> +\n> +} /* namespace options */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> index e87d2362..dc8d2906 100644\n> --- a/src/libcamera/control_serializer.cpp\n> +++ b/src/libcamera/control_serializer.cpp\n> @@ -16,6 +16,7 @@\n> \n> #include \n> #include \n> +#include \n> #include \n> \n> #include \n> @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> \tenum ipa_controls_id_map_type idMapType;\n> \tif (idmap == &controls::controls)\n> \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &options::options)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> \telse if (idmap == &properties::properties)\n> \t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> \telse\n> @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,\n> \tenum ipa_controls_id_map_type idMapType;\n> \tif (idmap == &controls::controls)\n> \t\tidMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> +\telse if (idmap == &options::options)\n> +\t\tidMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> \telse if (idmap == &properties::properties)\n> \t\tidMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> \telse\n> @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> \tcase IPA_CONTROL_ID_MAP_CONTROLS:\n> \t\tidMap = &controls::controls;\n> \t\tbreak;\n> +\tcase IPA_CONTROL_ID_MAP_OPTIONS:\n> +\t\tidMap = &options::options;\n> +\t\tbreak;\n> \tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> \t\tidMap = &properties::properties;\n> \t\tbreak;\n> @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> \t\t\tidMap = &controls::controls;\n> \t\t\tbreak;\n> \n> +\t\tcase IPA_CONTROL_ID_MAP_OPTIONS:\n> +\t\t\tidMap = &options::options;\n> +\t\t\tbreak;\n> +\n> \t\tcase IPA_CONTROL_ID_MAP_PROPERTIES:\n> \t\t\tidMap = &properties::properties;\n> \t\t\tbreak;\n> diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in\n> new file mode 100644\n> index 00000000..adfd0633\n> --- /dev/null\n> +++ b/src/libcamera/option_ids.cpp.in\n> @@ -0,0 +1,58 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2022, Google Inc.\n> + *\n> + * option_ids.cpp : Option ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#include \n> +\n> +/**\n> + * \\file option_ids.h\n> + * \\brief Camera option identifiers\n> + */\n> +\n> +namespace libcamera {\n> +\n> +/**\n> + * \\brief Namespace for libcamera options\n> + */\n> +namespace options {\n> +\n> +${controls_doc}\n> +\n> +/**\n> + * \\brief Namespace for libcamera draft options\n> + */\n> +namespace draft {\n> +\n> +${draft_controls_doc}\n> +\n> +} /* namespace draft */\n> +\n> +#ifndef __DOXYGEN__\n> +/*\n> + * Keep the options definitions hidden from doxygen as it incorrectly parses\n> + * them as functions.\n> + */\n> +${controls_def}\n> +\n> +namespace draft {\n> +\n> +${draft_controls_def}\n> +\n> +} /* namespace draft */\n> +#endif\n> +\n> +/**\n> + * \\brief List of all supported libcamera options\n> + */\n> +extern const ControlIdMap options {\n> +${controls_map}\n> +};\n> +\n> +} /* namespace options */\n> +\n> +} /* namespace libcamera */\n> diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml\n> new file mode 100644\n> index 00000000..3f9304be\n> --- /dev/null\n> +++ b/src/libcamera/option_ids.yaml\n> @@ -0,0 +1,16 @@\n> +# SPDX-License-Identifier: LGPL-2.1-or-later\n> +#\n> +# Copyright (C) 2022, Google Inc.\n> +#\n> +%YAML 1.2\n> +---\n> +controls:\n> + - PipelineConfigFile:\n> + type: string\n> + description: |\n> + Some platform supports customizing pipeline configuration preference\n> + for a camera module. The structure and contents of the configuration\n> + file is specific to each platform in YAML format. The PipelineConfigFile\n> + option can be used to specify the path to load the configuration file.\n\nI'm not totally thrilled about using ControlList for this, I feel it may\nbe a bit over-engineered, but I suppose I'll understand the rationale\nbetter after reviewing the whole series.\n\n> +\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 3E74CBF415\n\tfor ;\n\tWed, 16 Feb 2022 00:57:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 94D366112A;\n\tWed, 16 Feb 2022 01:57:23 +0100 (CET)","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 9C7AC601FC\n\tfor ;\n\tWed, 16 Feb 2022 01:57:21 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 0A5A325B;\n\tWed, 16 Feb 2022 01:57:20 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"JQ9Aloll\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1644973041;\n\tbh=dYyAVszS+YAAOqErW2NvOb8vWgOJLryNZn9NlTiMLMI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JQ9AlollyW5vkGfrfSYpdWIhlvL9X+Ef0rkl26nJz6j73S/a+e1cu6hgLSmR8w5dj\n\twBPHoi+XIIskGr5a2DFtT1big4UCfM3b9j0bz8358b9oaobIebZQnQKGS5RTI2D48B\n\tWLTp1viYLn9b8N/R0W/yUNfWkk+fQQ3rq7v7ItzE=","Date":"Wed, 16 Feb 2022 02:57:14 +0200","From":"Laurent Pinchart ","To":"Han-Lin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-2-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220209071917.559993-2-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","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":22177,"web_url":"https://patchwork.libcamera.org/comment/22177/","msgid":"","date":"2022-02-16T11:43:07","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","submitter":{"id":98,"url":"https://patchwork.libcamera.org/api/people/98/","name":"Hanlin Chen","email":"hanlinchen@chromium.org"},"content":"Hi Laurent,\nThanks for the comments.\n\nOn Wed, Feb 16, 2022 at 8:57 AM Laurent Pinchart\n wrote:\n>\n> Hi Han-lin,\n>\n> On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:\n> > Introduce option to customize behavior for a camera module, which might\n> > alter the camera characteristic and should be set before inspecting it.\n> >\n> > Re-use the Control generation infrastructure to generate options and define\n> > the first 'PipelineConfigFile' option, which allows user to specify\n> > preference for pipeline configuration.\n>\n> When thinking about this, I wasn't envisioning passing the configuration\n> file to libcamera through the C++ API, but looking for it in standard\n> locations on the file system. Could you explain the reason why you think\n> this should be dynamic ?\n\nWhat I had in mind is to let the application choose the preference per\ncamera module, and even with the same sensor,\nthe user might still want different tunings. And we don't need to have\na big configuration file for all parameters.\nThe option may extend to what we used to set in environment variables,\nlike LIBCAMERA_RPI_TUNING_FILE for example.\n\nYes, I think it's up for discussion, and depends on how we think it's\nuseful and whether it can extend.\nIf this is the only use case, I do agree it's over-engineered :|\n\n>\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > include/libcamera/ipa/ipa_controls.h | 1 +\n> > include/libcamera/meson.build | 3 +-\n> > include/libcamera/option_ids.h.in | 36 +++++++++++++++++\n> > src/libcamera/control_serializer.cpp | 12 ++++++\n> > src/libcamera/option_ids.cpp.in | 58 ++++++++++++++++++++++++++++\n> > src/libcamera/option_ids.yaml | 16 ++++++++\n> > 6 files changed, 125 insertions(+), 1 deletion(-)\n> > create mode 100644 include/libcamera/option_ids.h.in\n> > create mode 100644 src/libcamera/option_ids.cpp.in\n> > create mode 100644 src/libcamera/option_ids.yaml\n> >\n> > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > index da1a7596..8173a2e5 100644\n> > --- a/include/libcamera/ipa/ipa_controls.h\n> > +++ b/include/libcamera/ipa/ipa_controls.h\n> > @@ -18,6 +18,7 @@ extern \"C\" {\n> > enum ipa_controls_id_map_type {\n> > IPA_CONTROL_ID_MAP_CONTROLS,\n> > IPA_CONTROL_ID_MAP_PROPERTIES,\n> > + IPA_CONTROL_ID_MAP_OPTIONS,\n>\n> Do you have use cases for passing a configuration file to the IPA\n> modules ? As IPA modules are sandboxed, they don't have unrestricted\n> access to the filesystem. I think this could create issues, especially\n> if the path to the configuration is under the control of an application.\n\nI've seen cases that read the content of a file and pass it to IPA\nwith a FD to avoid the problem.\nIn fact, for ChromeOS, we control the access of IPA to a white-listed\nlocations, so it's something I can configure.\nFor other platforms, I think we need a definition of \"sandboxed\" and\nwho should do the sandboxing, like libcamera or the device vendor?\n\n>\n> > IPA_CONTROL_ID_MAP_V4L2,\n> > };\n> >\n> > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > index 408b7acf..a9b71bc3 100644\n> > --- a/include/libcamera/meson.build\n> > +++ b/include/libcamera/meson.build\n> > @@ -31,10 +31,11 @@ install_headers(libcamera_public_headers,\n> >\n> > libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir\n> >\n> > -# control_ids.h and property_ids.h\n> > +# control_ids.h, property_ids.h, and option_ids.h\n> > control_source_files = [\n> > 'control_ids',\n> > 'property_ids',\n> > + 'option_ids',\n> > ]\n> >\n> > control_headers = []\n> > diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in\n> > new file mode 100644\n> > index 00000000..53058a4b\n> > --- /dev/null\n> > +++ b/include/libcamera/option_ids.h.in\n> > @@ -0,0 +1,36 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * option_ids.h - Option ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace options {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +${controls}\n> > +\n> > +namespace draft {\n> > +\n> > +${draft_controls}\n> > +\n> > +} /* namespace draft */\n> > +\n> > +extern const ControlIdMap options;\n> > +\n> > +} /* namespace options */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > index e87d2362..dc8d2906 100644\n> > --- a/src/libcamera/control_serializer.cpp\n> > +++ b/src/libcamera/control_serializer.cpp\n> > @@ -16,6 +16,7 @@\n> >\n> > #include \n> > #include \n> > +#include \n> > #include \n> >\n> > #include \n> > @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > enum ipa_controls_id_map_type idMapType;\n> > if (idmap == &controls::controls)\n> > idMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > + else if (idmap == &options::options)\n> > + idMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> > else if (idmap == &properties::properties)\n> > idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > else\n> > @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,\n> > enum ipa_controls_id_map_type idMapType;\n> > if (idmap == &controls::controls)\n> > idMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > + else if (idmap == &options::options)\n> > + idMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> > else if (idmap == &properties::properties)\n> > idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > else\n> > @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > case IPA_CONTROL_ID_MAP_CONTROLS:\n> > idMap = &controls::controls;\n> > break;\n> > + case IPA_CONTROL_ID_MAP_OPTIONS:\n> > + idMap = &options::options;\n> > + break;\n> > case IPA_CONTROL_ID_MAP_PROPERTIES:\n> > idMap = &properties::properties;\n> > break;\n> > @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > idMap = &controls::controls;\n> > break;\n> >\n> > + case IPA_CONTROL_ID_MAP_OPTIONS:\n> > + idMap = &options::options;\n> > + break;\n> > +\n> > case IPA_CONTROL_ID_MAP_PROPERTIES:\n> > idMap = &properties::properties;\n> > break;\n> > diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in\n> > new file mode 100644\n> > index 00000000..adfd0633\n> > --- /dev/null\n> > +++ b/src/libcamera/option_ids.cpp.in\n> > @@ -0,0 +1,58 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2022, Google Inc.\n> > + *\n> > + * option_ids.cpp : Option ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#include \n> > +\n> > +/**\n> > + * \\file option_ids.h\n> > + * \\brief Camera option identifiers\n> > + */\n> > +\n> > +namespace libcamera {\n> > +\n> > +/**\n> > + * \\brief Namespace for libcamera options\n> > + */\n> > +namespace options {\n> > +\n> > +${controls_doc}\n> > +\n> > +/**\n> > + * \\brief Namespace for libcamera draft options\n> > + */\n> > +namespace draft {\n> > +\n> > +${draft_controls_doc}\n> > +\n> > +} /* namespace draft */\n> > +\n> > +#ifndef __DOXYGEN__\n> > +/*\n> > + * Keep the options definitions hidden from doxygen as it incorrectly parses\n> > + * them as functions.\n> > + */\n> > +${controls_def}\n> > +\n> > +namespace draft {\n> > +\n> > +${draft_controls_def}\n> > +\n> > +} /* namespace draft */\n> > +#endif\n> > +\n> > +/**\n> > + * \\brief List of all supported libcamera options\n> > + */\n> > +extern const ControlIdMap options {\n> > +${controls_map}\n> > +};\n> > +\n> > +} /* namespace options */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml\n> > new file mode 100644\n> > index 00000000..3f9304be\n> > --- /dev/null\n> > +++ b/src/libcamera/option_ids.yaml\n> > @@ -0,0 +1,16 @@\n> > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > +#\n> > +# Copyright (C) 2022, Google Inc.\n> > +#\n> > +%YAML 1.2\n> > +---\n> > +controls:\n> > + - PipelineConfigFile:\n> > + type: string\n> > + description: |\n> > + Some platform supports customizing pipeline configuration preference\n> > + for a camera module. The structure and contents of the configuration\n> > + file is specific to each platform in YAML format. The PipelineConfigFile\n> > + option can be used to specify the path to load the configuration file.\n>\n> I'm not totally thrilled about using ControlList for this, I feel it may\n> be a bit over-engineered, but I suppose I'll understand the rationale\n> better after reviewing the whole series.\n\n>\n> > +\n> > +...\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 53900BF415\n\tfor ;\n\tWed, 16 Feb 2022 11:43:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 952166112F;\n\tWed, 16 Feb 2022 12:43:21 +0100 (CET)","from mail-oo1-xc2f.google.com (mail-oo1-xc2f.google.com\n\t[IPv6:2607:f8b0:4864:20::c2f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id EE8CC601FF\n\tfor ;\n\tWed, 16 Feb 2022 12:43:19 +0100 (CET)","by mail-oo1-xc2f.google.com with SMTP id\n\tx6-20020a4a4106000000b003193022319cso2136112ooa.4\n\tfor ;\n\tWed, 16 Feb 2022 03:43:19 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=chromium.org header.i=@chromium.org\n\theader.b=\"cEqSfpvn\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org;\n\ts=google; \n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Xvmh979lIkGVboESPM1DSlc/1r1AHSrD0r7M2uwtNEw=;\n\tb=cEqSfpvnfb4JjvYHHCUCdRChLwAQq1KIq6Ht9KspDxmq0EGciGlRonhKKswoZ8PrjU\n\tdf6qkDlqQGxie2VljeaxpVKR5tXQ41jfwvO/c3UMKa8PcNyAY7LIGZKEP8C2XxUuosF5\n\tu0+eE84qS4AJ0digr748BTIEsGhQ9LgeTCXTU=","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Xvmh979lIkGVboESPM1DSlc/1r1AHSrD0r7M2uwtNEw=;\n\tb=cjyp2cemXcbK9ktEtUePyktgtiZ+EgUynDhbaJWQTZPeP8DDM463QUbqr41bunU6jT\n\t8AT+jcDHZw9/o2aWOUpYsrIcwtKf/hF8PYc1tstZUET1f8MMfOhfml0+bdaQu0+K0Kpg\n\t3/rTKj3P5WB8GsuGCZYVM9WPZv5VQ40RSY2MHATB4er984wxo0hbpuMYh/PtL8NiLIFp\n\tOmqxb6dF5a1FBOj5jLcqpf4Jv3aIPu6+/PI+hZ9EaozEJCwb1eWOIl3NQafB00rFgpTE\n\tjF1cvscGMM7x6RGkDykPEwfUWLFrB0AMjcz0eLaj3Djru7+vxOg9fziJ6nlykitObX7L\n\tqA9A==","X-Gm-Message-State":"AOAM533lRoj8GptGv9aCknKPghfSx7Cw07aTY8WmcU1g3g8UHGac0KFZ\n\t5cc9zOAoixqLHnc/eiYWm2t0p0TvZkSrsveZbvwmNw==","X-Google-Smtp-Source":"ABdhPJxb6znTjUoJjkIy69A0vnEAYMgCc6WRQddjqguBVPdFm+VfHwUfhXqLvKr+Y1Kssi83jVhxI2qbxyd8mOfxnM0=","X-Received":"by 2002:a05:6870:310d:b0:bd:e3e9:d242 with SMTP id\n\tv13-20020a056870310d00b000bde3e9d242mr299876oaa.142.1645011798490;\n\tWed, 16 Feb 2022 03:43:18 -0800 (PST)","MIME-Version":"1.0","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-2-hanlinchen@chromium.org>\n\t","In-Reply-To":"","From":"Hanlin Chen ","Date":"Wed, 16 Feb 2022 19:43:07 +0800","Message-ID":"","To":"Laurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","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":22597,"web_url":"https://patchwork.libcamera.org/comment/22597/","msgid":"","date":"2022-04-06T00:52:15","subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Han-Lin,\n\nOn Wed, Feb 16, 2022 at 07:43:07PM +0800, Hanlin Chen wrote:\n> On Wed, Feb 16, 2022 at 8:57 AM Laurent Pinchart wrote:\n> > On Wed, Feb 09, 2022 at 03:19:09PM +0800, Han-Lin Chen wrote:\n> > > Introduce option to customize behavior for a camera module, which might\n> > > alter the camera characteristic and should be set before inspecting it.\n> > >\n> > > Re-use the Control generation infrastructure to generate options and define\n> > > the first 'PipelineConfigFile' option, which allows user to specify\n> > > preference for pipeline configuration.\n> >\n> > When thinking about this, I wasn't envisioning passing the configuration\n> > file to libcamera through the C++ API, but looking for it in standard\n> > locations on the file system. Could you explain the reason why you think\n> > this should be dynamic ?\n> \n> What I had in mind is to let the application choose the preference per\n> camera module, and even with the same sensor,\n> the user might still want different tunings. And we don't need to have\n> a big configuration file for all parameters.\n> The option may extend to what we used to set in environment variables,\n> like LIBCAMERA_RPI_TUNING_FILE for example.\n\nYes, that's nice indeed. When it comes to the API between the pipeline\nhandler and the IPA module, I think using just a file name would be good\nenough. When it comes to the application-facing API, however, it's a\ngood question. I think I need to sleep over it. One thing I'd like to\nsee is how this will work in the default case, when the application\ndoesn't provide any option. Will we have a system-wide top-level\nconfiguration file that will contain the paths to per-camera IPA\nconfiguration (tuning) files ? If so, would the application be allowrd\nto specify a custom top-level configuration file too ? And possible both\na custom top-level configuration file and also IPA configuration files,\nor would the custom top-level file need to point to custom IPA files to\noverride the latter ?\n\n> Yes, I think it's up for discussion, and depends on how we think it's\n> useful and whether it can extend.\n> If this is the only use case, I do agree it's over-engineered :|\n> \n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > include/libcamera/ipa/ipa_controls.h | 1 +\n> > > include/libcamera/meson.build | 3 +-\n> > > include/libcamera/option_ids.h.in | 36 +++++++++++++++++\n> > > src/libcamera/control_serializer.cpp | 12 ++++++\n> > > src/libcamera/option_ids.cpp.in | 58 ++++++++++++++++++++++++++++\n> > > src/libcamera/option_ids.yaml | 16 ++++++++\n> > > 6 files changed, 125 insertions(+), 1 deletion(-)\n> > > create mode 100644 include/libcamera/option_ids.h.in\n> > > create mode 100644 src/libcamera/option_ids.cpp.in\n> > > create mode 100644 src/libcamera/option_ids.yaml\n> > >\n> > > diff --git a/include/libcamera/ipa/ipa_controls.h b/include/libcamera/ipa/ipa_controls.h\n> > > index da1a7596..8173a2e5 100644\n> > > --- a/include/libcamera/ipa/ipa_controls.h\n> > > +++ b/include/libcamera/ipa/ipa_controls.h\n> > > @@ -18,6 +18,7 @@ extern \"C\" {\n> > > enum ipa_controls_id_map_type {\n> > > IPA_CONTROL_ID_MAP_CONTROLS,\n> > > IPA_CONTROL_ID_MAP_PROPERTIES,\n> > > + IPA_CONTROL_ID_MAP_OPTIONS,\n> >\n> > Do you have use cases for passing a configuration file to the IPA\n> > modules ? As IPA modules are sandboxed, they don't have unrestricted\n> > access to the filesystem. I think this could create issues, especially\n> > if the path to the configuration is under the control of an application.\n> \n> I've seen cases that read the content of a file and pass it to IPA\n> with a FD to avoid the problem.\n> In fact, for ChromeOS, we control the access of IPA to a white-listed\n> locations, so it's something I can configure.\n> For other platforms, I think we need a definition of \"sandboxed\" and\n> who should do the sandboxing, like libcamera or the device vendor?\n\nAgreed, and that's an interesting discussion. We've always envisioned\nthe sandboxing to be under the control of the system integrator, with\nlibcamera providing a default implementation for reference. There is\nhowever no reason to keep the default and Chrome OS sandboxing\nimplementation separate just for the sake of it, if the Chrome OS\nimplementation ends up being suitable as a default. We are already using\nthe mojom IDL internally, and I'd like to modify the serialization\nbinary format to match mojo too (there's even a GSoC proposal for that\nif I recall properly). We however can't depend on the rest of mojo in\nthe default implementation, but I'm increasingly thinking that an IPA\ndaemon is likely a better option that spawning a process manually from\nlibcamera, but we may need to keep spawning as a fallback method, for\nunit tests. We would also need to find a way to make testing without\ninstalling easy (libcamera test applications can be run from the build\ndirectory even if a different version of libcamera is available\nsystem-wide, and that's a very very useful feature during the\ndevelopment that I want to keep). Lots of questions :-)\n\nAnyway, back to the more immediate topic, accessing a tuning file\ndirectly in the IPA module is fine with me for now (we do so already for\nRPi). I'm however not sure we need to pass its name through a\nControlList, we have a dedicated function parameter (or structure field,\nI don't remember) for RPi, and that seems good enough for now.\n\n> > > IPA_CONTROL_ID_MAP_V4L2,\n> > > };\n> > >\n> > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build\n> > > index 408b7acf..a9b71bc3 100644\n> > > --- a/include/libcamera/meson.build\n> > > +++ b/include/libcamera/meson.build\n> > > @@ -31,10 +31,11 @@ install_headers(libcamera_public_headers,\n> > >\n> > > libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir\n> > >\n> > > -# control_ids.h and property_ids.h\n> > > +# control_ids.h, property_ids.h, and option_ids.h\n\n\"option\" comes before \"property\" :-)\n\n> > > control_source_files = [\n> > > 'control_ids',\n> > > 'property_ids',\n> > > + 'option_ids',\n\nSame here.\n\n> > > ]\n> > >\n> > > control_headers = []\n> > > diff --git a/include/libcamera/option_ids.h.in b/include/libcamera/option_ids.h.in\n> > > new file mode 100644\n> > > index 00000000..53058a4b\n> > > --- /dev/null\n> > > +++ b/include/libcamera/option_ids.h.in\n> > > @@ -0,0 +1,36 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Google Inc.\n> > > + *\n> > > + * option_ids.h - Option ID list\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#pragma once\n> > > +\n> > > +#include \n> > > +\n> > > +#include \n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +namespace options {\n> > > +\n> > > +enum {\n> > > +${ids}\n> > > +};\n> > > +\n> > > +${controls}\n> > > +\n> > > +namespace draft {\n> > > +\n> > > +${draft_controls}\n> > > +\n> > > +} /* namespace draft */\n> > > +\n> > > +extern const ControlIdMap options;\n> > > +\n> > > +} /* namespace options */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/control_serializer.cpp b/src/libcamera/control_serializer.cpp\n> > > index e87d2362..dc8d2906 100644\n> > > --- a/src/libcamera/control_serializer.cpp\n> > > +++ b/src/libcamera/control_serializer.cpp\n> > > @@ -16,6 +16,7 @@\n> > >\n> > > #include \n> > > #include \n> > > +#include \n> > > #include \n> > >\n> > > #include \n> > > @@ -239,6 +240,8 @@ int ControlSerializer::serialize(const ControlInfoMap &infoMap,\n> > > enum ipa_controls_id_map_type idMapType;\n> > > if (idmap == &controls::controls)\n> > > idMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > > + else if (idmap == &options::options)\n> > > + idMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> > > else if (idmap == &properties::properties)\n> > > idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > > else\n> > > @@ -333,6 +336,8 @@ int ControlSerializer::serialize(const ControlList &list,\n> > > enum ipa_controls_id_map_type idMapType;\n> > > if (idmap == &controls::controls)\n> > > idMapType = IPA_CONTROL_ID_MAP_CONTROLS;\n> > > + else if (idmap == &options::options)\n> > > + idMapType = IPA_CONTROL_ID_MAP_OPTIONS;\n> > > else if (idmap == &properties::properties)\n> > > idMapType = IPA_CONTROL_ID_MAP_PROPERTIES;\n> > > else\n> > > @@ -458,6 +463,9 @@ ControlInfoMap ControlSerializer::deserialize(ByteStreamBuffer &\n> > > case IPA_CONTROL_ID_MAP_CONTROLS:\n> > > idMap = &controls::controls;\n> > > break;\n> > > + case IPA_CONTROL_ID_MAP_OPTIONS:\n> > > + idMap = &options::options;\n> > > + break;\n> > > case IPA_CONTROL_ID_MAP_PROPERTIES:\n> > > idMap = &properties::properties;\n> > > break;\n> > > @@ -589,6 +597,10 @@ ControlList ControlSerializer::deserialize(ByteStreamBuffer &buffer\n> > > idMap = &controls::controls;\n> > > break;\n> > >\n> > > + case IPA_CONTROL_ID_MAP_OPTIONS:\n> > > + idMap = &options::options;\n> > > + break;\n> > > +\n> > > case IPA_CONTROL_ID_MAP_PROPERTIES:\n> > > idMap = &properties::properties;\n> > > break;\n> > > diff --git a/src/libcamera/option_ids.cpp.in b/src/libcamera/option_ids.cpp.in\n> > > new file mode 100644\n> > > index 00000000..adfd0633\n> > > --- /dev/null\n> > > +++ b/src/libcamera/option_ids.cpp.in\n> > > @@ -0,0 +1,58 @@\n> > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > +/*\n> > > + * Copyright (C) 2022, Google Inc.\n> > > + *\n> > > + * option_ids.cpp : Option ID list\n> > > + *\n> > > + * This file is auto-generated. Do not edit.\n> > > + */\n> > > +\n> > > +#include \n> > > +\n> > > +/**\n> > > + * \\file option_ids.h\n> > > + * \\brief Camera option identifiers\n> > > + */\n> > > +\n> > > +namespace libcamera {\n> > > +\n> > > +/**\n> > > + * \\brief Namespace for libcamera options\n> > > + */\n> > > +namespace options {\n> > > +\n> > > +${controls_doc}\n> > > +\n> > > +/**\n> > > + * \\brief Namespace for libcamera draft options\n> > > + */\n> > > +namespace draft {\n> > > +\n> > > +${draft_controls_doc}\n> > > +\n> > > +} /* namespace draft */\n> > > +\n> > > +#ifndef __DOXYGEN__\n> > > +/*\n> > > + * Keep the options definitions hidden from doxygen as it incorrectly parses\n> > > + * them as functions.\n> > > + */\n> > > +${controls_def}\n> > > +\n> > > +namespace draft {\n> > > +\n> > > +${draft_controls_def}\n> > > +\n> > > +} /* namespace draft */\n> > > +#endif\n> > > +\n> > > +/**\n> > > + * \\brief List of all supported libcamera options\n> > > + */\n> > > +extern const ControlIdMap options {\n> > > +${controls_map}\n> > > +};\n> > > +\n> > > +} /* namespace options */\n> > > +\n> > > +} /* namespace libcamera */\n> > > diff --git a/src/libcamera/option_ids.yaml b/src/libcamera/option_ids.yaml\n> > > new file mode 100644\n> > > index 00000000..3f9304be\n> > > --- /dev/null\n> > > +++ b/src/libcamera/option_ids.yaml\n> > > @@ -0,0 +1,16 @@\n> > > +# SPDX-License-Identifier: LGPL-2.1-or-later\n> > > +#\n> > > +# Copyright (C) 2022, Google Inc.\n> > > +#\n> > > +%YAML 1.2\n> > > +---\n> > > +controls:\n> > > + - PipelineConfigFile:\n> > > + type: string\n> > > + description: |\n> > > + Some platform supports customizing pipeline configuration preference\n> > > + for a camera module. The structure and contents of the configuration\n> > > + file is specific to each platform in YAML format. The PipelineConfigFile\n> > > + option can be used to specify the path to load the configuration file.\n> >\n> > I'm not totally thrilled about using ControlList for this, I feel it may\n> > be a bit over-engineered, but I suppose I'll understand the rationale\n> > better after reviewing the whole series.\n> >\n> > > +\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 310A0C3256\n\tfor ;\n\tWed, 6 Apr 2022 00:52:22 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 858AC6563F;\n\tWed, 6 Apr 2022 02:52:21 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5E15E604B6\n\tfor ;\n\tWed, 6 Apr 2022 02:52:19 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id D984C482;\n\tWed, 6 Apr 2022 02:52:18 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1649206341;\n\tbh=/EMVaYxVdNSzF1NKQ5QxHvTS3Kna4BMKvgenmadenDg=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=rnXN3jM7RTz5P+xVrAG7IjepTPIbQhkT9IvgL5Ay4UvHRtu3wcqcobvmJeMsiRb2F\n\tZ3EqGomJ5DGU7k8Z//w+UVKLvo8hkBlRhQ9THaF+MZ36r+TgQp6SypmdQR3D38dL3U\n\tp3AQIV3z1Ija+e6A57UUcflXvQDHcv8ED1qALELeCpsaBfw76jEwvGfLBIuPricrC4\n\tu7WHqDnVzI8S3+VGCIgqK16SbDk5oQ83mR2cM5yZw/9QOQr8FUB1X6y16ZH6egubEJ\n\tpxFPfjSVZV/uaWAdhbbgJQg3TGZd96z5asC90O3gQJUyb6hIul9dz2IJrUHRKCUqYb\n\tOQgW++ossUG/g==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1649206339;\n\tbh=/EMVaYxVdNSzF1NKQ5QxHvTS3Kna4BMKvgenmadenDg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=R0skRsY4ondDhaYPAvGUJLmsV0pUVN9yZZJC+pJBAOrwqBtcKm8T47/ZoStTI1WXc\n\tO7suMqWXHfhluZnE5tHjLZUYbNaY2l1bMyL2Igx+T/MbX9brLc7aWwCSUkOKlwKfst\n\ta8Ax7Q538olm0Yozendt42a87HXJdVcyDLFcjukE="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"R0skRsY4\"; dkim-atps=neutral","Date":"Wed, 6 Apr 2022 03:52:15 +0300","To":"Hanlin Chen ","Message-ID":"","References":"<20220209071917.559993-1-hanlinchen@chromium.org>\n\t<20220209071917.559993-2-hanlinchen@chromium.org>\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/9] libcamera: Introduce option to\n\tcustomize behavior for a camera module","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","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]