[v1,1/2] libcamera: camera_sensor: getControls(): Use span
diff mbox series

Message ID 20250303193349.785692-1-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [v1,1/2] libcamera: camera_sensor: getControls(): Use span
Related show

Commit Message

Barnabás Pőcze March 3, 2025, 7:33 p.m. UTC
Do not force the caller to construct a vector.

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
 include/libcamera/internal/camera_sensor.h    |  3 ++-
 include/libcamera/internal/v4l2_device.h      |  2 +-
 src/libcamera/sensor/camera_sensor_legacy.cpp | 14 +++++++++-----
 src/libcamera/sensor/camera_sensor_raw.cpp    | 14 +++++++++-----
 src/libcamera/v4l2_device.cpp                 |  2 +-
 test/v4l2_videodevice/controls.cpp            | 14 +++++++++-----
 6 files changed, 31 insertions(+), 18 deletions(-)

Comments

Kieran Bingham July 21, 2025, 6:32 p.m. UTC | #1
Quoting Barnabás Pőcze (2025-03-03 19:33:48)
> Do not force the caller to construct a vector.
> 

Perhaps we could expand this:


"""
The V4L2 getControls implementation currently requires a std::vector to
be constructed.

Update this to use a Span to make it possible to store static const
arrays of the required ControlIds to remove dynamic allocation while
calling for a fixed range of controls.

Convert relevant callers as part of the change.
"""


This passed CI in March (for both commits in this series)

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1376848

And still applies cleanly.

Running CI now :

https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1475482

Succeeded.

