Message ID | 20231110110002.21381-1-naush@raspberrypi.com |
---|---|
Headers | show |
Series |
|
Related | show |
Hi, Gentle ping on some feedback for this work please. Thanks! Naush On Fri, 10 Nov 2023 at 10:59, Naushir Patuck <naush@raspberrypi.com> wrote: > > Hi, > > This change introduces a first attempt at implementing vendor specific controls > and properties in libcamera. Vendor controls and properties live in their own > namespace, and each vendor must now reserve a numeric control id range to avoid > any id value clashes. Vendor controls may either live in the libcamera > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > files. > > To designate a vendor control, the YAML control description must contain a > "vendor: <vendor_string>" tag. For example, > > - RaspberryPiExampleControl: > type: string > vendor: rpi > description: | > Example vendor control with the "rpi" vendor tag. > > will create a control in the libcamera::controls::rpi namespace, with the > numeric id value taken from the "rpi" reservation. Additionally, a #define > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > compile in vendor control support. Similar applies to the property generation. > > The current mechanism for draft controls and propertiers have been deprecated, > and now these are designated with the "draft" vendor tag. Note that this causes > an API breaking change since the numeric control id values for draft control > have their own designated range and namespace. So, for example, the use of > controls::NOISE_REDUCTION_MODE will need to be replaced with > controls::draft::NOISE_REDUCTION_MODE. > > Thanks, > Naush > > Naushir Patuck (5): > controls: Add vendor control/property support to generation scripts > controls: build: Allow separate vendor control YAML files > libcamera: control: Add vendor control id range reservation > libcamera: controls: Use vendor tags for draft controls and properties > documentation: Document vendor specific control and properties > handling > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > include/libcamera/control_ids.h.in | 6 +- > include/libcamera/meson.build | 58 +++++- > include/libcamera/property_ids.h.in | 8 +- > meson.build | 2 + > src/ipa/rpi/common/ipa_base.cpp | 2 +- > src/ipa/rpi/vc4/vc4.cpp | 2 +- > src/libcamera/control_ids.cpp.in | 16 +- > src/libcamera/control_ids.yaml | 20 +-- > src/libcamera/control_ids_rpi.yaml | 17 ++ > src/libcamera/control_ranges.yaml | 17 ++ > src/libcamera/meson.build | 20 ++- > src/libcamera/property_ids.cpp.in | 16 +- > src/libcamera/property_ids.yaml | 2 +- > src/py/libcamera/gen-py-controls.py | 31 +++- > src/py/libcamera/meson.build | 26 +-- > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > .../libcamera/py_properties_generated.cpp.in | 4 +- > utils/gen-controls.py | 169 +++++++++++++----- > 19 files changed, 348 insertions(+), 127 deletions(-) > create mode 100644 src/libcamera/control_ids_rpi.yaml > create mode 100644 src/libcamera/control_ranges.yaml > > -- > 2.34.1 >
Hi Naush, On Fri, Nov 17, 2023 at 10:41:18AM +0000, Naushir Patuck via libcamera-devel wrote: > Hi, > > Gentle ping on some feedback for this work please. I haven't forgotten, and I'll review this series today. I've attended LPC on Monday to Wednesday this week, which made the last few days quite hectic. > On Fri, 10 Nov 2023 at 10:59, Naushir Patuck wrote: > > > > Hi, > > > > This change introduces a first attempt at implementing vendor specific controls > > and properties in libcamera. Vendor controls and properties live in their own > > namespace, and each vendor must now reserve a numeric control id range to avoid > > any id value clashes. Vendor controls may either live in the libcamera > > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > > files. > > > > To designate a vendor control, the YAML control description must contain a > > "vendor: <vendor_string>" tag. For example, > > > > - RaspberryPiExampleControl: > > type: string > > vendor: rpi > > description: | > > Example vendor control with the "rpi" vendor tag. > > > > will create a control in the libcamera::controls::rpi namespace, with the > > numeric id value taken from the "rpi" reservation. Additionally, a #define > > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > > compile in vendor control support. Similar applies to the property generation. > > > > The current mechanism for draft controls and propertiers have been deprecated, > > and now these are designated with the "draft" vendor tag. Note that this causes > > an API breaking change since the numeric control id values for draft control > > have their own designated range and namespace. So, for example, the use of > > controls::NOISE_REDUCTION_MODE will need to be replaced with > > controls::draft::NOISE_REDUCTION_MODE. > > > > Thanks, > > Naush > > > > Naushir Patuck (5): > > controls: Add vendor control/property support to generation scripts > > controls: build: Allow separate vendor control YAML files > > libcamera: control: Add vendor control id range reservation > > libcamera: controls: Use vendor tags for draft controls and properties > > documentation: Document vendor specific control and properties > > handling > > > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > > include/libcamera/control_ids.h.in | 6 +- > > include/libcamera/meson.build | 58 +++++- > > include/libcamera/property_ids.h.in | 8 +- > > meson.build | 2 + > > src/ipa/rpi/common/ipa_base.cpp | 2 +- > > src/ipa/rpi/vc4/vc4.cpp | 2 +- > > src/libcamera/control_ids.cpp.in | 16 +- > > src/libcamera/control_ids.yaml | 20 +-- > > src/libcamera/control_ids_rpi.yaml | 17 ++ > > src/libcamera/control_ranges.yaml | 17 ++ > > src/libcamera/meson.build | 20 ++- > > src/libcamera/property_ids.cpp.in | 16 +- > > src/libcamera/property_ids.yaml | 2 +- > > src/py/libcamera/gen-py-controls.py | 31 +++- > > src/py/libcamera/meson.build | 26 +-- > > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > > .../libcamera/py_properties_generated.cpp.in | 4 +- > > utils/gen-controls.py | 169 +++++++++++++----- > > 19 files changed, 348 insertions(+), 127 deletions(-) > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > create mode 100644 src/libcamera/control_ranges.yaml
Hi Naush, Thank you for the patches. On Fri, Nov 10, 2023 at 10:59:57AM +0000, Naushir Patuck via libcamera-devel wrote: > Hi, > > This change introduces a first attempt at implementing vendor specific controls > and properties in libcamera. Vendor controls and properties live in their own > namespace, and each vendor must now reserve a numeric control id range to avoid > any id value clashes. Vendor controls may either live in the libcamera > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > files. > > To designate a vendor control, the YAML control description must contain a > "vendor: <vendor_string>" tag. For example, > > - RaspberryPiExampleControl: > type: string > vendor: rpi > description: | > Example vendor control with the "rpi" vendor tag. I'm concerned that having to tag each control with the vendor, coupled with the ability to declare vendor controls in the main control_ids.yaml file, will make it a bit messy and difficult to maintain. I would prefer enforcing separation of vendor controls in per-vendor files right away. You can then add the vendor property at the top level in the YAML file, instead of specifying it per-control. I'm also thinking that we should probably move the control and property YAML files to a subdirectory, but that's something I can do on top. > will create a control in the libcamera::controls::rpi namespace, with the > numeric id value taken from the "rpi" reservation. Additionally, a #define > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > compile in vendor control support. Similar applies to the property generation. > > The current mechanism for draft controls and propertiers have been deprecated, > and now these are designated with the "draft" vendor tag. Note that this causes > an API breaking change since the numeric control id values for draft control > have their own designated range and namespace. So, for example, the use of > controls::NOISE_REDUCTION_MODE will need to be replaced with > controls::draft::NOISE_REDUCTION_MODE. I like that, thanks. > Naushir Patuck (5): > controls: Add vendor control/property support to generation scripts > controls: build: Allow separate vendor control YAML files > libcamera: control: Add vendor control id range reservation > libcamera: controls: Use vendor tags for draft controls and properties > documentation: Document vendor specific control and properties > handling > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > include/libcamera/control_ids.h.in | 6 +- > include/libcamera/meson.build | 58 +++++- > include/libcamera/property_ids.h.in | 8 +- > meson.build | 2 + > src/ipa/rpi/common/ipa_base.cpp | 2 +- > src/ipa/rpi/vc4/vc4.cpp | 2 +- > src/libcamera/control_ids.cpp.in | 16 +- > src/libcamera/control_ids.yaml | 20 +-- > src/libcamera/control_ids_rpi.yaml | 17 ++ > src/libcamera/control_ranges.yaml | 17 ++ > src/libcamera/meson.build | 20 ++- > src/libcamera/property_ids.cpp.in | 16 +- > src/libcamera/property_ids.yaml | 2 +- > src/py/libcamera/gen-py-controls.py | 31 +++- > src/py/libcamera/meson.build | 26 +-- > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > .../libcamera/py_properties_generated.cpp.in | 4 +- > utils/gen-controls.py | 169 +++++++++++++----- > 19 files changed, 348 insertions(+), 127 deletions(-) > create mode 100644 src/libcamera/control_ids_rpi.yaml > create mode 100644 src/libcamera/control_ranges.yaml
Quoting Laurent Pinchart via libcamera-devel (2023-11-20 02:32:35) > Hi Naush, > > Thank you for the patches. > > On Fri, Nov 10, 2023 at 10:59:57AM +0000, Naushir Patuck via libcamera-devel wrote: > > Hi, > > > > This change introduces a first attempt at implementing vendor specific controls > > and properties in libcamera. Vendor controls and properties live in their own > > namespace, and each vendor must now reserve a numeric control id range to avoid > > any id value clashes. Vendor controls may either live in the libcamera > > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > > files. > > > > To designate a vendor control, the YAML control description must contain a > > "vendor: <vendor_string>" tag. For example, > > > > - RaspberryPiExampleControl: > > type: string > > vendor: rpi > > description: | > > Example vendor control with the "rpi" vendor tag. > > I'm concerned that having to tag each control with the vendor, coupled > with the ability to declare vendor controls in the main control_ids.yaml > file, will make it a bit messy and difficult to maintain. I would prefer > enforcing separation of vendor controls in per-vendor files right away. > You can then add the vendor property at the top level in the YAML file, > instead of specifying it per-control. > > I'm also thinking that we should probably move the control and property > YAML files to a subdirectory, but that's something I can do on top. I would also likely prefer a per-vendor file. > > will create a control in the libcamera::controls::rpi namespace, with the > > numeric id value taken from the "rpi" reservation. Additionally, a #define > > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > > compile in vendor control support. Similar applies to the property generation. > > > > The current mechanism for draft controls and propertiers have been deprecated, > > and now these are designated with the "draft" vendor tag. Note that this causes > > an API breaking change since the numeric control id values for draft control > > have their own designated range and namespace. So, for example, the use of > > controls::NOISE_REDUCTION_MODE will need to be replaced with > > controls::draft::NOISE_REDUCTION_MODE. > > I like that, thanks. Yes, moving draft controls to become 'a vendor' set is a good demonstration and abstraction here I think. Interestingly, or rather ... annoyingly - that wasn't identified by the abi-compatibility checker ... ( ``` sudo apt install abi-compliance-checker ./utils/abi-compat.sh origin/master HEAD ``` ) > > > Naushir Patuck (5): > > controls: Add vendor control/property support to generation scripts > > controls: build: Allow separate vendor control YAML files > > libcamera: control: Add vendor control id range reservation > > libcamera: controls: Use vendor tags for draft controls and properties > > documentation: Document vendor specific control and properties > > handling > > > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > > include/libcamera/control_ids.h.in | 6 +- > > include/libcamera/meson.build | 58 +++++- > > include/libcamera/property_ids.h.in | 8 +- > > meson.build | 2 + > > src/ipa/rpi/common/ipa_base.cpp | 2 +- > > src/ipa/rpi/vc4/vc4.cpp | 2 +- > > src/libcamera/control_ids.cpp.in | 16 +- > > src/libcamera/control_ids.yaml | 20 +-- > > src/libcamera/control_ids_rpi.yaml | 17 ++ > > src/libcamera/control_ranges.yaml | 17 ++ > > src/libcamera/meson.build | 20 ++- > > src/libcamera/property_ids.cpp.in | 16 +- > > src/libcamera/property_ids.yaml | 2 +- > > src/py/libcamera/gen-py-controls.py | 31 +++- > > src/py/libcamera/meson.build | 26 +-- > > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > > .../libcamera/py_properties_generated.cpp.in | 4 +- > > utils/gen-controls.py | 169 +++++++++++++----- > > 19 files changed, 348 insertions(+), 127 deletions(-) > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > create mode 100644 src/libcamera/control_ranges.yaml > > -- > Regards, > > Laurent Pinchart
Hi Kieran, On Mon, Nov 20, 2023 at 06:34:44AM +0000, Kieran Bingham wrote: > Quoting Laurent Pinchart via libcamera-devel (2023-11-20 02:32:35) > > On Fri, Nov 10, 2023 at 10:59:57AM +0000, Naushir Patuck via libcamera-devel wrote: > > > Hi, > > > > > > This change introduces a first attempt at implementing vendor specific controls > > > and properties in libcamera. Vendor controls and properties live in their own > > > namespace, and each vendor must now reserve a numeric control id range to avoid > > > any id value clashes. Vendor controls may either live in the libcamera > > > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > > > files. > > > > > > To designate a vendor control, the YAML control description must contain a > > > "vendor: <vendor_string>" tag. For example, > > > > > > - RaspberryPiExampleControl: > > > type: string > > > vendor: rpi > > > description: | > > > Example vendor control with the "rpi" vendor tag. > > > > I'm concerned that having to tag each control with the vendor, coupled > > with the ability to declare vendor controls in the main control_ids.yaml > > file, will make it a bit messy and difficult to maintain. I would prefer > > enforcing separation of vendor controls in per-vendor files right away. > > You can then add the vendor property at the top level in the YAML file, > > instead of specifying it per-control. > > > > I'm also thinking that we should probably move the control and property > > YAML files to a subdirectory, but that's something I can do on top. > > I would also likely prefer a per-vendor file. > > > > will create a control in the libcamera::controls::rpi namespace, with the > > > numeric id value taken from the "rpi" reservation. Additionally, a #define > > > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > > > compile in vendor control support. Similar applies to the property generation. > > > > > > The current mechanism for draft controls and propertiers have been deprecated, > > > and now these are designated with the "draft" vendor tag. Note that this causes > > > an API breaking change since the numeric control id values for draft control > > > have their own designated range and namespace. So, for example, the use of > > > controls::NOISE_REDUCTION_MODE will need to be replaced with > > > controls::draft::NOISE_REDUCTION_MODE. > > > > I like that, thanks. > > Yes, moving draft controls to become 'a vendor' set is a good > demonstration and abstraction here I think. > > > Interestingly, or rather ... annoyingly - that wasn't identified by the > abi-compatibility checker ... > > ( > ``` > sudo apt install abi-compliance-checker > ./utils/abi-compat.sh origin/master HEAD > ``` > ) As the tool has been unmaintained for 2 years, trying it libabigail may be a good idea when time permits. When :-) > > > Naushir Patuck (5): > > > controls: Add vendor control/property support to generation scripts > > > controls: build: Allow separate vendor control YAML files > > > libcamera: control: Add vendor control id range reservation > > > libcamera: controls: Use vendor tags for draft controls and properties > > > documentation: Document vendor specific control and properties > > > handling > > > > > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > > > include/libcamera/control_ids.h.in | 6 +- > > > include/libcamera/meson.build | 58 +++++- > > > include/libcamera/property_ids.h.in | 8 +- > > > meson.build | 2 + > > > src/ipa/rpi/common/ipa_base.cpp | 2 +- > > > src/ipa/rpi/vc4/vc4.cpp | 2 +- > > > src/libcamera/control_ids.cpp.in | 16 +- > > > src/libcamera/control_ids.yaml | 20 +-- > > > src/libcamera/control_ids_rpi.yaml | 17 ++ > > > src/libcamera/control_ranges.yaml | 17 ++ > > > src/libcamera/meson.build | 20 ++- > > > src/libcamera/property_ids.cpp.in | 16 +- > > > src/libcamera/property_ids.yaml | 2 +- > > > src/py/libcamera/gen-py-controls.py | 31 +++- > > > src/py/libcamera/meson.build | 26 +-- > > > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > > > .../libcamera/py_properties_generated.cpp.in | 4 +- > > > utils/gen-controls.py | 169 +++++++++++++----- > > > 19 files changed, 348 insertions(+), 127 deletions(-) > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > create mode 100644 src/libcamera/control_ranges.yaml
Hi Laurent and Kieran, Thank you for the feedback. On Mon, 20 Nov 2023 at 08:40, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > On Mon, Nov 20, 2023 at 06:34:44AM +0000, Kieran Bingham wrote: > > Quoting Laurent Pinchart via libcamera-devel (2023-11-20 02:32:35) > > > On Fri, Nov 10, 2023 at 10:59:57AM +0000, Naushir Patuck via libcamera-devel wrote: > > > > Hi, > > > > > > > > This change introduces a first attempt at implementing vendor specific controls > > > > and properties in libcamera. Vendor controls and properties live in their own > > > > namespace, and each vendor must now reserve a numeric control id range to avoid > > > > any id value clashes. Vendor controls may either live in the libcamera > > > > control_ids.yaml/property_ids.yaml files or preferably separate vendor specific > > > > files. > > > > > > > > To designate a vendor control, the YAML control description must contain a > > > > "vendor: <vendor_string>" tag. For example, > > > > > > > > - RaspberryPiExampleControl: > > > > type: string > > > > vendor: rpi > > > > description: | > > > > Example vendor control with the "rpi" vendor tag. > > > > > > I'm concerned that having to tag each control with the vendor, coupled > > > with the ability to declare vendor controls in the main control_ids.yaml > > > file, will make it a bit messy and difficult to maintain. I would prefer > > > enforcing separation of vendor controls in per-vendor files right away. > > > You can then add the vendor property at the top level in the YAML file, > > > instead of specifying it per-control. > > > > > > I'm also thinking that we should probably move the control and property > > > YAML files to a subdirectory, but that's something I can do on top. > > > > I would also likely prefer a per-vendor file. Sure, I'll rework this and only allow vendor specific files for vendor controls. Regards, Naush > > > > > > will create a control in the libcamera::controls::rpi namespace, with the > > > > numeric id value taken from the "rpi" reservation. Additionally, a #define > > > > LIBCAMERA_RPI_VENDOR_CONTROL will be available for applications to selectively > > > > compile in vendor control support. Similar applies to the property generation. > > > > > > > > The current mechanism for draft controls and propertiers have been deprecated, > > > > and now these are designated with the "draft" vendor tag. Note that this causes > > > > an API breaking change since the numeric control id values for draft control > > > > have their own designated range and namespace. So, for example, the use of > > > > controls::NOISE_REDUCTION_MODE will need to be replaced with > > > > controls::draft::NOISE_REDUCTION_MODE. > > > > > > I like that, thanks. > > > > Yes, moving draft controls to become 'a vendor' set is a good > > demonstration and abstraction here I think. > > > > > > Interestingly, or rather ... annoyingly - that wasn't identified by the > > abi-compatibility checker ... > > > > ( > > ``` > > sudo apt install abi-compliance-checker > > ./utils/abi-compat.sh origin/master HEAD > > ``` > > ) > > As the tool has been unmaintained for 2 years, trying it libabigail may > be a good idea when time permits. When :-) > > > > > Naushir Patuck (5): > > > > controls: Add vendor control/property support to generation scripts > > > > controls: build: Allow separate vendor control YAML files > > > > libcamera: control: Add vendor control id range reservation > > > > libcamera: controls: Use vendor tags for draft controls and properties > > > > documentation: Document vendor specific control and properties > > > > handling > > > > > > > > Documentation/guides/pipeline-handler.rst | 53 ++++++ > > > > include/libcamera/control_ids.h.in | 6 +- > > > > include/libcamera/meson.build | 58 +++++- > > > > include/libcamera/property_ids.h.in | 8 +- > > > > meson.build | 2 + > > > > src/ipa/rpi/common/ipa_base.cpp | 2 +- > > > > src/ipa/rpi/vc4/vc4.cpp | 2 +- > > > > src/libcamera/control_ids.cpp.in | 16 +- > > > > src/libcamera/control_ids.yaml | 20 +-- > > > > src/libcamera/control_ids_rpi.yaml | 17 ++ > > > > src/libcamera/control_ranges.yaml | 17 ++ > > > > src/libcamera/meson.build | 20 ++- > > > > src/libcamera/property_ids.cpp.in | 16 +- > > > > src/libcamera/property_ids.yaml | 2 +- > > > > src/py/libcamera/gen-py-controls.py | 31 +++- > > > > src/py/libcamera/meson.build | 26 +-- > > > > src/py/libcamera/py_controls_generated.cpp.in | 6 +- > > > > .../libcamera/py_properties_generated.cpp.in | 4 +- > > > > utils/gen-controls.py | 169 +++++++++++++----- > > > > 19 files changed, 348 insertions(+), 127 deletions(-) > > > > create mode 100644 src/libcamera/control_ids_rpi.yaml > > > > create mode 100644 src/libcamera/control_ranges.yaml > > -- > Regards, > > Laurent Pinchart