| Message ID | 20230904215954.3417076-1-kieran.bingham@ideasonboard.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran On Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote: > The Control framework generates an enumeration for each control and > property entry which is used to support the ControlIdMap for each. > > This enum listing provides hardcoded integer references for each control > and is a source of potential ABI breakage when introducing new controls. > > The enum values are expected to only be used internally, so reduce > public ABI exposure when modifying controls by moving the enum to new > internal header definitions. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > While trying to work towards a new 'patch' release, I hit the following > abi-compatbility issues: > > Binary compatibility: 99.4% > Source compatibility: 99.7% > Total binary compatibility problems: 2, warnings: 0 > Total source compatibility problems: 2, warnings: 0 > Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html > > This stems from the removal of the following symbols: > > Removed Symbols 2 > control_ids.h, libcamera.so.0.1.0 > namespace libcamera::controls::draft > SceneFlicker [data] > SceneFlickerValues [data] > > And also the introduction of four new controls: > AeFlickerDetected [data] > AeFlickerMode [data] > AeFlickerModeValues [data] > AeFlickerPeriod [data] > > All of these changes affect the enum values generated to produce the > ControlIdMap. > > This proposal aims to remove this from the Public ABI/API to prevent > potential issues on every modification to the control list. > > Of course - removing a draft control or renaming the controls is still > going to cause linkage problems with any application which may use those > symbols. > > In this specific case, I suspect we would be lucky as SceneFlicker and > SceneFlickerValues are likely to be unused as they were otherwise > unimplemented. > > include/libcamera/control_ids.h.in | 4 ---- > include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++ > include/libcamera/internal/meson.build | 20 ++++++++++++++++ > include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++ > include/libcamera/property_ids.h.in | 4 ---- > src/ipa/rpi/common/ipa_base.cpp | 3 +++ > src/libcamera/control_ids.cpp.in | 4 +++- > src/libcamera/meson.build | 1 + > src/libcamera/property_ids.cpp.in | 3 ++- > 9 files changed, 77 insertions(+), 10 deletions(-) > create mode 100644 include/libcamera/internal/control_ids.h.in > create mode 100644 include/libcamera/internal/property_ids.h.in > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > index 0718a8886f6c..a0c66192fdc9 100644 > --- a/include/libcamera/control_ids.h.in > +++ b/include/libcamera/control_ids.h.in > @@ -18,10 +18,6 @@ namespace libcamera { > > namespace controls { > > -enum { > -${ids} > -}; > - > ${controls} > > extern const ControlIdMap controls; > diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in > new file mode 100644 > index 000000000000..c9bfaf119578 > --- /dev/null > +++ b/include/libcamera/internal/control_ids.h.in > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * control_ids.h - Internal Control ID list > + * > + * This file is auto-generated. Do not edit. > + */ > + > +#pragma once > + > +#include <libcamera/controls.h> > + > +namespace libcamera { > + > +namespace controls { > + > +enum { > +${ids} > +}; > + > +} /* namespace controls */ > + > +} /* namespace libcamera */ > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > index 7f1f344014c4..dc7fa83ed978 100644 > --- a/include/libcamera/internal/meson.build > +++ b/include/libcamera/internal/meson.build > @@ -47,4 +47,24 @@ libcamera_internal_headers = files([ > 'yaml_parser.h', > ]) > > +# Internal control and property ID mappings > +internal_control_source_files = [ > + 'control_ids', > + 'property_ids', > +] > + > +internal_control_headers = [] > + > +foreach header : internal_control_source_files > + input_files = files('../../../src/libcamera/' + header + '.yaml', > + header + '.h.in') > + internal_control_headers += custom_target(header + '_h', > + input : input_files, > + output : header + '.h', > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'], > + install : false) > +endforeach > + > +libcamera_internal_headers += internal_control_headers > + > subdir('converter') > diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in > new file mode 100644 > index 000000000000..15f4950953b4 > --- /dev/null > +++ b/include/libcamera/internal/property_ids.h.in > @@ -0,0 +1,24 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2019, Google Inc. > + * > + * property_ids.h - Internal Property ID list > + * > + * This file is auto-generated. Do not edit. > + */ > + > +#pragma once > + > +#include <libcamera/controls.h> > + > +namespace libcamera { > + > +namespace properties { > + > +enum { > +${ids} > +}; > + > +} /* namespace properties */ > + > +} /* namespace libcamera */ Too bad we have users of the enumerated values, otherwise this could be hidden in the generated .cpp file > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > index ff0194083af0..0fbdcc0c8504 100644 > --- a/include/libcamera/property_ids.h.in > +++ b/include/libcamera/property_ids.h.in > @@ -17,10 +17,6 @@ namespace libcamera { > > namespace properties { > > -enum { > -${ids} > -}; > - > ${controls} > > namespace draft { > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > index a47ae3a9e5cb..8e083396cb01 100644 > --- a/src/ipa/rpi/common/ipa_base.cpp > +++ b/src/ipa/rpi/common/ipa_base.cpp > @@ -11,9 +11,12 @@ > > #include <libcamera/base/log.h> > #include <libcamera/base/span.h> > + > #include <libcamera/control_ids.h> > #include <libcamera/property_ids.h> > > +#include "libcamera/internal/control_ids.h" > + > #include "controller/af_algorithm.h" > #include "controller/af_status.h" > #include "controller/agc_algorithm.h" > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > index 5fb1c2c30558..20ef147ab826 100644 > --- a/src/libcamera/control_ids.cpp.in > +++ b/src/libcamera/control_ids.cpp.in > @@ -10,8 +10,10 @@ > #include <libcamera/control_ids.h> > #include <libcamera/controls.h> > > +#include "libcamera/internal/control_ids.h" > + > /** > - * \file control_ids.h > + * \file libcamera/control_ids.h > * \brief Camera control identifiers > */ > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > index b24f82965764..e7f5edb2f39b 100644 > --- a/src/libcamera/meson.build > +++ b/src/libcamera/meson.build > @@ -131,6 +131,7 @@ foreach source : control_source_files > control_sources += custom_target(source + '_cpp', > input : input_files, > output : source + '.cpp', > + depends : internal_control_headers, > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@']) > endforeach > > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in > index f917e3349766..c7dbf9838411 100644 > --- a/src/libcamera/property_ids.cpp.in > +++ b/src/libcamera/property_ids.cpp.in > @@ -7,10 +7,11 @@ > * This file is auto-generated. Do not edit. > */ > > +#include "libcamera/internal/property_ids.h" > #include <libcamera/property_ids.h> Do we want the public header to be included by the private ? (This for both properties and controls) Minor questions apart, as long as we don't want applications to use the enumared ids (and I don't think we want and I can't think of use cases why we should) I think this is a good change! Thanks j > > /** > - * \file property_ids.h > + * \file libcamera/property_ids.h > * \brief Camera property identifiers > */ > > -- > 2.34.1 >
On Tue, Sep 05, 2023 at 09:42:02AM +0200, Jacopo Mondi via libcamera-devel wrote: > On Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote: > > The Control framework generates an enumeration for each control and > > property entry which is used to support the ControlIdMap for each. > > > > This enum listing provides hardcoded integer references for each control > > and is a source of potential ABI breakage when introducing new controls. > > > > The enum values are expected to only be used internally, so reduce > > public ABI exposure when modifying controls by moving the enum to new > > internal header definitions. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > While trying to work towards a new 'patch' release, I hit the following > > abi-compatbility issues: > > > > Binary compatibility: 99.4% > > Source compatibility: 99.7% > > Total binary compatibility problems: 2, warnings: 0 > > Total source compatibility problems: 2, warnings: 0 > > Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html > > > > This stems from the removal of the following symbols: > > > > Removed Symbols 2 > > control_ids.h, libcamera.so.0.1.0 > > namespace libcamera::controls::draft > > SceneFlicker [data] > > SceneFlickerValues [data] > > > > And also the introduction of four new controls: > > AeFlickerDetected [data] > > AeFlickerMode [data] > > AeFlickerModeValues [data] > > AeFlickerPeriod [data] > > > > All of these changes affect the enum values generated to produce the > > ControlIdMap. > > > > This proposal aims to remove this from the Public ABI/API to prevent > > potential issues on every modification to the control list. > > > > Of course - removing a draft control or renaming the controls is still > > going to cause linkage problems with any application which may use those > > symbols. > > > > In this specific case, I suspect we would be lucky as SceneFlicker and > > SceneFlickerValues are likely to be unused as they were otherwise > > unimplemented. > > > > include/libcamera/control_ids.h.in | 4 ---- > > include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++ > > include/libcamera/internal/meson.build | 20 ++++++++++++++++ > > include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++ > > include/libcamera/property_ids.h.in | 4 ---- > > src/ipa/rpi/common/ipa_base.cpp | 3 +++ > > src/libcamera/control_ids.cpp.in | 4 +++- > > src/libcamera/meson.build | 1 + > > src/libcamera/property_ids.cpp.in | 3 ++- > > 9 files changed, 77 insertions(+), 10 deletions(-) > > create mode 100644 include/libcamera/internal/control_ids.h.in > > create mode 100644 include/libcamera/internal/property_ids.h.in > > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > > index 0718a8886f6c..a0c66192fdc9 100644 > > --- a/include/libcamera/control_ids.h.in > > +++ b/include/libcamera/control_ids.h.in > > @@ -18,10 +18,6 @@ namespace libcamera { > > > > namespace controls { > > > > -enum { > > -${ids} > > -}; > > - > > ${controls} > > > > extern const ControlIdMap controls; > > diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in > > new file mode 100644 > > index 000000000000..c9bfaf119578 > > --- /dev/null > > +++ b/include/libcamera/internal/control_ids.h.in > > @@ -0,0 +1,24 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * control_ids.h - Internal Control ID list > > + * > > + * This file is auto-generated. Do not edit. > > + */ > > + > > +#pragma once > > + > > +#include <libcamera/controls.h> > > + > > +namespace libcamera { > > + > > +namespace controls { > > + > > +enum { > > +${ids} > > +}; > > + > > +} /* namespace controls */ > > + > > +} /* namespace libcamera */ > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > > index 7f1f344014c4..dc7fa83ed978 100644 > > --- a/include/libcamera/internal/meson.build > > +++ b/include/libcamera/internal/meson.build > > @@ -47,4 +47,24 @@ libcamera_internal_headers = files([ > > 'yaml_parser.h', > > ]) > > > > +# Internal control and property ID mappings > > +internal_control_source_files = [ > > + 'control_ids', > > + 'property_ids', > > +] > > + > > +internal_control_headers = [] > > + > > +foreach header : internal_control_source_files > > + input_files = files('../../../src/libcamera/' + header + '.yaml', > > + header + '.h.in') > > + internal_control_headers += custom_target(header + '_h', > > + input : input_files, > > + output : header + '.h', > > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'], > > + install : false) > > +endforeach > > + > > +libcamera_internal_headers += internal_control_headers > > + > > subdir('converter') > > diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in > > new file mode 100644 > > index 000000000000..15f4950953b4 > > --- /dev/null > > +++ b/include/libcamera/internal/property_ids.h.in > > @@ -0,0 +1,24 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2019, Google Inc. > > + * > > + * property_ids.h - Internal Property ID list > > + * > > + * This file is auto-generated. Do not edit. > > + */ > > + > > +#pragma once > > + > > +#include <libcamera/controls.h> > > + > > +namespace libcamera { > > + > > +namespace properties { > > + > > +enum { > > +${ids} > > +}; > > + > > +} /* namespace properties */ > > + > > +} /* namespace libcamera */ > > Too bad we have users of the enumerated values, otherwise this could > be hidden in the generated .cpp file > > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > > index ff0194083af0..0fbdcc0c8504 100644 > > --- a/include/libcamera/property_ids.h.in > > +++ b/include/libcamera/property_ids.h.in > > @@ -17,10 +17,6 @@ namespace libcamera { > > > > namespace properties { > > > > -enum { > > -${ids} > > -}; > > - > > ${controls} > > > > namespace draft { > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > index a47ae3a9e5cb..8e083396cb01 100644 > > --- a/src/ipa/rpi/common/ipa_base.cpp > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > @@ -11,9 +11,12 @@ > > > > #include <libcamera/base/log.h> > > #include <libcamera/base/span.h> > > + > > #include <libcamera/control_ids.h> > > #include <libcamera/property_ids.h> > > > > +#include "libcamera/internal/control_ids.h" > > + > > #include "controller/af_algorithm.h" > > #include "controller/af_status.h" > > #include "controller/agc_algorithm.h" > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > > index 5fb1c2c30558..20ef147ab826 100644 > > --- a/src/libcamera/control_ids.cpp.in > > +++ b/src/libcamera/control_ids.cpp.in > > @@ -10,8 +10,10 @@ > > #include <libcamera/control_ids.h> > > #include <libcamera/controls.h> > > > > +#include "libcamera/internal/control_ids.h" > > + > > /** > > - * \file control_ids.h > > + * \file libcamera/control_ids.h > > * \brief Camera control identifiers > > */ > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > index b24f82965764..e7f5edb2f39b 100644 > > --- a/src/libcamera/meson.build > > +++ b/src/libcamera/meson.build > > @@ -131,6 +131,7 @@ foreach source : control_source_files > > control_sources += custom_target(source + '_cpp', > > input : input_files, > > output : source + '.cpp', > > + depends : internal_control_headers, > > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@']) > > endforeach > > > > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in > > index f917e3349766..c7dbf9838411 100644 > > --- a/src/libcamera/property_ids.cpp.in > > +++ b/src/libcamera/property_ids.cpp.in > > @@ -7,10 +7,11 @@ > > * This file is auto-generated. Do not edit. > > */ > > > > +#include "libcamera/internal/property_ids.h" > > #include <libcamera/property_ids.h> > > Do we want the public header to be included by the private ? (This for > both properties and controls) I would do so, yes. It makes sense and is simpler for the user. > Minor questions apart, as long as we don't want applications to use > the enumared ids (and I don't think we want and I can't think of use > cases why we should) I think this is a good change! Shining a bit of light on future plans, we'll have a C API, and numerical IDs will be part of the ABI. They will obviously need to be stable, adding new controls and deprecating existing controls shall not cause the ID of any other control to change. For this reason, I would prefer keeping the ID part of the public API and ABI. > > /** > > - * \file property_ids.h > > + * \file libcamera/property_ids.h > > * \brief Camera property identifiers > > */ > >
Hi Kieran, On Wed, Sep 06, 2023 at 07:40:40AM +0100, Kieran Bingham wrote: > Quoting Laurent Pinchart (2023-09-05 12:13:50) > > On Tue, Sep 05, 2023 at 09:42:02AM +0200, Jacopo Mondi via libcamera-devel wrote: > > > On Mon, Sep 04, 2023 at 10:59:54PM +0100, Kieran Bingham via libcamera-devel wrote: > > > > The Control framework generates an enumeration for each control and > > > > property entry which is used to support the ControlIdMap for each. > > > > > > > > This enum listing provides hardcoded integer references for each control > > > > and is a source of potential ABI breakage when introducing new controls. > > > > > > > > The enum values are expected to only be used internally, so reduce > > > > public ABI exposure when modifying controls by moving the enum to new > > > > internal header definitions. > > > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > > While trying to work towards a new 'patch' release, I hit the following > > > > abi-compatbility issues: > > > > > > > > Binary compatibility: 99.4% > > > > Source compatibility: 99.7% > > > > Total binary compatibility problems: 2, warnings: 0 > > > > Total source compatibility problems: 2, warnings: 0 > > > > Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html > > > > > > > > This stems from the removal of the following symbols: > > > > > > > > Removed Symbols 2 > > > > control_ids.h, libcamera.so.0.1.0 > > > > namespace libcamera::controls::draft > > > > SceneFlicker [data] > > > > SceneFlickerValues [data] > > > > > > > > And also the introduction of four new controls: > > > > AeFlickerDetected [data] > > > > AeFlickerMode [data] > > > > AeFlickerModeValues [data] > > > > AeFlickerPeriod [data] > > > > > > > > All of these changes affect the enum values generated to produce the > > > > ControlIdMap. > > > > > > > > This proposal aims to remove this from the Public ABI/API to prevent > > > > potential issues on every modification to the control list. > > > > > > > > Of course - removing a draft control or renaming the controls is still > > > > going to cause linkage problems with any application which may use those > > > > symbols. > > > > > > > > In this specific case, I suspect we would be lucky as SceneFlicker and > > > > SceneFlickerValues are likely to be unused as they were otherwise > > > > unimplemented. > > > > > > > > include/libcamera/control_ids.h.in | 4 ---- > > > > include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++ > > > > include/libcamera/internal/meson.build | 20 ++++++++++++++++ > > > > include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++ > > > > include/libcamera/property_ids.h.in | 4 ---- > > > > src/ipa/rpi/common/ipa_base.cpp | 3 +++ > > > > src/libcamera/control_ids.cpp.in | 4 +++- > > > > src/libcamera/meson.build | 1 + > > > > src/libcamera/property_ids.cpp.in | 3 ++- > > > > 9 files changed, 77 insertions(+), 10 deletions(-) > > > > create mode 100644 include/libcamera/internal/control_ids.h.in > > > > create mode 100644 include/libcamera/internal/property_ids.h.in > > > > > > > > diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in > > > > index 0718a8886f6c..a0c66192fdc9 100644 > > > > --- a/include/libcamera/control_ids.h.in > > > > +++ b/include/libcamera/control_ids.h.in > > > > @@ -18,10 +18,6 @@ namespace libcamera { > > > > > > > > namespace controls { > > > > > > > > -enum { > > > > -${ids} > > > > -}; > > > > - > > > > ${controls} > > > > > > > > extern const ControlIdMap controls; > > > > diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in > > > > new file mode 100644 > > > > index 000000000000..c9bfaf119578 > > > > --- /dev/null > > > > +++ b/include/libcamera/internal/control_ids.h.in > > > > @@ -0,0 +1,24 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2019, Google Inc. > > > > + * > > > > + * control_ids.h - Internal Control ID list > > > > + * > > > > + * This file is auto-generated. Do not edit. > > > > + */ > > > > + > > > > +#pragma once > > > > + > > > > +#include <libcamera/controls.h> > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace controls { > > > > + > > > > +enum { > > > > +${ids} > > > > +}; > > > > + > > > > +} /* namespace controls */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build > > > > index 7f1f344014c4..dc7fa83ed978 100644 > > > > --- a/include/libcamera/internal/meson.build > > > > +++ b/include/libcamera/internal/meson.build > > > > @@ -47,4 +47,24 @@ libcamera_internal_headers = files([ > > > > 'yaml_parser.h', > > > > ]) > > > > > > > > +# Internal control and property ID mappings > > > > +internal_control_source_files = [ > > > > + 'control_ids', > > > > + 'property_ids', > > > > +] > > > > + > > > > +internal_control_headers = [] > > > > + > > > > +foreach header : internal_control_source_files > > > > + input_files = files('../../../src/libcamera/' + header + '.yaml', > > > > + header + '.h.in') > > > > + internal_control_headers += custom_target(header + '_h', > > > > + input : input_files, > > > > + output : header + '.h', > > > > + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'], > > > > + install : false) > > > > +endforeach > > > > + > > > > +libcamera_internal_headers += internal_control_headers > > > > + > > > > subdir('converter') > > > > diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in > > > > new file mode 100644 > > > > index 000000000000..15f4950953b4 > > > > --- /dev/null > > > > +++ b/include/libcamera/internal/property_ids.h.in > > > > @@ -0,0 +1,24 @@ > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > +/* > > > > + * Copyright (C) 2019, Google Inc. > > > > + * > > > > + * property_ids.h - Internal Property ID list > > > > + * > > > > + * This file is auto-generated. Do not edit. > > > > + */ > > > > + > > > > +#pragma once > > > > + > > > > +#include <libcamera/controls.h> > > > > + > > > > +namespace libcamera { > > > > + > > > > +namespace properties { > > > > + > > > > +enum { > > > > +${ids} > > > > +}; > > > > + > > > > +} /* namespace properties */ > > > > + > > > > +} /* namespace libcamera */ > > > > > > Too bad we have users of the enumerated values, otherwise this could > > > be hidden in the generated .cpp file > > > > > > > diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in > > > > index ff0194083af0..0fbdcc0c8504 100644 > > > > --- a/include/libcamera/property_ids.h.in > > > > +++ b/include/libcamera/property_ids.h.in > > > > @@ -17,10 +17,6 @@ namespace libcamera { > > > > > > > > namespace properties { > > > > > > > > -enum { > > > > -${ids} > > > > -}; > > > > - > > > > ${controls} > > > > > > > > namespace draft { > > > > diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp > > > > index a47ae3a9e5cb..8e083396cb01 100644 > > > > --- a/src/ipa/rpi/common/ipa_base.cpp > > > > +++ b/src/ipa/rpi/common/ipa_base.cpp > > > > @@ -11,9 +11,12 @@ > > > > > > > > #include <libcamera/base/log.h> > > > > #include <libcamera/base/span.h> > > > > + > > > > #include <libcamera/control_ids.h> > > > > #include <libcamera/property_ids.h> > > > > > > > > +#include "libcamera/internal/control_ids.h" > > > > + > > > > #include "controller/af_algorithm.h" > > > > #include "controller/af_status.h" > > > > #include "controller/agc_algorithm.h" > > > > diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in > > > > index 5fb1c2c30558..20ef147ab826 100644 > > > > --- a/src/libcamera/control_ids.cpp.in > > > > +++ b/src/libcamera/control_ids.cpp.in > > > > @@ -10,8 +10,10 @@ > > > > #include <libcamera/control_ids.h> > > > > #include <libcamera/controls.h> > > > > > > > > +#include "libcamera/internal/control_ids.h" > > > > + > > > > /** > > > > - * \file control_ids.h > > > > + * \file libcamera/control_ids.h > > > > * \brief Camera control identifiers > > > > */ > > > > > > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build > > > > index b24f82965764..e7f5edb2f39b 100644 > > > > --- a/src/libcamera/meson.build > > > > +++ b/src/libcamera/meson.build > > > > @@ -131,6 +131,7 @@ foreach source : control_source_files > > > > control_sources += custom_target(source + '_cpp', > > > > input : input_files, > > > > output : source + '.cpp', > > > > + depends : internal_control_headers, > > > > command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@']) > > > > endforeach > > > > > > > > diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in > > > > index f917e3349766..c7dbf9838411 100644 > > > > --- a/src/libcamera/property_ids.cpp.in > > > > +++ b/src/libcamera/property_ids.cpp.in > > > > @@ -7,10 +7,11 @@ > > > > * This file is auto-generated. Do not edit. > > > > */ > > > > > > > > +#include "libcamera/internal/property_ids.h" > > > > #include <libcamera/property_ids.h> > > > > > > Do we want the public header to be included by the private ? (This for > > > both properties and controls) > > > > I would do so, yes. It makes sense and is simpler for the user. > > > > > Minor questions apart, as long as we don't want applications to use > > > the enumared ids (and I don't think we want and I can't think of use > > > cases why we should) I think this is a good change! > > > > Shining a bit of light on future plans, we'll have a C API, and > > numerical IDs will be part of the ABI. They will obviously need to be > > stable, adding new controls and deprecating existing controls shall not > > cause the ID of any other control to change. For this reason, I would > > prefer keeping the ID part of the public API and ABI. > > Well this is the RFC part. :-) > We don't yet have a C API and this table gives us ABI instability on > every control update. > > Or we could instead can only append controls in the YAML file instead of > keeping them in any grouping or sort order and keep the numbers stable > with a commit to do any sorting and cleanup done kept to apply before > a major release update. I think we need to keep the numbering stable. Only appending to the YAML file is one way, and I would go as far as not sorting even across major releases. We could try to sort when generating HTML documentation to make it nicer to search. There are other ways, such as specifying the number manually in the YAML file. That's nicer as it allows sorting entries, but worse as it's more error-prone. Maybe there's a clever option that would give us the best of both worlds ? > > > > /** > > > > - * \file property_ids.h > > > > + * \file libcamera/property_ids.h > > > > * \brief Camera property identifiers > > > > */ > > > >
diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in index 0718a8886f6c..a0c66192fdc9 100644 --- a/include/libcamera/control_ids.h.in +++ b/include/libcamera/control_ids.h.in @@ -18,10 +18,6 @@ namespace libcamera { namespace controls { -enum { -${ids} -}; - ${controls} extern const ControlIdMap controls; diff --git a/include/libcamera/internal/control_ids.h.in b/include/libcamera/internal/control_ids.h.in new file mode 100644 index 000000000000..c9bfaf119578 --- /dev/null +++ b/include/libcamera/internal/control_ids.h.in @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * control_ids.h - Internal Control ID list + * + * This file is auto-generated. Do not edit. + */ + +#pragma once + +#include <libcamera/controls.h> + +namespace libcamera { + +namespace controls { + +enum { +${ids} +}; + +} /* namespace controls */ + +} /* namespace libcamera */ diff --git a/include/libcamera/internal/meson.build b/include/libcamera/internal/meson.build index 7f1f344014c4..dc7fa83ed978 100644 --- a/include/libcamera/internal/meson.build +++ b/include/libcamera/internal/meson.build @@ -47,4 +47,24 @@ libcamera_internal_headers = files([ 'yaml_parser.h', ]) +# Internal control and property ID mappings +internal_control_source_files = [ + 'control_ids', + 'property_ids', +] + +internal_control_headers = [] + +foreach header : internal_control_source_files + input_files = files('../../../src/libcamera/' + header + '.yaml', + header + '.h.in') + internal_control_headers += custom_target(header + '_h', + input : input_files, + output : header + '.h', + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'], + install : false) +endforeach + +libcamera_internal_headers += internal_control_headers + subdir('converter') diff --git a/include/libcamera/internal/property_ids.h.in b/include/libcamera/internal/property_ids.h.in new file mode 100644 index 000000000000..15f4950953b4 --- /dev/null +++ b/include/libcamera/internal/property_ids.h.in @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2019, Google Inc. + * + * property_ids.h - Internal Property ID list + * + * This file is auto-generated. Do not edit. + */ + +#pragma once + +#include <libcamera/controls.h> + +namespace libcamera { + +namespace properties { + +enum { +${ids} +}; + +} /* namespace properties */ + +} /* namespace libcamera */ diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in index ff0194083af0..0fbdcc0c8504 100644 --- a/include/libcamera/property_ids.h.in +++ b/include/libcamera/property_ids.h.in @@ -17,10 +17,6 @@ namespace libcamera { namespace properties { -enum { -${ids} -}; - ${controls} namespace draft { diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp index a47ae3a9e5cb..8e083396cb01 100644 --- a/src/ipa/rpi/common/ipa_base.cpp +++ b/src/ipa/rpi/common/ipa_base.cpp @@ -11,9 +11,12 @@ #include <libcamera/base/log.h> #include <libcamera/base/span.h> + #include <libcamera/control_ids.h> #include <libcamera/property_ids.h> +#include "libcamera/internal/control_ids.h" + #include "controller/af_algorithm.h" #include "controller/af_status.h" #include "controller/agc_algorithm.h" diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in index 5fb1c2c30558..20ef147ab826 100644 --- a/src/libcamera/control_ids.cpp.in +++ b/src/libcamera/control_ids.cpp.in @@ -10,8 +10,10 @@ #include <libcamera/control_ids.h> #include <libcamera/controls.h> +#include "libcamera/internal/control_ids.h" + /** - * \file control_ids.h + * \file libcamera/control_ids.h * \brief Camera control identifiers */ diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build index b24f82965764..e7f5edb2f39b 100644 --- a/src/libcamera/meson.build +++ b/src/libcamera/meson.build @@ -131,6 +131,7 @@ foreach source : control_source_files control_sources += custom_target(source + '_cpp', input : input_files, output : source + '.cpp', + depends : internal_control_headers, command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@']) endforeach diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in index f917e3349766..c7dbf9838411 100644 --- a/src/libcamera/property_ids.cpp.in +++ b/src/libcamera/property_ids.cpp.in @@ -7,10 +7,11 @@ * This file is auto-generated. Do not edit. */ +#include "libcamera/internal/property_ids.h" #include <libcamera/property_ids.h> /** - * \file property_ids.h + * \file libcamera/property_ids.h * \brief Camera property identifiers */
The Control framework generates an enumeration for each control and property entry which is used to support the ControlIdMap for each. This enum listing provides hardcoded integer references for each control and is a source of potential ABI breakage when introducing new controls. The enum values are expected to only be used internally, so reduce public ABI exposure when modifying controls by moving the enum to new internal header definitions. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- While trying to work towards a new 'patch' release, I hit the following abi-compatbility issues: Binary compatibility: 99.4% Source compatibility: 99.7% Total binary compatibility problems: 2, warnings: 0 Total source compatibility problems: 2, warnings: 0 Report: compat_reports/libcamera/v0.1.0_to_v0.1.0-30-g59529ca6cb46/compat_report.html This stems from the removal of the following symbols: Removed Symbols 2 control_ids.h, libcamera.so.0.1.0 namespace libcamera::controls::draft SceneFlicker [data] SceneFlickerValues [data] And also the introduction of four new controls: AeFlickerDetected [data] AeFlickerMode [data] AeFlickerModeValues [data] AeFlickerPeriod [data] All of these changes affect the enum values generated to produce the ControlIdMap. This proposal aims to remove this from the Public ABI/API to prevent potential issues on every modification to the control list. Of course - removing a draft control or renaming the controls is still going to cause linkage problems with any application which may use those symbols. In this specific case, I suspect we would be lucky as SceneFlicker and SceneFlickerValues are likely to be unused as they were otherwise unimplemented. include/libcamera/control_ids.h.in | 4 ---- include/libcamera/internal/control_ids.h.in | 24 ++++++++++++++++++++ include/libcamera/internal/meson.build | 20 ++++++++++++++++ include/libcamera/internal/property_ids.h.in | 24 ++++++++++++++++++++ include/libcamera/property_ids.h.in | 4 ---- src/ipa/rpi/common/ipa_base.cpp | 3 +++ src/libcamera/control_ids.cpp.in | 4 +++- src/libcamera/meson.build | 1 + src/libcamera/property_ids.cpp.in | 3 ++- 9 files changed, 77 insertions(+), 10 deletions(-) create mode 100644 include/libcamera/internal/control_ids.h.in create mode 100644 include/libcamera/internal/property_ids.h.in