[libcamera-devel] [PATCH v1 1/5] controls: Add vendor control/property support to generation scripts
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Nov 20 04:17:40 CET 2023
Hi Naush,
Thank you for the patch.
On Fri, Nov 10, 2023 at 10:59:58AM +0000, Naushir Patuck via libcamera-devel wrote:
> Add support for vendor specific controls and properties to libcamera.
> The controls/properties are defined by a "vendor" tag in the YAML
> control description, for example:
>
> - RpiVendorControlExample:
If this was included in documentation I would avoid starting with
"RpiVendor" to avoid implying that vendor controls should be prefixed
with a vendor name, but for the commit message it's fine.
> type: string
> vendor: rpi
> description: |
> Test for libcamera vendor specific controls.
As indicated in a reply to the cover letter, I'm concerned that having
to tag each control with the vendor, coupled with the ability to declare
vendor controls in the main control_ids.yaml file, will make it a bit
messy and difficult to maintain. I would prefer enforcing separation of
vendor controls in per-vendor files right away. You can then add the
vendor property at the top level in the YAML file, instead of specifying
it per-control.
I'm also thinking that we should probably move the control and property
YAML files to a subdirectory, but that's something I can do on top.
> This will now generate a control id in the libcamera::controls::vendor::rpi
Unless I'm mistaken, that's libcamera::controls::rpi, there's no vendor
namespace.
> namespace, ensuring no id conflict between different vendors, core or
> draft libcamera controls. Similarly, a ControlIdMap control is generated
> in the libcamera::controls::rpi namespace.
>
> A #define LIBCAMERA_RPI_VENDOR_CONTROLS is also generated to allow
> applications to conditionally compile code if the specific vendor
> controls are present.
I wonder if this should go to a generated config.h at some point, and be
named LIBCAMERA_HAS_*. If anyone has an opinion on the topic, let's
discuss this already, and we can then update the implementation on top
of this series.
> For the python bindings, the control is available
> with libcamera.controls.rpi.RpiVendorControlExample. The above controls
> example applies similarly to properties.
>
> A new --mode flag is added to gen-controls.py to specify the mode of
> operation, either 'controls' or 'properties' to allow the code generator
> to correctly set the #define string.
>
> Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> ---
> include/libcamera/control_ids.h.in | 2 +
> include/libcamera/meson.build | 13 +-
> include/libcamera/property_ids.h.in | 2 +
> src/libcamera/control_ids.cpp.in | 6 +
> src/libcamera/meson.build | 5 +-
> src/libcamera/property_ids.cpp.in | 6 +
> src/py/libcamera/gen-py-controls.py | 16 +-
> src/py/libcamera/py_controls_generated.cpp.in | 3 +
> .../libcamera/py_properties_generated.cpp.in | 3 +
> utils/gen-controls.py | 144 ++++++++++++++----
> 10 files changed, 162 insertions(+), 38 deletions(-)
>
> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> index 0718a8886f6c..c97b09a82450 100644
> --- a/include/libcamera/control_ids.h.in
> +++ b/include/libcamera/control_ids.h.in
> @@ -32,6 +32,8 @@ ${draft_controls}
>
> } /* namespace draft */
>
> +${vendor_controls}
> +
Should we split vendor controls to separate files ? It would make it
possible for vendors to ship their control definitions separately from
libcamera, in case they have a closed-source IPA modules.
> } /* namespace controls */
>
> } /* namespace libcamera */
> diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> index a24c50d66a82..f736cca07228 100644
> --- a/include/libcamera/meson.build
> +++ b/include/libcamera/meson.build
> @@ -33,19 +33,20 @@ install_headers(libcamera_public_headers,
> libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir
>
> # control_ids.h and property_ids.h
> -control_source_files = [
> - 'control_ids',
> - 'property_ids',
> -]
> +control_source_files = {
> + 'control_ids': 'controls',
> + 'property_ids': 'properties',
> +}
>
> control_headers = []
>
> -foreach header : control_source_files
> +foreach header, mode : control_source_files
> input_files = files('../../src/libcamera/' + header +'.yaml', header + '.h.in')
> control_headers += custom_target(header + '_h',
> input : input_files,
> output : header + '.h',
> - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'],
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',
> + '--mode', mode],
> install : true,
> install_dir : libcamera_headers_install_dir)
> endforeach
> diff --git a/include/libcamera/property_ids.h.in b/include/libcamera/property_ids.h.in
> index ff0194083af0..47c5d6bf2e28 100644
> --- a/include/libcamera/property_ids.h.in
> +++ b/include/libcamera/property_ids.h.in
> @@ -31,6 +31,8 @@ ${draft_controls}
>
> extern const ControlIdMap properties;
>
> +${vendor_controls}
> +
> } /* namespace properties */
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/control_ids.cpp.in b/src/libcamera/control_ids.cpp.in
> index 5fb1c2c30558..d26eb930b30c 100644
> --- a/src/libcamera/control_ids.cpp.in
> +++ b/src/libcamera/control_ids.cpp.in
> @@ -33,6 +33,8 @@ ${draft_controls_doc}
>
> } /* namespace draft */
>
> +${vendor_controls_doc}
> +
> #ifndef __DOXYGEN__
> /*
> * Keep the controls definitions hidden from doxygen as it incorrectly parses
> @@ -45,6 +47,8 @@ namespace draft {
> ${draft_controls_def}
>
> } /* namespace draft */
> +
> +${vendor_controls_def}
> #endif
>
> /**
> @@ -57,6 +61,8 @@ extern const ControlIdMap controls {
> ${controls_map}
> };
>
> +${vendor_controls_map}
> +
> } /* namespace controls */
>
> } /* namespace libcamera */
> diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> index d0e26f6b4141..e49bf850b355 100644
> --- a/src/libcamera/meson.build
> +++ b/src/libcamera/meson.build
> @@ -127,12 +127,13 @@ endif
>
> control_sources = []
>
> -foreach source : control_source_files
> +foreach source, mode : control_source_files
> input_files = files(source +'.yaml', source + '.cpp.in')
> control_sources += custom_target(source + '_cpp',
> input : input_files,
> output : source + '.cpp',
> - command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@'])
> + command : [gen_controls, '-o', '@OUTPUT@', '@INPUT@',
> + '--mode', mode])
> endforeach
>
> libcamera_sources += control_sources
> diff --git a/src/libcamera/property_ids.cpp.in b/src/libcamera/property_ids.cpp.in
> index f917e3349766..ddbe714c3f00 100644
> --- a/src/libcamera/property_ids.cpp.in
> +++ b/src/libcamera/property_ids.cpp.in
> @@ -32,6 +32,8 @@ ${draft_controls_doc}
>
> } /* namespace draft */
>
> +${vendor_controls_doc}
> +
> #ifndef __DOXYGEN__
> /*
> * Keep the properties definitions hidden from doxygen as it incorrectly parses
> @@ -44,6 +46,8 @@ namespace draft {
> ${draft_controls_def}
>
> } /* namespace draft */
> +
> +${vendor_controls_def}
> #endif
>
> /**
> @@ -53,6 +57,8 @@ extern const ControlIdMap properties {
> ${controls_map}
> };
>
> +${vendor_controls_map}
> +
> } /* namespace properties */
>
> } /* namespace libcamera */
> diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py
> index 9948c41e42b1..e89de674966a 100755
> --- a/src/py/libcamera/gen-py-controls.py
> +++ b/src/py/libcamera/gen-py-controls.py
> @@ -24,12 +24,24 @@ def find_common_prefix(strings):
> def generate_py(controls, mode):
> out = ''
>
> + vendors_class_def = []
> + vendor_defs = []
> + vendors = []
> for ctrl in controls:
> name, ctrl = ctrl.popitem()
>
> + vendor = ctrl.get('vendor')
> + if vendor and vendor not in vendors:
> + vendors_class_def.append('class Py{}Controls\n{{\n}};\n'.format(vendor))
> + vendor_defs.append('\tauto {} = py::class_<Py{}Controls>(controls, \"{}\");'.format(vendor, vendor, vendor))
It looks like it may be time to switch to jinja templates. Not a
prerequisite for this series, although you may find it easier to rework
the generation script.
I won't review the details of the scripts and templates below just yet,
as they may be reworked based on the outcome of the discussions above.
> + vendors.append(vendor)
> +
> if ctrl.get('draft'):
> ns = 'libcamera::{}::draft::'.format(mode)
> container = 'draft'
> + elif vendor:
> + ns = 'libcamera::{}::{}::'.format(mode, vendor)
> + container = vendor
> else:
> ns = 'libcamera::{}::'.format(mode)
> container = 'controls'
> @@ -62,7 +74,9 @@ def generate_py(controls, mode):
>
> out += '\t;\n\n'
>
> - return {'controls': out}
> + return {'controls': out,
> + 'vendors_class_def': '\n'.join(vendors_class_def),
> + 'vendors_defs': '\n'.join(vendor_defs)}
>
>
> def fill_template(template, data):
> diff --git a/src/py/libcamera/py_controls_generated.cpp.in b/src/py/libcamera/py_controls_generated.cpp.in
> index 18fa57d948ea..ec4b55ef2011 100644
> --- a/src/py/libcamera/py_controls_generated.cpp.in
> +++ b/src/py/libcamera/py_controls_generated.cpp.in
> @@ -21,10 +21,13 @@ class PyDraftControls
> {
> };
>
> +${vendors_class_def}
> +
> void init_py_controls_generated(py::module& m)
> {
> auto controls = py::class_<PyControls>(m, "controls");
> auto draft = py::class_<PyDraftControls>(controls, "draft");
> +${vendors_defs}
>
> ${controls}
> }
> diff --git a/src/py/libcamera/py_properties_generated.cpp.in b/src/py/libcamera/py_properties_generated.cpp.in
> index e49b6e91bb83..87bc5e5d937e 100644
> --- a/src/py/libcamera/py_properties_generated.cpp.in
> +++ b/src/py/libcamera/py_properties_generated.cpp.in
> @@ -21,10 +21,13 @@ class PyDraftProperties
> {
> };
>
> +${vendors_class_def}
> +
> void init_py_properties_generated(py::module& m)
> {
> auto controls = py::class_<PyProperties>(m, "properties");
> auto draft = py::class_<PyDraftProperties>(controls, "draft");
> +${vendors_defs}
>
> ${controls}
> }
> diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> index 1075ae302ce1..dd55753e792c 100755
> --- a/utils/gen-controls.py
> +++ b/utils/gen-controls.py
> @@ -89,6 +89,11 @@ class Control(object):
> """Is the control a draft control"""
> return self.__data.get('draft') is not None
>
> + @property
> + def vendor(self):
> + """The vendor string, or None"""
> + return self.__data.get('vendor') if 'vendor' in self.__data else None
> +
> @property
> def name(self):
> """The control name (CamelCase)"""
> @@ -145,15 +150,22 @@ ${description}
> enum_values_start = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values = {''')
> enum_values_values = string.Template('''\tstatic_cast<int32_t>(${name}),''')
>
> - ctrls_doc = []
> - ctrls_def = []
> - draft_ctrls_doc = []
> - draft_ctrls_def = []
> - ctrls_map = []
> + ctrls_doc = {}
> + ctrls_def = {}
> + ctrls_map = {}
>
> for ctrl in controls:
> id_name = snake_case(ctrl.name).upper()
>
> + vendor = ctrl.vendor
> + if vendor is None:
> + vendor = 'draft' if ctrl.is_draft else 'libcamera'
> +
> + if vendor not in ctrls_doc:
> + ctrls_doc[vendor] = []
> + ctrls_def[vendor] = []
> + ctrls_map[vendor] = []
> +
> info = {
> 'name': ctrl.name,
> 'type': ctrl.type,
> @@ -161,11 +173,9 @@ ${description}
> 'id_name': id_name,
> }
>
> - target_doc = ctrls_doc
> - target_def = ctrls_def
> - if ctrl.is_draft:
> - target_doc = draft_ctrls_doc
> - target_def = draft_ctrls_def
> + target_doc = ctrls_doc[vendor]
> + target_def = ctrls_def[vendor]
> + target_ctrls_map = ctrls_map[vendor]
>
> if ctrl.is_enum:
> enum_doc = []
> @@ -201,41 +211,90 @@ ${description}
> target_doc.append(doc_template.substitute(info))
> target_def.append(def_template.substitute(info))
>
> - ctrls_map.append('\t{ ' + id_name + ', &' + ctrl.q_name + ' },')
> + target_ctrls_map.append('\t{ ' + id_name + ', &' + ctrl.q_name + ' },')
> +
> + vendor_ctrl_doc_sub = []
> + vendor_ctrl_template = string.Template('''
> +namespace ${vendor} {
> +
> +${vendor_controls_str}
> +
> +} /* namespace ${vendor} */''')
> +
> + for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> + vendor_ctrl_doc_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\n\n'.join(ctrls_doc[vendor])}))
> +
> + vendor_ctrl_def_sub = []
> + for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> + vendor_ctrl_def_sub.append(vendor_ctrl_template.substitute({'vendor': vendor, 'vendor_controls_str': '\n'.join(ctrls_def[vendor])}))
> +
> + vendor_ctrl_map_sub = []
> + vendor_ctrl_template = string.Template('''namespace ${vendor} {
> +/**
> + * \\brief List of all supported ${vendor} vendor controls
> + *
> + * Unless otherwise stated, all controls are bi-directional, i.e. they can be
> + * set through Request::controls() and returned out through Request::metadata().
> + */
> +extern const ControlIdMap controls {
> +${vendor_controls_map}
> +};
> +
> +} /* namespace ${vendor} */
> +''')
> +
> + for vendor in [v for v in ctrls_map.keys() if v not in ['libcamera', 'draft']]:
> + vendor_ctrl_map_sub.append(vendor_ctrl_template.substitute({'vendor': vendor,
> + 'vendor_controls_map': '\n'.join(ctrls_map[vendor])}))
>
> return {
> - 'controls_doc': '\n\n'.join(ctrls_doc),
> - 'controls_def': '\n'.join(ctrls_def),
> - 'draft_controls_doc': '\n\n'.join(draft_ctrls_doc),
> - 'draft_controls_def': '\n\n'.join(draft_ctrls_def),
> - 'controls_map': '\n'.join(ctrls_map),
> + 'controls_doc': '\n\n'.join(ctrls_doc['libcamera']),
> + 'controls_def': '\n'.join(ctrls_def['libcamera']),
> + 'draft_controls_doc': '\n\n'.join(ctrls_doc['draft']),
> + 'draft_controls_def': '\n\n'.join(ctrls_def['draft']),
> + 'controls_map': '\n'.join(ctrls_map['libcamera'] + ctrls_map['draft']),
> + 'vendor_controls_doc': '\n'.join(vendor_ctrl_doc_sub),
> + 'vendor_controls_def': '\n'.join(vendor_ctrl_def_sub),
> + 'vendor_controls_map': '\n'.join(vendor_ctrl_map_sub),
> }
>
>
> -def generate_h(controls):
> +def generate_h(controls, mode):
> enum_template_start = string.Template('''enum ${name}Enum {''')
> enum_value_template = string.Template('''\t${name} = ${value},''')
> enum_values_template = string.Template('''extern const std::array<const ControlValue, ${size}> ${name}Values;''')
> template = string.Template('''extern const Control<${type}> ${name};''')
>
> - ctrls = []
> - draft_ctrls = []
> - ids = []
> - id_value = 1
> + ctrls = {}
> + ids = {}
> + id_value = {}
>
> for ctrl in controls:
> id_name = snake_case(ctrl.name).upper()
>
> - ids.append('\t' + id_name + ' = ' + str(id_value) + ',')
> + vendor = ctrl.vendor
> + if vendor is None:
> + vendor = 'draft' if ctrl.is_draft else 'libcamera'
> +
> + if vendor not in ctrls:
> + ids[vendor] = []
> + id_value[vendor] = 1
> + ctrls[vendor] = []
> +
> + # Core and draft controls use the same ID value
> + target_ids = ids['libcamera'] if vendor in ['libcamera', 'draft'] else ids[vendor]
> + target_ids.append('\t' + id_name + ' = ' + str(id_value[vendor]) + ',')
>
> info = {
> 'name': ctrl.name,
> 'type': ctrl.type,
> }
>
> - target_ctrls = ctrls
> + target_ctrls = ctrls['libcamera']
> if ctrl.is_draft:
> - target_ctrls = draft_ctrls
> + target_ctrls = ctrls['draft']
> + elif vendor != 'libcamera':
> + target_ctrls = ctrls[vendor]
>
> if ctrl.is_enum:
> target_ctrls.append(enum_template_start.substitute(info))
> @@ -257,12 +316,37 @@ def generate_h(controls):
> target_ctrls.append(enum_values_template.substitute(values_info))
>
> target_ctrls.append(template.substitute(info))
> - id_value += 1
> + id_value[vendor] += 1
> +
> + vendor_template = string.Template('''
> +namespace ${vendor} {
> +
> +#define LIBCAMERA_${vendor_def}_VENDOR_${mode}
> +
> +enum {
> +${vendor_enums}
> +};
> +
> +extern const ControlIdMap controls;
> +
> +${vendor_controls}
> +
> +} /* namespace ${vendor} */
> +''')
> +
> + vendor_sub = []
> + for vendor in [v for v in ctrls.keys() if v not in ['libcamera', 'draft']]:
> + vendor_sub.append(vendor_template.substitute({'mode': mode.upper(),
> + 'vendor': vendor,
> + 'vendor_def': vendor.upper(),
> + 'vendor_enums': '\n'.join(ids[vendor]),
> + 'vendor_controls': '\n'.join(ctrls[vendor])}))
>
> return {
> - 'ids': '\n'.join(ids),
> - 'controls': '\n'.join(ctrls),
> - 'draft_controls': '\n'.join(draft_ctrls)
> + 'ids': '\n'.join(ids['libcamera']),
> + 'controls': '\n'.join(ctrls['libcamera']),
> + 'draft_controls': '\n'.join(ctrls['draft']),
> + 'vendor_controls': '\n'.join(vendor_sub)
> }
>
>
> @@ -284,6 +368,8 @@ def main(argv):
> help='Input file name.')
> parser.add_argument('template', type=str,
> help='Template file name.')
> + parser.add_argument('--mode', type=str, required=True, choices=['controls', 'properties'],
> + help='Mode of operation')
> args = parser.parse_args(argv[1:])
>
> data = open(args.input, 'rb').read()
> @@ -293,7 +379,7 @@ def main(argv):
> if args.template.endswith('.cpp.in'):
> data = generate_cpp(controls)
> elif args.template.endswith('.h.in'):
> - data = generate_h(controls)
> + data = generate_h(controls, args.mode)
> else:
> raise RuntimeError('Unknown template type')
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list