[libcamera-devel] [PATCH v4 16/31] libcamera: request: Store the streams in the request

[Jacopo Mondi]

Hi Laurent,

On Sat, Mar 23, 2019 at 03:30:00PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, Mar 20, 2019 at 05:30:40PM +0100, Jacopo Mondi wrote:
> > Store the streams which the request applies to and provide an accessor
> > method to return them in a set.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/request.h |  2 ++
> >  src/libcamera/request.cpp   | 16 ++++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 0dbd425115e8..1bf90de2c6f9 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -34,6 +34,7 @@ public:
> >
> >  	int setBuffers(const std::map<Stream *, Buffer *> &streamMap);
> >  	Buffer *findBuffer(Stream *stream) const;
> > +	const std::set<Stream *> &streams() const { return streams_; }
> >
> >  	Status status() const { return status_; }
> >
> > @@ -49,6 +50,7 @@ private:
> >  	Camera *camera_;
> >  	std::map<Stream *, Buffer *> bufferMap_;
> >  	std::unordered_set<Buffer *> pending_;
> > +	std::set<Stream *> streams_;
>
> This duplicates the keys stored in bufferMap_. One option to avoid this
> would be to implement the streams() function as

I don't think this is big, at all, we do expect a few streams per
camera, and the space required is for a few pointers and a container.

>
> std::set<Stream *> &Request::streams() const
> {
> 	std::set<Stream *> streams;
> 	for (auto const &it: bufferMap_)
> 		streams.insert(it.first);
> 	return streams;
> }
>
> This comes at an additional cost at runtime, whether this is preferable
> or not depends on the usage pattern, how many times do you expect this
> function to be called per request ?
>

If we really want to profile this though, I do expect this to be
called by pipeline handlers only at queueRequest time, where they do
receive a Request as paramter. Applications do set the buffers on the
request, but they might want to have them back from the request itself
too. So I expect this to be called once from pipeline handlers and
eventually by applications.

> Depending on how the caller(s) use the returned set, an std::vector
> could also be more efficient (slower lookup but faster push_back).

I used std:set for consistency, as it is the container used by the
Camera and the Request class when dealing with streams, but I do
expect callers to iterate on the stream list, and indeed we have to
push the Stream * back when constructing it, so a vector might make
more sense.

Thanks
   j

>
> >
> >  	Status status_;
> >  };
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index e0e77e972411..22c516208ede 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -51,6 +51,13 @@ Request::Request(Camera *camera)
> >  {
> >  }
> >
> > +/**
> > + * \fn Request::streams()
> > + * \brief Retrieve the set of streams contained in the request
> > + *
> > + * \return The set of streams contained in the request
> > + */
> > +
> >  /**
> >   * \brief Set the streams to capture with associated buffers
> >   * \param[in] streamMap The map of streams to buffers
> > @@ -65,6 +72,10 @@ int Request::setBuffers(const std::map<Stream *, Buffer *> &streamMap)
> >  	}
> >
> >  	bufferMap_ = streamMap;
> > +
> > +	for (const auto &map : streamMap)
> > +		streams_.insert(map.first);
> > +
> >  	return 0;
> >  }
> >
> > @@ -77,6 +88,11 @@ int Request::setBuffers(const std::map<Stream *, Buffer *> &streamMap)
> >   * map.
> >   */
> >
> > +/**
> > + * \var Request::streams_
> > + * \brief Set of streams in this request
> > + */
> > +
> >  /**
> >   * \brief Return the buffer associated with a stream
> >   * \param[in] stream The stream the buffer is associated to
>
> --
> Regards,
>
> Laurent Pinchart
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190327/b48776c8/attachment.sig>