[{"id":14797,"web_url":"https://patchwork.libcamera.org/comment/14797/","msgid":"","date":"2021-01-26T22:53:14","subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nThank you for the patch.\n\nOn Tue, Jan 26, 2021 at 04:24:08PM +0000, Naushir Patuck wrote:\n> In preparation for fast colour denoise, set the low resolution ISP\n> output stream (Output1) to a 1/4 resolution of the application requested\n> stream (Output0). This only happens if the application has not requested\n> an additional YUV or RGB stream.\n> \n> We also constrain this 1/4 resolution to at most 1200 pixels in the\n> largest dimension to avoid being too taxing on memory usage and system\n> bandwidth.\n> \n> Also switch the default StreamRole::VideoRecording to YUV420 to allow\n> fast colour denoise to run.\n\nCould you explain why all this is needed ? What happens if an\napplication requests the second output with a higher resolution that\nthis, will fast colour denoise still work correctly ?\n\n> Signed-off-by: Naushir Patuck \n> Reviewed-by: David Plowman \n> Reviewed-by: Kieran Bingham \n> ---\n> .../pipeline/raspberrypi/raspberrypi.cpp | 35 ++++++++++++++++++-\n> 1 file changed, 34 insertions(+), 1 deletion(-)\n> \n> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> index d9895c779725..fe4c75f09925 100644\n> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> \n> \t\tcase StreamRole::VideoRecording:\n> \t\t\tfmts = data->isp_[Isp::Output0].dev()->formats();\n> -\t\t\tpixelFormat = formats::NV12;\n> +\t\t\tpixelFormat = formats::YUV420;\n> \t\t\tsize = { 1920, 1080 };\n> \t\t\tbufferCount = 4;\n> \t\t\toutCount++;\n> @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t * StreamConfiguration appropriately.\n> \t */\n> \tV4L2DeviceFormat format;\n> +\tbool output1Set = false;\n> \tfor (unsigned i = 0; i < config->size(); i++) {\n> \t\tStreamConfiguration &cfg = config->at(i);\n> \n> @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \t\tformat.size = cfg.size;\n> \t\tformat.fourcc = fourcc;\n> \n> +\t\tLOG(RPI, Info) << \"Setting \" << stream->name() << \" to a resolution of \"\n> +\t\t\t << format.toString();\n> +\n> \t\tret = stream->dev()->setFormat(&format);\n> \t\tif (ret)\n> \t\t\treturn -EINVAL;\n> @@ -645,6 +649,35 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> \n> \t\tcfg.setStream(stream);\n> \t\tstream->setExternal(true);\n> +\n> +\t\tif (i != maxIndex)\n> +\t\t\toutput1Set = true;\n> +\t}\n> +\n> +\t/*\n> +\t * If ISP::Output1 stream has not been requested by the application, we\n> +\t * set it up for internal use now. This second stream will be used for\n> +\t * fast colour denoise, and must be a quarter resolution of the ISP::Output0\n> +\t * stream. However, also limit the maximum size to 1200 pixels in the\n> +\t * larger dimension, just to avoid being wasteful with buffer allocations\n> +\t * and memory bandwidth.\n> +\t */\n> +\tif (!output1Set) {\n> +\t\tV4L2DeviceFormat output1Format = format;\n> +\t\tconstexpr unsigned int maxDimensions = 1200;\n> +\t\tconst Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size);\n> +\n> +\t\toutput1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);\n> +\n> +\t\tLOG(RPI, Info) << \"Setting ISP Output1 (internal) to a resolution of \"\n> +\t\t\t << output1Format.toString();\n> +\n> +\t\tret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format);\n> +\t\tif (ret) {\n> +\t\t\tLOG(RPI, Error) << \"Failed to set format on ISP Output1: \"\n> +\t\t\t\t\t<< ret;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> \t}\n> \n> \t/* ISP statistics output format. */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 5848FC0F2B\n\tfor ;\n\tTue, 26 Jan 2021 22:53:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 275AA6832B;\n\tTue, 26 Jan 2021 23:53:35 +0100 (CET)","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 6741D6831A\n\tfor ;\n\tTue, 26 Jan 2021 23:53:34 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id EB1972C1;\n\tTue, 26 Jan 2021 23:53:33 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"J5mgWo80\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1611701614;\n\tbh=H2L3mdxzhDgQ4T9eOQSBhByUcNqht9plYf2ykMPsooo=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=J5mgWo80OBXnJ9CaXNAubr26fC3A0ExhucxQC6srwHhzAJgZjpU6YH9bGh29anbOY\n\t+6uLaUrP3URVu8wFqE1SS8UPW2f5BVpIQGVj8iVFf0bRAmteKALXRN9/cFZNdh/R99\n\tsFfq5QnJsg2WP3ZcQAAsDpfE90jMlM1MT57O3uHY=","Date":"Wed, 27 Jan 2021 00:53:14 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210126162412.824033-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14814,"web_url":"https://patchwork.libcamera.org/comment/14814/","msgid":"","date":"2021-01-27T08:14:29","subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\n\nOn Tue, 26 Jan 2021 at 22:53, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Tue, Jan 26, 2021 at 04:24:08PM +0000, Naushir Patuck wrote:\n> > In preparation for fast colour denoise, set the low resolution ISP\n> > output stream (Output1) to a 1/4 resolution of the application requested\n> > stream (Output0). This only happens if the application has not requested\n> > an additional YUV or RGB stream.\n> >\n> > We also constrain this 1/4 resolution to at most 1200 pixels in the\n> > largest dimension to avoid being too taxing on memory usage and system\n> > bandwidth.\n> >\n> > Also switch the default StreamRole::VideoRecording to YUV420 to allow\n> > fast colour denoise to run.\n>\n> Could you explain why all this is needed ? What happens if an\n> application requests the second output with a higher resolution that\n> this, will fast colour denoise still work correctly ?\n>\n\nFast colour denoise essentially works by doing some analysis at a 1:4 or\n1:1 resolution image when compared with the output image resolution. It\nthen applies correction on the output image directly based on this\nanalysis. If the application were to require a second output stream that\nis not 1:4 or 1:1 in resolution ratio, the colour denoise will silently be\nbypassed. There is another constraint that this analysis image must be in\nYUV420 format. Unfortunately, this is a limitation with our\nimplementation that we currently have.\n\nRegards,\nNaush\n\n\n>\n> > Signed-off-by: Naushir Patuck \n> > Reviewed-by: David Plowman \n> > Reviewed-by: Kieran Bingham \n> > ---\n> > .../pipeline/raspberrypi/raspberrypi.cpp | 35 ++++++++++++++++++-\n> > 1 file changed, 34 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > index d9895c779725..fe4c75f09925 100644\n> > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > @@ -496,7 +496,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> >\n> > case StreamRole::VideoRecording:\n> > fmts = data->isp_[Isp::Output0].dev()->formats();\n> > - pixelFormat = formats::NV12;\n> > + pixelFormat = formats::YUV420;\n> > size = { 1920, 1080 };\n> > bufferCount = 4;\n> > outCount++;\n> > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > * StreamConfiguration appropriately.\n> > */\n> > V4L2DeviceFormat format;\n> > + bool output1Set = false;\n> > for (unsigned i = 0; i < config->size(); i++) {\n> > StreamConfiguration &cfg = config->at(i);\n> >\n> > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> > format.size = cfg.size;\n> > format.fourcc = fourcc;\n> >\n> > + LOG(RPI, Info) << \"Setting \" << stream->name() << \" to a\n> resolution of \"\n> > + << format.toString();\n> > +\n> > ret = stream->dev()->setFormat(&format);\n> > if (ret)\n> > return -EINVAL;\n> > @@ -645,6 +649,35 @@ int PipelineHandlerRPi::configure(Camera *camera,\n> CameraConfiguration *config)\n> >\n> > cfg.setStream(stream);\n> > stream->setExternal(true);\n> > +\n> > + if (i != maxIndex)\n> > + output1Set = true;\n> > + }\n> > +\n> > + /*\n> > + * If ISP::Output1 stream has not been requested by the\n> application, we\n> > + * set it up for internal use now. This second stream will be used\n> for\n> > + * fast colour denoise, and must be a quarter resolution of the\n> ISP::Output0\n> > + * stream. However, also limit the maximum size to 1200 pixels in\n> the\n> > + * larger dimension, just to avoid being wasteful with buffer\n> allocations\n> > + * and memory bandwidth.\n> > + */\n> > + if (!output1Set) {\n> > + V4L2DeviceFormat output1Format = format;\n> > + constexpr unsigned int maxDimensions = 1200;\n> > + const Size limit = Size(maxDimensions,\n> maxDimensions).boundedToAspectRatio(format.size);\n> > +\n> > + output1Format.size = (format.size /\n> 2).boundedTo(limit).alignedDownTo(2, 2);\n> > +\n> > + LOG(RPI, Info) << \"Setting ISP Output1 (internal) to a\n> resolution of \"\n> > + << output1Format.toString();\n> > +\n> > + ret =\n> data->isp_[Isp::Output1].dev()->setFormat(&output1Format);\n> > + if (ret) {\n> > + LOG(RPI, Error) << \"Failed to set format on ISP\n> Output1: \"\n> > + << ret;\n> > + return -EINVAL;\n> > + }\n> > }\n> >\n> > /* ISP statistics output format. */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id A7A1BC0F2B\n\tfor ;\n\tWed, 27 Jan 2021 08:14:47 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 17BE068344;\n\tWed, 27 Jan 2021 09:14:47 +0100 (CET)","from mail-lf1-x12a.google.com (mail-lf1-x12a.google.com\n\t[IPv6:2a00:1450:4864:20::12a])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0F9866833C\n\tfor ;\n\tWed, 27 Jan 2021 09:14:46 +0100 (CET)","by mail-lf1-x12a.google.com with SMTP id f1so1440786lfu.3\n\tfor ;\n\tWed, 27 Jan 2021 00:14:45 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"FUP6tivw\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=diCmzr9UHhp/vt6oy8JYmA1dAp0wibhmsXtooYWvUtc=;\n\tb=FUP6tivwFL4KWslD+afrv+PTHOQbKFuUNYmug/Ek6fDaALZfvvlWz0/EuqNwtOaZ9u\n\tjeA5XVv2gq8pekgvMtLmvPZehXzze7eWmw7AB/hI+Jzn2pDckfYYkw8JEd/fM33sN9zD\n\tHIlxaqc3uTAq7W1CAXuasRm5fgUbYe+gztBSYv1cS15YED63kHdS8W3mmmKKAoSTdeqh\n\tl2MybDz5pRtYlZ1tlzL8+j7gZQ1+uIB4/l47x00e920P/bXdTipYQcZfYa6w+9huXYqU\n\tlZc93j3e/x2mlY8F4ncZtSWTm3rYil30XHQAYqZUstJP3pvJJgS8Lhqws2IHfakqY3C4\n\tDzAg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=diCmzr9UHhp/vt6oy8JYmA1dAp0wibhmsXtooYWvUtc=;\n\tb=mj4Y3yRNhKTAuDEUAEww93D4kTSxYaL8UzqXhCiAJgowme25LC1SJfvYTMPGVPdQ7Q\n\tAwua4isH6hpabZNO9I1S8w6maV84Va/A1WZQOQDIofd0/caeL1AcSGq1B6/dZcfxVs5S\n\tHU/7oEaXktbS/OMM133YDf3BL3X5Ao5+1Hp0or6wRwGslHkaVowTXbmvYiLQJl8W/JB7\n\tH8+w8V3ctgDbzPEd1JoIJdE5zVsED8MMdF7CK+QHyCOukrXWGwS63Kh5qZ1/3WHeSRlR\n\tEdWWm87Qb6mF7ufgBrjTjEKrO+UXmPtsUOKnnE8gS5GycMT5z41Y18/7a8lWLl81/Ph7\n\tFcUA==","X-Gm-Message-State":"AOAM531AC5KcaxlmdVvgY85Y41Fage9PGuU7/YfB6Y7Wb1s+N+8eDm6e\n\tEyOIrVFi5G095YwnikTDH439cRSJYPEnlzMaEfdOxILMOrTW0Q==","X-Google-Smtp-Source":"ABdhPJzYahdkIMcxF06Hk/Wrv7zrxNveYUqcUElgIicR8Vk/RvYKG9tw5LW/Ejj9lzcRopngYkdHNlzktJS4J99wYW0=","X-Received":"by 2002:ac2:44a3:: with SMTP id c3mr4375832lfm.210.1611735285404;\n\tWed, 27 Jan 2021 00:14:45 -0800 (PST)","MIME-Version":"1.0","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Wed, 27 Jan 2021 08:14:29 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============4653499819485994882==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":14998,"web_url":"https://patchwork.libcamera.org/comment/14998/","msgid":"","date":"2021-02-04T21:56:54","subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Wed, Jan 27, 2021 at 08:14:29AM +0000, Naushir Patuck wrote:\n> On Tue, 26 Jan 2021 at 22:53, Laurent Pinchart wrote:\n> > On Tue, Jan 26, 2021 at 04:24:08PM +0000, Naushir Patuck wrote:\n> > > In preparation for fast colour denoise, set the low resolution ISP\n> > > output stream (Output1) to a 1/4 resolution of the application requested\n> > > stream (Output0). This only happens if the application has not requested\n> > > an additional YUV or RGB stream.\n> > >\n> > > We also constrain this 1/4 resolution to at most 1200 pixels in the\n> > > largest dimension to avoid being too taxing on memory usage and system\n> > > bandwidth.\n> > >\n> > > Also switch the default StreamRole::VideoRecording to YUV420 to allow\n> > > fast colour denoise to run.\n> >\n> > Could you explain why all this is needed ? What happens if an\n> > application requests the second output with a higher resolution that\n> > this, will fast colour denoise still work correctly ?\n> \n> Fast colour denoise essentially works by doing some analysis at a 1:4 or\n> 1:1 resolution image when compared with the output image resolution. It\n> then applies correction on the output image directly based on this\n> analysis. If the application were to require a second output stream that\n> is not 1:4 or 1:1 in resolution ratio, the colour denoise will silently be\n> bypassed.\n\nThat's useful information. Maybe this could be captured in a comment in\nthe code ?\n\nI also wonder if we shouldn't consider this in the metadata we return.\nWhen the CDN is silently bypassed, wouldn't it be better if the metadata\nreflected that ?\n\n> There is another constraint that this analysis image must be in\n> YUV420 format. Unfortunately, this is a limitation with our\n> implementation that we currently have.\n\nOnly the analysis image, or also the main output image ? This would also\nbe useful to capture in a comment, so that it doesn't get ignored later\nwhen the code is reworked.\n\n> > > Signed-off-by: Naushir Patuck \n> > > Reviewed-by: David Plowman \n> > > Reviewed-by: Kieran Bingham \n> > > ---\n> > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 ++++++++++++++++++-\n> > > 1 file changed, 34 insertions(+), 1 deletion(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > index d9895c779725..fe4c75f09925 100644\n> > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > >\n> > > case StreamRole::VideoRecording:\n> > > fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > - pixelFormat = formats::NV12;\n\nHere for instance, you could write\n\n\t\t\t/*\n\t\t\t * The colour denoise algorithm require the analysis\n\t\t\t * image, produced by the second ISP output, to be in\n\t\t\t * YUV420 format. Select this format as the default, to\n\t\t\t * maximize chances that it will be picked by\n\t\t\t * applications and enable usage of the colour denoise\n\t\t\t * algorithm.\n\t\t\t */\n\nOr something more accurate, as I'm not sure to have picked all the\nimportant information correctly :-)\n\n> > > + pixelFormat = formats::YUV420;\n> > > size = { 1920, 1080 };\n> > > bufferCount = 4;\n> > > outCount++;\n> > > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > * StreamConfiguration appropriately.\n> > > */\n> > > V4L2DeviceFormat format;\n> > > + bool output1Set = false;\n> > > for (unsigned i = 0; i < config->size(); i++) {\n> > > StreamConfiguration &cfg = config->at(i);\n> > >\n> > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > format.size = cfg.size;\n> > > format.fourcc = fourcc;\n> > >\n> > > + LOG(RPI, Info) << \"Setting \" << stream->name() << \" to a resolution of \"\n> > > + << format.toString();\n> > > +\n> > > ret = stream->dev()->setFormat(&format);\n> > > if (ret)\n> > > return -EINVAL;\n> > > @@ -645,6 +649,35 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > >\n> > > cfg.setStream(stream);\n> > > stream->setExternal(true);\n> > > +\n> > > + if (i != maxIndex)\n> > > + output1Set = true;\n> > > + }\n> > > +\n> > > + /*\n> > > + * If ISP::Output1 stream has not been requested by the application, we\n> > > + * set it up for internal use now. This second stream will be used for\n> > > + * fast colour denoise, and must be a quarter resolution of the ISP::Output0\n> > > + * stream. However, also limit the maximum size to 1200 pixels in the\n> > > + * larger dimension, just to avoid being wasteful with buffer allocations\n> > > + * and memory bandwidth.\n> > > + */\n> > > + if (!output1Set) {\n> > > + V4L2DeviceFormat output1Format = format;\n\nIf only the analysis image has to be in YUV420 format, should YUV420 be\nselected here in case output 0 uses a different format ?\n\n> > > + constexpr unsigned int maxDimensions = 1200;\n> > > + const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size);\n> > > +\n> > > + output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);\n> > > +\n> > > + LOG(RPI, Info) << \"Setting ISP Output1 (internal) to a resolution of \"\n> > > + << output1Format.toString();\n> > > +\n> > > + ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format);\n> > > + if (ret) {\n> > > + LOG(RPI, Error) << \"Failed to set format on ISP Output1: \"\n> > > + << ret;\n> > > + return -EINVAL;\n> > > + }\n> > > }\n> > >\n> > > /* ISP statistics output format. */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 289C7BD162\n\tfor ;\n\tThu, 4 Feb 2021 21:57:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A39F661472;\n\tThu, 4 Feb 2021 22:57:18 +0100 (CET)","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 2C43A61430\n\tfor ;\n\tThu, 4 Feb 2021 22:57:17 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 8D94F45D;\n\tThu, 4 Feb 2021 22:57:16 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"Ehe2goFD\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612475836;\n\tbh=x7ujuPi9P53kOsnFU5sESWsoPHlgLBoU595EdRZk5ow=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Ehe2goFDsVjvtPRm9SJ1pw0t4U3z4xnFV0Etc2DcJ7h4cY8aqD7qsshj8GZCqMLIA\n\tF/Uyo+O4wVzMzfocYhHkMDE7X+jB1G048bw86dT8+E5Ze6gBlC6lNJpu3HMoIclSdf\n\tz6rNHcF1xVajLqtauD/sTFC6oAuk+5Ot42YtHbqM=","Date":"Thu, 4 Feb 2021 23:56:54 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-3-naush@raspberrypi.com>\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15029,"web_url":"https://patchwork.libcamera.org/comment/15029/","msgid":"","date":"2021-02-05T13:54:25","subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","submitter":{"id":34,"url":"https://patchwork.libcamera.org/api/people/34/","name":"Naushir Patuck","email":"naush@raspberrypi.com"},"content":"Hi Laurent,\n\n\nOn Thu, 4 Feb 2021 at 21:57, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> On Wed, Jan 27, 2021 at 08:14:29AM +0000, Naushir Patuck wrote:\n> > On Tue, 26 Jan 2021 at 22:53, Laurent Pinchart wrote:\n> > > On Tue, Jan 26, 2021 at 04:24:08PM +0000, Naushir Patuck wrote:\n> > > > In preparation for fast colour denoise, set the low resolution ISP\n> > > > output stream (Output1) to a 1/4 resolution of the application\n> requested\n> > > > stream (Output0). This only happens if the application has not\n> requested\n> > > > an additional YUV or RGB stream.\n> > > >\n> > > > We also constrain this 1/4 resolution to at most 1200 pixels in the\n> > > > largest dimension to avoid being too taxing on memory usage and\n> system\n> > > > bandwidth.\n> > > >\n> > > > Also switch the default StreamRole::VideoRecording to YUV420 to allow\n> > > > fast colour denoise to run.\n> > >\n> > > Could you explain why all this is needed ? What happens if an\n> > > application requests the second output with a higher resolution that\n> > > this, will fast colour denoise still work correctly ?\n> >\n> > Fast colour denoise essentially works by doing some analysis at a 1:4 or\n> > 1:1 resolution image when compared with the output image resolution. It\n> > then applies correction on the output image directly based on this\n> > analysis. If the application were to require a second output stream that\n> > is not 1:4 or 1:1 in resolution ratio, the colour denoise will silently\n> be\n> > bypassed.\n>\n> That's useful information. Maybe this could be captured in a comment in\n> the code ?\n>\n\nYes, that's a good idea.\n\n\n>\n> I also wonder if we shouldn't consider this in the metadata we return.\n> When the CDN is silently bypassed, wouldn't it be better if the metadata\n> reflected that ?\n>\n\nYes, we could consider that. The options I would consider are:\n1) Only return the metadata when/if the control is set by the\napplication indicating what has been applied (i.e. either set or not based\non the requested format).\n2) Return the metadata on each frame indicating the CDN status.\n\nMy preference would be for 1, it matches how the metadata returns when we\nset other ISP related params. but I am not entirely against doing 2. What\nare your thoughts?\n\n\n>\n> > There is another constraint that this analysis image must be in\n> > YUV420 format. Unfortunately, this is a limitation with our\n> > implementation that we currently have.\n>\n> Only the analysis image, or also the main output image ? This would also\n> be useful to capture in a comment, so that it doesn't get ignored later\n> when the code is reworked.\n>\n\nBoth images have to be YUV420 format.\n\n\n>\n> > > > Signed-off-by: Naushir Patuck \n> > > > Reviewed-by: David Plowman \n> > > > Reviewed-by: Kieran Bingham \n> > > > ---\n> > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35\n> ++++++++++++++++++-\n> > > > 1 file changed, 34 insertions(+), 1 deletion(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > index d9895c779725..fe4c75f09925 100644\n> > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > @@ -496,7 +496,7 @@ CameraConfiguration\n> *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > >\n> > > > case StreamRole::VideoRecording:\n> > > > fmts =\n> data->isp_[Isp::Output0].dev()->formats();\n> > > > - pixelFormat = formats::NV12;\n>\n> Here for instance, you could write\n>\n> /*\n> * The colour denoise algorithm require the\n> analysis\n> * image, produced by the second ISP output, to be\n> in\n> * YUV420 format. Select this format as the\n> default, to\n> * maximize chances that it will be picked by\n> * applications and enable usage of the colour\n> denoise\n> * algorithm.\n> */\n>\n> Or something more accurate, as I'm not sure to have picked all the\n> important information correctly :-)\n>\n\nSounds correct to me :) I will add a comment with that wording.\n\n\n>\n> > > > + pixelFormat = formats::YUV420;\n> > > > size = { 1920, 1080 };\n> > > > bufferCount = 4;\n> > > > outCount++;\n> > > > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera\n> *camera, CameraConfiguration *config)\n> > > > * StreamConfiguration appropriately.\n> > > > */\n> > > > V4L2DeviceFormat format;\n> > > > + bool output1Set = false;\n> > > > for (unsigned i = 0; i < config->size(); i++) {\n> > > > StreamConfiguration &cfg = config->at(i);\n> > > >\n> > > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera\n> *camera, CameraConfiguration *config)\n> > > > format.size = cfg.size;\n> > > > format.fourcc = fourcc;\n> > > >\n> > > > + LOG(RPI, Info) << \"Setting \" << stream->name() << \" to\n> a resolution of \"\n> > > > + << format.toString();\n> > > > +\n> > > > ret = stream->dev()->setFormat(&format);\n> > > > if (ret)\n> > > > return -EINVAL;\n> > > > @@ -645,6 +649,35 @@ int PipelineHandlerRPi::configure(Camera\n> *camera, CameraConfiguration *config)\n> > > >\n> > > > cfg.setStream(stream);\n> > > > stream->setExternal(true);\n> > > > +\n> > > > + if (i != maxIndex)\n> > > > + output1Set = true;\n> > > > + }\n> > > > +\n> > > > + /*\n> > > > + * If ISP::Output1 stream has not been requested by the\n> application, we\n> > > > + * set it up for internal use now. This second stream will be\n> used for\n> > > > + * fast colour denoise, and must be a quarter resolution of\n> the ISP::Output0\n> > > > + * stream. However, also limit the maximum size to 1200 pixels\n> in the\n> > > > + * larger dimension, just to avoid being wasteful with buffer\n> allocations\n> > > > + * and memory bandwidth.\n> > > > + */\n> > > > + if (!output1Set) {\n> > > > + V4L2DeviceFormat output1Format = format;\n>\n> If only the analysis image has to be in YUV420 format, should YUV420 be\n> selected here in case output 0 uses a different format ?\n>\n\nWhile this is true, I don't think it makes any practical difference. With\nthe existing code, if the output 0 format is YUV420, output 1 will be as\nwell, and we get colour denoise working. If output 1 format is not YUV420,\ncolour denoise will not work, so the format of output 1 is somewhat\nirrelevant. However, as per the comment in a previous email, what I really\nwant to happen here is to disable the Output 1 stream if Output 0 is not in\nYUV420 format. I've added a \\todo for that in the above comment.\n\nRegards,\nNaush\n\n\n>\n> > > > + constexpr unsigned int maxDimensions = 1200;\n> > > > + const Size limit = Size(maxDimensions,\n> maxDimensions).boundedToAspectRatio(format.size);\n> > > > +\n> > > > + output1Format.size = (format.size /\n> 2).boundedTo(limit).alignedDownTo(2, 2);\n> > > > +\n> > > > + LOG(RPI, Info) << \"Setting ISP Output1 (internal) to a\n> resolution of \"\n> > > > + << output1Format.toString();\n> > > > +\n> > > > + ret =\n> data->isp_[Isp::Output1].dev()->setFormat(&output1Format);\n> > > > + if (ret) {\n> > > > + LOG(RPI, Error) << \"Failed to set format on\n> ISP Output1: \"\n> > > > + << ret;\n> > > > + return -EINVAL;\n> > > > + }\n> > > > }\n> > > >\n> > > > /* ISP statistics output format. */\n>\n> --\n> Regards,\n>\n> Laurent Pinchart\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 9D04BBD160\n\tfor ;\n\tFri, 5 Feb 2021 13:54:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2C5CE614B9;\n\tFri, 5 Feb 2021 14:54:45 +0100 (CET)","from mail-lf1-x12d.google.com (mail-lf1-x12d.google.com\n\t[IPv6:2a00:1450:4864:20::12d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 93E2E614B0\n\tfor ;\n\tFri, 5 Feb 2021 14:54:43 +0100 (CET)","by mail-lf1-x12d.google.com with SMTP id i187so9992420lfd.4\n\tfor ;\n\tFri, 05 Feb 2021 05:54:43 -0800 (PST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"jKKDntem\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=Y3mG+23HgEm2zLVzv2+34zgf0EbEmdkK2Luu7lWK6vs=;\n\tb=jKKDntem4jlg/PN3c/UWljX5W8t9IKjmd7v+4GO3JrarhLRt9Y/P5B0vBizk6s1iIf\n\taayvcvSnWunzAvh2igdOEM5dgV/MMZyGcWM710ZsCzrptxwiL1CkwzQCsF+ZgwdHfc3p\n\teWNnAsspVnEP5cMCmsUF6c8uNLeak2Pvvfw9cfmFSNmdYMYXQ1cXXKpC7o3xSYqZAD9r\n\txJORsNENHdwbghVjwX90u1YqarGXB3/7DryXVO/kyJiQhp/XZAgznu9KvZfIGI7Vt+6s\n\tzq3Opj6HWNTvaTjT4wusgpEBrz8wz+tS1UMDH6XedoqILXi4naVRawUquot82lcBgSS3\n\tbWTA==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=Y3mG+23HgEm2zLVzv2+34zgf0EbEmdkK2Luu7lWK6vs=;\n\tb=GbPavKApdMne+doFfAf8m6lh4qbhReF3LaosJr3viHFykQQyZT6/d628Q5MpbDstJ9\n\t76h21uWWjJKLeB/Nf0KHVzzs2rvbmEkivaLlvxDYLWyH0cTAIqjN6f1XAb10VI35tDE2\n\tccqAzw9KL/nHv3A04QA204+sTmRjHF44z/hc3CX2e8sUpJzBHK4rby23kteQVjy2gwVJ\n\t4wj3po/u4or1//5admINiySzap/nyB5fR7djgdnwzAJVkLkqdBpmhVR+JOqWNDNvedyV\n\tUzlNGnvV4v/xBQTkKVZz0ZeLteS43s/VCw8aqN8xM/At7eBhL0G/J49pakZGbgiiJhYD\n\tc2MA==","X-Gm-Message-State":"AOAM5320aUEk7d8i1KjKUTRlDp6GNLPPS4hDGkYmYjIteIXktfIBsDBg\n\tbU2QOYqvie/Hf82iQ0oR97vhlxPr4jLTo0hUtMHBihAN/TElhw==","X-Google-Smtp-Source":"ABdhPJxvmPVojQdDDTg/q4htx++2dpYCLgYaxGJmHZKd3Ixcx2q0J+i6wwNe+xO+GY2lF1e7RCVTc4tZt4WPydfjKQA=","X-Received":"by 2002:a19:ee09:: with SMTP id g9mr2629625lfb.272.1612533281409;\n\tFri, 05 Feb 2021 05:54:41 -0800 (PST)","MIME-Version":"1.0","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-3-naush@raspberrypi.com>\n\t\n\t\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 5 Feb 2021 13:54:25 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"multipart/mixed;\n\tboundary=\"===============7873947376522305209==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15068,"web_url":"https://patchwork.libcamera.org/comment/15068/","msgid":"","date":"2021-02-09T00:29:06","subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Naush,\n\nOn Fri, Feb 05, 2021 at 01:54:25PM +0000, Naushir Patuck wrote:\n> On Thu, 4 Feb 2021 at 21:57, Laurent Pinchart wrote:\n> > On Wed, Jan 27, 2021 at 08:14:29AM +0000, Naushir Patuck wrote:\n> > > On Tue, 26 Jan 2021 at 22:53, Laurent Pinchart wrote:\n> > > > On Tue, Jan 26, 2021 at 04:24:08PM +0000, Naushir Patuck wrote:\n> > > > > In preparation for fast colour denoise, set the low resolution ISP\n> > > > > output stream (Output1) to a 1/4 resolution of the application requested\n> > > > > stream (Output0). This only happens if the application has not requested\n> > > > > an additional YUV or RGB stream.\n> > > > >\n> > > > > We also constrain this 1/4 resolution to at most 1200 pixels in the\n> > > > > largest dimension to avoid being too taxing on memory usage and system\n> > > > > bandwidth.\n> > > > >\n> > > > > Also switch the default StreamRole::VideoRecording to YUV420 to allow\n> > > > > fast colour denoise to run.\n> > > >\n> > > > Could you explain why all this is needed ? What happens if an\n> > > > application requests the second output with a higher resolution that\n> > > > this, will fast colour denoise still work correctly ?\n> > >\n> > > Fast colour denoise essentially works by doing some analysis at a 1:4 or\n> > > 1:1 resolution image when compared with the output image resolution. It\n> > > then applies correction on the output image directly based on this\n> > > analysis. If the application were to require a second output stream that\n> > > is not 1:4 or 1:1 in resolution ratio, the colour denoise will silently be\n> > > bypassed.\n> >\n> > That's useful information. Maybe this could be captured in a comment in\n> > the code ?\n> \n> Yes, that's a good idea.\n> \n> > I also wonder if we shouldn't consider this in the metadata we return.\n> > When the CDN is silently bypassed, wouldn't it be better if the metadata\n> > reflected that ?\n> \n> Yes, we could consider that. The options I would consider are:\n> 1) Only return the metadata when/if the control is set by the\n> application indicating what has been applied (i.e. either set or not based\n> on the requested format).\n> 2) Return the metadata on each frame indicating the CDN status.\n> \n> My preference would be for 1, it matches how the metadata returns when we\n> set other ISP related params. but I am not entirely against doing 2. What\n> are your thoughts?\n\nI think we should standardize on the second option. The rationale is\nthat an application should be able to easily figure out what exact\nparameters have been applied to a particular frame. This means that\n\n- Parameters that can change per-frame need to be returned in metadata\n at least every time they change\n- Parameters that are fixed for the capture session may not need to be\n returned in every frame\n\nWe could minimize the amount of metadata returned to application to only\nreport diffs, but that means applications would need to accumulate the\nchanges to get the current state, which is a bit of a burden. We could\nof course provide them with a helper to do so, but I'm also thinking\nabout the issue a compliance testing of pipeline handlers and IPAs, I\nbelieve it would be easier to check for compliance if the full state was\nreturned in every request.\n\nIf there are good arguments for a diff-based metadata API, the same way\nwe have a diff-based control API, I could be convinced otherwise.\n\n> > > There is another constraint that this analysis image must be in\n> > > YUV420 format. Unfortunately, this is a limitation with our\n> > > implementation that we currently have.\n> >\n> > Only the analysis image, or also the main output image ? This would also\n> > be useful to capture in a comment, so that it doesn't get ignored later\n> > when the code is reworked.\n> \n> Both images have to be YUV420 format.\n> \n> > > > > Signed-off-by: Naushir Patuck \n> > > > > Reviewed-by: David Plowman \n> > > > > Reviewed-by: Kieran Bingham \n> > > > > ---\n> > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 35 ++++++++++++++++++-\n> > > > > 1 file changed, 34 insertions(+), 1 deletion(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > index d9895c779725..fe4c75f09925 100644\n> > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp\n> > > > > @@ -496,7 +496,7 @@ CameraConfiguration *PipelineHandlerRPi::generateConfiguration(Camera *camera,\n> > > > >\n> > > > > case StreamRole::VideoRecording:\n> > > > > fmts = data->isp_[Isp::Output0].dev()->formats();\n> > > > > - pixelFormat = formats::NV12;\n> >\n> > Here for instance, you could write\n> >\n> > /*\n> > * The colour denoise algorithm require the analysis\n> > * image, produced by the second ISP output, to be in\n> > * YUV420 format. Select this format as the default, to\n> > * maximize chances that it will be picked by\n> > * applications and enable usage of the colour denoise\n> > * algorithm.\n> > */\n> >\n> > Or something more accurate, as I'm not sure to have picked all the\n> > important information correctly :-)\n> \n> Sounds correct to me :) I will add a comment with that wording.\n> \n> > > > > + pixelFormat = formats::YUV420;\n> > > > > size = { 1920, 1080 };\n> > > > > bufferCount = 4;\n> > > > > outCount++;\n> > > > > @@ -608,6 +608,7 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > * StreamConfiguration appropriately.\n> > > > > */\n> > > > > V4L2DeviceFormat format;\n> > > > > + bool output1Set = false;\n> > > > > for (unsigned i = 0; i < config->size(); i++) {\n> > > > > StreamConfiguration &cfg = config->at(i);\n> > > > >\n> > > > > @@ -632,6 +633,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > > format.size = cfg.size;\n> > > > > format.fourcc = fourcc;\n> > > > >\n> > > > > + LOG(RPI, Info) << \"Setting \" << stream->name() << \" to a resolution of \"\n> > > > > + << format.toString();\n> > > > > +\n> > > > > ret = stream->dev()->setFormat(&format);\n> > > > > if (ret)\n> > > > > return -EINVAL;\n> > > > > @@ -645,6 +649,35 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)\n> > > > >\n> > > > > cfg.setStream(stream);\n> > > > > stream->setExternal(true);\n> > > > > +\n> > > > > + if (i != maxIndex)\n> > > > > + output1Set = true;\n> > > > > + }\n> > > > > +\n> > > > > + /*\n> > > > > + * If ISP::Output1 stream has not been requested by the application, we\n> > > > > + * set it up for internal use now. This second stream will be used for\n> > > > > + * fast colour denoise, and must be a quarter resolution of the ISP::Output0\n> > > > > + * stream. However, also limit the maximum size to 1200 pixels in the\n> > > > > + * larger dimension, just to avoid being wasteful with buffer allocations\n> > > > > + * and memory bandwidth.\n> > > > > + */\n> > > > > + if (!output1Set) {\n> > > > > + V4L2DeviceFormat output1Format = format;\n> >\n> > If only the analysis image has to be in YUV420 format, should YUV420 be\n> > selected here in case output 0 uses a different format ?\n> \n> While this is true, I don't think it makes any practical difference. With\n> the existing code, if the output 0 format is YUV420, output 1 will be as\n> well, and we get colour denoise working. If output 1 format is not YUV420,\n> colour denoise will not work, so the format of output 1 is somewhat\n> irrelevant. However, as per the comment in a previous email, what I really\n> want to happen here is to disable the Output 1 stream if Output 0 is not in\n> YUV420 format. I've added a \\todo for that in the above comment.\n> \n> > > > > + constexpr unsigned int maxDimensions = 1200;\n> > > > > + const Size limit = Size(maxDimensions, maxDimensions).boundedToAspectRatio(format.size);\n> > > > > +\n> > > > > + output1Format.size = (format.size / 2).boundedTo(limit).alignedDownTo(2, 2);\n> > > > > +\n> > > > > + LOG(RPI, Info) << \"Setting ISP Output1 (internal) to a resolution of \"\n> > > > > + << output1Format.toString();\n> > > > > +\n> > > > > + ret = data->isp_[Isp::Output1].dev()->setFormat(&output1Format);\n> > > > > + if (ret) {\n> > > > > + LOG(RPI, Error) << \"Failed to set format on ISP Output1: \"\n> > > > > + << ret;\n> > > > > + return -EINVAL;\n> > > > > + }\n> > > > > }\n> > > > >\n> > > > > /* ISP statistics output format. */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 91EF7BD162\n\tfor ;\n\tTue, 9 Feb 2021 00:29:32 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1FF1061419;\n\tTue, 9 Feb 2021 01:29:32 +0100 (CET)","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 07E78602FF\n\tfor ;\n\tTue, 9 Feb 2021 01:29:31 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 6B717583;\n\tTue, 9 Feb 2021 01:29:30 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"isL7zdfc\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612830570;\n\tbh=9HCMwx6IKG8kS3vao45r/K+TlT4+Goy4e1pZowko7ik=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=isL7zdfcmZCT5UdyehvLFO3f0DAMg/Kn8lRFo63MiUeoHaCuaqNt/XKV1VkrbBl8w\n\t8rQYf1ZlFkVsIFKCEYxRUcVOv9ECFAFUb2rBg09EIu/02zJdO8HSfqp2sxRVOlgmYr\n\txawT0fujatRTCMTkYPaOSTHV0PXuyrxy6e06vqBM=","Date":"Tue, 9 Feb 2021 02:29:06 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210126162412.824033-1-naush@raspberrypi.com>\n\t<20210126162412.824033-3-naush@raspberrypi.com>\n\t\n\t\n\t\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v4 2/6] pipeline: raspberrypi: Set the\n\tISP Output1 to 1/4 resolution if unused","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]