[libcamera-devel,RFC,v1] libcamera: camera: Take span of StreamRole instead of vector

18285 diff mbox series

Change the parameter type of `generateConfiguration()`
from an std::vector to libcamera::Span. There is no need
to require a dynamic allocation. A span is preferable
to a const vector ref.

A new overload is added that accepts a C-style array so that

  cam->generateConfiguration({ ... })

keeps working.

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

Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
---

I think the change is reasonable, but there are two questions:

 * what to do with `StreamRoles`? should it be removed?
 * is the array overload needed? if an API break is acceptable
   it can be removed and users could write e.g.
     generateConfiguration(std::array { ... })

---
 Documentation/guides/pipeline-handler.rst          | 4 ++--
 include/libcamera/camera.h                         | 9 ++++++++-
 include/libcamera/internal/pipeline_handler.h      | 2 +-
 src/libcamera/camera.cpp                           | 2 +-
 src/libcamera/pipeline/imx8-isi/imx8-isi.cpp       | 4 ++--
 src/libcamera/pipeline/ipu3/ipu3.cpp               | 4 ++--
 src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
 src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 4 ++--
 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 ++++-
 12 files changed, 30 insertions(+), 20 deletions(-)

--
2.39.1

Hello Barnabás

On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> Date: Wed, 15 Feb 2023 17:48:52 +0000
> From: Barnabás Pőcze <pobrn@protonmail.com>
> To: libcamera-devel@lists.libcamera.org
> Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
>  of vector
>
> Change the parameter type of `generateConfiguration()`
> from an std::vector to libcamera::Span. There is no need
> to require a dynamic allocation. A span is preferable
> to a const vector ref.

I might be missing what the benefit would be here... I understand that, being
a Span nothing but a lightweight container for a pointer to memory and a size,
this allows to use multiple STL containers to pass roles in, but I
wonder if there any real benefit.

I do expect application to either parse user provided options and
thus need to construct a dynamically grown list of roles, or either
pass in an initializers list

> A new overload is added that accepts a C-style array so that
>
>   cam->generateConfiguration({ ... })
>
> keeps working.

For which you need an overload.

Can you expand a bit more on the intended use case ?

Thanks
  j

>
> There is no API break since a span can be constructed from
> a vector and the array overload takes care of the initializer lists,
> but this change causes an ABI break.
>
> Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> ---
>
> I think the change is reasonable, but there are two questions:
>
>  * what to do with `StreamRoles`? should it be removed?
>  * is the array overload needed? if an API break is acceptable
>    it can be removed and users could write e.g.
>      generateConfiguration(std::array { ... })
>
> ---
>  Documentation/guides/pipeline-handler.rst          | 4 ++--
>  include/libcamera/camera.h                         | 9 ++++++++-
>  include/libcamera/internal/pipeline_handler.h      | 2 +-
>  src/libcamera/camera.cpp                           | 2 +-
>  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp       | 4 ++--
>  src/libcamera/pipeline/ipu3/ipu3.cpp               | 4 ++--
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 4 ++--
>  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 ++++-
>  12 files changed, 30 insertions(+), 20 deletions(-)
>
> diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> index e1930fdf..92d6d043 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..ccf0d24c 100644
> --- a/include/libcamera/camera.h
> +++ b/include/libcamera/camera.h
> @@ -105,7 +105,14 @@ 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 = {});
> +
> +	template<std::size_t N>
> +	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRole (&roles)[N])
> +	{
> +		return generateConfiguration(Span<const StreamRole>(roles));
> +	}
> +
>  	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 0da167a7..0f68a511 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();
>
> diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> index 0c67e35d..df5919ed 100644
> --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> @@ -112,7 +112,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,
> @@ -571,7 +571,7 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
>
>  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/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 84120954..f62570cc 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -361,7 +361,7 @@ public:
>  	PipelineHandlerRPi(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,
> @@ -661,7 +661,7 @@ PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
>  }
>
>  std::unique_ptr<CameraConfiguration>
> -PipelineHandlerRPi::generateConfiguration(Camera *camera, const StreamRoles &roles)
> +PipelineHandlerRPi::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
>  {
>  	RPiCameraData *data = cameraData(camera);
>  	std::unique_ptr<CameraConfiguration> 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/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> index 24ded4db..9d71a369 100644
> --- a/src/libcamera/pipeline/simple/simple.cpp
> +++ b/src/libcamera/pipeline/simple/simple.cpp
> @@ -312,7 +312,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,
> @@ -1038,7 +1038,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)
> --
> 2.39.1
>

