| Message ID | 20250721104622.1550908-20-barnabas.pocze@ideasonboard.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: > Inject all metadata controls in the "debug" namespace into > the metadata plan of each camera so that they can be used > seamlessly. Dynamically sized array-like controls have a > hard-coded size of 32 elements. > > Additionally, a new type is added for inspecting properties of > a control type at runtime since that was not available previously. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > --- > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ > src/libcamera/controls.cpp | 2 +- > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ > 3 files changed, 85 insertions(+), 1 deletion(-) > create mode 100644 include/libcamera/internal/controls.h > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h > new file mode 100644 > index 000000000..be3f93e43 > --- /dev/null > +++ b/include/libcamera/internal/controls.h > @@ -0,0 +1,39 @@ > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > +/* > + * Copyright (C) 2025, Ideas on Board Oy > + */ > + > +#pragma once > + > +#include <libcamera/controls.h> > + > +namespace libcamera::controls::details { > + > +struct TypeInfo { > + std::size_t size = 0; > + std::size_t alignment = 0; > + > + explicit operator bool() const { return alignment != 0; } > + > + static constexpr TypeInfo get(ControlType t) > + { > + switch (t) { > + case ControlTypeNone: return {}; > + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; > + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; > + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; > + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; > + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; > + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; > + case ControlTypeFloat: return { sizeof(float), alignof(float) }; > + case ControlTypeString: return { sizeof(char), alignof(char) }; > + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; > + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; > + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; > + } As suggested in a previous review can these information be added to control_type ? Would template<> struct control_type<bool> { static constexpr ControlType value = ControlTypeBool; static constexpr std::size_t size = 0; static constexpr std::size_t element_size = sizeof(bool); static constexpr std::size_t alignment = alignof(bool); }; Work ? > + > + return {}; > + } > +}; > + > +} /* libcamera::controls::details */ > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > index a238141a5..d5a86da5e 100644 > --- a/src/libcamera/controls.cpp > +++ b/src/libcamera/controls.cpp > @@ -17,7 +17,7 @@ > #include "libcamera/internal/control_validator.h" > > /** > - * \file controls.h > + * \file libcamera/controls.h Unrelated ? > * \brief Framework to manage controls related to an object > * > * A control is a mean to govern or influence the operation of an object, and in > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > index edfa9cf58..c8ac7a673 100644 > --- a/src/libcamera/pipeline_handler.cpp > +++ b/src/libcamera/pipeline_handler.cpp > @@ -16,11 +16,13 @@ > #include <libcamera/base/utils.h> > > #include <libcamera/camera.h> > +#include <libcamera/control_ids.h> > #include <libcamera/framebuffer.h> > #include <libcamera/property_ids.h> > > #include "libcamera/internal/camera.h" > #include "libcamera/internal/camera_manager.h" > +#include "libcamera/internal/controls.h" > #include "libcamera/internal/device_enumerator.h" > #include "libcamera/internal/media_device.h" > #include "libcamera/internal/request.h" > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, > return std::string(); > } > > +namespace { > + > +/* > + * This is kind of hack. The metadata controls in the "debug" namespace I know, but I think for the time being is the best we can do :( > + * are forcefully injected into each Camera's MetadataListPlan so that > + * they work seamlessly without any additional setup. > + * > + * The dynamically-sized array-like controls have a maximum capacity > + * determined by the magic number below. > + */ > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) > +{ > + constexpr std::size_t kDynamicArrayCapacity = 32; > + > + for (const auto &[id, ctrl] : controls::controls) { > + if (!ctrl->isOutput()) > + continue; > + if (ctrl->vendor() != "debug") > + continue; > + if (mlp.get(id)) > + continue; > + > + std::size_t count = ctrl->size(); > + if (count == 0) > + count = 1; Why this ? Do you mind a comment unless is something trivial I have missed ? > + else if (ctrl->size() == libcamera::dynamic_extent) > + count = kDynamicArrayCapacity; > + > + const auto info = controls::details::TypeInfo::get(ctrl->type()); > + if (!info) > + continue; > + > + [[maybe_unused]] bool ok = mlp.set(id, > + info.size, info.alignment, > + count, ctrl->type(), ctrl->isArray()); > + ASSERT(ok); > + } > +} > + > +} /* namespace */ > + > /** > * \brief Register a camera to the camera manager and pipeline handler > * \param[in] camera The camera to be added > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) > Camera::Private *data = camera->_d(); > data->properties_.set(properties::SystemDevices, devnums); > > + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); > + Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > manager_->_d()->addCamera(std::move(camera)); > } > > -- > 2.50.1 >
Hi 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: >> Inject all metadata controls in the "debug" namespace into >> the metadata plan of each camera so that they can be used >> seamlessly. Dynamically sized array-like controls have a >> hard-coded size of 32 elements. >> >> Additionally, a new type is added for inspecting properties of >> a control type at runtime since that was not available previously. >> >> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >> --- >> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ >> src/libcamera/controls.cpp | 2 +- >> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ >> 3 files changed, 85 insertions(+), 1 deletion(-) >> create mode 100644 include/libcamera/internal/controls.h >> >> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h >> new file mode 100644 >> index 000000000..be3f93e43 >> --- /dev/null >> +++ b/include/libcamera/internal/controls.h >> @@ -0,0 +1,39 @@ >> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >> +/* >> + * Copyright (C) 2025, Ideas on Board Oy >> + */ >> + >> +#pragma once >> + >> +#include <libcamera/controls.h> >> + >> +namespace libcamera::controls::details { >> + >> +struct TypeInfo { >> + std::size_t size = 0; >> + std::size_t alignment = 0; >> + >> + explicit operator bool() const { return alignment != 0; } >> + >> + static constexpr TypeInfo get(ControlType t) >> + { >> + switch (t) { >> + case ControlTypeNone: return {}; >> + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; >> + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; >> + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; >> + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; >> + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; >> + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; >> + case ControlTypeFloat: return { sizeof(float), alignof(float) }; >> + case ControlTypeString: return { sizeof(char), alignof(char) }; >> + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; >> + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; >> + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; >> + } > > As suggested in a previous review can these information be added to > control_type ? > > Would > > template<> > struct control_type<bool> { > static constexpr ControlType value = ControlTypeBool; > static constexpr std::size_t size = 0; > static constexpr std::size_t element_size = sizeof(bool); > static constexpr std::size_t alignment = alignof(bool); > }; > > Work ? That could be useful yes, but you still need a function like the above to map the "runtime" type to the appropriate values. > >> + >> + return {}; >> + } >> +}; >> + >> +} /* libcamera::controls::details */ >> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >> index a238141a5..d5a86da5e 100644 >> --- a/src/libcamera/controls.cpp >> +++ b/src/libcamera/controls.cpp >> @@ -17,7 +17,7 @@ >> #include "libcamera/internal/control_validator.h" >> >> /** >> - * \file controls.h >> + * \file libcamera/controls.h > > Unrelated ? I can move it to a separate commit but it is needed. Because now we have two `controls.h`, and otherwise doxygen will complain. > >> * \brief Framework to manage controls related to an object >> * >> * A control is a mean to govern or influence the operation of an object, and in >> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >> index edfa9cf58..c8ac7a673 100644 >> --- a/src/libcamera/pipeline_handler.cpp >> +++ b/src/libcamera/pipeline_handler.cpp >> @@ -16,11 +16,13 @@ >> #include <libcamera/base/utils.h> >> >> #include <libcamera/camera.h> >> +#include <libcamera/control_ids.h> >> #include <libcamera/framebuffer.h> >> #include <libcamera/property_ids.h> >> >> #include "libcamera/internal/camera.h" >> #include "libcamera/internal/camera_manager.h" >> +#include "libcamera/internal/controls.h" >> #include "libcamera/internal/device_enumerator.h" >> #include "libcamera/internal/media_device.h" >> #include "libcamera/internal/request.h" >> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, >> return std::string(); >> } >> >> +namespace { >> + >> +/* >> + * This is kind of hack. The metadata controls in the "debug" namespace > > I know, but I think for the time being is the best we can do :( > >> + * are forcefully injected into each Camera's MetadataListPlan so that >> + * they work seamlessly without any additional setup. >> + * >> + * The dynamically-sized array-like controls have a maximum capacity >> + * determined by the magic number below. >> + */ >> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) >> +{ >> + constexpr std::size_t kDynamicArrayCapacity = 32; >> + >> + for (const auto &[id, ctrl] : controls::controls) { >> + if (!ctrl->isOutput()) >> + continue; >> + if (ctrl->vendor() != "debug") >> + continue; >> + if (mlp.get(id)) >> + continue; >> + >> + std::size_t count = ctrl->size(); >> + if (count == 0) >> + count = 1; > > Why this ? Do you mind a comment unless is something trivial I have > missed ? Non-array controls have their static size set to 0. See `control_type<...>::size`. I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` could be modified somehow to make this clearer. Regards, Barnabás Pőcze > >> + else if (ctrl->size() == libcamera::dynamic_extent) >> + count = kDynamicArrayCapacity; >> + >> + const auto info = controls::details::TypeInfo::get(ctrl->type()); >> + if (!info) >> + continue; >> + >> + [[maybe_unused]] bool ok = mlp.set(id, >> + info.size, info.alignment, >> + count, ctrl->type(), ctrl->isArray()); >> + ASSERT(ok); >> + } >> +} >> + >> +} /* namespace */ >> + >> /** >> * \brief Register a camera to the camera manager and pipeline handler >> * \param[in] camera The camera to be added >> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) >> Camera::Private *data = camera->_d(); >> data->properties_.set(properties::SystemDevices, devnums); >> >> + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); >> + > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Thanks > j > >> manager_->_d()->addCamera(std::move(camera)); >> } >> >> -- >> 2.50.1 >>
Hi Barnabás On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: > > > Inject all metadata controls in the "debug" namespace into > > > the metadata plan of each camera so that they can be used > > > seamlessly. Dynamically sized array-like controls have a > > > hard-coded size of 32 elements. > > > > > > Additionally, a new type is added for inspecting properties of > > > a control type at runtime since that was not available previously. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > --- > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ > > > src/libcamera/controls.cpp | 2 +- > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ > > > 3 files changed, 85 insertions(+), 1 deletion(-) > > > create mode 100644 include/libcamera/internal/controls.h > > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h > > > new file mode 100644 > > > index 000000000..be3f93e43 > > > --- /dev/null > > > +++ b/include/libcamera/internal/controls.h > > > @@ -0,0 +1,39 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > +/* > > > + * Copyright (C) 2025, Ideas on Board Oy > > > + */ > > > + > > > +#pragma once > > > + > > > +#include <libcamera/controls.h> > > > + > > > +namespace libcamera::controls::details { > > > + > > > +struct TypeInfo { > > > + std::size_t size = 0; > > > + std::size_t alignment = 0; > > > + > > > + explicit operator bool() const { return alignment != 0; } > > > + > > > + static constexpr TypeInfo get(ControlType t) > > > + { > > > + switch (t) { > > > + case ControlTypeNone: return {}; > > > + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; > > > + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; > > > + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; > > > + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; > > > + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; > > > + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; > > > + case ControlTypeFloat: return { sizeof(float), alignof(float) }; > > > + case ControlTypeString: return { sizeof(char), alignof(char) }; > > > + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; > > > + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; > > > + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; > > > + } > > > > As suggested in a previous review can these information be added to > > control_type ? > > > > Would > > > > template<> > > struct control_type<bool> { > > static constexpr ControlType value = ControlTypeBool; > > static constexpr std::size_t size = 0; > > static constexpr std::size_t element_size = sizeof(bool); > > static constexpr std::size_t alignment = alignof(bool); > > }; > > > > Work ? > > That could be useful yes, but you still need a function like the above to map > the "runtime" type to the appropriate values. If we use ctrl->type() yes, we need a run-time switch. I presume there is no easy way to go from Control<T> to control_info<T> when iterating over the controls::controls id map.. > > > > > > > > + > > > + return {}; > > > + } > > > +}; > > > + > > > +} /* libcamera::controls::details */ > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > index a238141a5..d5a86da5e 100644 > > > --- a/src/libcamera/controls.cpp > > > +++ b/src/libcamera/controls.cpp > > > @@ -17,7 +17,7 @@ > > > #include "libcamera/internal/control_validator.h" > > > > > > /** > > > - * \file controls.h > > > + * \file libcamera/controls.h > > > > Unrelated ? > > I can move it to a separate commit but it is needed. Because now we have > two `controls.h`, and otherwise doxygen will complain. > Ah sorry, I missed the reason and I thought it was just a leftover > > > > > > * \brief Framework to manage controls related to an object > > > * > > > * A control is a mean to govern or influence the operation of an object, and in > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > index edfa9cf58..c8ac7a673 100644 > > > --- a/src/libcamera/pipeline_handler.cpp > > > +++ b/src/libcamera/pipeline_handler.cpp > > > @@ -16,11 +16,13 @@ > > > #include <libcamera/base/utils.h> > > > > > > #include <libcamera/camera.h> > > > +#include <libcamera/control_ids.h> > > > #include <libcamera/framebuffer.h> > > > #include <libcamera/property_ids.h> > > > > > > #include "libcamera/internal/camera.h" > > > #include "libcamera/internal/camera_manager.h" > > > +#include "libcamera/internal/controls.h" > > > #include "libcamera/internal/device_enumerator.h" > > > #include "libcamera/internal/media_device.h" > > > #include "libcamera/internal/request.h" > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, > > > return std::string(); > > > } > > > > > > +namespace { > > > + > > > +/* > > > + * This is kind of hack. The metadata controls in the "debug" namespace > > > > I know, but I think for the time being is the best we can do :( > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that > > > + * they work seamlessly without any additional setup. > > > + * > > > + * The dynamically-sized array-like controls have a maximum capacity > > > + * determined by the magic number below. > > > + */ > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) > > > +{ > > > + constexpr std::size_t kDynamicArrayCapacity = 32; > > > + > > > + for (const auto &[id, ctrl] : controls::controls) { > > > + if (!ctrl->isOutput()) > > > + continue; > > > + if (ctrl->vendor() != "debug") > > > + continue; > > > + if (mlp.get(id)) > > > + continue; > > > + > > > + std::size_t count = ctrl->size(); > > > + if (count == 0) > > > + count = 1; > > > > Why this ? Do you mind a comment unless is something trivial I have > > missed ? > > Non-array controls have their static size set to 0. See `control_type<...>::size`. > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` > could be modified somehow to make this clearer. ack, a small comment would make it clear > > > Regards, > Barnabás Pőcze > > > > > > > + else if (ctrl->size() == libcamera::dynamic_extent) > > > + count = kDynamicArrayCapacity; > > > + > > > + const auto info = controls::details::TypeInfo::get(ctrl->type()); > > > + if (!info) > > > + continue; > > > + > > > + [[maybe_unused]] bool ok = mlp.set(id, > > > + info.size, info.alignment, > > > + count, ctrl->type(), ctrl->isArray()); > > > + ASSERT(ok); > > > + } > > > +} > > > + > > > +} /* namespace */ > > > + > > > /** > > > * \brief Register a camera to the camera manager and pipeline handler > > > * \param[in] camera The camera to be added > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) > > > Camera::Private *data = camera->_d(); > > > data->properties_.set(properties::SystemDevices, devnums); > > > > > > + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); > > > + > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Thanks > > j > > > > > manager_->_d()->addCamera(std::move(camera)); > > > } > > > > > > -- > > > 2.50.1 > > > >
Hi 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta: > Hi Barnabás > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote: >> Hi >> >> 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: >>> Hi Barnabás >>> >>> On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: >>>> Inject all metadata controls in the "debug" namespace into >>>> the metadata plan of each camera so that they can be used >>>> seamlessly. Dynamically sized array-like controls have a >>>> hard-coded size of 32 elements. >>>> >>>> Additionally, a new type is added for inspecting properties of >>>> a control type at runtime since that was not available previously. >>>> >>>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> >>>> --- >>>> include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ >>>> src/libcamera/controls.cpp | 2 +- >>>> src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ >>>> 3 files changed, 85 insertions(+), 1 deletion(-) >>>> create mode 100644 include/libcamera/internal/controls.h >>>> >>>> diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h >>>> new file mode 100644 >>>> index 000000000..be3f93e43 >>>> --- /dev/null >>>> +++ b/include/libcamera/internal/controls.h >>>> @@ -0,0 +1,39 @@ >>>> +/* SPDX-License-Identifier: LGPL-2.1-or-later */ >>>> +/* >>>> + * Copyright (C) 2025, Ideas on Board Oy >>>> + */ >>>> + >>>> +#pragma once >>>> + >>>> +#include <libcamera/controls.h> >>>> + >>>> +namespace libcamera::controls::details { >>>> + >>>> +struct TypeInfo { >>>> + std::size_t size = 0; >>>> + std::size_t alignment = 0; >>>> + >>>> + explicit operator bool() const { return alignment != 0; } >>>> + >>>> + static constexpr TypeInfo get(ControlType t) >>>> + { >>>> + switch (t) { >>>> + case ControlTypeNone: return {}; >>>> + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; >>>> + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; >>>> + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; >>>> + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; >>>> + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; >>>> + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; >>>> + case ControlTypeFloat: return { sizeof(float), alignof(float) }; >>>> + case ControlTypeString: return { sizeof(char), alignof(char) }; >>>> + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; >>>> + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; >>>> + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; >>>> + } >>> >>> As suggested in a previous review can these information be added to >>> control_type ? >>> >>> Would >>> >>> template<> >>> struct control_type<bool> { >>> static constexpr ControlType value = ControlTypeBool; >>> static constexpr std::size_t size = 0; >>> static constexpr std::size_t element_size = sizeof(bool); >>> static constexpr std::size_t alignment = alignof(bool); >>> }; >>> >>> Work ? >> >> That could be useful yes, but you still need a function like the above to map >> the "runtime" type to the appropriate values. > > If we use ctrl->type() yes, we need a run-time switch. > > I presume there is no easy way to go from Control<T> to > control_info<T> when iterating over the controls::controls id map.. > Only a `ControlId` is available, so there is no static type information available. Some kind of runtime switch is needed. This seemed like the simplest method. >> >> >> >>> >>>> + >>>> + return {}; >>>> + } >>>> +}; >>>> + >>>> +} /* libcamera::controls::details */ >>>> diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp >>>> index a238141a5..d5a86da5e 100644 >>>> --- a/src/libcamera/controls.cpp >>>> +++ b/src/libcamera/controls.cpp >>>> @@ -17,7 +17,7 @@ >>>> #include "libcamera/internal/control_validator.h" >>>> >>>> /** >>>> - * \file controls.h >>>> + * \file libcamera/controls.h >>> >>> Unrelated ? >> >> I can move it to a separate commit but it is needed. Because now we have >> two `controls.h`, and otherwise doxygen will complain. >> > > Ah sorry, I missed the reason and I thought it was just a leftover > >> >>> >>>> * \brief Framework to manage controls related to an object >>>> * >>>> * A control is a mean to govern or influence the operation of an object, and in >>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp >>>> index edfa9cf58..c8ac7a673 100644 >>>> --- a/src/libcamera/pipeline_handler.cpp >>>> +++ b/src/libcamera/pipeline_handler.cpp >>>> @@ -16,11 +16,13 @@ >>>> #include <libcamera/base/utils.h> >>>> >>>> #include <libcamera/camera.h> >>>> +#include <libcamera/control_ids.h> >>>> #include <libcamera/framebuffer.h> >>>> #include <libcamera/property_ids.h> >>>> >>>> #include "libcamera/internal/camera.h" >>>> #include "libcamera/internal/camera_manager.h" >>>> +#include "libcamera/internal/controls.h" >>>> #include "libcamera/internal/device_enumerator.h" >>>> #include "libcamera/internal/media_device.h" >>>> #include "libcamera/internal/request.h" >>>> @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, >>>> return std::string(); >>>> } >>>> >>>> +namespace { >>>> + >>>> +/* >>>> + * This is kind of hack. The metadata controls in the "debug" namespace >>> >>> I know, but I think for the time being is the best we can do :( >>> >>>> + * are forcefully injected into each Camera's MetadataListPlan so that >>>> + * they work seamlessly without any additional setup. >>>> + * >>>> + * The dynamically-sized array-like controls have a maximum capacity >>>> + * determined by the magic number below. >>>> + */ >>>> +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) >>>> +{ >>>> + constexpr std::size_t kDynamicArrayCapacity = 32; >>>> + >>>> + for (const auto &[id, ctrl] : controls::controls) { >>>> + if (!ctrl->isOutput()) >>>> + continue; >>>> + if (ctrl->vendor() != "debug") >>>> + continue; >>>> + if (mlp.get(id)) >>>> + continue; >>>> + >>>> + std::size_t count = ctrl->size(); >>>> + if (count == 0) >>>> + count = 1; >>> >>> Why this ? Do you mind a comment unless is something trivial I have >>> missed ? >> >> Non-array controls have their static size set to 0. See `control_type<...>::size`. >> I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` >> could be modified somehow to make this clearer. > > ack, a small comment would make it clear done Regards, Barnabás Pőcze > >> >> >> Regards, >> Barnabás Pőcze >> >> >>> >>>> + else if (ctrl->size() == libcamera::dynamic_extent) >>>> + count = kDynamicArrayCapacity; >>>> + >>>> + const auto info = controls::details::TypeInfo::get(ctrl->type()); >>>> + if (!info) >>>> + continue; >>>> + >>>> + [[maybe_unused]] bool ok = mlp.set(id, >>>> + info.size, info.alignment, >>>> + count, ctrl->type(), ctrl->isArray()); >>>> + ASSERT(ok); >>>> + } >>>> +} >>>> + >>>> +} /* namespace */ >>>> + >>>> /** >>>> * \brief Register a camera to the camera manager and pipeline handler >>>> * \param[in] camera The camera to be added >>>> @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) >>>> Camera::Private *data = camera->_d(); >>>> data->properties_.set(properties::SystemDevices, devnums); >>>> >>>> + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); >>>> + >>> >>> Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> >>> >>> Thanks >>> j >>> >>>> manager_->_d()->addCamera(std::move(camera)); >>>> } >>>> >>>> -- >>>> 2.50.1 >>>> >>
Hi Barnabás On Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote: > Hi > > 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta: > > Hi Barnabás > > > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote: > > > Hi > > > > > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: > > > > Hi Barnabás > > > > > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: > > > > > Inject all metadata controls in the "debug" namespace into > > > > > the metadata plan of each camera so that they can be used > > > > > seamlessly. Dynamically sized array-like controls have a > > > > > hard-coded size of 32 elements. > > > > > > > > > > Additionally, a new type is added for inspecting properties of > > > > > a control type at runtime since that was not available previously. > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > --- > > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ > > > > > src/libcamera/controls.cpp | 2 +- > > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ > > > > > 3 files changed, 85 insertions(+), 1 deletion(-) > > > > > create mode 100644 include/libcamera/internal/controls.h > > > > > > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h > > > > > new file mode 100644 > > > > > index 000000000..be3f93e43 > > > > > --- /dev/null > > > > > +++ b/include/libcamera/internal/controls.h > > > > > @@ -0,0 +1,39 @@ > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > +/* > > > > > + * Copyright (C) 2025, Ideas on Board Oy > > > > > + */ > > > > > + > > > > > +#pragma once > > > > > + > > > > > +#include <libcamera/controls.h> > > > > > + > > > > > +namespace libcamera::controls::details { > > > > > + > > > > > +struct TypeInfo { > > > > > + std::size_t size = 0; > > > > > + std::size_t alignment = 0; > > > > > + > > > > > + explicit operator bool() const { return alignment != 0; } > > > > > + > > > > > + static constexpr TypeInfo get(ControlType t) > > > > > + { > > > > > + switch (t) { > > > > > + case ControlTypeNone: return {}; > > > > > + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; > > > > > + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; > > > > > + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; > > > > > + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; > > > > > + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; > > > > > + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; > > > > > + case ControlTypeFloat: return { sizeof(float), alignof(float) }; > > > > > + case ControlTypeString: return { sizeof(char), alignof(char) }; > > > > > + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; > > > > > + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; > > > > > + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; > > > > > + } > > > > > > > > As suggested in a previous review can these information be added to > > > > control_type ? > > > > > > > > Would > > > > > > > > template<> > > > > struct control_type<bool> { > > > > static constexpr ControlType value = ControlTypeBool; > > > > static constexpr std::size_t size = 0; > > > > static constexpr std::size_t element_size = sizeof(bool); > > > > static constexpr std::size_t alignment = alignof(bool); > > > > }; > > > > > > > > Work ? > > > > > > That could be useful yes, but you still need a function like the above to map > > > the "runtime" type to the appropriate values. > > > > If we use ctrl->type() yes, we need a run-time switch. > > > > I presume there is no easy way to go from Control<T> to > > control_info<T> when iterating over the controls::controls id map.. > > > > Only a `ControlId` is available, so there is no static type information > available. Some kind of runtime switch is needed. This seemed like the > simplest method. Indeed. Do you think it would make sense to centralize the information in the existing control_type<T> and add a function similar in spirit to your above TypeInfo::get() to controls.h or would you prefer keeping them separate ? > > > > > > > > > > > > > > > > > > > > + > > > > > + return {}; > > > > > + } > > > > > +}; > > > > > + > > > > > +} /* libcamera::controls::details */ > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > index a238141a5..d5a86da5e 100644 > > > > > --- a/src/libcamera/controls.cpp > > > > > +++ b/src/libcamera/controls.cpp > > > > > @@ -17,7 +17,7 @@ > > > > > #include "libcamera/internal/control_validator.h" > > > > > > > > > > /** > > > > > - * \file controls.h > > > > > + * \file libcamera/controls.h > > > > > > > > Unrelated ? > > > > > > I can move it to a separate commit but it is needed. Because now we have > > > two `controls.h`, and otherwise doxygen will complain. > > > > > > > Ah sorry, I missed the reason and I thought it was just a leftover > > > > > > > > > > > > > > * \brief Framework to manage controls related to an object > > > > > * > > > > > * A control is a mean to govern or influence the operation of an object, and in > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > index edfa9cf58..c8ac7a673 100644 > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > @@ -16,11 +16,13 @@ > > > > > #include <libcamera/base/utils.h> > > > > > > > > > > #include <libcamera/camera.h> > > > > > +#include <libcamera/control_ids.h> > > > > > #include <libcamera/framebuffer.h> > > > > > #include <libcamera/property_ids.h> > > > > > > > > > > #include "libcamera/internal/camera.h" > > > > > #include "libcamera/internal/camera_manager.h" > > > > > +#include "libcamera/internal/controls.h" > > > > > #include "libcamera/internal/device_enumerator.h" > > > > > #include "libcamera/internal/media_device.h" > > > > > #include "libcamera/internal/request.h" > > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, > > > > > return std::string(); > > > > > } > > > > > > > > > > +namespace { > > > > > + > > > > > +/* > > > > > + * This is kind of hack. The metadata controls in the "debug" namespace > > > > > > > > I know, but I think for the time being is the best we can do :( > > > > > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that > > > > > + * they work seamlessly without any additional setup. > > > > > + * > > > > > + * The dynamically-sized array-like controls have a maximum capacity > > > > > + * determined by the magic number below. > > > > > + */ > > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) > > > > > +{ > > > > > + constexpr std::size_t kDynamicArrayCapacity = 32; > > > > > + > > > > > + for (const auto &[id, ctrl] : controls::controls) { > > > > > + if (!ctrl->isOutput()) > > > > > + continue; > > > > > + if (ctrl->vendor() != "debug") > > > > > + continue; > > > > > + if (mlp.get(id)) > > > > > + continue; > > > > > + > > > > > + std::size_t count = ctrl->size(); > > > > > + if (count == 0) > > > > > + count = 1; > > > > > > > > Why this ? Do you mind a comment unless is something trivial I have > > > > missed ? > > > > > > Non-array controls have their static size set to 0. See `control_type<...>::size`. > > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` > > > could be modified somehow to make this clearer. > > > > ack, a small comment would make it clear > > done > > > Regards, > Barnabás Pőcze > > > > > > > > > > > > > Regards, > > > Barnabás Pőcze > > > > > > > > > > > > > > > + else if (ctrl->size() == libcamera::dynamic_extent) > > > > > + count = kDynamicArrayCapacity; > > > > > + > > > > > + const auto info = controls::details::TypeInfo::get(ctrl->type()); > > > > > + if (!info) > > > > > + continue; > > > > > + > > > > > + [[maybe_unused]] bool ok = mlp.set(id, > > > > > + info.size, info.alignment, > > > > > + count, ctrl->type(), ctrl->isArray()); > > > > > + ASSERT(ok); > > > > > + } > > > > > +} > > > > > + > > > > > +} /* namespace */ > > > > > + > > > > > /** > > > > > * \brief Register a camera to the camera manager and pipeline handler > > > > > * \param[in] camera The camera to be added > > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) > > > > > Camera::Private *data = camera->_d(); > > > > > data->properties_.set(properties::SystemDevices, devnums); > > > > > > > > > > + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); > > > > > + > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > Thanks > > > > j > > > > > > > > > manager_->_d()->addCamera(std::move(camera)); > > > > > } > > > > > > > > > > -- > > > > > 2.50.1 > > > > > > > > >
On Sat, Aug 02, 2025 at 12:36:40PM +0200, Jacopo Mondi wrote: > Hi Barnabás > > On Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote: > > Hi > > > > 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta: > > > Hi Barnabás > > > > > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote: > > > > Hi > > > > > > > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: > > > > > Hi Barnabás > > > > > > > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: > > > > > > Inject all metadata controls in the "debug" namespace into > > > > > > the metadata plan of each camera so that they can be used > > > > > > seamlessly. Dynamically sized array-like controls have a > > > > > > hard-coded size of 32 elements. > > > > > > > > > > > > Additionally, a new type is added for inspecting properties of > > > > > > a control type at runtime since that was not available previously. > > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > --- > > > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ > > > > > > src/libcamera/controls.cpp | 2 +- > > > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ > > > > > > 3 files changed, 85 insertions(+), 1 deletion(-) > > > > > > create mode 100644 include/libcamera/internal/controls.h > > > > > > > > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h > > > > > > new file mode 100644 > > > > > > index 000000000..be3f93e43 > > > > > > --- /dev/null > > > > > > +++ b/include/libcamera/internal/controls.h > > > > > > @@ -0,0 +1,39 @@ > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > +/* > > > > > > + * Copyright (C) 2025, Ideas on Board Oy > > > > > > + */ > > > > > > + > > > > > > +#pragma once > > > > > > + > > > > > > +#include <libcamera/controls.h> > > > > > > + > > > > > > +namespace libcamera::controls::details { > > > > > > + > > > > > > +struct TypeInfo { > > > > > > + std::size_t size = 0; > > > > > > + std::size_t alignment = 0; > > > > > > + > > > > > > + explicit operator bool() const { return alignment != 0; } > > > > > > + > > > > > > + static constexpr TypeInfo get(ControlType t) > > > > > > + { > > > > > > + switch (t) { > > > > > > + case ControlTypeNone: return {}; > > > > > > + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; > > > > > > + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; > > > > > > + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; > > > > > > + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; > > > > > > + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; > > > > > > + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; > > > > > > + case ControlTypeFloat: return { sizeof(float), alignof(float) }; > > > > > > + case ControlTypeString: return { sizeof(char), alignof(char) }; > > > > > > + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; > > > > > > + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; > > > > > > + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; > > > > > > + } > > > > > > > > > > As suggested in a previous review can these information be added to > > > > > control_type ? > > > > > > > > > > Would > > > > > > > > > > template<> > > > > > struct control_type<bool> { > > > > > static constexpr ControlType value = ControlTypeBool; > > > > > static constexpr std::size_t size = 0; > > > > > static constexpr std::size_t element_size = sizeof(bool); > > > > > static constexpr std::size_t alignment = alignof(bool); > > > > > }; > > > > > > > > > > Work ? > > > > > > > > That could be useful yes, but you still need a function like the above to map > > > > the "runtime" type to the appropriate values. > > > > > > If we use ctrl->type() yes, we need a run-time switch. > > > > > > I presume there is no easy way to go from Control<T> to > > > control_info<T> when iterating over the controls::controls id map.. > > > > > > > Only a `ControlId` is available, so there is no static type information > > available. Some kind of runtime switch is needed. This seemed like the > > simplest method. > > Indeed. > > Do you think it would make sense to centralize the information in the > existing control_type<T> and add a function similar in spirit to your > above TypeInfo::get() to controls.h or would you prefer keeping them > separate ? > mmm, scratch that, I had a quick look and I haven't find a nice lean way to specialize a template on a type and a ControlValueType. Unless you have something better ready, scratch this > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > + return {}; > > > > > > + } > > > > > > +}; > > > > > > + > > > > > > +} /* libcamera::controls::details */ > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > > index a238141a5..d5a86da5e 100644 > > > > > > --- a/src/libcamera/controls.cpp > > > > > > +++ b/src/libcamera/controls.cpp > > > > > > @@ -17,7 +17,7 @@ > > > > > > #include "libcamera/internal/control_validator.h" > > > > > > > > > > > > /** > > > > > > - * \file controls.h > > > > > > + * \file libcamera/controls.h > > > > > > > > > > Unrelated ? > > > > > > > > I can move it to a separate commit but it is needed. Because now we have > > > > two `controls.h`, and otherwise doxygen will complain. > > > > > > > > > > Ah sorry, I missed the reason and I thought it was just a leftover > > > > > > > > > > > > > > > > > > * \brief Framework to manage controls related to an object > > > > > > * > > > > > > * A control is a mean to govern or influence the operation of an object, and in > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > > index edfa9cf58..c8ac7a673 100644 > > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > > @@ -16,11 +16,13 @@ > > > > > > #include <libcamera/base/utils.h> > > > > > > > > > > > > #include <libcamera/camera.h> > > > > > > +#include <libcamera/control_ids.h> > > > > > > #include <libcamera/framebuffer.h> > > > > > > #include <libcamera/property_ids.h> > > > > > > > > > > > > #include "libcamera/internal/camera.h" > > > > > > #include "libcamera/internal/camera_manager.h" > > > > > > +#include "libcamera/internal/controls.h" > > > > > > #include "libcamera/internal/device_enumerator.h" > > > > > > #include "libcamera/internal/media_device.h" > > > > > > #include "libcamera/internal/request.h" > > > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, > > > > > > return std::string(); > > > > > > } > > > > > > > > > > > > +namespace { > > > > > > + > > > > > > +/* > > > > > > + * This is kind of hack. The metadata controls in the "debug" namespace > > > > > > > > > > I know, but I think for the time being is the best we can do :( > > > > > > > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that > > > > > > + * they work seamlessly without any additional setup. > > > > > > + * > > > > > > + * The dynamically-sized array-like controls have a maximum capacity > > > > > > + * determined by the magic number below. > > > > > > + */ > > > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) > > > > > > +{ > > > > > > + constexpr std::size_t kDynamicArrayCapacity = 32; > > > > > > + > > > > > > + for (const auto &[id, ctrl] : controls::controls) { > > > > > > + if (!ctrl->isOutput()) > > > > > > + continue; > > > > > > + if (ctrl->vendor() != "debug") > > > > > > + continue; > > > > > > + if (mlp.get(id)) > > > > > > + continue; > > > > > > + > > > > > > + std::size_t count = ctrl->size(); > > > > > > + if (count == 0) > > > > > > + count = 1; > > > > > > > > > > Why this ? Do you mind a comment unless is something trivial I have > > > > > missed ? > > > > > > > > Non-array controls have their static size set to 0. See `control_type<...>::size`. > > > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` > > > > could be modified somehow to make this clearer. > > > > > > ack, a small comment would make it clear > > > > done > > > > > > Regards, > > Barnabás Pőcze > > > > > > > > > > > > > > > > > > > Regards, > > > > Barnabás Pőcze > > > > > > > > > > > > > > > > > > > + else if (ctrl->size() == libcamera::dynamic_extent) > > > > > > + count = kDynamicArrayCapacity; > > > > > > + > > > > > > + const auto info = controls::details::TypeInfo::get(ctrl->type()); > > > > > > + if (!info) > > > > > > + continue; > > > > > > + > > > > > > + [[maybe_unused]] bool ok = mlp.set(id, > > > > > > + info.size, info.alignment, > > > > > > + count, ctrl->type(), ctrl->isArray()); > > > > > > + ASSERT(ok); > > > > > > + } > > > > > > +} > > > > > > + > > > > > > +} /* namespace */ > > > > > > + > > > > > > /** > > > > > > * \brief Register a camera to the camera manager and pipeline handler > > > > > > * \param[in] camera The camera to be added > > > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) > > > > > > Camera::Private *data = camera->_d(); > > > > > > data->properties_.set(properties::SystemDevices, devnums); > > > > > > > > > > > > + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); > > > > > > + > > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > Thanks > > > > > j > > > > > > > > > > > manager_->_d()->addCamera(std::move(camera)); > > > > > > } > > > > > > > > > > > > -- > > > > > > 2.50.1 > > > > > > > > > > > >
On Sun, Aug 03, 2025 at 01:56:48PM +0200, Jacopo Mondi wrote: > On Sat, Aug 02, 2025 at 12:36:40PM +0200, Jacopo Mondi wrote: > > Hi Barnabás > > > > On Wed, Jul 30, 2025 at 04:49:13PM +0200, Barnabás Pőcze wrote: > > > Hi > > > > > > 2025. 07. 28. 18:02 keltezéssel, Jacopo Mondi írta: > > > > Hi Barnabás > > > > > > > > On Mon, Jul 28, 2025 at 05:19:50PM +0200, Barnabás Pőcze wrote: > > > > > Hi > > > > > > > > > > 2025. 07. 28. 12:19 keltezéssel, Jacopo Mondi írta: > > > > > > Hi Barnabás > > > > > > > > > > > > On Mon, Jul 21, 2025 at 12:46:19PM +0200, Barnabás Pőcze wrote: > > > > > > > Inject all metadata controls in the "debug" namespace into > > > > > > > the metadata plan of each camera so that they can be used > > > > > > > seamlessly. Dynamically sized array-like controls have a > > > > > > > hard-coded size of 32 elements. > > > > > > > > > > > > > > Additionally, a new type is added for inspecting properties of > > > > > > > a control type at runtime since that was not available previously. > > > > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > --- > > > > > > > include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ > > > > > > > src/libcamera/controls.cpp | 2 +- > > > > > > > src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ > > > > > > > 3 files changed, 85 insertions(+), 1 deletion(-) > > > > > > > create mode 100644 include/libcamera/internal/controls.h > > > > > > > > > > > > > > diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h > > > > > > > new file mode 100644 > > > > > > > index 000000000..be3f93e43 > > > > > > > --- /dev/null > > > > > > > +++ b/include/libcamera/internal/controls.h > > > > > > > @@ -0,0 +1,39 @@ > > > > > > > +/* SPDX-License-Identifier: LGPL-2.1-or-later */ > > > > > > > +/* > > > > > > > + * Copyright (C) 2025, Ideas on Board Oy > > > > > > > + */ > > > > > > > + > > > > > > > +#pragma once > > > > > > > + > > > > > > > +#include <libcamera/controls.h> > > > > > > > + > > > > > > > +namespace libcamera::controls::details { > > > > > > > + > > > > > > > +struct TypeInfo { > > > > > > > + std::size_t size = 0; > > > > > > > + std::size_t alignment = 0; > > > > > > > + > > > > > > > + explicit operator bool() const { return alignment != 0; } > > > > > > > + > > > > > > > + static constexpr TypeInfo get(ControlType t) > > > > > > > + { > > > > > > > + switch (t) { > > > > > > > + case ControlTypeNone: return {}; > > > > > > > + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; > > > > > > > + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; > > > > > > > + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; > > > > > > > + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; > > > > > > > + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; > > > > > > > + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; > > > > > > > + case ControlTypeFloat: return { sizeof(float), alignof(float) }; > > > > > > > + case ControlTypeString: return { sizeof(char), alignof(char) }; > > > > > > > + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; > > > > > > > + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; > > > > > > > + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; > > > > > > > + } > > > > > > > > > > > > As suggested in a previous review can these information be added to > > > > > > control_type ? > > > > > > > > > > > > Would > > > > > > > > > > > > template<> > > > > > > struct control_type<bool> { > > > > > > static constexpr ControlType value = ControlTypeBool; > > > > > > static constexpr std::size_t size = 0; > > > > > > static constexpr std::size_t element_size = sizeof(bool); > > > > > > static constexpr std::size_t alignment = alignof(bool); > > > > > > }; > > > > > > > > > > > > Work ? > > > > > > > > > > That could be useful yes, but you still need a function like the above to map > > > > > the "runtime" type to the appropriate values. > > > > > > > > If we use ctrl->type() yes, we need a run-time switch. > > > > > > > > I presume there is no easy way to go from Control<T> to > > > > control_info<T> when iterating over the controls::controls id map.. > > > > > > > > > > Only a `ControlId` is available, so there is no static type information > > > available. Some kind of runtime switch is needed. This seemed like the > > > simplest method. > > > > Indeed. > > > > Do you think it would make sense to centralize the information in the > > existing control_type<T> and add a function similar in spirit to your > > above TypeInfo::get() to controls.h or would you prefer keeping them > > separate ? > > > > mmm, scratch that, I had a quick look and I haven't find a nice lean > way to specialize a template on a type and a ControlValueType. Unless > you have something better ready, scratch this > Be aware however that we have a ControlValueSize map in controls.cpp, so this one and the newly introduced one should probably be unified even if we leave control_type<> alone for now > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > + > > > > > > > + return {}; > > > > > > > + } > > > > > > > +}; > > > > > > > + > > > > > > > +} /* libcamera::controls::details */ > > > > > > > diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp > > > > > > > index a238141a5..d5a86da5e 100644 > > > > > > > --- a/src/libcamera/controls.cpp > > > > > > > +++ b/src/libcamera/controls.cpp > > > > > > > @@ -17,7 +17,7 @@ > > > > > > > #include "libcamera/internal/control_validator.h" > > > > > > > > > > > > > > /** > > > > > > > - * \file controls.h > > > > > > > + * \file libcamera/controls.h > > > > > > > > > > > > Unrelated ? > > > > > > > > > > I can move it to a separate commit but it is needed. Because now we have > > > > > two `controls.h`, and otherwise doxygen will complain. > > > > > > > > > > > > > Ah sorry, I missed the reason and I thought it was just a leftover > > > > > > > > > > > > > > > > > > > > > > * \brief Framework to manage controls related to an object > > > > > > > * > > > > > > > * A control is a mean to govern or influence the operation of an object, and in > > > > > > > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp > > > > > > > index edfa9cf58..c8ac7a673 100644 > > > > > > > --- a/src/libcamera/pipeline_handler.cpp > > > > > > > +++ b/src/libcamera/pipeline_handler.cpp > > > > > > > @@ -16,11 +16,13 @@ > > > > > > > #include <libcamera/base/utils.h> > > > > > > > > > > > > > > #include <libcamera/camera.h> > > > > > > > +#include <libcamera/control_ids.h> > > > > > > > #include <libcamera/framebuffer.h> > > > > > > > #include <libcamera/property_ids.h> > > > > > > > > > > > > > > #include "libcamera/internal/camera.h" > > > > > > > #include "libcamera/internal/camera_manager.h" > > > > > > > +#include "libcamera/internal/controls.h" > > > > > > > #include "libcamera/internal/device_enumerator.h" > > > > > > > #include "libcamera/internal/media_device.h" > > > > > > > #include "libcamera/internal/request.h" > > > > > > > @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, > > > > > > > return std::string(); > > > > > > > } > > > > > > > > > > > > > > +namespace { > > > > > > > + > > > > > > > +/* > > > > > > > + * This is kind of hack. The metadata controls in the "debug" namespace > > > > > > > > > > > > I know, but I think for the time being is the best we can do :( > > > > > > > > > > > > > + * are forcefully injected into each Camera's MetadataListPlan so that > > > > > > > + * they work seamlessly without any additional setup. > > > > > > > + * > > > > > > > + * The dynamically-sized array-like controls have a maximum capacity > > > > > > > + * determined by the magic number below. > > > > > > > + */ > > > > > > > +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) > > > > > > > +{ > > > > > > > + constexpr std::size_t kDynamicArrayCapacity = 32; > > > > > > > + > > > > > > > + for (const auto &[id, ctrl] : controls::controls) { > > > > > > > + if (!ctrl->isOutput()) > > > > > > > + continue; > > > > > > > + if (ctrl->vendor() != "debug") > > > > > > > + continue; > > > > > > > + if (mlp.get(id)) > > > > > > > + continue; > > > > > > > + > > > > > > > + std::size_t count = ctrl->size(); > > > > > > > + if (count == 0) > > > > > > > + count = 1; > > > > > > > > > > > > Why this ? Do you mind a comment unless is something trivial I have > > > > > > missed ? > > > > > > > > > > Non-array controls have their static size set to 0. See `control_type<...>::size`. > > > > > I can add a comment if you think it deserves one. I suppose the `MetadataListPlan` > > > > > could be modified somehow to make this clearer. > > > > > > > > ack, a small comment would make it clear > > > > > > done > > > > > > > > > Regards, > > > Barnabás Pőcze > > > > > > > > > > > > > > > > > > > > > > > > > Regards, > > > > > Barnabás Pőcze > > > > > > > > > > > > > > > > > > > > > > > + else if (ctrl->size() == libcamera::dynamic_extent) > > > > > > > + count = kDynamicArrayCapacity; > > > > > > > + > > > > > > > + const auto info = controls::details::TypeInfo::get(ctrl->type()); > > > > > > > + if (!info) > > > > > > > + continue; > > > > > > > + > > > > > > > + [[maybe_unused]] bool ok = mlp.set(id, > > > > > > > + info.size, info.alignment, > > > > > > > + count, ctrl->type(), ctrl->isArray()); > > > > > > > + ASSERT(ok); > > > > > > > + } > > > > > > > +} > > > > > > > + > > > > > > > +} /* namespace */ > > > > > > > + > > > > > > > /** > > > > > > > * \brief Register a camera to the camera manager and pipeline handler > > > > > > > * \param[in] camera The camera to be added > > > > > > > @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) > > > > > > > Camera::Private *data = camera->_d(); > > > > > > > data->properties_.set(properties::SystemDevices, devnums); > > > > > > > > > > > > > > + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); > > > > > > > + > > > > > > > > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > > > > > Thanks > > > > > > j > > > > > > > > > > > > > manager_->_d()->addCamera(std::move(camera)); > > > > > > > } > > > > > > > > > > > > > > -- > > > > > > > 2.50.1 > > > > > > > > > > > > > > >
diff --git a/include/libcamera/internal/controls.h b/include/libcamera/internal/controls.h new file mode 100644 index 000000000..be3f93e43 --- /dev/null +++ b/include/libcamera/internal/controls.h @@ -0,0 +1,39 @@ +/* SPDX-License-Identifier: LGPL-2.1-or-later */ +/* + * Copyright (C) 2025, Ideas on Board Oy + */ + +#pragma once + +#include <libcamera/controls.h> + +namespace libcamera::controls::details { + +struct TypeInfo { + std::size_t size = 0; + std::size_t alignment = 0; + + explicit operator bool() const { return alignment != 0; } + + static constexpr TypeInfo get(ControlType t) + { + switch (t) { + case ControlTypeNone: return {}; + case ControlTypeBool: return { sizeof(bool), alignof(bool) }; + case ControlTypeByte: return { sizeof(uint8_t), alignof(uint8_t) }; + case ControlTypeUnsigned16: return { sizeof(uint16_t), alignof(uint16_t) }; + case ControlTypeUnsigned32: return { sizeof(uint32_t), alignof(uint32_t) }; + case ControlTypeInteger32: return { sizeof(int32_t), alignof(int32_t) }; + case ControlTypeInteger64: return { sizeof(int64_t), alignof(int64_t) }; + case ControlTypeFloat: return { sizeof(float), alignof(float) }; + case ControlTypeString: return { sizeof(char), alignof(char) }; + case ControlTypeRectangle: return { sizeof(Rectangle), alignof(Rectangle) }; + case ControlTypeSize: return { sizeof(Size), alignof(Size) }; + case ControlTypePoint: return { sizeof(Point), alignof(Point) }; + } + + return {}; + } +}; + +} /* libcamera::controls::details */ diff --git a/src/libcamera/controls.cpp b/src/libcamera/controls.cpp index a238141a5..d5a86da5e 100644 --- a/src/libcamera/controls.cpp +++ b/src/libcamera/controls.cpp @@ -17,7 +17,7 @@ #include "libcamera/internal/control_validator.h" /** - * \file controls.h + * \file libcamera/controls.h * \brief Framework to manage controls related to an object * * A control is a mean to govern or influence the operation of an object, and in diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp index edfa9cf58..c8ac7a673 100644 --- a/src/libcamera/pipeline_handler.cpp +++ b/src/libcamera/pipeline_handler.cpp @@ -16,11 +16,13 @@ #include <libcamera/base/utils.h> #include <libcamera/camera.h> +#include <libcamera/control_ids.h> #include <libcamera/framebuffer.h> #include <libcamera/property_ids.h> #include "libcamera/internal/camera.h" #include "libcamera/internal/camera_manager.h" +#include "libcamera/internal/controls.h" #include "libcamera/internal/device_enumerator.h" #include "libcamera/internal/media_device.h" #include "libcamera/internal/request.h" @@ -749,6 +751,47 @@ std::string PipelineHandler::configurationFile(const std::string &subdir, return std::string(); } +namespace { + +/* + * This is kind of hack. The metadata controls in the "debug" namespace + * are forcefully injected into each Camera's MetadataListPlan so that + * they work seamlessly without any additional setup. + * + * The dynamically-sized array-like controls have a maximum capacity + * determined by the magic number below. + */ +void extendMetadataPlanWithDebugMetadata(MetadataListPlan& mlp) +{ + constexpr std::size_t kDynamicArrayCapacity = 32; + + for (const auto &[id, ctrl] : controls::controls) { + if (!ctrl->isOutput()) + continue; + if (ctrl->vendor() != "debug") + continue; + if (mlp.get(id)) + continue; + + std::size_t count = ctrl->size(); + if (count == 0) + count = 1; + else if (ctrl->size() == libcamera::dynamic_extent) + count = kDynamicArrayCapacity; + + const auto info = controls::details::TypeInfo::get(ctrl->type()); + if (!info) + continue; + + [[maybe_unused]] bool ok = mlp.set(id, + info.size, info.alignment, + count, ctrl->type(), ctrl->isArray()); + ASSERT(ok); + } +} + +} /* namespace */ + /** * \brief Register a camera to the camera manager and pipeline handler * \param[in] camera The camera to be added @@ -794,6 +837,8 @@ void PipelineHandler::registerCamera(std::shared_ptr<Camera> camera) Camera::Private *data = camera->_d(); data->properties_.set(properties::SystemDevices, devnums); + extendMetadataPlanWithDebugMetadata(data->metadataPlan_); + manager_->_d()->addCamera(std::move(camera)); }
Inject all metadata controls in the "debug" namespace into the metadata plan of each camera so that they can be used seamlessly. Dynamically sized array-like controls have a hard-coded size of 32 elements. Additionally, a new type is added for inspecting properties of a control type at runtime since that was not available previously. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- include/libcamera/internal/controls.h | 39 +++++++++++++++++++++++ src/libcamera/controls.cpp | 2 +- src/libcamera/pipeline_handler.cpp | 45 +++++++++++++++++++++++++++ 3 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 include/libcamera/internal/controls.h