[{"id":1293,"web_url":"https://patchwork.libcamera.org/comment/1293/","msgid":"<20190405155333.GF5184@pendragon.ideasonboard.com>","date":"2019-04-05T15:53:33","subject":"Re: [libcamera-devel] [PATCH v2 8/8] libcamera: Switch to\n\tCameraConfiguration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nThank you for the patch.\n\nOn Fri, Apr 05, 2019 at 04:02:56AM +0200, Niklas Söderlund wrote:\n> Implement the camera configuration thru out the library, tests, cam and\n> qcam tools.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> include/libcamera/camera.h | 4 ++--\n> src/cam/main.cpp | 8 ++++----\n> src/libcamera/camera.cpp | 15 +++++++--------\n> src/libcamera/include/pipeline_handler.h | 6 +++---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++-----\n> src/libcamera/pipeline/uvcvideo.cpp | 10 +++++-----\n> src/libcamera/pipeline/vimc.cpp | 10 +++++-----\n> src/qcam/main_window.cpp | 2 +-\n> src/qcam/main_window.h | 2 +-\n> test/camera/camera_test.cpp | 6 +++---\n> test/camera/camera_test.h | 2 +-\n> test/camera/capture.cpp | 6 +++---\n> test/camera/configuration_default.cpp | 2 +-\n> test/camera/configuration_set.cpp | 4 ++--\n> test/camera/statemachine.cpp | 2 +-\n> 15 files changed, 44 insertions(+), 45 deletions(-)\n> \n> diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> index 311a51e4070d135c..91b641b4da0744d6 100644\n> --- a/include/libcamera/camera.h\n> +++ b/include/libcamera/camera.h\n> @@ -70,9 +70,9 @@ public:\n> \tint release();\n> \n> \tconst std::set &streams() const;\n> -\tstd::map\n> +\tCameraConfiguration\n> \tstreamConfiguration(const std::vector &usage);\n> -\tint configureStreams(std::map &config);\n> +\tint configureStreams(CameraConfiguration &config);\n\nCan we make that a const reference ?\n\n> \n> \tint allocateBuffers();\n> \tint freeBuffers();\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index d45ffd372d932d76..99ce564afd695b07 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -78,10 +78,10 @@ static int parseOptions(int argc, char *argv[])\n> \treturn 0;\n> }\n> \n> -static int prepareCameraConfig(std::map *config)\n> +static int prepareCameraConfig(CameraConfiguration *config)\n> {\n> \t*config = camera->streamConfiguration({ Stream::VideoRecording() });\n> -\tStream *stream = config->begin()->first;\n> +\tStream *stream = config->front();\n> \n> \tif (options.isSet(OptFormat)) {\n> \t\tKeyValueParser::Options format = options[OptFormat];\n> @@ -135,7 +135,7 @@ static void requestComplete(Request *request, const std::map\n> \n> static int capture()\n> {\n> -\tstd::map config;\n> +\tCameraConfiguration config;\n> \tstd::vector requests;\n> \tint ret;\n> \n> @@ -151,7 +151,7 @@ static int capture()\n> \t\treturn ret;\n> \t}\n> \n> -\tStream *stream = config.begin()->first;\n> +\tStream *stream = config.front();\n> \n> \tret = camera->allocateBuffers();\n> \tif (ret) {\n> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> index 16162c524297012f..da8fab18356579cb 100644\n> --- a/src/libcamera/camera.cpp\n> +++ b/src/libcamera/camera.cpp\n> @@ -476,11 +476,11 @@ const std::set &Camera::streams() const\n> * \\return A map of streams to configurations if the requested usages can be\n> * satisfied, or an empty map otherwise\n\nYou forgot to update the documentation.\n\n> */\n> -std::map\n> +CameraConfiguration\n> Camera::streamConfiguration(const std::vector &usages)\n> {\n> \tif (disconnected_ || !usages.size() || usages.size() > streams_.size())\n> -\t\treturn std::map{};\n> +\t\treturn CameraConfiguration();\n\nShould we add a valid() method to CameraConfiguration for the caller to\ntest for configuration validity ? It would be more explicit than testing\nfor emptiness.\n\n> \n> \treturn pipe_->streamConfiguration(this, usages);\n> }\n> @@ -509,7 +509,7 @@ Camera::streamConfiguration(const std::vector &usages)\n> * \\retval -EACCES The camera is not in a state where it can be configured\n> * \\retval -EINVAL The configuration is not valid\n> */\n\nAnd here too I think.\n\n> -int Camera::configureStreams(std::map &config)\n> +int Camera::configureStreams(CameraConfiguration &config)\n> {\n> \tint ret;\n> \n> @@ -525,8 +525,8 @@ int Camera::configureStreams(std::map &config)\n> \t\treturn -EINVAL;\n> \t}\n> \n> -\tfor (auto const &iter : config) {\n> -\t\tif (streams_.find(iter.first) == streams_.end())\n> +\tfor (Stream *stream : config) {\n> +\t\tif (streams_.find(stream) == streams_.end())\n> \t\t\treturn -EINVAL;\n> \t}\n> \n> @@ -535,9 +535,8 @@ int Camera::configureStreams(std::map &config)\n> \t\treturn ret;\n> \n> \tactiveStreams_.clear();\n> -\tfor (auto const &iter : config) {\n> -\t\tStream *stream = iter.first;\n> -\t\tconst StreamConfiguration &cfg = iter.second;\n> +\tfor (Stream *stream : config) {\n> +\t\tconst StreamConfiguration &cfg = config[stream];\n> \n> \t\tstream->configuration_ = cfg;\n> \t\tactiveStreams_.insert(stream);\n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 8a1706fad5377bf4..3a7e7339ccf47707 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -18,6 +18,7 @@ namespace libcamera {\n> class Buffer;\n> class BufferPool;\n> class Camera;\n> +class CameraConfiguration;\n> class CameraManager;\n> class DeviceEnumerator;\n> class MediaDevice;\n> @@ -52,10 +53,9 @@ public:\n> \n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> \n> -\tvirtual std::map\n> +\tvirtual CameraConfiguration\n> \tstreamConfiguration(Camera *camera, const std::vector &usages) = 0;\n> -\tvirtual int configureStreams(Camera *camera,\n> -\t\t\t\t std::map &config) = 0;\n> +\tvirtual int configureStreams(Camera *camera, CameraConfiguration &config) = 0;\n\nAnd here, can this be a const reference ?\n\n> \n> \tvirtual int allocateBuffers(Camera *camera, Stream *stream) = 0;\n> \tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 5d01504e24471bcf..a2442922a2afe41e 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -139,11 +139,11 @@ public:\n> \tPipelineHandlerIPU3(CameraManager *manager);\n> \t~PipelineHandlerIPU3();\n> \n> -\tstd::map\n> +\tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> \t\t\t const std::vector &usages) override;\n> \tint configureStreams(Camera *camera,\n> -\t\t\t std::map &config) override;\n> +\t\t\t CameraConfiguration &config) override;\n> \n> \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> \tint freeBuffers(Camera *camera, Stream *stream) override;\n> @@ -204,11 +204,11 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> \t\timguMediaDev_->release();\n> }\n> \n> -std::map\n> +CameraConfiguration\n> PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> \t\t\t\t\t const std::vector &usages)\n> {\n> -\tstd::map configs;\n> +\tCameraConfiguration configs;\n> \tIPU3CameraData *data = cameraData(camera);\n> \tStreamConfiguration *config = &configs[&data->stream_];\n> \n> @@ -234,7 +234,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> }\n> \n> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> -\t\t\t\t\t std::map &config)\n> +\t\t\t\t\t CameraConfiguration &config)\n> {\n> \tIPU3CameraData *data = cameraData(camera);\n> \tconst StreamConfiguration &cfg = config[&data->stream_];\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index dfff7116bc2aa5d3..f6da073ba0c47090 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -26,11 +26,11 @@ public:\n> \tPipelineHandlerUVC(CameraManager *manager);\n> \t~PipelineHandlerUVC();\n> \n> -\tstd::map\n> +\tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> \t\t\t const std::vector &usages) override;\n> \tint configureStreams(Camera *camera,\n> -\t\t\t std::map &config) override;\n> +\t\t\t CameraConfiguration &config) override;\n> \n> \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> \tint freeBuffers(Camera *camera, Stream *stream) override;\n> @@ -82,12 +82,12 @@ PipelineHandlerUVC::~PipelineHandlerUVC()\n> \t\tmedia_->release();\n> }\n> \n> -std::map\n> +CameraConfiguration\n> PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> \t\t\t\t\tconst std::vector &usages)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> -\tstd::map configs;\n> +\tCameraConfiguration configs;\n> \tStreamConfiguration config{};\n> \n> \tconfig.width = 640;\n> @@ -101,7 +101,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> }\n> \n> int PipelineHandlerUVC::configureStreams(Camera *camera,\n> -\t\t\t\t\t std::map &config)\n> +\t\t\t\t\t CameraConfiguration &config)\n> {\n> \tUVCCameraData *data = cameraData(camera);\n> \tStreamConfiguration *cfg = &config[&data->stream_];\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 4da4ca018d4b942a..6483ac842bf02dc2 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -26,11 +26,11 @@ public:\n> \tPipelineHandlerVimc(CameraManager *manager);\n> \t~PipelineHandlerVimc();\n> \n> -\tstd::map\n> +\tCameraConfiguration\n> \tstreamConfiguration(Camera *camera,\n> \t\t\t const std::vector &usages) override;\n> \tint configureStreams(Camera *camera,\n> -\t\t\t std::map &config) override;\n> +\t\t\t CameraConfiguration &config) override;\n> \n> \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> \tint freeBuffers(Camera *camera, Stream *stream) override;\n> @@ -82,12 +82,12 @@ PipelineHandlerVimc::~PipelineHandlerVimc()\n> \t\tmedia_->release();\n> }\n> \n> -std::map\n> +CameraConfiguration\n> PipelineHandlerVimc::streamConfiguration(Camera *camera,\n> \t\t\t\t\t const std::vector &usages)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> -\tstd::map configs;\n> +\tCameraConfiguration configs;\n> \tStreamConfiguration config{};\n> \n> \tconfig.width = 640;\n> @@ -101,7 +101,7 @@ PipelineHandlerVimc::streamConfiguration(Camera *camera,\n> }\n> \n> int PipelineHandlerVimc::configureStreams(Camera *camera,\n> -\t\t\t\t std::map &config)\n> +\t\t\t\t\t CameraConfiguration &config)\n> {\n> \tVimcCameraData *data = cameraData(camera);\n> \tStreamConfiguration *cfg = &config[&data->stream_];\n> diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> index faa3bc5739dd8453..4bc0440370410525 100644\n> --- a/src/qcam/main_window.cpp\n> +++ b/src/qcam/main_window.cpp\n> @@ -98,7 +98,7 @@ int MainWindow::startCapture()\n> \tint ret;\n> \n> \tconfig_ = camera_->streamConfiguration({ Stream::VideoRecording() });\n> -\tStream *stream = config_.begin()->first;\n> +\tStream *stream = config_.front();\n> \tret = camera_->configureStreams(config_);\n> \tif (ret < 0) {\n> \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n> diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> index 5e27a8fd6b4eb8f3..9b31da2bf4b75a03 100644\n> --- a/src/qcam/main_window.h\n> +++ b/src/qcam/main_window.h\n> @@ -46,7 +46,7 @@ private:\n> \n> \tstd::shared_ptr camera_;\n> \tbool isCapturing_;\n> -\tstd::map config_;\n> +\tCameraConfiguration config_;\n> \n> \tViewFinder *viewfinder_;\n> };\n> diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\n> index 1609c4b02842186a..42172543329e04e4 100644\n> --- a/test/camera/camera_test.cpp\n> +++ b/test/camera/camera_test.cpp\n> @@ -46,15 +46,15 @@ void CameraTest::cleanup()\n> \tcm_->stop();\n> };\n> \n> -bool CameraTest::configurationValid(const std::map &config) const\n> +bool CameraTest::configurationValid(CameraConfiguration &config)\n> {\n> \t/* Test that the configuration is not empty. */\n> \tif (config.empty())\n> \t\treturn false;\n> \n> \t/* Test that configuration is valid. */\n> -\tfor (auto const &it : config) {\n> -\t\tconst StreamConfiguration &conf = it.second;\n> +\tfor (Stream *stream : config) {\n> +\t\tconst StreamConfiguration conf = config[stream];\n> \n> \t\tif (conf.width == 0 || conf.height == 0 ||\n> \t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h\n> index 5801fad3281e1653..fffa3bf16a9df1a4 100644\n> --- a/test/camera/camera_test.h\n> +++ b/test/camera/camera_test.h\n> @@ -23,7 +23,7 @@ protected:\n> \tint init();\n> \tvoid cleanup();\n> \n> -\tbool configurationValid(const std::map &config) const;\n> +\tbool configurationValid(CameraConfiguration &config);\n\nconst CameraConfiguration & ?\n\n> \n> \tstd::shared_ptr camera_;\n> \n> diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> index b8dbdb62f9a50a33..e7aa3d97989c6c94 100644\n> --- a/test/camera/capture.cpp\n> +++ b/test/camera/capture.cpp\n> @@ -42,10 +42,10 @@ protected:\n> \n> \tint run()\n> \t{\n> -\t\tstd::map conf =\n> +\t\tCameraConfiguration conf =\n> \t\t\tcamera_->streamConfiguration({ Stream::VideoRecording() });\n> -\t\tStream *stream = conf.begin()->first;\n> -\t\tStreamConfiguration *sconf = &conf.begin()->second;\n> +\t\tStream *stream = conf.front();\n> +\t\tStreamConfiguration *sconf = &conf[stream];\n> \n> \t\tif (!configurationValid(conf)) {\n> \t\t\tcout << \"Failed to read default configuration\" << endl;\n> diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> index 09861716973d752c..9ae332368f80e8b9 100644\n> --- a/test/camera/configuration_default.cpp\n> +++ b/test/camera/configuration_default.cpp\n> @@ -18,7 +18,7 @@ class ConfigurationDefault : public CameraTest\n> protected:\n> \tint run()\n> \t{\n> -\t\tstd::map conf;\n> +\t\tCameraConfiguration conf;\n> \n> \t\t/* Test asking for configuration for a video stream. */\n> \t\tconf = camera_->streamConfiguration({ Stream::VideoRecording() });\n> diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> index 1bc01e66625eedf0..7224f1aa6657523d 100644\n> --- a/test/camera/configuration_set.cpp\n> +++ b/test/camera/configuration_set.cpp\n> @@ -18,9 +18,9 @@ class ConfigurationSet : public CameraTest\n> protected:\n> \tint run()\n> \t{\n> -\t\tstd::map conf =\n> +\t\tCameraConfiguration conf =\n> \t\t\tcamera_->streamConfiguration({ Stream::VideoRecording() });\n> -\t\tStreamConfiguration *sconf = &conf.begin()->second;\n> +\t\tStreamConfiguration *sconf = &conf[conf.front()];\n> \n> \t\tif (!configurationValid(conf)) {\n> \t\t\tcout << \"Failed to read default configuration\" << endl;\n> diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> index ab3c6fb5bea38c36..8ae93bee7ca178d4 100644\n> --- a/test/camera/statemachine.cpp\n> +++ b/test/camera/statemachine.cpp\n> @@ -265,7 +265,7 @@ protected:\n> \t\treturn TestPass;\n> \t}\n> \n> -\tstd::map defconf_;\n> +\tCameraConfiguration defconf_;\n> };\n> \n> } /* namespace */","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 1151260DB3\n\tfor ;\n\tFri, 5 Apr 2019 17:53:46 +0200 (CEST)","from pendragon.ideasonboard.com (unknown [109.140.214.47])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 49634E2;\n\tFri, 5 Apr 2019 17:53:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1554479624;\n\tbh=HJECtq46dRiDGGL+q9PPOmjzMQIXSikoolJGaundOjw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=FgOvnAlvYJOET4G4joerm2YL6BcolF/lF4x+zG7Q811Zl3lJBWg8IA38Ch9btl9sg\n\tP2zLsTlWdbl/5dADEGCN9Y7zH1M4z6Y0T5Bnbsa81ApuiThfNYw4rSpk9ASZvUksZv\n\tpKOAAYGCVQ5kRIbUYwXWiX4E+ENsSm1+qPP/QdLo=","Date":"Fri, 5 Apr 2019 18:53:33 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405155333.GF5184@pendragon.ideasonboard.com>","References":"<20190405020256.22520-1-niklas.soderlund@ragnatech.se>\n\t<20190405020256.22520-9-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405020256.22520-9-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v2 8/8] libcamera: Switch to\n\tCameraConfiguration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 15:53:46 -0000"}},{"id":1295,"web_url":"https://patchwork.libcamera.org/comment/1295/","msgid":"<20190405223431.GB15350@bigcity.dyn.berto.se>","date":"2019-04-05T22:34:31","subject":"Re: [libcamera-devel] [PATCH v2 8/8] libcamera: Switch to\n\tCameraConfiguration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2019-04-05 18:53:33 +0300, Laurent Pinchart wrote:\n> Hi Niklas,\n> \n> Thank you for the patch.\n> \n> On Fri, Apr 05, 2019 at 04:02:56AM +0200, Niklas Söderlund wrote:\n> > Implement the camera configuration thru out the library, tests, cam and\n> > qcam tools.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > include/libcamera/camera.h | 4 ++--\n> > src/cam/main.cpp | 8 ++++----\n> > src/libcamera/camera.cpp | 15 +++++++--------\n> > src/libcamera/include/pipeline_handler.h | 6 +++---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 10 +++++-----\n> > src/libcamera/pipeline/uvcvideo.cpp | 10 +++++-----\n> > src/libcamera/pipeline/vimc.cpp | 10 +++++-----\n> > src/qcam/main_window.cpp | 2 +-\n> > src/qcam/main_window.h | 2 +-\n> > test/camera/camera_test.cpp | 6 +++---\n> > test/camera/camera_test.h | 2 +-\n> > test/camera/capture.cpp | 6 +++---\n> > test/camera/configuration_default.cpp | 2 +-\n> > test/camera/configuration_set.cpp | 4 ++--\n> > test/camera/statemachine.cpp | 2 +-\n> > 15 files changed, 44 insertions(+), 45 deletions(-)\n> > \n> > diff --git a/include/libcamera/camera.h b/include/libcamera/camera.h\n> > index 311a51e4070d135c..91b641b4da0744d6 100644\n> > --- a/include/libcamera/camera.h\n> > +++ b/include/libcamera/camera.h\n> > @@ -70,9 +70,9 @@ public:\n> > \tint release();\n> > \n> > \tconst std::set &streams() const;\n> > -\tstd::map\n> > +\tCameraConfiguration\n> > \tstreamConfiguration(const std::vector &usage);\n> > -\tint configureStreams(std::map &config);\n> > +\tint configureStreams(CameraConfiguration &config);\n> \n> Can we make that a const reference ?\n\nWith a small addition to CameraConfiguration we can, I have done so for \nnext version.\n\n> \n> > \n> > \tint allocateBuffers();\n> > \tint freeBuffers();\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index d45ffd372d932d76..99ce564afd695b07 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -78,10 +78,10 @@ static int parseOptions(int argc, char *argv[])\n> > \treturn 0;\n> > }\n> > \n> > -static int prepareCameraConfig(std::map *config)\n> > +static int prepareCameraConfig(CameraConfiguration *config)\n> > {\n> > \t*config = camera->streamConfiguration({ Stream::VideoRecording() });\n> > -\tStream *stream = config->begin()->first;\n> > +\tStream *stream = config->front();\n> > \n> > \tif (options.isSet(OptFormat)) {\n> > \t\tKeyValueParser::Options format = options[OptFormat];\n> > @@ -135,7 +135,7 @@ static void requestComplete(Request *request, const std::map\n> > \n> > static int capture()\n> > {\n> > -\tstd::map config;\n> > +\tCameraConfiguration config;\n> > \tstd::vector requests;\n> > \tint ret;\n> > \n> > @@ -151,7 +151,7 @@ static int capture()\n> > \t\treturn ret;\n> > \t}\n> > \n> > -\tStream *stream = config.begin()->first;\n> > +\tStream *stream = config.front();\n> > \n> > \tret = camera->allocateBuffers();\n> > \tif (ret) {\n> > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp\n> > index 16162c524297012f..da8fab18356579cb 100644\n> > --- a/src/libcamera/camera.cpp\n> > +++ b/src/libcamera/camera.cpp\n> > @@ -476,11 +476,11 @@ const std::set &Camera::streams() const\n> > * \\return A map of streams to configurations if the requested usages can be\n> > * satisfied, or an empty map otherwise\n> \n> You forgot to update the documentation.\n> \n> > */\n> > -std::map\n> > +CameraConfiguration\n> > Camera::streamConfiguration(const std::vector &usages)\n> > {\n> > \tif (disconnected_ || !usages.size() || usages.size() > streams_.size())\n> > -\t\treturn std::map{};\n> > +\t\treturn CameraConfiguration();\n> \n> Should we add a valid() method to CameraConfiguration for the caller to\n> test for configuration validity ? It would be more explicit than testing\n> for emptiness.\n\nGood idea, I have added such a function in the next version.\n\n> \n> > \n> > \treturn pipe_->streamConfiguration(this, usages);\n> > }\n> > @@ -509,7 +509,7 @@ Camera::streamConfiguration(const std::vector &usages)\n> > * \\retval -EACCES The camera is not in a state where it can be configured\n> > * \\retval -EINVAL The configuration is not valid\n> > */\n> \n> And here too I think.\n> \n> > -int Camera::configureStreams(std::map &config)\n> > +int Camera::configureStreams(CameraConfiguration &config)\n> > {\n> > \tint ret;\n> > \n> > @@ -525,8 +525,8 @@ int Camera::configureStreams(std::map &config)\n> > \t\treturn -EINVAL;\n> > \t}\n> > \n> > -\tfor (auto const &iter : config) {\n> > -\t\tif (streams_.find(iter.first) == streams_.end())\n> > +\tfor (Stream *stream : config) {\n> > +\t\tif (streams_.find(stream) == streams_.end())\n> > \t\t\treturn -EINVAL;\n> > \t}\n> > \n> > @@ -535,9 +535,8 @@ int Camera::configureStreams(std::map &config)\n> > \t\treturn ret;\n> > \n> > \tactiveStreams_.clear();\n> > -\tfor (auto const &iter : config) {\n> > -\t\tStream *stream = iter.first;\n> > -\t\tconst StreamConfiguration &cfg = iter.second;\n> > +\tfor (Stream *stream : config) {\n> > +\t\tconst StreamConfiguration &cfg = config[stream];\n> > \n> > \t\tstream->configuration_ = cfg;\n> > \t\tactiveStreams_.insert(stream);\n> > diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> > index 8a1706fad5377bf4..3a7e7339ccf47707 100644\n> > --- a/src/libcamera/include/pipeline_handler.h\n> > +++ b/src/libcamera/include/pipeline_handler.h\n> > @@ -18,6 +18,7 @@ namespace libcamera {\n> > class Buffer;\n> > class BufferPool;\n> > class Camera;\n> > +class CameraConfiguration;\n> > class CameraManager;\n> > class DeviceEnumerator;\n> > class MediaDevice;\n> > @@ -52,10 +53,9 @@ public:\n> > \n> > \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> > \n> > -\tvirtual std::map\n> > +\tvirtual CameraConfiguration\n> > \tstreamConfiguration(Camera *camera, const std::vector &usages) = 0;\n> > -\tvirtual int configureStreams(Camera *camera,\n> > -\t\t\t\t std::map &config) = 0;\n> > +\tvirtual int configureStreams(Camera *camera, CameraConfiguration &config) = 0;\n> \n> And here, can this be a const reference ?\n\nYes.\n\n> \n> > \n> > \tvirtual int allocateBuffers(Camera *camera, Stream *stream) = 0;\n> > \tvirtual int freeBuffers(Camera *camera, Stream *stream) = 0;\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 5d01504e24471bcf..a2442922a2afe41e 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -139,11 +139,11 @@ public:\n> > \tPipelineHandlerIPU3(CameraManager *manager);\n> > \t~PipelineHandlerIPU3();\n> > \n> > -\tstd::map\n> > +\tCameraConfiguration\n> > \tstreamConfiguration(Camera *camera,\n> > \t\t\t const std::vector &usages) override;\n> > \tint configureStreams(Camera *camera,\n> > -\t\t\t std::map &config) override;\n> > +\t\t\t CameraConfiguration &config) override;\n> > \n> > \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > \tint freeBuffers(Camera *camera, Stream *stream) override;\n> > @@ -204,11 +204,11 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> > \t\timguMediaDev_->release();\n> > }\n> > \n> > -std::map\n> > +CameraConfiguration\n> > PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > \t\t\t\t\t const std::vector &usages)\n> > {\n> > -\tstd::map configs;\n> > +\tCameraConfiguration configs;\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tStreamConfiguration *config = &configs[&data->stream_];\n> > \n> > @@ -234,7 +234,7 @@ PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > }\n> > \n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> > -\t\t\t\t\t std::map &config)\n> > +\t\t\t\t\t CameraConfiguration &config)\n> > {\n> > \tIPU3CameraData *data = cameraData(camera);\n> > \tconst StreamConfiguration &cfg = config[&data->stream_];\n> > diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> > index dfff7116bc2aa5d3..f6da073ba0c47090 100644\n> > --- a/src/libcamera/pipeline/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> > @@ -26,11 +26,11 @@ public:\n> > \tPipelineHandlerUVC(CameraManager *manager);\n> > \t~PipelineHandlerUVC();\n> > \n> > -\tstd::map\n> > +\tCameraConfiguration\n> > \tstreamConfiguration(Camera *camera,\n> > \t\t\t const std::vector &usages) override;\n> > \tint configureStreams(Camera *camera,\n> > -\t\t\t std::map &config) override;\n> > +\t\t\t CameraConfiguration &config) override;\n> > \n> > \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > \tint freeBuffers(Camera *camera, Stream *stream) override;\n> > @@ -82,12 +82,12 @@ PipelineHandlerUVC::~PipelineHandlerUVC()\n> > \t\tmedia_->release();\n> > }\n> > \n> > -std::map\n> > +CameraConfiguration\n> > PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> > \t\t\t\t\tconst std::vector &usages)\n> > {\n> > \tUVCCameraData *data = cameraData(camera);\n> > -\tstd::map configs;\n> > +\tCameraConfiguration configs;\n> > \tStreamConfiguration config{};\n> > \n> > \tconfig.width = 640;\n> > @@ -101,7 +101,7 @@ PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> > }\n> > \n> > int PipelineHandlerUVC::configureStreams(Camera *camera,\n> > -\t\t\t\t\t std::map &config)\n> > +\t\t\t\t\t CameraConfiguration &config)\n> > {\n> > \tUVCCameraData *data = cameraData(camera);\n> > \tStreamConfiguration *cfg = &config[&data->stream_];\n> > diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> > index 4da4ca018d4b942a..6483ac842bf02dc2 100644\n> > --- a/src/libcamera/pipeline/vimc.cpp\n> > +++ b/src/libcamera/pipeline/vimc.cpp\n> > @@ -26,11 +26,11 @@ public:\n> > \tPipelineHandlerVimc(CameraManager *manager);\n> > \t~PipelineHandlerVimc();\n> > \n> > -\tstd::map\n> > +\tCameraConfiguration\n> > \tstreamConfiguration(Camera *camera,\n> > \t\t\t const std::vector &usages) override;\n> > \tint configureStreams(Camera *camera,\n> > -\t\t\t std::map &config) override;\n> > +\t\t\t CameraConfiguration &config) override;\n> > \n> > \tint allocateBuffers(Camera *camera, Stream *stream) override;\n> > \tint freeBuffers(Camera *camera, Stream *stream) override;\n> > @@ -82,12 +82,12 @@ PipelineHandlerVimc::~PipelineHandlerVimc()\n> > \t\tmedia_->release();\n> > }\n> > \n> > -std::map\n> > +CameraConfiguration\n> > PipelineHandlerVimc::streamConfiguration(Camera *camera,\n> > \t\t\t\t\t const std::vector &usages)\n> > {\n> > \tVimcCameraData *data = cameraData(camera);\n> > -\tstd::map configs;\n> > +\tCameraConfiguration configs;\n> > \tStreamConfiguration config{};\n> > \n> > \tconfig.width = 640;\n> > @@ -101,7 +101,7 @@ PipelineHandlerVimc::streamConfiguration(Camera *camera,\n> > }\n> > \n> > int PipelineHandlerVimc::configureStreams(Camera *camera,\n> > -\t\t\t\t std::map &config)\n> > +\t\t\t\t\t CameraConfiguration &config)\n> > {\n> > \tVimcCameraData *data = cameraData(camera);\n> > \tStreamConfiguration *cfg = &config[&data->stream_];\n> > diff --git a/src/qcam/main_window.cpp b/src/qcam/main_window.cpp\n> > index faa3bc5739dd8453..4bc0440370410525 100644\n> > --- a/src/qcam/main_window.cpp\n> > +++ b/src/qcam/main_window.cpp\n> > @@ -98,7 +98,7 @@ int MainWindow::startCapture()\n> > \tint ret;\n> > \n> > \tconfig_ = camera_->streamConfiguration({ Stream::VideoRecording() });\n> > -\tStream *stream = config_.begin()->first;\n> > +\tStream *stream = config_.front();\n> > \tret = camera_->configureStreams(config_);\n> > \tif (ret < 0) {\n> > \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n> > diff --git a/src/qcam/main_window.h b/src/qcam/main_window.h\n> > index 5e27a8fd6b4eb8f3..9b31da2bf4b75a03 100644\n> > --- a/src/qcam/main_window.h\n> > +++ b/src/qcam/main_window.h\n> > @@ -46,7 +46,7 @@ private:\n> > \n> > \tstd::shared_ptr camera_;\n> > \tbool isCapturing_;\n> > -\tstd::map config_;\n> > +\tCameraConfiguration config_;\n> > \n> > \tViewFinder *viewfinder_;\n> > };\n> > diff --git a/test/camera/camera_test.cpp b/test/camera/camera_test.cpp\n> > index 1609c4b02842186a..42172543329e04e4 100644\n> > --- a/test/camera/camera_test.cpp\n> > +++ b/test/camera/camera_test.cpp\n> > @@ -46,15 +46,15 @@ void CameraTest::cleanup()\n> > \tcm_->stop();\n> > };\n> > \n> > -bool CameraTest::configurationValid(const std::map &config) const\n> > +bool CameraTest::configurationValid(CameraConfiguration &config)\n> > {\n> > \t/* Test that the configuration is not empty. */\n> > \tif (config.empty())\n> > \t\treturn false;\n> > \n> > \t/* Test that configuration is valid. */\n> > -\tfor (auto const &it : config) {\n> > -\t\tconst StreamConfiguration &conf = it.second;\n> > +\tfor (Stream *stream : config) {\n> > +\t\tconst StreamConfiguration conf = config[stream];\n> > \n> > \t\tif (conf.width == 0 || conf.height == 0 ||\n> > \t\t conf.pixelFormat == 0 || conf.bufferCount == 0)\n> > diff --git a/test/camera/camera_test.h b/test/camera/camera_test.h\n> > index 5801fad3281e1653..fffa3bf16a9df1a4 100644\n> > --- a/test/camera/camera_test.h\n> > +++ b/test/camera/camera_test.h\n> > @@ -23,7 +23,7 @@ protected:\n> > \tint init();\n> > \tvoid cleanup();\n> > \n> > -\tbool configurationValid(const std::map &config) const;\n> > +\tbool configurationValid(CameraConfiguration &config);\n> \n> const CameraConfiguration & ?\n\nOne better we can delete this function and replace it with the new\nconfig.valid() function.\n\n> \n> > \n> > \tstd::shared_ptr camera_;\n> > \n> > diff --git a/test/camera/capture.cpp b/test/camera/capture.cpp\n> > index b8dbdb62f9a50a33..e7aa3d97989c6c94 100644\n> > --- a/test/camera/capture.cpp\n> > +++ b/test/camera/capture.cpp\n> > @@ -42,10 +42,10 @@ protected:\n> > \n> > \tint run()\n> > \t{\n> > -\t\tstd::map conf =\n> > +\t\tCameraConfiguration conf =\n> > \t\t\tcamera_->streamConfiguration({ Stream::VideoRecording() });\n> > -\t\tStream *stream = conf.begin()->first;\n> > -\t\tStreamConfiguration *sconf = &conf.begin()->second;\n> > +\t\tStream *stream = conf.front();\n> > +\t\tStreamConfiguration *sconf = &conf[stream];\n> > \n> > \t\tif (!configurationValid(conf)) {\n> > \t\t\tcout << \"Failed to read default configuration\" << endl;\n> > diff --git a/test/camera/configuration_default.cpp b/test/camera/configuration_default.cpp\n> > index 09861716973d752c..9ae332368f80e8b9 100644\n> > --- a/test/camera/configuration_default.cpp\n> > +++ b/test/camera/configuration_default.cpp\n> > @@ -18,7 +18,7 @@ class ConfigurationDefault : public CameraTest\n> > protected:\n> > \tint run()\n> > \t{\n> > -\t\tstd::map conf;\n> > +\t\tCameraConfiguration conf;\n> > \n> > \t\t/* Test asking for configuration for a video stream. */\n> > \t\tconf = camera_->streamConfiguration({ Stream::VideoRecording() });\n> > diff --git a/test/camera/configuration_set.cpp b/test/camera/configuration_set.cpp\n> > index 1bc01e66625eedf0..7224f1aa6657523d 100644\n> > --- a/test/camera/configuration_set.cpp\n> > +++ b/test/camera/configuration_set.cpp\n> > @@ -18,9 +18,9 @@ class ConfigurationSet : public CameraTest\n> > protected:\n> > \tint run()\n> > \t{\n> > -\t\tstd::map conf =\n> > +\t\tCameraConfiguration conf =\n> > \t\t\tcamera_->streamConfiguration({ Stream::VideoRecording() });\n> > -\t\tStreamConfiguration *sconf = &conf.begin()->second;\n> > +\t\tStreamConfiguration *sconf = &conf[conf.front()];\n> > \n> > \t\tif (!configurationValid(conf)) {\n> > \t\t\tcout << \"Failed to read default configuration\" << endl;\n> > diff --git a/test/camera/statemachine.cpp b/test/camera/statemachine.cpp\n> > index ab3c6fb5bea38c36..8ae93bee7ca178d4 100644\n> > --- a/test/camera/statemachine.cpp\n> > +++ b/test/camera/statemachine.cpp\n> > @@ -265,7 +265,7 @@ protected:\n> > \t\treturn TestPass;\n> > \t}\n> > \n> > -\tstd::map defconf_;\n> > +\tCameraConfiguration defconf_;\n> > };\n> > \n> > } /* namespace */\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B544760DB3\n\tfor ;\n\tSat, 6 Apr 2019 00:34:34 +0200 (CEST)","by mail-lj1-x22d.google.com with SMTP id l7so6553032ljg.6\n\tfor ;\n\tFri, 05 Apr 2019 15:34:34 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ts16sm3751023lji.61.2019.04.05.15.34.32\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 05 Apr 2019 15:34:32 -0700 (PDT)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to\n\t:user-agent; bh=ktxTl8iApgP0BfWOPMCc7LWdjVouPl4kX3hbyv+iSE8=;\n\tb=wsWH1CuJrz8fM7yP8nrpJyBb7hp6uFOPzXJvSKppNCGTkKnBJD7wySF2osPD17Vmkk\n\tFHqOx2PbeFhFfzzlMnMk6jUQWPVYk15qPObKPbvaKGSb8/pl+EXGYbpEoK5NJiBqowDv\n\tpuGW/Kq6E7SOzkJK26TOofrKRYhC0abd+yk/81gz7m30HuzndenWrdWu3lBekOt7iU+8\n\tIl6VDcrc3E0BPn7ivKgaTHwtVFejLXLCDdQBnbmFe5oq6XeRXU/aJyQ/0r/PHa5JMeuF\n\tDdOofFjNwKYhMfDWKDJpIwV4OvoOMbcKn/1zVu9ZpAlhu9VeCrH0YFX32JzxUxrS8n4h\n\tD0Tw==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to:user-agent;\n\tbh=ktxTl8iApgP0BfWOPMCc7LWdjVouPl4kX3hbyv+iSE8=;\n\tb=SZ9zSEdWDeiHvGnrDi6gxg9CiwOumk7DaVW6nk9pcsfetgVnLiXRY6jCyx6mNnTdHM\n\t+gkjra1mTk7KDF3CZjT/EUuktVNWdGMEvjv4adqvB6soLeM698Rp+QgX4oDRWyJcIbtk\n\tP4unP0m0/G0aFqYH7/h46soIMr3/c7+quf6KPiDn8/hezF9vcZ6wTqhwg9U+bk1HIQD3\n\tr/aKHtCVuQLJbG8ZB2Pq5C2VOE4jlAYLHQN4r/ylJl2pL6CJm74dKpkSrr6/DbyM6ECR\n\tg9NnIVgPj+mA05YvC4NE8em/xWzpfpo3awO5zxhFZlkE6Gv4nFUI1Tir7Yq1NUWBxbNA\n\tGk9Q==","X-Gm-Message-State":"APjAAAVSf3BKjdMTkzRkk1s2iAszyWKHl8UdPIJ9ppSRzKO8MyvQYUti\n\tMt6vcZPvt/QcISkuPIQ0+lJZZpz5KtQ=","X-Google-Smtp-Source":"APXvYqyYRbK5ud3txPCnTjFlIhfKQsdzgHeISqyRNFDDfUZwtfq0AT8rCDuPB2px14IzD5ezr1bXPA==","X-Received":"by 2002:a2e:99c5:: with SMTP id l5mr8122348ljj.55.1554503673983; \n\tFri, 05 Apr 2019 15:34:33 -0700 (PDT)","Date":"Sat, 6 Apr 2019 00:34:31 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190405223431.GB15350@bigcity.dyn.berto.se>","References":"<20190405020256.22520-1-niklas.soderlund@ragnatech.se>\n\t<20190405020256.22520-9-niklas.soderlund@ragnatech.se>\n\t<20190405155333.GF5184@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190405155333.GF5184@pendragon.ideasonboard.com>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v2 8/8] libcamera: Switch to\n\tCameraConfiguration","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 05 Apr 2019 22:34:35 -0000"}}]