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

[Jaslo Ziska]

Hi Laurent,

Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
> On Wed, Oct 02, 2024 at 07:27:34PM +0200, Jaslo Ziska wrote:
>> Laurent Pinchart <laurent.pinchart at ideasonboard.com> writes:
>> > On Wed, Oct 02, 2024 at 12:04:08PM +0200, Jaslo Ziska wrote:
>> >> Nicolas Dufresne <nicolas at ndufresne.ca> writes:
>> >> > Le lundi 30 septembre 2024 à 11:11 +0200, Jaslo Ziska a 
>> >> > écrit :
>> >> >> Laurent Pinchart <laurent.pinchart at ideasonboard.com> 
>> >> >> writes:
>> >> >> > Hi Jaslo,
>> >> >> > 
>> >> >> > I think this series is close to being ready, and I 
>> >> >> > would be very happy
>> >> >> > to merge this much awaited feature. Do you plan to send 
>> >> >> > a v3 ?
>> >> >> 
>> >> >> Yes, I very much plan on continuing to work on this. I 
>> >> >> was a 
>> >> >> pretty busy the past month but I should have more time 
>> >> >> later in 
>> >> >> October.
>> >> >> 
>> >> >> Unfortunately, I don't think that the series is quite 
>> >> >> ready. 
>> >> >> Nicolas had requested some bigger changes in v1 which 
>> >> >> still need 
>> >> >> to be discussed and implemented and I also want to add 
>> >> >> the 
>> >> >> readable/writeable and minimum/maximum properties to the 
>> >> >> yaml 
>> >> >> files (although this could also be done separately after 
>> >> >> merging).
>> >> >
>> >> > My proposal is to program a subset to be exposed as 
>> >> > properties, the one that are
>> >> > obviously usable and that the documentation make sense. I 
>> >> > think you did a great
>> >> > job on the generator and adding features later to support 
>> >> > controls that have
>> >> > more complex semantic is better then trying to support 
>> >> > them all 
>> >> > in one go today.
>> >> 
>> >> Sounds good. Where would you suggest the list of controls to 
>> >> be 
>> >> exposed should be? Maybe just a plain list in the Python 
>> >> generator 
>> >> file?
>> >
>> > The other option is to store the information in the yaml 
>> > files. I don't
>> > have a very strong opinion, the yaml files have the advantage 
>> > that all
>> > the data related to controls will be in a single place, but 
>> > the
>> > generator script has the advantage of keeping all 
>> > gstreamer-related data
>> > close to the gstreamer element. That may be useful when it 
>> > will be time
>> > to move libcamerasrc from the libcamera repository to the 
>> > gstreamer
>> > repository (if we ever do so). On the other hand, tha yaml 
>> > files are not
>> > installed, so there would still be issues.
>> 
>> Putting it inside the yaml would be an option, but I don't know 
>> whether having GStreamer specific stuff there is so good. But I 
>> think this will likely change anyway.
>> 
>> > Complete brainstorming mode, I'm wondering if at some point 
>> > in the
>> > future it would make sense to handle the gstreamer controls 
>> > in a more
>> > dynamic fashion by querying libcamera at runtime instead of 
>> > using code
>> > generation. This doesn't invalidate the approach you've taken 
>> > here, I
>> > think code generation is good for the time being.
>> 
>> If you mean that the GStreamer properties should get added 
>> later 
>> at runtime then this is unfortunately not possible. This is 
>> because the GStreamer controls are GObject properties and those 
>> need to be installed when the GObject object class is 
>> initialized. 
>> And because this happens before the GStreamer element is 
>> constructed (and therefore also before libcamera gets involved) 
>> it 
>> is not possible to know which properties are present and with 
>> which ranges.
>
> You can get the list of all the controls that libcamera supports 
> from
> libcamera::controls::controls. It's a global ControlIdMap 
> variable, a
> map from control numerical ids to ControlId instances. The 
> ControlId
> class gives you the control numerical id, its name, type, number 
> of
> elements if it's an array, and enumerators. We're adding the 
> control
> namespace as well, and could add additional information if 
> needed. We
> don't need to go this way though.

Ah yes, I think that would work too, I never thought about it that 
way. This still would not help with the actually available 
controls for the camera unfortunately.

I don't know which way would be the proper or better way to do it, 
both ways have their pros and cons.

Using the yaml files and the code generator has more flexibility 
but adds the complexity of the extra code. And using the 
libcamera::controls::controls variable would probably simplify the 
code but makes the GStreamer element dependent on the information 
which are exposed by the variable.

I'm open to both possibilities. Does anyone have opinions on this?

Best regards,

Jaslo