I think such a change is reasonable.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  include/libcamera/internal/camera_sensor.h    |  3 ++-
>  include/libcamera/internal/v4l2_device.h      |  2 +-
>  src/libcamera/sensor/camera_sensor_legacy.cpp | 14 +++++++++-----
>  src/libcamera/sensor/camera_sensor_raw.cpp    | 14 +++++++++-----
>  src/libcamera/v4l2_device.cpp                 |  2 +-
>  test/v4l2_videodevice/controls.cpp            | 14 +++++++++-----
>  6 files changed, 31 insertions(+), 18 deletions(-)
> 
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 13048f327..59a1604c5 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -14,6 +14,7 @@
>  #include <vector>
>  
>  #include <libcamera/base/class.h>
> +#include <libcamera/base/span.h>
>  
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
> @@ -74,7 +75,7 @@ public:
>         virtual BayerFormat::Order bayerOrder(Transform t) const = 0;
>  
>         virtual const ControlInfoMap &controls() const = 0;
> -       virtual ControlList getControls(const std::vector<uint32_t> &ids) = 0;
> +       virtual ControlList getControls(Span<const uint32_t> ids) = 0;
>         virtual int setControls(ControlList *ctrls) = 0;
>  
>         virtual const std::vector<controls::draft::TestPatternModeEnum> &
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index affe52c2a..4ab818d90 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -37,7 +37,7 @@ public:
>  
>         const ControlInfoMap &controls() const { return controls_; }
>  
> -       ControlList getControls(const std::vector<uint32_t> &ids);
> +       ControlList getControls(Span<const uint32_t> ids);
>         int setControls(ControlList *ctrls);
>  
>         const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index 32989c19c..1eccca519 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -90,7 +90,7 @@ public:
>         BayerFormat::Order bayerOrder(Transform t) const override;
>  
>         const ControlInfoMap &controls() const override;
> -       ControlList getControls(const std::vector<uint32_t> &ids) override;
> +       ControlList getControls(Span<const uint32_t> ids) override;
>         int setControls(ControlList *ctrls) override;
>  
>         const std::vector<controls::draft::TestPatternModeEnum> &
> @@ -907,9 +907,13 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
>          * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>          * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>          */
> -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> -                                                  V4L2_CID_HBLANK,
> -                                                  V4L2_CID_VBLANK });
> +       static constexpr uint32_t cids[] = {
> +               V4L2_CID_PIXEL_RATE,
> +               V4L2_CID_HBLANK,
> +               V4L2_CID_VBLANK,
> +       };
> +
> +       ControlList ctrls = subdev_->getControls(cids);
>         if (ctrls.empty()) {
>                 LOG(CameraSensor, Error)
>                         << "Failed to retrieve camera info controls";
> @@ -983,7 +987,7 @@ const ControlInfoMap &CameraSensorLegacy::controls() const
>         return subdev_->controls();
>  }
>  
> -ControlList CameraSensorLegacy::getControls(const std::vector<uint32_t> &ids)
> +ControlList CameraSensorLegacy::getControls(Span<const uint32_t> ids)
>  {
>         return subdev_->getControls(ids);
>  }
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index ab75b1f82..c65ecbb86 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -96,7 +96,7 @@ public:
>         BayerFormat::Order bayerOrder(Transform t) const override;
>  
>         const ControlInfoMap &controls() const override;
> -       ControlList getControls(const std::vector<uint32_t> &ids) override;
> +       ControlList getControls(Span<const uint32_t> ids) override;
>         int setControls(ControlList *ctrls) override;
>  
>         const std::vector<controls::draft::TestPatternModeEnum> &
> @@ -1022,9 +1022,13 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
>          * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>          * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>          */
> -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> -                                                  V4L2_CID_HBLANK,
> -                                                  V4L2_CID_VBLANK });
> +       static constexpr uint32_t cids[] = {
> +               V4L2_CID_PIXEL_RATE,
> +               V4L2_CID_HBLANK,
> +               V4L2_CID_VBLANK,
> +       };
> +
> +       ControlList ctrls = subdev_->getControls(cids);
>         if (ctrls.empty()) {
>                 LOG(CameraSensor, Error)
>                         << "Failed to retrieve camera info controls";
> @@ -1095,7 +1099,7 @@ const ControlInfoMap &CameraSensorRaw::controls() const
>         return subdev_->controls();
>  }
>  
> -ControlList CameraSensorRaw::getControls(const std::vector<uint32_t> &ids)
> +ControlList CameraSensorRaw::getControls(Span<const uint32_t> ids)
>  {
>         return subdev_->getControls(ids);
>  }
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2f65a43a0..089f45afd 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -174,7 +174,7 @@ void V4L2Device::close()
>   * \return The control values in a ControlList on success, or an empty list on
>   * error
>   */
> -ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> +ControlList V4L2Device::getControls(Span<const uint32_t> ids)
>  {
>         if (ids.empty())
>                 return {};
> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> index b0130295e..7990f37dc 100644
> --- a/test/v4l2_videodevice/controls.cpp
> +++ b/test/v4l2_videodevice/controls.cpp
> @@ -60,11 +60,15 @@ protected:
>                 const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
>  
>                 /* Test getting controls. */
> -               ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
> -                                                           V4L2_CID_CONTRAST,
> -                                                           V4L2_CID_SATURATION,
> -                                                           VIVID_CID_INTEGER64,
> -                                                           VIVID_CID_U8_4D_ARRAY });
> +               static constexpr uint32_t cids[] = {
> +                       V4L2_CID_BRIGHTNESS,
> +                       V4L2_CID_CONTRAST,
> +                       V4L2_CID_SATURATION,
> +                       VIVID_CID_INTEGER64,
> +                       VIVID_CID_U8_4D_ARRAY,
> +               };
> +
> +               ControlList ctrls = capture_->getControls(cids);
>                 if (ctrls.empty()) {
>                         cerr << "Failed to get controls" << endl;
>                         return TestFail;
> -- 
> 2.48.1
>
Dan Scally July 24, 2025, 7:02 a.m. UTC | #2
Hi Barnabas, thanks for the patch

On 03/03/2025 19:33, Barnabás Pőcze wrote:
> Do not force the caller to construct a vector.
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>
>   include/libcamera/internal/camera_sensor.h    |  3 ++-
>   include/libcamera/internal/v4l2_device.h      |  2 +-
>   src/libcamera/sensor/camera_sensor_legacy.cpp | 14 +++++++++-----
>   src/libcamera/sensor/camera_sensor_raw.cpp    | 14 +++++++++-----
>   src/libcamera/v4l2_device.cpp                 |  2 +-
>   test/v4l2_videodevice/controls.cpp            | 14 +++++++++-----
>   6 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> index 13048f327..59a1604c5 100644
> --- a/include/libcamera/internal/camera_sensor.h
> +++ b/include/libcamera/internal/camera_sensor.h
> @@ -14,6 +14,7 @@
>   #include <vector>
>   
>   #include <libcamera/base/class.h>
> +#include <libcamera/base/span.h>
>   
>   #include <libcamera/control_ids.h>
>   #include <libcamera/controls.h>
> @@ -74,7 +75,7 @@ public:
>   	virtual BayerFormat::Order bayerOrder(Transform t) const = 0;
>   
>   	virtual const ControlInfoMap &controls() const = 0;
> -	virtual ControlList getControls(const std::vector<uint32_t> &ids) = 0;
> +	virtual ControlList getControls(Span<const uint32_t> ids) = 0;
>   	virtual int setControls(ControlList *ctrls) = 0;
>   
>   	virtual const std::vector<controls::draft::TestPatternModeEnum> &
> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> index affe52c2a..4ab818d90 100644
> --- a/include/libcamera/internal/v4l2_device.h
> +++ b/include/libcamera/internal/v4l2_device.h
> @@ -37,7 +37,7 @@ public:
>   
>   	const ControlInfoMap &controls() const { return controls_; }
>   
> -	ControlList getControls(const std::vector<uint32_t> &ids);
> +	ControlList getControls(Span<const uint32_t> ids);
>   	int setControls(ControlList *ctrls);
>   
>   	const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;
> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> index 32989c19c..1eccca519 100644
> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> @@ -90,7 +90,7 @@ public:
>   	BayerFormat::Order bayerOrder(Transform t) const override;
>   
>   	const ControlInfoMap &controls() const override;
> -	ControlList getControls(const std::vector<uint32_t> &ids) override;
> +	ControlList getControls(Span<const uint32_t> ids) override;
>   	int setControls(ControlList *ctrls) override;
>   
>   	const std::vector<controls::draft::TestPatternModeEnum> &
> @@ -907,9 +907,13 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
>   	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>   	 * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>   	 */
> -	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> -						   V4L2_CID_HBLANK,
> -						   V4L2_CID_VBLANK });
> +	static constexpr uint32_t cids[] = {
> +		V4L2_CID_PIXEL_RATE,
> +		V4L2_CID_HBLANK,
> +		V4L2_CID_VBLANK,
> +	};
> +
> +	ControlList ctrls = subdev_->getControls(cids);
>   	if (ctrls.empty()) {
>   		LOG(CameraSensor, Error)
>   			<< "Failed to retrieve camera info controls";
> @@ -983,7 +987,7 @@ const ControlInfoMap &CameraSensorLegacy::controls() const
>   	return subdev_->controls();
>   }
>   
> -ControlList CameraSensorLegacy::getControls(const std::vector<uint32_t> &ids)
> +ControlList CameraSensorLegacy::getControls(Span<const uint32_t> ids)
>   {
>   	return subdev_->getControls(ids);
>   }
> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> index ab75b1f82..c65ecbb86 100644
> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> @@ -96,7 +96,7 @@ public:
>   	BayerFormat::Order bayerOrder(Transform t) const override;
>   
>   	const ControlInfoMap &controls() const override;
> -	ControlList getControls(const std::vector<uint32_t> &ids) override;
> +	ControlList getControls(Span<const uint32_t> ids) override;
>   	int setControls(ControlList *ctrls) override;
>   
>   	const std::vector<controls::draft::TestPatternModeEnum> &
> @@ -1022,9 +1022,13 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
>   	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>   	 * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>   	 */
> -	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> -						   V4L2_CID_HBLANK,
> -						   V4L2_CID_VBLANK });
> +	static constexpr uint32_t cids[] = {
> +		V4L2_CID_PIXEL_RATE,
> +		V4L2_CID_HBLANK,
> +		V4L2_CID_VBLANK,
> +	};
> +
> +	ControlList ctrls = subdev_->getControls(cids);
>   	if (ctrls.empty()) {
>   		LOG(CameraSensor, Error)
>   			<< "Failed to retrieve camera info controls";
> @@ -1095,7 +1099,7 @@ const ControlInfoMap &CameraSensorRaw::controls() const
>   	return subdev_->controls();
>   }
>   
> -ControlList CameraSensorRaw::getControls(const std::vector<uint32_t> &ids)
> +ControlList CameraSensorRaw::getControls(Span<const uint32_t> ids)
>   {
>   	return subdev_->getControls(ids);
>   }
> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> index 2f65a43a0..089f45afd 100644
> --- a/src/libcamera/v4l2_device.cpp
> +++ b/src/libcamera/v4l2_device.cpp
> @@ -174,7 +174,7 @@ void V4L2Device::close()
>    * \return The control values in a ControlList on success, or an empty list on
>    * error
>    */
> -ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> +ControlList V4L2Device::getControls(Span<const uint32_t> ids)
>   {
>   	if (ids.empty())
>   		return {};
> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> index b0130295e..7990f37dc 100644
> --- a/test/v4l2_videodevice/controls.cpp
> +++ b/test/v4l2_videodevice/controls.cpp
> @@ -60,11 +60,15 @@ protected:
>   		const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
>   
>   		/* Test getting controls. */
> -		ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
> -							    V4L2_CID_CONTRAST,
> -							    V4L2_CID_SATURATION,
> -							    VIVID_CID_INTEGER64,
> -							    VIVID_CID_U8_4D_ARRAY });
> +		static constexpr uint32_t cids[] = {
> +			V4L2_CID_BRIGHTNESS,
> +			V4L2_CID_CONTRAST,
> +			V4L2_CID_SATURATION,
> +			VIVID_CID_INTEGER64,
> +			VIVID_CID_U8_4D_ARRAY,
> +		};
> +
> +		ControlList ctrls = capture_->getControls(cids);
>   		if (ctrls.empty()) {
>   			cerr << "Failed to get controls" << endl;
>   			return TestFail;
Laurent Pinchart July 24, 2025, 12:14 p.m. UTC | #3
On Mon, Jul 21, 2025 at 07:32:57PM +0100, Kieran Bingham wrote:
> Quoting Barnabás Pőcze (2025-03-03 19:33:48)
> > Do not force the caller to construct a vector.
> 
> Perhaps we could expand this:
> 
> """
> The V4L2 getControls implementation currently requires a std::vector to
> be constructed.
> 
> Update this to use a Span to make it possible to store static const
> arrays of the required ControlIds to remove dynamic allocation while
> calling for a fixed range of controls.
> 
> Convert relevant callers as part of the change.
> """
> 
> This passed CI in March (for both commits in this series)
> 
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1376848
> 
> And still applies cleanly.
> 
> Running CI now :
> 
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1475482
> 
> Succeeded.
> 
> I think such a change is reasonable.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> > ---
> >  include/libcamera/internal/camera_sensor.h    |  3 ++-
> >  include/libcamera/internal/v4l2_device.h      |  2 +-
> >  src/libcamera/sensor/camera_sensor_legacy.cpp | 14 +++++++++-----
> >  src/libcamera/sensor/camera_sensor_raw.cpp    | 14 +++++++++-----
> >  src/libcamera/v4l2_device.cpp                 |  2 +-
> >  test/v4l2_videodevice/controls.cpp            | 14 +++++++++-----
> >  6 files changed, 31 insertions(+), 18 deletions(-)
> > 
> > diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
> > index 13048f327..59a1604c5 100644
> > --- a/include/libcamera/internal/camera_sensor.h
> > +++ b/include/libcamera/internal/camera_sensor.h
> > @@ -14,6 +14,7 @@
> >  #include <vector>
> >  
> >  #include <libcamera/base/class.h>
> > +#include <libcamera/base/span.h>
> >  
> >  #include <libcamera/control_ids.h>
> >  #include <libcamera/controls.h>
> > @@ -74,7 +75,7 @@ public:
> >         virtual BayerFormat::Order bayerOrder(Transform t) const = 0;
> >  
> >         virtual const ControlInfoMap &controls() const = 0;
> > -       virtual ControlList getControls(const std::vector<uint32_t> &ids) = 0;
> > +       virtual ControlList getControls(Span<const uint32_t> ids) = 0;
> >         virtual int setControls(ControlList *ctrls) = 0;
> >  
> >         virtual const std::vector<controls::draft::TestPatternModeEnum> &
> > diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
> > index affe52c2a..4ab818d90 100644
> > --- a/include/libcamera/internal/v4l2_device.h
> > +++ b/include/libcamera/internal/v4l2_device.h
> > @@ -37,7 +37,7 @@ public:
> >  
> >         const ControlInfoMap &controls() const { return controls_; }
> >  
> > -       ControlList getControls(const std::vector<uint32_t> &ids);
> > +       ControlList getControls(Span<const uint32_t> ids);
> >         int setControls(ControlList *ctrls);
> >  
> >         const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;
> > diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > index 32989c19c..1eccca519 100644
> > --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
> > @@ -90,7 +90,7 @@ public:
> >         BayerFormat::Order bayerOrder(Transform t) const override;
> >  
> >         const ControlInfoMap &controls() const override;
> > -       ControlList getControls(const std::vector<uint32_t> &ids) override;
> > +       ControlList getControls(Span<const uint32_t> ids) override;
> >         int setControls(ControlList *ctrls) override;
> >  
> >         const std::vector<controls::draft::TestPatternModeEnum> &
> > @@ -907,9 +907,13 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
> >          * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> >          * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
> >          */
> > -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> > -                                                  V4L2_CID_HBLANK,
> > -                                                  V4L2_CID_VBLANK });
> > +       static constexpr uint32_t cids[] = {
> > +               V4L2_CID_PIXEL_RATE,
> > +               V4L2_CID_HBLANK,
> > +               V4L2_CID_VBLANK,
> > +       };
> > +
> > +       ControlList ctrls = subdev_->getControls(cids);
> >         if (ctrls.empty()) {
> >                 LOG(CameraSensor, Error)
> >                         << "Failed to retrieve camera info controls";
> > @@ -983,7 +987,7 @@ const ControlInfoMap &CameraSensorLegacy::controls() const
> >         return subdev_->controls();
> >  }
> >  
> > -ControlList CameraSensorLegacy::getControls(const std::vector<uint32_t> &ids)
> > +ControlList CameraSensorLegacy::getControls(Span<const uint32_t> ids)
> >  {
> >         return subdev_->getControls(ids);
> >  }
> > diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
> > index ab75b1f82..c65ecbb86 100644
> > --- a/src/libcamera/sensor/camera_sensor_raw.cpp
> > +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
> > @@ -96,7 +96,7 @@ public:
> >         BayerFormat::Order bayerOrder(Transform t) const override;
> >  
> >         const ControlInfoMap &controls() const override;
> > -       ControlList getControls(const std::vector<uint32_t> &ids) override;
> > +       ControlList getControls(Span<const uint32_t> ids) override;
> >         int setControls(ControlList *ctrls) override;
> >  
> >         const std::vector<controls::draft::TestPatternModeEnum> &
> > @@ -1022,9 +1022,13 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
> >          * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
> >          * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
> >          */
> > -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
> > -                                                  V4L2_CID_HBLANK,
> > -                                                  V4L2_CID_VBLANK });
> > +       static constexpr uint32_t cids[] = {
> > +               V4L2_CID_PIXEL_RATE,
> > +               V4L2_CID_HBLANK,
> > +               V4L2_CID_VBLANK,
> > +       };
> > +
> > +       ControlList ctrls = subdev_->getControls(cids);
> >         if (ctrls.empty()) {
> >                 LOG(CameraSensor, Error)
> >                         << "Failed to retrieve camera info controls";
> > @@ -1095,7 +1099,7 @@ const ControlInfoMap &CameraSensorRaw::controls() const
> >         return subdev_->controls();
> >  }
> >  
> > -ControlList CameraSensorRaw::getControls(const std::vector<uint32_t> &ids)
> > +ControlList CameraSensorRaw::getControls(Span<const uint32_t> ids)
> >  {
> >         return subdev_->getControls(ids);
> >  }
> > diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
> > index 2f65a43a0..089f45afd 100644
> > --- a/src/libcamera/v4l2_device.cpp
> > +++ b/src/libcamera/v4l2_device.cpp
> > @@ -174,7 +174,7 @@ void V4L2Device::close()
> >   * \return The control values in a ControlList on success, or an empty list on
> >   * error
> >   */
> > -ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
> > +ControlList V4L2Device::getControls(Span<const uint32_t> ids)
> >  {
> >         if (ids.empty())
> >                 return {};
> > diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
> > index b0130295e..7990f37dc 100644
> > --- a/test/v4l2_videodevice/controls.cpp
> > +++ b/test/v4l2_videodevice/controls.cpp
> > @@ -60,11 +60,15 @@ protected:
> >                 const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
> >  
> >                 /* Test getting controls. */
> > -               ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
> > -                                                           V4L2_CID_CONTRAST,
> > -                                                           V4L2_CID_SATURATION,
> > -                                                           VIVID_CID_INTEGER64,
> > -                                                           VIVID_CID_U8_4D_ARRAY });
> > +               static constexpr uint32_t cids[] = {
> > +                       V4L2_CID_BRIGHTNESS,
> > +                       V4L2_CID_CONTRAST,
> > +                       V4L2_CID_SATURATION,
> > +                       VIVID_CID_INTEGER64,
> > +                       VIVID_CID_U8_4D_ARRAY,
> > +               };
> > +
> > +               ControlList ctrls = capture_->getControls(cids);
> >                 if (ctrls.empty()) {
> >                         cerr << "Failed to get controls" << endl;
> >                         return TestFail;
Barnabás Pőcze July 27, 2025, 6:13 p.m. UTC | #4
Hi

2025. 07. 21. 20:32 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-03-03 19:33:48)
>> Do not force the caller to construct a vector.
>>
> 
> Perhaps we could expand this:
> 
> 
> """
> The V4L2 getControls implementation currently requires a std::vector to
> be constructed.
> 
> Update this to use a Span to make it possible to store static const
> arrays of the required ControlIds to remove dynamic allocation while
> calling for a fixed range of controls.
> 
> Convert relevant callers as part of the change.
> """

