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

[Kieran Bingham]

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 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?

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