[libcamera-devel,RFC,v3,1/2] libcamera: camera: Take span of StreamRole instead of vector
diff mbox series

Message ID 20230509221750.1116531-1-pobrn@protonmail.com
State Accepted
Headers show
Series
  • [libcamera-devel,RFC,v3,1/2] libcamera: camera: Take span of StreamRole instead of vector
Related show

Commit Message

Barnabás Pőcze May 9, 2023, 11:07 p.m. UTC 
Change the parameter type of `generateConfiguration()` from `const std::vector&`
to `libcamera::Span`. A span is almost always preferable to a const vector ref
because it does not force dynamic allocation when none are needed, and it allows
any contiguous container to be used.

A new overload is added that accepts an initializer list so that

  cam->generateConfiguration({ ... })

keeps working.

There is no API break since a span can be constructed from a vector
and the initializer list overload takes care of the initializer lists,
but this change causes an ABI break.

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---
 Documentation/guides/pipeline-handler.rst           |  4 ++--
 include/libcamera/camera.h                          | 12 +++++++++++-
 include/libcamera/internal/pipeline_handler.h       |  2 +-
 src/libcamera/camera.cpp                            |  8 +++++++-
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
 src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
 src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
 src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
 src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
 src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
 src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
 src/py/libcamera/py_main.cpp                        |  5 ++++-
 13 files changed, 39 insertions(+), 20 deletions(-)

Comments

Kieran Bingham May 15, 2023, 11:28 p.m. UTC | #1 
Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> Change the parameter type of `generateConfiguration()` from `const std::vector&`
> to `libcamera::Span`. A span is almost always preferable to a const vector ref
> because it does not force dynamic allocation when none are needed, and it allows
> any contiguous container to be used.

Sounds like a reasonable move to me.

> 
> A new overload is added that accepts an initializer list so that
> 
>   cam->generateConfiguration({ ... })
> 
> keeps working.
> 
> There is no API break since a span can be constructed from a vector
> and the initializer list overload takes care of the initializer lists,
> but this change causes an ABI break.

That's fine too. I'd be interested how the abi-compliance-checker
reports this:

 Creating compatibility report ...
 Binary compatibility: 100%
 Source compatibility: 99.9%
 Total binary compatibility problems: 0, warnings: 0
 Total source compatibility problems: 2, warnings: 0
 Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html

Well - at least it noticed something ;-)


> 
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>  Documentation/guides/pipeline-handler.rst           |  4 ++--
>  include/libcamera/camera.h                          | 12 +++++++++++-
>  include/libcamera/internal/pipeline_handler.h       |  2 +-
>  src/libcamera/camera.cpp                            |  8 +++++++-
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
>  src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
>  src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
>  src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
>  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
>  src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
>  src/py/libcamera/py_main.cpp                        |  5 ++++-
>  13 files changed, 39 insertions(+), 20 deletions(-)
> 
> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> index 57644534..10b9c75c 100644
> --- a/Documentation/guides/pipeline-handler.rst
> +++ b/Documentation/guides/pipeline-handler.rst
> @@ -203,7 +203,7 @@ implementations for the overridden class members.
>            PipelineHandlerVivid(CameraManager *manager);
>  
>            CameraConfiguration *generateConfiguration(Camera *camera,
> -          const StreamRoles &roles) override;
> +          Span<const StreamRole> roles) override;
>            int configure(Camera *camera, CameraConfiguration *config) override;
>  
>            int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -223,7 +223,7 @@ implementations for the overridden class members.
>     }
>  
>     CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
> -                                                                    const StreamRoles &roles)
> +                                                                    Span<const StreamRole> roles)
>     {
>            return nullptr;
>     }
> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> index 5bb06584..004bc894 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -7,6 +7,7 @@
>  
>  #pragma once
>  
> +#include <initializer_list>
>  #include <memory>
>  #include <set>
>  #include <stdint.h>
> @@ -105,7 +106,16 @@ public:
>         const ControlList &properties() const;
>  
>         const std::set<Stream *> &streams() const;
> -       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> +
> +       std::unique_ptr<CameraConfiguration>
> +       generateConfiguration(Span<const StreamRole> roles = {});
> +
> +       std::unique_ptr<CameraConfiguration>
> +       generateConfiguration(std::initializer_list<StreamRole> roles)
> +       {
> +               return generateConfiguration(Span(roles.begin(), roles.end()));
> +       }
> +
>         int configure(CameraConfiguration *config);
>  
>         std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index 4c4dfe62..aaeb3a9e 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -49,7 +49,7 @@ public:
>         void release(Camera *camera);
>  
>         virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) = 0;
> +               Span<const StreamRole> roles) = 0;
>         virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>  
>         virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> index 99683e49..cf9ca01e 100644
> --- a/src/libcamera/camera.cpp
> +++ b/src/libcamera/camera.cpp
> @@ -938,7 +938,7 @@ const std::set<Stream *> &Camera::streams() const
>   * \return A CameraConfiguration if the requested roles can be satisfied, or a
>   * null pointer otherwise.
>   */
> -std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> +std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
>  {
>         Private *const d = _d();
>  
> @@ -971,6 +971,12 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
>         return config;
>  }
>  
> +/**
> + * \fn std::unique_ptr<CameraConfiguration> \
> + *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
> + * \overload
> + */
> +
>  /**
>   * \brief Configure the camera prior to capture
>   * \param[in] config The camera configurations to setup
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 449d9012..9bdfff0b 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -104,7 +104,7 @@ public:
>         bool match(DeviceEnumerator *enumerator) override;
>  
>         std::unique_ptr<CameraConfiguration>
> -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -739,7 +739,7 @@ StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
>  
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerISI::generateConfiguration(Camera *camera,
> -                                         const StreamRoles &roles)
> +                                         Span<const StreamRole> roles)
>  {
>         ISICameraData *data = cameraData(camera);
>         std::unique_ptr<ISICameraConfiguration> config =
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 355cb0cb..ada8c272 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -135,7 +135,7 @@ public:
>         PipelineHandlerIPU3(CameraManager *manager);
>  
>         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) override;
> +               Span<const StreamRole> roles) override;

Checkstyle is quite noisy on the alignments of these. But you haven't
changed the alignments...

I might be tempted to say we should align to checkstyle when applying
just to keep the style differences down.

>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -390,7 +390,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>  }
>  
>  std::unique_ptr<CameraConfiguration>
> -PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
> +PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>  {
>         IPU3CameraData *data = cameraData(camera);
>         std::unique_ptr<IPU3CameraConfiguration> config =
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 8a30fe06..1fdfde7b 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -148,7 +148,7 @@ public:
>         PipelineHandlerRkISP1(CameraManager *manager);
>  
>         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) override;
> +               Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -609,7 +609,7 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>  
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> -       const StreamRoles &roles)
> +       Span<const StreamRole> roles)
>  {
>         RkISP1CameraData *data = cameraData(camera);
>  
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> index ba1797bc..d7f1d547 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> @@ -381,7 +381,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
>  }
>  
>  std::unique_ptr<CameraConfiguration>
> -PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
> +PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>  {
>         CameraData *data = cameraData(camera);
>         std::unique_ptr<CameraConfiguration> config =
> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> index 6b19b56c..f648e810 100644
> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> @@ -214,7 +214,7 @@ public:
>                                                    BayerFormat::Packing packingReq);
>  
>         std::unique_ptr<CameraConfiguration>
> -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 050285fd..b9858353 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -316,7 +316,7 @@ public:
>         SimplePipelineHandler(CameraManager *manager);
>  
>         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) override;
> +               Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -1044,7 +1044,7 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
>  }
>  
>  std::unique_ptr<CameraConfiguration>
> -SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
> +SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>  {
>         SimpleCameraData *data = cameraData(camera);
>         std::unique_ptr<CameraConfiguration> config =
> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> index 277465b7..03935876 100644
> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> @@ -75,7 +75,7 @@ public:
>         PipelineHandlerUVC(CameraManager *manager);
>  
>         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) override;
> +               Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -180,7 +180,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>  
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerUVC::generateConfiguration(Camera *camera,
> -       const StreamRoles &roles)
> +       Span<const StreamRole> roles)
>  {
>         UVCCameraData *data = cameraData(camera);
>         std::unique_ptr<CameraConfiguration> config =
> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> index 204f5ad7..49ee949f 100644
> --- a/src/libcamera/pipeline/vimc/vimc.cpp
> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> @@ -85,7 +85,7 @@ public:
>         PipelineHandlerVimc(CameraManager *manager);
>  
>         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> -               const StreamRoles &roles) override;
> +               Span<const StreamRole> roles) override;
>         int configure(Camera *camera, CameraConfiguration *config) override;
>  
>         int exportFrameBuffers(Camera *camera, Stream *stream,
> @@ -191,7 +191,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>  
>  std::unique_ptr<CameraConfiguration>
>  PipelineHandlerVimc::generateConfiguration(Camera *camera,
> -       const StreamRoles &roles)
> +       Span<const StreamRole> roles)
>  {
>         VimcCameraData *data = cameraData(camera);
>         std::unique_ptr<CameraConfiguration> config =
> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> index d14e18e2..3f04871f 100644
> --- a/src/py/libcamera/py_main.cpp
> +++ b/src/py/libcamera/py_main.cpp
> @@ -156,7 +156,10 @@ PYBIND11_MODULE(_libcamera, m)
>                 })
>  
>                 /* Keep the camera alive, as StreamConfiguration contains a Stream* */
> -               .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
> +               .def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
> +                       return self.generateConfiguration(roles);
> +               }, py::keep_alive<0, 1>())
> +

