[libcamera-devel] [RFC PATCH v1] libcamera: camera: Take span of StreamRole instead of vector
Jacopo Mondi
jacopo.mondi at ideasonboard.com
Mon Feb 20 17:08:46 CET 2023
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 at 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 at protonmail.com>
> > > To: libcamera-devel at 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 at 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
> > >
> >
More information about the libcamera-devel
mailing list