[{"id":14990,"web_url":"https://patchwork.libcamera.org/comment/14990/","msgid":"","date":"2021-02-04T21:31:49","subject":"Re: [libcamera-devel] [PATCH v5 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 Fri, Jan 29, 2021 at 03:11:50PM +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> 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\nDoes this mean that the colour denoise can only run when outputting\nYUV420 ? Will it be silently disabled otherwise ? You set below the same\npixel format for outputs 0 and 1, if colour denoise can only run when\nthe format is YUV420, should we skip enabling output 1 if a different\npixel format has been selected, as that would waste resources ?\n\nActually, now that I think about it, and unless I'm mistaken, the\npipeline handler doesn't allocate and queue buffers for output 1 when\nonly output 0 is used by the application, so maybe it doesn't waste\nresources and it's fine as is ?\n\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\nShould this be a debug message ? I'd also write\n\n\t\tLOG(RPI, Debug) << \"Setting \" << stream->name() << \" to \"\n\t\t\t\t<< format.toString();\n\nas format.toString() will output both the resolution and pixel format.\n\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\nYou could also write\n\n\t\tconstexpr Size maxDimensions(1200, 1200);\n\t\tconst Size limit = maxDimensions.boundedToAspectRatio(format.size);\n\nUp to you.\n\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\nSame comment as above.\n\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 E4312BD162\n\tfor ;\n\tThu, 4 Feb 2021 21:32:14 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7801861465;\n\tThu, 4 Feb 2021 22:32:14 +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 769EA61430\n\tfor ;\n\tThu, 4 Feb 2021 22:32:12 +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 E8E5645D;\n\tThu, 4 Feb 2021 22:32:11 +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=\"lujjHlQd\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1612474332;\n\tbh=CIw7byYz/G5wb9WDhwlW5atebE2NEJ4y246rKCnLQt0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=lujjHlQdjceQzuuSojySOXslIvQvwMR8zBxYZAT7r4LvZrqMvcdeAKgT3BgLNQShP\n\t7nc3ps6xJXIzDdYkIBeLQQTtRkm4iOCRFm8qw69cpcDr2/9n/F9T6QbQQ7OveFJbpf\n\tgAADZdI4vLuCvhnE8G/+EEpJHcZRCznowwZI6sAI=","Date":"Thu, 4 Feb 2021 23:31:49 +0200","From":"Laurent Pinchart ","To":"Naushir Patuck ","Message-ID":"","References":"<20210129151154.1051163-1-naush@raspberrypi.com>\n\t<20210129151154.1051163-3-naush@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210129151154.1051163-3-naush@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v5 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":15027,"web_url":"https://patchwork.libcamera.org/comment/15027/","msgid":"","date":"2021-02-05T13:38:41","subject":"Re: [libcamera-devel] [PATCH v5 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\nThank you for your review feedback.\n\nOn Thu, 4 Feb 2021 at 21:32, Laurent Pinchart <\nlaurent.pinchart@ideasonboard.com> wrote:\n\n> Hi Naush,\n>\n> Thank you for the patch.\n>\n> On Fri, Jan 29, 2021 at 03:11:50PM +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> > 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>\n> Does this mean that the colour denoise can only run when outputting\n> YUV420 ? Will it be silently disabled otherwise ? You set below the same\n> pixel format for outputs 0 and 1, if colour denoise can only run when\n> the format is YUV420, should we skip enabling output 1 if a different\n> pixel format has been selected, as that would waste resources ?\n>\n\nThe colour denoise will only run if both image formats are YUV420.\nOtherwise it will silently fail. There is an optimisation to be had where\nwe could disable the output 1 stream if the format is not YUV420 and the\napplication has not requested a second stream, however see below....\n\n\n>\n> Actually, now that I think about it, and unless I'm mistaken, the\n> pipeline handler doesn't allocate and queue buffers for output 1 when\n> only output 0 is used by the application, so maybe it doesn't waste\n> resources and it's fine as is ?\n>\n\nSadly, this channel is currently always enabled - even if never used. I\nneed to do more rigorous testing of our drivers to ensure there is no\nproblem with disabling only output 1. Given that it would be default\nbehavior, I would like some confidence on the change before submitting.\nThis is also somewhat related to some work that I recently touched upon for\ndisable Unicam embedded streams when a sensor does not use time. I will\nadd a \\todo here to remind me to get to it.\n\n\n>\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> Should this be a debug message ? I'd also write\n>\n> LOG(RPI, Debug) << \"Setting \" << stream->name() << \" to \"\n> << format.toString();\n>\n> as format.toString() will output both the resolution and pixel format.\n>\n\nAck.\n\n\n>\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> You could also write\n>\n> constexpr Size maxDimensions(1200, 1200);\n> const Size limit =\n> maxDimensions.boundedToAspectRatio(format.size);\n>\n> Up to you.\n>\n\nAck.\n\n\n>\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> Same comment as above.\n>\n\nAck.\n\nRegards,\nNaush\n\n\n\n>\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 536F0BD160\n\tfor ;\n\tFri, 5 Feb 2021 13:39:02 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id C6F02614B8;\n\tFri, 5 Feb 2021 14:39:01 +0100 (CET)","from mail-lj1-x22d.google.com (mail-lj1-x22d.google.com\n\t[IPv6:2a00:1450:4864:20::22d])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 8CDD1614B0\n\tfor ;\n\tFri, 5 Feb 2021 14:38:59 +0100 (CET)","by mail-lj1-x22d.google.com with SMTP id l12so7784904ljc.3\n\tfor ;\n\tFri, 05 Feb 2021 05:38:59 -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=\"Erv92VDI\"; 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=PW0HOObzRXzQYVAe3FGhAUdYAs1is8j5jDJ6s14Fx7I=;\n\tb=Erv92VDITg+U2I97JD73mdpL5NDrnbbpMCAOMAQ56b25jQ9yiB2KcsNsc5MH88Ix6R\n\txTuTIq6UlgMzYIoZ06kn6DH7a3sNxF+cFCbapyvR87Bf8TnXmIvEv1ctt9jqXnWJDk0R\n\tHV5SxyfXBMZUc0iMtgyCDWp23915be+iu9T3OiWpy1NN/AWHHZZq0Ce1evm+bq7++mhg\n\tkprSevK5bsx2v3iH9VVtyPKLwT41Wy1n25nINVKNTc9LH6cmp7TYV3z0lT0GFYyKgpvb\n\tSa/H0ifKBe04FDEriIgME15sRa1MRhU+4QjNep5zi2wUWP2K7HvJxuLK0yuQIIrKns8i\n\tsqvg==","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=PW0HOObzRXzQYVAe3FGhAUdYAs1is8j5jDJ6s14Fx7I=;\n\tb=GbhixWKu9W7BqL61m5aseqhUJRM3tfl+9VsDJ0FnDEs5COHcfJFB1rnpr7pMbqRBhq\n\t3DBVew3HH+f3T+4J4WGI+t9ZoybcycGrXaJo5iv19eM8IgIgzt7BSlxl82YSlN8aqaGJ\n\tNPjThCiRva5etgO+khvyKMpCKu/mVY4V+vrBDDgzi386ycy2qAxNcIIZmEWshMiHjETp\n\tUynosXbXezJPyIbkCc87T3syESrXSNw//LwdvmTpBsCZrnmLVVgOB+Gl5YHVr6vqRwUR\n\t/kqZ+sQMLAGRAU8qXlRJF1C+m2woMgdbWAq5HIj+ltSypXZANUBhZLcihdEbUoqq6TCS\n\tGmZw==","X-Gm-Message-State":"AOAM531c/+AjkJG6J0UfUy00hATTRfhJuv+BhG2Ql1cgQZIiJW0yntJz\n\tFXYq5Ap41T0zTT0NAtyBdqBXsrI572eGLjCwn3hBGYLcntf8cQ==","X-Google-Smtp-Source":"ABdhPJxTGcONqj8mBivQwtlsp0sUOy098kB/jEkq+B4FCh9Yueo24wIjrQ+Iy5VAukxknXTiL9u3orFMZZXRPDiBQNU=","X-Received":"by 2002:a2e:7605:: with SMTP id r5mr2591685ljc.299.1612532337795;\n\tFri, 05 Feb 2021 05:38:57 -0800 (PST)","MIME-Version":"1.0","References":"<20210129151154.1051163-1-naush@raspberrypi.com>\n\t<20210129151154.1051163-3-naush@raspberrypi.com>\n\t","In-Reply-To":"","From":"Naushir Patuck ","Date":"Fri, 5 Feb 2021 13:38:41 +0000","Message-ID":"","To":"Laurent Pinchart ","Subject":"Re: [libcamera-devel] [PATCH v5 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=\"===============3340709954667268179==\"","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]