[PATCH v1] libcamera: camera: Ensure correct id maps are always set

[Kieran Bingham]

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 at ideasonboard.com>
> > >
> > > Makes very much sense, thanks
> > > Reviewed-by: Jacopo Mondi <jacopo.mondi at 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 at 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
> > > >