That's the only bit that I'm not sure how to fully parse the
need/change... but it doesn't look out of place - It would be nice to
see this tested or reviewed by someone handling more of the python parts
but other wise:


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

>                 .def("configure", &Camera::configure)
>  
>                 .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
> -- 
> 2.40.1
> 
>
Barnabás Pőcze May 18, 2023, 3:17 p.m. UTC | #2 
Hi


2023. május 16., kedd 1:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> > Change the parameter type of `generateConfiguration()` from `const std::vector&`
> > to `libcamera::Span`. A span is almost always preferable to a const vector ref
> > because it does not force dynamic allocation when none are needed, and it allows
> > any contiguous container to be used.
> 
> Sounds like a reasonable move to me.
> 
> >
> > A new overload is added that accepts an initializer list so that
> >
> >   cam->generateConfiguration({ ... })
> >
> > keeps working.
> >
> > There is no API break since a span can be constructed from a vector
> > and the initializer list overload takes care of the initializer lists,
> > but this change causes an ABI break.
> 
> That's fine too. I'd be interested how the abi-compliance-checker
> reports this:
> 
>  Creating compatibility report ...
>  Binary compatibility: 100%
>  Source compatibility: 99.9%
>  Total binary compatibility problems: 0, warnings: 0
>  Total source compatibility problems: 2, warnings: 0
>  Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
> 
> Well - at least it noticed something ;-)

Unfortunately I think that is incorrect. This is definitely an ABI break.
(And as far as I can tell not an API break.)


> 
> 
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  Documentation/guides/pipeline-handler.rst           |  4 ++--
> >  include/libcamera/camera.h                          | 12 +++++++++++-
> >  include/libcamera/internal/pipeline_handler.h       |  2 +-
> >  src/libcamera/camera.cpp                            |  8 +++++++-
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
> >  src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
> >  src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
> >  src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
> >  src/py/libcamera/py_main.cpp                        |  5 ++++-
> >  13 files changed, 39 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index 57644534..10b9c75c 100644
> > --- a/Documentation/guides/pipeline-handler.rst
> > +++ b/Documentation/guides/pipeline-handler.rst
> > @@ -203,7 +203,7 @@ implementations for the overridden class members.
> >            PipelineHandlerVivid(CameraManager *manager);
> >
> >            CameraConfiguration *generateConfiguration(Camera *camera,
> > -          const StreamRoles &roles) override;
> > +          Span<const StreamRole> roles) override;
> >            int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >            int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -223,7 +223,7 @@ implementations for the overridden class members.
> >     }
> >
> >     CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
> > -                                                                    const StreamRoles &roles)
> > +                                                                    Span<const StreamRole> roles)
> >     {
> >            return nullptr;
> >     }
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 5bb06584..004bc894 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> >
> >  #pragma once
> >
> > +#include <initializer_list>
> >  #include <memory>
> >  #include <set>
> >  #include <stdint.h>
> > @@ -105,7 +106,16 @@ public:
> >         const ControlList &properties() const;
> >
> >         const std::set<Stream *> &streams() const;
> > -       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > +
> > +       std::unique_ptr<CameraConfiguration>
> > +       generateConfiguration(Span<const StreamRole> roles = {});
> > +
> > +       std::unique_ptr<CameraConfiguration>
> > +       generateConfiguration(std::initializer_list<StreamRole> roles)
> > +       {
> > +               return generateConfiguration(Span(roles.begin(), roles.end()));
> > +       }
> > +
> >         int configure(CameraConfiguration *config);
> >
> >         std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 4c4dfe62..aaeb3a9e 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -49,7 +49,7 @@ public:
> >         void release(Camera *camera);
> >
> >         virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) = 0;
> > +               Span<const StreamRole> roles) = 0;
> >         virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >
> >         virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 99683e49..cf9ca01e 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -938,7 +938,7 @@ const std::set<Stream *> &Camera::streams() const
> >   * \return A CameraConfiguration if the requested roles can be satisfied, or a
> >   * null pointer otherwise.
> >   */
> > -std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> > +std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
> >  {
> >         Private *const d = _d();
> >
> > @@ -971,6 +971,12 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> >         return config;
> >  }
> >
> > +/**
> > + * \fn std::unique_ptr<CameraConfiguration> \
> > + *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
> > + * \overload
> > + */
> > +
> >  /**
> >   * \brief Configure the camera prior to capture
> >   * \param[in] config The camera configurations to setup
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 449d9012..9bdfff0b 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -104,7 +104,7 @@ public:
> >         bool match(DeviceEnumerator *enumerator) override;
> >
> >         std::unique_ptr<CameraConfiguration>
> > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -739,7 +739,7 @@ StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
> >
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerISI::generateConfiguration(Camera *camera,
> > -                                         const StreamRoles &roles)
> > +                                         Span<const StreamRole> roles)
> >  {
> >         ISICameraData *data = cameraData(camera);
> >         std::unique_ptr<ISICameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 355cb0cb..ada8c272 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -135,7 +135,7 @@ public:
> >         PipelineHandlerIPU3(CameraManager *manager);
> >
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> 
> Checkstyle is quite noisy on the alignments of these. But you haven't
> changed the alignments...
> 
> I might be tempted to say we should align to checkstyle when applying
> just to keep the style differences down.

So what should I do here?



> 
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -390,7 +390,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> >  }
> >
> >  std::unique_ptr<CameraConfiguration>
> > -PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         IPU3CameraData *data = cameraData(camera);
> >         std::unique_ptr<IPU3CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 8a30fe06..1fdfde7b 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -148,7 +148,7 @@ public:
> >         PipelineHandlerRkISP1(CameraManager *manager);
> >
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -609,7 +609,7 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> >
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         RkISP1CameraData *data = cameraData(camera);
> >
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index ba1797bc..d7f1d547 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -381,7 +381,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
> >  }
> >
> >  std::unique_ptr<CameraConfiguration>
> > -PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         CameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index 6b19b56c..f648e810 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -214,7 +214,7 @@ public:
> >                                                    BayerFormat::Packing packingReq);
> >
> >         std::unique_ptr<CameraConfiguration>
> > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 050285fd..b9858353 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -316,7 +316,7 @@ public:
> >         SimplePipelineHandler(CameraManager *manager);
> >
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -1044,7 +1044,7 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
> >  }
> >
> >  std::unique_ptr<CameraConfiguration>
> > -SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         SimpleCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 277465b7..03935876 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -75,7 +75,7 @@ public:
> >         PipelineHandlerUVC(CameraManager *manager);
> >
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -180,7 +180,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         UVCCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 204f5ad7..49ee949f 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -85,7 +85,7 @@ public:
> >         PipelineHandlerVimc(CameraManager *manager);
> >
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -191,7 +191,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> >
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         VimcCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > index d14e18e2..3f04871f 100644
> > --- a/src/py/libcamera/py_main.cpp
> > +++ b/src/py/libcamera/py_main.cpp
> > @@ -156,7 +156,10 @@ PYBIND11_MODULE(_libcamera, m)
> >                 })
> >
> >                 /* Keep the camera alive, as StreamConfiguration contains a Stream* */
> > -               .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
> > +               .def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
> > +                       return self.generateConfiguration(roles);
> > +               }, py::keep_alive<0, 1>())
> > +
> 
> That's the only bit that I'm not sure how to fully parse the
> need/change... but it doesn't look out of place - It would be nice to
> see this tested or reviewed by someone handling more of the python parts
> but other wise:

As far as I understand, pybind11 provides (de)serializers for some standard STL
types, std::vector is one of them, but it does not work with libcamera::Span
out of the box of course.
(https://pybind11.readthedocs.io/en/latest/advanced/cast/stl.html)


> 
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> >                 .def("configure", &Camera::configure)
> >
> >                 .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
> > --
> > 2.40.1
> >
> >
> 


Regards,
Barnabás Pőcze
Kieran Bingham May 18, 2023, 4:16 p.m. UTC | #3 
Quoting Barnabás Pőcze (2023-05-18 16:17:41)
> Hi
> 
> 
> 2023. május 16., kedd 1:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> > > Change the parameter type of `generateConfiguration()` from `const std::vector&`
> > > to `libcamera::Span`. A span is almost always preferable to a const vector ref
> > > because it does not force dynamic allocation when none are needed, and it allows
> > > any contiguous container to be used.
> > 
> > Sounds like a reasonable move to me.
> > 
> > >
> > > A new overload is added that accepts an initializer list so that
> > >
> > >   cam->generateConfiguration({ ... })
> > >
> > > keeps working.
> > >
> > > There is no API break since a span can be constructed from a vector
> > > and the initializer list overload takes care of the initializer lists,
> > > but this change causes an ABI break.
> > 
> > That's fine too. I'd be interested how the abi-compliance-checker
> > reports this:
> > 
> >  Creating compatibility report ...
> >  Binary compatibility: 100%
> >  Source compatibility: 99.9%
> >  Total binary compatibility problems: 0, warnings: 0
> >  Total source compatibility problems: 2, warnings: 0
> >  Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
> > 
> > Well - at least it noticed something ;-)
> 
> Unfortunately I think that is incorrect. This is definitely an ABI break.
> (And as far as I can tell not an API break.)
> 

Hrm... a little worrying for the abi-compliance-checker then...



> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >  Documentation/guides/pipeline-handler.rst           |  4 ++--
> > >  include/libcamera/camera.h                          | 12 +++++++++++-
> > >  include/libcamera/internal/pipeline_handler.h       |  2 +-
> > >  src/libcamera/camera.cpp                            |  8 +++++++-
> > >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
> > >  src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
> > >  src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
> > >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
> > >  src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
> > >  src/py/libcamera/py_main.cpp                        |  5 ++++-
> > >  13 files changed, 39 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > > index 57644534..10b9c75c 100644
> > > --- a/Documentation/guides/pipeline-handler.rst
> > > +++ b/Documentation/guides/pipeline-handler.rst
> > > @@ -203,7 +203,7 @@ implementations for the overridden class members.
> > >            PipelineHandlerVivid(CameraManager *manager);
> > >
> > >            CameraConfiguration *generateConfiguration(Camera *camera,
> > > -          const StreamRoles &roles) override;
> > > +          Span<const StreamRole> roles) override;
> > >            int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >            int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -223,7 +223,7 @@ implementations for the overridden class members.
> > >     }
> > >
> > >     CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
> > > -                                                                    const StreamRoles &roles)
> > > +                                                                    Span<const StreamRole> roles)
> > >     {
> > >            return nullptr;
> > >     }
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 5bb06584..004bc894 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -7,6 +7,7 @@
> > >
> > >  #pragma once
> > >
> > > +#include <initializer_list>
> > >  #include <memory>
> > >  #include <set>
> > >  #include <stdint.h>
> > > @@ -105,7 +106,16 @@ public:
> > >         const ControlList &properties() const;
> > >
> > >         const std::set<Stream *> &streams() const;
> > > -       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > > +
> > > +       std::unique_ptr<CameraConfiguration>
> > > +       generateConfiguration(Span<const StreamRole> roles = {});
> > > +
> > > +       std::unique_ptr<CameraConfiguration>
> > > +       generateConfiguration(std::initializer_list<StreamRole> roles)
> > > +       {
> > > +               return generateConfiguration(Span(roles.begin(), roles.end()));
> > > +       }
> > > +
> > >         int configure(CameraConfiguration *config);
> > >
> > >         std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > > index 4c4dfe62..aaeb3a9e 100644
> > > --- a/include/libcamera/internal/pipeline_handler.h
> > > +++ b/include/libcamera/internal/pipeline_handler.h
> > > @@ -49,7 +49,7 @@ public:
> > >         void release(Camera *camera);
> > >
> > >         virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) = 0;
> > > +               Span<const StreamRole> roles) = 0;
> > >         virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> > >
> > >         virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 99683e49..cf9ca01e 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -938,7 +938,7 @@ const std::set<Stream *> &Camera::streams() const
> > >   * \return A CameraConfiguration if the requested roles can be satisfied, or a
> > >   * null pointer otherwise.
> > >   */
> > > -std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> > > +std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
> > >  {
> > >         Private *const d = _d();
> > >
> > > @@ -971,6 +971,12 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> > >         return config;
> > >  }
> > >
> > > +/**
> > > + * \fn std::unique_ptr<CameraConfiguration> \
> > > + *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
> > > + * \overload
> > > + */
> > > +
> > >  /**
> > >   * \brief Configure the camera prior to capture
> > >   * \param[in] config The camera configurations to setup
> > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > index 449d9012..9bdfff0b 100644
> > > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > @@ -104,7 +104,7 @@ public:
> > >         bool match(DeviceEnumerator *enumerator) override;
> > >
> > >         std::unique_ptr<CameraConfiguration>
> > > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -739,7 +739,7 @@ StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > >  PipelineHandlerISI::generateConfiguration(Camera *camera,
> > > -                                         const StreamRoles &roles)
> > > +                                         Span<const StreamRole> roles)
> > >  {
> > >         ISICameraData *data = cameraData(camera);
> > >         std::unique_ptr<ISICameraConfiguration> config =
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 355cb0cb..ada8c272 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -135,7 +135,7 @@ public:
> > >         PipelineHandlerIPU3(CameraManager *manager);
> > >
> > >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) override;
> > > +               Span<const StreamRole> roles) override;
> > 
> > Checkstyle is quite noisy on the alignments of these. But you haven't
> > changed the alignments...
> > 
> > I might be tempted to say we should align to checkstyle when applying
> > just to keep the style differences down.
> 
> So what should I do here?

Up to you. You can fix the alignment to match checkstyle's desires, or
we can do it after or when applying the patch.

Depends if we actually need a second revision of this. I haven't seen
anything needing a change yet?

 
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -390,7 +390,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> > >  }
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > > -PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > > +PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> > >  {
> > >         IPU3CameraData *data = cameraData(camera);
> > >         std::unique_ptr<IPU3CameraConfiguration> config =
> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > index 8a30fe06..1fdfde7b 100644
> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > > @@ -148,7 +148,7 @@ public:
> > >         PipelineHandlerRkISP1(CameraManager *manager);
> > >
> > >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) override;
> > > +               Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -609,7 +609,7 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > >  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > > -       const StreamRoles &roles)
> > > +       Span<const StreamRole> roles)
> > >  {
> > >         RkISP1CameraData *data = cameraData(camera);
> > >
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > index ba1797bc..d7f1d547 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > > @@ -381,7 +381,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
> > >  }
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > > -PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > > +PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> > >  {
> > >         CameraData *data = cameraData(camera);
> > >         std::unique_ptr<CameraConfiguration> config =
> > > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > index 6b19b56c..f648e810 100644
> > > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > > @@ -214,7 +214,7 @@ public:
> > >                                                    BayerFormat::Packing packingReq);
> > >
> > >         std::unique_ptr<CameraConfiguration>
> > > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
> > > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 050285fd..b9858353 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -316,7 +316,7 @@ public:
> > >         SimplePipelineHandler(CameraManager *manager);
> > >
> > >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) override;
> > > +               Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -1044,7 +1044,7 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
> > >  }
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > > -SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > > +SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> > >  {
> > >         SimpleCameraData *data = cameraData(camera);
> > >         std::unique_ptr<CameraConfiguration> config =
> > > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > index 277465b7..03935876 100644
> > > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > > @@ -75,7 +75,7 @@ public:
> > >         PipelineHandlerUVC(CameraManager *manager);
> > >
> > >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) override;
> > > +               Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -180,7 +180,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > >  PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > > -       const StreamRoles &roles)
> > > +       Span<const StreamRole> roles)
> > >  {
> > >         UVCCameraData *data = cameraData(camera);
> > >         std::unique_ptr<CameraConfiguration> config =
> > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > > index 204f5ad7..49ee949f 100644
> > > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > > @@ -85,7 +85,7 @@ public:
> > >         PipelineHandlerVimc(CameraManager *manager);
> > >
> > >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > > -               const StreamRoles &roles) override;
> > > +               Span<const StreamRole> roles) override;
> > >         int configure(Camera *camera, CameraConfiguration *config) override;
> > >
> > >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > > @@ -191,7 +191,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > >  PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > > -       const StreamRoles &roles)
> > > +       Span<const StreamRole> roles)
> > >  {
> > >         VimcCameraData *data = cameraData(camera);
> > >         std::unique_ptr<CameraConfiguration> config =
> > > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > > index d14e18e2..3f04871f 100644
> > > --- a/src/py/libcamera/py_main.cpp
> > > +++ b/src/py/libcamera/py_main.cpp
> > > @@ -156,7 +156,10 @@ PYBIND11_MODULE(_libcamera, m)
> > >                 })
> > >
> > >                 /* Keep the camera alive, as StreamConfiguration contains a Stream* */
> > > -               .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
> > > +               .def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
> > > +                       return self.generateConfiguration(roles);
> > > +               }, py::keep_alive<0, 1>())
> > > +
> > 
> > That's the only bit that I'm not sure how to fully parse the
> > need/change... but it doesn't look out of place - It would be nice to
> > see this tested or reviewed by someone handling more of the python parts
> > but other wise:
> 
> As far as I understand, pybind11 provides (de)serializers for some standard STL
> types, std::vector is one of them, but it does not work with libcamera::Span
> out of the box of course.
> (https://pybind11.readthedocs.io/en/latest/advanced/cast/stl.html)
> 
> 
> > 
> > 
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > 
> > >                 .def("configure", &Camera::configure)
> > >
> > >                 .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
> > > --
> > > 2.40.1
> > >
> > >
> > 
> 
> 
> Regards,
> Barnabás Pőcze
Barnabás Pőcze May 25, 2023, 10:41 p.m. UTC | #4 
Hi


2023. május 18., csütörtök 18:16 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> Quoting Barnabás Pőcze (2023-05-18 16:17:41)
> > Hi
> >
> >
> > 2023. május 16., kedd 1:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> >
> > > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> > > > Change the parameter type of `generateConfiguration()` from `const std::vector&`
> > > > to `libcamera::Span`. A span is almost always preferable to a const vector ref
> > > > because it does not force dynamic allocation when none are needed, and it allows
> > > > any contiguous container to be used.
> > >
> > > Sounds like a reasonable move to me.
> > >
> > > >
> > > > A new overload is added that accepts an initializer list so that
> > > >
> > > >   cam->generateConfiguration({ ... })
> > > >
> > > > keeps working.
> > > >
> > > > There is no API break since a span can be constructed from a vector
> > > > and the initializer list overload takes care of the initializer lists,
> > > > but this change causes an ABI break.
> > >
> > > That's fine too. I'd be interested how the abi-compliance-checker
> > > reports this:
> > >
> > >  Creating compatibility report ...
> > >  Binary compatibility: 100%
> > >  Source compatibility: 99.9%
> > >  Total binary compatibility problems: 0, warnings: 0
> > >  Total source compatibility problems: 2, warnings: 0
> > >  Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
> > >
> > > Well - at least it noticed something ;-)
> >
> > Unfortunately I think that is incorrect. This is definitely an ABI break.
> > (And as far as I can tell not an API break.)
> >
> 
> Hrm... a little worrying for the abi-compliance-checker then...

Well, to be fair, it *is* an API break as well, but very unlikely to be
noticed by anyone. I don't know why it does not notice the ABI break,
the mangled names are completely different...


> [...]


Regards,
Barnabás Pőcze
Nicolas Dufresne May 30, 2023, 2:44 p.m. UTC | #5 
Hi,

