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

[Barnabás Pőcze]

Hi


2023. április 19., szerda 23:52 keltezéssel, Kieran Bingham <kieran.bingham at ideasonboard.com> írta:

> [...]
> > > > > 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 motivating example is in pipewire[0]. It made me wonder if there is
actually a reason for using a vector here. And as far as I could tell,
there wasn't one, so this patch was born. I would even wager to say that
in the vast majority of cases (like here), a `const std::vector<T>&` argument
can be replaced by `std::span<const T>` without significant consequences
since what is a `const std::vector<T>&` if not a worse `std::span<const T>`
(there are exceptions, of course)?

So my motivations for this patch were the following:

 - I thought this was a non-intrusive change;
 - it can get rid of the need for constructing the vector in certain cases.

And of course I am not arguing that this changes the world, but it is such
a simple change, that I thought "why not?". And as I have said I do understand
if this is considered a micro-optimization and rejected on that basis.


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

There are no API stability guarantees, right? So I would personally remove it, but
that might be too harsh of an approach. According to Debian Code Search, it is only
found in pipewire and libcamera[1]. (There are other users, I am sure.)


> [...]


Regards,
Barnabás Pőcze


[0]: https://gitlab.freedesktop.org/pipewire/pipewire/-/blob/83d2e85f490ea97e4ae94b95f20dd06566a14c31/spa/plugins/libcamera/libcamera-utils.cpp#L58
[1]: https://codesearch.debian.net/search?q=StreamRoles&literal=1