[{"id":24617,"web_url":"https://patchwork.libcamera.org/comment/24617/","msgid":"<20220817081929.fvorugcslmmqba7u@uno.localdomain>","date":"2022-08-17T08:19:29","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Han-Lin\n\nOn Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> Using Size::expandTo() to find the max resolution might generate a non-existent\n> resolution. For example, when application request streams for 1920x1080 and\n> 1600x1200, the max resolution will be wrongly 1920x1200 and fails the\n> configuration.\n>\n> Bug: https://bugs.libcamera.org/show_bug.cgi?id=139\n> Signed-off-by: Han-Lin Chen \n> ---\n> src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++----------\n> 1 file changed, 12 insertions(+), 10 deletions(-)\n>\n> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> index f65db3c8..47311953 100644\n> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t */\n> \tunsigned int rawCount = 0;\n> \tunsigned int yuvCount = 0;\n> +\tSize rawRequirement;\n> \tSize maxYuvSize;\n> \tSize rawSize;\n>\n> @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n>\n> \t\tif (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> \t\t\trawCount++;\n> -\t\t\trawSize.expandTo(cfg.size);\n> +\t\t\trawSize = std::max(rawSize, cfg.size);\n> \t\t} else {\n> \t\t\tyuvCount++;\n> -\t\t\tmaxYuvSize.expandTo(cfg.size);\n> +\t\t\tmaxYuvSize = std::max(maxYuvSize, cfg.size);\n> +\t\t\trawRequirement.expandTo(cfg.size);\n> \t\t}\n> \t}\n>\n> @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> \t * The output YUV streams will be limited in size to the maximum frame\n> \t * size requested for the RAW stream, if present.\n> \t *\n> -\t * If no raw stream is requested generate a size as large as the maximum\n> -\t * requested YUV size aligned to the ImgU constraints and bound by the\n> -\t * sensor's maximum resolution. See\n> +\t * If no raw stream is requested generate a size as large as the span\n> +\t * of all requested YUV size aligned to the ImgU constraints and bound\n> +\t * by the sensor's maximum resolution. See\n> \t * https://bugs.libcamera.org/show_bug.cgi?id=32\n> \t */\n> \tif (rawSize.isNull())\n> -\t\trawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> -\t\t\t\t\t\t ImgUDevice::kIFMaxCropHeight })\n> -\t\t\t\t .grownBy({ ImgUDevice::kOutputMarginWidth,\n> -\t\t\t\t\t ImgUDevice::kOutputMarginHeight })\n> -\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n> +\t\trawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> +\t\t\t\t\t\t ImgUDevice::kIFMaxCropHeight })\n> +\t\t\t\t .grownBy({ ImgUDevice::kOutputMarginWidth,\n> +\t\t\t\t\t ImgUDevice::kOutputMarginHeight })\n> +\t\t\t\t .boundedTo(data_->cio2_.sensor()->resolution());\n\nThis seems to fix the issue indeed, and maintain the current behaviour\nwhere the RAW stream size limits the YUV output size.\n\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n>\n> \tcio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> \tif (!cio2Configuration_.pixelFormat.isValid())\n> --\n> 2.37.1.595.g718a3a8f04-goog\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 2A2B0BE173\n\tfor ;\n\tWed, 17 Aug 2022 08:19:35 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4D62161FC0;\n\tWed, 17 Aug 2022 10:19:34 +0200 (CEST)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::222])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 2441061FA6\n\tfor ;\n\tWed, 17 Aug 2022 10:19:32 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 4B9A440004;\n\tWed, 17 Aug 2022 08:19:31 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1660724374;\n\tbh=31bS2pXPimlI6hCiH8h+CPIvnXFTpblFT/st48WtSNo=;\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=SCSAFEigX2BjGXVsaUx9TtdyltnMVnXYLEyH+1CAFOlaICrO5RYn7i22N6NkHl6ca\n\toP9KCZQbQNIDSKkiI2iXVrf/CzEzXb/wu/HZTixerqknoIF4t5QUdaobWePCwNUXlF\n\t/24jI1n71/aMEGjzwAbfLEmTPQN4zB0BUiMI9cJ1faJeJYL14gyLNkuA03ahjVlnAr\n\trJOdhsDT/yd7xOoZDrDY0M8iG5s3yJe6oWbc4/q3UEb0VZgp/tfGaMpv4LV87n2UrG\n\tDCc9E47rSFHUe5tWegZvp3jvrsh3DU5AibDUvuZ4KBHv00MheGWc9iRUv5Yjy8VXj0\n\tirTSXpwF4qucw==","Date":"Wed, 17 Aug 2022 10:19:29 +0200","To":"Han-Lin Chen ","Message-ID":"<20220817081929.fvorugcslmmqba7u@uno.localdomain>","References":"<20220812090103.1781246-1-hanlinchen@chromium.org>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220812090103.1781246-1-hanlinchen@chromium.org>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24767,"web_url":"https://patchwork.libcamera.org/comment/24767/","msgid":"<166142135320.72452.11871625082641974125@Monstersaurus>","date":"2022-08-25T09:55:53","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29)\n> Hi Han-Lin\n> \n> On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> > Using Size::expandTo() to find the max resolution might generate a non-existent\n> > resolution. For example, when application request streams for 1920x1080 and\n> > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the\n> > configuration.\n> >\n> > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139\n> > Signed-off-by: Han-Lin Chen \n> > ---\n> > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++----------\n> > 1 file changed, 12 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > index f65db3c8..47311953 100644\n> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > */\n> > unsigned int rawCount = 0;\n> > unsigned int yuvCount = 0;\n> > + Size rawRequirement;\n> > Size maxYuvSize;\n> > Size rawSize;\n> >\n> > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> >\n> > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > rawCount++;\n> > - rawSize.expandTo(cfg.size);\n> > + rawSize = std::max(rawSize, cfg.size);\n> > } else {\n> > yuvCount++;\n> > - maxYuvSize.expandTo(cfg.size);\n> > + maxYuvSize = std::max(maxYuvSize, cfg.size);\n> > + rawRequirement.expandTo(cfg.size);\n> > }\n> > }\n> >\n> > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > * The output YUV streams will be limited in size to the maximum frame\n> > * size requested for the RAW stream, if present.\n> > *\n> > - * If no raw stream is requested generate a size as large as the maximum\n> > - * requested YUV size aligned to the ImgU constraints and bound by the\n> > - * sensor's maximum resolution. See\n> > + * If no raw stream is requested generate a size as large as the span\n\nI know it's pre-existing, but could you add a comma here? Without it the\nsentence flows too fast in my head.\n\n\t\"If no raw stream is requested, generate a size as ...\n\nI think this is called a \"First conditional sentence\" \n\thttps://www.grammarly.com/blog/conditional-sentences/\n\n\n\n> > + * of all requested YUV size aligned to the ImgU constraints and bound\n> > + * by the sensor's maximum resolution. See\n\nI'm confused here about the use of 'a span of all requested YUV sizes'\nas that feels like it gets confused with a libcamera::Span concept which\nI don't think is what it means here.\n\nI would say\n\n\"If no raw stream is requested, generate a size from the largest given\nYUV stream, aligned to the ImgU constraints and bound by the sensor's\nmaximum resolution. See:\"\n\n\n> > * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > */\n> > if (rawSize.isNull())\n> > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > - ImgUDevice::kIFMaxCropHeight })\n> > - .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > - ImgUDevice::kOutputMarginHeight })\n> > - .boundedTo(data_->cio2_.sensor()->resolution());\n> > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > + ImgUDevice::kIFMaxCropHeight })\n> > + .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > + ImgUDevice::kOutputMarginHeight })\n> > + .boundedTo(data_->cio2_.sensor()->resolution());\n> \n> This seems to fix the issue indeed, and maintain the current behaviour\n> where the RAW stream size limits the YUV output size.\n> \n> Reviewed-by: Jacopo Mondi \n\nWith the comment updated in any appropriate way,\n\nReviewed-by: Kieran Bingham \n\n> \n> Thanks\n> j\n> \n> >\n> > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > if (!cio2Configuration_.pixelFormat.isValid())\n> > --\n> > 2.37.1.595.g718a3a8f04-goog\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 B3E33C0DA4\n\tfor ;\n\tThu, 25 Aug 2022 09:55:59 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 92B9461FC1;\n\tThu, 25 Aug 2022 11:55:58 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5D40361FBB\n\tfor ;\n\tThu, 25 Aug 2022 11:55:56 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 9A1032B3;\n\tThu, 25 Aug 2022 11:55:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661421358;\n\tbh=gh5CN2UqZl7dtbWvu/EfM7/uZY5zlIJYpWusvjGhjvU=;\n\th=In-Reply-To:References:To:Date:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=kpH2V1e0LndlFFfKXIe8FiF2+raQEFAONFqyWCduzdustRgORuf1quFASSGOPscGU\n\tnx0GNq52eux9Vh7sOtd4+OY9hq1OZBli/Usz01pglACFvhIcXrTTgnHKOwfhBy/7yS\n\tyEpkb/cFzEJg+ok9DQLITh6TkIh9A+o89KKPYu/8PBxqL6j9EN8i44VlcjosAksYM4\n\tkNt3u8ti8k/Hog3dIsNeC09Y6LGkmOHL7xAV0TuOrx2kuP92Bp5RT0FGJoMeD1FgGs\n\t74OPhaWvpw5fGqkt+L5HqAi85KlyxpdaH88Npnh0jjnPlz/YXmvRsy9UjZHK/34chU\n\tnaskTS3v60m+w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1661421355;\n\tbh=gh5CN2UqZl7dtbWvu/EfM7/uZY5zlIJYpWusvjGhjvU=;\n\th=In-Reply-To:References:Subject:From:Cc:To:Date:From;\n\tb=OewTM9DJr/r3+mCn8Q/a3RQPRNR5NfBX4OnMLPtGsQh08ZEwj/tjYzG55rCNyWTOy\n\tSyLxOfxBl4nYq9DxnVJ1rQAFUTxh7ZhclBjZQkUvpsC73XX+pHnlGaw6Ym5javDr+q\n\tR580XvTDH7imO00Q/9f82nbg5XSGAGauBHw9V/8E="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"OewTM9DJ\"; dkim-atps=neutral","Content-Type":"text/plain; charset=\"utf-8\"","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","In-Reply-To":"<20220817081929.fvorugcslmmqba7u@uno.localdomain>","References":"<20220812090103.1781246-1-hanlinchen@chromium.org>\n\t<20220817081929.fvorugcslmmqba7u@uno.localdomain>","To":"Han-Lin Chen , Jacopo Mondi ,\n\tJacopo Mondi via libcamera-devel ","Date":"Thu, 25 Aug 2022 10:55:53 +0100","Message-ID":"<166142135320.72452.11871625082641974125@Monstersaurus>","User-Agent":"alot/0.10","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","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","From":"Kieran Bingham via libcamera-devel\n\t","Reply-To":"Kieran Bingham ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24768,"web_url":"https://patchwork.libcamera.org/comment/24768/","msgid":"<20220825100541.pf45byhavwyqfeuz@uno.localdomain>","date":"2022-08-25T10:05:41","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Kieran\n\nOn Thu, Aug 25, 2022 at 10:55:53AM +0100, Kieran Bingham wrote:\n> Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29)\n> > Hi Han-Lin\n> >\n> > On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> > > Using Size::expandTo() to find the max resolution might generate a non-existent\n> > > resolution. For example, when application request streams for 1920x1080 and\n> > > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the\n> > > configuration.\n> > >\n> > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139\n> > > Signed-off-by: Han-Lin Chen \n> > > ---\n> > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++----------\n> > > 1 file changed, 12 insertions(+), 10 deletions(-)\n> > >\n> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > index f65db3c8..47311953 100644\n> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > */\n> > > unsigned int rawCount = 0;\n> > > unsigned int yuvCount = 0;\n> > > + Size rawRequirement;\n> > > Size maxYuvSize;\n> > > Size rawSize;\n> > >\n> > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > >\n> > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > rawCount++;\n> > > - rawSize.expandTo(cfg.size);\n> > > + rawSize = std::max(rawSize, cfg.size);\n> > > } else {\n> > > yuvCount++;\n> > > - maxYuvSize.expandTo(cfg.size);\n> > > + maxYuvSize = std::max(maxYuvSize, cfg.size);\n> > > + rawRequirement.expandTo(cfg.size);\n> > > }\n> > > }\n> > >\n> > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > * The output YUV streams will be limited in size to the maximum frame\n> > > * size requested for the RAW stream, if present.\n> > > *\n> > > - * If no raw stream is requested generate a size as large as the maximum\n> > > - * requested YUV size aligned to the ImgU constraints and bound by the\n> > > - * sensor's maximum resolution. See\n> > > + * If no raw stream is requested generate a size as large as the span\n>\n> I know it's pre-existing, but could you add a comma here? Without it the\n> sentence flows too fast in my head.\n>\n> \t\"If no raw stream is requested, generate a size as ...\n>\n> I think this is called a \"First conditional sentence\"\n> \thttps://www.grammarly.com/blog/conditional-sentences/\n>\n>\n>\n> > > + * of all requested YUV size aligned to the ImgU constraints and bound\n> > > + * by the sensor's maximum resolution. See\n>\n> I'm confused here about the use of 'a span of all requested YUV sizes'\n> as that feels like it gets confused with a libcamera::Span concept which\n> I don't think is what it means here.\n>\n> I would say\n>\n> \"If no raw stream is requested, generate a size from the largest given\n> YUV stream, aligned to the ImgU constraints and bound by the sensor's\n> maximum resolution. See:\"\n>\n>\n> > > * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > */\n> > > if (rawSize.isNull())\n> > > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > > - ImgUDevice::kIFMaxCropHeight })\n> > > - .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > > - ImgUDevice::kOutputMarginHeight })\n> > > - .boundedTo(data_->cio2_.sensor()->resolution());\n> > > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > > + ImgUDevice::kIFMaxCropHeight })\n> > > + .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > > + ImgUDevice::kOutputMarginHeight })\n> > > + .boundedTo(data_->cio2_.sensor()->resolution());\n> >\n> > This seems to fix the issue indeed, and maintain the current behaviour\n> > where the RAW stream size limits the YUV output size.\n> >\n> > Reviewed-by: Jacopo Mondi \n>\n> With the comment updated in any appropriate way,\n>\n> Reviewed-by: Kieran Bingham \n\nI can update the comment when applying the patch\n\n>\n> >\n> > Thanks\n> > j\n> >\n> > >\n> > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > if (!cio2Configuration_.pixelFormat.isValid())\n> > > --\n> > > 2.37.1.595.g718a3a8f04-goog\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 33F88C3272\n\tfor ;\n\tThu, 25 Aug 2022 10:05:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8BEB761FBF;\n\tThu, 25 Aug 2022 12:05:45 +0200 (CEST)","from relay9-d.mail.gandi.net (relay9-d.mail.gandi.net\n\t[IPv6:2001:4b98:dc4:8::229])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 125E761FA0\n\tfor ;\n\tThu, 25 Aug 2022 12:05:44 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 49054FF808;\n\tThu, 25 Aug 2022 10:05:43 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661421945;\n\tbh=LhPZO+7g4qPh6y49i+cLuPlqJRBdN2ALTJZ6vn0b1ZU=;\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=dITb2y5mQFNn3826h2TQVgBOmgeFNMJHiqIKdhuqaxrjWO5NNIGkaoKON1+L+RgVs\n\tta6bpyh5lwfXursIqdVzhczuFX8Lcp4ve1Ls1iBCXNgo0OwVKo+ybgRVk9t/yTzOal\n\tB3KUmFRXw2nJ314cq9KNUrgg1YaySn25NRadNovY15QFK2uI2hIWd9f8thG+kpeGA1\n\tq3gFKThAsCSM7vZ5RNQq04/xhBsaroazICZweLXs43dz9DsK0t0uO+Fdsat977rX6k\n\t5ymYG/ZFua8mnr3kTKvI1NPusS2I7N9SAzEFYzWooIrEhcZCjztjUZm3j8Zhs3nQcg\n\tYfaUmT900LJYw==","Date":"Thu, 25 Aug 2022 12:05:41 +0200","To":"Kieran Bingham ","Message-ID":"<20220825100541.pf45byhavwyqfeuz@uno.localdomain>","References":"<20220812090103.1781246-1-hanlinchen@chromium.org>\n\t<20220817081929.fvorugcslmmqba7u@uno.localdomain>\n\t<166142135320.72452.11871625082641974125@Monstersaurus>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<166142135320.72452.11871625082641974125@Monstersaurus>","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","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","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"Jacopo Mondi via libcamera-devel ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":24790,"web_url":"https://patchwork.libcamera.org/comment/24790/","msgid":"","date":"2022-08-26T09:58:29","subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","submitter":{"id":72,"url":"https://patchwork.libcamera.org/api/people/72/","name":"Han-lin Chen","email":"hanlinchen@google.com"},"content":"On Thu, Aug 25, 2022 at 6:05 PM Jacopo Mondi via libcamera-devel\n wrote:\n>\n> Hi Kieran\n>\n> On Thu, Aug 25, 2022 at 10:55:53AM +0100, Kieran Bingham wrote:\n> > Quoting Jacopo Mondi via libcamera-devel (2022-08-17 09:19:29)\n> > > Hi Han-Lin\n> > >\n> > > On Fri, Aug 12, 2022 at 05:01:03PM +0800, Han-Lin Chen via libcamera-devel wrote:\n> > > > Using Size::expandTo() to find the max resolution might generate a non-existent\n> > > > resolution. For example, when application request streams for 1920x1080 and\n> > > > 1600x1200, the max resolution will be wrongly 1920x1200 and fails the\n> > > > configuration.\n> > > >\n> > > > Bug: https://bugs.libcamera.org/show_bug.cgi?id=139\n> > > > Signed-off-by: Han-Lin Chen \n> > > > ---\n> > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 22 ++++++++++++----------\n> > > > 1 file changed, 12 insertions(+), 10 deletions(-)\n> > > >\n> > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > index f65db3c8..47311953 100644\n> > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp\n> > > > @@ -247,6 +247,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > */\n> > > > unsigned int rawCount = 0;\n> > > > unsigned int yuvCount = 0;\n> > > > + Size rawRequirement;\n> > > > Size maxYuvSize;\n> > > > Size rawSize;\n> > > >\n> > > > @@ -255,10 +256,11 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > >\n> > > > if (info.colourEncoding == PixelFormatInfo::ColourEncodingRAW) {\n> > > > rawCount++;\n> > > > - rawSize.expandTo(cfg.size);\n> > > > + rawSize = std::max(rawSize, cfg.size);\n> > > > } else {\n> > > > yuvCount++;\n> > > > - maxYuvSize.expandTo(cfg.size);\n> > > > + maxYuvSize = std::max(maxYuvSize, cfg.size);\n> > > > + rawRequirement.expandTo(cfg.size);\n> > > > }\n> > > > }\n> > > >\n> > > > @@ -287,17 +289,17 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()\n> > > > * The output YUV streams will be limited in size to the maximum frame\n> > > > * size requested for the RAW stream, if present.\n> > > > *\n> > > > - * If no raw stream is requested generate a size as large as the maximum\n> > > > - * requested YUV size aligned to the ImgU constraints and bound by the\n> > > > - * sensor's maximum resolution. See\n> > > > + * If no raw stream is requested generate a size as large as the span\n> >\n> > I know it's pre-existing, but could you add a comma here? Without it the\n> > sentence flows too fast in my head.\n> >\n> > \"If no raw stream is requested, generate a size as ...\n> >\n> > I think this is called a \"First conditional sentence\"\n> > https://www.grammarly.com/blog/conditional-sentences/\n> >\n> >\n> >\n> > > > + * of all requested YUV size aligned to the ImgU constraints and bound\n> > > > + * by the sensor's maximum resolution. See\n> >\n> > I'm confused here about the use of 'a span of all requested YUV sizes'\n> > as that feels like it gets confused with a libcamera::Span concept which\n> > I don't think is what it means here.\n> >\n> > I would say\n> >\n> > \"If no raw stream is requested, generate a size from the largest given\n> > YUV stream, aligned to the ImgU constraints and bound by the sensor's\n> > maximum resolution. See:\"\n> >\n> >\n> > > > * https://bugs.libcamera.org/show_bug.cgi?id=32\n> > > > */\n> > > > if (rawSize.isNull())\n> > > > - rawSize = maxYuvSize.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > > > - ImgUDevice::kIFMaxCropHeight })\n> > > > - .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > > > - ImgUDevice::kOutputMarginHeight })\n> > > > - .boundedTo(data_->cio2_.sensor()->resolution());\n> > > > + rawSize = rawRequirement.expandedTo({ ImgUDevice::kIFMaxCropWidth,\n> > > > + ImgUDevice::kIFMaxCropHeight })\n> > > > + .grownBy({ ImgUDevice::kOutputMarginWidth,\n> > > > + ImgUDevice::kOutputMarginHeight })\n> > > > + .boundedTo(data_->cio2_.sensor()->resolution());\n> > >\n> > > This seems to fix the issue indeed, and maintain the current behaviour\n> > > where the RAW stream size limits the YUV output size.\n> > >\n> > > Reviewed-by: Jacopo Mondi \n> >\n> > With the comment updated in any appropriate way,\n> >\n> > Reviewed-by: Kieran Bingham \n>\n> I can update the comment when applying the patch\nMany thanks Jacopo :)\n>\n> >\n> > >\n> > > Thanks\n> > > j\n> > >\n> > > >\n> > > > cio2Configuration_ = data_->cio2_.generateConfiguration(rawSize);\n> > > > if (!cio2Configuration_.pixelFormat.isValid())\n> > > > --\n> > > > 2.37.1.595.g718a3a8f04-goog\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 266D5C0DA4\n\tfor ;\n\tFri, 26 Aug 2022 09:58:44 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3812261FC0;\n\tFri, 26 Aug 2022 11:58:43 +0200 (CEST)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1661507923;\n\tbh=QsR9n4DyjYVHCjMbkcxMsnludcBVcH49BV2i4Hn23WM=;\n\th=References:In-Reply-To:Date:To:List-Id:List-Post:From:Cc:\n\tList-Subscribe:List-Unsubscribe:List-Archive:Reply-To:List-Help:\n\tSubject:From;\n\tb=ViycWFp+QBdytsliXa1lxuHjulFMCvDRUKdb3ZM6nsBN0mRc0ZJkMLS/FA+q/uhf7\n\tM5TkSmAw1503ykW4l4YIoTaWTpKxzQzw+X8E1mE86JHWy78I+oZZAMYax8Nxk3M8mv\n\tL/bOyRISOvN61D3vlsRnqQaBg+SXoooQYkN6D5hyUJs2uzfBUfFNqOBdQ5nxMRN6cE\n\tZOmIH6Yp1mcuMC/af0ehv0ZxpFS1eeRZz/XtnFZIDil1sXK4nAAcYllQ519pFhVK8S\n\tk+TNe1LyHIcM9tkoZTDMnWykSeuEi4S1JX+UDKKOmC357bl7izZ9OuI6CwnpmXw9Ah\n\trDcWTRvEJFrLw==","References":"<20220812090103.1781246-1-hanlinchen@chromium.org>\n\t<20220817081929.fvorugcslmmqba7u@uno.localdomain>\n\t<166142135320.72452.11871625082641974125@Monstersaurus>\n\t<20220825100541.pf45byhavwyqfeuz@uno.localdomain>","In-Reply-To":"<20220825100541.pf45byhavwyqfeuz@uno.localdomain>","Date":"Fri, 26 Aug 2022 17:58:29 +0800","To":"Jacopo Mondi ","MIME-Version":"1.0","Message-ID":"","List-Id":"","List-Post":"","From":"Han-lin Chen via libcamera-devel ","Precedence":"list","Cc":"Jacopo Mondi via libcamera-devel ","X-Mailman-Version":"2.1.29","X-BeenThere":"libcamera-devel@lists.libcamera.org","List-Subscribe":",\n\t","List-Unsubscribe":",\n\t","List-Archive":"","Reply-To":"Han-lin Chen ","List-Help":"","Subject":"Re: [libcamera-devel] [PATCH v2] libcamera: ipu3: Use std::max()\n\tinstead of expandTo() to get the max resolution","Content-Type":"message/rfc822","Content-Disposition":"inline","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]