| Message ID | 20231121145350.5956-2-naush@raspberrypi.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, Thank you for the patch. On Tue, Nov 21, 2023 at 02:53:45PM +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 file, for example: > > vendor: rpi > controls: > - MyExampleControl: > type: string > description: | > Test for libcamera vendor specific controls. > > This will now generate a control id in the libcamera::controls::rpi > 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. I think we'll have a problem here. There are many places in libcamera that use controls::controls today, and all of a sudden they won't have draft controls included there anymore. This includes the cam application or the Python bindings when they process metadata, the control serialization code, the Request constructor when it creates the ControlList for controls stored in the request, or various pipeline handlers and IPA modules when processing control lists. One option would be to keep a single ControlIdMap with all the controls, but that isn't going to work well long term. The better option would be to get rid of controls::controls, replacing it with Camera::controls() (conceptually, as they're not the same type, and Camera::controls() gets updated at runtime, leading to possible use-after-free). I don't want to block vendor controls completely until we get there, so I'm fine with an incremental approach, but I think we need to at least sketch the plan now to make sure the first step goes in the right direction. > A #define LIBCAMERA_HAS_RPI_VENDOR_CONTROLS is also generated to allow > applications to conditionally compile code if the specific vendor > controls are present. For the python bindings, the control is available > with libcamera.controls.rpi.MyExampleControl. The above controls > example applies similarly to properties. > > Existing libcamera controls defined in control_ids.yaml are given the > "libcamera" vendor tag. > > 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@raspberrypi.com> > --- > include/libcamera/control_ids.h.in | 2 + > include/libcamera/meson.build | 15 +- > include/libcamera/property_ids.h.in | 2 + > src/libcamera/control_ids.cpp.in | 6 + > src/libcamera/control_ids.yaml | 1 + > src/libcamera/meson.build | 5 +- > src/libcamera/property_ids.cpp.in | 6 + > src/libcamera/property_ids.yaml | 1 + > src/py/libcamera/gen-py-controls.py | 75 +++++---- > src/py/libcamera/py_controls_generated.cpp.in | 3 + > .../libcamera/py_properties_generated.cpp.in | 3 + > utils/gen-controls.py | 144 ++++++++++++++---- > 12 files changed, 194 insertions(+), 69 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} > + If we had to redo it, it could have been nicer to turn support for draft controls into vendor controls, instead of adding vendor controls and then using them to handle draft controls. I think that would have been easier to review. No need to rewrite the series, just a comment for the future. > } /* namespace controls */ > > } /* namespace libcamera */ > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > index a24c50d66a82..2c8c0258c95e 100644 > --- a/include/libcamera/meson.build > +++ b/include/libcamera/meson.build > @@ -32,20 +32,21 @@ 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_ids.h and property_ids.h and associated modes > +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/control_ids.yaml b/src/libcamera/control_ids.yaml > index 5827d7ecef49..ff74ce1deedb 100644 > --- a/src/libcamera/control_ids.yaml > +++ b/src/libcamera/control_ids.yaml > @@ -6,6 +6,7 @@ > --- > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > # set through Request::controls() and returned out through Request::metadata(). > +vendor: libcamera > controls: > - AeEnable: > type: bool > 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/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > index f35563842a5a..45f3609b4236 100644 > --- a/src/libcamera/property_ids.yaml > +++ b/src/libcamera/property_ids.yaml > @@ -4,6 +4,7 @@ > # > %YAML 1.1 > --- > +vendor: libcamera > controls: > - Location: > type: int32_t > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > index 9948c41e42b1..8f14cdafe313 100755 > --- a/src/py/libcamera/gen-py-controls.py > +++ b/src/py/libcamera/gen-py-controls.py > @@ -24,45 +24,59 @@ def find_common_prefix(strings): > def generate_py(controls, mode): > out = '' > > - for ctrl in controls: > - name, ctrl = ctrl.popitem() > - > - if ctrl.get('draft'): > - ns = 'libcamera::{}::draft::'.format(mode) > - container = 'draft' > - else: > - ns = 'libcamera::{}::'.format(mode) > - container = 'controls' > + vendors_class_def = [] > + vendor_defs = [] > + vendors = [] > + for vendor, ctrl_list in controls.items(): > + for ctrls in ctrl_list: > + name, ctrl = ctrls.popitem() > + > + if 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)) > + vendors.append(vendor) > + > + if ctrl.get('draft'): > + ns = 'libcamera::{}::draft::'.format(mode) > + container = 'draft' > + elif vendor != 'libcamera': > + ns = 'libcamera::{}::{}::'.format(mode, vendor) > + container = vendor > + else: > + ns = 'libcamera::{}::'.format(mode) > + container = 'controls' > > - out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' > + out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' > > - enum = ctrl.get('enum') > - if not enum: > - continue > + enum = ctrl.get('enum') > + if not enum: > + continue > > - cpp_enum = name + 'Enum' > + cpp_enum = name + 'Enum' > > - out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) > + out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) > > - if mode == 'controls': > - # Adjustments for controls > - if name == 'LensShadingMapMode': > - prefix = 'LensShadingMapMode' > + if mode == 'controls': > + # Adjustments for controls > + if name == 'LensShadingMapMode': > + prefix = 'LensShadingMapMode' > + else: > + prefix = find_common_prefix([e['name'] for e in enum]) > else: > + # Adjustments for properties > prefix = find_common_prefix([e['name'] for e in enum]) > - else: > - # Adjustments for properties > - prefix = find_common_prefix([e['name'] for e in enum]) > > - for entry in enum: > - cpp_enum = entry['name'] > - py_enum = entry['name'][len(prefix):] > + for entry in enum: > + cpp_enum = entry['name'] > + py_enum = entry['name'][len(prefix):] > > - out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) > + out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) > > - out += '\t;\n\n' > + out += '\t;\n\n' I think we've reached a major milestone: making Python code as unreadable as Perl :-) Not your fault, I'll try to find time to convert this to jinja templates. > > - 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): > @@ -90,7 +104,10 @@ def main(argv): > return -1 > > data = open(args.input, 'rb').read() > - controls = yaml.safe_load(data)['controls'] > + > + controls = {} > + vendor = yaml.safe_load(data)['vendor'] > + controls[vendor] = yaml.safe_load(data)['controls'] > > data = generate_py(controls, args.mode) > > 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..f7b5ec8c635d 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..8f2f8fdb02c3 100755 > --- a/utils/gen-controls.py > +++ b/utils/gen-controls.py > @@ -35,11 +35,12 @@ class ControlEnum(object): > > > class Control(object): > - def __init__(self, name, data): > + def __init__(self, name, data, vendor): > self.__name = name > self.__data = data > self.__enum_values = None > self.__size = None > + self.__vendor = vendor > > enum_values = data.get('enum') > if enum_values is not None: > @@ -89,6 +90,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.__vendor > + > @property > def name(self): > """The control name (CamelCase)""" > @@ -145,15 +151,19 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor > + if vendor not in ctrls_doc: > + ctrls_doc[vendor] = [] > + ctrls_def[vendor] = [] > + ctrls_map[vendor] = [] > + > info = { > 'name': ctrl.name, > 'type': ctrl.type, > @@ -161,11 +171,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 +209,87 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor > + 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 +311,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_HAS_${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,16 +363,19 @@ 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') Let's keep arguments sorted alphabetically, this should go before the '-o' option. I would also name if '--mode', '-m' to have a short argument, and change '-o' to '--output', '-o', for consistency. > args = parser.parse_args(argv[1:]) > > data = open(args.input, 'rb').read() > + vendor = yaml.safe_load(data)['vendor'] > controls = yaml.safe_load(data)['controls'] > - controls = [Control(*ctrl.popitem()) for ctrl in controls] > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > 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') >
Hi Laurent, Thank you for the feedback! On Thu, 23 Nov 2023 at 12:00, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > Thank you for the patch. > > On Tue, Nov 21, 2023 at 02:53:45PM +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 file, for example: > > > > vendor: rpi > > controls: > > - MyExampleControl: > > type: string > > description: | > > Test for libcamera vendor specific controls. > > > > This will now generate a control id in the libcamera::controls::rpi > > 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. > > I think we'll have a problem here. There are many places in libcamera > that use controls::controls today, and all of a sudden they won't have > draft controls included there anymore. This includes the cam application > or the Python bindings when they process metadata, the control > serialization code, the Request constructor when it creates the > ControlList for controls stored in the request, or various pipeline > handlers and IPA modules when processing control lists. Yes, this will indeed cause problems with some existing code. > > One option would be to keep a single ControlIdMap with all the controls, > but that isn't going to work well long term. The better option would be > to get rid of controls::controls, replacing it with Camera::controls() > (conceptually, as they're not the same type, and Camera::controls() gets > updated at runtime, leading to possible use-after-free). I don't want to > block vendor controls completely until we get there, so I'm fine with an > incremental approach, but I think we need to at least sketch the plan > now to make sure the first step goes in the right direction. Changing things around in this patch to keep a single ControlIdMap with all (libcamera/draft/vendor) controls is fairly trivial. I would suggest we start there to at least merge this functionality without any breakages and build on top of that. > > > A #define LIBCAMERA_HAS_RPI_VENDOR_CONTROLS is also generated to allow > > applications to conditionally compile code if the specific vendor > > controls are present. For the python bindings, the control is available > > with libcamera.controls.rpi.MyExampleControl. The above controls > > example applies similarly to properties. > > > > Existing libcamera controls defined in control_ids.yaml are given the > > "libcamera" vendor tag. > > > > 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@raspberrypi.com> > > --- > > include/libcamera/control_ids.h.in | 2 + > > include/libcamera/meson.build | 15 +- > > include/libcamera/property_ids.h.in | 2 + > > src/libcamera/control_ids.cpp.in | 6 + > > src/libcamera/control_ids.yaml | 1 + > > src/libcamera/meson.build | 5 +- > > src/libcamera/property_ids.cpp.in | 6 + > > src/libcamera/property_ids.yaml | 1 + > > src/py/libcamera/gen-py-controls.py | 75 +++++---- > > src/py/libcamera/py_controls_generated.cpp.in | 3 + > > .../libcamera/py_properties_generated.cpp.in | 3 + > > utils/gen-controls.py | 144 ++++++++++++++---- > > 12 files changed, 194 insertions(+), 69 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} > > + > > If we had to redo it, it could have been nicer to turn support for draft > controls into vendor controls, instead of adding vendor controls and > then using them to handle draft controls. I think that would have been > easier to review. No need to rewrite the series, just a comment for the > future. > > > } /* namespace controls */ > > > > } /* namespace libcamera */ > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build > > index a24c50d66a82..2c8c0258c95e 100644 > > --- a/include/libcamera/meson.build > > +++ b/include/libcamera/meson.build > > @@ -32,20 +32,21 @@ 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_ids.h and property_ids.h and associated modes > > +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/control_ids.yaml b/src/libcamera/control_ids.yaml > > index 5827d7ecef49..ff74ce1deedb 100644 > > --- a/src/libcamera/control_ids.yaml > > +++ b/src/libcamera/control_ids.yaml > > @@ -6,6 +6,7 @@ > > --- > > # Unless otherwise stated, all controls are bi-directional, i.e. they can be > > # set through Request::controls() and returned out through Request::metadata(). > > +vendor: libcamera > > controls: > > - AeEnable: > > type: bool > > 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/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml > > index f35563842a5a..45f3609b4236 100644 > > --- a/src/libcamera/property_ids.yaml > > +++ b/src/libcamera/property_ids.yaml > > @@ -4,6 +4,7 @@ > > # > > %YAML 1.1 > > --- > > +vendor: libcamera > > controls: > > - Location: > > type: int32_t > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py > > index 9948c41e42b1..8f14cdafe313 100755 > > --- a/src/py/libcamera/gen-py-controls.py > > +++ b/src/py/libcamera/gen-py-controls.py > > @@ -24,45 +24,59 @@ def find_common_prefix(strings): > > def generate_py(controls, mode): > > out = '' > > > > - for ctrl in controls: > > - name, ctrl = ctrl.popitem() > > - > > - if ctrl.get('draft'): > > - ns = 'libcamera::{}::draft::'.format(mode) > > - container = 'draft' > > - else: > > - ns = 'libcamera::{}::'.format(mode) > > - container = 'controls' > > + vendors_class_def = [] > > + vendor_defs = [] > > + vendors = [] > > + for vendor, ctrl_list in controls.items(): > > + for ctrls in ctrl_list: > > + name, ctrl = ctrls.popitem() > > + > > + if 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)) > > + vendors.append(vendor) > > + > > + if ctrl.get('draft'): > > + ns = 'libcamera::{}::draft::'.format(mode) > > + container = 'draft' > > + elif vendor != 'libcamera': > > + ns = 'libcamera::{}::{}::'.format(mode, vendor) > > + container = vendor > > + else: > > + ns = 'libcamera::{}::'.format(mode) > > + container = 'controls' > > > > - out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' > > + out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' > > > > - enum = ctrl.get('enum') > > - if not enum: > > - continue > > + enum = ctrl.get('enum') > > + if not enum: > > + continue > > > > - cpp_enum = name + 'Enum' > > + cpp_enum = name + 'Enum' > > > > - out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) > > + out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) > > > > - if mode == 'controls': > > - # Adjustments for controls > > - if name == 'LensShadingMapMode': > > - prefix = 'LensShadingMapMode' > > + if mode == 'controls': > > + # Adjustments for controls > > + if name == 'LensShadingMapMode': > > + prefix = 'LensShadingMapMode' > > + else: > > + prefix = find_common_prefix([e['name'] for e in enum]) > > else: > > + # Adjustments for properties > > prefix = find_common_prefix([e['name'] for e in enum]) > > - else: > > - # Adjustments for properties > > - prefix = find_common_prefix([e['name'] for e in enum]) > > > > - for entry in enum: > > - cpp_enum = entry['name'] > > - py_enum = entry['name'][len(prefix):] > > + for entry in enum: > > + cpp_enum = entry['name'] > > + py_enum = entry['name'][len(prefix):] > > > > - out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) > > + out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) > > > > - out += '\t;\n\n' > > + out += '\t;\n\n' > > I think we've reached a major milestone: making Python code as > unreadable as Perl :-) Not your fault, I'll try to find time to convert > this to jinja templates. I know... :) > > > > > - 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): > > @@ -90,7 +104,10 @@ def main(argv): > > return -1 > > > > data = open(args.input, 'rb').read() > > - controls = yaml.safe_load(data)['controls'] > > + > > + controls = {} > > + vendor = yaml.safe_load(data)['vendor'] > > + controls[vendor] = yaml.safe_load(data)['controls'] > > > > data = generate_py(controls, args.mode) > > > > 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..f7b5ec8c635d 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..8f2f8fdb02c3 100755 > > --- a/utils/gen-controls.py > > +++ b/utils/gen-controls.py > > @@ -35,11 +35,12 @@ class ControlEnum(object): > > > > > > class Control(object): > > - def __init__(self, name, data): > > + def __init__(self, name, data, vendor): > > self.__name = name > > self.__data = data > > self.__enum_values = None > > self.__size = None > > + self.__vendor = vendor > > > > enum_values = data.get('enum') > > if enum_values is not None: > > @@ -89,6 +90,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.__vendor > > + > > @property > > def name(self): > > """The control name (CamelCase)""" > > @@ -145,15 +151,19 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor > > + if vendor not in ctrls_doc: > > + ctrls_doc[vendor] = [] > > + ctrls_def[vendor] = [] > > + ctrls_map[vendor] = [] > > + > > info = { > > 'name': ctrl.name, > > 'type': ctrl.type, > > @@ -161,11 +171,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 +209,87 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor > > + 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 +311,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_HAS_${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,16 +363,19 @@ 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') > > Let's keep arguments sorted alphabetically, this should go before the > '-o' option. I would also name if '--mode', '-m' to have a short > argument, and change '-o' to '--output', '-o', for consistency. > > > args = parser.parse_args(argv[1:]) > > > > data = open(args.input, 'rb').read() > > + vendor = yaml.safe_load(data)['vendor'] > > controls = yaml.safe_load(data)['controls'] > > - controls = [Control(*ctrl.popitem()) for ctrl in controls] > > + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] > > > > 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
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} + } /* namespace controls */ } /* namespace libcamera */ diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build index a24c50d66a82..2c8c0258c95e 100644 --- a/include/libcamera/meson.build +++ b/include/libcamera/meson.build @@ -32,20 +32,21 @@ 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_ids.h and property_ids.h and associated modes +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/control_ids.yaml b/src/libcamera/control_ids.yaml index 5827d7ecef49..ff74ce1deedb 100644 --- a/src/libcamera/control_ids.yaml +++ b/src/libcamera/control_ids.yaml @@ -6,6 +6,7 @@ --- # Unless otherwise stated, all controls are bi-directional, i.e. they can be # set through Request::controls() and returned out through Request::metadata(). +vendor: libcamera controls: - AeEnable: type: bool 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/libcamera/property_ids.yaml b/src/libcamera/property_ids.yaml index f35563842a5a..45f3609b4236 100644 --- a/src/libcamera/property_ids.yaml +++ b/src/libcamera/property_ids.yaml @@ -4,6 +4,7 @@ # %YAML 1.1 --- +vendor: libcamera controls: - Location: type: int32_t diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py index 9948c41e42b1..8f14cdafe313 100755 --- a/src/py/libcamera/gen-py-controls.py +++ b/src/py/libcamera/gen-py-controls.py @@ -24,45 +24,59 @@ def find_common_prefix(strings): def generate_py(controls, mode): out = '' - for ctrl in controls: - name, ctrl = ctrl.popitem() - - if ctrl.get('draft'): - ns = 'libcamera::{}::draft::'.format(mode) - container = 'draft' - else: - ns = 'libcamera::{}::'.format(mode) - container = 'controls' + vendors_class_def = [] + vendor_defs = [] + vendors = [] + for vendor, ctrl_list in controls.items(): + for ctrls in ctrl_list: + name, ctrl = ctrls.popitem() + + if 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)) + vendors.append(vendor) + + if ctrl.get('draft'): + ns = 'libcamera::{}::draft::'.format(mode) + container = 'draft' + elif vendor != 'libcamera': + ns = 'libcamera::{}::{}::'.format(mode, vendor) + container = vendor + else: + ns = 'libcamera::{}::'.format(mode) + container = 'controls' - out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' + out += f'\t{container}.def_readonly_static("{name}", static_cast<const libcamera::ControlId *>(&{ns}{name}));\n\n' - enum = ctrl.get('enum') - if not enum: - continue + enum = ctrl.get('enum') + if not enum: + continue - cpp_enum = name + 'Enum' + cpp_enum = name + 'Enum' - out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) + out += '\tpy::enum_<{}{}>({}, \"{}\")\n'.format(ns, cpp_enum, container, cpp_enum) - if mode == 'controls': - # Adjustments for controls - if name == 'LensShadingMapMode': - prefix = 'LensShadingMapMode' + if mode == 'controls': + # Adjustments for controls + if name == 'LensShadingMapMode': + prefix = 'LensShadingMapMode' + else: + prefix = find_common_prefix([e['name'] for e in enum]) else: + # Adjustments for properties prefix = find_common_prefix([e['name'] for e in enum]) - else: - # Adjustments for properties - prefix = find_common_prefix([e['name'] for e in enum]) - for entry in enum: - cpp_enum = entry['name'] - py_enum = entry['name'][len(prefix):] + for entry in enum: + cpp_enum = entry['name'] + py_enum = entry['name'][len(prefix):] - out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) + out += '\t\t.value(\"{}\", {}{})\n'.format(py_enum, ns, cpp_enum) - out += '\t;\n\n' + 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): @@ -90,7 +104,10 @@ def main(argv): return -1 data = open(args.input, 'rb').read() - controls = yaml.safe_load(data)['controls'] + + controls = {} + vendor = yaml.safe_load(data)['vendor'] + controls[vendor] = yaml.safe_load(data)['controls'] data = generate_py(controls, args.mode) 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..f7b5ec8c635d 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..8f2f8fdb02c3 100755 --- a/utils/gen-controls.py +++ b/utils/gen-controls.py @@ -35,11 +35,12 @@ class ControlEnum(object): class Control(object): - def __init__(self, name, data): + def __init__(self, name, data, vendor): self.__name = name self.__data = data self.__enum_values = None self.__size = None + self.__vendor = vendor enum_values = data.get('enum') if enum_values is not None: @@ -89,6 +90,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.__vendor + @property def name(self): """The control name (CamelCase)""" @@ -145,15 +151,19 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor + if vendor not in ctrls_doc: + ctrls_doc[vendor] = [] + ctrls_def[vendor] = [] + ctrls_map[vendor] = [] + info = { 'name': ctrl.name, 'type': ctrl.type, @@ -161,11 +171,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 +209,87 @@ ${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 = 'draft' if ctrl.is_draft else ctrl.vendor + 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 +311,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_HAS_${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,16 +363,19 @@ 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() + vendor = yaml.safe_load(data)['vendor'] controls = yaml.safe_load(data)['controls'] - controls = [Control(*ctrl.popitem()) for ctrl in controls] + controls = [Control(*ctrl.popitem(), vendor) for ctrl in controls] 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')
Add support for vendor specific controls and properties to libcamera. The controls/properties are defined by a "vendor" tag in the YAML control description file, for example: vendor: rpi controls: - MyExampleControl: type: string description: | Test for libcamera vendor specific controls. This will now generate a control id in the libcamera::controls::rpi 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_HAS_RPI_VENDOR_CONTROLS is also generated to allow applications to conditionally compile code if the specific vendor controls are present. For the python bindings, the control is available with libcamera.controls.rpi.MyExampleControl. The above controls example applies similarly to properties. Existing libcamera controls defined in control_ids.yaml are given the "libcamera" vendor tag. 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@raspberrypi.com> --- include/libcamera/control_ids.h.in | 2 + include/libcamera/meson.build | 15 +- include/libcamera/property_ids.h.in | 2 + src/libcamera/control_ids.cpp.in | 6 + src/libcamera/control_ids.yaml | 1 + src/libcamera/meson.build | 5 +- src/libcamera/property_ids.cpp.in | 6 + src/libcamera/property_ids.yaml | 1 + src/py/libcamera/gen-py-controls.py | 75 +++++---- src/py/libcamera/py_controls_generated.cpp.in | 3 + .../libcamera/py_properties_generated.cpp.in | 3 + utils/gen-controls.py | 144 ++++++++++++++---- 12 files changed, 194 insertions(+), 69 deletions(-)