Le jeudi 25 mai 2023 à 22:41 +0000, Barnabás Pőcze via libcamera-devel a écrit :
> Hi
> 
> 
> 2023. május 18., csütörtök 18:16 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > Quoting Barnabás Pőcze (2023-05-18 16:17:41)
> > > Hi
> > > 
> > > 
> > > 2023. május 16., kedd 1:28 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> > > 
> > > > Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> > > > > Change the parameter type of `generateConfiguration()` from `const std::vector&`
> > > > > to `libcamera::Span`. A span is almost always preferable to a const vector ref
> > > > > because it does not force dynamic allocation when none are needed, and it allows
> > > > > any contiguous container to be used.
> > > > 
> > > > Sounds like a reasonable move to me.
> > > > 
> > > > > 
> > > > > A new overload is added that accepts an initializer list so that
> > > > > 
> > > > >   cam->generateConfiguration({ ... })
> > > > > 
> > > > > keeps working.
> > > > > 
> > > > > There is no API break since a span can be constructed from a vector
> > > > > and the initializer list overload takes care of the initializer lists,
> > > > > but this change causes an ABI break.
> > > > 
> > > > That's fine too. I'd be interested how the abi-compliance-checker
> > > > reports this:
> > > > 
> > > >  Creating compatibility report ...
> > > >  Binary compatibility: 100%
> > > >  Source compatibility: 99.9%
> > > >  Total binary compatibility problems: 0, warnings: 0
> > > >  Total source compatibility problems: 2, warnings: 0
> > > >  Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
> > > > 
> > > > Well - at least it noticed something ;-)
> > > 
> > > Unfortunately I think that is incorrect. This is definitely an ABI break.
> > > (And as far as I can tell not an API break.)
> > > 
> > 
> > Hrm... a little worrying for the abi-compliance-checker then...
> 
> Well, to be fair, it *is* an API break as well, but very unlikely to be
> noticed by anyone. I don't know why it does not notice the ABI break,
> the mangled names are completely different...

Removing symbols is an API break. Changing the size of a structure in a way that
both the application and the lib may endup with different size interpretation is
an ABI break.

I think what you are are saying is that this is build compatible with the old
version, avoiding the pain of porting, but a rebuilt is needed in order to use
the new symbols (which in C++ are generated, but they remains API in the context
of generic checkers).

> 
> 
> > [...]
> 
> 
> Regards,
> Barnabás Pőcze
Laurent Pinchart May 30, 2023, 5:03 p.m. UTC | #6 
Hello,

(CC'ing Tomi)

On Tue, May 16, 2023 at 12:28:11AM +0100, Kieran Bingham via libcamera-devel wrote:
> Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
> > Change the parameter type of `generateConfiguration()` from `const std::vector&`
> > to `libcamera::Span`. A span is almost always preferable to a const vector ref
> > because it does not force dynamic allocation when none are needed, and it allows
> > any contiguous container to be used.
> 
> Sounds like a reasonable move to me.
> 
> > A new overload is added that accepts an initializer list so that
> > 
> >   cam->generateConfiguration({ ... })
> > 
> > keeps working.
> > 
> > There is no API break since a span can be constructed from a vector
> > and the initializer list overload takes care of the initializer lists,
> > but this change causes an ABI break.
> 
> That's fine too. I'd be interested how the abi-compliance-checker
> reports this:
> 
>  Creating compatibility report ...
>  Binary compatibility: 100%
>  Source compatibility: 99.9%
>  Total binary compatibility problems: 0, warnings: 0
>  Total source compatibility problems: 2, warnings: 0
>  Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
> 
> Well - at least it noticed something ;-)
> 
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >  Documentation/guides/pipeline-handler.rst           |  4 ++--
> >  include/libcamera/camera.h                          | 12 +++++++++++-
> >  include/libcamera/internal/pipeline_handler.h       |  2 +-
> >  src/libcamera/camera.cpp                            |  8 +++++++-
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
> >  src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
> >  src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
> >  src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
> >  src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
> >  src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
> >  src/py/libcamera/py_main.cpp                        |  5 ++++-
> >  13 files changed, 39 insertions(+), 20 deletions(-)
> > 
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index 57644534..10b9c75c 100644
> > --- a/Documentation/guides/pipeline-handler.rst
> > +++ b/Documentation/guides/pipeline-handler.rst
> > @@ -203,7 +203,7 @@ implementations for the overridden class members.
> >            PipelineHandlerVivid(CameraManager *manager);
> >  
> >            CameraConfiguration *generateConfiguration(Camera *camera,
> > -          const StreamRoles &roles) override;
> > +          Span<const StreamRole> roles) override;
> >            int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >            int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -223,7 +223,7 @@ implementations for the overridden class members.
> >     }
> >  
> >     CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
> > -                                                                    const StreamRoles &roles)
> > +                                                                    Span<const StreamRole> roles)
> >     {
> >            return nullptr;
> >     }
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 5bb06584..004bc894 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -7,6 +7,7 @@
> >  
> >  #pragma once
> >  
> > +#include <initializer_list>
> >  #include <memory>
> >  #include <set>
> >  #include <stdint.h>
> > @@ -105,7 +106,16 @@ public:
> >         const ControlList &properties() const;
> >  
> >         const std::set<Stream *> &streams() const;
> > -       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
> > +
> > +       std::unique_ptr<CameraConfiguration>
> > +       generateConfiguration(Span<const StreamRole> roles = {});
> > +
> > +       std::unique_ptr<CameraConfiguration>
> > +       generateConfiguration(std::initializer_list<StreamRole> roles)
> > +       {
> > +               return generateConfiguration(Span(roles.begin(), roles.end()));
> > +       }
> > +
> >         int configure(CameraConfiguration *config);
> >  
> >         std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index 4c4dfe62..aaeb3a9e 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -49,7 +49,7 @@ public:
> >         void release(Camera *camera);
> >  
> >         virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) = 0;
> > +               Span<const StreamRole> roles) = 0;
> >         virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
> >  
> >         virtual int exportFrameBuffers(Camera *camera, Stream *stream,
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 99683e49..cf9ca01e 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -938,7 +938,7 @@ const std::set<Stream *> &Camera::streams() const
> >   * \return A CameraConfiguration if the requested roles can be satisfied, or a
> >   * null pointer otherwise.
> >   */
> > -std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
> > +std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
> >  {
> >         Private *const d = _d();
> >  
> > @@ -971,6 +971,12 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
> >         return config;
> >  }
> >  
> > +/**
> > + * \fn std::unique_ptr<CameraConfiguration> \
> > + *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
> > + * \overload
> > + */
> > +
> >  /**
> >   * \brief Configure the camera prior to capture
> >   * \param[in] config The camera configurations to setup
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 449d9012..9bdfff0b 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -104,7 +104,7 @@ public:
> >         bool match(DeviceEnumerator *enumerator) override;
> >  
> >         std::unique_ptr<CameraConfiguration>
> > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -739,7 +739,7 @@ StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
> >  
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerISI::generateConfiguration(Camera *camera,
> > -                                         const StreamRoles &roles)
> > +                                         Span<const StreamRole> roles)
> >  {
> >         ISICameraData *data = cameraData(camera);
> >         std::unique_ptr<ISICameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 355cb0cb..ada8c272 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -135,7 +135,7 @@ public:
> >         PipelineHandlerIPU3(CameraManager *manager);
> >  
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> 
> Checkstyle is quite noisy on the alignments of these. But you haven't
> changed the alignments...
> 
> I might be tempted to say we should align to checkstyle when applying
> just to keep the style differences down.
> 
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -390,7 +390,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
> >  }
> >  
> >  std::unique_ptr<CameraConfiguration>
> > -PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         IPU3CameraData *data = cameraData(camera);
> >         std::unique_ptr<IPU3CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 8a30fe06..1fdfde7b 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -148,7 +148,7 @@ public:
> >         PipelineHandlerRkISP1(CameraManager *manager);
> >  
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -609,7 +609,7 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
> >  
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         RkISP1CameraData *data = cameraData(camera);
> >  
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > index ba1797bc..d7f1d547 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
> > @@ -381,7 +381,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
> >  }
> >  
> >  std::unique_ptr<CameraConfiguration>
> > -PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         CameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > index 6b19b56c..f648e810 100644
> > --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
> > @@ -214,7 +214,7 @@ public:
> >                                                    BayerFormat::Packing packingReq);
> >  
> >         std::unique_ptr<CameraConfiguration>
> > -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
> > diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 050285fd..b9858353 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -316,7 +316,7 @@ public:
> >         SimplePipelineHandler(CameraManager *manager);
> >  
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -1044,7 +1044,7 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
> >  }
> >  
> >  std::unique_ptr<CameraConfiguration>
> > -SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >         SimpleCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > index 277465b7..03935876 100644
> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
> > @@ -75,7 +75,7 @@ public:
> >         PipelineHandlerUVC(CameraManager *manager);
> >  
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -180,7 +180,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
> >  
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         UVCCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
> > index 204f5ad7..49ee949f 100644
> > --- a/src/libcamera/pipeline/vimc/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc/vimc.cpp
> > @@ -85,7 +85,7 @@ public:
> >         PipelineHandlerVimc(CameraManager *manager);
> >  
> >         std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
> > -               const StreamRoles &roles) override;
> > +               Span<const StreamRole> roles) override;
> >         int configure(Camera *camera, CameraConfiguration *config) override;
> >  
> >         int exportFrameBuffers(Camera *camera, Stream *stream,
> > @@ -191,7 +191,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
> >  
> >  std::unique_ptr<CameraConfiguration>
> >  PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > -       const StreamRoles &roles)
> > +       Span<const StreamRole> roles)
> >  {
> >         VimcCameraData *data = cameraData(camera);
> >         std::unique_ptr<CameraConfiguration> config =
> > diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
> > index d14e18e2..3f04871f 100644
> > --- a/src/py/libcamera/py_main.cpp
> > +++ b/src/py/libcamera/py_main.cpp
> > @@ -156,7 +156,10 @@ PYBIND11_MODULE(_libcamera, m)
> >                 })
> >  
> >                 /* Keep the camera alive, as StreamConfiguration contains a Stream* */
> > -               .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
> > +               .def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
> > +                       return self.generateConfiguration(roles);
> > +               }, py::keep_alive<0, 1>())
> > +
> 
> That's the only bit that I'm not sure how to fully parse the
> need/change... but it doesn't look out of place - It would be nice to
> see this tested or reviewed by someone handling more of the python parts
> but other wise:

