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 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.
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