[libcamera-devel,v4,16/31] libcamera: request: Store the streams in the request
diff mbox series

Message ID 20190320163055.22056-17-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: ipu3: Add ImgU support + multiple streams
Related show

Commit Message

Jacopo Mondi March 20, 2019, 4:30 p.m. UTC 
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@jmondi.org>
---
 include/libcamera/request.h |  2 ++
 src/libcamera/request.cpp   | 16 ++++++++++++++++
 2 files changed, 18 insertions(+)

Comments

Laurent Pinchart March 23, 2019, 1:30 p.m. UTC | #1 
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@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

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 ?

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

>  
>  	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
Jacopo Mondi March 27, 2019, 8:28 a.m. UTC | #2 
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@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
Laurent Pinchart April 1, 2019, 9:45 p.m. UTC | #3 
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@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

Patch
diff mbox series 

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