[{"id":710,"web_url":"https://patchwork.libcamera.org/comment/710/","msgid":"<20190131000211.GE5358@pendragon.ideasonboard.com>","date":"2019-01-31T00:02:55","subject":"Re: [libcamera-devel] [PATCH v5 5/6] libcamera: pipeline: extend\n\tpipelines to support stream configuration","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 Wed, Jan 30, 2019 at 12:56:14PM +0100, Niklas Söderlund wrote:\n> All streams which are to be used for capture need to be configured at\n> the same time. This allows the pipeline handler to take any dependencies\n> between the different streams and their configuration into account when\n> setting up the hardware.\n> \n> Extend the pipeline API and all pipeline implementations with two new\n> functions, one to read a default configuration and one to set a new\n> configuration. Both functions operate on a group of streams which the\n> pipeline handler should consider when performing the operations.\n> \n> In the current implemented pipelines this is rather easy as they only\n> have one stream each per camera. Furthermore as there is yet no way for\n> the pipeline handlers to interact with the hardware all they do is\n> return a null format and logs that a default configuration have been\n\ns/and logs/, log/\n\n> requested and log that a new configuration have been requested. Future\n\ns/have been requested/has been set/\n\n> work based on more components are needed to make the pipelines return a\n> good default format and actually interact with the hardware.\n> \n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/include/pipeline_handler.h | 7 +++++\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 36 ++++++++++++++++++++++++\n> src/libcamera/pipeline/uvcvideo.cpp | 35 +++++++++++++++++++++++\n> src/libcamera/pipeline/vimc.cpp | 35 +++++++++++++++++++++++\n> src/libcamera/pipeline_handler.cpp | 34 ++++++++++++++++++++++\n> 5 files changed, 147 insertions(+)\n> \n> diff --git a/src/libcamera/include/pipeline_handler.h b/src/libcamera/include/pipeline_handler.h\n> index 080997e22e545e01..b4321f0fa0f765be 100644\n> --- a/src/libcamera/include/pipeline_handler.h\n> +++ b/src/libcamera/include/pipeline_handler.h\n> @@ -18,6 +18,8 @@ class Camera;\n> class CameraManager;\n> class DeviceEnumerator;\n> class MediaDevice;\n> +class Stream;\n> +class StreamConfiguration;\n> \n> class CameraData\n> {\n> @@ -38,6 +40,11 @@ public:\n> \tPipelineHandler(CameraManager *manager);\n> \tvirtual ~PipelineHandler();\n> \n> +\tvirtual std::map\n> +\tstreamConfiguration(Camera *camera, std::vector &streams) = 0;\n\nAs this function doesn't change the state of the PipelineHandler, you\ncan make it\n\n\tvirtual std::map\n\tstreamConfiguration(Camera *camera, std::vector &streams) const = 0;\n\n> +\tvirtual int configureStreams(Camera *camera,\n> +\t\t\t\t std::map &config) = 0;\n\nAnd I would pass a const reference to config:\n\n\tvirtual int configureStreams(Camera *camera,\n\t\t\t\t const std::map &config) = 0;\n\n> +\n> \tvirtual bool match(DeviceEnumerator *enumerator) = 0;\n> \n> protected:\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 52844da78419943d..39553b7c19823a52 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -28,6 +28,12 @@ public:\n> \tPipelineHandlerIPU3(CameraManager *manager);\n> \t~PipelineHandlerIPU3();\n> \n> +\tstd::map\n> +\tstreamConfiguration(Camera *camera,\n> +\t\t\t std::vector &streams);\n> +\tint configureStreams(Camera *camera,\n> +\t\t\t std::map &config);\n> +\n\nA good practice is to mark overloaded functions with override.\n\n> \tbool match(DeviceEnumerator *enumerator);\n> \n> private:\n> @@ -68,6 +74,36 @@ PipelineHandlerIPU3::~PipelineHandlerIPU3()\n> \t\timgu_->release();\n> }\n> \n> +std::map\n> +PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> +\t\t\t\t\t std::vector &streams)\n> +{\n> +\tIPU3CameraData *data = dynamic_cast(cameraData(camera));\n\nWe don't yse dynamic_cast in libcamera (see the Google C++ style guide).\nThe cameraData() method defined by PipelineHandlerIPU3 returns a\nIPU3CameraData pointer already, so you can just do\n\n\tIPU3CameraData *data = cameraData(camera));\n\n> +\tstd::map configs;\n> +\n> +\tStreamConfiguration config{};\n> +\n> +\tLOG(IPU3, Info) << \"TODO: Return a good default format\";\n> +\n> +\tconfigs[&data->stream_] = config;\n> +\n> +\treturn configs;\n> +}\n> +\n> +int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> +\t\t\t\t\t std::map &config)\n> +{\n> +\tIPU3CameraData *data = dynamic_cast(cameraData(camera));\n> +\n> +\tStreamConfiguration *cfg = &config[&data->stream_];\n\nThis will throw an exception if the stream isn't part of the config. It\nmay be acceptable in a stub implementation, but will need to be fixed\nvery soon.\n\nAll these comments apply to the other pipeline handlers.\n\n> +\n> +\tLOG(IPU3, Info) << \"TODO: Configure the camera for resolution \" <<\n> +\t\tcfg->width << \"x\" << cfg->height;\n> +\n> +\treturn 0;\n> +}\n> +\n> bool PipelineHandlerIPU3::match(DeviceEnumerator *enumerator)\n> {\n> \tDeviceMatch cio2_dm(\"ipu3-cio2\");\n> diff --git a/src/libcamera/pipeline/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo.cpp\n> index 8ea7ac74d2a5395d..4647ed95e929940a 100644\n> --- a/src/libcamera/pipeline/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo.cpp\n> @@ -9,18 +9,27 @@\n> #include \n> \n> #include \"device_enumerator.h\"\n> +#include \"log.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> #include \"v4l2_device.h\"\n> \n> namespace libcamera {\n> \n> +LOG_DEFINE_CATEGORY(UVC)\n> +\n> class PipelineHandlerUVC : public PipelineHandler\n> {\n> public:\n> \tPipelineHandlerUVC(CameraManager *manager);\n> \t~PipelineHandlerUVC();\n> \n> +\tstd::map\n> +\tstreamConfiguration(Camera *camera,\n> +\t\t\t std::vector &streams);\n> +\tint configureStreams(Camera *camera,\n> +\t\t\t std::map &config);\n> +\n> \tbool match(DeviceEnumerator *enumerator);\n> \n> private:\n> @@ -43,6 +52,32 @@ PipelineHandlerUVC::~PipelineHandlerUVC()\n> \t\tmedia_->release();\n> }\n> \n> +std::map\n> +PipelineHandlerUVC::streamConfiguration(Camera *camera,\n> +\t\t\t\t\tstd::vector &streams)\n> +{\n> +\tstd::map configs;\n> +\n> +\tStreamConfiguration config{};\n> +\n> +\tLOG(UVC, Info) << \"TODO: Return a good default format\";\n> +\n> +\tconfigs[&stream_] = config;\n> +\n> +\treturn configs;\n> +}\n> +\n> +int PipelineHandlerUVC::configureStreams(Camera *camera,\n> +\t\t\t\t\t std::map &config)\n> +{\n> +\tStreamConfiguration *cfg = &config[&stream_];\n> +\n> +\tLOG(UVC, Info) << \"TODO: Configure the camera for resolution \" <<\n> +\t\tcfg->width << \"x\" << cfg->height;\n> +\n> +\treturn 0;\n> +}\n> +\n> bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> {\n> \tDeviceMatch dm(\"uvcvideo\");\n> diff --git a/src/libcamera/pipeline/vimc.cpp b/src/libcamera/pipeline/vimc.cpp\n> index 9e1cf11a20c7a7e3..835f8f512c4ac53a 100644\n> --- a/src/libcamera/pipeline/vimc.cpp\n> +++ b/src/libcamera/pipeline/vimc.cpp\n> @@ -9,17 +9,26 @@\n> #include \n> \n> #include \"device_enumerator.h\"\n> +#include \"log.h\"\n> #include \"media_device.h\"\n> #include \"pipeline_handler.h\"\n> \n> namespace libcamera {\n> \n> +LOG_DEFINE_CATEGORY(VIMC)\n> +\n> class PipeHandlerVimc : public PipelineHandler\n> {\n> public:\n> \tPipeHandlerVimc(CameraManager *manager);\n> \t~PipeHandlerVimc();\n> \n> +\tstd::map\n> +\tstreamConfiguration(Camera *camera,\n> +\t\t\t std::vector &streams);\n> +\tint configureStreams(Camera *camera,\n> +\t\t\t std::map &config);\n> +\n> \tbool match(DeviceEnumerator *enumerator);\n> \n> private:\n> @@ -38,6 +47,32 @@ PipeHandlerVimc::~PipeHandlerVimc()\n> \t\tmedia_->release();\n> }\n> \n> +std::map\n> +PipeHandlerVimc::streamConfiguration(Camera *camera,\n> +\t\t\t\t std::vector &streams)\n> +{\n> +\tstd::map configs;\n> +\n> +\tStreamConfiguration config{};\n> +\n> +\tLOG(VIMC, Info) << \"TODO: Return a good default format\";\n> +\n> +\tconfigs[&stream_] = config;\n> +\n> +\treturn configs;\n> +}\n> +\n> +int PipeHandlerVimc::configureStreams(Camera *camera,\n> +\t\t\t\t std::map &config)\n> +{\n> +\tStreamConfiguration *cfg = &config[&stream_];\n> +\n> +\tLOG(VIMC, Info) << \"TODO: Configure the camera for resolution \" <<\n> +\t\tcfg->width << \"x\" << cfg->height;\n> +\n> +\treturn 0;\n> +}\n> +\n> bool PipeHandlerVimc::match(DeviceEnumerator *enumerator)\n> {\n> \tDeviceMatch dm(\"vimc\");\n> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp\n> index 1c8640799f714686..3a560e10c442717f 100644\n> --- a/src/libcamera/pipeline_handler.cpp\n> +++ b/src/libcamera/pipeline_handler.cpp\n> @@ -75,6 +75,40 @@ PipelineHandler::~PipelineHandler()\n> {\n> };\n> \n> +\n> +/**\n> + * \\fn PipelineHandler::streamConfiguration()\n> + * \\brief Retrieve a group of stream configurations for a specified camera\n> + * \\param[in] camera The camera to fetch default configuration from\n> + * \\param[in] streams An array of streams to fetch information about\n> + *\n> + * Retrieve the species camera's default configuration for a specified group of\n> + * streams. The caller shall populate the \\a streams array with the streams it\n> + * wish to fetch the configuration from. The map of streams and configuration\n> + * returned can then be examined by the caller to learn about the defualt\n> + * parameters for the specified streams.\n> + *\n> + * The intended companion to this is \\a configureStreams() which can be used to\n> + * change the group of streams parameters.\n> + *\n> + * \\return A map of successfully retrieved streams and configurations or an\n> + * empty map on error.\n> + */\n> +\n> +/**\n> + * \\fn PipelineHandler::configureStreams()\n> + * \\brief Configure a group of streams for capture\n> + * \\param[in] camera The camera to configure\n> + * \\param[in] config A map of stream configurations to apply\n> + *\n> + * Configure the specified group of streams for \\a camera according to the\n> + * configuration specified in \\a configs. The intended caller of this interface\n> + * is the Camera class which will receive configuration to apply from the\n> + * application.\n> + *\n> + * \\return 0 on success or a negative error code on error.\n> + */\n> +\n> /**\n> * \\fn PipelineHandler::match(DeviceEnumerator *enumerator)\n> * \\brief Match media devices and create camera instances","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 12D6260C78\n\tfor ;\n\tThu, 31 Jan 2019 01:03:02 +0100 (CET)","from pendragon.ideasonboard.com (85-76-34-136-nat.elisa-mobile.fi\n\t[85.76.34.136])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 206B041;\n\tThu, 31 Jan 2019 01:02:59 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1548892981;\n\tbh=kqUNrdjJtqQMzbCY4N/w1N+HneTWZYthuZwARi/lWPw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=p7TgJpE4yyRJrD+1CO2M7D324Opu908wzpGvgVTKb+1/AL9Ps8mjYHLz7IqSoSzJX\n\tr1ck2J7PsUkp+gLYffQQZHzEwdR1UeVSt1falE7EYHf9j4YH2Ug2Gfq2JAGpSwKTtI\n\t1lHHv+ERTau3IrlVb0d/U6NFpppYdF3KR7RepJrE=","Date":"Thu, 31 Jan 2019 02:02:55 +0200","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190131000211.GE5358@pendragon.ideasonboard.com>","References":"<20190130115615.17362-1-niklas.soderlund@ragnatech.se>\n\t<20190130115615.17362-6-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":"<20190130115615.17362-6-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v5 5/6] libcamera: pipeline: extend\n\tpipelines to support stream configuration","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":"Thu, 31 Jan 2019 00:03:02 -0000"}}]