Tomi, could you review this part ?

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

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

> >                 .def("configure", &Camera::configure)
> >  
> >                 .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
Tomi Valkeinen May 30, 2023, 5:42 p.m. UTC | #7 
On 30/05/2023 20:03, Laurent Pinchart wrote:
> Hello,
> 
> (CC'ing Tomi)
> 
> On Tue, May 16, 2023 at 12:28:11AM +0100, Kieran Bingham via libcamera-devel wrote:
>> Quoting Barnabás Pőcze via libcamera-devel (2023-05-10 00:07:57)
>>> Change the parameter type of `generateConfiguration()` from `const std::vector&`
>>> to `libcamera::Span`. A span is almost always preferable to a const vector ref
>>> because it does not force dynamic allocation when none are needed, and it allows
>>> any contiguous container to be used.
>>
>> Sounds like a reasonable move to me.
>>
>>> A new overload is added that accepts an initializer list so that
>>>
>>>    cam->generateConfiguration({ ... })
>>>
>>> keeps working.
>>>
>>> There is no API break since a span can be constructed from a vector
>>> and the initializer list overload takes care of the initializer lists,
>>> but this change causes an ABI break.
>>
>> That's fine too. I'd be interested how the abi-compliance-checker
>> reports this:
>>
>>   Creating compatibility report ...
>>   Binary compatibility: 100%
>>   Source compatibility: 99.9%
>>   Total binary compatibility problems: 0, warnings: 0
>>   Total source compatibility problems: 2, warnings: 0
>>   Report: compat_reports/libcamera/v0.0.5_to_v0.0.5-22-g5c94fbcc9d05/compat_report.html
>>
>> Well - at least it noticed something ;-)
>>
>>> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
>>> ---
>>>   Documentation/guides/pipeline-handler.rst           |  4 ++--
>>>   include/libcamera/camera.h                          | 12 +++++++++++-
>>>   include/libcamera/internal/pipeline_handler.h       |  2 +-
>>>   src/libcamera/camera.cpp                            |  8 +++++++-
>>>   src/libcamera/pipeline/imx8-isi/imx8-isi.cpp        |  4 ++--
>>>   src/libcamera/pipeline/ipu3/ipu3.cpp                |  4 ++--
>>>   src/libcamera/pipeline/rkisp1/rkisp1.cpp            |  4 ++--
>>>   src/libcamera/pipeline/rpi/common/pipeline_base.cpp |  2 +-
>>>   src/libcamera/pipeline/rpi/common/pipeline_base.h   |  2 +-
>>>   src/libcamera/pipeline/simple/simple.cpp            |  4 ++--
>>>   src/libcamera/pipeline/uvcvideo/uvcvideo.cpp        |  4 ++--
>>>   src/libcamera/pipeline/vimc/vimc.cpp                |  4 ++--
>>>   src/py/libcamera/py_main.cpp                        |  5 ++++-
>>>   13 files changed, 39 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
>>> index 57644534..10b9c75c 100644
>>> --- a/Documentation/guides/pipeline-handler.rst
>>> +++ b/Documentation/guides/pipeline-handler.rst
>>> @@ -203,7 +203,7 @@ implementations for the overridden class members.
>>>             PipelineHandlerVivid(CameraManager *manager);
>>>   
>>>             CameraConfiguration *generateConfiguration(Camera *camera,
>>> -          const StreamRoles &roles) override;
>>> +          Span<const StreamRole> roles) override;
>>>             int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>             int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -223,7 +223,7 @@ implementations for the overridden class members.
>>>      }
>>>   
>>>      CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
>>> -                                                                    const StreamRoles &roles)
>>> +                                                                    Span<const StreamRole> roles)
>>>      {
>>>             return nullptr;
>>>      }
>>> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
>>> index 5bb06584..004bc894 100644
>>> --- a/include/libcamera/camera.h
>>> +++ b/include/libcamera/camera.h
>>> @@ -7,6 +7,7 @@
>>>   
>>>   #pragma once
>>>   
>>> +#include <initializer_list>
>>>   #include <memory>
>>>   #include <set>
>>>   #include <stdint.h>
>>> @@ -105,7 +106,16 @@ public:
>>>          const ControlList &properties() const;
>>>   
>>>          const std::set<Stream *> &streams() const;
>>> -       std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
>>> +
>>> +       std::unique_ptr<CameraConfiguration>
>>> +       generateConfiguration(Span<const StreamRole> roles = {});
>>> +
>>> +       std::unique_ptr<CameraConfiguration>
>>> +       generateConfiguration(std::initializer_list<StreamRole> roles)
>>> +       {
>>> +               return generateConfiguration(Span(roles.begin(), roles.end()));
>>> +       }
>>> +
>>>          int configure(CameraConfiguration *config);
>>>   
>>>          std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> index 4c4dfe62..aaeb3a9e 100644
>>> --- a/include/libcamera/internal/pipeline_handler.h
>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>> @@ -49,7 +49,7 @@ public:
>>>          void release(Camera *camera);
>>>   
>>>          virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) = 0;
>>> +               Span<const StreamRole> roles) = 0;
>>>          virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
>>>   
>>>          virtual int exportFrameBuffers(Camera *camera, Stream *stream,
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 99683e49..cf9ca01e 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -938,7 +938,7 @@ const std::set<Stream *> &Camera::streams() const
>>>    * \return A CameraConfiguration if the requested roles can be satisfied, or a
>>>    * null pointer otherwise.
>>>    */
>>> -std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
>>> +std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
>>>   {
>>>          Private *const d = _d();
>>>   
>>> @@ -971,6 +971,12 @@ std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
>>>          return config;
>>>   }
>>>   
>>> +/**
>>> + * \fn std::unique_ptr<CameraConfiguration> \
>>> + *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
>>> + * \overload
>>> + */
>>> +
>>>   /**
>>>    * \brief Configure the camera prior to capture
>>>    * \param[in] config The camera configurations to setup
>>> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> index 449d9012..9bdfff0b 100644
>>> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
>>> @@ -104,7 +104,7 @@ public:
>>>          bool match(DeviceEnumerator *enumerator) override;
>>>   
>>>          std::unique_ptr<CameraConfiguration>
>>> -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
>>> +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -739,7 +739,7 @@ StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>>   PipelineHandlerISI::generateConfiguration(Camera *camera,
>>> -                                         const StreamRoles &roles)
>>> +                                         Span<const StreamRole> roles)
>>>   {
>>>          ISICameraData *data = cameraData(camera);
>>>          std::unique_ptr<ISICameraConfiguration> config =
>>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> index 355cb0cb..ada8c272 100644
>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
>>> @@ -135,7 +135,7 @@ public:
>>>          PipelineHandlerIPU3(CameraManager *manager);
>>>   
>>>          std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) override;
>>> +               Span<const StreamRole> roles) override;
>>
>> Checkstyle is quite noisy on the alignments of these. But you haven't
>> changed the alignments...
>>
>> I might be tempted to say we should align to checkstyle when applying
>> just to keep the style differences down.
>>
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -390,7 +390,7 @@ PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
>>>   }
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>> -PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
>>> +PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>>>   {
>>>          IPU3CameraData *data = cameraData(camera);
>>>          std::unique_ptr<IPU3CameraConfiguration> config =
>>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> index 8a30fe06..1fdfde7b 100644
>>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>>> @@ -148,7 +148,7 @@ public:
>>>          PipelineHandlerRkISP1(CameraManager *manager);
>>>   
>>>          std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) override;
>>> +               Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -609,7 +609,7 @@ PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>>   PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
>>> -       const StreamRoles &roles)
>>> +       Span<const StreamRole> roles)
>>>   {
>>>          RkISP1CameraData *data = cameraData(camera);
>>>   
>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> index ba1797bc..d7f1d547 100644
>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
>>> @@ -381,7 +381,7 @@ V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
>>>   }
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>> -PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
>>> +PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>>>   {
>>>          CameraData *data = cameraData(camera);
>>>          std::unique_ptr<CameraConfiguration> config =
>>> diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
>>> index 6b19b56c..f648e810 100644
>>> --- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
>>> +++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
>>> @@ -214,7 +214,7 @@ public:
>>>                                                     BayerFormat::Packing packingReq);
>>>   
>>>          std::unique_ptr<CameraConfiguration>
>>> -       generateConfiguration(Camera *camera, const StreamRoles &roles) override;
>>> +       generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
>>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
>>> index 050285fd..b9858353 100644
>>> --- a/src/libcamera/pipeline/simple/simple.cpp
>>> +++ b/src/libcamera/pipeline/simple/simple.cpp
>>> @@ -316,7 +316,7 @@ public:
>>>          SimplePipelineHandler(CameraManager *manager);
>>>   
>>>          std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) override;
>>> +               Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -1044,7 +1044,7 @@ SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
>>>   }
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>> -SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
>>> +SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>>>   {
>>>          SimpleCameraData *data = cameraData(camera);
>>>          std::unique_ptr<CameraConfiguration> config =
>>> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> index 277465b7..03935876 100644
>>> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
>>> @@ -75,7 +75,7 @@ public:
>>>          PipelineHandlerUVC(CameraManager *manager);
>>>   
>>>          std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) override;
>>> +               Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -180,7 +180,7 @@ PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>>   PipelineHandlerUVC::generateConfiguration(Camera *camera,
>>> -       const StreamRoles &roles)
>>> +       Span<const StreamRole> roles)
>>>   {
>>>          UVCCameraData *data = cameraData(camera);
>>>          std::unique_ptr<CameraConfiguration> config =
>>> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
>>> index 204f5ad7..49ee949f 100644
>>> --- a/src/libcamera/pipeline/vimc/vimc.cpp
>>> +++ b/src/libcamera/pipeline/vimc/vimc.cpp
>>> @@ -85,7 +85,7 @@ public:
>>>          PipelineHandlerVimc(CameraManager *manager);
>>>   
>>>          std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
>>> -               const StreamRoles &roles) override;
>>> +               Span<const StreamRole> roles) override;
>>>          int configure(Camera *camera, CameraConfiguration *config) override;
>>>   
>>>          int exportFrameBuffers(Camera *camera, Stream *stream,
>>> @@ -191,7 +191,7 @@ PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
>>>   
>>>   std::unique_ptr<CameraConfiguration>
>>>   PipelineHandlerVimc::generateConfiguration(Camera *camera,
>>> -       const StreamRoles &roles)
>>> +       Span<const StreamRole> roles)
>>>   {
>>>          VimcCameraData *data = cameraData(camera);
>>>          std::unique_ptr<CameraConfiguration> config =
>>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>>> index d14e18e2..3f04871f 100644
>>> --- a/src/py/libcamera/py_main.cpp
>>> +++ b/src/py/libcamera/py_main.cpp
>>> @@ -156,7 +156,10 @@ PYBIND11_MODULE(_libcamera, m)
>>>                  })
>>>   
>>>                  /* Keep the camera alive, as StreamConfiguration contains a Stream* */
>>> -               .def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
>>> +               .def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
>>> +                       return self.generateConfiguration(roles);
>>> +               }, py::keep_alive<0, 1>())
>>> +
>>
>> That's the only bit that I'm not sure how to fully parse the
>> need/change... but it doesn't look out of place - It would be nice to
>> see this tested or reviewed by someone handling more of the python parts
>> but other wise:
> 
> Tomi, could you review this part ?

