| Message ID | 20250303193349.785692-1-barnabas.pocze@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
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 >
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;
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;
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 >>
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;
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(-)