Hi


2023. február 20., hétfő 9:23 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hello Barnabás
> 
> On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > From: Barnabás Pőcze <pobrn@protonmail.com>
> > To: libcamera-devel@lists.libcamera.org
> > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> >  of vector
> >
> > Change the parameter type of `generateConfiguration()`
> > from an std::vector to libcamera::Span. There is no need
> > to require a dynamic allocation. A span is preferable
> > to a const vector ref.
> 
> I might be missing what the benefit would be here... I understand that, being
> a Span nothing but a lightweight container for a pointer to memory and a size,
> this allows to use multiple STL containers to pass roles in, but I
> wonder if there any real benefit.
> 
> I do expect application to either parse user provided options and
> thus need to construct a dynamically grown list of roles, or either
> pass in an initializers list
> 
> > A new overload is added that accepts a C-style array so that
> >
> >   cam->generateConfiguration({ ... })
> >
> > keeps working.
> 
> For which you need an overload.
> 
> Can you expand a bit more on the intended use case ?

The way I see it, there are two benefits:
 * any container that places elements in contiguous memory should work
   (e.g. std::array and std::vector with a custom allocator, which were
    previously not supported)
 * the cost of constructing an std::vector can be avoided in some cases
   (e.g. when a fixed list of roles is used)

But of course I understand if it is considered a micro optimization
and rejected on that basis.


Best regards,
Barnabás Pőcze

> 
> Thanks
>   j
> 
> >
> > There is no API break since a span can be constructed from
> > a vector and the array overload takes care of the initializer lists,
> > but this change causes an ABI break.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > ---
> >
> > I think the change is reasonable, but there are two questions:
> >
> >  * what to do with `StreamRoles`? should it be removed?
> >  * is the array overload needed? if an API break is acceptable
> >    it can be removed and users could write e.g.
> >      generateConfiguration(std::array { ... })
> >
> > ---
> >  Documentation/guides/pipeline-handler.rst          | 4 ++--
> >  include/libcamera/camera.h                         | 9 ++++++++-
> >  include/libcamera/internal/pipeline_handler.h      | 2 +-
> >  src/libcamera/camera.cpp                           | 2 +-
> >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp       | 4 ++--
> >  src/libcamera/pipeline/ipu3/ipu3.cpp               | 4 ++--
> >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 4 ++--
> >  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 ++++-
> >  12 files changed, 30 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index e1930fdf..92d6d043 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..ccf0d24c 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -105,7 +105,14 @@ 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 = {});
> > +
> > +	template<std::size_t N>
> > +	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRole (&roles)[N])
> > +	{
> > +		return generateConfiguration(Span<const StreamRole>(roles));
> > +	}
> > +
> >  	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 0da167a7..0f68a511 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();
> >
> > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > index 0c67e35d..df5919ed 100644
> > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > @@ -112,7 +112,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,
> > @@ -571,7 +571,7 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
> >
> >  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/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > index 84120954..f62570cc 100644
> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > @@ -361,7 +361,7 @@ public:
> >  	PipelineHandlerRPi(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,
> > @@ -661,7 +661,7 @@ PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> >  }
> >
> >  std::unique_ptr<CameraConfiguration>
> > -PipelineHandlerRPi::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > +PipelineHandlerRPi::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> >  {
> >  	RPiCameraData *data = cameraData(camera);
> >  	std::unique_ptr<CameraConfiguration> 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/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > index 24ded4db..9d71a369 100644
> > --- a/src/libcamera/pipeline/simple/simple.cpp
> > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > @@ -312,7 +312,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,
> > @@ -1038,7 +1038,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)
> > --
> > 2.39.1
> >
>

Hi Barnabás

