[libcamera-devel] [PATCH v2] Add enable_auto_focus option to the GStreamer plugin

[Laurent Pinchart]

Hi Nicolas,

On Mon, Jun 05, 2023 at 01:52:41PM -0400, Nicolas Dufresne wrote:
> Le lundi 05 juin 2023 à 19:04 +0300, Laurent Pinchart a écrit
> > > 
> [...]
> 
> > > > Nicolas, is there any particular rule when it comes to naming properties
> > > > for GStreamer elements ?
> > > 
> > > The GLib part of it is:
> > > 
> > >    A property name consists of one or more segments consisting of ASCII letters and
> > >    digits, separated by either the - or _ character. The first character of a
> > >    property name must be a letter. These are the same rules as for signal naming
> > >    (see g_signal_new()).
> > > 
> > > I strongly prefer - instead of _. Generally speaking, I would like this
> > > discussion to happen before we start adding controls. One of the thing I was
> > > considering was to auto-generate the propertly from the yaml,
> > 
> > Seems like we have the same goal :-) I've just replied to your other
> > e-mail suggesting this.
> > 
> > Another option would be to create the mappings dynamically, based on the
> > information reported by the camera at runtime.
> 
> You can't add/remove/modify GObject properties at run-time. The properties are
> considered part of the API and should be stable.
> 
> The alternative would be to generate per camera elements, as used for CODECs,
> but this currently only works for hardwired hardware and would not work well for
> hot-plug capable cameras. This would need lots more infrastructure into
> GStreamer, and for this reason, I would refrain from going that direction.

OK. Then the code generation option is the best option. How should a
control that is not supported by a particular camera be handled ?

I've looked at v4l2src, and as far as I can understand, there's a single
"extra-controls" property that stores a key,value list. Is this a
mechanism you would recommend ?

> > > and clearly this
> > > one is not a 1:1 mapping to the libcamera controls. But at the same time, there
> > > is bunch of needed exception for semantic reasons. The yaml is not descriptive
> > > enough to configure fully the properties unfortunately.
> > 
> > We can extend the yaml file with additional information if needed,
> > and/or extend the information reported by the camera at runtime
> > (assuming GStreamer wouldn't have unreasonable requirements of course
> > :-)).
> 
> Currently known issues are arrays, which can possibly be exposed as lists
> properties, but aren't as nice to use. Based on current usage of arrays, a
> simple mapping like -red/-blue/-red would look nicer. To be discussed.
> 
> Ranges are not all well exposed. I think the idea is that range are per camera,
> but this isn't valid for GObject properties.

Yes, ranges are per camera, and that's not something we can change. The
ranges can even change at runtime depending on the camera configuration,
although I'd like to find a better API than that.

If this doesn't fit the needs of GStreamer then we'll need to find a
different mechanism we can use on the GStreamer side.

> There is probably more, but I cannot remember at the moment.
> 
> > > As its easier to express what I'm hoping for with code, here's my own WIP, which
> > > is much more C++ like then this one. I'm missing the AutoFocusMode.
> > > 
> > > https://gitlab.collabora.com/nicolas/libcamera/-/commit/83d67744ccbd60ad67d2afc70be7f2d95032e4d7
> > 
> > Nice ! Thank you for working on that. I'll try to give it a look ASAP.
> 
> Its a pleasure. Cedric, for your interest, this does not means in anyway we'll
> reject your changes. I think the only changes I'd suggest is to use a enum, and
> expose auto-focus-mode with the 2 modes you currently support. This way we won't
> break our users application as soon as we have these generalized.

-- 
Regards,

Laurent Pinchart