My proposal is the following:

"""
The function takes a const std::vector reference, but it does
not actually need an `std::vector`. So use a `libcamera::Span`
instead to avoid forcing the caller to construct a vector.
"""

Regards,
Barnabás Pőcze

> 
> 
> This passed CI in March (for both commits in this series)
> 
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1376848
> 
> And still applies cleanly.
> 
> Running CI now :
> 
> https://gitlab.freedesktop.org/camera/libcamera/-/pipelines/1475482
> 
> Succeeded.
> 
> I think such a change is reasonable.
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   include/libcamera/internal/camera_sensor.h    |  3 ++-
>>   include/libcamera/internal/v4l2_device.h      |  2 +-
>>   src/libcamera/sensor/camera_sensor_legacy.cpp | 14 +++++++++-----
>>   src/libcamera/sensor/camera_sensor_raw.cpp    | 14 +++++++++-----
>>   src/libcamera/v4l2_device.cpp                 |  2 +-
>>   test/v4l2_videodevice/controls.cpp            | 14 +++++++++-----
>>   6 files changed, 31 insertions(+), 18 deletions(-)
>>
>> diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
>> index 13048f327..59a1604c5 100644
>> --- a/include/libcamera/internal/camera_sensor.h
>> +++ b/include/libcamera/internal/camera_sensor.h
>> @@ -14,6 +14,7 @@
>>   #include <vector>
>>   
>>   #include <libcamera/base/class.h>
>> +#include <libcamera/base/span.h>
>>   
>>   #include <libcamera/control_ids.h>
>>   #include <libcamera/controls.h>
>> @@ -74,7 +75,7 @@ public:
>>          virtual BayerFormat::Order bayerOrder(Transform t) const = 0;
>>   
>>          virtual const ControlInfoMap &controls() const = 0;
>> -       virtual ControlList getControls(const std::vector<uint32_t> &ids) = 0;
>> +       virtual ControlList getControls(Span<const uint32_t> ids) = 0;
>>          virtual int setControls(ControlList *ctrls) = 0;
>>   
>>          virtual const std::vector<controls::draft::TestPatternModeEnum> &
>> diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
>> index affe52c2a..4ab818d90 100644
>> --- a/include/libcamera/internal/v4l2_device.h
>> +++ b/include/libcamera/internal/v4l2_device.h
>> @@ -37,7 +37,7 @@ public:
>>   
>>          const ControlInfoMap &controls() const { return controls_; }
>>   
>> -       ControlList getControls(const std::vector<uint32_t> &ids);
>> +       ControlList getControls(Span<const uint32_t> ids);
>>          int setControls(ControlList *ctrls);
>>   
>>          const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;
>> diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> index 32989c19c..1eccca519 100644
>> --- a/src/libcamera/sensor/camera_sensor_legacy.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
>> @@ -90,7 +90,7 @@ public:
>>          BayerFormat::Order bayerOrder(Transform t) const override;
>>   
>>          const ControlInfoMap &controls() const override;
>> -       ControlList getControls(const std::vector<uint32_t> &ids) override;
>> +       ControlList getControls(Span<const uint32_t> ids) override;
>>          int setControls(ControlList *ctrls) override;
>>   
>>          const std::vector<controls::draft::TestPatternModeEnum> &
>> @@ -907,9 +907,13 @@ int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
>>           * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>>           * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>>           */
>> -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
>> -                                                  V4L2_CID_HBLANK,
>> -                                                  V4L2_CID_VBLANK });
>> +       static constexpr uint32_t cids[] = {
>> +               V4L2_CID_PIXEL_RATE,
>> +               V4L2_CID_HBLANK,
>> +               V4L2_CID_VBLANK,
>> +       };
>> +
>> +       ControlList ctrls = subdev_->getControls(cids);
>>          if (ctrls.empty()) {
>>                  LOG(CameraSensor, Error)
>>                          << "Failed to retrieve camera info controls";
>> @@ -983,7 +987,7 @@ const ControlInfoMap &CameraSensorLegacy::controls() const
>>          return subdev_->controls();
>>   }
>>   
>> -ControlList CameraSensorLegacy::getControls(const std::vector<uint32_t> &ids)
>> +ControlList CameraSensorLegacy::getControls(Span<const uint32_t> ids)
>>   {
>>          return subdev_->getControls(ids);
>>   }
>> diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
>> index ab75b1f82..c65ecbb86 100644
>> --- a/src/libcamera/sensor/camera_sensor_raw.cpp
>> +++ b/src/libcamera/sensor/camera_sensor_raw.cpp
>> @@ -96,7 +96,7 @@ public:
>>          BayerFormat::Order bayerOrder(Transform t) const override;
>>   
>>          const ControlInfoMap &controls() const override;
>> -       ControlList getControls(const std::vector<uint32_t> &ids) override;
>> +       ControlList getControls(Span<const uint32_t> ids) override;
>>          int setControls(ControlList *ctrls) override;
>>   
>>          const std::vector<controls::draft::TestPatternModeEnum> &
>> @@ -1022,9 +1022,13 @@ int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
>>           * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
>>           * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
>>           */
>> -       ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
>> -                                                  V4L2_CID_HBLANK,
>> -                                                  V4L2_CID_VBLANK });
>> +       static constexpr uint32_t cids[] = {
>> +               V4L2_CID_PIXEL_RATE,
>> +               V4L2_CID_HBLANK,
>> +               V4L2_CID_VBLANK,
>> +       };
>> +
>> +       ControlList ctrls = subdev_->getControls(cids);
>>          if (ctrls.empty()) {
>>                  LOG(CameraSensor, Error)
>>                          << "Failed to retrieve camera info controls";
>> @@ -1095,7 +1099,7 @@ const ControlInfoMap &CameraSensorRaw::controls() const
>>          return subdev_->controls();
>>   }
>>   
>> -ControlList CameraSensorRaw::getControls(const std::vector<uint32_t> &ids)
>> +ControlList CameraSensorRaw::getControls(Span<const uint32_t> ids)
>>   {
>>          return subdev_->getControls(ids);
>>   }
>> diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
>> index 2f65a43a0..089f45afd 100644
>> --- a/src/libcamera/v4l2_device.cpp
>> +++ b/src/libcamera/v4l2_device.cpp
>> @@ -174,7 +174,7 @@ void V4L2Device::close()
>>    * \return The control values in a ControlList on success, or an empty list on
>>    * error
>>    */
>> -ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
>> +ControlList V4L2Device::getControls(Span<const uint32_t> ids)
>>   {
>>          if (ids.empty())
>>                  return {};
>> diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
>> index b0130295e..7990f37dc 100644
>> --- a/test/v4l2_videodevice/controls.cpp
>> +++ b/test/v4l2_videodevice/controls.cpp
>> @@ -60,11 +60,15 @@ protected:
>>                  const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
>>   
>>                  /* Test getting controls. */
>> -               ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
>> -                                                           V4L2_CID_CONTRAST,
>> -                                                           V4L2_CID_SATURATION,
>> -                                                           VIVID_CID_INTEGER64,
>> -                                                           VIVID_CID_U8_4D_ARRAY });
>> +               static constexpr uint32_t cids[] = {
>> +                       V4L2_CID_BRIGHTNESS,
>> +                       V4L2_CID_CONTRAST,
>> +                       V4L2_CID_SATURATION,
>> +                       VIVID_CID_INTEGER64,
>> +                       VIVID_CID_U8_4D_ARRAY,
>> +               };
>> +
>> +               ControlList ctrls = capture_->getControls(cids);
>>                  if (ctrls.empty()) {
>>                          cerr << "Failed to get controls" << endl;
>>                          return TestFail;
>> -- 
>> 2.48.1
>>

Patch
diff mbox series

diff --git a/include/libcamera/internal/camera_sensor.h b/include/libcamera/internal/camera_sensor.h
index 13048f327..59a1604c5 100644
--- a/include/libcamera/internal/camera_sensor.h
+++ b/include/libcamera/internal/camera_sensor.h
@@ -14,6 +14,7 @@ 
 #include <vector>
 
 #include <libcamera/base/class.h>
+#include <libcamera/base/span.h>
 
 #include <libcamera/control_ids.h>
 #include <libcamera/controls.h>
@@ -74,7 +75,7 @@  public:
 	virtual BayerFormat::Order bayerOrder(Transform t) const = 0;
 
 	virtual const ControlInfoMap &controls() const = 0;
-	virtual ControlList getControls(const std::vector<uint32_t> &ids) = 0;
+	virtual ControlList getControls(Span<const uint32_t> ids) = 0;
 	virtual int setControls(ControlList *ctrls) = 0;
 
 	virtual const std::vector<controls::draft::TestPatternModeEnum> &
diff --git a/include/libcamera/internal/v4l2_device.h b/include/libcamera/internal/v4l2_device.h
index affe52c2a..4ab818d90 100644
--- a/include/libcamera/internal/v4l2_device.h
+++ b/include/libcamera/internal/v4l2_device.h
@@ -37,7 +37,7 @@  public:
 
 	const ControlInfoMap &controls() const { return controls_; }
 
-	ControlList getControls(const std::vector<uint32_t> &ids);
+	ControlList getControls(Span<const uint32_t> ids);
 	int setControls(ControlList *ctrls);
 
 	const struct v4l2_query_ext_ctrl *controlInfo(uint32_t id) const;
diff --git a/src/libcamera/sensor/camera_sensor_legacy.cpp b/src/libcamera/sensor/camera_sensor_legacy.cpp
index 32989c19c..1eccca519 100644
--- a/src/libcamera/sensor/camera_sensor_legacy.cpp
+++ b/src/libcamera/sensor/camera_sensor_legacy.cpp
@@ -90,7 +90,7 @@  public:
 	BayerFormat::Order bayerOrder(Transform t) const override;
 
 	const ControlInfoMap &controls() const override;
-	ControlList getControls(const std::vector<uint32_t> &ids) override;
+	ControlList getControls(Span<const uint32_t> ids) override;
 	int setControls(ControlList *ctrls) override;
 
 	const std::vector<controls::draft::TestPatternModeEnum> &
@@ -907,9 +907,13 @@  int CameraSensorLegacy::sensorInfo(IPACameraSensorInfo *info) const
 	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
 	 * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
 	 */
-	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
-						   V4L2_CID_HBLANK,
-						   V4L2_CID_VBLANK });
+	static constexpr uint32_t cids[] = {
+		V4L2_CID_PIXEL_RATE,
+		V4L2_CID_HBLANK,
+		V4L2_CID_VBLANK,
+	};
+
+	ControlList ctrls = subdev_->getControls(cids);
 	if (ctrls.empty()) {
 		LOG(CameraSensor, Error)
 			<< "Failed to retrieve camera info controls";
@@ -983,7 +987,7 @@  const ControlInfoMap &CameraSensorLegacy::controls() const
 	return subdev_->controls();
 }
 