On Mon, Feb 20, 2023 at 12:58:54PM +0000, Barnabás Pőcze wrote:
> Hi
>
>
> 2023. február 20., hétfő 9:23 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
>
> > Hello Barnabás
> >
> > On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > > From: Barnabás Pőcze <pobrn@protonmail.com>
> > > To: libcamera-devel@lists.libcamera.org
> > > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> > >  of vector
> > >
> > > Change the parameter type of `generateConfiguration()`
> > > from an std::vector to libcamera::Span. There is no need
> > > to require a dynamic allocation. A span is preferable
> > > to a const vector ref.
> >
> > I might be missing what the benefit would be here... I understand that, being
> > a Span nothing but a lightweight container for a pointer to memory and a size,
> > this allows to use multiple STL containers to pass roles in, but I
> > wonder if there any real benefit.
> >
> > I do expect application to either parse user provided options and
> > thus need to construct a dynamically grown list of roles, or either
> > pass in an initializers list
> >
> > > A new overload is added that accepts a C-style array so that
> > >
> > >   cam->generateConfiguration({ ... })
> > >
> > > keeps working.
> >
> > For which you need an overload.
> >
> > Can you expand a bit more on the intended use case ?
>
> The way I see it, there are two benefits:
>  * any container that places elements in contiguous memory should work
>    (e.g. std::array and std::vector with a custom allocator, which were
>     previously not supported)

You're correct, but considering how applications are expected to
construct the StreamRole vector, I hardly see that being strictly
necessary to be done in an array

>  * the cost of constructing an std::vector can be avoided in some cases
>    (e.g. when a fixed list of roles is used)

Correct, but this is not an hard path and the generateConfiguration()
is expected to be called once per streaming session at most

>
> But of course I understand if it is considered a micro optimization
> and rejected on that basis.
>

If this wasn't changing the public API it would have be easier indeed
to accept it.

Let's see what others think

>
> Best regards,
> Barnabás Pőcze
>
> >
> > Thanks
> >   j
> >
> > >
> > > There is no API break since a span can be constructed from
> > > a vector and the array overload takes care of the initializer lists,
> > > but this change causes an ABI break.
> > >
> > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com>
> > > ---
> > >
> > > I think the change is reasonable, but there are two questions:
> > >
> > >  * what to do with `StreamRoles`? should it be removed?
> > >  * is the array overload needed? if an API break is acceptable
> > >    it can be removed and users could write e.g.
> > >      generateConfiguration(std::array { ... })
> > >
> > > ---
> > >  Documentation/guides/pipeline-handler.rst          | 4 ++--
> > >  include/libcamera/camera.h                         | 9 ++++++++-
> > >  include/libcamera/internal/pipeline_handler.h      | 2 +-
> > >  src/libcamera/camera.cpp                           | 2 +-
> > >  src/libcamera/pipeline/imx8-isi/imx8-isi.cpp       | 4 ++--
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp               | 4 ++--
> > >  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 4 ++--
> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp           | 4 ++--
> > >  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 ++++-
> > >  12 files changed, 30 insertions(+), 20 deletions(-)
> > >
> > > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > > index e1930fdf..92d6d043 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..ccf0d24c 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -105,7 +105,14 @@ 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 = {});
> > > +
> > > +	template<std::size_t N>
> > > +	std::unique_ptr<CameraConfiguration> generateConfiguration(const StreamRole (&roles)[N])
> > > +	{
> > > +		return generateConfiguration(Span<const StreamRole>(roles));
> > > +	}
> > > +
> > >  	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 0da167a7..0f68a511 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();
> > >
> > > diff --git a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > index 0c67e35d..df5919ed 100644
> > > --- a/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > +++ b/src/libcamera/pipeline/imx8-isi/imx8-isi.cpp
> > > @@ -112,7 +112,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,
> > > @@ -571,7 +571,7 @@ PipelineHandlerISI::PipelineHandlerISI(CameraManager *manager)
> > >
> > >  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/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > index 84120954..f62570cc 100644
> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> > > @@ -361,7 +361,7 @@ public:
> > >  	PipelineHandlerRPi(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,
> > > @@ -661,7 +661,7 @@ PipelineHandlerRPi::PipelineHandlerRPi(CameraManager *manager)
> > >  }
> > >
> > >  std::unique_ptr<CameraConfiguration>
> > > -PipelineHandlerRPi::generateConfiguration(Camera *camera, const StreamRoles &roles)
> > > +PipelineHandlerRPi::generateConfiguration(Camera *camera, Span<const StreamRole> roles)
> > >  {
> > >  	RPiCameraData *data = cameraData(camera);
> > >  	std::unique_ptr<CameraConfiguration> 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/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp
> > > index 24ded4db..9d71a369 100644
> > > --- a/src/libcamera/pipeline/simple/simple.cpp
> > > +++ b/src/libcamera/pipeline/simple/simple.cpp
> > > @@ -312,7 +312,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,
> > > @@ -1038,7 +1038,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)
> > > --
> > > 2.39.1
> > >
> >

