[{"id":1490,"web_url":"https://patchwork.libcamera.org/comment/1490/","msgid":"<20190419133527.GL7006@pendragon.ideasonboard.com>","date":"2019-04-19T13:35:27","subject":"Re: [libcamera-devel] [PATCH v8 4/8] 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 Fri, Apr 19, 2019 at 03:25:27PM +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, with the viewfinder stream able to\n> capture both continuous video streams and still images, and the main\n> output stream supporting still images only. 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 | 124 +++++++++++++++++++++------\n> 1 file changed, 98 insertions(+), 26 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index 46384d88dddd..0130a83973ca 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -5,6 +5,7 @@\n> * ipu3.cpp - Pipeline handler for Intel IPU3\n> */\n> \n> +#include \n> #include \n> #include \n> #include \n> @@ -221,34 +222,105 @@ 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_ << \"' format set to \"\n> -\t\t<< config.toString();\n> -\n> -\tconfigs[&data->vfStream_] = config;\n> -\tLOG(IPU3, Debug)\n> -\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' format set to \"\n> -\t\t<< config.toString();\n> -\n> -\treturn configs;\n> +\tfor (const StreamUsage &usage : usages) {\n> +\t\tStreamConfiguration streamConfig = {};\n> +\t\tStreamUsage::Role role = usage.role();\n> +\t\tIPU3Stream *stream = nullptr;\n> +\n> +\t\tswitch (role) {\n> +\t\tcase StreamUsage::Role::StillCapture:\n> +\t\t\t/*\n> +\t\t\t * Don't allow viewfinder or video capture on the\n> +\t\t\t * 'output' stream. This is an artificial limitation\n> +\t\t\t * until we figure out the capabilities of the\n> +\t\t\t * hardware.\n\nThis comment is a bit confusing. How about \"Pick the output stream by\ndefault as the Viewfinder and VideoRecording roles are not allowed on\nthe viewfinder stream.\" ?\n\n> +\t\t\t */\n> +\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> +\t\t\t\tstream = &data->outStream_;\n> +\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> +\t\t\t\tstream = &data->vfStream_;\n> +\t\t\t} else {\n> +\t\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t\t<< \"No stream available for requested role \"\n> +\t\t\t\t\t<< role;\n> +\t\t\t\tbreak;\n> +\t\t\t}\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> +\n> +\t\t\tbreak;\n\nPlease add a blank line here.\n\n> +\t\tcase StreamUsage::Role::Viewfinder:\n> +\t\tcase 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> +\t\t\t *\n> +\t\t\t * \\todo This is an artificial limitation until we\n> +\t\t\t * figure out the exact capabilities of the hardware.\n> +\t\t\t */\n> +\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> +\t\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t\t<< \"No stream available for requested role \"\n> +\t\t\t\t\t<< role;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\n> +\t\t\tstream = &data->vfStream_;\n> +\n> +\t\t\t/*\n> +\t\t\t * Align the requested viewfinder size to the\n> +\t\t\t * maximum available sensor resolution and to the\n> +\t\t\t * IPU3 alignment constraints.\n> +\t\t\t */\n> +\t\t\tconst Size &res = data->cio2_.sensor_->resolution();\n> +\t\t\tunsigned int width = std::min(usage.size().width,\n> +\t\t\t\t\t\t res.width);\n> +\t\t\tunsigned int height = std::min(usage.size().height,\n> +\t\t\t\t\t\t res.height);\n> +\t\t\tstreamConfig.width = width & ~7;\n> +\t\t\tstreamConfig.height = height & ~3;\n> +\n> +\t\t\tbreak;\n> +\t\t}\n\nAnd a blank line here too.\n\n> +\t\tdefault:\n> +\t\t\tLOG(IPU3, Error)\n> +\t\t\t\t<< \"Requested stream role not supported: \" << role;\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!stream)\n> +\t\t\treturn cameraConfig;\n\nThis will return a valid configuration if the first role was fulfilled\nbut the first one wasn't. You need to return CameraConfiguration{} here.\n\nWith this fixed,\n\nReviewed-by: Laurent Pinchart \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.toString();\n> +\t}\n> +\n> +\treturn cameraConfig;\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 2D92F60B2E\n\tfor ;\n\tFri, 19 Apr 2019 15:35:45 +0200 (CEST)","from pendragon.ideasonboard.com (unknown\n\t[IPv6:2001:14ba:21f5:5b00:ce28:277f:58d7:3ca4])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8835331A;\n\tFri, 19 Apr 2019 15:35:44 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1555680944;\n\tbh=GUA3z7h8qqqwrATA8BLFMh0vjPg5IMHNCZHKXICXtRc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=GksgGTseSD5pjwbpyH2ms8G5HGF3D3r4DYxSM19Jg7ykBaGRKdppRf2TGvtTIukdH\n\tI68mbcfT6pvL6N4iuaNZbXz0oON4ub7GMqKj/12qfFU9yduo5lPa9cuihCXKAinqLO\n\t5aquEW8oU9CygbIEkYOigxLmOMXUZP5uZ1ctghM0=","Date":"Fri, 19 Apr 2019 16:35:27 +0300","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419133527.GL7006@pendragon.ideasonboard.com>","References":"<20190419132531.17856-1-jacopo@jmondi.org>\n\t<20190419132531.17856-5-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20190419132531.17856-5-jacopo@jmondi.org>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH v8 4/8] 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 13:35:45 -0000"}},{"id":1491,"web_url":"https://patchwork.libcamera.org/comment/1491/","msgid":"<20190419135905.vjtoyx2uxzhjvqyy@uno.localdomain>","date":"2019-04-19T13:59:05","subject":"Re: [libcamera-devel] [PATCH v8 4/8] 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 Fri, Apr 19, 2019 at 04:35:27PM +0300, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Apr 19, 2019 at 03:25:27PM +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, with the viewfinder stream able to\n> > capture both continuous video streams and still images, and the main\n> > output stream supporting still images only. 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 | 124 +++++++++++++++++++++------\n> > 1 file changed, 98 insertions(+), 26 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index 46384d88dddd..0130a83973ca 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -5,6 +5,7 @@\n> > * ipu3.cpp - Pipeline handler for Intel IPU3\n> > */\n> >\n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -221,34 +222,105 @@ 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_ << \"' format set to \"\n> > -\t\t<< config.toString();\n> > -\n> > -\tconfigs[&data->vfStream_] = config;\n> > -\tLOG(IPU3, Debug)\n> > -\t\t<< \"Stream '\" << data->vfStream_.name_ << \"' format set to \"\n> > -\t\t<< config.toString();\n> > -\n> > -\treturn configs;\n> > +\tfor (const StreamUsage &usage : usages) {\n> > +\t\tStreamConfiguration streamConfig = {};\n> > +\t\tStreamUsage::Role role = usage.role();\n> > +\t\tIPU3Stream *stream = nullptr;\n> > +\n> > +\t\tswitch (role) {\n> > +\t\tcase StreamUsage::Role::StillCapture:\n> > +\t\t\t/*\n> > +\t\t\t * Don't allow viewfinder or video capture on the\n> > +\t\t\t * 'output' stream. This is an artificial limitation\n> > +\t\t\t * until we figure out the capabilities of the\n> > +\t\t\t * hardware.\n>\n> This comment is a bit confusing. How about \"Pick the output stream by\n> default as the Viewfinder and VideoRecording roles are not allowed on\n> the viewfinder stream.\" ?\n>\n\nTaken in with\ns/viewfinder stream/output stream/\n\n> > +\t\t\t */\n> > +\t\t\tif (streams.find(&data->outStream_) != streams.end()) {\n> > +\t\t\t\tstream = &data->outStream_;\n> > +\t\t\t} else if (streams.find(&data->vfStream_) != streams.end()) {\n> > +\t\t\t\tstream = &data->vfStream_;\n> > +\t\t\t} else {\n> > +\t\t\t\tLOG(IPU3, Error)\n> > +\t\t\t\t\t<< \"No stream available for requested role \"\n> > +\t\t\t\t\t<< role;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\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> > +\n> > +\t\t\tbreak;\n>\n> Please add a blank line here.\n>\n> > +\t\tcase StreamUsage::Role::Viewfinder:\n> > +\t\tcase 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> > +\t\t\t *\n> > +\t\t\t * \\todo This is an artificial limitation until we\n> > +\t\t\t * figure out the exact capabilities of the hardware.\n> > +\t\t\t */\n> > +\t\t\tif (streams.find(&data->vfStream_) == streams.end()) {\n> > +\t\t\t\tLOG(IPU3, Error)\n> > +\t\t\t\t\t<< \"No stream available for requested role \"\n> > +\t\t\t\t\t<< role;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\n> > +\t\t\tstream = &data->vfStream_;\n> > +\n> > +\t\t\t/*\n> > +\t\t\t * Align the requested viewfinder size to the\n> > +\t\t\t * maximum available sensor resolution and to the\n> > +\t\t\t * IPU3 alignment constraints.\n> > +\t\t\t */\n> > +\t\t\tconst Size &res = data->cio2_.sensor_->resolution();\n> > +\t\t\tunsigned int width = std::min(usage.size().width,\n> > +\t\t\t\t\t\t res.width);\n> > +\t\t\tunsigned int height = std::min(usage.size().height,\n> > +\t\t\t\t\t\t res.height);\n> > +\t\t\tstreamConfig.width = width & ~7;\n> > +\t\t\tstreamConfig.height = height & ~3;\n> > +\n> > +\t\t\tbreak;\n> > +\t\t}\n>\n> And a blank line here too.\n>\n> > +\t\tdefault:\n> > +\t\t\tLOG(IPU3, Error)\n> > +\t\t\t\t<< \"Requested stream role not supported: \" << role;\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\n> > +\t\tif (!stream)\n> > +\t\t\treturn cameraConfig;\n>\n> This will return a valid configuration if the first role was fulfilled\n> but the first one wasn't. You need to return CameraConfiguration{} here.\n\nIndeed.\n\n>\n> With this fixed,\n>\n> Reviewed-by: Laurent Pinchart \n\nThanks\n j\n\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.toString();\n> > +\t}\n> > +\n> > +\treturn cameraConfig;\n> > }\n> >\n> > int PipelineHandlerIPU3::configureStreams(Camera *camera,\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"","Received":["from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 98D7A60B2E\n\tfor ;\n\tFri, 19 Apr 2019 15:58:12 +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 relay8-d.mail.gandi.net (Postfix) with ESMTPSA id 15EC81BF209;\n\tFri, 19 Apr 2019 13:58:11 +0000 (UTC)"],"X-Originating-IP":"2.224.242.101","Date":"Fri, 19 Apr 2019 15:59:05 +0200","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20190419135905.vjtoyx2uxzhjvqyy@uno.localdomain>","References":"<20190419132531.17856-1-jacopo@jmondi.org>\n\t<20190419132531.17856-5-jacopo@jmondi.org>\n\t<20190419133527.GL7006@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"r23incagk5buxdkn\"","Content-Disposition":"inline","In-Reply-To":"<20190419133527.GL7006@pendragon.ideasonboard.com>","User-Agent":"NeoMutt/20180716","Subject":"Re: [libcamera-devel] [PATCH v8 4/8] 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 13:58:12 -0000"}}]