-ControlList CameraSensorLegacy::getControls(const std::vector<uint32_t> &ids)
+ControlList CameraSensorLegacy::getControls(Span<const uint32_t> ids)
 {
 	return subdev_->getControls(ids);
 }
diff --git a/src/libcamera/sensor/camera_sensor_raw.cpp b/src/libcamera/sensor/camera_sensor_raw.cpp
index ab75b1f82..c65ecbb86 100644
--- a/src/libcamera/sensor/camera_sensor_raw.cpp
+++ b/src/libcamera/sensor/camera_sensor_raw.cpp
@@ -96,7 +96,7 @@  public:
 	BayerFormat::Order bayerOrder(Transform t) const override;
 
 	const ControlInfoMap &controls() const override;
-	ControlList getControls(const std::vector<uint32_t> &ids) override;
+	ControlList getControls(Span<const uint32_t> ids) override;
 	int setControls(ControlList *ctrls) override;
 
 	const std::vector<controls::draft::TestPatternModeEnum> &
@@ -1022,9 +1022,13 @@  int CameraSensorRaw::sensorInfo(IPACameraSensorInfo *info) const
 	 * duration through V4L2 controls. Support for the V4L2_CID_PIXEL_RATE,
 	 * V4L2_CID_HBLANK and V4L2_CID_VBLANK controls is mandatory.
 	 */