Hi


2023. február 20., hétfő 17:08 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:

> Hi Barnabás
> 
> On Mon, Feb 20, 2023 at 12:58:54PM +0000, Barnabás Pőcze wrote:
> > Hi
> >
> >
> > 2023. február 20., hétfő 9:23 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
> >
> > > Hello Barnabás
> > >
> > > On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > > > From: Barnabás Pőcze <pobrn@protonmail.com>
> > > > To: libcamera-devel@lists.libcamera.org
> > > > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> > > >  of vector
> > > >
> > > > Change the parameter type of `generateConfiguration()`
> > > > from an std::vector to libcamera::Span. There is no need
> > > > to require a dynamic allocation. A span is preferable
> > > > to a const vector ref.
> > >
> > > I might be missing what the benefit would be here... I understand that, being
> > > a Span nothing but a lightweight container for a pointer to memory and a size,
> > > this allows to use multiple STL containers to pass roles in, but I
> > > wonder if there any real benefit.
> > >
> > > I do expect application to either parse user provided options and
> > > thus need to construct a dynamically grown list of roles, or either
> > > pass in an initializers list
> > >
> > > > A new overload is added that accepts a C-style array so that
> > > >
> > > >   cam->generateConfiguration({ ... })
> > > >
> > > > keeps working.
> > >
> > > For which you need an overload.
> > >
> > > Can you expand a bit more on the intended use case ?
> >
> > The way I see it, there are two benefits:
> >  * any container that places elements in contiguous memory should work
> >    (e.g. std::array and std::vector with a custom allocator, which were
> >     previously not supported)
> 
> You're correct, but considering how applications are expected to
> construct the StreamRole vector, I hardly see that being strictly
> necessary to be done in an array
> 
> >  * the cost of constructing an std::vector can be avoided in some cases
> >    (e.g. when a fixed list of roles is used)
> 
> Correct, but this is not an hard path and the generateConfiguration()
> is expected to be called once per streaming session at most
> 
> >
> > But of course I understand if it is considered a micro optimization
> > and rejected on that basis.
> >
> 
> If this wasn't changing the public API it would have be easier indeed
> to accept it.

Technically it changes the API, but I would argue that in such a minimal
way that it is essentially unnoticeable. And as far as I understand
libcamera does not provide stable API/ABI guarantees yet. Am I mistaken?


> 
> Let's see what others think

I am wondering if anyone has other thoughts about this? If not, would it be
possible for me to get a definitive yes/no?


> [...]


Regards,
Barnabás Pőcze

Hi,

