[PATCH 0/3] gstreamer: Generate controls from control_ids_*.yaml files

[Laurent Pinchart]

On Mon, Aug 05, 2024 at 05:02:04PM -0400, Nicolas Dufresne wrote:
> Le lundi 05 août 2024 à 11:28 +0200, Jaslo Ziska a écrit :
> > Hi everyone,
> > 
> > in this patchset I implemented libcamerasrc properties which are generated from
> > the control_ids_*.yaml files.
> 
> I'm very happy to see that someone found that time to work on this.

Likewise :-)

> > The first commit removes the auto-focus-mode property which was already
> > implemented (it is added again in the next commit).
> > The second commit adds a Python script to generate C++ code capable of
> > interacting with the controls and adds the controls to libcamerasrc.
> > The third commit is just a small fix for the missing closing "greater than"
> > symbol in the author string I noticed.
> > 
> > The gstlibcamera-controls.h file is taken from Nicolas' branch with the change
> > that I removed the enum with the control names from the class. Instead the enum
> > variants from libcamera::controls:: are now used directly.
> > The structure of the gstlibcamera-controls.cpp.in file is also taken from
> > Nicolas. The only change is that its content now gets generated by
> > gen-gst-controls.py
> > 
> > The boilerplate of gen-gst-controls.py is mostly copied from gen-controls.py.
> > This is also where I have some questions:
> > 
> > The definition of the ControlEnum and Control Python classes (and some helper
> > functions) is now duplicate code. Should this be handled differently somehow to
> > avoid the code duplication?

I would be nice to avoid code duplication if possible, yes. I'll review
the patch and comment there.

> > I haven't added a copyright to gen-gst-controls.py yet as I am not sure because
> > I copied much of it from gen-controls.py.
> 
> Haven't checked, but I would probably have copied the copyright from original
> and added mine under.

That's a good approach.

> > Another small issue is that I haven't implemented the Rectangle type yet and
> > thus the controls using it won't show up. The reason for this is that there
> > is the AfWindows control which is an array of Rectangles. As the gstreamer
> > properties can only be glib types I wasn't sure what to do here: For a
> > single Rectangle you could use an array and make the entries (x, y, w, h)
> > but what about an array of Rectangles? Should it use an array with 4 * N
> > entries, so (x, y, w, h) for each value?
> 
> The type GstValueArray can nest other GValue types (was needed for
> GstValueFraction). So in theory you can have an array of of N rectangle array
> (size 4). In serialized form it would look like:
> 
>   property="< <x1, y1, w1, h1>, <x2, y2, w2, h2>, ...>"
> 
> The C API that let you assemble/deassemble this is pretty tedious though. We
> usually prefer not to always go through the de-serializer of course. If you look
> around, most of the time these are split in multiple properties, but it also
> means that your control can be inconsistent while its being set, so pretty
> difficult to set at run-time.
> 
> So even though complex, I think its a fair approach since the only other option
> is to use a custom serialization/deserialzation and a string type property.
> 
> > At the moment the gstreamer properties all have zero (or the first enum value)
> > as a default and the minimum and maximum numeric values as minimum and maximum
> > values for numbers. Also all controls are defined as readable and writeable.
> > Because of this (maybe as a discussion) I have a small wish list of things I'd
> > like to see added in the control_ids_*.yaml files which would greatly improve
> > the gstreamer properties:
> > For enum and bool controls: the default value if available.
> 
> While in GStreamer, we often see the implementation differ from the default
> after moving the element to READY state (so we could just read it back later), I
> like the implied consistency that having default (were that actually make sense)
> would bring. I had the same impression where I first looked at it.

The problem is that the default value may be device-specific. For some
enum controls, the set of supported values may even differ between
devices.

> > For numeric controls: the minimum, maximum and default value.

Same problem here, this is device-specific, so we can't provide that.

> > And for all controls: whether it is read-only, write-only or read-write.

There we may be able to provide some information. I would need to check
if a control defined as read-write in the spec could possibly be
implemented as read-only for some devices though.

> > Jaslo Ziska (3):
> >   gstreamer: Remove auto-focus-mode property
> >   gstreamer: Generate controls from control_ids_*.yaml files
> >   gstreamer: Fix missing "greater than" symbol in author string
> 
> I will have a look this week, thanks for working on this.
> 
> Nicolas
> 
> > 
> >  src/gstreamer/gstlibcamera-controls.cpp.in |  46 +++
> >  src/gstreamer/gstlibcamera-controls.h      |  36 ++
> >  src/gstreamer/gstlibcamerasrc.cpp          |  47 +--
> >  src/gstreamer/meson.build                  |  14 +
> >  utils/gen-gst-controls.py                  | 398 +++++++++++++++++++++
> >  utils/meson.build                          |   1 +
> >  6 files changed, 510 insertions(+), 32 deletions(-)
> >  create mode 100644 src/gstreamer/gstlibcamera-controls.cpp.in
> >  create mode 100644 src/gstreamer/gstlibcamera-controls.h
> >  create mode 100755 utils/gen-gst-controls.py

-- 
Regards,

Laurent Pinchart