| Message ID | 20250317181146.214160-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás 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@ideasonboard.com> Do we really want this ? Applications should discover controls programmatically. 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' %} > -- > 2.49.0 >
Hi 2025. 03. 17. 19:54 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > 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@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 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? Regards, Barnabás Pőcze > > 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' %} >> -- >> 2.49.0 >>
Hi Barnabás On Mon, Mar 17, 2025 at 10:00:20PM +0100, Barnabás Pőcze wrote: > Hi > > > 2025. 03. 17. 19:54 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > 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@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. > > > Regards, > Barnabás Pőcze > > > > > > 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' %} > > > -- > > > 2.49.0 > > > >
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@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. > > > 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' %}
Hi Laurent 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@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. 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... 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
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@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. > > 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. > 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' %}
Quoting Laurent Pinchart (2025-03-18 20:43:57) > 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@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. > > > > 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. I think something like this would be helpful to some applications already.. We might always think 'the latest libcamera' is the best to use - but LTS distributions will still exist shipping libcamera 0.3 when we reach later versions - and 'applications' will need a way to still be able to compile against the older versions where possible. > > 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}} This honestly seems pretty 'cheap' and valuable. ... I think it's a good idea ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > {% endfor -%} > > > > > > > > > > > > > > {% if vendor != 'libcamera' %} > > -- > Regards, > > Laurent Pinchart
Hi 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@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. 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. However, this also means that you can potentially get more type safety since you have access to the `Control<...>` object with the type information. 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. >> >> 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. In any case, I just wanted to see if there were any concrete plans or ideas regarding this question, hence the RFC patch. Regards, Barnabás Pőcze > >> 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
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@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@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' %}
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' %}
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@ideasonboard.com> --- include/libcamera/control_ids.h.in | 1 + 1 file changed, 1 insertion(+)