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

[Jaslo Ziska]

Hi Nicolas and Laurent,

thanks for the reviews.

Nicolas Dufresne <nicolas at ndufresne.ca> writes:
> 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.

Just to be certain: you would suggest an array of arrays instead 
of an array of length 4N?

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

Sorry if it wasn't clear in the original text, that is exactly 
what I meant. Just to have limits where it is already written in 
the documentation and limits where the values are just physically 
constrained.

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

I think that's a good idea to get information about the camera 
capabilities at runtime. Should I try to add something like this 
in the next revision?

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

I'm not sure about this, can't most controls theoretically be 
hardware specific? Then most controls would not show up as 
gstreamer properties. In my opinion I would rather have the 
controls show up like normal and then not be able to use them.

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

Best regards,

Jaslo