Looks fine to me.

  Tomi

Patch
diff mbox series 

diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
index 57644534..10b9c75c 100644
--- a/Documentation/guides/pipeline-handler.rst
+++ b/Documentation/guides/pipeline-handler.rst
@@ -203,7 +203,7 @@  implementations for the overridden class members.
           PipelineHandlerVivid(CameraManager *manager);
 
           CameraConfiguration *generateConfiguration(Camera *camera,
-          const StreamRoles &roles) override;
+          Span<const StreamRole> roles) override;
           int configure(Camera *camera, CameraConfiguration *config) override;
 
           int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -223,7 +223,7 @@  implementations for the overridden class members.
    }
 
    CameraConfiguration *PipelineHandlerVivid::generateConfiguration(Camera *camera,
-                                                                    const StreamRoles &roles)
+                                                                    Span<const StreamRole> roles)
    {
           return nullptr;
    }
diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
index 5bb06584..004bc894 100644
--- a/include/libcamera/camera.h
+++ b/include/libcamera/camera.h
@@ -7,6 +7,7 @@ 
 
 #pragma once
 
+#include <initializer_list>
 #include <memory>
 #include <set>
 #include <stdint.h>
@@ -105,7 +106,16 @@  public:
 	const ControlList &properties() const;
 
 	const std::set<Stream *> &streams() const;
