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

[Jacopo Mondi]

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

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