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

[Laurent Pinchart]

Hi Jacopo,

On Wed, Mar 27, 2019 at 09:28:01AM +0100, Jacopo Mondi wrote:
> On Sat, Mar 23, 2019 at 03:30:00PM +0200, Laurent Pinchart wrote:
> > 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.

It's not just a matter of space, storing two separate copies of the same
information is usually a way to get them out of sync at some point.
Unless there's a good reason to do so (which could include various forms
of optimization), it should be avoided.

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

std::set is useful as an input parameter to methods of the Camera class
to make it impossible for the caller to store multiple instances of the
same object in the container. This avoids potentialy bugs by making them
impossible in the first place. To pass information back to application,
usage of other types of containers isn't ruled out. We have to decide in
each case which container is the best for the task at hand.

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