[libcamera-devel] [PATCH v3 5/8] libcamera: request: Add streams() method

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Fri, Apr 05, 2019 at 01:34:33PM +0200, Niklas Söderlund wrote:
> On 2019-04-03 17:07:32 +0200, Jacopo Mondi wrote:
> > Add a method to retrieve the list of streams contained in a requests.

s/requests/request/

> > 
> > This is useful for pipeline handler willing to cycle on all the streams

s/handler/handlers/
s/willing/that need to/

> > a request contains at queueRequest() time.

Wouldn't it be better to expose the streams to buffers map instead ?
The findBuffer() method doesn't seem very nice to use.

> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  include/libcamera/request.h |  3 +++
> >  src/libcamera/request.cpp   | 15 +++++++++++++++
> >  2 files changed, 18 insertions(+)
> > 
> > diff --git a/include/libcamera/request.h b/include/libcamera/request.h
> > index 0dbd425115e8..5ac4d20d1d9f 100644
> > --- a/include/libcamera/request.h
> > +++ b/include/libcamera/request.h
> > @@ -7,6 +7,7 @@
> >  #ifndef __LIBCAMERA_REQUEST_H__
> >  #define __LIBCAMERA_REQUEST_H__
> >  
> > +#include <list>
> >  #include <map>
> >  #include <unordered_set>
> >  
> > @@ -35,6 +36,8 @@ public:
> >  	int setBuffers(const std::map<Stream *, Buffer *> &streamMap);
> >  	Buffer *findBuffer(Stream *stream) const;
> >  
> > +	const std::list<Stream *> streams() const;
> > +
> >  	Status status() const { return status_; }
> >  
> >  private:
> > diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
> > index e0e77e972411..3a7841fb2bb3 100644
> > --- a/src/libcamera/request.cpp
> > +++ b/src/libcamera/request.cpp
> > @@ -93,6 +93,21 @@ Buffer *Request::findBuffer(Stream *stream) const
> >  	return it->second;
> >  }
> >  
> > +/**
> > + * \brief Retrieve the set of streams contained in the request
> > + *
> > + * \return The set of streams contained in the request
> > + */
> > +const std::list<Stream *> Request::streams() const
> 
> I would make this std::set<Stream *> to have a API guarantee there are 
> no duplicated Stream *. Whit this fixed,
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> > +{
> > +	std::list<Stream *> streams = {};
> > +
> > +	for (auto const &it : bufferMap_)
> > +		streams.push_front(it.first);
> > +
> > +	return streams;
> > +}
> > +
> >  /**
> >   * \fn Request::status()
> >   * \brief Retrieve the request completion status

-- 
Regards,

Laurent Pinchart