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

[Kieran Bingham]

Quoting Barnabás Pőcze (2023-04-20 12:35:15)
> 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,

[0] is helpful context thanks. Without that - it's hard to understand
the motivation that's all. We can make changes in the API - but without
understanding 'why' it's hard to quantify, or see how it makes
applications better.

[0] to me looks like a clear case that the user API can be improved with
this indeed.


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

I think this change sounds reasonable - it's just the original patch
felt like it was presented as "we can do this so why not" rather than
showing "why do" instead.

Internally it's easier, but when we change the public API - a
perspective of how it makes things better for the consumers is helpful.



> > 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.)

No - no API guarantee right now indeed. For exactly this reason. But
that's also why I think this change needs to consider 'what is right'
overall.

My worry is if we make the 'StreamRoles' type redundant, then it should
be considered or I worry that it will be confusing that some places use
"std::span<StreamRole>" while other points use "StreamRoles".

Or maybe it's still applicable that we should keep both ?

--
Kieran


> 
> > [...]
> 
> 
> 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