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

[Nicolas Dufresne]

Hi Laurent,

Le mercredi 07 août 2024 à 19:21 +0300, Laurent Pinchart a écrit :
> 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.

This isn't black and white, some of the controls have the range documented, but
no machine parsable form to apply into GStreamer. For the other, there is common
sense limits that could be applied.

We don't have a run-time mechanism for range in GStreamer properties (well in
GOBject), but we can have a runtime check to provide developers sensible
feedback. If this becomes critical, we could introduce a property description (a
property that provides a description of all the supported controls as seen at
run-time, as I would hope the range is known at runtime considering libcamera
app don't usually have HW specific code). This is fairly easy using a
GstStructure.

If though, you have HW specific controls that clearly don't allow for non-
hardware aware application to use, I would strongly recommend to flag them in
the yaml and skip them in the generator. For these special things, I would
rather add a signal that passes the libcamera object and the request, and let
the application modify it in the way it wants before the request is queued.

> 
> > > 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.

I've sent a breakdown reply using the output of gst-inspect earlier, hope this
will clarify what I mean. One thing to keep in mind, the range is for
documentation and to assist developers, it is not critical and can be changed
later. It is clear that there is down-side of auto-generated controls, as it
gives less room for GStreamer specific instructions to our users. But as I
suggest in my previous reply, we can in the generator have some addition and
quirks.

> 
> > > 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
>