[libcamera-devel] [RFC PATCH] libcamera: Privatise control enums
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 5 13:13:50 CEST 2023
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 at 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
> > */
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list