Quoting Barnabás Pőcze via libcamera-devel (2023-04-19 15:55:39)
> Hi
> 
> 
> 2023. február 20., hétfő 17:08 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
> 
> > Hi Barnabás
> > 
> > On Mon, Feb 20, 2023 at 12:58:54PM +0000, Barnabás Pőcze wrote:
> > > Hi
> > >
> > >
> > > 2023. február 20., hétfő 9:23 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta:
> > >
> > > > Hello Barnabás
> > > >
> > > > On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > > > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > > > > From: Barnabás Pőcze <pobrn@protonmail.com>
> > > > > To: libcamera-devel@lists.libcamera.org
> > > > > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> > > > >  of vector
> > > > >
> > > > > Change the parameter type of `generateConfiguration()`
> > > > > from an std::vector to libcamera::Span. There is no need
> > > > > to require a dynamic allocation. A span is preferable
> > > > > to a const vector ref.
> > > >
> > > > I might be missing what the benefit would be here... I understand that, being
> > > > a Span nothing but a lightweight container for a pointer to memory and a size,
> > > > this allows to use multiple STL containers to pass roles in, but I
> > > > wonder if there any real benefit.
> > > >
> > > > I do expect application to either parse user provided options and
> > > > thus need to construct a dynamically grown list of roles, or either
> > > > pass in an initializers list
> > > >
> > > > > A new overload is added that accepts a C-style array so that
> > > > >
> > > > >   cam->generateConfiguration({ ... })
> > > > >
> > > > > keeps working.
> > > >
> > > > For which you need an overload.
> > > >
> > > > Can you expand a bit more on the intended use case ?
> > >
> > > The way I see it, there are two benefits:
> > >  * any container that places elements in contiguous memory should work
> > >    (e.g. std::array and std::vector with a custom allocator, which were
> > >     previously not supported)
> > 
> > You're correct, but considering how applications are expected to
> > construct the StreamRole vector, I hardly see that being strictly
> > necessary to be done in an array
> > 
> > >  * the cost of constructing an std::vector can be avoided in some cases
> > >    (e.g. when a fixed list of roles is used)
> > 
> > Correct, but this is not an hard path and the generateConfiguration()
> > is expected to be called once per streaming session at most
> > 
> > >
> > > But of course I understand if it is considered a micro optimization
> > > and rejected on that basis.
> > >
> > 
> > If this wasn't changing the public API it would have be easier indeed
> > to accept it.
> 
> Technically it changes the API, but I would argue that in such a minimal
> way that it is essentially unnoticeable. And as far as I understand
> libcamera does not provide stable API/ABI guarantees yet. Am I mistaken?
> 
> 
> > 
> > Let's see what others think
> 
> I am wondering if anyone has other thoughts about this? If not, would it be
> possible for me to get a definitive yes/no?

No specific objection to it here, This will also require an update to
the vivid pipeline handler, but as that's external we'd have to handle
that separately.

Looking at the way StreamRoles is used in applications, it's already
mostly a dynamic allocation and that is passed by reference into
libcamera.

Is there a specific target to this to remove allocations for a given
use-case? (I know getting rid of allocations at runtime is highly
desireable for real time performance for instance).

The ABI change here is not an issue, and even though it's an API
'change' it's still compatible as far as I can tell - so no objection
here either.

I think Jacopo asked a question about the existing StreamRoles 'using'
statement. Would you see that as something we should keep? or remove
(or deprecate?) with this change?

--
Kieran



> 
> 
> > [...]
> 
> 
> Regards,
> Barnabás Pőcze

Hi


2023. április 19., szerda 23:52 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:

> [...]
> > > > > On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > > > > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > > > > > From: Barnabás Pőcze <pobrn@protonmail.com>
> > > > > > To: libcamera-devel@lists.libcamera.org
> > > > > > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> > > > > >  of vector
> > > > > >
> > > > > > Change the parameter type of `generateConfiguration()`
> > > > > > from an std::vector to libcamera::Span. There is no need
> > > > > > to require a dynamic allocation. A span is preferable
> > > > > > to a const vector ref.
> > > > >
> > > > > I might be missing what the benefit would be here... I understand that, being
> > > > > a Span nothing but a lightweight container for a pointer to memory and a size,
> > > > > this allows to use multiple STL containers to pass roles in, but I
> > > > > wonder if there any real benefit.
> > > > >
> > > > > I do expect application to either parse user provided options and
> > > > > thus need to construct a dynamically grown list of roles, or either
> > > > > pass in an initializers list
> > > > >
> > > > > > A new overload is added that accepts a C-style array so that
> > > > > >
> > > > > >   cam->generateConfiguration({ ... })
> > > > > >
> > > > > > keeps working.
> > > > >
> > > > > For which you need an overload.
> > > > >
> > > > > Can you expand a bit more on the intended use case ?
> > > >
> > > > The way I see it, there are two benefits:
> > > >  * any container that places elements in contiguous memory should work
> > > >    (e.g. std::array and std::vector with a custom allocator, which were
> > > >     previously not supported)
> > >
> > > You're correct, but considering how applications are expected to
> > > construct the StreamRole vector, I hardly see that being strictly
> > > necessary to be done in an array
> > >
> > > >  * the cost of constructing an std::vector can be avoided in some cases
> > > >    (e.g. when a fixed list of roles is used)
> > >
> > > Correct, but this is not an hard path and the generateConfiguration()
> > > is expected to be called once per streaming session at most
> > >
> > > >
> > > > But of course I understand if it is considered a micro optimization
> > > > and rejected on that basis.
> > > >
> > >
> > > If this wasn't changing the public API it would have be easier indeed
> > > to accept it.
> >
> > Technically it changes the API, but I would argue that in such a minimal
> > way that it is essentially unnoticeable. And as far as I understand
> > libcamera does not provide stable API/ABI guarantees yet. Am I mistaken?
> >
> >
> > >
> > > Let's see what others think
> >
> > I am wondering if anyone has other thoughts about this? If not, would it be
> > possible for me to get a definitive yes/no?
> 
> No specific objection to it here, This will also require an update to
> the vivid pipeline handler, but as that's external we'd have to handle
> that separately.
> 
> Looking at the way StreamRoles is used in applications, it's already
> mostly a dynamic allocation and that is passed by reference into
> libcamera.
> 
> Is there a specific target to this to remove allocations for a given
> use-case? (I know getting rid of allocations at runtime is highly
> desireable for real time performance for instance).

