| Message ID | 20220518131329.66994-18-tomi.valkeinen@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Tomi, Thank you for the patch. On Wed, May 18, 2022 at 04:13:28PM +0300, Tomi Valkeinen wrote: > Generate a list of pixel formats under "libcamera.formats". > > I'm not super happy about this solution, as the user can change the > formats (libcamera.formats.XRGB8888 = None) and, for some reason, I won't make it a blocker for this patch, but is there a way this could be fixed ? > pybind11-stubgen doesn't produce stubs for this. > > However, other than the two issues above, it works, and I haven't > figured out a better way. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > src/py/libcamera/gen-py-formats.py | 56 ++++++++++++++++++++ > src/py/libcamera/meson.build | 12 +++++ > src/py/libcamera/py_formats_generated.cpp.in | 21 ++++++++ > src/py/libcamera/py_main.cpp | 3 ++ > 4 files changed, 92 insertions(+) > create mode 100755 src/py/libcamera/gen-py-formats.py > create mode 100644 src/py/libcamera/py_formats_generated.cpp.in > > diff --git a/src/py/libcamera/gen-py-formats.py b/src/py/libcamera/gen-py-formats.py > new file mode 100755 > index 00000000..72565a25 > --- /dev/null > +++ b/src/py/libcamera/gen-py-formats.py > @@ -0,0 +1,56 @@ > +#!/usr/bin/env python3 > +# SPDX-License-Identifier: GPL-2.0-or-later > +# > +# Generate Python format definitions from YAML > + > +import argparse > +import string > +import sys > +import yaml > + > + > +def generate(formats): > + fmts = [] > + > + for format in formats: > + name, format = format.popitem() > + fmts.append(f'formats.attr("{name}") = &libcamera::formats::{name};') > + > + return {'formats': '\n'.join(fmts)} > + > + > +def fill_template(template, data): > + with open(template, encoding='utf-8') as f: > + template = f.read() > + > + template = string.Template(template) > + return template.substitute(data) > + > + > +def main(argv): > + parser = argparse.ArgumentParser() > + parser.add_argument('-o', dest='output', metavar='file', type=str, > + help='Output file name. Defaults to standard output if not specified.') > + parser.add_argument('input', type=str, > + help='Input file name.') > + parser.add_argument('template', type=str, > + help='Template file name.') > + args = parser.parse_args(argv[1:]) > + > + with open(args.input, encoding='utf-8') as f: > + formats = yaml.safe_load(f)['formats'] > + > + data = generate(formats) > + data = fill_template(args.template, data) > + > + if args.output: > + with open(args.output, 'w', encoding='utf-8') as f: > + f.write(data) > + else: > + sys.stdout.write(data) > + > + return 0 > + > + > +if __name__ == '__main__': > + sys.exit(main(sys.argv)) > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > index a3388c63..0a7b65f3 100644 > --- a/src/py/libcamera/meson.build > +++ b/src/py/libcamera/meson.build > @@ -30,6 +30,18 @@ pycamera_sources += custom_target('py_gen_controls', > output : ['py_control_enums_generated.cpp'], > command : [gen_py_control_enums, '-o', '@OUTPUT@', '@INPUT@']) > > +gen_py_formats_input_files = files([ > + '../../libcamera/formats.yaml', > + 'py_formats_generated.cpp.in', > +]) > + > +gen_py_formats = files('gen-py-formats.py') > + > +pycamera_sources += custom_target('py_gen_formats', > + input : gen_py_formats_input_files, > + output : ['py_formats_generated.cpp'], > + command : [gen_py_formats, '-o', '@OUTPUT@', '@INPUT@']) > + > pycamera_deps = [ > libcamera_public, > py3_dep, > diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in > new file mode 100644 > index 00000000..1100c024 > --- /dev/null > +++ b/src/py/libcamera/py_formats_generated.cpp.in > @@ -0,0 +1,21 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > + * > + * Python bindings - Auto-generated formats > + * > + * This file is auto-generated. Do not edit. > + */ > + > +#include <libcamera/formats.h> > + > +#include <pybind11/smart_holder.h> > + > +namespace py = pybind11; > + > +void init_py_formats_generated(py::module& m) > +{ > +auto formats = m.def_submodule("formats"); > + > +${formats} Could you indent this ? It's nicer to keep generated files readable. Apart from that, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > +} > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > index 1d941160..e7066841 100644 > --- a/src/py/libcamera/py_main.cpp > +++ b/src/py/libcamera/py_main.cpp > @@ -132,6 +132,7 @@ static void handleRequestCompleted(Request *req) > > void init_py_enums(py::module &m); > void init_py_control_enums_generated(py::module &m); > +void init_py_formats_generated(py::module &m); > void init_py_geometry(py::module &m); > > PYBIND11_MODULE(_libcamera, m) > @@ -171,6 +172,8 @@ PYBIND11_MODULE(_libcamera, m) > auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); > auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); > > + init_py_formats_generated(m); > + > /* Global functions */ > m.def("log_set_level", &logSetLevel); >
On 18/05/2022 16:54, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Wed, May 18, 2022 at 04:13:28PM +0300, Tomi Valkeinen wrote: >> Generate a list of pixel formats under "libcamera.formats". >> >> I'm not super happy about this solution, as the user can change the >> formats (libcamera.formats.XRGB8888 = None) and, for some reason, > > I won't make it a blocker for this patch, but is there a way this could > be fixed ? I don't know. I don't think it's a big issue. Usually you can overwrite all kinds of things in Python, so this is no different. I think another way would be to implement this in pure python (generated .py file). Perhaps a Python enum would work here. Whatever we figure out in the future, I believe the style the user uses will stay the same (libcamera.format.XRGB8888). >> pybind11-stubgen doesn't produce stubs for this. >> >> However, other than the two issues above, it works, and I haven't >> figured out a better way. >> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> src/py/libcamera/gen-py-formats.py | 56 ++++++++++++++++++++ >> src/py/libcamera/meson.build | 12 +++++ >> src/py/libcamera/py_formats_generated.cpp.in | 21 ++++++++ >> src/py/libcamera/py_main.cpp | 3 ++ >> 4 files changed, 92 insertions(+) >> create mode 100755 src/py/libcamera/gen-py-formats.py >> create mode 100644 src/py/libcamera/py_formats_generated.cpp.in >> >> diff --git a/src/py/libcamera/gen-py-formats.py b/src/py/libcamera/gen-py-formats.py >> new file mode 100755 >> index 00000000..72565a25 >> --- /dev/null >> +++ b/src/py/libcamera/gen-py-formats.py >> @@ -0,0 +1,56 @@ >> +#!/usr/bin/env python3 >> +# SPDX-License-Identifier: GPL-2.0-or-later >> +# >> +# Generate Python format definitions from YAML >> + >> +import argparse >> +import string >> +import sys >> +import yaml >> + >> + >> +def generate(formats): >> + fmts = [] >> + >> + for format in formats: >> + name, format = format.popitem() >> + fmts.append(f'formats.attr("{name}") = &libcamera::formats::{name};') >> + >> + return {'formats': '\n'.join(fmts)} >> + >> + >> +def fill_template(template, data): >> + with open(template, encoding='utf-8') as f: >> + template = f.read() >> + >> + template = string.Template(template) >> + return template.substitute(data) >> + >> + >> +def main(argv): >> + parser = argparse.ArgumentParser() >> + parser.add_argument('-o', dest='output', metavar='file', type=str, >> + help='Output file name. Defaults to standard output if not specified.') >> + parser.add_argument('input', type=str, >> + help='Input file name.') >> + parser.add_argument('template', type=str, >> + help='Template file name.') >> + args = parser.parse_args(argv[1:]) >> + >> + with open(args.input, encoding='utf-8') as f: >> + formats = yaml.safe_load(f)['formats'] >> + >> + data = generate(formats) >> + data = fill_template(args.template, data) >> + >> + if args.output: >> + with open(args.output, 'w', encoding='utf-8') as f: >> + f.write(data) >> + else: >> + sys.stdout.write(data) >> + >> + return 0 >> + >> + >> +if __name__ == '__main__': >> + sys.exit(main(sys.argv)) >> diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build >> index a3388c63..0a7b65f3 100644 >> --- a/src/py/libcamera/meson.build >> +++ b/src/py/libcamera/meson.build >> @@ -30,6 +30,18 @@ pycamera_sources += custom_target('py_gen_controls', >> output : ['py_control_enums_generated.cpp'], >> command : [gen_py_control_enums, '-o', '@OUTPUT@', '@INPUT@']) >> >> +gen_py_formats_input_files = files([ >> + '../../libcamera/formats.yaml', >> + 'py_formats_generated.cpp.in', >> +]) >> + >> +gen_py_formats = files('gen-py-formats.py') >> + >> +pycamera_sources += custom_target('py_gen_formats', >> + input : gen_py_formats_input_files, >> + output : ['py_formats_generated.cpp'], >> + command : [gen_py_formats, '-o', '@OUTPUT@', '@INPUT@']) >> + >> pycamera_deps = [ >> libcamera_public, >> py3_dep, >> diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in >> new file mode 100644 >> index 00000000..1100c024 >> --- /dev/null >> +++ b/src/py/libcamera/py_formats_generated.cpp.in >> @@ -0,0 +1,21 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> + * >> + * Python bindings - Auto-generated formats >> + * >> + * This file is auto-generated. Do not edit. >> + */ >> + >> +#include <libcamera/formats.h> >> + >> +#include <pybind11/smart_holder.h> >> + >> +namespace py = pybind11; >> + >> +void init_py_formats_generated(py::module& m) >> +{ >> +auto formats = m.def_submodule("formats"); >> + >> +${formats} > > Could you indent this ? It's nicer to keep generated files readable. Sure. Tomi
Quoting Laurent Pinchart (2022-05-18 14:54:39) > Hi Tomi, > > Thank you for the patch. > > On Wed, May 18, 2022 at 04:13:28PM +0300, Tomi Valkeinen wrote: > > Generate a list of pixel formats under "libcamera.formats". > > > > I'm not super happy about this solution, as the user can change the > > formats (libcamera.formats.XRGB8888 = None) and, for some reason, > > I won't make it a blocker for this patch, but is there a way this could > be fixed ? > Are these types generated with this name 'libcamera.' or is that inherited by being in the module? I.e. would "import libcamera as cam" make these cam.formats.XRGB8888 ? > > pybind11-stubgen doesn't produce stubs for this. > > > > However, other than the two issues above, it works, and I haven't > > figured out a better way. > > > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > --- > > src/py/libcamera/gen-py-formats.py | 56 ++++++++++++++++++++ > > src/py/libcamera/meson.build | 12 +++++ > > src/py/libcamera/py_formats_generated.cpp.in | 21 ++++++++ > > src/py/libcamera/py_main.cpp | 3 ++ > > 4 files changed, 92 insertions(+) > > create mode 100755 src/py/libcamera/gen-py-formats.py > > create mode 100644 src/py/libcamera/py_formats_generated.cpp.in > > > > diff --git a/src/py/libcamera/gen-py-formats.py b/src/py/libcamera/gen-py-formats.py > > new file mode 100755 > > index 00000000..72565a25 > > --- /dev/null > > +++ b/src/py/libcamera/gen-py-formats.py > > @@ -0,0 +1,56 @@ > > +#!/usr/bin/env python3 > > +# SPDX-License-Identifier: GPL-2.0-or-later > > +# > > +# Generate Python format definitions from YAML > > + > > +import argparse > > +import string > > +import sys > > +import yaml > > + > > + > > +def generate(formats): > > + fmts = [] > > + > > + for format in formats: > > + name, format = format.popitem() > > + fmts.append(f'formats.attr("{name}") = &libcamera::formats::{name};') > > + > > + return {'formats': '\n'.join(fmts)} > > + > > + > > +def fill_template(template, data): > > + with open(template, encoding='utf-8') as f: > > + template = f.read() > > + > > + template = string.Template(template) > > + return template.substitute(data) > > + > > + > > +def main(argv): > > + parser = argparse.ArgumentParser() > > + parser.add_argument('-o', dest='output', metavar='file', type=str, > > + help='Output file name. Defaults to standard output if not specified.') > > + parser.add_argument('input', type=str, > > + help='Input file name.') > > + parser.add_argument('template', type=str, > > + help='Template file name.') > > + args = parser.parse_args(argv[1:]) > > + > > + with open(args.input, encoding='utf-8') as f: > > + formats = yaml.safe_load(f)['formats'] > > + > > + data = generate(formats) > > + data = fill_template(args.template, data) > > + > > + if args.output: > > + with open(args.output, 'w', encoding='utf-8') as f: > > + f.write(data) > > + else: > > + sys.stdout.write(data) > > + > > + return 0 > > + > > + > > +if __name__ == '__main__': > > + sys.exit(main(sys.argv)) > > diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build > > index a3388c63..0a7b65f3 100644 > > --- a/src/py/libcamera/meson.build > > +++ b/src/py/libcamera/meson.build > > @@ -30,6 +30,18 @@ pycamera_sources += custom_target('py_gen_controls', > > output : ['py_control_enums_generated.cpp'], > > command : [gen_py_control_enums, '-o', '@OUTPUT@', '@INPUT@']) > > > > +gen_py_formats_input_files = files([ > > + '../../libcamera/formats.yaml', > > + 'py_formats_generated.cpp.in', > > +]) > > + > > +gen_py_formats = files('gen-py-formats.py') > > + > > +pycamera_sources += custom_target('py_gen_formats', > > + input : gen_py_formats_input_files, > > + output : ['py_formats_generated.cpp'], > > + command : [gen_py_formats, '-o', '@OUTPUT@', '@INPUT@']) > > + > > pycamera_deps = [ > > libcamera_public, > > py3_dep, > > diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in > > new file mode 100644 > > index 00000000..1100c024 > > --- /dev/null > > +++ b/src/py/libcamera/py_formats_generated.cpp.in > > @@ -0,0 +1,21 @@ > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > +/* > > + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > > + * > > + * Python bindings - Auto-generated formats > > + * > > + * This file is auto-generated. Do not edit. > > + */ > > + > > +#include <libcamera/formats.h> > > + > > +#include <pybind11/smart_holder.h> > > + > > +namespace py = pybind11; > > + > > +void init_py_formats_generated(py::module& m) > > +{ > > +auto formats = m.def_submodule("formats"); > > + > > +${formats} > > Could you indent this ? It's nicer to keep generated files readable. > > Apart from that, > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > +} > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp > > index 1d941160..e7066841 100644 > > --- a/src/py/libcamera/py_main.cpp > > +++ b/src/py/libcamera/py_main.cpp > > @@ -132,6 +132,7 @@ static void handleRequestCompleted(Request *req) > > > > void init_py_enums(py::module &m); > > void init_py_control_enums_generated(py::module &m); > > +void init_py_formats_generated(py::module &m); > > void init_py_geometry(py::module &m); > > > > PYBIND11_MODULE(_libcamera, m) > > @@ -171,6 +172,8 @@ PYBIND11_MODULE(_libcamera, m) > > auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); > > auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); > > > > + init_py_formats_generated(m); > > + > > /* Global functions */ > > m.def("log_set_level", &logSetLevel); > > > > -- > Regards, > > Laurent Pinchart
On 18/05/2022 16:54, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Wed, May 18, 2022 at 04:13:28PM +0300, Tomi Valkeinen wrote: >> Generate a list of pixel formats under "libcamera.formats". >> >> I'm not super happy about this solution, as the user can change the >> formats (libcamera.formats.XRGB8888 = None) and, for some reason, > > I won't make it a blocker for this patch, but is there a way this could > be fixed ? Another way would be to: class dummy_base {}; void init_py_formats_generated(py::module& m) { py::class_<dummy_base>(m, "formats") .def_readonly_static("R8", &libcamera::formats::R8) .def_readonly_static("R10", &libcamera::formats::R10) .def_readonly_static("R12", &libcamera::formats::R12) ... So, we would create a class to hold the formats, and then we can use def_readonly_static. We do need a C++ base class, thus the dummy_base... If we don't mind changing the API a bit, we could have these readonly statics inside PixelFormat, so: libcamera.PixelFormat.MJPEG Tomi
Hi Tomi, On Wed, May 18, 2022 at 06:00:01PM +0300, Tomi Valkeinen wrote: > On 18/05/2022 16:54, Laurent Pinchart wrote: > > On Wed, May 18, 2022 at 04:13:28PM +0300, Tomi Valkeinen wrote: > >> Generate a list of pixel formats under "libcamera.formats". > >> > >> I'm not super happy about this solution, as the user can change the > >> formats (libcamera.formats.XRGB8888 = None) and, for some reason, > > > > I won't make it a blocker for this patch, but is there a way this could > > be fixed ? > > Another way would be to: > > class dummy_base {}; > > void init_py_formats_generated(py::module& m) > { > py::class_<dummy_base>(m, "formats") > .def_readonly_static("R8", &libcamera::formats::R8) > .def_readonly_static("R10", &libcamera::formats::R10) > .def_readonly_static("R12", &libcamera::formats::R12) > ... > > So, we would create a class to hold the formats, and then we can use > def_readonly_static. We do need a C++ base class, thus the dummy_base... The dummy_base class name is internal, not exposed to Python, right ? I'm then fine with this. We should use a name that would be a bit more explicit, such as PyFormats for instance. > If we don't mind changing the API a bit, we could have these readonly > statics inside PixelFormat, so: > > libcamera.PixelFormat.MJPEG If possible I'd like to keep the two APIs as close as possible.
diff --git a/src/py/libcamera/gen-py-formats.py b/src/py/libcamera/gen-py-formats.py new file mode 100755 index 00000000..72565a25 --- /dev/null +++ b/src/py/libcamera/gen-py-formats.py @@ -0,0 +1,56 @@ +#!/usr/bin/env python3 +# SPDX-License-Identifier: GPL-2.0-or-later +# +# Generate Python format definitions from YAML + +import argparse +import string +import sys +import yaml + + +def generate(formats): + fmts = [] + + for format in formats: + name, format = format.popitem() + fmts.append(f'formats.attr("{name}") = &libcamera::formats::{name};') + + return {'formats': '\n'.join(fmts)} + + +def fill_template(template, data): + with open(template, encoding='utf-8') as f: + template = f.read() + + template = string.Template(template) + return template.substitute(data) + + +def main(argv): + parser = argparse.ArgumentParser() + parser.add_argument('-o', dest='output', metavar='file', type=str, + help='Output file name. Defaults to standard output if not specified.') + parser.add_argument('input', type=str, + help='Input file name.') + parser.add_argument('template', type=str, + help='Template file name.') + args = parser.parse_args(argv[1:]) + + with open(args.input, encoding='utf-8') as f: + formats = yaml.safe_load(f)['formats'] + + data = generate(formats) + data = fill_template(args.template, data) + + if args.output: + with open(args.output, 'w', encoding='utf-8') as f: + f.write(data) + else: + sys.stdout.write(data) + + return 0 + + +if __name__ == '__main__': + sys.exit(main(sys.argv)) diff --git a/src/py/libcamera/meson.build b/src/py/libcamera/meson.build index a3388c63..0a7b65f3 100644 --- a/src/py/libcamera/meson.build +++ b/src/py/libcamera/meson.build @@ -30,6 +30,18 @@ pycamera_sources += custom_target('py_gen_controls', output : ['py_control_enums_generated.cpp'], command : [gen_py_control_enums, '-o', '@OUTPUT@', '@INPUT@']) +gen_py_formats_input_files = files([ + '../../libcamera/formats.yaml', + 'py_formats_generated.cpp.in', +]) + +gen_py_formats = files('gen-py-formats.py') + +pycamera_sources += custom_target('py_gen_formats', + input : gen_py_formats_input_files, + output : ['py_formats_generated.cpp'], + command : [gen_py_formats, '-o', '@OUTPUT@', '@INPUT@']) + pycamera_deps = [ libcamera_public, py3_dep, diff --git a/src/py/libcamera/py_formats_generated.cpp.in b/src/py/libcamera/py_formats_generated.cpp.in new file mode 100644 index 00000000..1100c024 --- /dev/null +++ b/src/py/libcamera/py_formats_generated.cpp.in @@ -0,0 +1,21 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2022, Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> + * + * Python bindings - Auto-generated formats + * + * This file is auto-generated. Do not edit. + */ + +#include <libcamera/formats.h> + +#include <pybind11/smart_holder.h> + +namespace py = pybind11; + +void init_py_formats_generated(py::module& m) +{ +auto formats = m.def_submodule("formats"); + +${formats} +} diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp index 1d941160..e7066841 100644 --- a/src/py/libcamera/py_main.cpp +++ b/src/py/libcamera/py_main.cpp @@ -132,6 +132,7 @@ static void handleRequestCompleted(Request *req) void init_py_enums(py::module &m); void init_py_control_enums_generated(py::module &m); +void init_py_formats_generated(py::module &m); void init_py_geometry(py::module &m); PYBIND11_MODULE(_libcamera, m) @@ -171,6 +172,8 @@ PYBIND11_MODULE(_libcamera, m) auto pyColorSpaceRange = py::enum_<ColorSpace::Range>(pyColorSpace, "Range"); auto pyPixelFormat = py::class_<PixelFormat>(m, "PixelFormat"); + init_py_formats_generated(m); + /* Global functions */ m.def("log_set_level", &logSetLevel);
Generate a list of pixel formats under "libcamera.formats". I'm not super happy about this solution, as the user can change the formats (libcamera.formats.XRGB8888 = None) and, for some reason, pybind11-stubgen doesn't produce stubs for this. However, other than the two issues above, it works, and I haven't figured out a better way. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> --- src/py/libcamera/gen-py-formats.py | 56 ++++++++++++++++++++ src/py/libcamera/meson.build | 12 +++++ src/py/libcamera/py_formats_generated.cpp.in | 21 ++++++++ src/py/libcamera/py_main.cpp | 3 ++ 4 files changed, 92 insertions(+) create mode 100755 src/py/libcamera/gen-py-formats.py create mode 100644 src/py/libcamera/py_formats_generated.cpp.in