[libcamera-devel] [PATCH v3 3/7] build: controls: Rework how controls and properties are generated
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 30 10:08:13 CET 2023
On Tue, Nov 28, 2023 at 03:09:10PM +0000, Kieran Bingham via libcamera-devel wrote:
> Quoting Naushir Patuck via libcamera-devel (2023-11-27 15:45:35)
> > Hi Jacopo,
> >
> > Thank you for the feedback.
> >
> > On Mon, 27 Nov 2023 at 15:38, Jacopo Mondi
> > <jacopo.mondi at ideasonboard.com> wrote:
> > >
> > > Hi Naush
> > >
> > > On Fri, Nov 24, 2023 at 12:37:09PM +0000, Naushir Patuck via libcamera-devel wrote:
> > > > Add support for using separate YAML files for controls and properties
> > > > generation. The mapping of vendor/pipeline handler to control file is
> > > > done through the controls_map variable in include/libcamera/meson.build.
> > > >
> > > > This simplifies management of vendor control definitions and avoids
> > > > possible merge conflicts when changing the control_ids.yaml file for
> > > > core and draft controls. With this change, libcamera and draft controls
> > > > and properties files are designated the 'libcamera' vendor tag.
> > > >
> > > > In this change, we also rename control_ids.yaml -> control_ids_core.yaml
> > > > and property_ids.yaml -> property_ids_core.yaml to designate these as
> > > > core libcamera controls.
> > > >
> > > > Signed-off-by: Naushir Patuck <naush at raspberrypi.com>
> > > > ---
> > > > Documentation/guides/pipeline-handler.rst | 8 +--
> > > > include/libcamera/meson.build | 52 +++++++++++++++----
> > > > meson.build | 2 +
> > > > ...control_ids.yaml => control_ids_core.yaml} | 0
> > > > src/libcamera/meson.build | 21 ++++++--
> > > > ...operty_ids.yaml => property_ids_core.yaml} | 0
> > > > src/py/libcamera/gen-py-controls.py | 10 ++--
> > > > src/py/libcamera/meson.build | 12 ++++-
> > > > utils/gen-controls.py | 13 +++--
> > > > 9 files changed, 88 insertions(+), 30 deletions(-)
> > > > rename src/libcamera/{control_ids.yaml => control_ids_core.yaml} (100%)
> > > > rename src/libcamera/{property_ids.yaml => property_ids_core.yaml} (100%)
> > > >
> > > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > > > index 10b9c75c2a7f..66d428a19c4f 100644
> > > > --- a/Documentation/guides/pipeline-handler.rst
> > > > +++ b/Documentation/guides/pipeline-handler.rst
> > > > @@ -587,12 +587,12 @@ immutable properties of the ``Camera`` device.
> > > >
> > > > The libcamera controls and properties are defined in YAML form which is
> > > > processed to automatically generate documentation and interfaces. Controls are
> > > > -defined by the src/libcamera/`control_ids.yaml`_ file and camera properties
> > > > -are defined by src/libcamera/`properties_ids.yaml`_.
> > > > +defined by the src/libcamera/`control_ids_core.yaml`_ file and camera properties
> > > > +are defined by src/libcamera/`properties_ids_core.yaml`_.
> > > >
> > > > .. _controls framework: https://libcamera.org/api-html/controls_8h.html
> > > > -.. _control_ids.yaml: https://libcamera.org/api-html/control__ids_8h.html
> > > > -.. _properties_ids.yaml: https://libcamera.org/api-html/property__ids_8h.html
> > > > +.. _control_ids_core.yaml: https://libcamera.org/api-html/control__ids_8h.html
> > > > +.. _properties_ids_core.yaml: https://libcamera.org/api-html/property__ids_8h.html
> > > >
> > > > Pipeline handlers can optionally register the list of controls an application
> > > > can set as well as a list of immutable camera properties. Being both
> > > > diff --git a/include/libcamera/meson.build b/include/libcamera/meson.build
> > > > index 5fb772e6dd14..a763c8ff4344 100644
> > > > --- a/include/libcamera/meson.build
> > > > +++ b/include/libcamera/meson.build
> > > > @@ -32,22 +32,56 @@ install_headers(libcamera_public_headers,
> > > >
> > > > libcamera_headers_install_dir = get_option('includedir') / libcamera_include_dir
> > > >
> > > > -# control_ids.h and property_ids.h and associated modes
> > > > -control_source_files = {
> > > > - 'control_ids': 'controls',
> > > > - 'property_ids': 'properties',
> > > > +controls_map = {
> > > > + 'controls': {
> > > > + 'core': 'control_ids_core.yaml',
> > > > + },
> > > > +
> > > > + 'properties': {
> > > > + 'core': 'property_ids_core.yaml',
> > > > + }
> > > > }
> > > >
> > > > control_headers = []
> > > > +controls_files = []
> > > > +properties_files = []
> > > > +
> > > > +foreach mode, entry : controls_map
> > > > + files_list = []
> > > > + input_files = []
> > > > + foreach vendor, header : entry
> > > > + if vendor != 'core' and vendor != 'draft'
> > > > + if vendor not in pipelines
> > > > + message('vendor not in pipelines')
> > >
> > > This gives
> > >
> > > Message: vendor not in pipelines
> > > Message: vendor not in pipelines
> > >
> > > in the meson output log, that's not really useful imo
> >
> > Indeed. This was my debug message and should really not be in there!
> >
> > >
> > > > + continue
> > > > + endif
> > > > + endif
> > > > +
> > > > + if header in files_list
> > > > + message('header in files list')
> > > > + continue
> > > > + endif
> > > > +
> > > > + files_list += header
> > > > + input_files += files('../../src/libcamera/' + header)
> > > > + endforeach
> > > > +
> > > > + outfile = ''
> > > > + if mode == 'controls'
> > > > + outfile = 'control_ids.h'
> > > > + controls_files += files_list
> > > > + else
> > > > + outfile = 'property_ids.h'
> > > > + properties_files += files_list
> > > > + endif
> > > >
> > > > -foreach header, mode : control_source_files
> > > > - input_files = files('../../src/libcamera/' + header +'.yaml')
> > > > - template_file = files(header + '.h.in')
> > > > + template_file = files(outfile + '.in')
> > > > control_headers += custom_target(header + '_h',
> > > > input : input_files,
> > > > - output : header + '.h',
> > > > + output : outfile,
> > > > command : [gen_controls, '-o', '@OUTPUT@',
> > > > - '--mode', mode, '-t', template_file, '@INPUT@'],
> > > > + '--mode', mode, '-t', template_file,
> > > > + '@INPUT@'],
> > > > install : true,
> > > > install_dir : libcamera_headers_install_dir)
> > > > endforeach
> > > > diff --git a/meson.build b/meson.build
> > > > index e9a1c7e360ce..ee57cb780149 100644
> > > > --- a/meson.build
> > > > +++ b/meson.build
> > > > @@ -267,6 +267,8 @@ py_mod.find_installation('python3', modules : py_modules)
> > > > summary({
> > > > 'Enabled pipelines': pipelines,
> > > > 'Enabled IPA modules': enabled_ipa_names,
> > > > + 'Controls files': controls_files,
> > > > + 'Properties files': properties_files,
> > >
> > > Controls files : control_ids_draft.yaml
> > > control_ids_core.yaml
> > > Properties files : property_ids_draft.yaml
> > > property_ids_core.yaml
> > > This (not from this patch but with the whole series applied) is more
> > > useful indeed!
> > >
> > > > 'Hotplug support': libudev.found(),
> > > > 'Tracing support': tracing_enabled,
> > > > 'Android support': android_enabled,
> > > > diff --git a/src/libcamera/control_ids.yaml b/src/libcamera/control_ids_core.yaml
> > > > similarity index 100%
> > > > rename from src/libcamera/control_ids.yaml
> > > > rename to src/libcamera/control_ids_core.yaml
> > > > diff --git a/src/libcamera/meson.build b/src/libcamera/meson.build
> > > > index 05ee38daf22b..6d9902e6ffd1 100644
> > > > --- a/src/libcamera/meson.build
> > > > +++ b/src/libcamera/meson.build
> > > > @@ -127,12 +127,23 @@ endif
> > > >
> > > > control_sources = []
> > > >
> > > > -foreach source, mode : control_source_files
> > > > - input_files = files(source +'.yaml')
> > > > - template_file = files(source + '.cpp.in')
> > > > - control_sources += custom_target(source + '_cpp',
> > > > +controls_mode_files = {
> > > > + 'controls' : controls_files,
> > > > + 'properties' : properties_files,
> > > > +}
> > > > +
> > > > +foreach mode, input_files : controls_mode_files
> > > > + input_files = files(input_files)
> > > > +
> > > > + if mode == 'controls'
> > > > + template_file = files('control_ids.cpp.in')
> > > > + else
> > > > + template_file = files('property_ids.cpp.in')
> > > > + endif
> > > > +
> > > > + control_sources += custom_target(mode + '_cpp',
> > > > input : input_files,
> > > > - output : source + '.cpp',
> > > > + output : mode + '_ids.cpp',
> > > > command : [gen_controls, '-o', '@OUTPUT@',
> > > > '--mode', mode, '-t', template_file, '@INPUT@'])
> > > > endforeach
> > > > diff --git a/src/libcamera/property_ids.yaml b/src/libcamera/property_ids_core.yaml
> > > > similarity index 100%
> > > > rename from src/libcamera/property_ids.yaml
> > > > rename to src/libcamera/property_ids_core.yaml
> > > > diff --git a/src/py/libcamera/gen-py-controls.py b/src/py/libcamera/gen-py-controls.py
> > > > index cfcfd4d16acf..8ae8d5126e39 100755
> > > > --- a/src/py/libcamera/gen-py-controls.py
> > > > +++ b/src/py/libcamera/gen-py-controls.py
> > > > @@ -95,7 +95,7 @@ def main(argv):
> > > > help='Output file name. Defaults to standard output if not specified.')
> > > > parser.add_argument('--template', '-t', type=str, required=True,
> > > > help='Template file name.')
> > > > - parser.add_argument('input', type=str,
> > > > + parser.add_argument('input', type=str, nargs='+',
> > > > help='Input file name.')
> > > > args = parser.parse_args(argv[1:])
> > > >
> > > > @@ -103,11 +103,11 @@ def main(argv):
> > > > print(f'Invalid mode option "{args.mode}"', file=sys.stderr)
> > > > return -1
> > > >
> > > > - data = open(args.input, 'rb').read()
> > > > -
> > > > controls = {}
> > > > - vendor = yaml.safe_load(data)['vendor']
> > > > - controls[vendor] = yaml.safe_load(data)['controls']
> > > > + for input in args.input:
> > > > + data = open(input, 'rb').read()
> > > > + 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/meson.build b/src/py/libcamera/meson.build
> > > > index 1c3ea1843ac0..31af63ec0dc6 100644
> > > > --- a/src/py/libcamera/meson.build
> > > > +++ b/src/py/libcamera/meson.build
> > > > @@ -28,11 +28,15 @@ pycamera_sources = files([
> > > >
> > > > # Generate controls
> > > >
> > > > -gen_py_controls_input_files = files('../../libcamera/control_ids.yaml')
> > > > +gen_py_controls_input_files = []
> > > > gen_py_controls_template = files('py_controls_generated.cpp.in')
> > > >
> > > > gen_py_controls = files('gen-py-controls.py')
> > > >
> > > > +foreach file : controls_files
> > > > + gen_py_controls_input_files += files('../../libcamera/' + file)
> > > > +endforeach
> > > > +
> > > > pycamera_sources += custom_target('py_gen_controls',
> > > > input : gen_py_controls_input_files,
> > > > output : ['py_controls_generated.cpp'],
> > > > @@ -41,9 +45,13 @@ pycamera_sources += custom_target('py_gen_controls',
> > > >
> > > > # Generate properties
> > > >
> > > > -gen_py_property_enums_input_files = files('../../libcamera/property_ids.yaml')
> > > > +gen_py_property_enums_input_files = []
> > > > gen_py_properties_template = files('py_properties_generated.cpp.in')
> > > >
> > > > +foreach file : properties_files
> > > > + gen_py_property_enums_input_files += files('../../libcamera/' + file)
> > > > +endforeach
> > > > +
> > > > pycamera_sources += custom_target('py_gen_properties',
> > > > input : gen_py_property_enums_input_files,
> > > > output : ['py_properties_generated.cpp'],
> > > > diff --git a/utils/gen-controls.py b/utils/gen-controls.py
> > > > index 6680ecf84acb..30c58f35473c 100755
> > > > --- a/utils/gen-controls.py
> > > > +++ b/utils/gen-controls.py
> > > > @@ -12,6 +12,7 @@ import operator
> > > > import string
> > > > import sys
> > > > import yaml
> > > > +import os
> > > >
> > > >
> > > > class ControlEnum(object):
> > > > @@ -339,15 +340,17 @@ def main(argv):
> > > > help='Output file name. Defaults to standard output if not specified.')
> > > > parser.add_argument('--template', '-t', dest='template', type=str, required=True,
> > > > help='Template file name.')
> > > > - parser.add_argument('input', type=str,
> > > > + parser.add_argument('input', type=str, nargs='+',
> > > > help='Input file name.')
> > > >
> > > > 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(), vendor) for ctrl in controls]
> > > > + controls = []
> > > > + for input in args.input:
> > > > + data = open(input, 'rb').read()
> > > > + vendor = yaml.safe_load(data)['vendor']
> > > > + ctrls = yaml.safe_load(data)['controls']
> > > > + controls = controls + [Control(*ctrl.popitem(), vendor) for ctrl in ctrls]
> > >
> > > How does thi work in Python, should data be closed ?
> >
> > I think it does get closed since we don't hold onto a reference to the
> > open() return value. But please correct me if that is wrong since I
> > was (blindly) following what the existing code was doing.
> >
>
> Not a python expert enough here, but I know I often use "
> with open(input, 'rb') as file:
> data = file.read
>
> etc ... which automatically closes the file at the end of that scope.
That's the most pythonic way to handle scoping for a file object indeed,
when explicit management of the scope is needed (i.e. when you need to
know exactly when the file will be closed).
> With the style above - there is no handle to the opened file - so we
> don't have anything to call close on ... so it's a bit of refactoring
> either way - unless we can be sure python closes automatically.
Python will close the file. The reference count of object returned by
open() reaches 0 when the read() function returns, so the file will get
closed immediately as far as I understand:
>>> class Foo(object):
... def __init__(self):
... print('__init__')
... def __del__(self):
... print('__del__')
...
>>> f = Foo()
__init__
>>> f = None ; print('deleted')
__del__
deleted
> But ... we're certainly going to close when the file exits...
And there are few files anyway, so even if the objects stayed around
until a garbage collection pass it wouldn't matter that much.
> With that resolved in a suitable way (which could just be confirming
> that it autocloses if that works) and the debug message removed:
>
>
> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>
> > > Anyway, this all looks good, with the above 'message' dropped
> > >
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi at ideasonboard.com>
> > >
> > > >
> > > > if args.template.endswith('.cpp.in'):
> > > > data = generate_cpp(controls)
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list