The motivating example is in pipewire[0]. It made me wonder if there is
actually a reason for using a vector here. And as far as I could tell,
there wasn't one, so this patch was born. I would even wager to say that
in the vast majority of cases (like here), a `const std::vector<T>&` argument
can be replaced by `std::span<const T>` without significant consequences
since what is a `const std::vector<T>&` if not a worse `std::span<const T>`
(there are exceptions, of course)?

So my motivations for this patch were the following:

 - I thought this was a non-intrusive change;
 - it can get rid of the need for constructing the vector in certain cases.

And of course I am not arguing that this changes the world, but it is such
a simple change, that I thought "why not?". And as I have said I do understand
if this is considered a micro-optimization and rejected on that basis.


> 
> The ABI change here is not an issue, and even though it's an API
> 'change' it's still compatible as far as I can tell - so no objection
> here either.
> 
> I think Jacopo asked a question about the existing StreamRoles 'using'
> statement. Would you see that as something we should keep? or remove
> (or deprecate?) with this change?

There are no API stability guarantees, right? So I would personally remove it, but
that might be too harsh of an approach. According to Debian Code Search, it is only
found in pipewire and libcamera[1]. (There are other users, I am sure.)


> [...]


Regards,
Barnabás Pőcze


[0]: https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/83d2e85f490ea97e4ae94b95f20dd06566a14c31/spa/plugins/libcamera/libcamera-utils.cpp#L58
[1]: https://codesearch.debian.net/search?q=StreamRoles&literal=1

