[libcamera-devel] [PATCH v2 7/8] libcamera: camera: Add CameraConfiguration
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Fri Apr 5 17:45:47 CEST 2019
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);
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.
> >>>
> >>> 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
More information about the libcamera-devel
mailing list