[{"id":1811,"web_url":"https://patchwork.libcamera.org/comment/1811/","msgid":"<20190610065949.GD4806@pendragon.ideasonboard.com>","date":"2019-06-10T06:59:49","subject":"Re: [libcamera-devel] [PATCH 14/17] cam: Move camera configuration\n\tpreparation to CamApp","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 Mon, May 27, 2019 at 02:15:40AM +0200, Niklas Söderlund wrote:\n> Most of the camera configuration preparation which is done in the\n\ns/which/that/ or s/which is //\n\n> Capture module is not specific to capturing and could be useful for\n> other modules. Extract the generic parts to CamApp and do basic\n> preparation of the configuration before passing it to modules.\n> \n> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>\n\nReviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>\n\n> ---\n>  src/cam/capture.cpp | 84 ++++-----------------------------------------\n>  src/cam/capture.h   |  7 ++--\n>  src/cam/main.cpp    | 75 ++++++++++++++++++++++++++++++++++++++--\n>  3 files changed, 83 insertions(+), 83 deletions(-)\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index a4aa44af25828f23..e455612b36238157 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -15,8 +15,8 @@\n>  \n>  using namespace libcamera;\n>  \n> -Capture::Capture(Camera *camera)\n> -\t: camera_(camera), writer_(nullptr), last_(0)\n> +Capture::Capture(Camera *camera, CameraConfiguration *config)\n> +\t: camera_(camera), config_(config), writer_(nullptr), last_(0)\n>  {\n>  }\n>  \n> @@ -29,13 +29,13 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n>  \t\treturn -ENODEV;\n>  \t}\n>  \n> -\tret = prepareConfig(options);\n> -\tif (ret) {\n> -\t\tstd::cout << \"Failed to prepare camera configuration\" << std::endl;\n> -\t\treturn -EINVAL;\n> +\tstreamName_.clear();\n> +\tfor (unsigned int index = 0; index < config_->size(); ++index) {\n> +\t\tStreamConfiguration &cfg = config_->at(index);\n> +\t\tstreamName_[cfg.stream()] = \"stream\" + std::to_string(index);\n>  \t}\n>  \n> -\tret = camera_->configure(config_.get());\n> +\tret = camera_->configure(config_);\n>  \tif (ret < 0) {\n>  \t\tstd::cout << \"Failed to configure camera\" << std::endl;\n>  \t\treturn ret;\n> @@ -64,80 +64,10 @@ int Capture::run(EventLoop *loop, const OptionsParser::Options &options)\n>  \t}\n>  \n>  \tcamera_->freeBuffers();\n> -\tconfig_.reset();\n>  \n>  \treturn ret;\n>  }\n>  \n> -int Capture::prepareConfig(const OptionsParser::Options &options)\n> -{\n> -\tStreamRoles roles;\n> -\n> -\tif (options.isSet(OptStream)) {\n> -\t\tconst std::vector<OptionValue> &streamOptions =\n> -\t\t\toptions[OptStream].toArray();\n> -\n> -\t\t/* Use roles and get a default configuration. */\n> -\t\tfor (auto const &value : streamOptions) {\n> -\t\t\tKeyValueParser::Options opt = value.toKeyValues();\n> -\n> -\t\t\tif (!opt.isSet(\"role\")) {\n> -\t\t\t\troles.push_back(StreamRole::VideoRecording);\n> -\t\t\t} else if (opt[\"role\"].toString() == \"viewfinder\") {\n> -\t\t\t\troles.push_back(StreamRole::Viewfinder);\n> -\t\t\t} else if (opt[\"role\"].toString() == \"video\") {\n> -\t\t\t\troles.push_back(StreamRole::VideoRecording);\n> -\t\t\t} else if (opt[\"role\"].toString() == \"still\") {\n> -\t\t\t\troles.push_back(StreamRole::StillCapture);\n> -\t\t\t} else {\n> -\t\t\t\tstd::cerr << \"Unknown stream role \"\n> -\t\t\t\t\t  << opt[\"role\"].toString() << std::endl;\n> -\t\t\t\treturn -EINVAL;\n> -\t\t\t}\n> -\t\t}\n> -\t} else {\n> -\t\t/* If no configuration is provided assume a single video stream. */\n> -\t\troles.push_back(StreamRole::VideoRecording);\n> -\t}\n> -\n> -\tconfig_ = camera_->generateConfiguration(roles);\n> -\tif (!config_ || config_->size() != roles.size()) {\n> -\t\tstd::cerr << \"Failed to get default stream configuration\"\n> -\t\t\t  << std::endl;\n> -\t\treturn -EINVAL;\n> -\t}\n> -\n> -\t/* Apply configuration if explicitly requested. */\n> -\tif (options.isSet(OptStream)) {\n> -\t\tconst std::vector<OptionValue> &streamOptions =\n> -\t\t\toptions[OptStream].toArray();\n> -\n> -\t\tunsigned int i = 0;\n> -\t\tfor (auto const &value : streamOptions) {\n> -\t\t\tKeyValueParser::Options opt = value.toKeyValues();\n> -\t\t\tStreamConfiguration &cfg = config_->at(i++);\n> -\n> -\t\t\tif (opt.isSet(\"width\"))\n> -\t\t\t\tcfg.size.width = opt[\"width\"];\n> -\n> -\t\t\tif (opt.isSet(\"height\"))\n> -\t\t\t\tcfg.size.height = opt[\"height\"];\n> -\n> -\t\t\t/* TODO: Translate 4CC string to ID. */\n> -\t\t\tif (opt.isSet(\"pixelformat\"))\n> -\t\t\t\tcfg.pixelFormat = opt[\"pixelformat\"];\n> -\t\t}\n> -\t}\n> -\n> -\tstreamName_.clear();\n> -\tfor (unsigned int index = 0; index < config_->size(); ++index) {\n> -\t\tStreamConfiguration &cfg = config_->at(index);\n> -\t\tstreamName_[cfg.stream()] = \"stream\" + std::to_string(index);\n> -\t}\n> -\n> -\treturn 0;\n> -}\n> -\n>  int Capture::capture(EventLoop *loop)\n>  {\n>  \tint ret;\n> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> index a97d1f44d229c214..1d4a25a84a51403b 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -20,19 +20,18 @@\n>  class Capture\n>  {\n>  public:\n> -\tCapture(libcamera::Camera *camera);\n> +\tCapture(libcamera::Camera *camera,\n> +\t\tlibcamera::CameraConfiguration *config);\n>  \n>  \tint run(EventLoop *loop, const OptionsParser::Options &options);\n>  private:\n> -\tint prepareConfig(const OptionsParser::Options &options);\n> -\n>  \tint capture(EventLoop *loop);\n>  \n>  \tvoid requestComplete(libcamera::Request *request,\n>  \t\t\t     const std::map<libcamera::Stream *, libcamera::Buffer *> &buffers);\n>  \n>  \tlibcamera::Camera *camera_;\n> -\tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n> +\tlibcamera::CameraConfiguration *config_;\n>  \n>  \tstd::map<libcamera::Stream *, std::string> streamName_;\n>  \tBufferWriter *writer_;\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index dbf04917bcc5aa38..338740d1512c7189 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -33,19 +33,21 @@ public:\n>  \n>  private:\n>  \tint parseOptions(int argc, char *argv[]);\n> +\tint prepareConfig();\n>  \tint run();\n>  \n>  \tstatic CamApp *app_;\n>  \tOptionsParser::Options options_;\n>  \tCameraManager *cm_;\n>  \tstd::shared_ptr<Camera> camera_;\n> +\tstd::unique_ptr<libcamera::CameraConfiguration> config_;\n>  \tEventLoop *loop_;\n>  };\n>  \n>  CamApp *CamApp::app_ = nullptr;\n>  \n>  CamApp::CamApp()\n> -\t: cm_(nullptr), camera_(nullptr), loop_(nullptr)\n> +\t: cm_(nullptr), camera_(nullptr), config_(nullptr), loop_(nullptr)\n>  {\n>  \tCamApp::app_ = this;\n>  }\n> @@ -90,6 +92,10 @@ int CamApp::init(int argc, char **argv)\n>  \t\t}\n>  \n>  \t\tstd::cout << \"Using camera \" << camera_->name() << std::endl;\n> +\n> +\t\tret = prepareConfig();\n> +\t\tif (ret)\n> +\t\t\treturn ret;\n>  \t}\n>  \n>  \tloop_ = new EventLoop(cm_->eventDispatcher());\n> @@ -107,6 +113,8 @@ void CamApp::cleanup()\n>  \t\tcamera_.reset();\n>  \t}\n>  \n> +\tconfig_.reset();\n> +\n>  \tcm_->stop();\n>  }\n>  \n> @@ -168,6 +176,69 @@ int CamApp::parseOptions(int argc, char *argv[])\n>  \treturn 0;\n>  }\n>  \n> +int CamApp::prepareConfig()\n> +{\n> +\tStreamRoles roles;\n> +\n> +\tif (options_.isSet(OptStream)) {\n> +\t\tconst std::vector<OptionValue> &streamOptions =\n> +\t\t\toptions_[OptStream].toArray();\n> +\n> +\t\t/* Use roles and get a default configuration. */\n> +\t\tfor (auto const &value : streamOptions) {\n> +\t\t\tKeyValueParser::Options opt = value.toKeyValues();\n> +\n> +\t\t\tif (!opt.isSet(\"role\")) {\n> +\t\t\t\troles.push_back(StreamRole::VideoRecording);\n> +\t\t\t} else if (opt[\"role\"].toString() == \"viewfinder\") {\n> +\t\t\t\troles.push_back(StreamRole::Viewfinder);\n> +\t\t\t} else if (opt[\"role\"].toString() == \"video\") {\n> +\t\t\t\troles.push_back(StreamRole::VideoRecording);\n> +\t\t\t} else if (opt[\"role\"].toString() == \"still\") {\n> +\t\t\t\troles.push_back(StreamRole::StillCapture);\n> +\t\t\t} else {\n> +\t\t\t\tstd::cerr << \"Unknown stream role \"\n> +\t\t\t\t\t  << opt[\"role\"].toString() << std::endl;\n> +\t\t\t\treturn -EINVAL;\n> +\t\t\t}\n> +\t\t}\n> +\t} else {\n> +\t\t/* If no configuration is provided assume a single video stream. */\n> +\t\troles.push_back(StreamRole::VideoRecording);\n> +\t}\n> +\n> +\tconfig_ = camera_->generateConfiguration(roles);\n> +\tif (!config_ || config_->size() != roles.size()) {\n> +\t\tstd::cerr << \"Failed to get default stream configuration\"\n> +\t\t\t  << std::endl;\n> +\t\treturn -EINVAL;\n> +\t}\n> +\n> +\t/* Apply configuration if explicitly requested. */\n> +\tif (options_.isSet(OptStream)) {\n> +\t\tconst std::vector<OptionValue> &streamOptions =\n> +\t\t\toptions_[OptStream].toArray();\n> +\n> +\t\tunsigned int i = 0;\n> +\t\tfor (auto const &value : streamOptions) {\n> +\t\t\tKeyValueParser::Options opt = value.toKeyValues();\n> +\t\t\tStreamConfiguration &cfg = config_->at(i++);\n> +\n> +\t\t\tif (opt.isSet(\"width\"))\n> +\t\t\t\tcfg.size.width = opt[\"width\"];\n> +\n> +\t\t\tif (opt.isSet(\"height\"))\n> +\t\t\t\tcfg.size.height = opt[\"height\"];\n> +\n> +\t\t\t/* TODO: Translate 4CC string to ID. */\n> +\t\t\tif (opt.isSet(\"pixelformat\"))\n> +\t\t\t\tcfg.pixelFormat = opt[\"pixelformat\"];\n> +\t\t}\n> +\t}\n> +\n> +\treturn 0;\n> +}\n> +\n>  int CamApp::run()\n>  {\n>  \tif (options_.isSet(OptList)) {\n> @@ -177,7 +248,7 @@ int CamApp::run()\n>  \t}\n>  \n>  \tif (options_.isSet(OptCapture)) {\n> -\t\tCapture capture(camera_.get());\n> +\t\tCapture capture(camera_.get(), config_.get());\n>  \t\treturn capture.run(loop_, options_);\n>  \t}\n>","headers":{"Return-Path":"<laurent.pinchart@ideasonboard.com>","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 6CC0A6376E\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 10 Jun 2019 09:00:06 +0200 (CEST)","from pendragon.ideasonboard.com (85-76-134-17-nat.elisa-mobile.fi\n\t[85.76.134.17])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 688D8569;\n\tMon, 10 Jun 2019 09:00:05 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1560150006;\n\tbh=D0uwfJEVpkA+R3ae6nJ08JTO9QmTVreQdtv0kd1qaBc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=hx+M39DWf+j2T178a5yse5GSqTtWe/ZRS/+eH7I5gMZRAvpOMxh5uWR/Z43qrLceo\n\tkQCmfc+0KM56Zvj99ni+4N+8HcvAjxrHvc+1kCd8lZxBDsYNGK6JSX6zFARb0emU51\n\tcqbh4LmJJ7fYSYdmFMFkDSjByaBYXierNK+fJphc=","Date":"Mon, 10 Jun 2019 09:59:49 +0300","From":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","To":"Niklas =?utf-8?q?S=C3=B6derlund?= <niklas.soderlund@ragnatech.se>","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190610065949.GD4806@pendragon.ideasonboard.com>","References":"<20190527001543.13593-1-niklas.soderlund@ragnatech.se>\n\t<20190527001543.13593-15-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":"<20190527001543.13593-15-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 14/17] cam: Move camera configuration\n\tpreparation to CamApp","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","X-List-Received-Date":"Mon, 10 Jun 2019 07:00:06 -0000"}}]