[{"id":1338,"web_url":"https://patchwork.libcamera.org/comment/1338/","msgid":"<20190414204518.GP1980@bigcity.dyn.berto.se>","date":"2019-04-14T20:45:18","subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin stream configuration","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your work.\n\nOn 2019-04-09 21:25:47 +0200, Jacopo Mondi wrote:\n> Use and inspect the stream roles provided by the application to\n> streamConfiguration() to assign streams to their intended roles and\n> return a default configuration associated with them.\n> \n> Support a limited number of usages, as the viewfinder stream can\n> optionally be used for capturing still images, but the main output\n> stream cannot be used as viewfinder or for video recording purposes.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++-------\n> 1 file changed, 79 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 75ffdc56d157..70a92783076f 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -227,36 +227,89 @@ CameraConfiguration\n> PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> \t\t\t\t\t const std::vector &usages)\n> {\n> -\tCameraConfiguration configs;\n> \tIPU3CameraData *data = cameraData(camera);\n> -\tStreamConfiguration config = {};\n> +\tCameraConfiguration configs;\n> +\tstd::vector availableStreams = {\n\nUsing a std::set instead of std::vector here would be useful. As you \ncan't have the same stream in the set twice there is no limitation \nmoving from a vector to a set.\n\n> +\t\t&data->outStream_,\n> +\t\t&data->vfStream_,\n> +\t};\n> \n> -\t/*\n> -\t * FIXME: Soraka: the maximum resolution reported by both sensors\n> -\t * (2592x1944 for ov5670 and 4224x3136 for ov13858) are returned as\n> -\t * default configurations but they're not correctly processed by the\n> -\t * ImgU. Resolutions up tp 2560x1920 have been validated.\n> -\t *\n> -\t * \\todo Clarify ImgU alignement requirements.\n> -\t */\n> -\tconfig.width = 2560;\n> -\tconfig.height = 1920;\n> -\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> -\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> -\n> -\tconfigs[&data->outStream_] = config;\n> -\tLOG(IPU3, Debug)\n> -\t\t<< \"Stream 'output' format set to \" << config.width << \"x\"\n> -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> -\t\t<< std::setw(8) << config.pixelFormat;\n> -\n> -\tconfigs[&data->vfStream_] = config;\n> -\tLOG(IPU3, Debug)\n> -\t\t<< \"Stream 'viewfinder' format set to \" << config.width << \"x\"\n> -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> -\t\t<< std::setw(8) << config.pixelFormat;\n> +\tfor (const StreamUsage &usage : usages) {\n> +\t\tstd::vector::iterator found;\n\nCan be refactored away if availableStreams is a std::set, see example \nbellow.\n\n> +\t\tenum StreamUsage::Role r = usage.role();\n\ns/enum//\n\n> +\t\tStreamConfiguration config = {};\n> +\t\tIPU3Stream *stream = nullptr;\n> +\n> +\t\tstd::vector::iterator s = availableStreams.begin();\n> +\t\tif (r == StreamUsage::Role::StillCapture) {\n> +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> +\t\t\t\t/*\n> +\t\t\t\t * We can use the viewfinder stream in case\n> +\t\t\t\t * the 'StillCapture' usage is required\n> +\t\t\t\t * multiple times.\n> +\t\t\t\t */\n> +\t\t\t\tif (*s == &data->outStream_) {\n> +\t\t\t\t\tstream = &data->outStream_;\n> +\t\t\t\t\tfound = s;\n> +\t\t\t\t\tbreak;\n> +\t\t\t\t} else {\n> +\t\t\t\t\tstream = &data->vfStream_;\n> +\t\t\t\t\tfound = s;\n> +\t\t\t\t}\n> +\t\t\t}\n\nThe for() loop can be replaced with this if availableStreams is std::set\n\n\n if (availableStreams.find(&data->outStream_) != availableStreams.end())\n stream = &data->outStream_;\n else\n stream = &data->vfStream_;\n\n> +\n> +\t\t\t/*\n> +\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> +\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> +\t\t\t * ov13858) are returned as default configurations but\n> +\t\t\t * they're not correctly processed by the ImgU.\n> +\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> +\t\t\t *\n> +\t\t\t * \\todo Clarify ImgU alignment requirements.\n> +\t\t\t */\n> +\t\t\tconfig.width = 2560;\n> +\t\t\tconfig.height = 1920;\n> +\t\t} else if (r == StreamUsage::Role::Viewfinder ||\n> +\t\t\t r == StreamUsage::Role::VideoRecording) {\n> +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> +\t\t\t\t/*\n> +\t\t\t\t * We can't use the 'output' stream for\n> +\t\t\t\t * viewfinder or video capture usages.\n> +\t\t\t\t */\n> +\t\t\t\tif (*s != &data->vfStream_)\n> +\t\t\t\t\tcontinue;\n> +\n> +\t\t\t\tstream = &data->vfStream_;\n> +\t\t\t\tfound = s;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tconfig.width = 640;\n> +\t\t\tconfig.height = 480;\n> +\t\t}\n> +\n> +\t\tif (stream == nullptr)\n> +\t\t\tgoto error;\n> +\n> +\t\tavailableStreams.erase(found);\n\nWith std::set\n\n availableStreams.erase(stream);\n\n> +\n> +\t\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> +\t\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\t\tconfigs[stream] = config;\n> +\n> +\t\tLOG(IPU3, Debug)\n> +\t\t\t<< \"Stream \" << stream->name_ << \" format set to \"\n> +\t\t\t<< config.width << \"x\" << config.height\n> +\t\t\t<< \"-0x\" << std::hex << std::setfill('0')\n> +\t\t\t<< std::setw(8) << config.pixelFormat;\n> +\t}\n> \n> \treturn configs;\n> +\n> +error:\n> +\tLOG(IPU3, Error) << \"Requested stream roles not supported\";\n> +\treturn CameraConfiguration{};\n> }\n> \n> int PipelineHandlerIPU3::configureStreams(Camera *camera,\n> -- \n> 2.21.0\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DA99F60DBE\n\tfor ;\n\tSun, 14 Apr 2019 22:45:19 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id w23so11394821lfc.9\n\tfor ;\n\tSun, 14 Apr 2019 13:45:19 -0700 (PDT)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tk25sm9601372ljc.14.2019.04.14.13.45.18\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tSun, 14 Apr 2019 13:45:18 -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=jVjRT/r/0YODc1LPCoQ9ol/YJ8faTsGy5Pc2VxFAPQo=;\n\tb=zKVJAFIGn2O+I40BIPaPymZKZtu59q/GW9m8dHhVO2Tnikz9z7dS1G7JA1gGqCFy6E\n\t2GlFO/dM3e67f6Ciw6ddmYRCDv0QXWAsQkCZJCZvT1KmSDY102gB7as3t/e5VtNc2xoJ\n\tZXhGLCH5anKKrgbrSVrzpUJtaz0lM3y3PIjdhaDVR3OQGgF4f1eN6JmztT8d7t2XyKjD\n\tVrgnOtGMpFzgpVAXYmFZ/jUMM1lkBPjUgCWzDx+F+6htC4YxRA0vabR7Jz/1sHotllxl\n\tcK63gC6RXV4NJBiMXDluTBckAps0NNuAh+98mEmgarcEXzlw26YPbuVYc/etRg/1H973\n\tVX8w==","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=jVjRT/r/0YODc1LPCoQ9ol/YJ8faTsGy5Pc2VxFAPQo=;\n\tb=N2spB/svLyN6SiPqe1PWP+UlQ+/3gdIQZN8fNDpVFu6nhbJM0s/pxAwb5d+Cw0iANO\n\tQCdA5kdorYGDJVBAs48qXrr7NA12JbZ1S8E8WxAIW/9ZKJ+vzhu+TcgIdbEUOnI7fTIm\n\t4PxBX0hOFMLvVj03nDdhpLyy2sT9W+FmOzqEZPan7MaeIOfGWNrTIUnSGbOLZsY9fssG\n\tKhfzvBAqZA5nU5BWfYQfXb4OhgAfNnGa7dFrGST/JUAmRd91BGkQFK7xZtxSYUSFq/HN\n\tdjJrVM16b+BbZugteeyyS/2T0KJRRkNa9FNGoVCv73R8agalkPbwfMUuEF/cpQ9JG556\n\tvVXg==","X-Gm-Message-State":"APjAAAW/vx34TQstR86QAEPyacLtawvRnlF5jnEJ/PyxQ1gf/pNkKpNs\n\tmS2VBe1qPNFWvU+mDv8jHIL8lSJqjj0=","X-Google-Smtp-Source":"APXvYqyrbv+Hr6HlKtu4agKJa/wlZWIvkIGm2XoC4p2IBcwMuRRabD0QdhtI3LBGwD/Ty3RTXIFEEA==","X-Received":"by 2002:ac2:43d8:: with SMTP id\n\tu24mr13499108lfl.94.1555274719182; \n\tSun, 14 Apr 2019 13:45:19 -0700 (PDT)","Date":"Sun, 14 Apr 2019 22:45:18 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190414204518.GP1980@bigcity.dyn.berto.se>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-12-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190409192548.20325-12-jacopo@jmondi.org>","User-Agent":"Mutt/1.11.3 (2019-02-01)","Subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin 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":"Sun, 14 Apr 2019 20:45:20 -0000"}},{"id":1353,"web_url":"https://patchwork.libcamera.org/comment/1353/","msgid":"<20190415144244.GB17083@pendragon.ideasonboard.com>","date":"2019-04-15T14:42:44","subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin stream configuration","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote:\n> On 2019-04-09 21:25:47 +0200, Jacopo Mondi wrote:\n> > Use and inspect the stream roles provided by the application to\n> > streamConfiguration() to assign streams to their intended roles and\n> > return a default configuration associated with them.\n> > \n> > Support a limited number of usages, as the viewfinder stream can\n> > optionally be used for capturing still images, but the main output\n> > stream cannot be used as viewfinder or for video recording purposes.\n\nIs this a limitation of the device, or of the pipeline handler ?\n\n> > \n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++-------\n> > 1 file changed, 79 insertions(+), 26 deletions(-)\n> > \n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 75ffdc56d157..70a92783076f 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -227,36 +227,89 @@ CameraConfiguration\n> > PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > \t\t\t\t\t const std::vector &usages)\n> > {\n> > -\tCameraConfiguration configs;\n> > \tIPU3CameraData *data = cameraData(camera);\n> > -\tStreamConfiguration config = {};\n> > +\tCameraConfiguration configs;\n\nI wouldn't use the plural for a single object. How about\nCameraConfiguration config and StreamConfiguration cfg ? Or\nCameraConfiguration ccfg and StreamConfiguration scfg ? There's also the\noption of cameraConfig and streamConfig but that may be a bit long.\n\n> > +\tstd::vector availableStreams = {\n> \n> Using a std::set instead of std::vector here would be useful. As you \n> can't have the same stream in the set twice there is no limitation \n> moving from a vector to a set.\n> \n> > +\t\t&data->outStream_,\n> > +\t\t&data->vfStream_,\n> > +\t};\n> > \n> > -\t/*\n> > -\t * FIXME: Soraka: the maximum resolution reported by both sensors\n> > -\t * (2592x1944 for ov5670 and 4224x3136 for ov13858) are returned as\n> > -\t * default configurations but they're not correctly processed by the\n> > -\t * ImgU. Resolutions up tp 2560x1920 have been validated.\n> > -\t *\n> > -\t * \\todo Clarify ImgU alignement requirements.\n> > -\t */\n> > -\tconfig.width = 2560;\n> > -\tconfig.height = 1920;\n> > -\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > -\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> > -\n> > -\tconfigs[&data->outStream_] = config;\n> > -\tLOG(IPU3, Debug)\n> > -\t\t<< \"Stream 'output' format set to \" << config.width << \"x\"\n> > -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> > -\t\t<< std::setw(8) << config.pixelFormat;\n> > -\n> > -\tconfigs[&data->vfStream_] = config;\n> > -\tLOG(IPU3, Debug)\n> > -\t\t<< \"Stream 'viewfinder' format set to \" << config.width << \"x\"\n> > -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> > -\t\t<< std::setw(8) << config.pixelFormat;\n> > +\tfor (const StreamUsage &usage : usages) {\n> > +\t\tstd::vector::iterator found;\n> \n> Can be refactored away if availableStreams is a std::set, see example \n> bellow.\n> \n> > +\t\tenum StreamUsage::Role r = usage.role();\n> \n> s/enum//\n\nAnd s/r/role/ ? r is a bit confusing, it also commonly refers to a\nrectangle.\n\n> > +\t\tStreamConfiguration config = {};\n> > +\t\tIPU3Stream *stream = nullptr;\n> > +\n> > +\t\tstd::vector::iterator s = availableStreams.begin();\n> > +\t\tif (r == StreamUsage::Role::StillCapture) {\n> > +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * We can use the viewfinder stream in case\n> > +\t\t\t\t * the 'StillCapture' usage is required\n> > +\t\t\t\t * multiple times.\n> > +\t\t\t\t */\n> > +\t\t\t\tif (*s == &data->outStream_) {\n> > +\t\t\t\t\tstream = &data->outStream_;\n> > +\t\t\t\t\tfound = s;\n> > +\t\t\t\t\tbreak;\n> > +\t\t\t\t} else {\n> > +\t\t\t\t\tstream = &data->vfStream_;\n> > +\t\t\t\t\tfound = s;\n> > +\t\t\t\t}\n> > +\t\t\t}\n> \n> The for() loop can be replaced with this if availableStreams is std::set\n> \n> \n> if (availableStreams.find(&data->outStream_) != availableStreams.end())\n> stream = &data->outStream_;\n> else\n> stream = &data->vfStream_;\n> \n> > +\n> > +\t\t\t/*\n> > +\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> > +\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> > +\t\t\t * ov13858) are returned as default configurations but\n> > +\t\t\t * they're not correctly processed by the ImgU.\n> > +\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> > +\t\t\t *\n> > +\t\t\t * \\todo Clarify ImgU alignment requirements.\n> > +\t\t\t */\n> > +\t\t\tconfig.width = 2560;\n> > +\t\t\tconfig.height = 1920;\n> > +\t\t} else if (r == StreamUsage::Role::Viewfinder ||\n> > +\t\t\t r == StreamUsage::Role::VideoRecording) {\n> > +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> > +\t\t\t\t/*\n> > +\t\t\t\t * We can't use the 'output' stream for\n> > +\t\t\t\t * viewfinder or video capture usages.\n> > +\t\t\t\t */\n> > +\t\t\t\tif (*s != &data->vfStream_)\n> > +\t\t\t\t\tcontinue;\n> > +\n> > +\t\t\t\tstream = &data->vfStream_;\n> > +\t\t\t\tfound = s;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n\nAnd here\n\n\t\t\tif (availableStreams.find(&data->vfStream_) !=\n\t\t\t availableStreams.end())\n\t\t\t\tstream = &data->vfStream_;\n\nYou may want to rename availableStreams to streams if it helps\nshortening lines, up to you.\n\n> > +\n> > +\t\t\tconfig.width = 640;\n> > +\t\t\tconfig.height = 480;\n\nShould you use the resolution requested by the Viewfinder usage ?\n\n> > +\t\t}\n> > +\n> > +\t\tif (stream == nullptr)\n> > +\t\t\tgoto error;\n> > +\n> > +\t\tavailableStreams.erase(found);\n> \n> With std::set\n> \n> availableStreams.erase(stream);\n> \n> > +\n> > +\t\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > +\t\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> > +\n> > +\t\tconfigs[stream] = config;\n> > +\n> > +\t\tLOG(IPU3, Debug)\n> > +\t\t\t<< \"Stream \" << stream->name_ << \" format set to \"\n> > +\t\t\t<< config.width << \"x\" << config.height\n> > +\t\t\t<< \"-0x\" << std::hex << std::setfill('0')\n> > +\t\t\t<< std::setw(8) << config.pixelFormat;\n> > +\t}\n> > \n> > \treturn configs;\n> > +\n> > +error:\n> > +\tLOG(IPU3, Error) << \"Requested stream roles not supported\";\n> > +\treturn CameraConfiguration{};\n> > }\n> > \n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,","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 7F21F600F9\n\tfor ;\n\tMon, 15 Apr 2019 16:42:53 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(dfj612yhrgyx302h3jwwy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CD12A333;\n\tMon, 15 Apr 2019 16:42:52 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555339373;\n\tbh=pQwX+2xZjFai3+qcTflKqXL6W5QE2lIvnssoS7sgyvQ=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=IR9vDG4tuQjMSqVr+vpTeQS3IOzmW0JkfDVaZCKN+wOfXJflkQuhUq/nqxDLF+q0a\n\t84llvBDowHdMSOuF5Nk8CWkTT6AhaCYbrbPX7csw/FkkNKDvd46cAFnnxjFz8Rlxrw\n\tBb4f77jVGu/IIVTxW+RncNdxJa20HTMjYXaGemF4=","Date":"Mon, 15 Apr 2019 17:42:44 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190415144244.GB17083@pendragon.ideasonboard.com>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-12-jacopo@jmondi.org>\n\t<20190414204518.GP1980@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20190414204518.GP1980@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin 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":"Mon, 15 Apr 2019 14:42:53 -0000"}},{"id":1444,"web_url":"https://patchwork.libcamera.org/comment/1444/","msgid":"<20190418101629.ofsljltiucskiedy@uno.localdomain>","date":"2019-04-18T10:16:29","subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin stream configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Mon, Apr 15, 2019 at 05:42:44PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Sun, Apr 14, 2019 at 10:45:18PM +0200, Niklas Söderlund wrote:\n> > On 2019-04-09 21:25:47 +0200, Jacopo Mondi wrote:\n> > > Use and inspect the stream roles provided by the application to\n> > > streamConfiguration() to assign streams to their intended roles and\n> > > return a default configuration associated with them.\n> > >\n> > > Support a limited number of usages, as the viewfinder stream can\n> > > optionally be used for capturing still images, but the main output\n> > > stream cannot be used as viewfinder or for video recording purposes.\n>\n> Is this a limitation of the device, or of the pipeline handler ?\n>\n\nIs a limitation I introduced in the pipeline handler as I assume\nviewfinder frame rate cannot be sustained by the still capture\noutput node. It's an assumption though, but I kept it there to\ndemonstrate how to use roles in assigning configurations.\n\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 105 ++++++++++++++++++++-------\n> > > 1 file changed, 79 insertions(+), 26 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index 75ffdc56d157..70a92783076f 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -227,36 +227,89 @@ CameraConfiguration\n> > > PipelineHandlerIPU3::streamConfiguration(Camera *camera,\n> > > \t\t\t\t\t const std::vector &usages)\n> > > {\n> > > -\tCameraConfiguration configs;\n> > > \tIPU3CameraData *data = cameraData(camera);\n> > > -\tStreamConfiguration config = {};\n> > > +\tCameraConfiguration configs;\n>\n> I wouldn't use the plural for a single object. How about\n> CameraConfiguration config and StreamConfiguration cfg ? Or\n> CameraConfiguration ccfg and StreamConfiguration scfg ? There's also the\n> option of cameraConfig and streamConfig but that may be a bit long.\n>\n> > > +\tstd::vector availableStreams = {\n> >\n> > Using a std::set instead of std::vector here would be useful. As you\n> > can't have the same stream in the set twice there is no limitation\n> > moving from a vector to a set.\n> >\n> > > +\t\t&data->outStream_,\n> > > +\t\t&data->vfStream_,\n> > > +\t};\n> > >\n> > > -\t/*\n> > > -\t * FIXME: Soraka: the maximum resolution reported by both sensors\n> > > -\t * (2592x1944 for ov5670 and 4224x3136 for ov13858) are returned as\n> > > -\t * default configurations but they're not correctly processed by the\n> > > -\t * ImgU. Resolutions up tp 2560x1920 have been validated.\n> > > -\t *\n> > > -\t * \\todo Clarify ImgU alignement requirements.\n> > > -\t */\n> > > -\tconfig.width = 2560;\n> > > -\tconfig.height = 1920;\n> > > -\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > > -\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> > > -\n> > > -\tconfigs[&data->outStream_] = config;\n> > > -\tLOG(IPU3, Debug)\n> > > -\t\t<< \"Stream 'output' format set to \" << config.width << \"x\"\n> > > -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> > > -\t\t<< std::setw(8) << config.pixelFormat;\n> > > -\n> > > -\tconfigs[&data->vfStream_] = config;\n> > > -\tLOG(IPU3, Debug)\n> > > -\t\t<< \"Stream 'viewfinder' format set to \" << config.width << \"x\"\n> > > -\t\t<< config.height << \"-0x\" << std::hex << std::setfill('0')\n> > > -\t\t<< std::setw(8) << config.pixelFormat;\n> > > +\tfor (const StreamUsage &usage : usages) {\n> > > +\t\tstd::vector::iterator found;\n> >\n> > Can be refactored away if availableStreams is a std::set, see example\n> > bellow.\n> >\n> > > +\t\tenum StreamUsage::Role r = usage.role();\n> >\n> > s/enum//\n>\n> And s/r/role/ ? r is a bit confusing, it also commonly refers to a\n> rectangle.\n>\n> > > +\t\tStreamConfiguration config = {};\n> > > +\t\tIPU3Stream *stream = nullptr;\n> > > +\n> > > +\t\tstd::vector::iterator s = availableStreams.begin();\n> > > +\t\tif (r == StreamUsage::Role::StillCapture) {\n> > > +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * We can use the viewfinder stream in case\n> > > +\t\t\t\t * the 'StillCapture' usage is required\n> > > +\t\t\t\t * multiple times.\n> > > +\t\t\t\t */\n> > > +\t\t\t\tif (*s == &data->outStream_) {\n> > > +\t\t\t\t\tstream = &data->outStream_;\n> > > +\t\t\t\t\tfound = s;\n> > > +\t\t\t\t\tbreak;\n> > > +\t\t\t\t} else {\n> > > +\t\t\t\t\tstream = &data->vfStream_;\n> > > +\t\t\t\t\tfound = s;\n> > > +\t\t\t\t}\n> > > +\t\t\t}\n> >\n> > The for() loop can be replaced with this if availableStreams is std::set\n> >\n> >\n> > if (availableStreams.find(&data->outStream_) != availableStreams.end())\n> > stream = &data->outStream_;\n> > else\n> > stream = &data->vfStream_;\n> >\n\nMuch better, thanks!\nJust one note, this is not enough, as we might get asked for 3\nviewfinders, so we have to make sure vfStream_ is available as well,\nand fail eventually if it is not.\n\n> > > +\n> > > +\t\t\t/*\n> > > +\t\t\t * FIXME: Soraka: the maximum resolution reported by\n> > > +\t\t\t * both sensors (2592x1944 for ov5670 and 4224x3136 for\n> > > +\t\t\t * ov13858) are returned as default configurations but\n> > > +\t\t\t * they're not correctly processed by the ImgU.\n> > > +\t\t\t * Resolutions up tp 2560x1920 have been validated.\n> > > +\t\t\t *\n> > > +\t\t\t * \\todo Clarify ImgU alignment requirements.\n> > > +\t\t\t */\n> > > +\t\t\tconfig.width = 2560;\n> > > +\t\t\tconfig.height = 1920;\n> > > +\t\t} else if (r == StreamUsage::Role::Viewfinder ||\n> > > +\t\t\t r == StreamUsage::Role::VideoRecording) {\n> > > +\t\t\tfor (; s < availableStreams.end(); ++s) {\n> > > +\t\t\t\t/*\n> > > +\t\t\t\t * We can't use the 'output' stream for\n> > > +\t\t\t\t * viewfinder or video capture usages.\n> > > +\t\t\t\t */\n> > > +\t\t\t\tif (*s != &data->vfStream_)\n> > > +\t\t\t\t\tcontinue;\n> > > +\n> > > +\t\t\t\tstream = &data->vfStream_;\n> > > +\t\t\t\tfound = s;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n>\n> And here\n>\n> \t\t\tif (availableStreams.find(&data->vfStream_) !=\n> \t\t\t availableStreams.end())\n> \t\t\t\tstream = &data->vfStream_;\n>\n> You may want to rename availableStreams to streams if it helps\n> shortening lines, up to you.\n>\n> > > +\n> > > +\t\t\tconfig.width = 640;\n> > > +\t\t\tconfig.height = 480;\n>\n> Should you use the resolution requested by the Viewfinder usage ?\n>\n\nYes indeed!\n\nThanks, with Niklas and your suggestions this looks much much better!\n\n> > > +\t\t}\n> > > +\n> > > +\t\tif (stream == nullptr)\n> > > +\t\t\tgoto error;\n> > > +\n> > > +\t\tavailableStreams.erase(found);\n> >\n> > With std::set\n> >\n> > availableStreams.erase(stream);\n> >\n> > > +\n> > > +\t\tconfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > > +\t\tconfig.bufferCount = IPU3_BUFFER_COUNT;\n> > > +\n> > > +\t\tconfigs[stream] = config;\n> > > +\n> > > +\t\tLOG(IPU3, Debug)\n> > > +\t\t\t<< \"Stream \" << stream->name_ << \" format set to \"\n> > > +\t\t\t<< config.width << \"x\" << config.height\n> > > +\t\t\t<< \"-0x\" << std::hex << std::setfill('0')\n> > > +\t\t\t<< std::setw(8) << config.pixelFormat;\n> > > +\t}\n> > >\n> > > \treturn configs;\n> > > +\n> > > +error:\n> > > +\tLOG(IPU3, Error) << \"Requested stream roles not supported\";\n> > > +\treturn CameraConfiguration{};\n> > > }\n> > >\n> > > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net\n\t[217.70.183.200])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8706B60DB4\n\tfor ;\n\tThu, 18 Apr 2019 12:15:37 +0200 (CEST)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay7-d.mail.gandi.net (Postfix) with ESMTPSA id DCB7D2000D;\n\tThu, 18 Apr 2019 10:15:36 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Thu, 18 Apr 2019 12:16:29 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"Niklas =?utf-8?q?S=C3=B6derlund?= ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20190418101629.ofsljltiucskiedy@uno.localdomain>","References":"<20190409192548.20325-1-jacopo@jmondi.org>\n\t<20190409192548.20325-12-jacopo@jmondi.org>\n\t<20190414204518.GP1980@bigcity.dyn.berto.se>\n\t<20190415144244.GB17083@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"c326txmeyzbivuvw\"","Content-Disposition":"inline","In-Reply-To":"<20190415144244.GB17083@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v4 11/12] libcamera: ipu3: Use roles\n\tin 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, 18 Apr 2019 10:15:37 -0000"}}]