[RFC PATCH v1] libcamera: controls: Generate macro for each control
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Mon Mar 24 22:15:07 CET 2025
On Thu, Mar 20, 2025 at 11:24:11AM +0100, Barnabás Pőcze wrote:
> 2025. 03. 18. 21:43 keltezéssel, Laurent Pinchart írta:
> > On Tue, Mar 18, 2025 at 06:22:45PM +0100, Jacopo Mondi wrote:
> >> On Tue, Mar 18, 2025 at 05:00:15PM +0200, Laurent Pinchart wrote:
> >>> On Tue, Mar 18, 2025 at 09:11:27AM +0100, Jacopo Mondi wrote:
> >>>> On Mon, Mar 17, 2025 at 10:00:20PM +0100, Barnabás Pőcze wrote:
> >>>>> 2025. 03. 17. 19:54 keltezéssel, Jacopo Mondi írta:
> >>>>>> On Mon, Mar 17, 2025 at 07:11:46PM +0100, Barnabás Pőcze wrote:
> >>>>>>> Generate a macro in the form of LIBCAMERA_HAS_$VENDOR_$MODE_$NAME
> >>>>>>> for each control so that its existence can be checked easily
> >>>>>>> and without extra version checks.
> >>>>>>>
> >>>>>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze at ideasonboard.com>
> >>>>>>
> >>>>>> Do we really want this ?
> >>>>>
> >>>>> Good question, hence the RFC; I don't think there is a very nice
> >>>>> way to discover (and use) controls at comptime/runtime. Also note
> >>>>
> >>>> What would the use case be ?
> >>>>
> >>>>> that the control namespaces already have their own existence macros
> >>>>> (e.g. `LIBCAMERA_HAS_RPI_VENDOR_CONTROLS`).
> >>>>
> >>>>>> Applications should discover controls
> >>>>>> programmatically.
> >>>>>
> >>>>> What do you have in mind? Looking up the numeric id in the appropriate
> >>>>> `ControlIdMap` by the (namespace, name) pair?
> >>>>
> >>>> If you mean Camera::controls() and Camera::properties() then yes, I
> >>>> think applications should enumerate the supported controls from there
> >>>> and not rely on compile-time defines, as even if a control is defined
> >>>> it doesn't mean it is supported by the camera.
> >>>
> >>> True, but an application that would check if Camera::controls()
> >>> contains, for instance, controls::AE_ENABLE with
> >>>
> >>> if (camera->controls().count(controls::AE_ENABLE))
> >>>
> >>> will not compile if the libcamera version it compiles against does not
> >>> define the AeEnable control. Barnabás' patch allows fixing that with
> >>> conditional compilation.
>
> What I had in mind specifically, was something like this:
>
> const libcamera::ControlId *
> find_libcamera_control(std::string_view name, std::string_view vendor = "libcamera")
> {
> for (const auto& [ id, ctrl ] : libcamera::controls::controls) {
> if (ctrl->name() == name && ctrl->vendor() == vendor)
> return ctrl;
> }
> return nullptr;
> }
>
> then during initialization, you can potentially do:
>
> static const auto lc_Gamma = find_libcamera_control("Gamma");
> ...
>
> and then
>
> if (lc_Gamma && camera->controls().count(lc_Gamma->id())) {
> /* libcamera has the Gamma control and the camera supports it */
> }
>
> Then the set of controls supported (only) depends on the runtime version
> of libcamera, not the comptime one.
It's an interesting idea, albeit quite expensive.
> With the proposed macro, the comptime version has to know about the control
> in order for it to be supported in any way, so it admittedly cannot support
> the scenario where the runtime version is upgraded to a new one that supports
> the "new" controls.
If an application is compiled against an older version of libcamera and
run against a newer version, not being able to access newer controls
doesn't seem to be a severe limitation to me.
> However, this also means that you can potentially get
> more type safety since you have access to the `Control<...>` object with
> the type information.
That's one of the features of the Control API that I like :-) It gets
fairly dangerous at runtime otherwise.
> So maybe the best of both worlds could be had by merging the two into something
> like this:
>
> template<typename T>
> const Control<T> *
> findControl(const ControlIdMap& m, std::string_view name, std::string_view vendor = "libcamera")
> {
> for (const auto& [ id, ctrl ] : m) {
> if (ctrl->name() == name && ctrl->vendor() == vendor) {
> if (/* `ctrl` is appropriate for type `T` /*)
> return static_cast<const Control<T> *>(ctrl);
>
> /* warning or something */
> break;
> }
> }
> return nullptr;
> }
>
> and then user code could define the controls they wish to use:
>
> static const auto lc_gamma = libcamera::findControl<float>(libcamera::controls::controls, "Gamma");
> static const auto lc_model = libcamera::findControl<std::string>(libcamera::properties::properties, "Model");
> ...
>
> And then `ControlList` could be change to also accept pointers to `Control<T>`
> and ignore `nullptr`s. Yes, this version requires that the user code repeats
> the type, but not sure if that can reasonably be avoided, and this is still
> most likely better than manually setting up the `ControlValue` instances, etc.
Is it worth it, compared to conditional compilation using the macros in
this patch ? I think we're trying with findControl() to fix a problem
that applications don't really suffer from. If they want to code
something like the above I won't stop them, but making it possible to
check at build time if a control is supported by the API seems better
than forcing each application to use a more complex method. The macros
will likely be even more useful with the plain C API.
> >> Do we really want applications to #ifdef on controls ?? I thought
> >> controls are part of ABI, so once an application knows which version
> >> of libcamera it link against, the list of defined controls should be
> >> fixed. But yes, we don't have ABI stability, so...
> >
> > Version checks are another option, I'm not against that.
>
> If I had to argue, I would say version checks hide the intent more than a
> dedicated mechanism, and they can be caught off guard by back-porting.
I think you're right. I tend to focus on upstream development instead of
backporting, but this patch is clean enough so I don't see a reason to
make backports more painful just for the sake of it :-)
> In any case, I just wanted to see if there were any concrete plans or ideas
> regarding this question, hence the RFC patch.
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >> Not that I'm against this patch, just that seems to encourage bad code
> >> practices in applications.
> >>
> >>>>>> Or is this useful for ABI compliance validation ?
> >>>>>>
> >>>>>>> ---
> >>>>>>> include/libcamera/control_ids.h.in | 1 +
> >>>>>>> 1 file changed, 1 insertion(+)
> >>>>>>>
> >>>>>>> diff --git a/include/libcamera/control_ids.h.in b/include/libcamera/control_ids.h.in
> >>>>>>> index 5d0594c68..8f037589d 100644
> >>>>>>> --- a/include/libcamera/control_ids.h.in
> >>>>>>> +++ b/include/libcamera/control_ids.h.in
> >>>>>>> @@ -49,6 +49,7 @@ extern const std::array<const ControlValue, {{ctrl.enum_values_count}}> {{ctrl.n
> >>>>>>> extern const std::map<std::string, {{ctrl.type}}> {{ctrl.name}}NameValueMap;
> >>>>>>> {% endif -%}
> >>>>>>> extern const Control<{{ctrl.type}}> {{ctrl.name}};
> >>>>>>> +#define LIBCAMERA_HAS_{{vendor|upper}}_{{mode|upper}}_{{ctrl.name|snake_case|upper}}
> >>>>>>> {% endfor -%}
> >>>>>>>
> >>>>>>> {% if vendor != 'libcamera' %}
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list