[libcamera-devel] [PATCH 6/8] libcamera: store stream pointers in sets instead of a vectors
Jacopo Mondi
jacopo at jmondi.org
Wed Feb 27 10:50:47 CET 2019
Hi Niklas,
On Tue, Feb 26, 2019 at 08:06:09PM +0100, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2019-02-26 18:18:52 +0100, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Tue, Feb 26, 2019 at 03:18:55AM +0100, Niklas Söderlund wrote:
> > > The arrays that store Stream pointers shall always contain unique
> > > values. Storing them in vectors opens up for the same stream pointer
> > > appearing twice. Remove this possibility by storing them in a set which
> > > grantees each element is unique.
> > >
> >
> > Have you considered unordered_set of the ordering of the strems is
> > somehow relevant?
>
> I did consider it and judge that moving to is probably what we want.
> However we have not discussed if the order we add Streams to a array
> matters or not so I went for std::set instead as it behaves similar to a
> std::vector in this regard while enforcing the problem I wish to solve,
> that a Stream pointer shall not be present more then once in the array.
>
> If/Once we judge that injection order is not important it's trivial to
> change this to std::unorderd_set. The only drawback of std::set is that
> it is a tad slower.
>
Yes, the advantage is surely limited, this is not performance critical
code.
Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Thanks
j
> >
> > Thanks
> > j
> >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > ---
> > > include/libcamera/camera.h | 11 ++++++-----
> > > src/cam/main.cpp | 10 +++++-----
> > > src/libcamera/camera.cpp | 10 +++++-----
> > > src/libcamera/include/pipeline_handler.h | 2 +-
> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++---
> > > src/libcamera/pipeline/uvcvideo.cpp | 6 +++---
> > > src/libcamera/pipeline/vimc.cpp | 6 +++---
> > > 7 files changed, 26 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > > index 8c8545b074e8ae13..1e75a6dafa4e7c5f 100644
> > > --- a/include/libcamera/camera.h
> > > +++ b/include/libcamera/camera.h
> > > @@ -9,6 +9,7 @@
> > >
> > > #include <map>
> > > #include <memory>
> > > +#include <set>
> > > #include <string>
> > >
> > > #include <libcamera/request.h>
> > > @@ -27,7 +28,7 @@ class Camera final
> > > public:
> > > static std::shared_ptr<Camera> create(PipelineHandler *pipe,
> > > const std::string &name,
> > > - const std::vector<Stream *> &streams);
> > > + const std::set<Stream *> &streams);
> > >
> > > Camera(const Camera &) = delete;
> > > Camera &operator=(const Camera &) = delete;
> > > @@ -40,9 +41,9 @@ public:
> > > int acquire();
> > > void release();
> > >
> > > - const std::vector<Stream *> &streams() const;
> > > + const std::set<Stream *> &streams() const;
> > > std::map<Stream *, StreamConfiguration>
> > > - streamConfiguration(std::vector<Stream *> &streams);
> > > + streamConfiguration(std::set<Stream *> &streams);
> > > int configureStreams(std::map<Stream *, StreamConfiguration> &config);
> > >
> > > int allocateBuffers();
> > > @@ -73,8 +74,8 @@ private:
> > >
> > > std::shared_ptr<PipelineHandler> pipe_;
> > > std::string name_;
> > > - std::vector<Stream *> streams_;
> > > - std::vector<Stream *> activeStreams_;
> > > + std::set<Stream *> streams_;
> > > + std::set<Stream *> activeStreams_;
> > >
> > > State state_;
> > > };
> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > > index 8df8844c33a2d052..13aa35968f38964d 100644
> > > --- a/src/cam/main.cpp
> > > +++ b/src/cam/main.cpp
> > > @@ -75,10 +75,10 @@ static int parseOptions(int argc, char *argv[])
> > > return 0;
> > > }
> > >
> > > -static int configureStreams(Camera *camera, std::vector<Stream *> &streams)
> > > +static int configureStreams(Camera *camera, std::set<Stream *> &streams)
> > > {
> > > KeyValueParser::Options format = options[OptFormat];
> > > - Stream *id = streams.front();
> > > + Stream *id = *streams.begin();
> > >
> > > std::map<Stream *, StreamConfiguration> config =
> > > camera->streamConfiguration(streams);
> > > @@ -132,7 +132,7 @@ static int capture()
> > > {
> > > int ret;
> > >
> > > - std::vector<Stream *> streams = camera->streams();
> > > + std::set<Stream *> streams = camera->streams();
> > > std::vector<Request *> requests;
> > >
> > > ret = configureStreams(camera.get(), streams);
> > > @@ -141,7 +141,7 @@ static int capture()
> > > return ret;
> > > }
> > >
> > > - Stream *stream = streams.front();
> > > + Stream *stream = *streams.begin();
> > >
> > > ret = camera->allocateBuffers();
> > > if (ret) {
> > > @@ -237,7 +237,7 @@ int main(int argc, char **argv)
> > > goto out;
> > > }
> > >
> > > - const std::vector<Stream *> &streams = camera->streams();
> > > + const std::set<Stream *> &streams = camera->streams();
> > > if (streams.size() != 1) {
> > > std::cout << "Camera has " << streams.size()
> > > << " streams, only 1 is supported"
> > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > index c50b14bbd904fc1c..938f21fe80ef8ceb 100644
> > > --- a/src/libcamera/camera.cpp
> > > +++ b/src/libcamera/camera.cpp
> > > @@ -66,7 +66,7 @@ LOG_DECLARE_CATEGORY(Camera)
> > > */
> > > std::shared_ptr<Camera> Camera::create(PipelineHandler *pipe,
> > > const std::string &name,
> > > - const std::vector<Stream *> &streams)
> > > + const std::set<Stream *> &streams)
> > > {
> > > struct Allocator : std::allocator<Camera> {
> > > void construct(void *p, PipelineHandler *pipe,
> > > @@ -223,10 +223,10 @@ void Camera::release()
> > > *
> > > * \return An array of all the camera's streams.
> > > */
> > > -const std::vector<Stream *> &Camera::streams() const
> > > +const std::set<Stream *> &Camera::streams() const
> > > {
> > > if (!stateIsAtleast(Free))
> > > - std::vector<Stream *>{};
> > > + std::set<Stream *>{};
> > >
> > > return streams_;
> > > }
> > > @@ -248,7 +248,7 @@ const std::vector<Stream *> &Camera::streams() const
> > > * empty list on error.
> > > */
> > > std::map<Stream *, StreamConfiguration>
> > > -Camera::streamConfiguration(std::vector<Stream *> &streams)
> > > +Camera::streamConfiguration(std::set<Stream *> &streams)
> > > {
> > > if (!stateIsAtleast(Free) || !streams.size())
> > > return std::map<Stream *, StreamConfiguration>{};
> > > @@ -300,7 +300,7 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > > const StreamConfiguration &cfg = iter.second;
> > >
> > > stream->configuration_ = cfg;
> > > - activeStreams_.push_back(stream);
> > > + activeStreams_.insert(stream);
> > >
> > > /*
> > > * Allocate buffer objects in the pool.
> > > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > > index 4363dcd8ed8ead97..4cd9b90c609c067e 100644
> > > --- a/src/libcamera/include/pipeline_handler.h
> > > +++ b/src/libcamera/include/pipeline_handler.h
> > > @@ -45,7 +45,7 @@ public:
> > > virtual bool match(DeviceEnumerator *enumerator) = 0;
> > >
> > > virtual std::map<Stream *, StreamConfiguration>
> > > - streamConfiguration(Camera *camera, std::vector<Stream *> &streams) = 0;
> > > + streamConfiguration(Camera *camera, std::set<Stream *> &streams) = 0;
> > > virtual int configureStreams(Camera *camera,
> > > std::map<Stream *, StreamConfiguration> &config) = 0;
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 9694d0ce51abda41..1066fbe350d934a0 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -32,7 +32,7 @@ public:
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams) override;
> > > + std::set<Stream *> &streams) override;
> > > int configureStreams(Camera *camera,
> > > std::map<Stream *, StreamConfiguration> &config) override;
> > >
> > > @@ -95,7 +95,7 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams)
> > > + std::set<Stream *> &streams)
> > > {
> > > IPU3CameraData *data = cameraData(camera);
> > > std::map<Stream *, StreamConfiguration> configs;
> > > @@ -374,7 +374,7 @@ void PipelineHandlerIPU3::registerCameras()
> > > std::unique_ptr<IPU3CameraData> data = utils::make_unique<IPU3CameraData>();
> > >
> > > std::string cameraName = sensor->name() + " " + std::to_string(id);
> > > - std::vector<Stream *> streams{ &data->stream_ };
> > > + std::set<Stream *> streams{ &data->stream_ };
> > > std::shared_ptr<Camera> camera = Camera::create(this, cameraName, streams);
> > >
> > > /*
> > > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > > index b7b8ff109109e9c5..7b697c0685630d92 100644
> > > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > > @@ -27,7 +27,7 @@ public:
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams) override;
> > > + std::set<Stream *> &streams) override;
> > > int configureStreams(Camera *camera,
> > > std::map<Stream *, StreamConfiguration> &config) override;
> > >
> > > @@ -63,7 +63,7 @@ PipelineHandlerUVC::~PipelineHandlerUVC()
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > PipelineHandlerUVC::streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams)
> > > + std::set<Stream *> &streams)
> > > {
> > > std::map<Stream *, StreamConfiguration> configs;
> > >
> > > @@ -171,7 +171,7 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)
> > > return false;
> > > }
> > >
> > > - std::vector<Stream *> streams{ &stream_ };
> > > + std::set<Stream *> streams{ &stream_ };
> > > std::shared_ptr<Camera> camera = Camera::create(this, media_->model(), streams);
> > > registerCamera(std::move(camera));
> > > hotplugMediaDevice(media_.get());
> > > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > > index 812777cffbb3e618..9ba96323d335e010 100644
> > > --- a/src/libcamera/pipeline/vimc.cpp
> > > +++ b/src/libcamera/pipeline/vimc.cpp
> > > @@ -27,7 +27,7 @@ public:
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams) override;
> > > + std::set<Stream *> &streams) override;
> > > int configureStreams(Camera *camera,
> > > std::map<Stream *, StreamConfiguration> &config) override;
> > >
> > > @@ -62,7 +62,7 @@ PipelineHandlerVimc::~PipelineHandlerVimc()
> > >
> > > std::map<Stream *, StreamConfiguration>
> > > PipelineHandlerVimc::streamConfiguration(Camera *camera,
> > > - std::vector<Stream *> &streams)
> > > + std::set<Stream *> &streams)
> > > {
> > > std::map<Stream *, StreamConfiguration> configs;
> > >
> > > @@ -171,7 +171,7 @@ bool PipelineHandlerVimc::match(DeviceEnumerator *enumerator)
> > > return false;
> > > }
> > >
> > > - std::vector<Stream *> streams{ &stream_ };
> > > + std::set<Stream *> streams{ &stream_ };
> > > std::shared_ptr<Camera> camera = Camera::create(this, "VIMC Sensor B",
> > > streams);
> > > registerCamera(std::move(camera));
> > > --
> > > 2.20.1
> > >
> > > _______________________________________________
> > > 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/20190227/3a6b14af/attachment-0001.sig>
More information about the libcamera-devel
mailing list