[libcamera-devel] [RFC PATCH] libcamera: Privatise control enums
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Tue Sep 5 09:42:02 CEST 2023
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 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)
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
>
More information about the libcamera-devel
mailing list