[libcamera-devel] [RFC PATCH v3 1/2] libcamera: camera: Take span of StreamRole instead of vector
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu May 18 18:16:16 CEST 2023
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 at 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 at 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 at ideasonboard.com>
> >
> > > .def("configure", &Camera::configure)
> > >
> > > .def("create_request", &Camera::createRequest, py::arg("cookie") = 0)
> > > --
> > > 2.40.1
> > >
> > >
> >
>
>
> Regards,
> Barnabás Pőcze
More information about the libcamera-devel
mailing list