-	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRoles &roles = {});
+
+	std::unique_ptr<CameraConfiguration>
+	generateConfiguration(Span<const StreamRole> roles = {});
+
+	std::unique_ptr<CameraConfiguration>
+	generateConfiguration(std::initializer_list<StreamRole> roles)
+	{
+		return generateConfiguration(Span(roles.begin(), roles.end()));
+	}
+
 	int configure(CameraConfiguration *config);
 
 	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
index 4c4dfe62..aaeb3a9e 100644
--- a/include/libcamera/internal/pipeline_handler.h
+++ b/include/libcamera/internal/pipeline_handler.h
@@ -49,7 +49,7 @@  public:
 	void release(Camera *camera);
 
 	virtual std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) = 0;
+		Span<const StreamRole> roles) = 0;
 	virtual int configure(Camera *camera, CameraConfiguration *config) = 0;
 
 	virtual int exportFrameBuffers(Camera *camera, Stream *stream,
diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
index 99683e49..cf9ca01e 100644
--- a/src/libcamera/camera.cpp
+++ b/src/libcamera/camera.cpp
@@ -938,7 +938,7 @@  const std::set<Stream *> &Camera::streams() const
  * \return A CameraConfiguration if the requested roles can be satisfied, or a
  * null pointer otherwise.
  */
-std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamRoles &roles)
+std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(Span<const StreamRole> roles)
 {
 	Private *const d = _d();
 
@@ -971,6 +971,12 @@  std::unique_ptr<CameraConfiguration> Camera::generateConfiguration(const StreamR
 	return config;
 }
 
+/**
+ * \fn std::unique_ptr<CameraConfiguration> \
+ *     Camera::generateConfiguration(std::initializer_list<StreamRole> roles)
+ * \overload
+ */
+
 /**
  * \brief Configure the camera prior to capture
  * \param[in] config The camera configurations to setup
diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
index 449d9012..9bdfff0b 100644
--- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
+++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
@@ -104,7 +104,7 @@  public:
 	bool match(DeviceEnumerator *enumerator) override;
 
 	std::unique_ptr<CameraConfiguration>
-	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
+	generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -739,7 +739,7 @@  StreamConfiguration PipelineHandlerISI::generateRawConfiguration(Camera *camera)
 
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerISI::generateConfiguration(Camera *camera,
-					  const StreamRoles &roles)
+					  Span<const StreamRole> roles)
 {
 	ISICameraData *data = cameraData(camera);
 	std::unique_ptr<ISICameraConfiguration> config =
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index 355cb0cb..ada8c272 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -135,7 +135,7 @@  public:
 	PipelineHandlerIPU3(CameraManager *manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) override;
+		Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -390,7 +390,7 @@  PipelineHandlerIPU3::PipelineHandlerIPU3(CameraManager *manager)
 }
 
 std::unique_ptr<CameraConfiguration>
-PipelineHandlerIPU3::generateConfiguration(Camera *camera, const StreamRoles &roles)
+PipelineHandlerIPU3::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
 {
 	IPU3CameraData *data = cameraData(camera);
 	std::unique_ptr<IPU3CameraConfiguration> config =
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 8a30fe06..1fdfde7b 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -148,7 +148,7 @@  public:
 	PipelineHandlerRkISP1(CameraManager *manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) override;
+		Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -609,7 +609,7 @@  PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager)
 
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
-	const StreamRoles &roles)
+	Span<const StreamRole> roles)
 {
 	RkISP1CameraData *data = cameraData(camera);
 
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
index ba1797bc..d7f1d547 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.cpp
@@ -381,7 +381,7 @@  V4L2DeviceFormat PipelineHandlerBase::toV4L2DeviceFormat(const V4L2VideoDevice *
 }
 
 std::unique_ptr<CameraConfiguration>
-PipelineHandlerBase::generateConfiguration(Camera *camera, const StreamRoles &roles)
+PipelineHandlerBase::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
 {
 	CameraData *data = cameraData(camera);
 	std::unique_ptr<CameraConfiguration> config =
diff --git a/src/libcamera/pipeline/rpi/common/pipeline_base.h b/src/libcamera/pipeline/rpi/common/pipeline_base.h
index 6b19b56c..f648e810 100644
--- a/src/libcamera/pipeline/rpi/common/pipeline_base.h
+++ b/src/libcamera/pipeline/rpi/common/pipeline_base.h
@@ -214,7 +214,7 @@  public:
 						   BayerFormat::Packing packingReq);
 
 	std::unique_ptr<CameraConfiguration>
-	generateConfiguration(Camera *camera, const StreamRoles &roles) override;
+	generateConfiguration(Camera *camera, Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, libcamera::Stream *stream,
diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
index 050285fd..b9858353 100644
--- a/src/libcamera/pipeline/simple/simple.cpp
+++ b/src/libcamera/pipeline/simple/simple.cpp
@@ -316,7 +316,7 @@  public:
 	SimplePipelineHandler(CameraManager *manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) override;
+		Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -1044,7 +1044,7 @@  SimplePipelineHandler::SimplePipelineHandler(CameraManager *manager)
 }
 
 std::unique_ptr<CameraConfiguration>
-SimplePipelineHandler::generateConfiguration(Camera *camera, const StreamRoles &roles)
+SimplePipelineHandler::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
 {
 	SimpleCameraData *data = cameraData(camera);
 	std::unique_ptr<CameraConfiguration> config =
diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
index 277465b7..03935876 100644
--- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
+++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp
@@ -75,7 +75,7 @@  public:
 	PipelineHandlerUVC(CameraManager *manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) override;
+		Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -180,7 +180,7 @@  PipelineHandlerUVC::PipelineHandlerUVC(CameraManager *manager)
 
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerUVC::generateConfiguration(Camera *camera,
-	const StreamRoles &roles)
+	Span<const StreamRole> roles)
 {
 	UVCCameraData *data = cameraData(camera);
 	std::unique_ptr<CameraConfiguration> config =
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp
index 204f5ad7..49ee949f 100644
--- a/src/libcamera/pipeline/vimc/vimc.cpp
+++ b/src/libcamera/pipeline/vimc/vimc.cpp
@@ -85,7 +85,7 @@  public:
 	PipelineHandlerVimc(CameraManager *manager);
 
 	std::unique_ptr<CameraConfiguration> generateConfiguration(Camera *camera,
-		const StreamRoles &roles) override;
+		Span<const StreamRole> roles) override;
 	int configure(Camera *camera, CameraConfiguration *config) override;
 
 	int exportFrameBuffers(Camera *camera, Stream *stream,
@@ -191,7 +191,7 @@  PipelineHandlerVimc::PipelineHandlerVimc(CameraManager *manager)
 
 std::unique_ptr<CameraConfiguration>
 PipelineHandlerVimc::generateConfiguration(Camera *camera,
-	const StreamRoles &roles)
+	Span<const StreamRole> roles)
 {
 	VimcCameraData *data = cameraData(camera);
 	std::unique_ptr<CameraConfiguration> config =
diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
index d14e18e2..3f04871f 100644
--- a/src/py/libcamera/py_main.cpp
+++ b/src/py/libcamera/py_main.cpp
@@ -156,7 +156,10 @@  PYBIND11_MODULE(_libcamera, m)
 		})
 
 		/* Keep the camera alive, as StreamConfiguration contains a Stream* */
-		.def("generate_configuration", &Camera::generateConfiguration, py::keep_alive<0, 1>())
+		.def("generate_configuration", [](Camera &self, const std::vector<StreamRole> &roles) {
+			return self.generateConfiguration(roles);
+		}, py::keep_alive<0, 1>())
+
 		.def("configure", &Camera::configure)
 
 		.def("create_request", &Camera::createRequest, py::arg("cookie") = 0)