-	ControlList ctrls = subdev_->getControls({ V4L2_CID_PIXEL_RATE,
-						   V4L2_CID_HBLANK,
-						   V4L2_CID_VBLANK });
+	static constexpr uint32_t cids[] = {
+		V4L2_CID_PIXEL_RATE,
+		V4L2_CID_HBLANK,
+		V4L2_CID_VBLANK,
+	};
+
+	ControlList ctrls = subdev_->getControls(cids);
 	if (ctrls.empty()) {
 		LOG(CameraSensor, Error)
 			<< "Failed to retrieve camera info controls";
@@ -1095,7 +1099,7 @@  const ControlInfoMap &CameraSensorRaw::controls() const
 	return subdev_->controls();
 }
 
-ControlList CameraSensorRaw::getControls(const std::vector<uint32_t> &ids)
+ControlList CameraSensorRaw::getControls(Span<const uint32_t> ids)
 {
 	return subdev_->getControls(ids);
 }
diff --git a/src/libcamera/v4l2_device.cpp b/src/libcamera/v4l2_device.cpp
index 2f65a43a0..089f45afd 100644
--- a/src/libcamera/v4l2_device.cpp
+++ b/src/libcamera/v4l2_device.cpp
@@ -174,7 +174,7 @@  void V4L2Device::close()
  * \return The control values in a ControlList on success, or an empty list on
  * error
  */
