[{"id":27735,"web_url":"https://patchwork.libcamera.org/comment/27735/","msgid":"","date":"2023-09-05T07:42:02","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Kieran\n\nOn Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote:\n> The Control framework generates an enumeration for each control and\n> property entry which is used to support the ControlIdMap for each.\n>\n> This enum listing provides hardcoded integer references for each control\n> and is a source of potential ABI breakage when introducing new controls.\n>\n> The enum values are expected to only be used internally, so reduce\n> public ABI exposure when modifying controls by moving the enum to new\n> internal header definitions.\n>\n> Signed-off-by: Kieran Bingham \n> ---\n> While trying to work towards a new 'patch' release, I hit the following\n> abi-compatbility issues:\n>\n> Binary compatibility: 99.4%\n> Source compatibility: 99.7%\n> Total binary compatibility problems: 2, warnings: 0\n> Total source compatibility problems: 2, warnings: 0\n> Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html\n>\n> This stems from the removal of the following symbols:\n>\n> Removed Symbols 2\n> control_ids.h, libcamera.so.0.1.0\n> namespace libcamera::controls::draft\n> SceneFlicker [data]\n> SceneFlickerValues [data]\n>\n> And also the introduction of four new controls:\n> AeFlickerDetected [data]\n> AeFlickerMode [data]\n> AeFlickerModeValues [data]\n> AeFlickerPeriod [data]\n>\n> All of these changes affect the enum values generated to produce the\n> ControlIdMap.\n>\n> This proposal aims to remove this from the Public ABI/API to prevent\n> potential issues on every modification to the control list.\n>\n> Of course - removing a draft control or renaming the controls is still\n> going to cause linkage problems with any application which may use those\n> symbols.\n>\n> In this specific case, I suspect we would be lucky as SceneFlicker and\n> SceneFlickerValues are likely to be unused as they were otherwise\n> unimplemented.\n>\n> include/libcamera/control_ids.h.in | 4 ----\n> include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++\n> include/libcamera/internal/meson.build | 20 ++++++++++++++++\n> include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++\n> include/libcamera/property_ids.h.in | 4 ----\n> src/ipa/rpi/common/ipa_base.cpp | 3 +++\n> src/libcamera/control_ids.cpp.in | 4 +++-\n> src/libcamera/meson.build | 1 +\n> src/libcamera/property_ids.cpp.in | 3 ++-\n> 9 files changed, 77 insertions(+), 10 deletions(-)\n> create mode 100644 include/libcamera/internal/control_ids.h.in\n> create mode 100644 include/libcamera/internal/property_ids.h.in\n>\n> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> index 0718a8886f6c..a0c66192fdc9 100644\n> --- a/include/libcamera/control_ids.h.in\n> +++ b/include/libcamera/control_ids.h.in\n> @@ -18,10 +18,6 @@ namespace libcamera {\n>\n> namespace controls {\n>\n> -enum {\n> -${ids}\n> -};\n> -\n> ${controls}\n>\n> extern const ControlIdMap controls;\n> diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in\n> new file mode 100644\n> index 000000000000..c9bfaf119578\n> --- /dev/null\n> +++ b/include/libcamera/internal/control_ids.h.in\n> @@ -0,0 +1,24 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * control_ids.h - Internal Control ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace controls {\n> +\n> +enum {\n> +${ids}\n> +};\n> +\n> +} /* namespace controls */\n> +\n> +} /* namespace libcamera */\n> diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> index 7f1f344014c4..dc7fa83ed978 100644\n> --- a/include/libcamera/internal/meson.build\n> +++ b/include/libcamera/internal/meson.build\n> @@ -47,4 +47,24 @@ libcamera_internal_headers = files([\n> 'yaml_parser.h',\n> ])\n>\n> +# Internal control and property ID mappings\n> +internal_control_source_files = [\n> + 'control_ids',\n> + 'property_ids',\n> +]\n> +\n> +internal_control_headers = []\n> +\n> +foreach header : internal_control_source_files\n> + input_files = files('../../../src/libcamera/' + header + '.yaml',\n> + header + '.h.in')\n> + internal_control_headers += custom_target(header + '_h',\n> + input : input_files,\n> + output : header + '.h',\n> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> + install : false)\n> +endforeach\n> +\n> +libcamera_internal_headers += internal_control_headers\n> +\n> subdir('converter')\n> diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in\n> new file mode 100644\n> index 000000000000..15f4950953b4\n> --- /dev/null\n> +++ b/include/libcamera/internal/property_ids.h.in\n> @@ -0,0 +1,24 @@\n> +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> +/*\n> + * Copyright (C) 2019, Google Inc.\n> + *\n> + * property_ids.h - Internal Property ID list\n> + *\n> + * This file is auto-generated. Do not edit.\n> + */\n> +\n> +#pragma once\n> +\n> +#include \n> +\n> +namespace libcamera {\n> +\n> +namespace properties {\n> +\n> +enum {\n> +${ids}\n> +};\n> +\n> +} /* namespace properties */\n> +\n> +} /* namespace libcamera */\n\nToo bad we have users of the enumerated values, otherwise this could\nbe hidden in the generated .cpp file\n\n> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> index ff0194083af0..0fbdcc0c8504 100644\n> --- a/include/libcamera/property_ids.h.in\n> +++ b/include/libcamera/property_ids.h.in\n> @@ -17,10 +17,6 @@ namespace libcamera {\n>\n> namespace properties {\n>\n> -enum {\n> -${ids}\n> -};\n> -\n> ${controls}\n>\n> namespace draft {\n> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> index a47ae3a9e5cb..8e083396cb01 100644\n> --- a/src/ipa/rpi/common/ipa_base.cpp\n> +++ b/src/ipa/rpi/common/ipa_base.cpp\n> @@ -11,9 +11,12 @@\n>\n> #include \n> #include \n> +\n> #include \n> #include \n>\n> +#include \"libcamera/internal/control_ids.h\"\n> +\n> #include \"controller/af_algorithm.h\"\n> #include \"controller/af_status.h\"\n> #include \"controller/agc_algorithm.h\"\n> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> index 5fb1c2c30558..20ef147ab826 100644\n> --- a/src/libcamera/control_ids.cpp.in\n> +++ b/src/libcamera/control_ids.cpp.in\n> @@ -10,8 +10,10 @@\n> #include \n> #include \n>\n> +#include \"libcamera/internal/control_ids.h\"\n> +\n> /**\n> - * \\file control_ids.h\n> + * \\file libcamera/control_ids.h\n> * \\brief Camera control identifiers\n> */\n>\n> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> index b24f82965764..e7f5edb2f39b 100644\n> --- a/src/libcamera/meson.build\n> +++ b/src/libcamera/meson.build\n> @@ -131,6 +131,7 @@ foreach source : control_source_files\n> control_sources += custom_target(source + '_cpp',\n> input : input_files,\n> output : source + '.cpp',\n> + depends : internal_control_headers,\n> command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> endforeach\n>\n> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> index f917e3349766..c7dbf9838411 100644\n> --- a/src/libcamera/property_ids.cpp.in\n> +++ b/src/libcamera/property_ids.cpp.in\n> @@ -7,10 +7,11 @@\n> * This file is auto-generated. Do not edit.\n> */\n>\n> +#include \"libcamera/internal/property_ids.h\"\n> #include \n\nDo we want the public header to be included by the private ? (This for\nboth properties and controls)\n\nMinor questions apart, as long as we don't want applications to use\nthe enumared ids (and I don't think we want and I can't think of use\ncases why we should) I think this is a good change!\n\nThanks\n j\n\n>\n> /**\n> - * \\file property_ids.h\n> + * \\file libcamera/property_ids.h\n> * \\brief Camera property identifiers\n> */\n>\n> --\n> 2.34.1\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 65EA2BE080\n\tfor ;\n\tTue, 5 Sep 2023 07:42:08 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 72191628EB;\n\tTue, 5 Sep 2023 09:42:07 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0318B627DF\n\tfor ;\n\tTue, 5 Sep 2023 09:42:05 +0200 (CEST)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 3A6D775A;\n\tTue, 5 Sep 2023 09:40:39 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693899727;\n\tbh=2JhC12SzAztvIwT8DTaVIghvSrVeL6CmEPQG/k9+zTE=;\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=hVKqBV14vJR/aV45b0rt9+MTf1RclWXQ7d/km25/78CsjXSrgQma3wFjTonSuz3rV\n\twdPbRML+6i4mb3JLqhSYQFMrNZlBfozyQ2TVgA6SEGHWN2BRbrcJyzGdk2vuNH9lri\n\thKEBMvO8C+oifcAF09PZUXoV4q99SizTN+k8pDcv76NU6mCSn62M6IebTJ5AyGNezG\n\tU922joyJwXZr09L+gyW6PCrEdeyM/UYjZ4lUzHpkhAUIWQAWcSiWbB1o3erDaJlKFw\n\tVFETgqHX/ZvUCPhdrdZDt1fyrJTnZov4/+wp47AvZWfcQSJjuFsD8CANEF4+wpEV39\n\tR1SOVK2rZ70hA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693899639;\n\tbh=2JhC12SzAztvIwT8DTaVIghvSrVeL6CmEPQG/k9+zTE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=tkww7dKHGuIAi4E/12fC/UGp9KfCpFrF5p6S0f7Dd3M4iKs5m3+dtcauwMvbB8kkP\n\t5FZuKXhE6Iw61MYuF7+566wDu6eCMC8FalRsWVi13AJ6Vdrao3ZxBYIitXX4vILK1g\n\ttrUBtkLhwHv0GzFls/HcNmksqKZksFPrHsd0+0xM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"tkww7dKH\"; dkim-atps=neutral","Date":"Tue, 5 Sep 2023 09:42:02 +0200","To":"Kieran Bingham ","Message-ID":"","References":"<20230904215954.3417076-1-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230904215954.3417076-1-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","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":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27737,"web_url":"https://patchwork.libcamera.org/comment/27737/","msgid":"<20230905111350.GE11700@pendragon.ideasonboard.com>","date":"2023-09-05T11:13:50","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Tue, Sep 05, 2023 at 09:42:02AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> On Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > The Control framework generates an enumeration for each control and\n> > property entry which is used to support the ControlIdMap for each.\n> >\n> > This enum listing provides hardcoded integer references for each control\n> > and is a source of potential ABI breakage when introducing new controls.\n> >\n> > The enum values are expected to only be used internally, so reduce\n> > public ABI exposure when modifying controls by moving the enum to new\n> > internal header definitions.\n> >\n> > Signed-off-by: Kieran Bingham \n> > ---\n> > While trying to work towards a new 'patch' release, I hit the following\n> > abi-compatbility issues:\n> >\n> > Binary compatibility: 99.4%\n> > Source compatibility: 99.7%\n> > Total binary compatibility problems: 2, warnings: 0\n> > Total source compatibility problems: 2, warnings: 0\n> > Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html\n> >\n> > This stems from the removal of the following symbols:\n> >\n> > Removed Symbols 2\n> > control_ids.h, libcamera.so.0.1.0\n> > namespace libcamera::controls::draft\n> > SceneFlicker [data]\n> > SceneFlickerValues [data]\n> >\n> > And also the introduction of four new controls:\n> > AeFlickerDetected [data]\n> > AeFlickerMode [data]\n> > AeFlickerModeValues [data]\n> > AeFlickerPeriod [data]\n> >\n> > All of these changes affect the enum values generated to produce the\n> > ControlIdMap.\n> >\n> > This proposal aims to remove this from the Public ABI/API to prevent\n> > potential issues on every modification to the control list.\n> >\n> > Of course - removing a draft control or renaming the controls is still\n> > going to cause linkage problems with any application which may use those\n> > symbols.\n> >\n> > In this specific case, I suspect we would be lucky as SceneFlicker and\n> > SceneFlickerValues are likely to be unused as they were otherwise\n> > unimplemented.\n> >\n> > include/libcamera/control_ids.h.in | 4 ----\n> > include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++\n> > include/libcamera/internal/meson.build | 20 ++++++++++++++++\n> > include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++\n> > include/libcamera/property_ids.h.in | 4 ----\n> > src/ipa/rpi/common/ipa_base.cpp | 3 +++\n> > src/libcamera/control_ids.cpp.in | 4 +++-\n> > src/libcamera/meson.build | 1 +\n> > src/libcamera/property_ids.cpp.in | 3 ++-\n> > 9 files changed, 77 insertions(+), 10 deletions(-)\n> > create mode 100644 include/libcamera/internal/control_ids.h.in\n> > create mode 100644 include/libcamera/internal/property_ids.h.in\n> >\n> > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> > index 0718a8886f6c..a0c66192fdc9 100644\n> > --- a/include/libcamera/control_ids.h.in\n> > +++ b/include/libcamera/control_ids.h.in\n> > @@ -18,10 +18,6 @@ namespace libcamera {\n> >\n> > namespace controls {\n> >\n> > -enum {\n> > -${ids}\n> > -};\n> > -\n> > ${controls}\n> >\n> > extern const ControlIdMap controls;\n> > diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in\n> > new file mode 100644\n> > index 000000000000..c9bfaf119578\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/control_ids.h.in\n> > @@ -0,0 +1,24 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * control_ids.h - Internal Control ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace controls {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +} /* namespace controls */\n> > +\n> > +} /* namespace libcamera */\n> > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > index 7f1f344014c4..dc7fa83ed978 100644\n> > --- a/include/libcamera/internal/meson.build\n> > +++ b/include/libcamera/internal/meson.build\n> > @@ -47,4 +47,24 @@ libcamera_internal_headers = files([\n> > 'yaml_parser.h',\n> > ])\n> >\n> > +# Internal control and property ID mappings\n> > +internal_control_source_files = [\n> > + 'control_ids',\n> > + 'property_ids',\n> > +]\n> > +\n> > +internal_control_headers = []\n> > +\n> > +foreach header : internal_control_source_files\n> > + input_files = files('../../../src/libcamera/' + header + '.yaml',\n> > + header + '.h.in')\n> > + internal_control_headers += custom_target(header + '_h',\n> > + input : input_files,\n> > + output : header + '.h',\n> > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > + install : false)\n> > +endforeach\n> > +\n> > +libcamera_internal_headers += internal_control_headers\n> > +\n> > subdir('converter')\n> > diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in\n> > new file mode 100644\n> > index 000000000000..15f4950953b4\n> > --- /dev/null\n> > +++ b/include/libcamera/internal/property_ids.h.in\n> > @@ -0,0 +1,24 @@\n> > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > +/*\n> > + * Copyright (C) 2019, Google Inc.\n> > + *\n> > + * property_ids.h - Internal Property ID list\n> > + *\n> > + * This file is auto-generated. Do not edit.\n> > + */\n> > +\n> > +#pragma once\n> > +\n> > +#include \n> > +\n> > +namespace libcamera {\n> > +\n> > +namespace properties {\n> > +\n> > +enum {\n> > +${ids}\n> > +};\n> > +\n> > +} /* namespace properties */\n> > +\n> > +} /* namespace libcamera */\n> \n> Too bad we have users of the enumerated values, otherwise this could\n> be hidden in the generated .cpp file\n> \n> > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > index ff0194083af0..0fbdcc0c8504 100644\n> > --- a/include/libcamera/property_ids.h.in\n> > +++ b/include/libcamera/property_ids.h.in\n> > @@ -17,10 +17,6 @@ namespace libcamera {\n> >\n> > namespace properties {\n> >\n> > -enum {\n> > -${ids}\n> > -};\n> > -\n> > ${controls}\n> >\n> > namespace draft {\n> > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > index a47ae3a9e5cb..8e083396cb01 100644\n> > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > @@ -11,9 +11,12 @@\n> >\n> > #include \n> > #include \n> > +\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/control_ids.h\"\n> > +\n> > #include \"controller/af_algorithm.h\"\n> > #include \"controller/af_status.h\"\n> > #include \"controller/agc_algorithm.h\"\n> > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > index 5fb1c2c30558..20ef147ab826 100644\n> > --- a/src/libcamera/control_ids.cpp.in\n> > +++ b/src/libcamera/control_ids.cpp.in\n> > @@ -10,8 +10,10 @@\n> > #include \n> > #include \n> >\n> > +#include \"libcamera/internal/control_ids.h\"\n> > +\n> > /**\n> > - * \\file control_ids.h\n> > + * \\file libcamera/control_ids.h\n> > * \\brief Camera control identifiers\n> > */\n> >\n> > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > index b24f82965764..e7f5edb2f39b 100644\n> > --- a/src/libcamera/meson.build\n> > +++ b/src/libcamera/meson.build\n> > @@ -131,6 +131,7 @@ foreach source : control_source_files\n> > control_sources += custom_target(source + '_cpp',\n> > input : input_files,\n> > output : source + '.cpp',\n> > + depends : internal_control_headers,\n> > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > endforeach\n> >\n> > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > index f917e3349766..c7dbf9838411 100644\n> > --- a/src/libcamera/property_ids.cpp.in\n> > +++ b/src/libcamera/property_ids.cpp.in\n> > @@ -7,10 +7,11 @@\n> > * This file is auto-generated. Do not edit.\n> > */\n> >\n> > +#include \"libcamera/internal/property_ids.h\"\n> > #include \n> \n> Do we want the public header to be included by the private ? (This for\n> both properties and controls)\n\nI would do so, yes. It makes sense and is simpler for the user.\n\n> Minor questions apart, as long as we don't want applications to use\n> the enumared ids (and I don't think we want and I can't think of use\n> cases why we should) I think this is a good change!\n\nShining a bit of light on future plans, we'll have a C API, and\nnumerical IDs will be part of the ABI. They will obviously need to be\nstable, adding new controls and deprecating existing controls shall not\ncause the ID of any other control to change. For this reason, I would\nprefer keeping the ID part of the public API and ABI.\n\n> > /**\n> > - * \\file property_ids.h\n> > + * \\file libcamera/property_ids.h\n> > * \\brief Camera property identifiers\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 1C752BE080\n\tfor ;\n\tTue, 5 Sep 2023 11:13:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 53838628EC;\n\tTue, 5 Sep 2023 13:13:39 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 14D89628D8\n\tfor ;\n\tTue, 5 Sep 2023 13:13:38 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(ftip006315900.acc1.colindale.21cn-nte.bt.net [81.134.214.249])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6830EAF2;\n\tTue, 5 Sep 2023 13:12:11 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693912419;\n\tbh=b0uyZLD/vGXEQwC9lshA1Lnuwh+1hw4Fu+dbSgky0AI=;\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=GCqDL3cybpNvYTmcS8K9R7vx78Pg7f/1Fh5LDXuFV2EnOY+EpL0UvGxDFPz3NxEwr\n\tvUh5lwl3hV5xRaOlwlwJbIhAuuxp8DGZcbn9F/EA+K2X252UlZu6eg+xUwKF1Fyhh1\n\tazRe7n85od/bOMWqUksoxk3oow9TGIsrIbOdUcLsQKd0ER9EKakuNy0zjFVXvfdcwW\n\tXBnQYJIzJrUfBkWuPiutaNyunfrgqbnUIjySmqzSL9J1Q1xyVchRjZP90PEqzBmYEF\n\tsqROBH7iP/X9leOz9cbb/pjyD/WSZ20ud7N+CwYOJgNwKgMRO3DzQb4R43tKk0s+d/\n\tbe8bDqksWOaHQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693912331;\n\tbh=b0uyZLD/vGXEQwC9lshA1Lnuwh+1hw4Fu+dbSgky0AI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=rswFeauLDxGcXIYtZ70WmO0pP3QtFOfP9RcbIfpXkbqs7NjD6D8Z4lj1ZjCwGfm9/\n\t+x58tiXgwzXV7AU/V79ZsPxcDsoBAe93zgGzHkfJAl3PagYLQf8U0fxQ5gyZgwAWj5\n\tK8lz1ggCekBAAsFkwUB4I6PmTVckbS8uUqjO/0OM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"rswFeauL\"; dkim-atps=neutral","Date":"Tue, 5 Sep 2023 14:13:50 +0300","To":"Jacopo Mondi ","Message-ID":"<20230905111350.GE11700@pendragon.ideasonboard.com>","References":"<20230904215954.3417076-1-kieran.bingham@ideasonboard.com>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","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 ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27742,"web_url":"https://patchwork.libcamera.org/comment/27742/","msgid":"<20230906135307.GB25420@pendragon.ideasonboard.com>","date":"2023-09-06T13:53:07","subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nOn Wed, Sep 06, 2023 at 07:40:40AM +0100, Kieran Bingham wrote:\n> Quoting Laurent Pinchart (2023-09-05 12:13:50)\n> > On Tue, Sep 05, 2023 at 09:42:02AM +0200, Jacopo Mondi via libcamera-devel wrote:\n> > > On Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote:\n> > > > The Control framework generates an enumeration for each control and\n> > > > property entry which is used to support the ControlIdMap for each.\n> > > >\n> > > > This enum listing provides hardcoded integer references for each control\n> > > > and is a source of potential ABI breakage when introducing new controls.\n> > > >\n> > > > The enum values are expected to only be used internally, so reduce\n> > > > public ABI exposure when modifying controls by moving the enum to new\n> > > > internal header definitions.\n> > > >\n> > > > Signed-off-by: Kieran Bingham \n> > > > ---\n> > > > While trying to work towards a new 'patch' release, I hit the following\n> > > > abi-compatbility issues:\n> > > >\n> > > > Binary compatibility: 99.4%\n> > > > Source compatibility: 99.7%\n> > > > Total binary compatibility problems: 2, warnings: 0\n> > > > Total source compatibility problems: 2, warnings: 0\n> > > > Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html\n> > > >\n> > > > This stems from the removal of the following symbols:\n> > > >\n> > > > Removed Symbols 2\n> > > > control_ids.h, libcamera.so.0.1.0\n> > > > namespace libcamera::controls::draft\n> > > > SceneFlicker [data]\n> > > > SceneFlickerValues [data]\n> > > >\n> > > > And also the introduction of four new controls:\n> > > > AeFlickerDetected [data]\n> > > > AeFlickerMode [data]\n> > > > AeFlickerModeValues [data]\n> > > > AeFlickerPeriod [data]\n> > > >\n> > > > All of these changes affect the enum values generated to produce the\n> > > > ControlIdMap.\n> > > >\n> > > > This proposal aims to remove this from the Public ABI/API to prevent\n> > > > potential issues on every modification to the control list.\n> > > >\n> > > > Of course - removing a draft control or renaming the controls is still\n> > > > going to cause linkage problems with any application which may use those\n> > > > symbols.\n> > > >\n> > > > In this specific case, I suspect we would be lucky as SceneFlicker and\n> > > > SceneFlickerValues are likely to be unused as they were otherwise\n> > > > unimplemented.\n> > > >\n> > > > include/libcamera/control_ids.h.in | 4 ----\n> > > > include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++\n> > > > include/libcamera/internal/meson.build | 20 ++++++++++++++++\n> > > > include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++\n> > > > include/libcamera/property_ids.h.in | 4 ----\n> > > > src/ipa/rpi/common/ipa_base.cpp | 3 +++\n> > > > src/libcamera/control_ids.cpp.in | 4 +++-\n> > > > src/libcamera/meson.build | 1 +\n> > > > src/libcamera/property_ids.cpp.in | 3 ++-\n> > > > 9 files changed, 77 insertions(+), 10 deletions(-)\n> > > > create mode 100644 include/libcamera/internal/control_ids.h.in\n> > > > create mode 100644 include/libcamera/internal/property_ids.h.in\n> > > >\n> > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in\n> > > > index 0718a8886f6c..a0c66192fdc9 100644\n> > > > --- a/include/libcamera/control_ids.h.in\n> > > > +++ b/include/libcamera/control_ids.h.in\n> > > > @@ -18,10 +18,6 @@ namespace libcamera {\n> > > >\n> > > > namespace controls {\n> > > >\n> > > > -enum {\n> > > > -${ids}\n> > > > -};\n> > > > -\n> > > > ${controls}\n> > > >\n> > > > extern const ControlIdMap controls;\n> > > > diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in\n> > > > new file mode 100644\n> > > > index 000000000000..c9bfaf119578\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/internal/control_ids.h.in\n> > > > @@ -0,0 +1,24 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Google Inc.\n> > > > + *\n> > > > + * control_ids.h - Internal Control ID list\n> > > > + *\n> > > > + * This file is auto-generated. Do not edit.\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace controls {\n> > > > +\n> > > > +enum {\n> > > > +${ids}\n> > > > +};\n> > > > +\n> > > > +} /* namespace controls */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build\n> > > > index 7f1f344014c4..dc7fa83ed978 100644\n> > > > --- a/include/libcamera/internal/meson.build\n> > > > +++ b/include/libcamera/internal/meson.build\n> > > > @@ -47,4 +47,24 @@ libcamera_internal_headers = files([\n> > > > 'yaml_parser.h',\n> > > > ])\n> > > >\n> > > > +# Internal control and property ID mappings\n> > > > +internal_control_source_files = [\n> > > > + 'control_ids',\n> > > > + 'property_ids',\n> > > > +]\n> > > > +\n> > > > +internal_control_headers = []\n> > > > +\n> > > > +foreach header : internal_control_source_files\n> > > > + input_files = files('../../../src/libcamera/' + header + '.yaml',\n> > > > + header + '.h.in')\n> > > > + internal_control_headers += custom_target(header + '_h',\n> > > > + input : input_files,\n> > > > + output : header + '.h',\n> > > > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],\n> > > > + install : false)\n> > > > +endforeach\n> > > > +\n> > > > +libcamera_internal_headers += internal_control_headers\n> > > > +\n> > > > subdir('converter')\n> > > > diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in\n> > > > new file mode 100644\n> > > > index 000000000000..15f4950953b4\n> > > > --- /dev/null\n> > > > +++ b/include/libcamera/internal/property_ids.h.in\n> > > > @@ -0,0 +1,24 @@\n> > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */\n> > > > +/*\n> > > > + * Copyright (C) 2019, Google Inc.\n> > > > + *\n> > > > + * property_ids.h - Internal Property ID list\n> > > > + *\n> > > > + * This file is auto-generated. Do not edit.\n> > > > + */\n> > > > +\n> > > > +#pragma once\n> > > > +\n> > > > +#include \n> > > > +\n> > > > +namespace libcamera {\n> > > > +\n> > > > +namespace properties {\n> > > > +\n> > > > +enum {\n> > > > +${ids}\n> > > > +};\n> > > > +\n> > > > +} /* namespace properties */\n> > > > +\n> > > > +} /* namespace libcamera */\n> > > \n> > > Too bad we have users of the enumerated values, otherwise this could\n> > > be hidden in the generated .cpp file\n> > > \n> > > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in\n> > > > index ff0194083af0..0fbdcc0c8504 100644\n> > > > --- a/include/libcamera/property_ids.h.in\n> > > > +++ b/include/libcamera/property_ids.h.in\n> > > > @@ -17,10 +17,6 @@ namespace libcamera {\n> > > >\n> > > > namespace properties {\n> > > >\n> > > > -enum {\n> > > > -${ids}\n> > > > -};\n> > > > -\n> > > > ${controls}\n> > > >\n> > > > namespace draft {\n> > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp\n> > > > index a47ae3a9e5cb..8e083396cb01 100644\n> > > > --- a/src/ipa/rpi/common/ipa_base.cpp\n> > > > +++ b/src/ipa/rpi/common/ipa_base.cpp\n> > > > @@ -11,9 +11,12 @@\n> > > >\n> > > > #include \n> > > > #include \n> > > > +\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/control_ids.h\"\n> > > > +\n> > > > #include \"controller/af_algorithm.h\"\n> > > > #include \"controller/af_status.h\"\n> > > > #include \"controller/agc_algorithm.h\"\n> > > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in\n> > > > index 5fb1c2c30558..20ef147ab826 100644\n> > > > --- a/src/libcamera/control_ids.cpp.in\n> > > > +++ b/src/libcamera/control_ids.cpp.in\n> > > > @@ -10,8 +10,10 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \"libcamera/internal/control_ids.h\"\n> > > > +\n> > > > /**\n> > > > - * \\file control_ids.h\n> > > > + * \\file libcamera/control_ids.h\n> > > > * \\brief Camera control identifiers\n> > > > */\n> > > >\n> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build\n> > > > index b24f82965764..e7f5edb2f39b 100644\n> > > > --- a/src/libcamera/meson.build\n> > > > +++ b/src/libcamera/meson.build\n> > > > @@ -131,6 +131,7 @@ foreach source : control_source_files\n> > > > control_sources += custom_target(source + '_cpp',\n> > > > input : input_files,\n> > > > output : source + '.cpp',\n> > > > + depends : internal_control_headers,\n> > > > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])\n> > > > endforeach\n> > > >\n> > > > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in\n> > > > index f917e3349766..c7dbf9838411 100644\n> > > > --- a/src/libcamera/property_ids.cpp.in\n> > > > +++ b/src/libcamera/property_ids.cpp.in\n> > > > @@ -7,10 +7,11 @@\n> > > > * This file is auto-generated. Do not edit.\n> > > > */\n> > > >\n> > > > +#include \"libcamera/internal/property_ids.h\"\n> > > > #include \n> > > \n> > > Do we want the public header to be included by the private ? (This for\n> > > both properties and controls)\n> > \n> > I would do so, yes. It makes sense and is simpler for the user.\n> > \n> > > Minor questions apart, as long as we don't want applications to use\n> > > the enumared ids (and I don't think we want and I can't think of use\n> > > cases why we should) I think this is a good change!\n> > \n> > Shining a bit of light on future plans, we'll have a C API, and\n> > numerical IDs will be part of the ABI. They will obviously need to be\n> > stable, adding new controls and deprecating existing controls shall not\n> > cause the ID of any other control to change. For this reason, I would\n> > prefer keeping the ID part of the public API and ABI.\n> \n> Well this is the RFC part.\n\n:-)\n\n> We don't yet have a C API and this table gives us ABI instability on\n> every control update.\n> \n> Or we could instead can only append controls in the YAML file instead of\n> keeping them in any grouping or sort order and keep the numbers stable\n> with a commit to do any sorting and cleanup done kept to apply before\n> a major release update.\n\nI think we need to keep the numbering stable. Only appending to the YAML\nfile is one way, and I would go as far as not sorting even across major\nreleases. We could try to sort when generating HTML documentation to\nmake it nicer to search.\n\nThere are other ways, such as specifying the number manually in the YAML\nfile. That's nicer as it allows sorting entries, but worse as it's more\nerror-prone. Maybe there's a clever option that would give us the best\nof both worlds ?\n\n> > > > /**\n> > > > - * \\file property_ids.h\n> > > > + * \\file libcamera/property_ids.h\n> > > > * \\brief Camera property identifiers\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 BD0F3C0F1B\n\tfor ;\n\tWed, 6 Sep 2023 13:52:58 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B4780628EC;\n\tWed, 6 Sep 2023 15:52:57 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7850161DF7\n\tfor ;\n\tWed, 6 Sep 2023 15:52:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(ftip006315900.acc1.colindale.21cn-nte.bt.net [81.134.214.249])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 19DC4DA8;\n\tWed, 6 Sep 2023 15:51:29 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1694008377;\n\tbh=xRmQdPNxBnB2BeosRYMO/ya5/fn5UMOGJ9bVXMOtrv8=;\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=32PXPKbYI7tDCeaRqYCeL1Qyei2HS7D6Ag4eBxBz9tKqxY3Wq0cYZhTnPzYXrTeOw\n\t/nHQ8Y9mehnpIHevMGpji1LtIh7N5cOiFdDVFKDLr2jxi1+mkPMqiqGJmZgP3tcMaG\n\ttKEVpF27Iu6EyGobbAkyWMvGoH/rFFojFqJr5z9SwxXSDXRSsDdkv+DOQ4aoB3pjWs\n\tnTEkWTiva20wW+7qvuqxPH849JxmMpbSWWGcrmd7MlTMZmsXc4DuKeaBb4aJqM685U\n\t/Ebhxy1KCW/g1zVGvTYOCTEe45XzobQ7DCq8LzIj9BMylCzVR6j8tXnF8K6/o0fbv4\n\tglHqpZD5NzuVQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1694008289;\n\tbh=xRmQdPNxBnB2BeosRYMO/ya5/fn5UMOGJ9bVXMOtrv8=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DJRfWVejXSYuEVzTETBDh+v5HYfr5RJci3Xqi6olyFYeb+98X+oKmGtZl4R7Oxip7\n\t+Mm8kqiEhhKc5h2eAiCBbeqmXeyLNqPEJCwLfp6kLQmcKpAskUFzYOJU4vw85wYsHf\n\twog/tiqI+FMKn64Dy3rP8BtA2S7JmBLERcxxjcFQ="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DJRfWVej\"; dkim-atps=neutral","Date":"Wed, 6 Sep 2023 16:53:07 +0300","To":"Kieran Bingham ","Message-ID":"<20230906135307.GB25420@pendragon.ideasonboard.com>","References":"<20230904215954.3417076-1-kieran.bingham@ideasonboard.com>\n\t\n\t<20230905111350.GE11700@pendragon.ideasonboard.com>\n\t<169398244003.137962.8095588109236455387@ping.linuxembedded.co.uk>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<169398244003.137962.8095588109236455387@ping.linuxembedded.co.uk>","Subject":"Re: [libcamera-devel] [RFC PATCH] libcamera: Privatise control enums","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":"Jacopo Mondi ,\n\tlibcamera devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]