[{"id":29011,"web_url":"https://patchwork.libcamera.org/comment/29011/","msgid":"<20240320121749.GD6052@pendragon.ideasonboard.com>","date":"2024-03-20T12:17:49","subject":"Re: [PATCH v6 01/18] libcamera: pipeline: simple: fix size\n\tadjustment in validate()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Milan,\n\nThank you for the patch.\n\nOn Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal wrote:\n> From: Andrey Konovalov \n> \n> SimpleCameraConfiguration::validate() adjusts the configuration of its\n> streams (if the size is not in the outputSizes) to the captureSize. But\n> the captureSize itself can be not in the outputSizes, and then the\n> adjusted configuration won't be valid resulting in camera configuration\n> failure.\n> \n> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n> Tested-by: Pavel Machek \n> Reviewed-by: Milan Zamazal \n> Reviewed-by: Pavel Machek \n> Signed-off-by: Andrey Konovalov \n> Signed-off-by: Hans de Goede \n> ---\n> src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--\n> 1 file changed, 35 insertions(+), 2 deletions(-)\n> \n> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n> index 01f2a977..04e77f7e 100644\n> --- a/src/libcamera/pipeline/simple/simple.cpp\n> +++ b/src/libcamera/pipeline/simple/simple.cpp\n> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n> {\n> }\n> \n> +namespace {\n> +\n> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)\n> +{\n> +\tASSERT(supportedSizes.min <= supportedSizes.max);\n> +\n> +\tif (supportedSizes.min == supportedSizes.max)\n> +\t\treturn supportedSizes.max;\n> +\n> +\tunsigned int hStep = supportedSizes.hStep;\n> +\tunsigned int vStep = supportedSizes.vStep;\n> +\n> +\tif (hStep == 0)\n> +\t\thStep = supportedSizes.max.width - supportedSizes.min.width;\n> +\tif (vStep == 0)\n> +\t\tvStep = supportedSizes.max.height - supportedSizes.min.height;\n> +\n> +\tSize adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);\n> +\n> +\treturn adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);\n\nThose lines are getting long. You can write\n\n\tSize adjusted = requestedSize.boundedTo(supportedSizes.max)\n\t\t\t\t .expandedTo(supportedSizes.min);\n\treturn adjusted.shrunkBy(supportedSizes.min)\n\t\t .alignedDownTo(hStep, vStep)\n\t\t .grownBy(supportedSizes.min);\n\nor even\n\n\treturn requestedSize.boundedTo(supportedSizes.max)\n\t\t\t .expandedTo(supportedSizes.min)\n\t\t\t .shrunkBy(supportedSizes.min)\n\t\t\t .alignedDownTo(hStep, vStep)\n\t\t\t .grownBy(supportedSizes.min);\n\nI can change this when applying if you tell me which version you like\nbest (if any).\n\nReviewed-by: Laurent Pinchart \n\n> +}\n> +\n> +} /* namespace */\n> +\n> CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> {\n> \tconst CameraSensor *sensor = data_->sensor_.get();\n> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n> \t\t}\n> \n> \t\tif (!pipeConfig_->outputSizes.contains(cfg.size)) {\n> +\t\t\tSize adjustedSize = pipeConfig_->captureSize;\n> +\t\t\t/*\n> +\t\t\t * The converter (when present) may not be able to output\n> +\t\t\t * a size identical to its input size. The capture size is thus\n> +\t\t\t * not guaranteed to be a valid output size. In such cases, use\n> +\t\t\t * the smaller valid output size closest to the requested.\n> +\t\t\t */\n> +\t\t\tif (!pipeConfig_->outputSizes.contains(adjustedSize))\n> +\t\t\t\tadjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);\n> \t\t\tLOG(SimplePipeline, Debug)\n> \t\t\t\t<< \"Adjusting size from \" << cfg.size\n> -\t\t\t\t<< \" to \" << pipeConfig_->captureSize;\n> -\t\t\tcfg.size = pipeConfig_->captureSize;\n> +\t\t\t\t<< \" to \" << adjustedSize;\n> +\t\t\tcfg.size = adjustedSize;\n> \t\t\tstatus = Adjusted;\n> \t\t}\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 B323FBD160\n\tfor ;\n\tWed, 20 Mar 2024 12:17:55 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id F2EBA62822;\n\tWed, 20 Mar 2024 13:17:54 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 353BC603AC\n\tfor ;\n\tWed, 20 Mar 2024 13:17:53 +0100 (CET)","from pendragon.ideasonboard.com (81-175-209-231.bb.dnainternet.fi\n\t[81.175.209.231])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5027EB1;\n\tWed, 20 Mar 2024 13:17:25 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"pzvkcwpe\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1710937045;\n\tbh=3/mrXeAn6Vid/rjoCBCzLDXTY3s2MCREAdNgtuKU028=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=pzvkcwpe7c3Ku0qPQtRWr8fLW6PJPqpd3mehOFdn5n+NHmhuaiUpYktLrjEFFArfg\n\tpwhqin8XnV4Ib0l0ft0NE2BgH8mF13jqm8Jr6AyDGYpEw3hJgig3Q1hgw2clnGz3sn\n\tJLQ1QRtqXVma51r1QyCHUYuCA992dXmHmpYMkhEY=","Date":"Wed, 20 Mar 2024 14:17:49 +0200","From":"Laurent Pinchart ","To":"Milan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","Subject":"Re: [PATCH v6 01/18] libcamera: pipeline: simple: fix size\n\tadjustment in validate()","Message-ID":"<20240320121749.GD6052@pendragon.ideasonboard.com>","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-2-mzamazal@redhat.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20240319123622.675599-2-mzamazal@redhat.com>","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":29013,"web_url":"https://patchwork.libcamera.org/comment/29013/","msgid":"","date":"2024-03-20T12:53:21","subject":"Re: [PATCH v6 01/18] libcamera: pipeline: simple: fix size\n\tadjustment in validate()","submitter":{"id":179,"url":"https://patchwork.libcamera.org/api/people/179/","name":"Andrei Konovalov","email":"andrey.konovalov.ynk@gmail.com"},"content":"Hi Laurent and Milan,\n\nOn 20.03.2024 15:17, Laurent Pinchart wrote:\n> Hi Milan,\n> \n> Thank you for the patch.\n> \n> On Tue, Mar 19, 2024 at 01:35:48PM +0100, Milan Zamazal wrote:\n>> From: Andrey Konovalov \n>>\n>> SimpleCameraConfiguration::validate() adjusts the configuration of its\n>> streams (if the size is not in the outputSizes) to the captureSize. But\n>> the captureSize itself can be not in the outputSizes, and then the\n>> adjusted configuration won't be valid resulting in camera configuration\n>> failure.\n>>\n>> Tested-by: Bryan O'Donoghue # sc8280xp Lenovo x13s\n>> Tested-by: Pavel Machek \n>> Reviewed-by: Milan Zamazal \n>> Reviewed-by: Pavel Machek \n>> Signed-off-by: Andrey Konovalov \n>> Signed-off-by: Hans de Goede \n>> ---\n>> src/libcamera/pipeline/simple/simple.cpp | 37 ++++++++++++++++++++++--\n>> 1 file changed, 35 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/src/libcamera/pipeline/simple/simple.cpp b/src/libcamera/pipeline/simple/simple.cpp\n>> index 01f2a977..04e77f7e 100644\n>> --- a/src/libcamera/pipeline/simple/simple.cpp\n>> +++ b/src/libcamera/pipeline/simple/simple.cpp\n>> @@ -882,6 +882,30 @@ SimpleCameraConfiguration::SimpleCameraConfiguration(Camera *camera,\n>> {\n>> }\n>> \n>> +namespace {\n>> +\n>> +static Size adjustSize(const Size &requestedSize, const SizeRange &supportedSizes)\n>> +{\n>> +\tASSERT(supportedSizes.min <= supportedSizes.max);\n>> +\n>> +\tif (supportedSizes.min == supportedSizes.max)\n>> +\t\treturn supportedSizes.max;\n>> +\n>> +\tunsigned int hStep = supportedSizes.hStep;\n>> +\tunsigned int vStep = supportedSizes.vStep;\n>> +\n>> +\tif (hStep == 0)\n>> +\t\thStep = supportedSizes.max.width - supportedSizes.min.width;\n>> +\tif (vStep == 0)\n>> +\t\tvStep = supportedSizes.max.height - supportedSizes.min.height;\n>> +\n>> +\tSize adjusted = requestedSize.boundedTo(supportedSizes.max).expandedTo(supportedSizes.min);\n>> +\n>> +\treturn adjusted.shrunkBy(supportedSizes.min).alignedDownTo(hStep, vStep).grownBy(supportedSizes.min);\n> \n> Those lines are getting long. You can write\n> \n> \tSize adjusted = requestedSize.boundedTo(supportedSizes.max)\n> \t\t\t\t .expandedTo(supportedSizes.min);\n> \treturn adjusted.shrunkBy(supportedSizes.min)\n> \t\t .alignedDownTo(hStep, vStep)\n> \t\t .grownBy(supportedSizes.min);\n> \n> or even\n> \n> \treturn requestedSize.boundedTo(supportedSizes.max)\n> \t\t\t .expandedTo(supportedSizes.min)\n> \t\t\t .shrunkBy(supportedSizes.min)\n> \t\t\t .alignedDownTo(hStep, vStep)\n> \t\t\t .grownBy(supportedSizes.min);\n> \n> I can change this when applying if you tell me which version you like\n> best (if any).\n\nAs for me, the both versions look good.\nThe first version matching my taste a tiny bit better probably.\n\nThanks,\nAndrei\n\n> Reviewed-by: Laurent Pinchart \n> \n>> +}\n>> +\n>> +} /* namespace */\n>> +\n>> CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> {\n>> \tconst CameraSensor *sensor = data_->sensor_.get();\n>> @@ -998,10 +1022,19 @@ CameraConfiguration::Status SimpleCameraConfiguration::validate()\n>> \t\t}\n>> \n>> \t\tif (!pipeConfig_->outputSizes.contains(cfg.size)) {\n>> +\t\t\tSize adjustedSize = pipeConfig_->captureSize;\n>> +\t\t\t/*\n>> +\t\t\t * The converter (when present) may not be able to output\n>> +\t\t\t * a size identical to its input size. The capture size is thus\n>> +\t\t\t * not guaranteed to be a valid output size. In such cases, use\n>> +\t\t\t * the smaller valid output size closest to the requested.\n>> +\t\t\t */\n>> +\t\t\tif (!pipeConfig_->outputSizes.contains(adjustedSize))\n>> +\t\t\t\tadjustedSize = adjustSize(cfg.size, pipeConfig_->outputSizes);\n>> \t\t\tLOG(SimplePipeline, Debug)\n>> \t\t\t\t<< \"Adjusting size from \" << cfg.size\n>> -\t\t\t\t<< \" to \" << pipeConfig_->captureSize;\n>> -\t\t\tcfg.size = pipeConfig_->captureSize;\n>> +\t\t\t\t<< \" to \" << adjustedSize;\n>> +\t\t\tcfg.size = adjustedSize;\n>> \t\t\tstatus = Adjusted;\n>> \t\t}\n>> \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 33D31BD160\n\tfor ;\n\tWed, 20 Mar 2024 12:53:26 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2EFE262822;\n\tWed, 20 Mar 2024 13:53:25 +0100 (CET)","from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com\n\t[IPv6:2a00:1450:4864:20::52b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4DB16603AC\n\tfor ;\n\tWed, 20 Mar 2024 13:53:24 +0100 (CET)","by mail-ed1-x52b.google.com with SMTP id\n\t4fb4d7f45d1cf-568d323e7fbso4679635a12.3\n\tfor ;\n\tWed, 20 Mar 2024 05:53:24 -0700 (PDT)","from [192.168.118.26] ([87.116.160.98])\n\tby smtp.gmail.com with ESMTPSA id\n\tp13-20020a170906228d00b00a4674a2bb3csm7043270eja.1.2024.03.20.05.53.22\n\t(version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);\n\tWed, 20 Mar 2024 05:53:23 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key;\n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"HSQOi/F4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20230601; t=1710939204; x=1711544004;\n\tdarn=lists.libcamera.org; \n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:from:to:cc:subject:date:message-id:reply-to;\n\tbh=lbVnXlPaI8m8FviDVJ1Sdt8GppMnVp6C+SUURNQklm0=;\n\tb=HSQOi/F4jzxLVrz+BfRwkU4RPUPD4EOIQiY718vDxDJQ8DzlHPEkv1TUIAuGz2G5cl\n\tFUH5/RxUA/hzb8OkURMEWULB7ZT3wqeOTrv9zhMrzxrGlvq0n0k6KQcw0aqUOTK7N4VL\n\tPCTvr+WWAih0x8Es6PaNJfg0Md5fo5FfEUr4Oq3Bn8cMTU90hG+K/ZsU18MOil6KBcN9\n\t+KIwj/Gzvdt46WW0Fl+yJktvdfGd9RoahPfhrxE/O3EdOQH8RMo7ca+ur5QZCp29n0HX\n\thqOLRQTc+kXGCgsjKTafdfGOB+lVDG83Uu1S1mL55YyrmkqdQe9c5442GBb7cuKu6Qso\n\tWf+Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20230601; t=1710939204; x=1711544004;\n\th=content-transfer-encoding:in-reply-to:from:references:cc:to\n\t:content-language:subject:user-agent:mime-version:date:message-id\n\t:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to;\n\tbh=lbVnXlPaI8m8FviDVJ1Sdt8GppMnVp6C+SUURNQklm0=;\n\tb=rZ8I80epqv3gIm9u/NZDuZ8NnIUWsr85HqszKMUrGJHH85SNDigRuQxAfg0fb61sHE\n\tbcc543m81IbKYjm3H7CNxeOHdoriGBMqQuwNMCQ9qsIXBfmHa7HiGk/PLt3ZAYI+I/YI\n\tW2JeWTY/YgV8XuY1hU6xq7tt/MQO1mU+sDb7UiEBdkxUPsc67cWX1daEmjoqK6i1L4/X\n\toHsytBiWYbbZdRdA4Kyy3kldmT/G/w7su8xUEkT6fVGk8ow7nGWquCwPfEKWIKq1Q82a\n\tbyXtzudMDEcZZObguL+gkqYY3FiS/0I18TFAdkrzAUv/ZgtpC1WO6UAf6oXQ1pFmfoxM\n\tbXDw==","X-Gm-Message-State":"AOJu0Yx8V2t0lA+ozbROybJzCLOie7lejVOxj4PcreUsakLyFxkixg2b\n\tsc364ORI4QSatmtAPGD33V5hluSV1sh2weQgEDqhGp6NtEJoChLT","X-Google-Smtp-Source":"AGHT+IGtDzNCbM3MKrqK7qpVGHZWgczweVSw8uwTgEq+PiefJdthOval6cRFvVVlLwmX1tGpW4gbsA==","X-Received":"by 2002:a17:906:d0d2:b0:a46:ecc1:87f9 with SMTP id\n\tbq18-20020a170906d0d200b00a46ecc187f9mr2039110ejb.7.1710939203462; \n\tWed, 20 Mar 2024 05:53:23 -0700 (PDT)","Message-ID":"","Date":"Wed, 20 Mar 2024 15:53:21 +0300","MIME-Version":"1.0","User-Agent":"Mozilla Thunderbird","Subject":"Re: [PATCH v6 01/18] libcamera: pipeline: simple: fix size\n\tadjustment in validate()","Content-Language":"en-US","To":"Laurent Pinchart ,\n\tMilan Zamazal ","Cc":"libcamera-devel@lists.libcamera.org,\n\tAndrey Konovalov ,\n\tBryan O'Donoghue ,\n\tMaxime Ripard , Pavel Machek ,\n\tHans de Goede ,\n\tKieran Bingham ","References":"<20240319123622.675599-1-mzamazal@redhat.com>\n\t<20240319123622.675599-2-mzamazal@redhat.com>\n\t<20240320121749.GD6052@pendragon.ideasonboard.com>","From":"Andrei Konovalov ","In-Reply-To":"<20240320121749.GD6052@pendragon.ideasonboard.com>","Content-Type":"text/plain; charset=UTF-8; format=flowed","Content-Transfer-Encoding":"7bit","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","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]