[libcamera-devel] [PATCH v2 3/6] libcamera: Refactor the camera configuration storage and API
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue May 21 15:14:36 CEST 2019
Hi Jacopo,
On Tue, May 21, 2019 at 11:19:41AM +0200, Jacopo Mondi wrote:
> On Sun, May 19, 2019 at 06:00:44PM +0300, Laurent Pinchart wrote:
> > Refactor the CameraConfiguration structure to not rely on Stream
> > instances. This is a step towards making the camera configuration object
> > more powerful with configuration validation using "try" semantics.
> >
> > The CameraConfiguration now exposes a simple vector-like API to access
> > the contained stream configurations. Both operator[]() and at() are
> > provided to access elements. The isEmpty() method is renamed to empty()
> > and the methods reordered to match the std::vector class.
> >
> > As applications need access to the Stream instances associated with the
> > configuration entries in order to associate buffers with streams when
> > creating requests, expose the stream selected by the pipeline handler
> > through a new StreamConfiguration::stream().
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 36 +--
> > include/libcamera/stream.h | 7 +
> > src/cam/main.cpp | 35 +--
> > src/libcamera/camera.cpp | 268 +++++++++++------------
> > src/libcamera/include/pipeline_handler.h | 2 +-
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 32 +--
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 12 +-
> > src/libcamera/pipeline/uvcvideo.cpp | 23 +-
> > src/libcamera/pipeline/vimc.cpp | 23 +-
> > src/libcamera/pipeline_handler.cpp | 4 +
> > src/libcamera/stream.cpp | 22 ++
> > src/qcam/main_window.cpp | 4 +-
> > test/camera/capture.cpp | 4 +-
> > test/camera/configuration_set.cpp | 2 +-
> > 14 files changed, 251 insertions(+), 223 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 42ba5201eabc..284e5276a055 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -25,30 +25,36 @@ class Request;
> > class CameraConfiguration
> > {
> > public:
> > - using iterator = std::vector<Stream *>::iterator;
> > - using const_iterator = std::vector<Stream *>::const_iterator;
> > + using iterator = std::vector<StreamConfiguration>::iterator;
> > + using const_iterator = std::vector<StreamConfiguration>::const_iterator;
> >
> > CameraConfiguration();
> >
> > + void addConfiguration(const StreamConfiguration &cfg);
>
> I would use push_back() to match the std::vector API
I've thought about it, but the goal here isn't to mimick the vector API
completely. For an interation point of view I think it makes sense to
operate as a vector, but when it comes to building the configuration I
think we need explicit calls (same for validation). addConfiguration()
may not be the best name either, maybe addStreamConfiguration() ?
There's also a high chance we'll refactor this some more.
> > +
> > + bool isValid() const;
>
> As you s/isEmpty()/empty()/ should we s/isValid()/valid()/ ?
Don't make me go back to isEmpty() ;-)
> Ah, don't worry, this will become validate(), just noticed
>
> > +
> > + StreamConfiguration &at(unsigned int index);
> > + const StreamConfiguration &at(unsigned int index) const;
> > + StreamConfiguration &operator[](unsigned int index)
> > + {
> > + return at(index);
> > + }
> > + const StreamConfiguration &operator[](unsigned int index) const
> > + {
> > + return at(index);
> > + }
>
> Ok, so we have [] and at() like std::vector, but it seems to me their
> behaviour is inverted.
>
> std::vector::at() performs bound checking, and
> CameraConfiguration::at() is implemented with std::vector::operator[],
> which does not perform bounds checking
>
> std::vector::operator[] does not perform bounds checking, but
> CameraConfiguration::operator[] is implemented with std::vector::at()
> which performs bound checking.
No, CameraConfiguration::operator[] is implemented with
CameraConfiguration::at(), which, as you noted above, doesn't perform
bound checking as it's implemented with std::vector::operator[].
>
> Is this intentional ?
As we don't support exceptions, it makes no real difference, and as a
result I went for the operator that won't throw an exception.
> I would also question the need to have both operator[] and at()
> accessors, I know std::vector does, but do we need that or is it just
> an API expansion we could save?
It was requested by Niklas to replace (*config)[] with config->at() in
the callers. I have no personal problem with (*config)[], and if that's
desired, I'm fine dropping at().
> > +
> > iterator begin();
> > - iterator end();
> > const_iterator begin() const;
> > + iterator end();
> > const_iterator end() const;
> >
> > - bool isValid() const;
> > - bool isEmpty() const;
> > + bool empty() const;
> > std::size_t size() const;
> >
> > - Stream *front();
> > - const Stream *front() const;
> > -
> > - Stream *operator[](unsigned int index) const;
> > - StreamConfiguration &operator[](Stream *stream);
> > - const StreamConfiguration &operator[](Stream *stream) const;
> > -
> > private:
> > - std::vector<Stream *> order_;
> > - std::map<Stream *, StreamConfiguration> config_;
> > + std::vector<StreamConfiguration> config_;
> > };
> >
> > class Camera final
> > @@ -72,7 +78,7 @@ public:
> >
> > const std::set<Stream *> &streams() const;
> > CameraConfiguration generateConfiguration(const StreamRoles &roles);
> > - int configure(const CameraConfiguration &config);
> > + int configure(CameraConfiguration &config);
> >
> > int allocateBuffers();
> > int freeBuffers();
> > diff --git a/include/libcamera/stream.h b/include/libcamera/stream.h
> > index 59bdf217eb31..47c007ed52e2 100644
> > --- a/include/libcamera/stream.h
> > +++ b/include/libcamera/stream.h
> > @@ -16,6 +16,7 @@
> > namespace libcamera {
> >
> > class Camera;
> > +class Stream;
> >
> > struct StreamConfiguration {
> > unsigned int pixelFormat;
> > @@ -23,7 +24,13 @@ struct StreamConfiguration {
> >
> > unsigned int bufferCount;
> >
> > + Stream *stream() const { return stream_; }
> > + void setStream(Stream *stream) { stream_ = stream; }
> > +
> > std::string toString() const;
> > +
> > +private:
> > + Stream *stream_;
>
> Should we protect access to unitialized streams_ ?
> If StreamConfiguration is not inialized to {} what's the value of
> *stream_ ?
It's indeed not initialised, but applications are not allowed to use it
before the stream has been set by Camera::configure(). I can add a
default constructor, but I really hope to remove Stream from the public
API. I suppose it then won't hurt if I add the constructor just to be
safe in the meantime.
> > };
> >
> > enum StreamRole {
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index d603228c0116..cd165bea34cd 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -89,12 +89,9 @@ static int prepareCameraConfig(CameraConfiguration *config)
> > {
> > StreamRoles roles;
> >
> > - streamInfo.clear();
> > -
> > /* If no configuration is provided assume a single video stream. */
> > if (!options.isSet(OptStream)) {
> > *config = camera->generateConfiguration({ StreamRole::VideoRecording });
> > - streamInfo[config->front()] = "stream0";
> > return 0;
> > }
> >
> > @@ -129,27 +126,20 @@ static int prepareCameraConfig(CameraConfiguration *config)
> > }
> >
> > /* Apply configuration explicitly requested. */
> > - CameraConfiguration::iterator it = config->begin();
> > + unsigned int i = 0;
> > for (auto const &value : streamOptions) {
> > KeyValueParser::Options conf = value.toKeyValues();
>
> nit and not related to this patch: s/conf/opt ? otherwise we have
> *config and cfg in this loop, which is quite confusing.
I agree. I'll add a patch.
> > - Stream *stream = *it;
> > - it++;
> > + StreamConfiguration &cfg = (*config)[i++];
> >
> > if (conf.isSet("width"))
> > - (*config)[stream].size.width = conf["width"];
> > + cfg.size.width = conf["width"];
> >
> > if (conf.isSet("height"))
> > - (*config)[stream].size.height = conf["height"];
> > + cfg.size.height = conf["height"];
> >
> > /* TODO: Translate 4CC string to ID. */
> > if (conf.isSet("pixelformat"))
> > - (*config)[stream].pixelFormat = conf["pixelformat"];
> > - }
> > -
> > - unsigned int index = 0;
> > - for (Stream *stream : *config) {
> > - streamInfo[stream] = "stream" + std::to_string(index);
> > - index++;
> > + cfg.pixelFormat = conf["pixelformat"];
> > }
> >
> > return 0;
> > @@ -216,6 +206,13 @@ static int capture()
> > return ret;
> > }
> >
> > + streamInfo.clear();
> > +
> > + for (unsigned int index = 0; index < config.size(); ++index) {
> > + StreamConfiguration &cfg = config[index];
> > + streamInfo[cfg.stream()] = "stream" + std::to_string(index);
> > + }
> > +
> > ret = camera->allocateBuffers();
> > if (ret) {
> > std::cerr << "Failed to allocate buffers"
> > @@ -227,8 +224,10 @@ static int capture()
> >
> > /* Identify the stream with the least number of buffers. */
> > unsigned int nbuffers = UINT_MAX;
> > - for (Stream *stream : config)
> > + for (StreamConfiguration &cfg : config) {
> > + Stream *stream = cfg.stream();
> > nbuffers = std::min(nbuffers, stream->bufferPool().count());
> > + }
> >
> > /*
> > * TODO: make cam tool smarter to support still capture by for
> > @@ -245,8 +244,10 @@ static int capture()
> > }
> >
> > std::map<Stream *, Buffer *> map;
> > - for (Stream *stream : config)
> > + for (StreamConfiguration &cfg : config) {
> > + Stream *stream = cfg.stream();
> > map[stream] = &stream->bufferPool().buffers()[i];
> > + }
> >
> > ret = request->setBuffers(map);
> > if (ret < 0) {
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index a3921f91f1c9..5848330f639a 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -46,72 +46,40 @@ LOG_DECLARE_CATEGORY(Camera)
> > * \class CameraConfiguration
> > * \brief Hold configuration for streams of the camera
> >
> > - * The CameraConfiguration holds an ordered list of streams and their associated
> > - * StreamConfiguration. From a data storage point of view, the class operates as
> > - * a map of Stream pointers to StreamConfiguration, with entries accessed with
> > - * operator[](Stream *). Accessing an entry for a Stream pointer not yet stored
> > - * in the configuration inserts a new empty entry.
> > - *
> > - * The class also suppors iterators, and from that point of view operates as a
> > - * vector of Stream pointers. The streams are iterated in insertion order, and
> > - * the operator[](int) returns the Stream pointer based on its insertion index.
> > - * Accessing a stream with an invalid index returns a null pointer.
> > + * The CameraConfiguration holds an ordered list of stream configurations. It
> > + * supports iterators and operates as a vector of StreamConfiguration instances.
> > + * The stream configurations are inserted by addConfiguration(), and the
> > + * operator[](int) returns a reference to the StreamConfiguration based on its
> > + * insertion index. Accessing a stream configuration with an invalid index
> > + * results in undefined behaviour.
>
> As operator[] is implemented with std::vector::at() accessing with an
> invalid index, an exception is thrown (even if we don't use them in
> the library).
It's actually implemented with std::evctor::operator[] as explained
above :-)
> I would document CameraConfiguration::at() as well, or just provide one of
> the two only.
The at() functions are documented ;-)
> > */
> >
> > /**
> > * \typedef CameraConfiguration::iterator
> > - * \brief Iterator for the streams in the configuration
> > + * \brief Iterator for the stream configurations in the camera configuration
> > */
> >
> > /**
> > * \typedef CameraConfiguration::const_iterator
> > - * \brief Const iterator for the streams in the configuration
> > + * \brief Const iterator for the stream configuration in the camera
> > + * configuration
> > */
> >
> > /**
> > * \brief Create an empty camera configuration
> > */
> > CameraConfiguration::CameraConfiguration()
> > - : order_({}), config_({})
> > + : config_({})
> > {
> > }
> >
> > /**
> > - * \brief Retrieve an iterator to the first stream in the sequence
> > - * \return An iterator to the first stream
> > + * \brief Add a stream configuration to the camera configuration
> > + * \param[in] cfg The stream configuration
> > */
> > -std::vector<Stream *>::iterator CameraConfiguration::begin()
> > +void CameraConfiguration::addConfiguration(const StreamConfiguration &cfg)
> > {
> > - return order_.begin();
> > -}
> > -
> > -/**
> > - * \brief Retrieve an iterator pointing to the past-the-end stream in the
> > - * sequence
> > - * \return An iterator to the element following the last stream
> > - */
> > -std::vector<Stream *>::iterator CameraConfiguration::end()
> > -{
> > - return order_.end();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const iterator to the first element of the streams
> > - * \return A const iterator to the first stream
> > - */
> > -std::vector<Stream *>::const_iterator CameraConfiguration::begin() const
> > -{
> > - return order_.begin();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const iterator pointing to the past-the-end stream in the
> > - * sequence
> > - * \return A const iterator to the element following the last stream
> > - */
> > -std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> > -{
> > - return order_.end();
> > + config_.push_back(cfg);
> > }
> >
> > /**
> > @@ -125,12 +93,10 @@ std::vector<Stream *>::const_iterator CameraConfiguration::end() const
> > */
> > bool CameraConfiguration::isValid() const
> > {
> > - if (isEmpty())
> > + if (empty())
> > return false;
> >
> > - for (auto const &it : config_) {
> > - const StreamConfiguration &cfg = it.second;
> > -
> > + for (const StreamConfiguration &cfg : config_) {
> > if (cfg.size.width == 0 || cfg.size.height == 0 ||
> > cfg.pixelFormat == 0 || cfg.bufferCount == 0)
> > return false;
> > @@ -139,13 +105,108 @@ bool CameraConfiguration::isValid() const
> > return true;
> > }
> >
> > +/**
> > + * \brief Retrieve a reference to a stream configuration
> > + * \param[in] index Numerical index
>
> STL uses pos in place of index. Not sure we care, though.
I had considered this too :-) In the end I went for index to be
consistent with what we usually use (and I think it's also more explicit
than pos in this particular case).
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
>
> > + */
> > +StreamConfiguration &CameraConfiguration::at(unsigned int index)
> > +{
> > + return config_[index];
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +const StreamConfiguration &CameraConfiguration::at(unsigned int index) const
> > +{
> > + return config_[index];
> > +}
> > +
> > +/**
> > + * \fn StreamConfiguration &CameraConfiguration::operator[](unsigned int)
> > + * \brief Retrieve a reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +
> > +/**
> > + * \fn const StreamConfiguration &CameraConfiguration::operator[](unsigned int) const
> > + * \brief Retrieve a const reference to a stream configuration
> > + * \param[in] index Numerical index
> > + *
> > + * The \a index represents the zero based insertion order of stream
> > + * configuration into the camera configuration with addConfiguration(). Calling
> > + * this method with an invalid index results in undefined behaviour.
> > + *
> > + * \return The stream configuration
> > + */
> > +
> > +/**
> > + * \brief Retrieve an iterator to the first stream configuration in the
> > + * sequence
> > + * \return An iterator to the first stream configuration
> > + */
> > +CameraConfiguration::iterator CameraConfiguration::begin()
> > +{
> > + return config_.begin();
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const iterator to the first element of the stream
> > + * configurations
> > + * \return A const iterator to the first stream configuration
> > + */
> > +CameraConfiguration::const_iterator CameraConfiguration::begin() const
> > +{
> > + return config_.begin();
> > +}
> > +
> > +/**
> > + * \brief Retrieve an iterator pointing to the past-the-end stream
> > + * configuration in the sequence
> > + * \return An iterator to the element following the last stream configuration
> > + */
> > +CameraConfiguration::iterator CameraConfiguration::end()
>
> Aren't these StreamConfiguration::iterators ?
There's no StreamConfiguration::iterator. CameraConfiguration::iterator
is the iterator that iterates over the contents of CameraConfiguration.
This is similar to std::vector::iterator iterating over the contents of
the vector, we don't use int::iterator to iterate over a
std::vector<int>. The iterator is implemented by the container, not the
contained data.
> > +{
> > + return config_.end();
> > +}
> > +
> > +/**
> > + * \brief Retrieve a const iterator pointing to the past-the-end stream
> > + * configuration in the sequence
> > + * \return A const iterator to the element following the last stream
> > + * configuration
> > + */
> > +CameraConfiguration::const_iterator CameraConfiguration::end() const
> > +{
> > + return config_.end();
> > +}
> > +
> > /**
> > * \brief Check if the camera configuration is empty
> > * \return True if the configuration is empty
> > */
> > -bool CameraConfiguration::isEmpty() const
> > +bool CameraConfiguration::empty() const
> > {
> > - return order_.empty();
> > + return config_.empty();
> > }
> >
> > /**
> > @@ -154,75 +215,7 @@ bool CameraConfiguration::isEmpty() const
> > */
> > 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 Access the first stream in the configuration
> > - * \return The first const stream pointer in the configuration
> > - */
> > -const Stream *CameraConfiguration::front() const
> > -{
> > - return order_.front();
> > -}
> > -
> > -/**
> > - * \brief Retrieve a stream pointer from index
> > - * \param[in] index Numerical index
> > - *
> > - * The \a index represents the zero based insertion order of stream and stream
> > - * configuration into the camera configuration.
> > - *
> > - * \return The stream pointer at index, or a nullptr if the index is out of
> > - * bounds
> > - */
> > -Stream *CameraConfiguration::operator[](unsigned int index) const
> > -{
> > - if (index >= order_.size())
> > - return nullptr;
> > -
> > - return order_.at(index);
> > -}
> > -
> > -/**
> > - * \brief Retrieve a reference to a stream configuration
> > - * \param[in] stream Stream to retrieve configuration for
> > - *
> > - * If the camera configuration does not yet contain a configuration for
> > - * the requested stream, create and return an empty stream configuration.
> > - *
> > - * \return The configuration for the stream
> > - */
> > -StreamConfiguration &CameraConfiguration::operator[](Stream *stream)
> > -{
> > - if (config_.find(stream) == config_.end())
> > - order_.push_back(stream);
> > -
> > - return config_[stream];
> > -}
> > -
> > -/**
> > - * \brief Retrieve a const reference to a stream configuration
> > - * \param[in] stream Stream to retrieve configuration for
> > - *
> > - * No new stream configuration is created if called with \a stream that is not
> > - * already part of the camera configuration, doing so is an invalid operation
> > - * and results in undefined behaviour.
> > - *
> > - * \return The configuration for the stream
> > - */
> > -const StreamConfiguration &CameraConfiguration::operator[](Stream *stream) const
> > -{
> > - return config_.at(stream);
> > + return config_.size();
> > }
> >
> > /**
> > @@ -561,13 +554,9 @@ Camera::generateConfiguration(const StreamRoles &roles)
> > CameraConfiguration config = pipe_->generateConfiguration(this, roles);
> >
> > std::ostringstream msg("streams configuration:", std::ios_base::ate);
> > - unsigned int index = 0;
> >
> > - for (Stream *stream : config) {
> > - const StreamConfiguration &cfg = config[stream];
> > - msg << " (" << index << ") " << cfg.toString();
> > - index++;
> > - }
> > + for (unsigned int index = 0; index < config.size(); ++index)
> > + msg << " (" << index << ") " << config[index].toString();
> >
> > LOG(Camera, Debug) << msg.str();
> >
> > @@ -593,12 +582,15 @@ Camera::generateConfiguration(const StreamRoles &roles)
> > *
> > * This function affects the state of the camera, see \ref camera_operation.
> > *
> > + * Upon return the StreamConfiguration entries in \a config are associated with
> > + * Stream instances which can be retrieved with StreamConfiguration::stream().
> > + *
> > * \return 0 on success or a negative error code otherwise
> > * \retval -ENODEV The camera has been disconnected from the system
> > * \retval -EACCES The camera is not in a state where it can be configured
> > * \retval -EINVAL The configuration is not valid
> > */
> > -int Camera::configure(const CameraConfiguration &config)
> > +int Camera::configure(CameraConfiguration &config)
> > {
> > int ret;
> >
> > @@ -615,16 +607,11 @@ int Camera::configure(const CameraConfiguration &config)
> > }
> >
> > std::ostringstream msg("configuring streams:", std::ios_base::ate);
> > - unsigned int index = 0;
> >
> > - for (Stream *stream : config) {
> > - if (streams_.find(stream) == streams_.end())
> > - return -EINVAL;
> > -
> > - const StreamConfiguration &cfg = config[stream];
> > - msg << std::dec << " (" << index << ") " << cfg.toString();
> > -
> > - index++;
> > + for (unsigned int index = 0; index < config.size(); ++index) {
> > + StreamConfiguration &cfg = config[index];
> > + cfg.setStream(nullptr);
> > + msg << " (" << index << ") " << cfg.toString();
>
> Isn't this better printed after pipe->configure(), once we know all
> streams configuration have a stream assigned ?
It shouldn't make a difference as the stream is not printed, and I think
it's useful to print it before in case pipe->configure() fails, to help
debugging the issue.
> Overall this is very good. The fact StreamConfiguration instances are
> associated to Stream just after configure() might require clearly
> preventing applications to try access it, but I think the
> documentation is quite clear on that.
I don't like that part too much, but I didn't go through great lengths
to fix it as I plan to remove the Stream anyway.
> The biggest part is discussing the CameraConfiguration API I guess,
> which we might want to make more similar to std::vector.
>
> > }
> >
> > LOG(Camera, Info) << msg.str();
> > @@ -634,8 +621,11 @@ int Camera::configure(const CameraConfiguration &config)
> > return ret;
> >
> > activeStreams_.clear();
> > - for (Stream *stream : config) {
> > - const StreamConfiguration &cfg = config[stream];
> > + for (const StreamConfiguration &cfg : config) {
> > + Stream *stream = cfg.stream();
> > + if (!stream)
> > + LOG(Camera, Fatal)
> > + << "Pipeline handler failed to update stream configuration";
> >
> > stream->configuration_ = cfg;
> > activeStreams_.insert(stream);
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 3352cb0e5bc9..a025905ab68f 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -62,7 +62,7 @@ public:
> >
> > virtual CameraConfiguration
> > generateConfiguration(Camera *camera, const StreamRoles &roles) = 0;
> > - virtual int configure(Camera *camera, const CameraConfiguration &config) = 0;
> > + virtual int configure(Camera *camera, CameraConfiguration &config) = 0;
> >
> > virtual int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) = 0;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index d234a8ac5289..ed0ef69de1d1 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -152,8 +152,7 @@ public:
> >
> > CameraConfiguration
> > generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera,
> > - const CameraConfiguration &config) override;
> > + int configure(Camera *camera, CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -299,14 +298,13 @@ PipelineHandlerIPU3::generateConfiguration(Camera *camera,
> > cfg.pixelFormat = V4L2_PIX_FMT_NV12;
> > cfg.bufferCount = IPU3_BUFFER_COUNT;
> >
> > - config[stream] = cfg;
> > + config.addConfiguration(cfg);
> > }
> >
> > return config;
> > }
> >
> > -int PipelineHandlerIPU3::configure(Camera *camera,
> > - const CameraConfiguration &config)
> > +int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration &config)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > IPU3Stream *outStream = &data->outStream_;
> > @@ -318,9 +316,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> >
> > outStream->active_ = false;
> > vfStream->active_ = false;
> > - for (Stream *s : config) {
> > - IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> > - const StreamConfiguration &cfg = config[stream];
> > + for (StreamConfiguration &cfg : config) {
> > + /*
> > + * Pick a stream for the configuration entry.
> > + * \todo: This is a naive temporary implementation that will be
> > + * reworked when implementing camera configuration validation.
> > + */
> > + IPU3Stream *stream = vfStream->active_ ? outStream : vfStream;
> >
> > /*
> > * Verify that the requested size respects the IPU3 alignment
> > @@ -355,6 +357,7 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> > sensorSize.height = cfg.size.height;
> >
> > stream->active_ = true;
> > + cfg.setStream(stream);
> > }
> >
> > /*
> > @@ -379,10 +382,9 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> > return ret;
> >
> > /* Apply the format to the configured streams output devices. */
> > - for (Stream *s : config) {
> > - IPU3Stream *stream = static_cast<IPU3Stream *>(s);
> > -
> > - ret = imgu->configureOutput(stream->device_, config[stream]);
> > + for (StreamConfiguration &cfg : config) {
> > + IPU3Stream *stream = static_cast<IPU3Stream *>(cfg.stream());
> > + ret = imgu->configureOutput(stream->device_, cfg);
> > if (ret)
> > return ret;
> > }
> > @@ -393,15 +395,13 @@ int PipelineHandlerIPU3::configure(Camera *camera,
> > * be at least one active stream in the configuration request).
> > */
> > if (!outStream->active_) {
> > - ret = imgu->configureOutput(outStream->device_,
> > - config[vfStream]);
> > + ret = imgu->configureOutput(outStream->device_, config[0]);
> > if (ret)
> > return ret;
> > }
> >
> > if (!vfStream->active_) {
> > - ret = imgu->configureOutput(vfStream->device_,
> > - config[outStream]);
> > + ret = imgu->configureOutput(vfStream->device_, config[0]);
> > if (ret)
> > return ret;
> > }
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 4bd8c5101a96..ec6980b0943a 100644
> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > @@ -36,8 +36,7 @@ public:
> >
> > CameraConfiguration generateConfiguration(Camera *camera,
> > const StreamRoles &roles) override;
> > - int configure(Camera *camera,
> > - const CameraConfiguration &config) override;
> > + int configure(Camera *camera, CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -117,16 +116,15 @@ CameraConfiguration PipelineHandlerRkISP1::generateConfiguration(Camera *camera,
> > cfg.size = data->sensor_->resolution();
> > cfg.bufferCount = RKISP1_BUFFER_COUNT;
> >
> > - config[&data->stream_] = cfg;
> > + config.addConfiguration(cfg);
> >
> > return config;
> > }
> >
> > -int PipelineHandlerRkISP1::configure(Camera *camera,
> > - const CameraConfiguration &config)
> > +int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration &config)
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > - const StreamConfiguration &cfg = config[&data->stream_];
> > + StreamConfiguration &cfg = config[0];
> > CameraSensor *sensor = data->sensor_;
> > int ret;
> >
> > @@ -217,6 +215,8 @@ int PipelineHandlerRkISP1::configure(Camera *camera,
> > return -EINVAL;
> > }
> >
> > + cfg.setStream(&data->stream_);
> > +
> > return 0;
> > }
> >
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index d2e1f7d4e5b2..5dcc868b2fc9 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -27,8 +27,7 @@ public:
> >
> > CameraConfiguration
> > generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera,
> > - const CameraConfiguration &config) override;
> > + int configure(Camera *camera, CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -78,38 +77,38 @@ CameraConfiguration
> > PipelineHandlerUVC::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > - UVCCameraData *data = cameraData(camera);
> > CameraConfiguration config;
> > - StreamConfiguration cfg{};
> > + StreamConfiguration cfg;
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_YUYV;
> > cfg.size = { 640, 480 };
> > cfg.bufferCount = 4;
> >
> > - config[&data->stream_] = cfg;
> > + config.addConfiguration(cfg);
> >
> > return config;
> > }
> >
> > -int PipelineHandlerUVC::configure(Camera *camera,
> > - const CameraConfiguration &config)
> > +int PipelineHandlerUVC::configure(Camera *camera, CameraConfiguration &config)
> > {
> > UVCCameraData *data = cameraData(camera);
> > - const StreamConfiguration *cfg = &config[&data->stream_];
> > + StreamConfiguration &cfg = config[0];
> > int ret;
> >
> > V4L2DeviceFormat format = {};
> > - format.fourcc = cfg->pixelFormat;
> > - format.size = cfg->size;
> > + format.fourcc = cfg.pixelFormat;
> > + format.size = cfg.size;
> >
> > ret = data->video_->setFormat(&format);
> > if (ret)
> > return ret;
> >
> > - if (format.size != cfg->size ||
> > - format.fourcc != cfg->pixelFormat)
> > + if (format.size != cfg.size ||
> > + format.fourcc != cfg.pixelFormat)
> > return -EINVAL;
> >
> > + cfg.setStream(&data->stream_);
> > +
> > return 0;
> > }
> >
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 17e2491e5c27..af6b6f21e3c5 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -27,8 +27,7 @@ public:
> >
> > CameraConfiguration
> > generateConfiguration(Camera *camera, const StreamRoles &roles) override;
> > - int configure(Camera *camera,
> > - const CameraConfiguration &config) override;
> > + int configure(Camera *camera, CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera,
> > const std::set<Stream *> &streams) override;
> > @@ -78,38 +77,38 @@ CameraConfiguration
> > PipelineHandlerVimc::generateConfiguration(Camera *camera,
> > const StreamRoles &roles)
> > {
> > - VimcCameraData *data = cameraData(camera);
> > CameraConfiguration config;
> > - StreamConfiguration cfg{};
> > + StreamConfiguration cfg;
> >
> > cfg.pixelFormat = V4L2_PIX_FMT_RGB24;
> > cfg.size = { 640, 480 };
> > cfg.bufferCount = 4;
> >
> > - config[&data->stream_] = cfg;
> > + config.addConfiguration(cfg);
> >
> > return config;
> > }
> >
> > -int PipelineHandlerVimc::configure(Camera *camera,
> > - const CameraConfiguration &config)
> > +int PipelineHandlerVimc::configure(Camera *camera, CameraConfiguration &config)
> > {
> > VimcCameraData *data = cameraData(camera);
> > - const StreamConfiguration *cfg = &config[&data->stream_];
> > + StreamConfiguration &cfg = config[0];
> > int ret;
> >
> > V4L2DeviceFormat format = {};
> > - format.fourcc = cfg->pixelFormat;
> > - format.size = cfg->size;
> > + format.fourcc = cfg.pixelFormat;
> > + format.size = cfg.size;
> >
> > ret = data->video_->setFormat(&format);
> > if (ret)
> > return ret;
> >
> > - if (format.size != cfg->size ||
> > - format.fourcc != cfg->pixelFormat)
> > + if (format.size != cfg.size ||
> > + format.fourcc != cfg.pixelFormat)
> > return -EINVAL;
> >
> > + cfg.setStream(&data->stream_);
> > +
> > return 0;
> > }
> >
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index 81c11149c9fe..4185e3557dcb 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -255,6 +255,10 @@ void PipelineHandler::unlock()
> > * configuration of a subset of the streams can't be satisfied, the
> > * whole configuration is considered invalid.
> > *
> > + * Once the configuration is validated and the camera configured, the pipeline
> > + * handler shall associate a Stream instance to each StreamConfiguration entry
> > + * in the CameraConfiguration with the StreamConfiguration::setStream() method.
> > + *
> > * \return 0 on success or a negative error code otherwise
> > */
> >
> > diff --git a/src/libcamera/stream.cpp b/src/libcamera/stream.cpp
> > index fe4c4ecf4150..0c59a31a3a05 100644
> > --- a/src/libcamera/stream.cpp
> > +++ b/src/libcamera/stream.cpp
> > @@ -58,6 +58,28 @@ namespace libcamera {
> > * \brief Requested number of buffers to allocate for the stream
> > */
> >
> > +/**
> > + * \fn StreamConfiguration::stream()
> > + * \brief Retrieve the stream associated with the configuration
> > + *
> > + * When a camera is configured with Camera::configure() Stream instances are
> > + * associated with each stream configuration entry. This method retrieves the
> > + * associated Stream, which remains valid until the next call to
> > + * Camera::configure() or Camera::release().
> > + *
> > + * \return The stream associated with the configuration
> > + */
> > +
> > +/**
> > + * \fn StreamConfiguration::setStream()
> > + * \brief Associate a stream with a configuration
> > + *
> > + * This method is meant for the PipelineHandler::configure() method and shall
> > + * not be called by applications.
> > + *
> > + * \param[in] stream The stream
> > + */
> > +
> > /**
> > * \brief Assemble and return a string describing the configuration
> > *
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index a984aaca764f..06ae2985f80d 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -98,14 +98,14 @@ int MainWindow::startCapture()
> > int ret;
> >
> > config_ = camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - Stream *stream = config_.front();
> > ret = camera_->configure(config_);
> > if (ret < 0) {
> > std::cout << "Failed to configure camera" << std::endl;
> > return ret;
> > }
> >
> > - const StreamConfiguration &cfg = config_[stream];
> > + const StreamConfiguration &cfg = config_[0];
> > + Stream *stream = cfg.stream();
> > ret = viewfinder_->setFormat(cfg.pixelFormat, cfg.size.width,
> > cfg.size.height);
> > if (ret < 0) {
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index e7e6438203b9..bfd11eefedcf 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -44,8 +44,7 @@ protected:
> > {
> > CameraConfiguration config =
> > camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - Stream *stream = config.front();
> > - StreamConfiguration *cfg = &config[stream];
> > + StreamConfiguration *cfg = &config[0];
> >
> > if (!config.isValid()) {
> > cout << "Failed to read default configuration" << endl;
> > @@ -67,6 +66,7 @@ protected:
> > return TestFail;
> > }
> >
> > + Stream *stream = cfg->stream();
> > BufferPool &pool = stream->bufferPool();
> > std::vector<Request *> requests;
> > for (Buffer &buffer : pool.buffers()) {
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 76d8bc3e40a4..25b5db67103a 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -20,7 +20,7 @@ protected:
> > {
> > CameraConfiguration config =
> > camera_->generateConfiguration({ StreamRole::VideoRecording });
> > - StreamConfiguration *cfg = &config[config.front()];
> > + StreamConfiguration *cfg = &config[0];
> >
> > if (!config.isValid()) {
> > cout << "Failed to read default configuration" << endl;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list