[libcamera-devel] [RFC PATCH v2 1/2] libcamera: camera: Take span of StreamRole instead of vector

[Barnabás Pőcze]

Hi


2023. május 9., kedd 9:13 keltezéssel, Laurent Pinchart <laurent.pinchart at ideasonboard.com> írta:

> Hi Barnabás,
>
> Thank you for the patch.
>
> On Wed, May 03, 2023 at 12:47:28PM +0000, Barnabás Pőcze via libcamera-devel wrote:
> > 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.
>
> There's no need to wrap at 60 columns, you can use the normal 72 columns
> wrapping for commit messages.

I did not want to break "const std::vector&" and not breaking before
that makes the line ~80 chars long. I will send a new version with that.


>
> > A new overload is added that accepts an initializer list so that
> >
> >   cam->generateConfiguration({ ... })
> >
> > keeps working.
> >
> > There is no API break since a span can be constructed from a vector
> > and the initializer list overload takes care of the initializer lists,
> > but this change causes an ABI break.
> >
> > Signed-off-by: Barnabás Pőcze <pobrn 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                           |  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, 33 insertions(+), 20 deletions(-)
> >
> > diff --git a/Documentation/guides/pipeline-handler.rst b/Documentation/guides/pipeline-handler.rst
> > index 1acd1812..e08bb72f 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..425fe51b 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 = {});
> > +
> > +	/**
> > +	 * \overload
> > +	 */
>
> Documentation belongs to the .cpp file.

ACK


>
> > +	std::unique_ptr<CameraConfiguration> generateConfiguration(std::initializer_list<StreamRole> roles)
>
> I'd wrap this to
>
> 	std::unique_ptr<CameraConfiguration>
> 	generateConfiguration(std::initializer_list<StreamRole> roles)
>
> as it's getting long. Maybe above as well.


ACK

>
> > +	{
> > +		return generateConfiguration(Span(roles.begin(), roles.end()));
> > +	}
> > +
> >  	int configure(CameraConfiguration *config);
> >
> >  	std::unique_ptr<Request> createRequest(uint64_t cookie = 0);
> [...]


Regards,
Barnabás Pőcze