[{"id":23530,"web_url":"https://patchwork.libcamera.org/comment/23530/","msgid":"","date":"2022-06-22T16:06:55","subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"On Wed, Jun 22, 2022 at 6:02 PM Daniel Semkowicz wrote:\n>\n> This change is required for AfWindows control from capture script.\n> Parser expects array of arrays of parameters, so it is possible to\n> specify multiple rectangles.\n> ---\n> src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----\n> src/cam/capture_script.h | 9 +++-\n> 2 files changed, 102 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> index 9f22d5f7..ff620abd 100644\n> --- a/src/cam/capture_script.cpp\n> +++ b/src/cam/capture_script.cpp\n> @@ -232,12 +232,19 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> return -EINVAL;\n> }\n>\n> - std::string value = parseScalar();\n> - if (value.empty())\n> - return -EINVAL;\n> -\n> const ControlId *controlId = it->second;\n> - ControlValue val = unpackControl(controlId, value);\n> + ControlValue val;\n> +\n> + if (controlId->type() == ControlTypeRectangle) {\n> + int result = parseRectangles(val);\n> + if (result)\n> + return result;\n> + } else {\n> + int result = parseBasicType(controlId, val);\n> + if (result)\n> + return result;\n> + }\n> +\n> controls.set(controlId->id(), val);\n>\n> return 0;\n> @@ -252,6 +259,77 @@ std::string CaptureScript::parseScalar()\n> return eventScalarValue(event);\n> }\n>\n> +int CaptureScript::parseBasicType(const ControlId *id, ControlValue &controlValue)\n> +{\n> + std::string value = parseScalar();\n> + if (value.empty())\n> + return -EINVAL;\n> +\n> + controlValue = unpackBasicType(id, value);\n> +\n> + return 0;\n> +}\n> +\n> +int CaptureScript::parseRectangles(libcamera::ControlValue &controlValue)\n> +{\n> + std::vector rectangles;\n> +\n> + EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> + if (!event)\n> + return -EINVAL;\n> +\n> + while (1) {\n> + event = nextEvent();\n> + if (!event)\n> + return -EINVAL;\n> +\n> + if (event->type == YAML_SEQUENCE_END_EVENT) {\n> + break;\n> + }\n> +\n> + if (event->type != YAML_SEQUENCE_START_EVENT) {\n> + return -EINVAL;\n> + }\n> +\n> + std::vector values = parseArray();\n> + if (values.size() != 4) {\n> + std::cerr << \"Error parsing Rectangle: \"\n> + << \"expected array with 4 parameters\"\n> + << std::endl;\n> + return -EINVAL;\n> + }\n> +\n> + Rectangle rect = unpackRectangle(values);\n> + rectangles.push_back(rect);\n> + }\n> +\n> + controlValue.set(Span(rectangles));\n> +\n> + return 0;\n> +}\n> +\n> +std::vector CaptureScript::parseArray()\n> +{\n> + std::vector values;\n> +\n> + while (1) {\n> + EventPtr event = nextEvent();\n> + if (!event)\n> + break;\n> +\n> + if (event->type != YAML_SCALAR_EVENT) {\n> + break;\n> + }\n> +\n> + std::string value = eventScalarValue(event);\n> + values.push_back(value);\n> + if (value.empty())\n> + break;\n> + }\n> +\n> + return values;\n> +}\n> +\n> void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> {\n> static const std::map typeNames = {\n> @@ -277,7 +355,7 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> << typeName << \" control \" << id->name() << std::endl;\n> }\n>\n> -ControlValue CaptureScript::unpackControl(const ControlId *id,\n> +ControlValue CaptureScript::unpackBasicType(const ControlId *id,\n> const std::string &repr)\n> {\n> ControlValue value{};\n> @@ -325,12 +403,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,\n> break;\n> }\n> case ControlTypeRectangle:\n> - /* \\todo Parse rectangles. */\n> - break;\n> case ControlTypeSize:\n> - /* \\todo Parse Sizes. */\n> + unpackFailure(id, repr);\n> break;\n> }\n>\n> return value;\n> }\n> +\n> +libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector &strVec)\n> +{\n> + int x = strtol(strVec[0].c_str(), NULL, 10);\n> + int y = strtol(strVec[1].c_str(), NULL, 10);\n> + unsigned int width = strtoul(strVec[2].c_str(), NULL, 10);\n> + unsigned int height = strtoul(strVec[3].c_str(), NULL, 10);\n> +\n> + return Rectangle(x, y, width, height);\n> +}\n> diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> index 8b4f8f62..73b7e101 100644\n> --- a/src/cam/capture_script.h\n> +++ b/src/cam/capture_script.h\n> @@ -52,11 +52,16 @@ private:\n> int parseFrames();\n> int parseFrame(EventPtr event);\n> int parseControl(EventPtr event, libcamera::ControlList &controls);\n> + int parseBasicType(const libcamera::ControlId *id,\n> + libcamera::ControlValue &controlValue);\n> + int parseRectangles(libcamera::ControlValue &controlValue);\n>\n> std::string parseScalar();\n> + std::vector parseArray();\n>\n> void unpackFailure(const libcamera::ControlId *id,\n> const std::string &repr);\n> - libcamera::ControlValue unpackControl(const libcamera::ControlId *id,\n> - const std::string &repr);\n> + libcamera::ControlValue unpackBasicType(const libcamera::ControlId *id,\n> + const std::string &repr);\n> + libcamera::Rectangle unpackRectangle(const std::vector &strVec);\n> };\n> --\n> 2.34.1\n>\n\n\nSorry, I forgot to sign-off this commit... I will fix this after\nreview with any additional corrections\n\nBest regards\n\nDaniel","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 CEF94BD808\n\tfor ;\n\tWed, 22 Jun 2022 16:07:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 2D81965637;\n\tWed, 22 Jun 2022 18:07:09 +0200 (CEST)","from mail-lf1-x130.google.com (mail-lf1-x130.google.com\n\t[IPv6:2a00:1450:4864:20::130])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 5A38C61FB2\n\tfor ;\n\tWed, 22 Jun 2022 18:07:07 +0200 (CEST)","by mail-lf1-x130.google.com with SMTP id t25so28554442lfg.7\n\tfor ;\n\tWed, 22 Jun 2022 09:07:07 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655914029;\n\tbh=4aHs2z/HBNlhTtblzfA60DPRkZFYab0X1B8PWqDgldA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:\n\tFrom;\n\tb=ft4zTuzcg9nUSN1M9tF4paDxYH+NeiE8mX5QYO1tV/e/T1QgESvjzAQ6v1urz9cWx\n\t8tgvchB5rC7jRatLrwWhUivjbeaGzacNC1NW5lKFe9s438QPoH2f4cqcMau6mMCXSD\n\t7ZG343KFGD4UBs6pehOYli7oguAPSTw54fWPg4w8ogTweRhq9/ek/RO0OsXCBuFaGe\n\txDsIHhg2RfUThQeofcZ/OuhDsD3FIAM7w1VMJuIYjwpF3DiAM51MrshuiTBHzUsE/g\n\tvS0YTmcexSDNzEvc63ZFkaCffAwncJEzU5tI6ZLtoJfQ2hTzY0gjhU0Q7KV8QrFW6q\n\tw3HmKLwKg0FWA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to;\n\tbh=oGal9wBxD7yzFgQBNS5Brgt9DYEe02+AvROT2hYF9Rc=;\n\tb=kEc8Tggl/5FyucAqw6iD+oETq6HbQcBLP95R67Cl3GbYiKNDJmQMCdVYBqNkecvmOX\n\tnRoltWq1vBRomEc2nWi9syIZKtK4zBFE89tZ4o9PEuDjyFICDQHrslyitivA0oy31T32\n\t5OWCq4Zl5l3MxkeidV7RTXb0a4hN+YJ5+whkskiOMqTJLBevOS8cWT3yH+O1/T/LpMPS\n\tM63UqjFzox7wdCO9z9XMbshE+YoXBSnjwKK/xEnqeueISuFMuZWefm1ACjldQ9jm7Ywj\n\tOAGPsWhCLihIKfhvHvOdPIXNYCujm/VVeWSt/UWdXh6M72uoLo+hNnN+dViKo15LksO0\n\tqA9w=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"kEc8Tggl\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to;\n\tbh=oGal9wBxD7yzFgQBNS5Brgt9DYEe02+AvROT2hYF9Rc=;\n\tb=wD9czOPXus2PRYttzXDf6GRWuDENRo4sE0q87fc5mzoaLrDFMScebJ4NcZBJtmqhHS\n\tmdRKN3dvHl5d7WIKDfbmBGlc0YamNz02VdEQ1I9YeJpuZZu5p588u3zdQEPYHTEq1LKc\n\tg4Y5u0cgofKdcGsldS1HrOFORbs6+xQTRFMAYaZgnqk7wrgPOHgFrWmRwRZVH/oi2Zgu\n\t4KFfWcPZmgmmIUboPnzPHh5QVLBO7CirYG/esCqMQJ26MfCr4DbPIgHdjLf1N5gm8MsU\n\t6b4FTr8f2zh/7ZwiDabRceXE8o9P15yTMo4pWhLE6lEo9DPSKwWARRJzRTnBKDn6qGxX\n\tA6Nw==","X-Gm-Message-State":"AJIora/FcKt4qKgV3lWRgoYJgtN9+DnEwUNVIhWbHMYs3Pda6P+5glFU\n\typzr3CqGErb4OTTMWfM626rRzmdDYo4B8DG9xUCv29Fp75WIE5qK","X-Google-Smtp-Source":"AGRyM1sYnirAwKWup8edpa2D0bwLY1Ku11NioNr/7y4qDy5ETDUsQ0gb9qoAWmi4eeVxhgPI4GAye7rOCsDup4EtseA=","X-Received":"by 2002:a19:431a:0:b0:47f:6162:126 with SMTP id\n\tq26-20020a19431a000000b0047f61620126mr2679173lfa.394.1655914026470;\n\tWed, 22 Jun 2022 09:07:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20220622160226.42220-1-dse@thaumatec.com>\n\t<20220622160226.42220-2-dse@thaumatec.com>","In-Reply-To":"<20220622160226.42220-2-dse@thaumatec.com>","Date":"Wed, 22 Jun 2022 18:06:55 +0200","Message-ID":"","To":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23539,"web_url":"https://patchwork.libcamera.org/comment/23539/","msgid":"<20220623090146.4daqj73j63anj5jb@uno.localdomain>","date":"2022-06-23T09:01:46","subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel,\n\nOn Wed, Jun 22, 2022 at 06:02:26PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> This change is required for AfWindows control from capture script.\n> Parser expects array of arrays of parameters, so it is possible to\n> specify multiple rectangles.\n\nYou know already :) Missing SoB tag\n\n> ---\n> src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----\n> src/cam/capture_script.h | 9 +++-\n> 2 files changed, 102 insertions(+), 11 deletions(-)\n>\n> diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> index 9f22d5f7..ff620abd 100644\n> --- a/src/cam/capture_script.cpp\n> +++ b/src/cam/capture_script.cpp\n> @@ -232,12 +232,19 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> \t\treturn -EINVAL;\n> \t}\n>\n> -\tstd::string value = parseScalar();\n> -\tif (value.empty())\n> -\t\treturn -EINVAL;\n> -\n> \tconst ControlId *controlId = it->second;\n> -\tControlValue val = unpackControl(controlId, value);\n> +\tControlValue val;\n> +\n> +\tif (controlId->type() == ControlTypeRectangle) {\n> +\t\tint result = parseRectangles(val);\n> +\t\tif (result)\n> +\t\t\treturn result;\n> +\t} else {\n> +\t\tint result = parseBasicType(controlId, val);\n> +\t\tif (result)\n> +\t\t\treturn result;\n> +\t}\n> +\n\nWon't all of this be better placed in the \"case ControlTypeRectangle:\"\nstatement of CaptureScript::unpackControl() ? Have I missed why is it\nnecessary to bring it at this level ? Can't you call parseRectangles()\nfrom there ? if it's the parseScalar() call required to parse a basic\ntype this can't it be handled as:\n\nControlValue CaptureScript::unpackControl(const ControlId *id)\n{\n\n /* Parse complex types */\n swith (id->type()) {\n case Rectangle:\n parseRectangles();\n break\n case Size:\n /* error not supported */\n break;\n default:\n break;\n }\n\n /* Parse basic types represented by a single scalar. */\n\n std::string repr = parseScalar();\n if (value.empty())\n return -EVINVAL;\n\n switch (id->type()) {\n case None:\n case Bool:\n ....\n\n }\n}\n\nDo you think this would be possible ?\n\n> \tcontrols.set(controlId->id(), val);\n>\n> \treturn 0;\n> @@ -252,6 +259,77 @@ std::string CaptureScript::parseScalar()\n> \treturn eventScalarValue(event);\n> }\n>\n> +int CaptureScript::parseBasicType(const ControlId *id, ControlValue &controlValue)\n> +{\n> +\tstd::string value = parseScalar();\n> +\tif (value.empty())\n> +\t\treturn -EINVAL;\n> +\n> +\tcontrolValue = unpackBasicType(id, value);\n> +\n> +\treturn 0;\n> +}\n> +\n> +int CaptureScript::parseRectangles(libcamera::ControlValue &controlValue)\n> +{\n> +\tstd::vector rectangles;\n> +\n> +\tEventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> +\tif (!event)\n> +\t\treturn -EINVAL;\n> +\n> +\twhile (1) {\n> +\t\tevent = nextEvent();\n> +\t\tif (!event)\n> +\t\t\treturn -EINVAL;\n> +\n> +\t\tif (event->type == YAML_SEQUENCE_END_EVENT) {\n> +\t\t\tbreak;\n> +\t\t}\n\nno braces for single line statement.\n\n> +\n> +\t\tif (event->type != YAML_SEQUENCE_START_EVENT) {\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n\nI would consider a switch to make sure we catch all cases, even with\nhill-formed yaml files\n\n while (1) {\n bool sequenceEnd = false;\n\n switch (event->type) {\n case YAML_SEQUENCE_END_EVENT:\n sequencEnd = true;\n break;\n case YAML_SEQUENCE_START_EVENT:\n break;\n default:\n return -EINVAL;\n }\n\n if (sequenceEnd)\n break;\n> +\n> +\t\tstd::vector values = parseArray();\n> +\t\tif (values.size() != 4) {\n> +\t\t\tstd::cerr << \"Error parsing Rectangle: \"\n> +\t\t\t\t << \"expected array with 4 parameters\"\n> +\t\t\t\t << std::endl;\n> +\t\t\treturn -EINVAL;\n> +\t\t}\n> +\n> +\t\tRectangle rect = unpackRectangle(values);\n> +\t\trectangles.push_back(rect);\n> +\t}\n> +\n> +\tcontrolValue.set(Span(rectangles));\n> +\n> +\treturn 0;\n> +}\n> +\n> +std::vector CaptureScript::parseArray()\n> +{\n> +\tstd::vector values;\n> +\n> +\twhile (1) {\n> +\t\tEventPtr event = nextEvent();\n> +\t\tif (!event)\n> +\t\t\tbreak;\n> +\n> +\t\tif (event->type != YAML_SCALAR_EVENT) {\n> +\t\t\tbreak;\n> +\t\t}\n\nSimilar comments as the ones on the previous function\n\n> +\n> +\t\tstd::string value = eventScalarValue(event);\n> +\t\tvalues.push_back(value);\n> +\t\tif (value.empty())\n\nWhat is this check for ? Shouldn't it be done before adding the\nelement to the vector ?\n\n> +\t\t\tbreak;\n> +\t}\n> +\n> +\treturn values;\n> +}\n> +\n> void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> {\n> \tstatic const std::map typeNames = {\n> @@ -277,7 +355,7 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> \t\t << typeName << \" control \" << id->name() << std::endl;\n> }\n>\n> -ControlValue CaptureScript::unpackControl(const ControlId *id,\n> +ControlValue CaptureScript::unpackBasicType(const ControlId *id,\n> \t\t\t\t\t const std::string &repr)\n> {\n> \tControlValue value{};\n> @@ -325,12 +403,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,\n> \t\tbreak;\n> \t}\n> \tcase ControlTypeRectangle:\n> -\t\t/* \\todo Parse rectangles. */\n> -\t\tbreak;\n> \tcase ControlTypeSize:\n> -\t\t/* \\todo Parse Sizes. */\n> +\t\tunpackFailure(id, repr);\n> \t\tbreak;\n> \t}\n>\n> \treturn value;\n> }\n> +\n> +libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector &strVec)\n> +{\n> +\tint x = strtol(strVec[0].c_str(), NULL, 10);\n> +\tint y = strtol(strVec[1].c_str(), NULL, 10);\n> +\tunsigned int width = strtoul(strVec[2].c_str(), NULL, 10);\n> +\tunsigned int height = strtoul(strVec[3].c_str(), NULL, 10);\n> +\n> +\treturn Rectangle(x, y, width, height);\n> +}\n\nVery happy to see Rectangle supported in the capture script! Are you\nusing it to test which functionality ?\n\nThanks for the patch\n j\n\n> diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> index 8b4f8f62..73b7e101 100644\n> --- a/src/cam/capture_script.h\n> +++ b/src/cam/capture_script.h\n> @@ -52,11 +52,16 @@ private:\n> \tint parseFrames();\n> \tint parseFrame(EventPtr event);\n> \tint parseControl(EventPtr event, libcamera::ControlList &controls);\n> +\tint parseBasicType(const libcamera::ControlId *id,\n> +\t\t\t libcamera::ControlValue &controlValue);\n> +\tint parseRectangles(libcamera::ControlValue &controlValue);\n>\n> \tstd::string parseScalar();\n> +\tstd::vector parseArray();\n>\n> \tvoid unpackFailure(const libcamera::ControlId *id,\n> \t\t\t const std::string &repr);\n> -\tlibcamera::ControlValue unpackControl(const libcamera::ControlId *id,\n> -\t\t\t\t\t const std::string &repr);\n> +\tlibcamera::ControlValue unpackBasicType(const libcamera::ControlId *id,\n> +\t\t\t\t\t\tconst std::string &repr);\n> +\tlibcamera::Rectangle unpackRectangle(const std::vector &strVec);\n> };\n> --\n> 2.34.1\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 09166BE173\n\tfor ;\n\tThu, 23 Jun 2022 09:01:51 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3C16365635;\n\tThu, 23 Jun 2022 11:01:50 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 08E9760412\n\tfor ;\n\tThu, 23 Jun 2022 11:01:49 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id BEA53100017;\n\tThu, 23 Jun 2022 09:01:47 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655974910;\n\tbh=AZJ6L47oNJBzMXg9qREmG9758T8dTeuzKUD8Z46B0Hs=;\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=lRk4x23PACMEtEbqR7O0kqt8ekmRs6cvf4UrDFnNUvOqqlxlfPxY4sbPjbvIRGrdd\n\t3f4hE/gAHGj04hVb0sVJ3MPEy+NV+4qm1Nfx8cyRpabNIx1aF7Nru1JkEaMhccXkM/\n\t1JhCuV4hc/LWoJj0hz12mza8O8zHNTRF1IAjbebt0ZbwRTml7PKqmqWTfVxNMz9E/7\n\tTL7RhNODJRaazZWlLN5uztlLA2Q0vYrIWmYH8F0xkG0w87+2eC9qkuTljc3Op0IrJi\n\tGXhexf8Gd0NO9/WKKU1aUKiDtA9YXx37hSlhaUBoXWwSEaDqREstsYAEhHu+dWa6lY\n\tyJOhAvfvunCag==","Date":"Thu, 23 Jun 2022 11:01:46 +0200","To":"Daniel Semkowicz ","Message-ID":"<20220623090146.4daqj73j63anj5jb@uno.localdomain>","References":"<20220622160226.42220-1-dse@thaumatec.com>\n\t<20220622160226.42220-2-dse@thaumatec.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"<20220622160226.42220-2-dse@thaumatec.com>","Subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","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":23540,"web_url":"https://patchwork.libcamera.org/comment/23540/","msgid":"","date":"2022-06-23T10:07:37","subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","submitter":{"id":126,"url":"https://patchwork.libcamera.org/api/people/126/","name":"Daniel Semkowicz","email":"dse@thaumatec.com"},"content":"Hi Jacopo,\n\nOn Thu, Jun 23, 2022 at 11:01 AM Jacopo Mondi wrote:\n>\n> Hi Daniel,\n>\n> On Wed, Jun 22, 2022 at 06:02:26PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > This change is required for AfWindows control from capture script.\n> > Parser expects array of arrays of parameters, so it is possible to\n> > specify multiple rectangles.\n>\n> You know already :) Missing SoB tag\n>\n> > ---\n> > src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----\n> > src/cam/capture_script.h | 9 +++-\n> > 2 files changed, 102 insertions(+), 11 deletions(-)\n> >\n> > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > index 9f22d5f7..ff620abd 100644\n> > --- a/src/cam/capture_script.cpp\n> > +++ b/src/cam/capture_script.cpp\n> > @@ -232,12 +232,19 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > return -EINVAL;\n> > }\n> >\n> > - std::string value = parseScalar();\n> > - if (value.empty())\n> > - return -EINVAL;\n> > -\n> > const ControlId *controlId = it->second;\n> > - ControlValue val = unpackControl(controlId, value);\n> > + ControlValue val;\n> > +\n> > + if (controlId->type() == ControlTypeRectangle) {\n> > + int result = parseRectangles(val);\n> > + if (result)\n> > + return result;\n> > + } else {\n> > + int result = parseBasicType(controlId, val);\n> > + if (result)\n> > + return result;\n> > + }\n> > +\n>\n> Won't all of this be better placed in the \"case ControlTypeRectangle:\"\n> statement of CaptureScript::unpackControl() ? Have I missed why is it\n> necessary to bring it at this level ? Can't you call parseRectangles()\n> from there ? if it's the parseScalar() call required to parse a basic\n> type this can't it be handled as:\n>\n> ControlValue CaptureScript::unpackControl(const ControlId *id)\n> {\n>\n> /* Parse complex types */\n> swith (id->type()) {\n> case Rectangle:\n> parseRectangles();\n> break\n> case Size:\n> /* error not supported */\n> break;\n> default:\n> break;\n> }\n>\n> /* Parse basic types represented by a single scalar. */\n>\n> std::string repr = parseScalar();\n> if (value.empty())\n> return -EVINVAL;\n>\n> switch (id->type()) {\n> case None:\n> case Bool:\n> ....\n>\n> }\n> }\n>\n> Do you think this would be possible ?\n\nIt looked like the unpackControl() method was specialized to only unpack\nthe specific type from the string that was already read from yaml.\nAnd reading the string from the yaml scalar event was done in parseScalar().\nI just tried to maintain the already existing convention of separated\nfunctionality of \"parse\" and \"unpack\" methods.\n\nUnfortunately, in both cases We still need to have two paths (for basic\nand complex types) - two switches instead of one.\n\nOne thing I see better in your approach is that We will have both switches\nin one function and I think it would be a more clear to read.\n\n>\n> > controls.set(controlId->id(), val);\n> >\n> > return 0;\n> > @@ -252,6 +259,77 @@ std::string CaptureScript::parseScalar()\n> > return eventScalarValue(event);\n> > }\n> >\n> > +int CaptureScript::parseBasicType(const ControlId *id, ControlValue &controlValue)\n> > +{\n> > + std::string value = parseScalar();\n> > + if (value.empty())\n> > + return -EINVAL;\n> > +\n> > + controlValue = unpackBasicType(id, value);\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +int CaptureScript::parseRectangles(libcamera::ControlValue &controlValue)\n> > +{\n> > + std::vector rectangles;\n> > +\n> > + EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > + if (!event)\n> > + return -EINVAL;\n> > +\n> > + while (1) {\n> > + event = nextEvent();\n> > + if (!event)\n> > + return -EINVAL;\n> > +\n> > + if (event->type == YAML_SEQUENCE_END_EVENT) {\n> > + break;\n> > + }\n>\n> no braces for single line statement.\n>\n> > +\n> > + if (event->type != YAML_SEQUENCE_START_EVENT) {\n> > + return -EINVAL;\n> > + }\n>\n> I would consider a switch to make sure we catch all cases, even with\n> hill-formed yaml files\n>\n> while (1) {\n> bool sequenceEnd = false;\n>\n> switch (event->type) {\n> case YAML_SEQUENCE_END_EVENT:\n> sequencEnd = true;\n> break;\n> case YAML_SEQUENCE_START_EVENT:\n> break;\n> default:\n> return -EINVAL;\n> }\n>\n> if (sequenceEnd)\n> break;\n> > +\n> > + std::vector values = parseArray();\n> > + if (values.size() != 4) {\n> > + std::cerr << \"Error parsing Rectangle: \"\n> > + << \"expected array with 4 parameters\"\n> > + << std::endl;\n> > + return -EINVAL;\n> > + }\n> > +\n> > + Rectangle rect = unpackRectangle(values);\n> > + rectangles.push_back(rect);\n> > + }\n> > +\n> > + controlValue.set(Span(rectangles));\n> > +\n> > + return 0;\n> > +}\n> > +\n> > +std::vector CaptureScript::parseArray()\n> > +{\n> > + std::vector values;\n> > +\n> > + while (1) {\n> > + EventPtr event = nextEvent();\n> > + if (!event)\n> > + break;\n> > +\n> > + if (event->type != YAML_SCALAR_EVENT) {\n> > + break;\n> > + }\n>\n> Similar comments as the ones on the previous function\n>\n\nSure, I will correct these points.\n\n> > +\n> > + std::string value = eventScalarValue(event);\n> > + values.push_back(value);\n> > + if (value.empty())\n>\n> What is this check for ? Shouldn't it be done before adding the\n> element to the vector ?\n>\n\nOf course it should... I overlooked that...\n\n> > + break;\n> > + }\n> > +\n> > + return values;\n> > +}\n> > +\n> > void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> > {\n> > static const std::map typeNames = {\n> > @@ -277,7 +355,7 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> > << typeName << \" control \" << id->name() << std::endl;\n> > }\n> >\n> > -ControlValue CaptureScript::unpackControl(const ControlId *id,\n> > +ControlValue CaptureScript::unpackBasicType(const ControlId *id,\n> > const std::string &repr)\n> > {\n> > ControlValue value{};\n> > @@ -325,12 +403,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,\n> > break;\n> > }\n> > case ControlTypeRectangle:\n> > - /* \\todo Parse rectangles. */\n> > - break;\n> > case ControlTypeSize:\n> > - /* \\todo Parse Sizes. */\n> > + unpackFailure(id, repr);\n> > break;\n> > }\n> >\n> > return value;\n> > }\n> > +\n> > +libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector &strVec)\n> > +{\n> > + int x = strtol(strVec[0].c_str(), NULL, 10);\n> > + int y = strtol(strVec[1].c_str(), NULL, 10);\n> > + unsigned int width = strtoul(strVec[2].c_str(), NULL, 10);\n> > + unsigned int height = strtoul(strVec[3].c_str(), NULL, 10);\n> > +\n> > + return Rectangle(x, y, width, height);\n> > +}\n>\n> Very happy to see Rectangle supported in the capture script! Are you\n> using it to test which functionality ?\n>\nAfWindows. I am working on AF for rkisp1. I have a very basic AF that\njust moves through the full focal range of lens and choose the best sharpness.\nI was not pushing it to the upstream as this works only on AfTrigger in\nsuch approach, so no continuous mode. I planned to change the algorithm\nto something similar as already implemented in IPU3 and then push it\nin the final form. But as I am writing this message now, I am wondering\nif maybe it would be better to push it even in this simple form as these\nchanges also introduce some side-functionalities needed for Af, such as\nlens control from ipu. And later push the algorithm with continuous mode.\nThis would introduce some functionalities earlier but not fully working.\nWhat do you think?\n\nBest regards\nDaniel\n\n> Thanks for the patch\n> j\n>\n> > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > index 8b4f8f62..73b7e101 100644\n> > --- a/src/cam/capture_script.h\n> > +++ b/src/cam/capture_script.h\n> > @@ -52,11 +52,16 @@ private:\n> > int parseFrames();\n> > int parseFrame(EventPtr event);\n> > int parseControl(EventPtr event, libcamera::ControlList &controls);\n> > + int parseBasicType(const libcamera::ControlId *id,\n> > + libcamera::ControlValue &controlValue);\n> > + int parseRectangles(libcamera::ControlValue &controlValue);\n> >\n> > std::string parseScalar();\n> > + std::vector parseArray();\n> >\n> > void unpackFailure(const libcamera::ControlId *id,\n> > const std::string &repr);\n> > - libcamera::ControlValue unpackControl(const libcamera::ControlId *id,\n> > - const std::string &repr);\n> > + libcamera::ControlValue unpackBasicType(const libcamera::ControlId *id,\n> > + const std::string &repr);\n> > + libcamera::Rectangle unpackRectangle(const std::vector &strVec);\n> > };\n> > --\n> > 2.34.1\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 018CABD808\n\tfor ;\n\tThu, 23 Jun 2022 10:07:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 52C9B65632;\n\tThu, 23 Jun 2022 12:07:52 +0200 (CEST)","from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com\n\t[IPv6:2a00:1450:4864:20::12f])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id CDE216048F\n\tfor ;\n\tThu, 23 Jun 2022 12:07:49 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id f39so16837781lfv.3\n\tfor ;\n\tThu, 23 Jun 2022 03:07:49 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655978872;\n\tbh=MU3+IJrjqjklnHfERwZzJq9TWtuiiSrDAyIHE+nkATs=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=ZOuVhEQd1ovAk/eHBEGWfrOUyOltPXEO2mWVma/9lWCd0hZQSoxKhThApB/MY6rt6\n\tPx/bZFMkaFi++kzAt5kv3o9s/TiqRGqljFbolqkDKVHT0Sm4n3BA1rtr00MTfCsUet\n\t3Rwwz/9hqlhjM+YdxRqBvTk9ccjrGJFnAuC7lsfbLrjietltIlTFZEbMQMVXsspkgB\n\toF8IXypfcF9IiNtVBVY5XwjZX5fANbcf+dYcwK6cemPFFlaPBWi3fZujOpCdIzOxZm\n\tIVevTAfdmLoxL1JoDBLljmuRgFSeOiqQAs3LWBROdJqY2YFtMxVR6LDAAtqkZQAuYx\n\tQMn2apr2sQyQA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=thaumatec-com.20210112.gappssmtp.com; s=20210112;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc; bh=iice3P+pBH/0Rz7B7FHq/w7fdG2qUw9szTSG16kgs9A=;\n\tb=km7PCWLyrZf6dmcNQGc4ArekXiOzJZyEWIwQapBxS9TbkU93hvCmbeHXzPw+cFmNJ+\n\tZgo4xrj4EXFQ3ZwTWOeBgfALABKGq1oT1JA+l2S/88jYFwMFyJyy9sy0IrXl52xiQ1PQ\n\tZOJClYwdCxCfzY4LSIZGXloPW2UdPgAq5RqanlnI0HvoIfVYN8+BBeSrPBJJJn6spgWM\n\tkzAPNUiNZFmjtmO+rz2LoO/aphbEicq476HFqPoQLBqTNHf9W1QxPLi2xds/WrScz8kc\n\tNb9rZ7s9VuBZmfaTWL2UlI9r32Io5R1HdGERTMVPDWlg78lZhwVN8JreJgqqhYA7GQAQ\n\tDuBA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected)\n\theader.d=thaumatec-com.20210112.gappssmtp.com\n\theader.i=@thaumatec-com.20210112.gappssmtp.com header.b=\"km7PCWLy\"; \n\tdkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20210112;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc;\n\tbh=iice3P+pBH/0Rz7B7FHq/w7fdG2qUw9szTSG16kgs9A=;\n\tb=UoS1tGBu0EqcmJONsZeAYQNfak/6Z8ShELekZzvKS5Hd+04GuDlFpdaGvKT6jl60HU\n\tuqxzEYFiAqaFeCZOlYdyPqxyH+UYEoEmFgapbjFVEG6GdOZ0bXAGfef3T1dukOEk5MAF\n\tPgLI7xn+ocgrZNKWTZ/DFarghXzHNGokueaM3AIeCuGoawEbwCRbPRl3yhZRjZAnbxKl\n\tfCqxn40EDcTa/Ri8+3a0xrr3cKMTfB4SG82rBRyp45dt2hQBGRQc3PePGS0dEHIYwuSh\n\t6Bl21LFNg4fD7fbuCk6px4Np2rbi+oo6a/EFaG/AG6DR5Qa24kClxY2h4sLSlnK6XC0c\n\tVm1g==","X-Gm-Message-State":"AJIora/M8LVk6tkumAqoWPoMpM9QG/t4f2Wj0DNgKgZwMhdNSCkCCw+m\n\thXcC1nObGvyviDRroboZaNllYZPxv5f/GZa5gRS/m97ijL0NjbWk","X-Google-Smtp-Source":"AGRyM1subEgWtRyoBff16Eel7D9E1YEBiFlRhw6I7BHhCcAksMSjLpLTD2rBTXPgMH38tBE3JHD5EO14s0CbDa38RWA=","X-Received":"by 2002:a05:6512:1684:b0:47f:5f27:b006 with SMTP id\n\tbu4-20020a056512168400b0047f5f27b006mr4948816lfb.225.1655978868880;\n\tThu, 23 Jun 2022 03:07:48 -0700 (PDT)","MIME-Version":"1.0","References":"<20220622160226.42220-1-dse@thaumatec.com>\n\t<20220622160226.42220-2-dse@thaumatec.com>\n\t<20220623090146.4daqj73j63anj5jb@uno.localdomain>","In-Reply-To":"<20220623090146.4daqj73j63anj5jb@uno.localdomain>","Date":"Thu, 23 Jun 2022 12:07:37 +0200","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"text/plain; charset=\"UTF-8\"","Subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","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":"Daniel Semkowicz via libcamera-devel\n\t","Reply-To":"Daniel Semkowicz ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":23541,"web_url":"https://patchwork.libcamera.org/comment/23541/","msgid":"<20220623104135.zkvx3myvawc6bwtr@uno.localdomain>","date":"2022-06-23T10:41:35","subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Daniel,\n\nOn Thu, Jun 23, 2022 at 12:07:37PM +0200, Daniel Semkowicz wrote:\n> Hi Jacopo,\n>\n> On Thu, Jun 23, 2022 at 11:01 AM Jacopo Mondi wrote:\n> >\n> > Hi Daniel,\n> >\n> > On Wed, Jun 22, 2022 at 06:02:26PM +0200, Daniel Semkowicz via libcamera-devel wrote:\n> > > This change is required for AfWindows control from capture script.\n> > > Parser expects array of arrays of parameters, so it is possible to\n> > > specify multiple rectangles.\n> >\n> > You know already :) Missing SoB tag\n> >\n> > > ---\n> > > src/cam/capture_script.cpp | 104 +++++++++++++++++++++++++++++++++----\n> > > src/cam/capture_script.h | 9 +++-\n> > > 2 files changed, 102 insertions(+), 11 deletions(-)\n> > >\n> > > diff --git a/src/cam/capture_script.cpp b/src/cam/capture_script.cpp\n> > > index 9f22d5f7..ff620abd 100644\n> > > --- a/src/cam/capture_script.cpp\n> > > +++ b/src/cam/capture_script.cpp\n> > > @@ -232,12 +232,19 @@ int CaptureScript::parseControl(EventPtr event, ControlList &controls)\n> > > return -EINVAL;\n> > > }\n> > >\n> > > - std::string value = parseScalar();\n> > > - if (value.empty())\n> > > - return -EINVAL;\n> > > -\n> > > const ControlId *controlId = it->second;\n> > > - ControlValue val = unpackControl(controlId, value);\n> > > + ControlValue val;\n> > > +\n> > > + if (controlId->type() == ControlTypeRectangle) {\n> > > + int result = parseRectangles(val);\n> > > + if (result)\n> > > + return result;\n> > > + } else {\n> > > + int result = parseBasicType(controlId, val);\n> > > + if (result)\n> > > + return result;\n> > > + }\n> > > +\n> >\n> > Won't all of this be better placed in the \"case ControlTypeRectangle:\"\n> > statement of CaptureScript::unpackControl() ? Have I missed why is it\n> > necessary to bring it at this level ? Can't you call parseRectangles()\n> > from there ? if it's the parseScalar() call required to parse a basic\n> > type this can't it be handled as:\n> >\n> > ControlValue CaptureScript::unpackControl(const ControlId *id)\n> > {\n> >\n> > /* Parse complex types */\n> > swith (id->type()) {\n> > case Rectangle:\n> > parseRectangles();\n> > break\n> > case Size:\n> > /* error not supported */\n> > break;\n> > default:\n> > break;\n> > }\n> >\n> > /* Parse basic types represented by a single scalar. */\n> >\n> > std::string repr = parseScalar();\n> > if (value.empty())\n> > return -EVINVAL;\n> >\n> > switch (id->type()) {\n> > case None:\n> > case Bool:\n> > ....\n> >\n> > }\n> > }\n> >\n> > Do you think this would be possible ?\n>\n> It looked like the unpackControl() method was specialized to only unpack\n> the specific type from the string that was already read from yaml.\n> And reading the string from the yaml scalar event was done in parseScalar().\n> I just tried to maintain the already existing convention of separated\n> functionality of \"parse\" and \"unpack\" methods.\n>\n> Unfortunately, in both cases We still need to have two paths (for basic\n> and complex types) - two switches instead of one.\n>\n> One thing I see better in your approach is that We will have both switches\n> in one function and I think it would be a more clear to read.\n>\n> >\n> > > controls.set(controlId->id(), val);\n> > >\n> > > return 0;\n> > > @@ -252,6 +259,77 @@ std::string CaptureScript::parseScalar()\n> > > return eventScalarValue(event);\n> > > }\n> > >\n> > > +int CaptureScript::parseBasicType(const ControlId *id, ControlValue &controlValue)\n> > > +{\n> > > + std::string value = parseScalar();\n> > > + if (value.empty())\n> > > + return -EINVAL;\n> > > +\n> > > + controlValue = unpackBasicType(id, value);\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +int CaptureScript::parseRectangles(libcamera::ControlValue &controlValue)\n> > > +{\n> > > + std::vector rectangles;\n> > > +\n> > > + EventPtr event = nextEvent(YAML_SEQUENCE_START_EVENT);\n> > > + if (!event)\n> > > + return -EINVAL;\n> > > +\n> > > + while (1) {\n> > > + event = nextEvent();\n> > > + if (!event)\n> > > + return -EINVAL;\n> > > +\n> > > + if (event->type == YAML_SEQUENCE_END_EVENT) {\n> > > + break;\n> > > + }\n> >\n> > no braces for single line statement.\n> >\n> > > +\n> > > + if (event->type != YAML_SEQUENCE_START_EVENT) {\n> > > + return -EINVAL;\n> > > + }\n> >\n> > I would consider a switch to make sure we catch all cases, even with\n> > hill-formed yaml files\n> >\n> > while (1) {\n> > bool sequenceEnd = false;\n> >\n> > switch (event->type) {\n> > case YAML_SEQUENCE_END_EVENT:\n> > sequencEnd = true;\n> > break;\n> > case YAML_SEQUENCE_START_EVENT:\n> > break;\n> > default:\n> > return -EINVAL;\n> > }\n> >\n> > if (sequenceEnd)\n> > break;\n> > > +\n> > > + std::vector values = parseArray();\n> > > + if (values.size() != 4) {\n> > > + std::cerr << \"Error parsing Rectangle: \"\n> > > + << \"expected array with 4 parameters\"\n> > > + << std::endl;\n> > > + return -EINVAL;\n> > > + }\n> > > +\n> > > + Rectangle rect = unpackRectangle(values);\n> > > + rectangles.push_back(rect);\n> > > + }\n> > > +\n> > > + controlValue.set(Span(rectangles));\n> > > +\n> > > + return 0;\n> > > +}\n> > > +\n> > > +std::vector CaptureScript::parseArray()\n> > > +{\n> > > + std::vector values;\n> > > +\n> > > + while (1) {\n> > > + EventPtr event = nextEvent();\n> > > + if (!event)\n> > > + break;\n> > > +\n> > > + if (event->type != YAML_SCALAR_EVENT) {\n> > > + break;\n> > > + }\n> >\n> > Similar comments as the ones on the previous function\n> >\n>\n> Sure, I will correct these points.\n>\n> > > +\n> > > + std::string value = eventScalarValue(event);\n> > > + values.push_back(value);\n> > > + if (value.empty())\n> >\n> > What is this check for ? Shouldn't it be done before adding the\n> > element to the vector ?\n> >\n>\n> Of course it should... I overlooked that...\n>\n> > > + break;\n> > > + }\n> > > +\n> > > + return values;\n> > > +}\n> > > +\n> > > void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> > > {\n> > > static const std::map typeNames = {\n> > > @@ -277,7 +355,7 @@ void CaptureScript::unpackFailure(const ControlId *id, const std::string &repr)\n> > > << typeName << \" control \" << id->name() << std::endl;\n> > > }\n> > >\n> > > -ControlValue CaptureScript::unpackControl(const ControlId *id,\n> > > +ControlValue CaptureScript::unpackBasicType(const ControlId *id,\n> > > const std::string &repr)\n> > > {\n> > > ControlValue value{};\n> > > @@ -325,12 +403,20 @@ ControlValue CaptureScript::unpackControl(const ControlId *id,\n> > > break;\n> > > }\n> > > case ControlTypeRectangle:\n> > > - /* \\todo Parse rectangles. */\n> > > - break;\n> > > case ControlTypeSize:\n> > > - /* \\todo Parse Sizes. */\n> > > + unpackFailure(id, repr);\n> > > break;\n> > > }\n> > >\n> > > return value;\n> > > }\n> > > +\n> > > +libcamera::Rectangle CaptureScript::unpackRectangle(const std::vector &strVec)\n> > > +{\n> > > + int x = strtol(strVec[0].c_str(), NULL, 10);\n> > > + int y = strtol(strVec[1].c_str(), NULL, 10);\n> > > + unsigned int width = strtoul(strVec[2].c_str(), NULL, 10);\n> > > + unsigned int height = strtoul(strVec[3].c_str(), NULL, 10);\n> > > +\n> > > + return Rectangle(x, y, width, height);\n> > > +}\n> >\n> > Very happy to see Rectangle supported in the capture script! Are you\n> > using it to test which functionality ?\n> >\n> AfWindows. I am working on AF for rkisp1. I have a very basic AF that\n\nAwesome!\n\n> just moves through the full focal range of lens and choose the best sharpness.\n> I was not pushing it to the upstream as this works only on AfTrigger in\n> such approach, so no continuous mode. I planned to change the algorithm\n> to something similar as already implemented in IPU3 and then push it\n> in the final form. But as I am writing this message now, I am wondering\n\nFYI there's a port of the same IPU3 algorithm for RPi if it helps\nmaking a comparison\nhttps://patchwork.libcamera.org/project/libcamera/list/?series=3174\n\n+Cc Jean-Michel as he's the author\n\n\n> if maybe it would be better to push it even in this simple form as these\n> changes also introduce some side-functionalities needed for Af, such as\n> lens control from ipu. And later push the algorithm with continuous mode.\n\nI think gradual steps is indeed better. The trigger-based version can\nbe submitted first, and continuous later.\n\nThe best of the best would be a coordinate effort in having both the\nRkISP1 and the RPi AF algorithm merged, so feel free to comment/review\nthe RPi series if you have comments on it.\n\n> This would introduce some functionalities earlier but not fully working.\n> What do you think?\n\nAs long as they do not introduce regressions, and there's a plan to\nuse them, I think it's probably fine..\n\nThanks\n j\n\n>\n> Best regards\n> Daniel\n>\n> > Thanks for the patch\n> > j\n> >\n> > > diff --git a/src/cam/capture_script.h b/src/cam/capture_script.h\n> > > index 8b4f8f62..73b7e101 100644\n> > > --- a/src/cam/capture_script.h\n> > > +++ b/src/cam/capture_script.h\n> > > @@ -52,11 +52,16 @@ private:\n> > > int parseFrames();\n> > > int parseFrame(EventPtr event);\n> > > int parseControl(EventPtr event, libcamera::ControlList &controls);\n> > > + int parseBasicType(const libcamera::ControlId *id,\n> > > + libcamera::ControlValue &controlValue);\n> > > + int parseRectangles(libcamera::ControlValue &controlValue);\n> > >\n> > > std::string parseScalar();\n> > > + std::vector parseArray();\n> > >\n> > > void unpackFailure(const libcamera::ControlId *id,\n> > > const std::string &repr);\n> > > - libcamera::ControlValue unpackControl(const libcamera::ControlId *id,\n> > > - const std::string &repr);\n> > > + libcamera::ControlValue unpackBasicType(const libcamera::ControlId *id,\n> > > + const std::string &repr);\n> > > + libcamera::Rectangle unpackRectangle(const std::vector &strVec);\n> > > };\n> > > --\n> > > 2.34.1\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 DE479BE173\n\tfor ;\n\tThu, 23 Jun 2022 10:41:39 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 49CF665635;\n\tThu, 23 Jun 2022 12:41:39 +0200 (CEST)","from relay8-d.mail.gandi.net (relay8-d.mail.gandi.net\n\t[217.70.183.201])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 262FC6048F\n\tfor ;\n\tThu, 23 Jun 2022 12:41:38 +0200 (CEST)","(Authenticated sender: jacopo@jmondi.org)\n\tby mail.gandi.net (Postfix) with ESMTPSA id 5C4BF1BF20C;\n\tThu, 23 Jun 2022 10:41:37 +0000 (UTC)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1655980899;\n\tbh=0wHIMVHUR1qJ/9V9St8VnLAJNmI/0a3gAB9K4iL1b7k=;\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=SI02xVDW0dPHxCNVAVUU5rpRDM8gmqDlmiRdoq147omOHwBGW+yDwt/9rVbARZSg5\n\tbMBEpCS+zQ7MUUPddilpNHvYKdlVaJ4Q/9dU30vut82sV8UpkR36GblryoDAt1OWnm\n\tCpHBkKvpn8XPmZs9CIkOZYrpv6yD75wwTslTnnkgLV1y7MnicF6t9CWm9CUe2g98Ak\n\tIpJbALBCkY3UznUgSQLS+fiLygyvXD4KL6rbooBlPCF/ompfAmtzDvfBqXptBKo2cx\n\t2wzyULH2wNjS9IFXaJkHYyRzIFsq5RWhQqrgEawO31sH32FjozutLCymNL9eqPI6/K\n\tcCKNaQBQzIakg==","Date":"Thu, 23 Jun 2022 12:41:35 +0200","To":"Daniel Semkowicz ,\n\tJean-Michel Hautbois ","Message-ID":"<20220623104135.zkvx3myvawc6bwtr@uno.localdomain>","References":"<20220622160226.42220-1-dse@thaumatec.com>\n\t<20220622160226.42220-2-dse@thaumatec.com>\n\t<20220623090146.4daqj73j63anj5jb@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH 1/1] cam: Add Rectangle type parsing\n\tin capture script","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\" "}}]