[libcamera-devel] [PATCH v1 0/5] Vendor controls and properties

[Naushir Patuck]

Hi Laurent and Kieran,

Thank you for the feedback.

On Mon, 20 Nov 2023 at 08:40, Laurent Pinchart
<laurent.pinchart at 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