[{"id":1465,"web_url":"https://patchwork.libcamera.org/comment/1465/","msgid":"<20190418203923.GB4794@pendragon.ideasonboard.com>","date":"2019-04-18T20:39:23","subject":"Re: [libcamera-devel] [PATCH v6 2/6] libcamera: ipu3: Use roles in\n\tstream 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 Thu, Apr 18, 2019 at 06:46:34PM +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\nThis is an artificial limitation, isn't it ? I'd like that to be\nrecorded in the commit message. Maybe\n\nSupport a limited number of usages, with the viewfinder stream able to\ncapture both continuous video streams and still images, and the main\noutput stream supporting still images images. This is an artificial\nlimitation until we figure out the exact capabilities of the hardware.\n\n> Signed-off-by: Jacopo Mondi \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++---------\n> 1 file changed, 69 insertions(+), 30 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index b9eb872fd5b2..8c2504499f33 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -221,38 +221,77 @@ 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 cameraConfig;\n> +\tstd::set streams = {\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 '\" << data->outStream_.name_ << \"' set to \"\n> -\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> -\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> -\t\t<< config.pixelFormat;\n> -\n> -\tconfigs[&data->vfStream_] = config;\n> -\tLOG(IPU3, Debug)\n> -\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' set to \"\n> -\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> -\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> -\t\t<< config.pixelFormat;\n> -\n> -\treturn configs;\n> +\tfor (const StreamUsage &usage : usages) {\n> +\t\tstd::vector::iterator found;\n> +\t\tStreamUsage::Role role = usage.role();\n> +\t\tStreamConfiguration streamConfig = {};\n> +\t\tIPU3Stream *stream = nullptr;\n> +\n> +\t\tif (role == StreamUsage::Role::StillCapture) {\n> +\t\t\t/*\n> +\t\t\t * We can use the viewfinder stream in case the\n> +\t\t\t * 'StillCapture' usage is required multiple times.\n> +\t\t\t */\n> +\t\t\tif (streams.find(&data->outStream_) != streams.end())\n> +\t\t\t\tstream = &data->outStream_;\n> +\t\t\telse if (streams.find(&data->vfStream_) != streams.end())\n> +\t\t\t\tstream = &data->vfStream_;\n> +\t\t\telse\n> +\t\t\t\tgoto error;\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\tstreamConfig.width = 2560;\n> +\t\t\tstreamConfig.height = 1920;\n> +\t\t} else if (role == StreamUsage::Role::Viewfinder ||\n> +\t\t\t role == StreamUsage::Role::VideoRecording) {\n> +\t\t\t/*\n> +\t\t\t * We can't use the 'output' stream for viewfinder or\n> +\t\t\t * video capture usages.\n\nAnd here, \"Don't allow viewfinder or video capture on the 'output'\nstream. This is an artificial limitation until we figure out the\ncapabilities of the hardware.\"\n\n> +\t\t\t */\n> +\t\t\tif (streams.find(&data->vfStream_) == streams.end())\n> +\t\t\t\tgoto error;\n> +\n> +\t\t\tstream = &data->vfStream_;\n> +\n> +\t\t\tstreamConfig.width = usage.size().width;\n> +\t\t\tstreamConfig.height = usage.size().height;\n\nShould this be rounded to the alignment requirements of the ImgU, and\nclamped to the minimum - maximum achievable sizes ?\n\n> +\t\t}\n\nYou will crash if none of those roles match. Let's use a switch() ...\ncase and return an error in the default case.\n\n> +\n> +\t\tstreams.erase(stream);\n> +\n> +\t\tstreamConfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> +\t\tstreamConfig.bufferCount = IPU3_BUFFER_COUNT;\n> +\n> +\t\tcameraConfig[stream] = streamConfig;\n> +\n> +\t\tLOG(IPU3, Debug)\n> +\t\t\t<< \"Stream '\" << stream->name_ << \"' format set to \"\n> +\t\t\t<< streamConfig.width << \"x\" << streamConfig.height\n> +\t\t\t<< \"-0x\" << std::hex << std::setfill('0')\n> +\t\t\t<< std::setw(8) << streamConfig.pixelFormat;\n\nHere too you can use streamConfig.toString() if my patches get merged\nfirst. Otherwise I'll rebase them.\n\n> +\t}\n> +\n> +\treturn cameraConfig;\n> +\n> +error:\n> +\tLOG(IPU3, Error) << \"Requested stream roles not supported\";\n> +\n> +\treturn CameraConfiguration{};\n> }\n> \n> int PipelineHandlerIPU3::configureStreams(Camera *camera,","headers":{"Return-Path":"","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 661EA600F9\n\tfor ;\n\tThu, 18 Apr 2019 22:39:40 +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 CDC8B333;\n\tThu, 18 Apr 2019 22:39:39 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555619980;\n\tbh=TovHrdEXBg/MxRnrgufpD4fa9D0ipmSqsep7dXE+1MI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=JKZxkpy9kN5E0Kpgz9AuuRpMz2XUap17wMsictAe6EhM1cmCa6z+O81vtXTvunEjv\n\tUcGjyQlDAbRXCQk1pKQLBbK+WA000Jzh4PhmrdVMoI51CzQ7BggqVxcWhwh0e/TLsY\n\trFozrrhbHJdrgnCZA5pfpOshpbm4bZiPdZPgVKTU=","Date":"Thu, 18 Apr 2019 23:39:23 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190418203923.GB4794@pendragon.ideasonboard.com>","References":"<20190418164638.400-1-jacopo@jmondi.org>\n\t<20190418164638.400-3-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190418164638.400-3-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v6 2/6] libcamera: ipu3: Use roles in\n\tstream 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 20:39:40 -0000"}},{"id":1469,"web_url":"https://patchwork.libcamera.org/comment/1469/","msgid":"<20190419064708.6wz2lozuonecvo6s@uno.localdomain>","date":"2019-04-19T06:47:08","subject":"Re: [libcamera-devel] [PATCH v6 2/6] libcamera: ipu3: Use roles in\n\tstream configuration","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"HI Laurent,\n\nOn Thu, Apr 18, 2019 at 11:39:23PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Thu, Apr 18, 2019 at 06:46:34PM +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> This is an artificial limitation, isn't it ? I'd like that to be\n> recorded in the commit message. Maybe\n>\n> Support a limited number of usages, with the viewfinder stream able to\n> capture both continuous video streams and still images, and the main\n> output stream supporting still images images. This is an artificial\n> limitation until we figure out the exact capabilities of the hardware.\n>\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 99 +++++++++++++++++++---------\n> > 1 file changed, 69 insertions(+), 30 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index b9eb872fd5b2..8c2504499f33 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -221,38 +221,77 @@ 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 cameraConfig;\n> > +\tstd::set streams = {\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 '\" << data->outStream_.name_ << \"' set to \"\n> > -\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> > -\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> > -\t\t<< config.pixelFormat;\n> > -\n> > -\tconfigs[&data->vfStream_] = config;\n> > -\tLOG(IPU3, Debug)\n> > -\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' set to \"\n> > -\t\t<< config.width << \"x\" << config.height << \"-0x\"\n> > -\t\t<< std::hex << std::setfill('0') << std::setw(8)\n> > -\t\t<< config.pixelFormat;\n> > -\n> > -\treturn configs;\n> > +\tfor (const StreamUsage &usage : usages) {\n> > +\t\tstd::vector::iterator found;\n> > +\t\tStreamUsage::Role role = usage.role();\n> > +\t\tStreamConfiguration streamConfig = {};\n> > +\t\tIPU3Stream *stream = nullptr;\n> > +\n> > +\t\tif (role == StreamUsage::Role::StillCapture) {\n> > +\t\t\t/*\n> > +\t\t\t * We can use the viewfinder stream in case the\n> > +\t\t\t * 'StillCapture' usage is required multiple times.\n> > +\t\t\t */\n> > +\t\t\tif (streams.find(&data->outStream_) != streams.end())\n> > +\t\t\t\tstream = &data->outStream_;\n> > +\t\t\telse if (streams.find(&data->vfStream_) != streams.end())\n> > +\t\t\t\tstream = &data->vfStream_;\n> > +\t\t\telse\n> > +\t\t\t\tgoto error;\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\tstreamConfig.width = 2560;\n> > +\t\t\tstreamConfig.height = 1920;\n> > +\t\t} else if (role == StreamUsage::Role::Viewfinder ||\n> > +\t\t\t role == StreamUsage::Role::VideoRecording) {\n> > +\t\t\t/*\n> > +\t\t\t * We can't use the 'output' stream for viewfinder or\n> > +\t\t\t * video capture usages.\n>\n> And here, \"Don't allow viewfinder or video capture on the 'output'\n> stream. This is an artificial limitation until we figure out the\n> capabilities of the hardware.\"\n>\n> > +\t\t\t */\n> > +\t\t\tif (streams.find(&data->vfStream_) == streams.end())\n> > +\t\t\t\tgoto error;\n> > +\n> > +\t\t\tstream = &data->vfStream_;\n> > +\n> > +\t\t\tstreamConfig.width = usage.size().width;\n> > +\t\t\tstreamConfig.height = usage.size().height;\n>\n> Should this be rounded to the alignment requirements of the ImgU, and\n> clamped to the minimum - maximum achievable sizes ?\n>\n\nI should get them from the camera sensor, at least the max size...\n\n> > +\t\t}\n>\n> You will crash if none of those roles match. Let's use a switch() ...\n> case and return an error in the default case.\n>\n\nActully there are no more roles to match, but this will hopefully\nchange soon. I'll goto error in that case.\n\n> > +\n> > +\t\tstreams.erase(stream);\n> > +\n> > +\t\tstreamConfig.pixelFormat = V4L2_PIX_FMT_NV12;\n> > +\t\tstreamConfig.bufferCount = IPU3_BUFFER_COUNT;\n> > +\n> > +\t\tcameraConfig[stream] = streamConfig;\n> > +\n> > +\t\tLOG(IPU3, Debug)\n> > +\t\t\t<< \"Stream '\" << stream->name_ << \"' format set to \"\n> > +\t\t\t<< streamConfig.width << \"x\" << streamConfig.height\n> > +\t\t\t<< \"-0x\" << std::hex << std::setfill('0')\n> > +\t\t\t<< std::setw(8) << streamConfig.pixelFormat;\n>\n> Here too you can use streamConfig.toString() if my patches get merged\n> first. Otherwise I'll rebase them.\n>\n\nIt just depends which one will get in first.\n\nThanks\n j\n\n> > +\t}\n> > +\n> > +\treturn cameraConfig;\n> > +\n> > +error:\n> > +\tLOG(IPU3, Error) << \"Requested stream roles not supported\";\n> > +\n> > +\treturn CameraConfiguration{};\n> > }\n> >\n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7AAD260B1B\n\tfor ;\n\tFri, 19 Apr 2019 08:46:15 +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 relay2-d.mail.gandi.net (Postfix) with ESMTPSA id EF5E540008;\n\tFri, 19 Apr 2019 06:46:14 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 19 Apr 2019 08:47:08 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419064708.6wz2lozuonecvo6s@uno.localdomain>","References":"<20190418164638.400-1-jacopo@jmondi.org>\n\t<20190418164638.400-3-jacopo@jmondi.org>\n\t<20190418203923.GB4794@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"nevqfhoaasxjtciy\"","Content-Disposition":"inline","In-Reply-To":"<20190418203923.GB4794@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v6 2/6] libcamera: ipu3: Use roles in\n\tstream 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":"Fri, 19 Apr 2019 06:46:15 -0000"}}]