Quoting Barnabás Pőcze (2023-04-20 12:35:15)
> Hi
> 
> 
> 2023. április 19., szerda 23:52 keltezéssel, Kieran Bingham <kieran.bingham@ideasonboard.com> írta:
> 
> > [...]
> > > > > > On Wed, Feb 15, 2023 at 05:48:52PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > > > > > > Date: Wed, 15 Feb 2023 17:48:52 +0000
> > > > > > > From: Barnabás Pőcze <pobrn@protonmail.com>
> > > > > > > To: libcamera-devel@lists.libcamera.org
> > > > > > > Subject: [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead
> > > > > > >  of vector
> > > > > > >
> > > > > > > Change the parameter type of `generateConfiguration()`
> > > > > > > from an std::vector to libcamera::Span. There is no need
> > > > > > > to require a dynamic allocation. A span is preferable
> > > > > > > to a const vector ref.
> > > > > >
> > > > > > I might be missing what the benefit would be here... I understand that, being
> > > > > > a Span nothing but a lightweight container for a pointer to memory and a size,
> > > > > > this allows to use multiple STL containers to pass roles in, but I
> > > > > > wonder if there any real benefit.
> > > > > >
> > > > > > I do expect application to either parse user provided options and
> > > > > > thus need to construct a dynamically grown list of roles, or either
> > > > > > pass in an initializers list
> > > > > >
> > > > > > > A new overload is added that accepts a C-style array so that
> > > > > > >
> > > > > > >   cam->generateConfiguration({ ... })
> > > > > > >
> > > > > > > keeps working.
> > > > > >
> > > > > > For which you need an overload.
> > > > > >
> > > > > > Can you expand a bit more on the intended use case ?
> > > > >
> > > > > The way I see it, there are two benefits:
> > > > >  * any container that places elements in contiguous memory should work
> > > > >    (e.g. std::array and std::vector with a custom allocator, which were
> > > > >     previously not supported)
> > > >
> > > > You're correct, but considering how applications are expected to
> > > > construct the StreamRole vector, I hardly see that being strictly
> > > > necessary to be done in an array
> > > >
> > > > >  * the cost of constructing an std::vector can be avoided in some cases
> > > > >    (e.g. when a fixed list of roles is used)
> > > >
> > > > Correct, but this is not an hard path and the generateConfiguration()
> > > > is expected to be called once per streaming session at most
> > > >
> > > > >
> > > > > But of course I understand if it is considered a micro optimization
> > > > > and rejected on that basis.
> > > > >
> > > >
> > > > If this wasn't changing the public API it would have be easier indeed
> > > > to accept it.
> > >
> > > Technically it changes the API, but I would argue that in such a minimal
> > > way that it is essentially unnoticeable. And as far as I understand
> > > libcamera does not provide stable API/ABI guarantees yet. Am I mistaken?
> > >
> > >
> > > >
> > > > Let's see what others think
> > >
> > > I am wondering if anyone has other thoughts about this? If not, would it be
> > > possible for me to get a definitive yes/no?
> > 
> > No specific objection to it here, This will also require an update to
> > the vivid pipeline handler, but as that's external we'd have to handle
> > that separately.
> > 
> > Looking at the way StreamRoles is used in applications, it's already
> > mostly a dynamic allocation and that is passed by reference into
> > libcamera.
> > 
> > Is there a specific target to this to remove allocations for a given
> > use-case? (I know getting rid of allocations at runtime is highly
> > desireable for real time performance for instance).
> 
> The motivating example is in pipewire[0]. It made me wonder if there is
> actually a reason for using a vector here. And as far as I could tell,

[0] is helpful context thanks. Without that - it's hard to understand
the motivation that's all. We can make changes in the API - but without
understanding 'why' it's hard to quantify, or see how it makes
applications better.

[0] to me looks like a clear case that the user API can be improved with
this indeed.


> there wasn't one, so this patch was born. I would even wager to say that
> in the vast majority of cases (like here), a `const std::vector<T>&` argument
> can be replaced by `std::span<const T>` without significant consequences
> since what is a `const std::vector<T>&` if not a worse `std::span<const T>`
> (there are exceptions, of course)?
> 
> So my motivations for this patch were the following:
> 
>  - I thought this was a non-intrusive change;
>  - it can get rid of the need for constructing the vector in certain cases.
> 
> And of course I am not arguing that this changes the world, but it is such
> a simple change, that I thought "why not?". And as I have said I do understand
> if this is considered a micro-optimization and rejected on that basis.

I think this change sounds reasonable - it's just the original patch
felt like it was presented as "we can do this so why not" rather than
showing "why do" instead.

Internally it's easier, but when we change the public API - a
perspective of how it makes things better for the consumers is helpful.



> > The ABI change here is not an issue, and even though it's an API
> > 'change' it's still compatible as far as I can tell - so no objection
> > here either.
> > 
> > I think Jacopo asked a question about the existing StreamRoles 'using'
> > statement. Would you see that as something we should keep? or remove
> > (or deprecate?) with this change?
> 
> There are no API stability guarantees, right? So I would personally remove it, but
> that might be too harsh of an approach. According to Debian Code Search, it is only
> found in pipewire and libcamera[1]. (There are other users, I am sure.)

No - no API guarantee right now indeed. For exactly this reason. But
that's also why I think this change needs to consider 'what is right'
overall.

My worry is if we make the 'StreamRoles' type redundant, then it should
be considered or I worry that it will be confusing that some places use
"std::span<StreamRole>" while other points use "StreamRoles".

Or maybe it's still applicable that we should keep both ?

--
Kieran


> 
> > [...]
> 
> 
> Regards,
> Barnabás Pőcze
> 
> 
> [0]: https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/83d2e85f490ea97e4ae94b95f20dd06566a14c31/spa/plugins/libcamera/libcamera-utils.cpp#L58
> [1]: https://codesearch.debian.net/search?q=StreamRoles&literal=1