[{"id":26669,"web_url":"https://patchwork.libcamera.org/comment/26669/","msgid":"<20230319200044.GF13726@pendragon.ideasonboard.com>","date":"2023-03-19T20:00:44","subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","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 Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> Currently each RkISP1 path (main and self) generate a configuration\n> by bounding the sensor's resolution to their respective maximum output\n> aspect ratio and size.\n> \n> \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> \t\t\t\t\t   .boundedTo(resolution);\n> \n> In the case of self path, whose maximum size is 1920x1920, the generated\n> configuration could get assigned unusual sizes, as the result of the\n> above operation\n\ns/$/./\n\n> \n> As an example, with the imx258 sensor whose resolution is 4208x3118, the\n> resulting size for the Viewfinder use case is an unusual 1920x1423.\n> \n> Fix this by assigning to each role a desired output size:\n> - Use the sensor's resolution for StillCapture and Raw\n> - Use 1080p for Viewfinder and VideoRecording\n\nWill the pipeline handler crop the full sensor resolution to 4096x2304\nbefore scaling it down to 1080p, or will it just scale ? In the latter\ncase, the pixel ratio won't be square, which isn't good.\n\n> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> ---\n>  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-\n>  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--\n>  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +\n>  3 files changed, 20 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> index ebc9bef8688a..60ab7380034c 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> @@ -164,6 +164,8 @@ public:\n>  \tbool match(DeviceEnumerator *enumerator) override;\n>  \n>  private:\n> +\tstatic constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> +\n>  \tRkISP1CameraData *cameraData(Camera *camera)\n>  \t{\n>  \t\treturn static_cast<RkISP1CameraData *>(camera->_d());\n> @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \n>  \tfor (const StreamRole role : roles) {\n>  \t\tbool useMainPath = mainPathAvailable;\n> +\t\tSize size;\n>  \n>  \t\tswitch (role) {\n>  \t\tcase StreamRole::StillCapture:\n>  \t\t\t/* JPEG encoders typically expect sYCC. */\n>  \t\t\tif (!colorSpace)\n>  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> +\n> +\t\t\tsize = data->sensor_->resolution();\n>  \t\t\tbreak;\n>  \n>  \t\tcase StreamRole::Viewfinder:\n> @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \t\t\t */\n>  \t\t\tif (!colorSpace)\n>  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> +\n> +\t\t\tsize = kRkISP1PreviewSize;\n>  \t\t\tbreak;\n>  \n>  \t\tcase StreamRole::VideoRecording:\n>  \t\t\t/* Rec. 709 is a good default for HD video recording. */\n>  \t\t\tif (!colorSpace)\n>  \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> +\n> +\t\t\tsize = kRkISP1PreviewSize;\n>  \t\t\tbreak;\n>  \n>  \t\tcase StreamRole::Raw:\n> @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \t\t\t}\n>  \n>  \t\t\tcolorSpace = ColorSpace::Raw;\n> +\t\t\tsize = data->sensor_->resolution();\n>  \t\t\tbreak;\n>  \n>  \t\tdefault:\n> @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n>  \t\t}\n>  \n>  \t\tStreamConfiguration cfg =\n> -\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> +\t\t\tpath->generateConfiguration(data->sensor_.get(), size, role);\n>  \t\tif (!cfg.pixelFormat.isValid())\n>  \t\t\treturn nullptr;\n>  \n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> index 5079b268c464..a27ac6fc35cb 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()\n>  }\n>  \n>  StreamConfiguration\n> -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> +RkISP1Path::generateConfiguration(const CameraSensor *sensor,\n> +\t\t\t\t  const Size &size,\n> +\t\t\t\t  StreamRole role)\n\nRkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n\t\t\t\t  StreamRole role)\n\n>  {\n>  \tconst std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();\n>  \tconst Size &resolution = sensor->resolution();\n>  \n> +\t/* Min and max resolutions to populate the available stream formats. */\n>  \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n>  \t\t\t\t\t   .boundedTo(resolution);\n>  \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n>  \n> +\t/* The desired stream size, bound to the max resolution. */\n> +\tSize streamSize = size.boundedTo(maxResolution);\n> +\n>  \t/* Create the list of supported stream formats. */\n>  \tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n>  \tunsigned int rawBitsPerPixel = 0;\n> @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n>  \tStreamFormats formats(streamFormats);\n>  \tStreamConfiguration cfg(formats);\n>  \tcfg.pixelFormat = format;\n> -\tcfg.size = maxResolution;\n> +\tcfg.size = streamSize;\n>  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n>  \n>  \treturn cfg;\n> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> index bdf3f95b95e1..cd77957ee4a6 100644\n> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> @@ -41,6 +41,7 @@ public:\n>  \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n>  \n>  \tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> +\t\t\t\t\t\t  const Size &resolution,\n>  \t\t\t\t\t\t  StreamRole role);\n>  \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n>  \t\t\t\t\t     StreamConfiguration *cfg);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 DCC6FC0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tSun, 19 Mar 2023 20:00:45 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5C26F626DA;\n\tSun, 19 Mar 2023 21:00:45 +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 A47FA626CA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tSun, 19 Mar 2023 21:00:43 +0100 (CET)","from pendragon.ideasonboard.com (213-209-177-213.ip.skylogicnet.com\n\t[213.209.177.213])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id CB3112A0C;\n\tSun, 19 Mar 2023 21:00:41 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679256045;\n\tbh=nn3Nx49N1gPWlCAxT6t7Mi+D7s9KMizlRCc7itURXFs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=KPX/p8pnW2mS3IjcXfSa3iS74IR4c+Hk6/93lVyAf4YfaxIyjRvOr8dYwOfVJdoNQ\n\tsuodRpb333tyiRF0D5LtXOZid/3hTwxC1qKvilTVIDJy1eU2r2wDY5bFxL3GFyNzAY\n\tWTXErEKQq+iW+ku+i0e/df22q7UJ3rU9t05p0oBvKETIAdrWq+VgGdRWQP9h5fBzAq\n\t0SymSgPZpv3qw/D5SBrAdkmVpgk4//3lYMkF4IiL9T9Ieb1ng+4/ktsF3lrFdliC82\n\taQ9cLkL/qk59NcsbK1B2xe7vTWx/+sSSAbkpYmzUkl5F48GFr7SJKpl6MDxLgWV4u9\n\tQYy80JHh/1QUA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679256043;\n\tbh=nn3Nx49N1gPWlCAxT6t7Mi+D7s9KMizlRCc7itURXFs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X0Z9YD7ARLeEXJU5Evs8WIjAxlXhdw2EBaTIOMp9kuQEgTgDLxUuoJFnJ1CBKOXAi\n\tSKEiKMvLf4QlmJOsGvMAFetiP1uBl1wgt0Mmo5VDCKzlfu/afWaVFumPWqFtgBMmdp\n\t2okpk4sHuAAsS6k/Lm0G4RDVYHmeS3TrNxqfjKJI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"X0Z9YD7A\"; dkim-atps=neutral","Date":"Sun, 19 Mar 2023 22:00:44 +0200","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20230319200044.GF13726@pendragon.ideasonboard.com>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26674,"web_url":"https://patchwork.libcamera.org/comment/26674/","msgid":"<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>","date":"2023-03-20T07:21:32","subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > Currently each RkISP1 path (main and self) generate a configuration\n> > by bounding the sensor's resolution to their respective maximum output\n> > aspect ratio and size.\n> >\n> > \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > \t\t\t\t\t   .boundedTo(resolution);\n> >\n> > In the case of self path, whose maximum size is 1920x1920, the generated\n> > configuration could get assigned unusual sizes, as the result of the\n> > above operation\n>\n> s/$/./\n>\n> >\n> > As an example, with the imx258 sensor whose resolution is 4208x3118, the\n> > resulting size for the Viewfinder use case is an unusual 1920x1423.\n> >\n> > Fix this by assigning to each role a desired output size:\n> > - Use the sensor's resolution for StillCapture and Raw\n> > - Use 1080p for Viewfinder and VideoRecording\n>\n> Will the pipeline handler crop the full sensor resolution to 4096x2304\n> before scaling it down to 1080p, or will it just scale ? In the latter\n> case, the pixel ratio won't be square, which isn't good.\n>\n\nI'm not sure I agree this isn't good. If we struggle to maintain the\nsensor's aspect ratio we get:\n1) weird and unusual sizes like the above reported 1920x1423\n2) different results depending on which sensor is connected\n\nI think a known 1080p size is a much more predictable behavior\nfor applications, regardless of the aspect ratio.\n\n> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > ---\n> >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-\n> >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--\n> >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +\n> >  3 files changed, 20 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > index ebc9bef8688a..60ab7380034c 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > @@ -164,6 +164,8 @@ public:\n> >  \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> >  private:\n> > +\tstatic constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> > +\n> >  \tRkISP1CameraData *cameraData(Camera *camera)\n> >  \t{\n> >  \t\treturn static_cast<RkISP1CameraData *>(camera->_d());\n> > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >\n> >  \tfor (const StreamRole role : roles) {\n> >  \t\tbool useMainPath = mainPathAvailable;\n> > +\t\tSize size;\n> >\n> >  \t\tswitch (role) {\n> >  \t\tcase StreamRole::StillCapture:\n> >  \t\t\t/* JPEG encoders typically expect sYCC. */\n> >  \t\t\tif (!colorSpace)\n> >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > +\n> > +\t\t\tsize = data->sensor_->resolution();\n> >  \t\t\tbreak;\n> >\n> >  \t\tcase StreamRole::Viewfinder:\n> > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >  \t\t\t */\n> >  \t\t\tif (!colorSpace)\n> >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > +\n> > +\t\t\tsize = kRkISP1PreviewSize;\n> >  \t\t\tbreak;\n> >\n> >  \t\tcase StreamRole::VideoRecording:\n> >  \t\t\t/* Rec. 709 is a good default for HD video recording. */\n> >  \t\t\tif (!colorSpace)\n> >  \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> > +\n> > +\t\t\tsize = kRkISP1PreviewSize;\n> >  \t\t\tbreak;\n> >\n> >  \t\tcase StreamRole::Raw:\n> > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >  \t\t\t}\n> >\n> >  \t\t\tcolorSpace = ColorSpace::Raw;\n> > +\t\t\tsize = data->sensor_->resolution();\n> >  \t\t\tbreak;\n> >\n> >  \t\tdefault:\n> > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> >  \t\t}\n> >\n> >  \t\tStreamConfiguration cfg =\n> > -\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> > +\t\t\tpath->generateConfiguration(data->sensor_.get(), size, role);\n> >  \t\tif (!cfg.pixelFormat.isValid())\n> >  \t\t\treturn nullptr;\n> >\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > index 5079b268c464..a27ac6fc35cb 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()\n> >  }\n> >\n> >  StreamConfiguration\n> > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,\n> > +\t\t\t\t  const Size &size,\n> > +\t\t\t\t  StreamRole role)\n>\n> RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> \t\t\t\t  StreamRole role)\n>\n> >  {\n> >  \tconst std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();\n> >  \tconst Size &resolution = sensor->resolution();\n> >\n> > +\t/* Min and max resolutions to populate the available stream formats. */\n> >  \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> >  \t\t\t\t\t   .boundedTo(resolution);\n> >  \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> >\n> > +\t/* The desired stream size, bound to the max resolution. */\n> > +\tSize streamSize = size.boundedTo(maxResolution);\n> > +\n> >  \t/* Create the list of supported stream formats. */\n> >  \tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> >  \tunsigned int rawBitsPerPixel = 0;\n> > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> >  \tStreamFormats formats(streamFormats);\n> >  \tStreamConfiguration cfg(formats);\n> >  \tcfg.pixelFormat = format;\n> > -\tcfg.size = maxResolution;\n> > +\tcfg.size = streamSize;\n> >  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> >\n> >  \treturn cfg;\n> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > index bdf3f95b95e1..cd77957ee4a6 100644\n> > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > @@ -41,6 +41,7 @@ public:\n> >  \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> >\n> >  \tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> > +\t\t\t\t\t\t  const Size &resolution,\n> >  \t\t\t\t\t\t  StreamRole role);\n> >  \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> >  \t\t\t\t\t     StreamConfiguration *cfg);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 891D5C0F1B\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Mar 2023 07:21:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8AF55626DB;\n\tMon, 20 Mar 2023 08:21:37 +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 30B3B603A9\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon, 20 Mar 2023 08:21:36 +0100 (CET)","from ideasonboard.com (93-61-96-190.ip145.fastwebnet.it\n\t[93.61.96.190])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 70A211373;\n\tMon, 20 Mar 2023 08:21:35 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679296897;\n\tbh=Biu1Bddjc+WLU8KGbZPNOXTi4JOXaAFT13E8kVxj8OU=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=gbEBr5iQbl5nF6q6cwMT3hTilr35EYs5EhT7wRzjrkvRgY7ruhDscdxF2OAbbrvQI\n\tcm1vKfGV7dwj7UJr12PUpYlSZP0LN9pqpMdReGcKjMDb6aRCmIJolXEJbbsaecBzHy\n\tKiFJdp+l2JOuPXwSk0Mk2yooBTnqS9/u1dq+gNQ9kQ/+ZqigXDc3KeES2aRf1JYBiQ\n\tnPp5GHMTJrrdiNlbgHbxrYfYEaThtDioBRHhdFxuArTE3mCvtr91KiM30JRJgwDXcV\n\t+QapYTEmfHV92qM8IUmD8x9TI/9GjM1le69yc+ekAl2vFXmm9Ux+VPsT13FMJMxhqK\n\tJqJO3VdLTkVjw==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679296895;\n\tbh=Biu1Bddjc+WLU8KGbZPNOXTi4JOXaAFT13E8kVxj8OU=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=Rzxaup49qZ9xawvhIgJf5FzvQ4cneX6ikzJuUktDBuvxzCSxcdzNO9oN/V+Lct9zm\n\tO3zru/KCgVif402sVpUFaa4OCRDexyfbAQUx5IfjAVWcERVgg0v3NNr9lArjSV5e6c\n\tyXBngv+uOlufZEBNIZc9Gu2w/Y9qPKXNK6Kj0f1o="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"Rzxaup49\"; dkim-atps=neutral","Date":"Mon, 20 Mar 2023 08:21:32 +0100","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>\n\t<20230319200044.GF13726@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230319200044.GF13726@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26684,"web_url":"https://patchwork.libcamera.org/comment/26684/","msgid":"<20230320232802.GS20234@pendragon.ideasonboard.com>","date":"2023-03-20T23:28:02","subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:\n> On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:\n> > On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > > Currently each RkISP1 path (main and self) generate a configuration\n> > > by bounding the sensor's resolution to their respective maximum output\n> > > aspect ratio and size.\n> > >\n> > > \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > > \t\t\t\t\t   .boundedTo(resolution);\n> > >\n> > > In the case of self path, whose maximum size is 1920x1920, the generated\n> > > configuration could get assigned unusual sizes, as the result of the\n> > > above operation\n> >\n> > s/$/./\n> >\n> > > As an example, with the imx258 sensor whose resolution is 4208x3118, the\n> > > resulting size for the Viewfinder use case is an unusual 1920x1423.\n> > >\n> > > Fix this by assigning to each role a desired output size:\n> > > - Use the sensor's resolution for StillCapture and Raw\n> > > - Use 1080p for Viewfinder and VideoRecording\n> >\n> > Will the pipeline handler crop the full sensor resolution to 4096x2304\n> > before scaling it down to 1080p, or will it just scale ? In the latter\n> > case, the pixel ratio won't be square, which isn't good.\n> \n> I'm not sure I agree this isn't good. If we struggle to maintain the\n> sensor's aspect ratio we get:\n> 1) weird and unusual sizes like the above reported 1920x1423\n\nThat's not great indeed.\n\n> 2) different results depending on which sensor is connected\n\nThat's to be expected, as sensors have different capabilities :-)\n\n> I think a known 1080p size is a much more predictable behavior\n> for applications, regardless of the aspect ratio.\n\nUnless the application explicitly requests a non-square pixel ratio,\ndefaulting to 1920x1080 with non-square pixels *really* bad. There are\nmultiple options in this case, starting from a 4208x3118 sensor\nresolution:\n\n- Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect\n  ratio\n- Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect\n  ratio\n\nBoth will produce square pixels.\n\n> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > ---\n> > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-\n> > >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--\n> > >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +\n> > >  3 files changed, 20 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > index ebc9bef8688a..60ab7380034c 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > @@ -164,6 +164,8 @@ public:\n> > >  \tbool match(DeviceEnumerator *enumerator) override;\n> > >\n> > >  private:\n> > > +\tstatic constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> > > +\n> > >  \tRkISP1CameraData *cameraData(Camera *camera)\n> > >  \t{\n> > >  \t\treturn static_cast<RkISP1CameraData *>(camera->_d());\n> > > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > >\n> > >  \tfor (const StreamRole role : roles) {\n> > >  \t\tbool useMainPath = mainPathAvailable;\n> > > +\t\tSize size;\n> > >\n> > >  \t\tswitch (role) {\n> > >  \t\tcase StreamRole::StillCapture:\n> > >  \t\t\t/* JPEG encoders typically expect sYCC. */\n> > >  \t\t\tif (!colorSpace)\n> > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > +\n> > > +\t\t\tsize = data->sensor_->resolution();\n> > >  \t\t\tbreak;\n> > >\n> > >  \t\tcase StreamRole::Viewfinder:\n> > > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > >  \t\t\t */\n> > >  \t\t\tif (!colorSpace)\n> > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > +\n> > > +\t\t\tsize = kRkISP1PreviewSize;\n> > >  \t\t\tbreak;\n> > >\n> > >  \t\tcase StreamRole::VideoRecording:\n> > >  \t\t\t/* Rec. 709 is a good default for HD video recording. */\n> > >  \t\t\tif (!colorSpace)\n> > >  \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> > > +\n> > > +\t\t\tsize = kRkISP1PreviewSize;\n> > >  \t\t\tbreak;\n> > >\n> > >  \t\tcase StreamRole::Raw:\n> > > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > >  \t\t\t}\n> > >\n> > >  \t\t\tcolorSpace = ColorSpace::Raw;\n> > > +\t\t\tsize = data->sensor_->resolution();\n> > >  \t\t\tbreak;\n> > >\n> > >  \t\tdefault:\n> > > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > >  \t\t}\n> > >\n> > >  \t\tStreamConfiguration cfg =\n> > > -\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> > > +\t\t\tpath->generateConfiguration(data->sensor_.get(), size, role);\n> > >  \t\tif (!cfg.pixelFormat.isValid())\n> > >  \t\t\treturn nullptr;\n> > >\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > index 5079b268c464..a27ac6fc35cb 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()\n> > >  }\n> > >\n> > >  StreamConfiguration\n> > > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,\n> > > +\t\t\t\t  const Size &size,\n> > > +\t\t\t\t  StreamRole role)\n> >\n> > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > \t\t\t\t  StreamRole role)\n> >\n> > >  {\n> > >  \tconst std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();\n> > >  \tconst Size &resolution = sensor->resolution();\n> > >\n> > > +\t/* Min and max resolutions to populate the available stream formats. */\n> > >  \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > >  \t\t\t\t\t   .boundedTo(resolution);\n> > >  \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > >\n> > > +\t/* The desired stream size, bound to the max resolution. */\n> > > +\tSize streamSize = size.boundedTo(maxResolution);\n> > > +\n> > >  \t/* Create the list of supported stream formats. */\n> > >  \tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > >  \tunsigned int rawBitsPerPixel = 0;\n> > > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > >  \tStreamFormats formats(streamFormats);\n> > >  \tStreamConfiguration cfg(formats);\n> > >  \tcfg.pixelFormat = format;\n> > > -\tcfg.size = maxResolution;\n> > > +\tcfg.size = streamSize;\n> > >  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > >\n> > >  \treturn cfg;\n> > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > index bdf3f95b95e1..cd77957ee4a6 100644\n> > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > @@ -41,6 +41,7 @@ public:\n> > >  \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> > >\n> > >  \tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> > > +\t\t\t\t\t\t  const Size &resolution,\n> > >  \t\t\t\t\t\t  StreamRole role);\n> > >  \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> > >  \t\t\t\t\t     StreamConfiguration *cfg);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 4CC6BC0F2A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon, 20 Mar 2023 23:28:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 5282A626E3;\n\tTue, 21 Mar 2023 00:27:59 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id ED58D626CA\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Mar 2023 00:27:56 +0100 (CET)","from pendragon.ideasonboard.com (213-243-189-158.bb.dnainternet.fi\n\t[213.243.189.158])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 2E603496;\n\tTue, 21 Mar 2023 00:27:56 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679354879;\n\tbh=N3OHkQ4rQDLQ3Xb4flBJtCW0Tz+uMYh4Nl01RvWbM9E=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=vTgep++513JGmOp9gZmiG42fykMnKrZ5GK6nSS9du4h+xuOrnhV3UWQFY3tHfOj7Y\n\tapQGoLklJedCEEH1aVKAOzJHUtkxH4QyWbh6sa/RtuVks4kTtLhV64U7/lJYfLEPnd\n\tqgCgGgjqetqGw3yVeY7kTFkaZMa2Df0X/HkLT1uJUH0GpP2SsEVXnNVi7HWxfXqU30\n\trVgnQUZYlZouVYiHzQBRQL60wB4apGTr839IZnHFHZekmmKGpU+9Tzxm8v4l37g6+q\n\tuTciva4//J7tPwGz+KmT33tAACcWdjezpqLNTEMRACRH187FBxpg0xQ3fVfb58TxVH\n\tQJNPMnQ3WJ3zQ==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679354876;\n\tbh=N3OHkQ4rQDLQ3Xb4flBJtCW0Tz+uMYh4Nl01RvWbM9E=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=TV8QdUj6+cZdf69ECdHMdBSHh/oc802PRXA8fee4yU/lfDM1OXeWdJXKwTe0j7kFy\n\t7Iz+rBIvXYPIXYmlNg6+rWfBvMToVeaww//YJThEB6OP8883LRLH2wA4XuT6wIe6+I\n\tCurfGp7ibDzrY/EG5u5zcFdX1GF2G92ROwYv/Zrc="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"TV8QdUj6\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 01:28:02 +0200","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20230320232802.GS20234@pendragon.ideasonboard.com>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>\n\t<20230319200044.GF13726@pendragon.ideasonboard.com>\n\t<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":26702,"web_url":"https://patchwork.libcamera.org/comment/26702/","msgid":"<20230321163553.33rwpvouuayumx4n@uno.localdomain>","date":"2023-03-21T16:35:53","subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hi Laurent\n\nOn Tue, Mar 21, 2023 at 01:28:02AM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:\n> > On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:\n> > > On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > > > Currently each RkISP1 path (main and self) generate a configuration\n> > > > by bounding the sensor's resolution to their respective maximum output\n> > > > aspect ratio and size.\n> > > >\n> > > > \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > > > \t\t\t\t\t   .boundedTo(resolution);\n> > > >\n> > > > In the case of self path, whose maximum size is 1920x1920, the generated\n> > > > configuration could get assigned unusual sizes, as the result of the\n> > > > above operation\n> > >\n> > > s/$/./\n> > >\n> > > > As an example, with the imx258 sensor whose resolution is 4208x3118, the\n> > > > resulting size for the Viewfinder use case is an unusual 1920x1423.\n> > > >\n> > > > Fix this by assigning to each role a desired output size:\n> > > > - Use the sensor's resolution for StillCapture and Raw\n> > > > - Use 1080p for Viewfinder and VideoRecording\n> > >\n> > > Will the pipeline handler crop the full sensor resolution to 4096x2304\n> > > before scaling it down to 1080p, or will it just scale ? In the latter\n> > > case, the pixel ratio won't be square, which isn't good.\n> >\n> > I'm not sure I agree this isn't good. If we struggle to maintain the\n> > sensor's aspect ratio we get:\n> > 1) weird and unusual sizes like the above reported 1920x1423\n>\n> That's not great indeed.\n>\n> > 2) different results depending on which sensor is connected\n>\n> That's to be expected, as sensors have different capabilities :-)\n>\n> > I think a known 1080p size is a much more predictable behavior\n> > for applications, regardless of the aspect ratio.\n>\n> Unless the application explicitly requests a non-square pixel ratio,\n> defaulting to 1920x1080 with non-square pixels *really* bad. There are\n> multiple options in this case, starting from a 4208x3118 sensor\n> resolution:\n>\n> - Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect\n>   ratio\n\nThanks for clarifying this.\n\nI'll add a small patch to the series to crop at the resizer sink pad.\nIn this way the aspect ratio should be maintained before downscaling\n\nBy default, the pipeline handler when asked for 1080p selects a\nsmaller sensor size than the full resolution, but to validate your\nquestion I hacked it to always use full res and the result I have is:\n\nrkisp1_path.cpp:331 Configured main resizer input pad with 4208x3120-YUYV8_2X8 crop (0, 0)/4208x2368\nrkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8\n\nWithout the hack, a size of 2104x1560 is selected from the sensor,\nwhich gets cropped to 2104x1184 before getting downscaled to 1920x1080\n\nrkisp1_path.cpp:331 Configured main resizer input pad with 2104x1560-YUYV8_2X8 crop (0, 0)/2104x1184\nrkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8\n\nWould it work if I add that patch to the series before this one ?\n\n> - Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect\n>   ratio\n>\n> Both will produce square pixels.\n>\n> > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > ---\n> > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-\n> > > >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--\n> > > >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +\n> > > >  3 files changed, 20 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > index ebc9bef8688a..60ab7380034c 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > @@ -164,6 +164,8 @@ public:\n> > > >  \tbool match(DeviceEnumerator *enumerator) override;\n> > > >\n> > > >  private:\n> > > > +\tstatic constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> > > > +\n> > > >  \tRkISP1CameraData *cameraData(Camera *camera)\n> > > >  \t{\n> > > >  \t\treturn static_cast<RkISP1CameraData *>(camera->_d());\n> > > > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > >\n> > > >  \tfor (const StreamRole role : roles) {\n> > > >  \t\tbool useMainPath = mainPathAvailable;\n> > > > +\t\tSize size;\n> > > >\n> > > >  \t\tswitch (role) {\n> > > >  \t\tcase StreamRole::StillCapture:\n> > > >  \t\t\t/* JPEG encoders typically expect sYCC. */\n> > > >  \t\t\tif (!colorSpace)\n> > > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > > +\n> > > > +\t\t\tsize = data->sensor_->resolution();\n> > > >  \t\t\tbreak;\n> > > >\n> > > >  \t\tcase StreamRole::Viewfinder:\n> > > > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > >  \t\t\t */\n> > > >  \t\t\tif (!colorSpace)\n> > > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > > +\n> > > > +\t\t\tsize = kRkISP1PreviewSize;\n> > > >  \t\t\tbreak;\n> > > >\n> > > >  \t\tcase StreamRole::VideoRecording:\n> > > >  \t\t\t/* Rec. 709 is a good default for HD video recording. */\n> > > >  \t\t\tif (!colorSpace)\n> > > >  \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> > > > +\n> > > > +\t\t\tsize = kRkISP1PreviewSize;\n> > > >  \t\t\tbreak;\n> > > >\n> > > >  \t\tcase StreamRole::Raw:\n> > > > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > >  \t\t\t}\n> > > >\n> > > >  \t\t\tcolorSpace = ColorSpace::Raw;\n> > > > +\t\t\tsize = data->sensor_->resolution();\n> > > >  \t\t\tbreak;\n> > > >\n> > > >  \t\tdefault:\n> > > > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > >  \t\t}\n> > > >\n> > > >  \t\tStreamConfiguration cfg =\n> > > > -\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> > > > +\t\t\tpath->generateConfiguration(data->sensor_.get(), size, role);\n> > > >  \t\tif (!cfg.pixelFormat.isValid())\n> > > >  \t\t\treturn nullptr;\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > index 5079b268c464..a27ac6fc35cb 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()\n> > > >  }\n> > > >\n> > > >  StreamConfiguration\n> > > > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > > > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,\n> > > > +\t\t\t\t  const Size &size,\n> > > > +\t\t\t\t  StreamRole role)\n> > >\n> > > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > > \t\t\t\t  StreamRole role)\n> > >\n> > > >  {\n> > > >  \tconst std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();\n> > > >  \tconst Size &resolution = sensor->resolution();\n> > > >\n> > > > +\t/* Min and max resolutions to populate the available stream formats. */\n> > > >  \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > > >  \t\t\t\t\t   .boundedTo(resolution);\n> > > >  \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > > >\n> > > > +\t/* The desired stream size, bound to the max resolution. */\n> > > > +\tSize streamSize = size.boundedTo(maxResolution);\n> > > > +\n> > > >  \t/* Create the list of supported stream formats. */\n> > > >  \tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > > >  \tunsigned int rawBitsPerPixel = 0;\n> > > > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > > >  \tStreamFormats formats(streamFormats);\n> > > >  \tStreamConfiguration cfg(formats);\n> > > >  \tcfg.pixelFormat = format;\n> > > > -\tcfg.size = maxResolution;\n> > > > +\tcfg.size = streamSize;\n> > > >  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > > >\n> > > >  \treturn cfg;\n> > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > index bdf3f95b95e1..cd77957ee4a6 100644\n> > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > @@ -41,6 +41,7 @@ public:\n> > > >  \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> > > >\n> > > >  \tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> > > > +\t\t\t\t\t\t  const Size &resolution,\n> > > >  \t\t\t\t\t\t  StreamRole role);\n> > > >  \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> > > >  \t\t\t\t\t     StreamConfiguration *cfg);\n>\n> --\n> Regards,\n>\n> Laurent Pinchart","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 827A5C0F2A\n\tfor <parsemail@patchwork.libcamera.org>;\n\tTue, 21 Mar 2023 16:36:06 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D33A761ED3;\n\tTue, 21 Mar 2023 17:36:05 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 07FB461ED1\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tTue, 21 Mar 2023 17:36:04 +0100 (CET)","from ideasonboard.com (host-87-18-61-243.retail.telecomitalia.it\n\t[87.18.61.243])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 53FEB10A;\n\tTue, 21 Mar 2023 17:36:00 +0100 (CET)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1679416565;\n\tbh=di8+K9139M1YIgrY2gWb0SW/5m7xaWzRTdYN0nshsjI=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=I+IwCoIwzfNls/uATHKWuy8ltmXQRO47OIVg8vhRC84atRfYUMzF1vcPrCBCTXovG\n\t91uazd9RyB+CbD55ur5pyaK5x7128lYOSOAbpEsoP+LSGBcBqf+yrbJrmxsJ2q2iuV\n\tujeJx/N5a/rOmhaLR5u4MRKTCQlWu8JQ349uEo7LfkxadWufFtY2zK7G3miNG+/zNm\n\tR5ioEZs+HqiRpTrgt6JOn7bPPzIBbsMO4b29eoHlhu4hFnGoX0cO1LJBXjsX5Msr9X\n\t9cu6YL3WXzOgTCPScsreSMyQ3dPSgLnYd7J/4HAVTl/C+50Ax50gnI9Xpe/IPc7FDZ\n\tlT/OnDEpYEogg==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1679416563;\n\tbh=di8+K9139M1YIgrY2gWb0SW/5m7xaWzRTdYN0nshsjI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=M0NIpqlDpFCg9Na6HdETXR6pK28IzedLVpjoAVUJ/FrbmqfFMiWeNQO1fiLjzGbTv\n\tOCG3Z7aE+AWO6fSSVVS3jSDKu+1S+EkuBQTUxP5jt5czwVZtcglAoVLH4XCKYigwPf\n\tlP3qG7yQbi9uj/0EdvbalOIiflM7naRrns+PRkj8="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"M0NIpqlD\"; dkim-atps=neutral","Date":"Tue, 21 Mar 2023 17:35:53 +0100","To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Message-ID":"<20230321163553.33rwpvouuayumx4n@uno.localdomain>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>\n\t<20230319200044.GF13726@pendragon.ideasonboard.com>\n\t<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>\n\t<20230320232802.GS20234@pendragon.ideasonboard.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230320232802.GS20234@pendragon.ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Jacopo Mondi via libcamera-devel <libcamera-devel@lists.libcamera.org>","Reply-To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Cc":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>,\n\tlibcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}},{"id":27255,"web_url":"https://patchwork.libcamera.org/comment/27255/","msgid":"<20230605141117.GA31538@pendragon.ideasonboard.com>","date":"2023-06-05T14:11:17","subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Tue, Mar 21, 2023 at 05:35:53PM +0100, Jacopo Mondi wrote:\n> On Tue, Mar 21, 2023 at 01:28:02AM +0200, Laurent Pinchart wrote:\n> > On Mon, Mar 20, 2023 at 08:21:32AM +0100, Jacopo Mondi wrote:\n> > > On Sun, Mar 19, 2023 at 10:00:44PM +0200, Laurent Pinchart wrote:\n> > > > On Tue, Mar 07, 2023 at 12:48:02PM +0100, Jacopo Mondi via libcamera-devel wrote:\n> > > > > Currently each RkISP1 path (main and self) generate a configuration\n> > > > > by bounding the sensor's resolution to their respective maximum output\n> > > > > aspect ratio and size.\n> > > > >\n> > > > > \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > > > > \t\t\t\t\t   .boundedTo(resolution);\n> > > > >\n> > > > > In the case of self path, whose maximum size is 1920x1920, the generated\n> > > > > configuration could get assigned unusual sizes, as the result of the\n> > > > > above operation\n> > > >\n> > > > s/$/./\n> > > >\n> > > > > As an example, with the imx258 sensor whose resolution is 4208x3118, the\n> > > > > resulting size for the Viewfinder use case is an unusual 1920x1423.\n> > > > >\n> > > > > Fix this by assigning to each role a desired output size:\n> > > > > - Use the sensor's resolution for StillCapture and Raw\n> > > > > - Use 1080p for Viewfinder and VideoRecording\n> > > >\n> > > > Will the pipeline handler crop the full sensor resolution to 4096x2304\n> > > > before scaling it down to 1080p, or will it just scale ? In the latter\n> > > > case, the pixel ratio won't be square, which isn't good.\n> > >\n> > > I'm not sure I agree this isn't good. If we struggle to maintain the\n> > > sensor's aspect ratio we get:\n> > > 1) weird and unusual sizes like the above reported 1920x1423\n> >\n> > That's not great indeed.\n> >\n> > > 2) different results depending on which sensor is connected\n> >\n> > That's to be expected, as sensors have different capabilities :-)\n> >\n> > > I think a known 1080p size is a much more predictable behavior\n> > > for applications, regardless of the aspect ratio.\n> >\n> > Unless the application explicitly requests a non-square pixel ratio,\n> > defaulting to 1920x1080 with non-square pixels *really* bad. There are\n> > multiple options in this case, starting from a 4208x3118 sensor\n> > resolution:\n> >\n> > - Cropping to 4192x2358 and downscaling to 1920x1080, for a 16:9 aspect\n> >   ratio\n> \n> Thanks for clarifying this.\n> \n> I'll add a small patch to the series to crop at the resizer sink pad.\n> In this way the aspect ratio should be maintained before downscaling\n\nSounds good :-)\n\n> By default, the pipeline handler when asked for 1080p selects a\n> smaller sensor size than the full resolution, but to validate your\n> question I hacked it to always use full res and the result I have is:\n> \n> rkisp1_path.cpp:331 Configured main resizer input pad with 4208x3120-YUYV8_2X8 crop (0, 0)/4208x2368\n> rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8\n\n1920x1080 is exactly 16:9, while 4208x2368 is very close but not quite.\nThe best option in this case would be to crop to 4192x2358. This is 16\npixels less than the full active resolution on the horizontal direction,\nand I think this is by design.\n\nISP processing blocks often process pixels based on the value of their\nneighbours. This means that pixels too close to the edges are special\ncases, as the neighbours needed by the processing may be outside of the\nimage. ISPs need to work around that, to still output a somewhat\nmeaningful value for pixels close to the edges. Depending on the ISP\narchitecture, the processing blocks will for instance duplicate the\nvalue of the edge pixels, or mirror the image around the edge. That's a\nbest effort approach, and it means that the pixels close to the edges\non the ISP output will likely be of lower quality than the other pixels.\nThey are thus often cropped out from the output.\n\nCamera sensors take this into account, and include additional pixels on\nall edges to account for the ISP processing. 8 lines and columns on each\nside is a common value. It is thus not surprising that the best image\nwidth before downscaling to 1920x1080 is exactly 16 pixels less than the\nsensor output.\n\nThe best ISP input to generate 1920x1080 is thus (4192+16)x(2358+16) =\n4208x2374. On the scaler input, the ISP should be configured to crop 8\npixels on each side, producing 4192x2358. This can then be downscaled to\n1920x1080 for a 16:9 image with perfectly square pixels.\n\nThere's no need to take all this into account in this patch series, but\nwe should at some point. We will possibly need helpers to perform those\ncalculations, and I expect they will differ between different ISPs.\n\nNote that producing 4:3 images should go through the same type of\ncalculation. I'm however slightly annoyed there, as 4208x3120 is close\nto 4:3, so I would have expected (4208-16)x(3120-16) to be exactly 4:3.\nIt isn't. The best option seems to be (4208-72)x(3120-18) = 4136x3102,\nwhich has lots of horizontal cropping.\n\n> Without the hack, a size of 2104x1560 is selected from the sensor,\n> which gets cropped to 2104x1184 before getting downscaled to 1920x1080\n> \n> rkisp1_path.cpp:331 Configured main resizer input pad with 2104x1560-YUYV8_2X8 crop (0, 0)/2104x1184\n> rkisp1_path.cpp:337 Configuring main resizer output pad with 1920x1080-YUYV8_2X8\n> \n> Would it work if I add that patch to the series before this one ?\n\nYes, that's fine.\n\n> > - Cropping to 4136x3102 and downscaling to 1980x1440, for a 4:3 aspect\n> >   ratio\n> >\n> > Both will produce square pixels.\n> >\n> > > > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>\n> > > > > Reviewed-by: Paul Elder <paul.elder@ideasonboard.com>\n> > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>\n> > > > > ---\n> > > > >  src/libcamera/pipeline/rkisp1/rkisp1.cpp      | 12 +++++++++++-\n> > > > >  src/libcamera/pipeline/rkisp1/rkisp1_path.cpp | 10 ++++++++--\n> > > > >  src/libcamera/pipeline/rkisp1/rkisp1_path.h   |  1 +\n> > > > >  3 files changed, 20 insertions(+), 3 deletions(-)\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > index ebc9bef8688a..60ab7380034c 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp\n> > > > > @@ -164,6 +164,8 @@ public:\n> > > > >  \tbool match(DeviceEnumerator *enumerator) override;\n> > > > >\n> > > > >  private:\n> > > > > +\tstatic constexpr Size kRkISP1PreviewSize = { 1920, 1080 };\n> > > > > +\n> > > > >  \tRkISP1CameraData *cameraData(Camera *camera)\n> > > > >  \t{\n> > > > >  \t\treturn static_cast<RkISP1CameraData *>(camera->_d());\n> > > > > @@ -651,12 +653,15 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > > >\n> > > > >  \tfor (const StreamRole role : roles) {\n> > > > >  \t\tbool useMainPath = mainPathAvailable;\n> > > > > +\t\tSize size;\n> > > > >\n> > > > >  \t\tswitch (role) {\n> > > > >  \t\tcase StreamRole::StillCapture:\n> > > > >  \t\t\t/* JPEG encoders typically expect sYCC. */\n> > > > >  \t\t\tif (!colorSpace)\n> > > > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > > > +\n> > > > > +\t\t\tsize = data->sensor_->resolution();\n> > > > >  \t\t\tbreak;\n> > > > >\n> > > > >  \t\tcase StreamRole::Viewfinder:\n> > > > > @@ -666,12 +671,16 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > > >  \t\t\t */\n> > > > >  \t\t\tif (!colorSpace)\n> > > > >  \t\t\t\tcolorSpace = ColorSpace::Sycc;\n> > > > > +\n> > > > > +\t\t\tsize = kRkISP1PreviewSize;\n> > > > >  \t\t\tbreak;\n> > > > >\n> > > > >  \t\tcase StreamRole::VideoRecording:\n> > > > >  \t\t\t/* Rec. 709 is a good default for HD video recording. */\n> > > > >  \t\t\tif (!colorSpace)\n> > > > >  \t\t\t\tcolorSpace = ColorSpace::Rec709;\n> > > > > +\n> > > > > +\t\t\tsize = kRkISP1PreviewSize;\n> > > > >  \t\t\tbreak;\n> > > > >\n> > > > >  \t\tcase StreamRole::Raw:\n> > > > > @@ -682,6 +691,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > > >  \t\t\t}\n> > > > >\n> > > > >  \t\t\tcolorSpace = ColorSpace::Raw;\n> > > > > +\t\t\tsize = data->sensor_->resolution();\n> > > > >  \t\t\tbreak;\n> > > > >\n> > > > >  \t\tdefault:\n> > > > > @@ -700,7 +710,7 @@ PipelineHandlerRkISP1::generateConfiguration(Camera *camera,\n> > > > >  \t\t}\n> > > > >\n> > > > >  \t\tStreamConfiguration cfg =\n> > > > > -\t\t\tpath->generateConfiguration(data->sensor_.get(), role);\n> > > > > +\t\t\tpath->generateConfiguration(data->sensor_.get(), size, role);\n> > > > >  \t\tif (!cfg.pixelFormat.isValid())\n> > > > >  \t\t\treturn nullptr;\n> > > > >\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > index 5079b268c464..a27ac6fc35cb 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.cpp\n> > > > > @@ -127,15 +127,21 @@ void RkISP1Path::populateFormats()\n> > > > >  }\n> > > > >\n> > > > >  StreamConfiguration\n> > > > > -RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > > > > +RkISP1Path::generateConfiguration(const CameraSensor *sensor,\n> > > > > +\t\t\t\t  const Size &size,\n> > > > > +\t\t\t\t  StreamRole role)\n> > > >\n> > > > RkISP1Path::generateConfiguration(const CameraSensor *sensor, const Size &size,\n> > > > \t\t\t\t  StreamRole role)\n> > > >\n> > > > >  {\n> > > > >  \tconst std::vector<unsigned int> &mbusCodes = sensor->mbusCodes();\n> > > > >  \tconst Size &resolution = sensor->resolution();\n> > > > >\n> > > > > +\t/* Min and max resolutions to populate the available stream formats. */\n> > > > >  \tSize maxResolution = maxResolution_.boundedToAspectRatio(resolution)\n> > > > >  \t\t\t\t\t   .boundedTo(resolution);\n> > > > >  \tSize minResolution = minResolution_.expandedToAspectRatio(resolution);\n> > > > >\n> > > > > +\t/* The desired stream size, bound to the max resolution. */\n> > > > > +\tSize streamSize = size.boundedTo(maxResolution);\n> > > > > +\n> > > > >  \t/* Create the list of supported stream formats. */\n> > > > >  \tstd::map<PixelFormat, std::vector<SizeRange>> streamFormats;\n> > > > >  \tunsigned int rawBitsPerPixel = 0;\n> > > > > @@ -189,7 +195,7 @@ RkISP1Path::generateConfiguration(const CameraSensor *sensor, StreamRole role)\n> > > > >  \tStreamFormats formats(streamFormats);\n> > > > >  \tStreamConfiguration cfg(formats);\n> > > > >  \tcfg.pixelFormat = format;\n> > > > > -\tcfg.size = maxResolution;\n> > > > > +\tcfg.size = streamSize;\n> > > > >  \tcfg.bufferCount = RKISP1_BUFFER_COUNT;\n> > > > >\n> > > > >  \treturn cfg;\n> > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > > index bdf3f95b95e1..cd77957ee4a6 100644\n> > > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h\n> > > > > @@ -41,6 +41,7 @@ public:\n> > > > >  \tbool isEnabled() const { return link_->flags() & MEDIA_LNK_FL_ENABLED; }\n> > > > >\n> > > > >  \tStreamConfiguration generateConfiguration(const CameraSensor *sensor,\n> > > > > +\t\t\t\t\t\t  const Size &resolution,\n> > > > >  \t\t\t\t\t\t  StreamRole role);\n> > > > >  \tCameraConfiguration::Status validate(const CameraSensor *sensor,\n> > > > >  \t\t\t\t\t     StreamConfiguration *cfg);","headers":{"Return-Path":"<libcamera-devel-bounces@lists.libcamera.org>","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 27035C3200\n\tfor <parsemail@patchwork.libcamera.org>;\n\tMon,  5 Jun 2023 14:11:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8727B6287D;\n\tMon,  5 Jun 2023 16:11:20 +0200 (CEST)","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 0808362709\n\tfor <libcamera-devel@lists.libcamera.org>;\n\tMon,  5 Jun 2023 16:11:19 +0200 (CEST)","from pendragon.ideasonboard.com (om126156242094.26.openmobile.ne.jp\n\t[126.156.242.94])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 694ABBC;\n\tMon,  5 Jun 2023 16:10:53 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1685974280;\n\tbh=U0Le8x15immxO0xNcN2cj84nPgTRlp0tUxXFMLOFjVs=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=3y1d+T1AQ4N4cvQnNmWu6gOgUz2KcJO3IY/ekiW14LMSiE+Wnm+G45fmK3sk11luO\n\tMMRX0Vz3vqUWDyQbW7jTDq5VKVGI/+fEaYLx08/rtH/i6b4gftb6zdOVb5v/2Kp/hA\n\t7ffPvxI1ZIJ/suYrJQgLXrr8yG79cv5CX5gK9hyJ+tluhpJAmaWnl3+CnXnAI8Zgai\n\tui0ngj51MJ2K6V5K67PCDf8bLqjHr93VI6bqBai2N7zmB7MNrzIpiEbzNF8APlPXgV\n\tzJueiQ1QsdccjfnNiABsrgoBha8iL4zW6uzos+nJUJlGg18xtrsOI006Z+p/IJOlbC\n\tM+SR6pbHDT95Q==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1685974254;\n\tbh=U0Le8x15immxO0xNcN2cj84nPgTRlp0tUxXFMLOFjVs=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=ctlglBEojwv6OWD8IzyjesAPlY36/an6HK6N5OfgLHtvgtXSoSOqWTQqoB3WiJeMd\n\tuIJy3y2BdT3nmcxLJ05jayyZSwoHl6vywuV6Wr6eWwNr1r96P13Hq55jN2uA0Tydlh\n\tvM41kHdvFX8osgiRFJ67/Xz5HjAh8Ni17D5gK4pI="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"ctlglBEo\"; dkim-atps=neutral","Date":"Mon, 5 Jun 2023 17:11:17 +0300","To":"Jacopo Mondi <jacopo.mondi@ideasonboard.com>","Message-ID":"<20230605141117.GA31538@pendragon.ideasonboard.com>","References":"<20230307114804.42291-1-jacopo.mondi@ideasonboard.com>\n\t<20230307114804.42291-3-jacopo.mondi@ideasonboard.com>\n\t<20230319200044.GF13726@pendragon.ideasonboard.com>\n\t<20230320072132.mcwgzd4sehtgoemw@uno.localdomain>\n\t<20230320232802.GS20234@pendragon.ideasonboard.com>\n\t<20230321163553.33rwpvouuayumx4n@uno.localdomain>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20230321163553.33rwpvouuayumx4n@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 2/4] libcamera: rkisp1: Assign\n\tsizes to roles","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"<libcamera-devel.lists.libcamera.org>","List-Unsubscribe":"<https://lists.libcamera.org/options/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=unsubscribe>","List-Archive":"<https://lists.libcamera.org/pipermail/libcamera-devel/>","List-Post":"<mailto:libcamera-devel@lists.libcamera.org>","List-Help":"<mailto:libcamera-devel-request@lists.libcamera.org?subject=help>","List-Subscribe":"<https://lists.libcamera.org/listinfo/libcamera-devel>,\n\t<mailto:libcamera-devel-request@lists.libcamera.org?subject=subscribe>","From":"Laurent Pinchart via libcamera-devel\n\t<libcamera-devel@lists.libcamera.org>","Reply-To":"Laurent Pinchart <laurent.pinchart@ideasonboard.com>","Cc":"libcamera-devel@lists.libcamera.org, libcamera@luigi311.com","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" <libcamera-devel-bounces@lists.libcamera.org>"}}]