[libcamera-devel] [PATCH v2 7/8] libcamera: camera: Add CameraConfiguration
Jacopo Mondi
jacopo at jmondi.org
Fri Apr 5 13:12:11 CEST 2019
Hi Niklas,
On Fri, Apr 05, 2019 at 12:21:07PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback. I will just address conceptual thing in this
> reply and circle back to the nitty-gritty details in an other reply :-)
>
> On 2019-04-05 11:47:46 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> > thanks for the patch.
> >
> > On Fri, Apr 05, 2019 at 04:02:55AM +0200, Niklas Söderlund wrote:
> > > To properly support both multiple streams and usages the library must
> > > provide a method to map the stream usages to the returned stream
> > > configuration. Add a camera configuration object to handle this mapping.
> > >
> > > Applications can iterate over the returned camera configuration to
> > > retrieve the streams selected by the library in the same order as the
> > > usages it provided to the library. Further more the application can use
> >
> > "in the same order as it provided usages to the library"
> >
> > furthermore
> >
> > > the streams to retrieve there configuration using operator[] of the
> >
> > "a stream pointer to retrieve its configuration" or
> > "the streams to retrieve their configuration"
> >
> > > camera configuration.
> > >
> >
> > This is a nice and clean way to replace the cumbersome map<Stream *,
> > StreamConfiguration> used by both streamConfiguration() and
> > configureStream() and makes it easy to access a configuration from a
> > Stream *, so it's nice. Though, I don't see how this would help in
> > handling the "what stream is what" problem at requestComplete() time.
> > What's missing is the association with a name/id to a configuration,
> > is this something planned for later or am I missing how this should be
> > used?
>
> This works today, a dumb way for applications to implement this could
> look like:
>
> /* Setup streams */
> CameraConfiguration config =
> camera->streamConfiguration({ Stream::Foo(), Stream::Bar() });
>
> camera->configureStreams(config);
>
> ...
>
> camera->requestCompleted.connect(requestComplete);
>
> .. star camera etc...
>
>
> void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> {
> int i = 0;
> for (stream : config) {
> if (buffers.find(stream) != buffer.end())
> printf("Request contains buffer for role position %d\n", i);
>
> i++
> }
> }
>
> More likely the application will create its own
> std::map<Stream*, ApplicationStreamData> data structure and populate it
> once using the loop above.
I see. So this aims to replace the id/name based stream identification
in general.
Does this supersedes the idea of having configureStream() assign
id/names to streams configuration? I sill like the idea of having
application assign ids to roles, the id is copied to the returned
stream configuration assigned to a stream, and copied to the stream at
configureStream() time, so that applications could:
#define STREAM_VF 0
#define STREAM_STILL 1
CameraConfiguration config = streamConfiguration({STREAM_VF,
Viewfinder(640x480),
{STREAM_STILL,
StillCapture()});
...
configureStream(config); // Assign the id in the conf to the
// stream
....
void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
{
for (auto it : buffers) {
Stream *s = it.first;
if (s.id == STREAM_VF)
//display
if (s.id == STREAM_STILL)
//save to disk
}
}
They could most probably handle that internally with a map or
something indeed, but this creates a logical connection between the
intended role and an id applications can control.
>
> >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > include/libcamera/camera.h | 26 ++++++++
> > > src/libcamera/camera.cpp | 121 +++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 147 insertions(+)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 0386671c902e55e8..311a51e4070d135c 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -14,6 +14,7 @@
> > >
> > > #include <libcamera/request.h>
> > > #include <libcamera/signal.h>
> > > +#include <libcamera/stream.h>
> > >
> > > namespace libcamera {
> > >
> > > @@ -24,6 +25,31 @@ class Stream;
> > > class StreamConfiguration;
> > > class StreamUsage;
> > >
> > > +class CameraConfiguration
> > > +{
> > > +public:
> > > + using iterator = std::vector<Stream *>::iterator;
> > > + using const_iterator = std::vector<Stream *>::const_iterator;
> >
> > Have you considered giving this class iterator traits and implement
> > methods required for at least, say, ForwardIterator?
> > http://www.cplusplus.com/reference/iterator/ForwardIterator/
> >
> > That would make possible for applications to use STL library functions
> > that operates on iterators on the class. In example, can you use
> > range-based for loops on this class?
>
> This is a full fledge iterator already that support
> random_access_iterator_tag. It provides the proper begin() and end()
> functions which proxies the iterators for order_ which is a std::vector
> :-)
Do you mean the CameraConfiguration class is a
randome_access_iterator?
I see a lot of methods listed here but not supported by the class
http://www.cplusplus.com/reference/iterator/RandomAccessIterator/
If you forward them to a standard vector, then fine.
Anyway, this is good enough for now most probably.
Thanks
j
>
> The STL library functions such as ranged based loops works and are
> already used in the next patch of this series on this data type.
>
> >
> > > +
> > > + CameraConfiguration();
> > > +
> > > + iterator begin();
> > > + iterator end();
> > > + const_iterator begin() const;
> > > + const_iterator end() const;
> > > +
> > > + bool empty() const;
> > > + std::size_t size() const;
> > > +
> > > + Stream *front();
> > > +
> > > + StreamConfiguration &operator[](Stream *stream);
> > > +
> > > +private:
> > > + std::vector<Stream *> order_;
> > > + std::map<Stream *, StreamConfiguration> config_;
> >
> > I wonder if keeping the Stream * in two separate places might lead to
> > issues in keeping the two in sync. But these are privates, so it
> > should be fine...
> >
> > > +};
> > > +
> > > class Camera final
> > > {
> > > public:
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index 63fde0ffc3d02d6c..16162c524297012f 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -39,6 +39,127 @@ namespace libcamera {
> > >
> > > LOG_DECLARE_CATEGORY(Camera)
> > >
> > > +/**
> > > + * \class CameraConfiguration
> > > + * \brief Hold configuration for streams of the camera that applications
> > I would drop what's after the "of the camera".
> > > + * wish to modify and apply.
> > > + *
> > > + * The CameraConfiguration is filled with information by the application either
> > > + * manually or with the streamConfiguration() helper. The helper takes a list
> > > + * list of usages describing how the application intents to use the camera, the
> > s/list list/list/
> > s/intents/intends/
> > s/use the camera/use the stream/
> >
> > > + * application in returns is provided with a default camera configuration it
> > s/in returns/in return/
> > > + * can tweak.
> > > + *
> > > + * Applications iterates over the CameraConfiguration to discover which streams
> > s/iterates/iterate/
> > > + * the camera have selected for its usages and can inspect the configuration
> > s/the camera have selected for its usage/the camera has associated to a usage/
> > s/can inspect/can access/
> > > + * using the operator[].
> > > + */
> > > +
> > > +/**
> > > + * \typedef CameraConfiguration::iterator
> > > + * \brief Iterator for the streams in the configuration
> > > + */
> > > +
> > > +/**
> > > + * \typedef CameraConfiguration::const_iterator
> > > + * \brief Const iterator for the streams in the configuration
> > > + */
> > > +
> > > +/**
> > > + * \brief Create an empty camera configuration
> > > + */
> > > +CameraConfiguration::CameraConfiguration()
> > > + : order_({}), config_({})
> > > +{
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve an iterator to the first element of the streams
> > > + *
> >
> > The standard iterator documentation for begin reports:
> > "Returns an iterator pointing to the first element in the sequence"
> >
> > Could we match this?
> >
> > > + * \return An iterator to the first stream
> > > + */
> > > +std::vector<Stream *>::iterator CameraConfiguration::begin()
> > > +{
> > > + return order_.begin();
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve an iterator to the end of the streams
> >
> > Same as per 'begin()':
> >
> > "Returns an iterator pointing to the past-the-end element in the sequence"
> >
> > > + *
> > > + * \return An iterator to the element following the last stream
> > > + */
> > > +std::vector<Stream *>::iterator CameraConfiguration::end()
> > > +{
> > > + return order_.end();
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve an iterator to the first element of the streams
> > > + *
> > > + * \return An iterator to the first stream
> > > + */
> > > +std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> > > +{
> > > + return order_.begin();
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve an iterator to the end of the streams
> > > + *
> > > + * \return An iterator to the element following the last stream
> > > + */
> > > +std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> > > +{
> > > + return order_.end();
> > > +}
> > > +
> > > +/**
> > > + * \brief Checks whether the camera configuration is empty
> > > + *
> > > + * \return True if the configuration is empty
> > > + */
> > > +bool CameraConfiguration::empty() const
> > > +{
> > > + return order_.empty();
> > > +}
> > > +
> > > +/**
> > > + * \brief Check the number of stream configurations
> > > + *
> > > + * \return Number of stream configurations
> > > + */
> > > +std::size_t CameraConfiguration::size() const
> > > +{
> > > + return order_.size();
> > > +}
> > > +
> > > +/**
> > > + * \brief Access the first stream in the configuration
> > > + *
> > > + * \return The first stream in the configuration
> > > + */
> > > +Stream *CameraConfiguration::front()
> > > +{
> > > + return order_.front();
> > > +}
> > > +
> > > +/**
> > > + * \brief Retrieve a reference to a stream configuration
> > > + * \param[in] stream Stream to retrieve configuration for
> > > + *
> > > + * If the camera configuration do not yet contain configuration for the
> >
> > contain a configuration
> >
> > > + * requested stream an empty stream configuration is created and returned.
> >
> > empty one
> >
> > > + *
> > > + * \return Configuration for the stream
> >
> > The configuration
> >
> > > + */
> > > +StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> > > +{
> > > + if (config_.find(stream) == config_.end())
> > > + order_.push_back(stream);
> > > +
> > > + return config_[stream];
> > > +}
> > > +
> >
> > I don't know how welcome would the idea of making of this class a
> > full-fledged iterator, but I think increment, decrement and access by
> > integer index should be implemented to make it more useful. Have you
> > considered that?
>
> As stated above it is already implemented. Well not integer indexed as
> the whole idea if this object is to make it Stream* indexed but preserve
> the insertion order when iterating over it to allow mapping stream
> usages to stream configurations.
>
> >
> > Thanks
> > j
> >
> > > /**
> > > * \class Camera
> > > * \brief Camera device
> > > --
> > > 2.21.0
> > >
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
>
>
>
> --
> Regards,
> Niklas Söderlund
-------------- 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/20190405/8562a80b/attachment-0001.sig>
More information about the libcamera-devel
mailing list