[libcamera-devel] [PATCH v2 8/8] libcamera: Switch to CameraConfiguration
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sat Apr 6 00:34:31 CEST 2019
Hi Laurent,
Thanks for your feedback.
On 2019-04-05 18:53:33 +0300, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Apr 05, 2019 at 04:02:56AM +0200, Niklas Söderlund wrote:
> > Implement the camera configuration thru out the library, tests, cam and
> > qcam tools.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > include/libcamera/camera.h | 4 ++--
> > src/cam/main.cpp | 8 ++++----
> > src/libcamera/camera.cpp | 15 +++++++--------
> > src/libcamera/include/pipeline_handler.h | 6 +++---
> > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++-----
> > src/libcamera/pipeline/uvcvideo.cpp | 10 +++++-----
> > src/libcamera/pipeline/vimc.cpp | 10 +++++-----
> > src/qcam/main_window.cpp | 2 +-
> > src/qcam/main_window.h | 2 +-
> > test/camera/camera_test.cpp | 6 +++---
> > test/camera/camera_test.h | 2 +-
> > test/camera/capture.cpp | 6 +++---
> > test/camera/configuration_default.cpp | 2 +-
> > test/camera/configuration_set.cpp | 4 ++--
> > test/camera/statemachine.cpp | 2 +-
> > 15 files changed, 44 insertions(+), 45 deletions(-)
> >
> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h
> > index 311a51e4070d135c..91b641b4da0744d6 100644
> > --- a/include/libcamera/camera.h
> > +++ b/include/libcamera/camera.h
> > @@ -70,9 +70,9 @@ public:
> > int release();
> >
> > const std::set<Stream *> &streams() const;
> > - std::map<Stream *, StreamConfiguration>
> > + CameraConfiguration
> > streamConfiguration(const std::vector<StreamUsage> &usage);
> > - int configureStreams(std::map<Stream *, StreamConfiguration> &config);
> > + int configureStreams(CameraConfiguration &config);
>
> Can we make that a const reference ?
With a small addition to CameraConfiguration we can, I have done so for
next version.
>
> >
> > int allocateBuffers();
> > int freeBuffers();
> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp
> > index d45ffd372d932d76..99ce564afd695b07 100644
> > --- a/src/cam/main.cpp
> > +++ b/src/cam/main.cpp
> > @@ -78,10 +78,10 @@ static int parseOptions(int argc, char *argv[])
> > return 0;
> > }
> >
> > -static int prepareCameraConfig(std::map<Stream *, StreamConfiguration> *config)
> > +static int prepareCameraConfig(CameraConfiguration *config)
> > {
> > *config = camera->streamConfiguration({ Stream::VideoRecording() });
> > - Stream *stream = config->begin()->first;
> > + Stream *stream = config->front();
> >
> > if (options.isSet(OptFormat)) {
> > KeyValueParser::Options format = options[OptFormat];
> > @@ -135,7 +135,7 @@ static void requestComplete(Request *request, const std::map<Stream *, Buffer *>
> >
> > static int capture()
> > {
> > - std::map<Stream *, StreamConfiguration> config;
> > + CameraConfiguration config;
> > std::vector<Request *> requests;
> > int ret;
> >
> > @@ -151,7 +151,7 @@ static int capture()
> > return ret;
> > }
> >
> > - Stream *stream = config.begin()->first;
> > + Stream *stream = config.front();
> >
> > ret = camera->allocateBuffers();
> > if (ret) {
> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > index 16162c524297012f..da8fab18356579cb 100644
> > --- a/src/libcamera/camera.cpp
> > +++ b/src/libcamera/camera.cpp
> > @@ -476,11 +476,11 @@ const std::set<Stream *> &Camera::streams() const
> > * \return A map of streams to configurations if the requested usages can be
> > * satisfied, or an empty map otherwise
>
> You forgot to update the documentation.
>
> > */
> > -std::map<Stream *, StreamConfiguration>
> > +CameraConfiguration
> > Camera::streamConfiguration(const std::vector<StreamUsage> &usages)
> > {
> > if (disconnected_ || !usages.size() || usages.size() > streams_.size())
> > - return std::map<Stream *, StreamConfiguration>{};
> > + return CameraConfiguration();
>
> Should we add a valid() method to CameraConfiguration for the caller to
> test for configuration validity ? It would be more explicit than testing
> for emptiness.
Good idea, I have added such a function in the next version.
>
> >
> > return pipe_->streamConfiguration(this, usages);
> > }
> > @@ -509,7 +509,7 @@ Camera::streamConfiguration(const std::vector<StreamUsage> &usages)
> > * \retval -EACCES The camera is not in a state where it can be configured
> > * \retval -EINVAL The configuration is not valid
> > */
>
> And here too I think.
>
> > -int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > +int Camera::configureStreams(CameraConfiguration &config)
> > {
> > int ret;
> >
> > @@ -525,8 +525,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > return -EINVAL;
> > }
> >
> > - for (auto const &iter : config) {
> > - if (streams_.find(iter.first) == streams_.end())
> > + for (Stream *stream : config) {
> > + if (streams_.find(stream) == streams_.end())
> > return -EINVAL;
> > }
> >
> > @@ -535,9 +535,8 @@ int Camera::configureStreams(std::map<Stream *, StreamConfiguration> &config)
> > return ret;
> >
> > activeStreams_.clear();
> > - for (auto const &iter : config) {
> > - Stream *stream = iter.first;
> > - const StreamConfiguration &cfg = iter.second;
> > + for (Stream *stream : config) {
> > + const StreamConfiguration &cfg = config[stream];
> >
> > stream->configuration_ = cfg;
> > activeStreams_.insert(stream);
> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h
> > index 8a1706fad5377bf4..3a7e7339ccf47707 100644
> > --- a/src/libcamera/include/pipeline_handler.h
> > +++ b/src/libcamera/include/pipeline_handler.h
> > @@ -18,6 +18,7 @@ namespace libcamera {
> > class Buffer;
> > class BufferPool;
> > class Camera;
> > +class CameraConfiguration;
> > class CameraManager;
> > class DeviceEnumerator;
> > class MediaDevice;
> > @@ -52,10 +53,9 @@ public:
> >
> > virtual bool match(DeviceEnumerator *enumerator) = 0;
> >
> > - virtual std::map<Stream *, StreamConfiguration>
> > + virtual CameraConfiguration
> > streamConfiguration(Camera *camera, const std::vector<StreamUsage> &usages) = 0;
> > - virtual int configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config) = 0;
> > + virtual int configureStreams(Camera *camera, CameraConfiguration &config) = 0;
>
> And here, can this be a const reference ?
Yes.
>
> >
> > virtual int allocateBuffers(Camera *camera, Stream *stream) = 0;
> > virtual int freeBuffers(Camera *camera, Stream *stream) = 0;
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 5d01504e24471bcf..a2442922a2afe41e 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -139,11 +139,11 @@ public:
> > PipelineHandlerIPU3(CameraManager *manager);
> > ~PipelineHandlerIPU3();
> >
> > - std::map<Stream *, StreamConfiguration>
> > + CameraConfiguration
> > streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages) override;
> > int configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config) override;
> > + CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera, Stream *stream) override;
> > int freeBuffers(Camera *camera, Stream *stream) override;
> > @@ -204,11 +204,11 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()
> > imguMediaDev_->release();
> > }
> >
> > -std::map<Stream *, StreamConfiguration>
> > +CameraConfiguration
> > PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages)
> > {
> > - std::map<Stream *, StreamConfiguration> configs;
> > + CameraConfiguration configs;
> > IPU3CameraData *data = cameraData(camera);
> > StreamConfiguration *config = &configs[&data->stream_];
> >
> > @@ -234,7 +234,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,
> > }
> >
> > int PipelineHandlerIPU3::configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config)
> > + CameraConfiguration &config)
> > {
> > IPU3CameraData *data = cameraData(camera);
> > const StreamConfiguration &cfg = config[&data->stream_];
> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp
> > index dfff7116bc2aa5d3..f6da073ba0c47090 100644
> > --- a/src/libcamera/pipeline/uvcvideo.cpp
> > +++ b/src/libcamera/pipeline/uvcvideo.cpp
> > @@ -26,11 +26,11 @@ public:
> > PipelineHandlerUVC(CameraManager *manager);
> > ~PipelineHandlerUVC();
> >
> > - std::map<Stream *, StreamConfiguration>
> > + CameraConfiguration
> > streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages) override;
> > int configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config) override;
> > + CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera, Stream *stream) override;
> > int freeBuffers(Camera *camera, Stream *stream) override;
> > @@ -82,12 +82,12 @@ PipelineHandlerUVC::~PipelineHandlerUVC()
> > media_->release();
> > }
> >
> > -std::map<Stream *, StreamConfiguration>
> > +CameraConfiguration
> > PipelineHandlerUVC::streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages)
> > {
> > UVCCameraData *data = cameraData(camera);
> > - std::map<Stream *, StreamConfiguration> configs;
> > + CameraConfiguration configs;
> > StreamConfiguration config{};
> >
> > config.width = 640;
> > @@ -101,7 +101,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera,
> > }
> >
> > int PipelineHandlerUVC::configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config)
> > + CameraConfiguration &config)
> > {
> > UVCCameraData *data = cameraData(camera);
> > StreamConfiguration *cfg = &config[&data->stream_];
> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp
> > index 4da4ca018d4b942a..6483ac842bf02dc2 100644
> > --- a/src/libcamera/pipeline/vimc.cpp
> > +++ b/src/libcamera/pipeline/vimc.cpp
> > @@ -26,11 +26,11 @@ public:
> > PipelineHandlerVimc(CameraManager *manager);
> > ~PipelineHandlerVimc();
> >
> > - std::map<Stream *, StreamConfiguration>
> > + CameraConfiguration
> > streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages) override;
> > int configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config) override;
> > + CameraConfiguration &config) override;
> >
> > int allocateBuffers(Camera *camera, Stream *stream) override;
> > int freeBuffers(Camera *camera, Stream *stream) override;
> > @@ -82,12 +82,12 @@ PipelineHandlerVimc::~PipelineHandlerVimc()
> > media_->release();
> > }
> >
> > -std::map<Stream *, StreamConfiguration>
> > +CameraConfiguration
> > PipelineHandlerVimc::streamConfiguration(Camera *camera,
> > const std::vector<StreamUsage> &usages)
> > {
> > VimcCameraData *data = cameraData(camera);
> > - std::map<Stream *, StreamConfiguration> configs;
> > + CameraConfiguration configs;
> > StreamConfiguration config{};
> >
> > config.width = 640;
> > @@ -101,7 +101,7 @@ PipelineHandlerVimc::streamConfiguration(Camera *camera,
> > }
> >
> > int PipelineHandlerVimc::configureStreams(Camera *camera,
> > - std::map<Stream *, StreamConfiguration> &config)
> > + CameraConfiguration &config)
> > {
> > VimcCameraData *data = cameraData(camera);
> > StreamConfiguration *cfg = &config[&data->stream_];
> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp
> > index faa3bc5739dd8453..4bc0440370410525 100644
> > --- a/src/qcam/main_window.cpp
> > +++ b/src/qcam/main_window.cpp
> > @@ -98,7 +98,7 @@ int MainWindow::startCapture()
> > int ret;
> >
> > config_ = camera_->streamConfiguration({ Stream::VideoRecording() });
> > - Stream *stream = config_.begin()->first;
> > + Stream *stream = config_.front();
> > ret = camera_->configureStreams(config_);
> > if (ret < 0) {
> > std::cout << "Failed to configure camera" << std::endl;
> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h
> > index 5e27a8fd6b4eb8f3..9b31da2bf4b75a03 100644
> > --- a/src/qcam/main_window.h
> > +++ b/src/qcam/main_window.h
> > @@ -46,7 +46,7 @@ private:
> >
> > std::shared_ptr<Camera> camera_;
> > bool isCapturing_;
> > - std::map<Stream *, StreamConfiguration> config_;
> > + CameraConfiguration config_;
> >
> > ViewFinder *viewfinder_;
> > };
> > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp
> > index 1609c4b02842186a..42172543329e04e4 100644
> > --- a/test/camera/camera_test.cpp
> > +++ b/test/camera/camera_test.cpp
> > @@ -46,15 +46,15 @@ void CameraTest::cleanup()
> > cm_->stop();
> > };
> >
> > -bool CameraTest::configurationValid(const std::map<Stream *, StreamConfiguration> &config) const
> > +bool CameraTest::configurationValid(CameraConfiguration &config)
> > {
> > /* Test that the configuration is not empty. */
> > if (config.empty())
> > return false;
> >
> > /* Test that configuration is valid. */
> > - for (auto const &it : config) {
> > - const StreamConfiguration &conf = it.second;
> > + for (Stream *stream : config) {
> > + const StreamConfiguration conf = config[stream];
> >
> > if (conf.width == 0 || conf.height == 0 ||
> > conf.pixelFormat == 0 || conf.bufferCount == 0)
> > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h
> > index 5801fad3281e1653..fffa3bf16a9df1a4 100644
> > --- a/test/camera/camera_test.h
> > +++ b/test/camera/camera_test.h
> > @@ -23,7 +23,7 @@ protected:
> > int init();
> > void cleanup();
> >
> > - bool configurationValid(const std::map<Stream *, StreamConfiguration> &config) const;
> > + bool configurationValid(CameraConfiguration &config);
>
> const CameraConfiguration & ?
One better we can delete this function and replace it with the new
config.valid() function.
>
> >
> > std::shared_ptr<Camera> camera_;
> >
> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp
> > index b8dbdb62f9a50a33..e7aa3d97989c6c94 100644
> > --- a/test/camera/capture.cpp
> > +++ b/test/camera/capture.cpp
> > @@ -42,10 +42,10 @@ protected:
> >
> > int run()
> > {
> > - std::map<Stream *, StreamConfiguration> conf =
> > + CameraConfiguration conf =
> > camera_->streamConfiguration({ Stream::VideoRecording() });
> > - Stream *stream = conf.begin()->first;
> > - StreamConfiguration *sconf = &conf.begin()->second;
> > + Stream *stream = conf.front();
> > + StreamConfiguration *sconf = &conf[stream];
> >
> > if (!configurationValid(conf)) {
> > cout << "Failed to read default configuration" << endl;
> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp
> > index 09861716973d752c..9ae332368f80e8b9 100644
> > --- a/test/camera/configuration_default.cpp
> > +++ b/test/camera/configuration_default.cpp
> > @@ -18,7 +18,7 @@ class ConfigurationDefault : public CameraTest
> > protected:
> > int run()
> > {
> > - std::map<Stream *, StreamConfiguration> conf;
> > + CameraConfiguration conf;
> >
> > /* Test asking for configuration for a video stream. */
> > conf = camera_->streamConfiguration({ Stream::VideoRecording() });
> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp
> > index 1bc01e66625eedf0..7224f1aa6657523d 100644
> > --- a/test/camera/configuration_set.cpp
> > +++ b/test/camera/configuration_set.cpp
> > @@ -18,9 +18,9 @@ class ConfigurationSet : public CameraTest
> > protected:
> > int run()
> > {
> > - std::map<Stream *, StreamConfiguration> conf =
> > + CameraConfiguration conf =
> > camera_->streamConfiguration({ Stream::VideoRecording() });
> > - StreamConfiguration *sconf = &conf.begin()->second;
> > + StreamConfiguration *sconf = &conf[conf.front()];
> >
> > if (!configurationValid(conf)) {
> > cout << "Failed to read default configuration" << endl;
> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp
> > index ab3c6fb5bea38c36..8ae93bee7ca178d4 100644
> > --- a/test/camera/statemachine.cpp
> > +++ b/test/camera/statemachine.cpp
> > @@ -265,7 +265,7 @@ protected:
> > return TestPass;
> > }
> >
> > - std::map<Stream *, StreamConfiguration> defconf_;
> > + CameraConfiguration defconf_;
> > };
> >
> > } /* namespace */
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list