>> The one thing that Nicolas suggested is adding a special 
>> property 
>> to the GStreamer element which can then return the available 
>> properties and their ranges at runtime. That would be the 
>> GstStructure which I was asking about.
>> 
>> >> > If the selection is all rw, the marking of that can wait. 
>> >> > Min/max would be my
>> >> > preference to have, but as we are breaking the API anyway 
>> >> > still, we can also do
>> >> > small break in the gstreamer element later too (as long as 
>> >> > its well reported in
>> >> > the release notes).
>> >> 
>> >> You also suggested a GStreamer property that returns a 
>> >> GstStructure which lists all the actually present controls 
>> >> for 
>> >> the 
>> >> camera and their actual limits. Do you still want this 
>> >> implemented 
>> >> and if so how do you imagine this structure to be laid out?
>> >> 
>> >> >> > On Tue, Aug 13, 2024 at 02:25:04PM +0200, Jaslo Ziska 
>> >> >> > wrote:
>> >> >> > > Hi everyone,
>> >> >> > > 
>> >> >> > > this is the second version of the patch set to 
>> >> >> > > implement 
>> >> >> > > gstreamer controls
>> >> >> > > from the yaml files.
>> >> >> > > This now depends on "[PATCH 00/10] libcamera: Improve 
>> >> >> > > code generation for
>> >> >> > > controls" for the code generation.
>> >> >> > > 
>> >> >> > > The following things changed:
>> >> >> > > 
>> >> >> > > The (old) third commit fixing a typo has been removed 
>> >> >> > > as 
>> >> >> > > it is already merged.
>> >> >> > > 
>> >> >> > > The first commit is new: it removes the 
>> >> >> > > auto-focus-mode 
>> >> >> > > property from the
>> >> >> > > device provider where it has been added on accident.
>> >> >> > > 
>> >> >> > > The commit message has been updated in the second 
>> >> >> > > commit.
>> >> >> > > 
>> >> >> > > The third commit, which adds the code that generates 
>> >> >> > > the 
>> >> >> > > gstreamer controls,
>> >> >> > > now makes use of jinja2 and the new controls.py file. 
>> >> >> > > As 
>> >> >> > > a consequence the
>> >> >> > > whole code generation has been redone.
>> >> >> > > 
>> >> >> > > There are also some new features:
>> >> >> > > 
>> >> >> > > It is now possible to read the metadata returned by 
>> >> >> > > requests from the gstreamer
>> >> >> > > properties. This is done using a new function 
>> >> >> > > readMetadata() which reads the
>> >> >> > > ControlList in requestCompleted().
>> >> >> > > 
>> >> >> > > Before a control is set it is now checked whether 
>> >> >> > > this 
>> >> >> > > control is actually
>> >> >> > > supported by the camera. This is done by checking the 
>> >> >> > > cameras ControlInfoMap.
>> >> >> > > 
>> >> >> > > The Rectangle type is now supported.
>> >> >> > > 
>> >> >> > > Some checks were added to make sure the arrays passed 
>> >> >> > > to 
>> >> >> > > the element have the
>> >> >> > > correct length.
>> >> >> > > 
>> >> >> > > Best regards,
>> >> >> > > 
>> >> >> > > Jaslo
>> >> >> > > 
>> >> >> > > Jaslo Ziska (3):
>> >> >> > >   gstreamer: Remove auto-focus-mode property from 
>> >> >> > >   device provider
>> >> >> > >   gstreamer: Remove auto-focus-mode property from 
>> >> >> > >   libcamerasrc
>> >> >> > >   gstreamer: Generate controls from 
>> >> >> > >   control_ids_*.yaml files
>> >> >> > > 
>> >> >> > >  src/gstreamer/gstlibcamera-controls.cpp.in | 296 
>> >> >> > >  +++++++++++++++++++++
>> >> >> > >  src/gstreamer/gstlibcamera-controls.h      |  43 +++
>> >> >> > >  src/gstreamer/gstlibcameraprovider.cpp     |  15 --
>> >> >> > >  src/gstreamer/gstlibcamerasrc.cpp          |  50 
>> >> >> > >  ++--
>> >> >> > >  src/gstreamer/meson.build                  |  10 +
>> >> >> > >  utils/codegen/controls.py                  |   8 +
>> >> >> > >  utils/codegen/gen-gst-controls.py          | 151 
>> >> >> > >  +++++++++++
>> >> >> > >  utils/codegen/meson.build                  |   1 +
>> >> >> > >  8 files changed, 528 insertions(+), 46 deletions(-)
>> >> >> > >  create mode 100644 
>> >> >> > >  src/gstreamer/gstlibcamera-controls.cpp.in
>> >> >> > >  create mode 100644 
>> >> >> > >  src/gstreamer/gstlibcamera-controls.h
>> >> >> > >  create mode 100755 utils/codegen/gen-gst-controls.py