-ControlList V4L2Device::getControls(const std::vector<uint32_t> &ids)
+ControlList V4L2Device::getControls(Span<const uint32_t> ids)
 {
 	if (ids.empty())
 		return {};
diff --git a/test/v4l2_videodevice/controls.cpp b/test/v4l2_videodevice/controls.cpp
index b0130295e..7990f37dc 100644
--- a/test/v4l2_videodevice/controls.cpp
+++ b/test/v4l2_videodevice/controls.cpp
@@ -60,11 +60,15 @@  protected:
 		const ControlInfo &u8 = infoMap.find(VIVID_CID_U8_4D_ARRAY)->second;
 
 		/* Test getting controls. */
-		ControlList ctrls = capture_->getControls({ V4L2_CID_BRIGHTNESS,
-							    V4L2_CID_CONTRAST,
-							    V4L2_CID_SATURATION,
-							    VIVID_CID_INTEGER64,
-							    VIVID_CID_U8_4D_ARRAY });
+		static constexpr uint32_t cids[] = {
+			V4L2_CID_BRIGHTNESS,
+			V4L2_CID_CONTRAST,
+			V4L2_CID_SATURATION,
+			VIVID_CID_INTEGER64,
+			VIVID_CID_U8_4D_ARRAY,
+		};
+
+		ControlList ctrls = capture_->getControls(cids);
 		if (ctrls.empty()) {
 			cerr << "Failed to get controls" << endl;
 			return TestFail;