[libcamera-devel] [PATCH v2 7/8] libcamera: camera: Add CameraConfiguration
Niklas Söderlund
niklas.soderlund at ragnatech.se
Fri Apr 5 23:20:23 CEST 2019
Hi Jacopo, Laurent,
Thanks for all your feedback, specially the ones improving my
documentation. I have taken all your comments in this area in the next
version.
On 2019-04-05 18:45:47 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Apr 05, 2019 at 01:12:11PM +0200, Jacopo Mondi wrote:
> > On Fri, Apr 05, 2019 at 12:21:07PM +0200, Niklas Söderlund wrote:
> > > On 2019-04-05 11:47:46 +0200, Jacopo Mondi wrote:
> > >> 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
>
> s/usages/stream usages/
>
> > >>> provide a method to map the stream usages to the returned stream
> > >>> configuration. Add a camera configuration object to handle this mapping.
>
> s/stream configuration/streams configurations/
>
> > >>>
> > >>> 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"
>
> I'd side with Niklas on this one, your version makes "order" refer to
> "provided", while I think it should refer to "usages" (we provide all
> usages in one go).
>
> > >>
> > >> 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.
>
> While I agree the above is nice, I think the CameraConfiguration class
> is good for now. We may add IDs later if needed, based on how code will
> look like in our applications.
>
> By the way, the above application code could also do
>
> const Stream *streamViewfinder = nullptr;
> const Stream *streamStill = nullptr;
>
> ...
> CameraConfiguration config = streamConfiguration({Viewfinder(640x480), StillCapture()});
> streamViewfinder = config[0];
> streamStill = config[1];
> ...
>
> void requestComplete(Request *request, const std::map<Stream *, Buffer *> &buffers)
> {
> for (auto it : buffers) {
> Stream *s = it.first;
>
> if (s == streamViewfinder)
> //display
> if (s == streamStill)
> //save to disk
> }
> }
>
> if we added a
>
> Stream *operator[](int index);
I like this, will add this for next version.
>
> operator to the CameraConfiguration class.
>
> > >>> 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>
>
> Is this needed ? I think you only use Stream pointers in this header.
No it's not needed. It's a left over from my prototype stage, thanks for
spotting it.
>
> > >>>
> > >>> 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.
> >
> > > 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/
>
> s/use the camera/use the streams/
>
> Although "use streams of the camera" may be better, as the helper
> doesn't receive streams.
>
> > >>> + * application in returns is provided with a default camera configuration it
> > >>
> > >> s/in returns/in return/
> > >>
> > >>> + * can tweak.
>
> s/tweak/tune/
>
> > >>> + *
> > >>> + * 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/
>
> "to the usages"
>
> > >> 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
>
> s/whether/if/
>
> Kieran recently taught me that "whether" requires two clauses (whether A
> or B).
>
> > >>> + *
> > >>> + * \return True if the configuration is empty
> > >>> + */
> > >>> +bool CameraConfiguration::empty() const
> > >>> +{
> > >>> + return order_.empty();
> > >>> +}
> > >>> +
> > >>> +/**
> > >>> + * \brief Check the number of stream configurations
>
> s/Check/Retrieve/
>
> > >>> + *
> > >>> + * \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
>
> "If the camera configuration does not yet contain a configuration for
> the requested stream, create and return an empty stream configuration."
>
> > >>
> > >>> + *
> > >>> + * \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.
> > >
> > >>> /**
> > >>> * \class Camera
> > >>> * \brief Camera device
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list