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

[Barnabás Pőcze]

Hi


2023. február 20., hétfő 17:08 keltezéssel, Jacopo Mondi <jacopo.mondi at 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 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.

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