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

[Nicolas Dufresne]

Hi,

Le jeudi 03 octobre 2024 à 15:10 +0300, Laurent Pinchart a écrit :
> On Thu, Oct 03, 2024 at 12:18:54PM +0200, Jaslo Ziska wrote:
> > 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.
> 
> One thing missing in ControlId (beside the namespace that we will add
> soon) that you would need for libcamerasrc is documentation. I'm not too
> keen on adding that to ControlId, it's lots of text, which would
> probably then need localization.

We have stopped localization of non-user facing text in GStreamer. That means
the documentation (including properties, signal, enums description), debug
traces and internal errors are not translated.

Nicolas

> 
> > I'm open to both possibilities. Does anyone have opinions on this?
> 
> I think it's more important at this point to have a working version than
> a perfect version, so I'm fine continuing with the current approach.
> 
> > > > 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
>