| Message ID | 20250310170246.185147-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > `Camera::Private::properties_` is a default constructed `ControlList`, > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > is in a similar situation. > > Extend the `Camera::Private` constructor to initialize the control id map > of both properly. > > Multiple pipeline handlers copy the sensor's property list and > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > classes set the proper id map, the camera properties will have it too. > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > and thus there will be no id map set. To fix this, extend the > `Camera::Private` constructor to set `properties::properties`. > > As for `controlInfo_`, all pipeline handlers overwrite it during > camera initialization (and thus it will have the correct id map), > but still initialize the id map so that it is set at all times. > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> Makes very much sense, thanks Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > --- > src/libcamera/camera.cpp | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > index 56c585199..c180a5fdd 100644 > --- a/src/libcamera/camera.cpp > +++ b/src/libcamera/camera.cpp > @@ -21,6 +21,7 @@ > #include <libcamera/color_space.h> > #include <libcamera/control_ids.h> > #include <libcamera/framebuffer_allocator.h> > +#include <libcamera/property_ids.h> > #include <libcamera/request.h> > #include <libcamera/stream.h> > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > * \param[in] pipe The pipeline handler responsible for the camera device > */ > Camera::Private::Private(PipelineHandler *pipe) > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > + : controlInfo_({}, controls::controls), properties_(properties::properties), > + requestSequence_(0), pipe_(pipe->shared_from_this()), > disconnected_(false), state_(CameraAvailable) > { > } > -- > 2.48.1 >
Quoting Jacopo Mondi (2025-03-17 17:33:06) > Hi Barnabás > > On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > > `Camera::Private::properties_` is a default constructed `ControlList`, > > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > > is in a similar situation. > > > > Extend the `Camera::Private` constructor to initialize the control id map > > of both properly. > > > > Multiple pipeline handlers copy the sensor's property list and > > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > > classes set the proper id map, the camera properties will have it too. > > > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > > and thus there will be no id map set. To fix this, extend the > > `Camera::Private` constructor to set `properties::properties`. > > > > As for `controlInfo_`, all pipeline handlers overwrite it during > > camera initialization (and thus it will have the correct id map), > > but still initialize the id map so that it is set at all times. > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > Makes very much sense, thanks > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Yes, I like the sound of this too. Do we have correct uses of ControlList using the default constructor ? Maybe we should try to remove or deprecate it so that ControlLists should always construct with a correct type ? But I fear we already use ControlList independently so that wouldn't be possible or feasible so as this 'fixes' pipeline handlers in the default case ... Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > src/libcamera/camera.cpp | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > index 56c585199..c180a5fdd 100644 > > --- a/src/libcamera/camera.cpp > > +++ b/src/libcamera/camera.cpp > > @@ -21,6 +21,7 @@ > > #include <libcamera/color_space.h> > > #include <libcamera/control_ids.h> > > #include <libcamera/framebuffer_allocator.h> > > +#include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > #include <libcamera/stream.h> > > > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > * \param[in] pipe The pipeline handler responsible for the camera device > > */ > > Camera::Private::Private(PipelineHandler *pipe) > > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > > + : controlInfo_({}, controls::controls), properties_(properties::properties), > > + requestSequence_(0), pipe_(pipe->shared_from_this()), > > disconnected_(false), state_(CameraAvailable) > > { > > } > > -- > > 2.48.1 > >
Hi Kieran On Mon, Mar 17, 2025 at 05:51:11PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-03-17 17:33:06) > > Hi Barnabás > > > > On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > > > `Camera::Private::properties_` is a default constructed `ControlList`, > > > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > > > is in a similar situation. > > > > > > Extend the `Camera::Private` constructor to initialize the control id map > > > of both properly. > > > > > > Multiple pipeline handlers copy the sensor's property list and > > > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > > > classes set the proper id map, the camera properties will have it too. > > > > > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > > > and thus there will be no id map set. To fix this, extend the > > > `Camera::Private` constructor to set `properties::properties`. > > > > > > As for `controlInfo_`, all pipeline handlers overwrite it during > > > camera initialization (and thus it will have the correct id map), > > > but still initialize the id map so that it is set at all times. > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > Makes very much sense, thanks > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Yes, I like the sound of this too. > > Do we have correct uses of ControlList using the default constructor ? > Maybe we should try to remove or deprecate it so that ControlLists > should always construct with a correct type ? We don't have a ControlIdMap for V4L2 controls unfortunately... > > But I fear we already use ControlList independently so that wouldn't be > possible or feasible so as this 'fixes' pipeline handlers in the default > case ... > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > --- > > > src/libcamera/camera.cpp | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > index 56c585199..c180a5fdd 100644 > > > --- a/src/libcamera/camera.cpp > > > +++ b/src/libcamera/camera.cpp > > > @@ -21,6 +21,7 @@ > > > #include <libcamera/color_space.h> > > > #include <libcamera/control_ids.h> > > > #include <libcamera/framebuffer_allocator.h> > > > +#include <libcamera/property_ids.h> > > > #include <libcamera/request.h> > > > #include <libcamera/stream.h> > > > > > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > * \param[in] pipe The pipeline handler responsible for the camera device > > > */ > > > Camera::Private::Private(PipelineHandler *pipe) > > > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > > > + : controlInfo_({}, controls::controls), properties_(properties::properties), > > > + requestSequence_(0), pipe_(pipe->shared_from_this()), > > > disconnected_(false), state_(CameraAvailable) > > > { > > > } > > > -- > > > 2.48.1 > > >
Quoting Jacopo Mondi (2025-03-17 18:50:06) > Hi Kieran > > On Mon, Mar 17, 2025 at 05:51:11PM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2025-03-17 17:33:06) > > > Hi Barnabás > > > > > > On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > > > > `Camera::Private::properties_` is a default constructed `ControlList`, > > > > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > > > > is in a similar situation. > > > > > > > > Extend the `Camera::Private` constructor to initialize the control id map > > > > of both properly. > > > > > > > > Multiple pipeline handlers copy the sensor's property list and > > > > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > > > > classes set the proper id map, the camera properties will have it too. > > > > > > > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > > > > and thus there will be no id map set. To fix this, extend the > > > > `Camera::Private` constructor to set `properties::properties`. > > > > > > > > As for `controlInfo_`, all pipeline handlers overwrite it during > > > > camera initialization (and thus it will have the correct id map), > > > > but still initialize the id map so that it is set at all times. > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > Makes very much sense, thanks > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > Yes, I like the sound of this too. > > > > Do we have correct uses of ControlList using the default constructor ? > > Maybe we should try to remove or deprecate it so that ControlLists > > should always construct with a correct type ? > > We don't have a ControlIdMap for V4L2 controls unfortunately... I have a low priority 'todo' somewhere in my notes to investigate what could happen if we set up a 'vendor namespace' of V4L2 controls that map directly to V4L2 controls ... I 'almost' think that's possible!? Based on include/uapi/linux/v4l2-controls.h we could 'vendor' the V4L2 control classes ... and then have a direct mapping of V4L2 controls as libcamera::V4L2::* with a bit of python to automate the mapping. All of the V4L2 classes are high up in the int32 address space: /* Control classes */ #define V4L2_CTRL_CLASS_USER 0x00980000 /* Old-style 'user' controls */ #define V4L2_CTRL_CLASS_CODEC 0x00990000 /* Stateful codec controls */ #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */ #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator controls */ #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */ #define V4L2_CTRL_CLASS_JPEG 0x009d0000 /* JPEG-compression controls */ #define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009e0000 /* Image source controls */ #define V4L2_CTRL_CLASS_IMAGE_PROC 0x009f0000 /* Image processing controls */ #define V4L2_CTRL_CLASS_DV 0x00a00000 /* Digital Video controls */ #define V4L2_CTRL_CLASS_FM_RX 0x00a10000 /* FM Receiver controls */ #define V4L2_CTRL_CLASS_RF_TUNER 0x00a20000 /* RF tuner controls */ #define V4L2_CTRL_CLASS_DETECT 0x00a30000 /* Detection controls */ #define V4L2_CTRL_CLASS_CODEC_STATELESS 0x00a40000 /* Stateless codecs controls */ #define V4L2_CTRL_CLASS_COLORIMETRY 0x00a50000 /* Colorimetry controls */ which is incredibly convenient ... > > But I fear we already use ControlList independently so that wouldn't be > > possible or feasible so as this 'fixes' pipeline handlers in the default > > case ... > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > --- > > > > src/libcamera/camera.cpp | 4 +++- > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > index 56c585199..c180a5fdd 100644 > > > > --- a/src/libcamera/camera.cpp > > > > +++ b/src/libcamera/camera.cpp > > > > @@ -21,6 +21,7 @@ > > > > #include <libcamera/color_space.h> > > > > #include <libcamera/control_ids.h> > > > > #include <libcamera/framebuffer_allocator.h> > > > > +#include <libcamera/property_ids.h> > > > > #include <libcamera/request.h> > > > > #include <libcamera/stream.h> > > > > > > > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > > * \param[in] pipe The pipeline handler responsible for the camera device > > > > */ > > > > Camera::Private::Private(PipelineHandler *pipe) > > > > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > + : controlInfo_({}, controls::controls), properties_(properties::properties), > > > > + requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > disconnected_(false), state_(CameraAvailable) > > > > { > > > > } > > > > -- > > > > 2.48.1 > > > >
Hi Kieran On Mon, Mar 17, 2025 at 09:50:30PM +0000, Kieran Bingham wrote: > Quoting Jacopo Mondi (2025-03-17 18:50:06) > > Hi Kieran > > > > On Mon, Mar 17, 2025 at 05:51:11PM +0000, Kieran Bingham wrote: > > > Quoting Jacopo Mondi (2025-03-17 17:33:06) > > > > Hi Barnabás > > > > > > > > On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > > > > > `Camera::Private::properties_` is a default constructed `ControlList`, > > > > > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > > > > > is in a similar situation. > > > > > > > > > > Extend the `Camera::Private` constructor to initialize the control id map > > > > > of both properly. > > > > > > > > > > Multiple pipeline handlers copy the sensor's property list and > > > > > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > > > > > classes set the proper id map, the camera properties will have it too. > > > > > > > > > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > > > > > and thus there will be no id map set. To fix this, extend the > > > > > `Camera::Private` constructor to set `properties::properties`. > > > > > > > > > > As for `controlInfo_`, all pipeline handlers overwrite it during > > > > > camera initialization (and thus it will have the correct id map), > > > > > but still initialize the id map so that it is set at all times. > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > > Makes very much sense, thanks > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > Yes, I like the sound of this too. > > > > > > Do we have correct uses of ControlList using the default constructor ? > > > Maybe we should try to remove or deprecate it so that ControlLists > > > should always construct with a correct type ? > > > > We don't have a ControlIdMap for V4L2 controls unfortunately... > > I have a low priority 'todo' somewhere in my notes to investigate what > could happen if we set up a 'vendor namespace' of V4L2 controls that map > directly to V4L2 controls ... I 'almost' think that's possible!? > > Based on include/uapi/linux/v4l2-controls.h we could 'vendor' the V4L2 > control classes ... and then have a direct mapping of V4L2 controls as > libcamera::V4L2::* with a bit of python to automate the mapping. > > All of the V4L2 classes are high up in the int32 address space: > > /* Control classes */ > #define V4L2_CTRL_CLASS_USER 0x00980000 /* Old-style 'user' controls */ > #define V4L2_CTRL_CLASS_CODEC 0x00990000 /* Stateful codec controls */ > #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */ > #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator controls */ > #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */ > #define V4L2_CTRL_CLASS_JPEG 0x009d0000 /* JPEG-compression controls */ > #define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009e0000 /* Image source controls */ > #define V4L2_CTRL_CLASS_IMAGE_PROC 0x009f0000 /* Image processing controls */ > #define V4L2_CTRL_CLASS_DV 0x00a00000 /* Digital Video controls */ > #define V4L2_CTRL_CLASS_FM_RX 0x00a10000 /* FM Receiver controls */ > #define V4L2_CTRL_CLASS_RF_TUNER 0x00a20000 /* RF tuner controls */ > #define V4L2_CTRL_CLASS_DETECT 0x00a30000 /* Detection controls */ > #define V4L2_CTRL_CLASS_CODEC_STATELESS 0x00a40000 /* Stateless codecs controls */ > #define V4L2_CTRL_CLASS_COLORIMETRY 0x00a50000 /* Colorimetry controls */ > > > which is incredibly convenient ... Yes, that's convenient. And we discussed about mapping v4l2 controls to libcamera controls many times years ago. However the enumeration of v4l2 controls is not under our control, and I can imagine that bad things happen if the enumeration of v4l2 controls on the device that runs libcamera differ from the one we have in our headers file copy. Newly introduced controls in mainline should only be added to the existing ones, so it should be safe ? However, and I'm not sure it's something we should really be concerned with or not, this becomes a problem with controls introduced downstream in vendor's BSP, and I presume quite some people runs libcamera on some sort of BSP, specifically for some mature platforms where the mainline kernel drivers have been absorbed in the BSP versions. In this case, to generate a libcamera control for you vendor control, you will have to recompile. Do we care ? > > > > > > But I fear we already use ControlList independently so that wouldn't be > > > possible or feasible so as this 'fixes' pipeline handlers in the default > > > case ... > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > --- > > > > > src/libcamera/camera.cpp | 4 +++- > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > > index 56c585199..c180a5fdd 100644 > > > > > --- a/src/libcamera/camera.cpp > > > > > +++ b/src/libcamera/camera.cpp > > > > > @@ -21,6 +21,7 @@ > > > > > #include <libcamera/color_space.h> > > > > > #include <libcamera/control_ids.h> > > > > > #include <libcamera/framebuffer_allocator.h> > > > > > +#include <libcamera/property_ids.h> > > > > > #include <libcamera/request.h> > > > > > #include <libcamera/stream.h> > > > > > > > > > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > > > * \param[in] pipe The pipeline handler responsible for the camera device > > > > > */ > > > > > Camera::Private::Private(PipelineHandler *pipe) > > > > > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > > + : controlInfo_({}, controls::controls), properties_(properties::properties), > > > > > + requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > > disconnected_(false), state_(CameraAvailable) > > > > > { > > > > > } > > > > > -- > > > > > 2.48.1 > > > > >
Quoting Jacopo Mondi (2025-03-18 07:45:12) > Hi Kieran > > On Mon, Mar 17, 2025 at 09:50:30PM +0000, Kieran Bingham wrote: > > Quoting Jacopo Mondi (2025-03-17 18:50:06) > > > Hi Kieran > > > > > > On Mon, Mar 17, 2025 at 05:51:11PM +0000, Kieran Bingham wrote: > > > > Quoting Jacopo Mondi (2025-03-17 17:33:06) > > > > > Hi Barnabás > > > > > > > > > > On Mon, Mar 10, 2025 at 06:02:46PM +0100, Barnabás Pőcze wrote: > > > > > > `Camera::Private::properties_` is a default constructed `ControlList`, > > > > > > therefore it does not have an associated `ControlIdMap`. `controlInfo_` > > > > > > is in a similar situation. > > > > > > > > > > > > Extend the `Camera::Private` constructor to initialize the control id map > > > > > > of both properly. > > > > > > > > > > > > Multiple pipeline handlers copy the sensor's property list and > > > > > > set that as camera properties, and since the `CameraSensor{Legacy,Raw}` > > > > > > classes set the proper id map, the camera properties will have it too. > > > > > > > > > > > > However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, > > > > > > and thus there will be no id map set. To fix this, extend the > > > > > > `Camera::Private` constructor to set `properties::properties`. > > > > > > > > > > > > As for `controlInfo_`, all pipeline handlers overwrite it during > > > > > > camera initialization (and thus it will have the correct id map), > > > > > > but still initialize the id map so that it is set at all times. > > > > > > > > > > > > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> > > > > > > > > > > Makes very much sense, thanks > > > > > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > > > > > > > Yes, I like the sound of this too. > > > > > > > > Do we have correct uses of ControlList using the default constructor ? > > > > Maybe we should try to remove or deprecate it so that ControlLists > > > > should always construct with a correct type ? > > > > > > We don't have a ControlIdMap for V4L2 controls unfortunately... > > > > I have a low priority 'todo' somewhere in my notes to investigate what > > could happen if we set up a 'vendor namespace' of V4L2 controls that map > > directly to V4L2 controls ... I 'almost' think that's possible!? > > > > Based on include/uapi/linux/v4l2-controls.h we could 'vendor' the V4L2 > > control classes ... and then have a direct mapping of V4L2 controls as > > libcamera::V4L2::* with a bit of python to automate the mapping. > > > > All of the V4L2 classes are high up in the int32 address space: > > > > /* Control classes */ > > #define V4L2_CTRL_CLASS_USER 0x00980000 /* Old-style 'user' controls */ > > #define V4L2_CTRL_CLASS_CODEC 0x00990000 /* Stateful codec controls */ > > #define V4L2_CTRL_CLASS_CAMERA 0x009a0000 /* Camera class controls */ > > #define V4L2_CTRL_CLASS_FM_TX 0x009b0000 /* FM Modulator controls */ > > #define V4L2_CTRL_CLASS_FLASH 0x009c0000 /* Camera flash controls */ > > #define V4L2_CTRL_CLASS_JPEG 0x009d0000 /* JPEG-compression controls */ > > #define V4L2_CTRL_CLASS_IMAGE_SOURCE 0x009e0000 /* Image source controls */ > > #define V4L2_CTRL_CLASS_IMAGE_PROC 0x009f0000 /* Image processing controls */ > > #define V4L2_CTRL_CLASS_DV 0x00a00000 /* Digital Video controls */ > > #define V4L2_CTRL_CLASS_FM_RX 0x00a10000 /* FM Receiver controls */ > > #define V4L2_CTRL_CLASS_RF_TUNER 0x00a20000 /* RF tuner controls */ > > #define V4L2_CTRL_CLASS_DETECT 0x00a30000 /* Detection controls */ > > #define V4L2_CTRL_CLASS_CODEC_STATELESS 0x00a40000 /* Stateless codecs controls */ > > #define V4L2_CTRL_CLASS_COLORIMETRY 0x00a50000 /* Colorimetry controls */ > > > > > > which is incredibly convenient ... > > Yes, that's convenient. And we discussed about mapping v4l2 controls > to libcamera controls many times years ago. Back then we didn't have control namespaces though ... > However the enumeration of v4l2 controls is not under our control, and > I can imagine that bad things happen if the enumeration of v4l2 > controls on the device that runs libcamera differ from the one we have > in our headers file copy. Indeed, but at least mapping the ones we use directly seems better to align things. And as they'd be generated - they should be able to be constructed from known defintions. > Newly introduced controls in mainline should only be added to the > existing ones, so it should be safe ? Absolutely - include/uapi/linux/v4l2-controls.h is a Linux ABI. > However, and I'm not sure it's something we should really be concerned > with or not, this becomes a problem with controls introduced > downstream in vendor's BSP, and I presume quite some people runs > libcamera on some sort of BSP, specifically for some mature platforms > where the mainline kernel drivers have been absorbed in the BSP > versions. In this case, to generate a libcamera control for you vendor > control, you will have to recompile. If they're going to add new controls, they'll have to recompile anyway. To me - putting V4L2 controls into a namespace is more about clarifying the usage of V4L2 ids' and also ensuring that those IDs are 'reserved' so could never conflict with libcamera controls. They could still be referenced by the V4L2 macros for the ID. But the ID would be clearly defined. > Do we care ? > About which bit? Vendor BSPs needing recompilation? Not me ;-) -- Kieran > > > > > > > > > > But I fear we already use ControlList independently so that wouldn't be > > > > possible or feasible so as this 'fixes' pipeline handlers in the default > > > > case ... > > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > > > > > > > --- > > > > > > src/libcamera/camera.cpp | 4 +++- > > > > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp > > > > > > index 56c585199..c180a5fdd 100644 > > > > > > --- a/src/libcamera/camera.cpp > > > > > > +++ b/src/libcamera/camera.cpp > > > > > > @@ -21,6 +21,7 @@ > > > > > > #include <libcamera/color_space.h> > > > > > > #include <libcamera/control_ids.h> > > > > > > #include <libcamera/framebuffer_allocator.h> > > > > > > +#include <libcamera/property_ids.h> > > > > > > #include <libcamera/request.h> > > > > > > #include <libcamera/stream.h> > > > > > > > > > > > > @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF > > > > > > * \param[in] pipe The pipeline handler responsible for the camera device > > > > > > */ > > > > > > Camera::Private::Private(PipelineHandler *pipe) > > > > > > - : requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > > > + : controlInfo_({}, controls::controls), properties_(properties::properties), > > > > > > + requestSequence_(0), pipe_(pipe->shared_from_this()), > > > > > > disconnected_(false), state_(CameraAvailable) > > > > > > { > > > > > > } > > > > > > -- > > > > > > 2.48.1 > > > > > >
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp index 56c585199..c180a5fdd 100644 --- a/src/libcamera/camera.cpp +++ b/src/libcamera/camera.cpp @@ -21,6 +21,7 @@ #include <libcamera/color_space.h> #include <libcamera/control_ids.h> #include <libcamera/framebuffer_allocator.h> +#include <libcamera/property_ids.h> #include <libcamera/request.h> #include <libcamera/stream.h> @@ -587,7 +588,8 @@ CameraConfiguration::Status CameraConfiguration::validateColorSpaces(ColorSpaceF * \param[in] pipe The pipeline handler responsible for the camera device */ Camera::Private::Private(PipelineHandler *pipe) - : requestSequence_(0), pipe_(pipe->shared_from_this()), + : controlInfo_({}, controls::controls), properties_(properties::properties), + requestSequence_(0), pipe_(pipe->shared_from_this()), disconnected_(false), state_(CameraAvailable) { }
`Camera::Private::properties_` is a default constructed `ControlList`, therefore it does not have an associated `ControlIdMap`. `controlInfo_` is in a similar situation. Extend the `Camera::Private` constructor to initialize the control id map of both properly. Multiple pipeline handlers copy the sensor's property list and set that as camera properties, and since the `CameraSensor{Legacy,Raw}` classes set the proper id map, the camera properties will have it too. However, some pipelines, e.g. `uvcvideo` or `virtual`, do not do so, and thus there will be no id map set. To fix this, extend the `Camera::Private` constructor to set `properties::properties`. As for `controlInfo_`, all pipeline handlers overwrite it during camera initialization (and thus it will have the correct id map), but still initialize the id map so that it is set at all times. Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com> --- src/libcamera/camera.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)