[{"id":12882,"web_url":"https://patchwork.libcamera.org/comment/12882/","msgid":"<20200929223356.GC1271798@oden.dyn.berto.se>","date":"2020-09-29T22:33:56","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi David,\n\nThanks for your patch.\n\nOn 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> This allows the user to request digital zoom by adding, for example,\n> \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> the middle of the image.\n> \n> Signed-off-by: David Plowman \n\nIf I understand the cover letter correctly this is just a rebase so I \nunderstand not all comments from v2 are addressed. But just to reiterate \nmy comments from v2.\n\n> ---\n> src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> src/cam/capture.h | 2 +-\n> src/cam/main.cpp | 3 +++\n> src/cam/main.h | 1 +\n> 4 files changed, 28 insertions(+), 3 deletions(-)\n> \n> diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> index 5510c009..3b50c591 100644\n> --- a/src/cam/capture.cpp\n> +++ b/src/cam/capture.cpp\n> @@ -10,6 +10,9 @@\n> #include \n> #include \n> \n> +#include \n> +#include \n> +\n> #include \"capture.h\"\n> #include \"main.h\"\n> \n> @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> \n> \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> \n> -\tret = capture(allocator);\n> +\tret = capture(allocator, options);\n> \n> \tif (options.isSet(OptFile)) {\n> \t\tdelete writer_;\n> @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> \treturn ret;\n> }\n> \n> -int Capture::capture(FrameBufferAllocator *allocator)\n> +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> {\n> \tint ret;\n> \n> @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> \t\trequests.push_back(request);\n> \t}\n> \n> +\t/* Set up digital zoom if it was requested on the command line. */\n> +\tif (options.isSet(OptZoom)) {\n> +\t\tstd::string zoom = options[OptZoom].toString();\n> +\t\tfloat x, y, width, height;\n> +\n> +\t\tif (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n\nThis is the wrong place to parse the input from the user. If the zoom \noptions are global something similar like StreamKeyValueParser\n(ZoomKeyValueParser?) should be created. If it is to be moved a stream \nlevel StreamKeyValueParser should be extended.\n\n> +\t\t\tSize sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> +\t\t\tRectangle crop(x * sensorArea.width,\n> +\t\t\t\t y * sensorArea.height,\n> +\t\t\t\t width * sensorArea.width,\n> +\t\t\t\t height * sensorArea.height);\n> +\n> +\t\t\trequests.front()->controls().set(controls::IspCrop, crop);\n\nDo we wish to expose the ISP in the public API? If we do is the ISP\n(from an application point of view) the correct level to operate on?\nWould not setting this on a stream level cover more hardware?\n\nI'm thinking of an ISP that can apply different crop parameters to two\n(or more) source pads from a single sink pad. Having a control that sets\na global crop on the ISP would that not limit such hardware or am I\nmissing something?\n\n> +\t\t} else {\n> +\t\t\tstd::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> +\t\t}\n> +\t}\n> +\n> \tret = camera_->start();\n> \tif (ret) {\n> \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> diff --git a/src/cam/capture.h b/src/cam/capture.h\n> index 0aebdac9..52806164 100644\n> --- a/src/cam/capture.h\n> +++ b/src/cam/capture.h\n> @@ -29,7 +29,7 @@ public:\n> \n> \tint run(const OptionsParser::Options &options);\n> private:\n> -\tint capture(libcamera::FrameBufferAllocator *allocator);\n> +\tint capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> \n> \tvoid requestComplete(libcamera::Request *request);\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index 244720b4..b871d049 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> \tparser.addOption(OptStrictFormats, OptionNone,\n> \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> \t\t\t \"strict-formats\");\n> +\tparser.addOption(OptZoom, OptionString,\n> +\t\t\t \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> +\t\t\t \"zoom\", ArgumentRequired, \"zoom\");\n> \n> \toptions_ = parser.parse(argc, argv);\n> \tif (!options_.valid())\n> diff --git a/src/cam/main.h b/src/cam/main.h\n> index ea8dfd33..a1fd899f 100644\n> --- a/src/cam/main.h\n> +++ b/src/cam/main.h\n> @@ -17,6 +17,7 @@ enum {\n> \tOptListProperties = 'p',\n> \tOptMonitor = 'm',\n> \tOptStream = 's',\n> +\tOptZoom = 'z',\n> \tOptListControls = 256,\n> \tOptStrictFormats = 257,\n> };\n> -- \n> 2.20.1\n> \n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 6DFA8C3B5B\n\tfor ;\n\tTue, 29 Sep 2020 22:34:00 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id D0367621D8;\n\tWed, 30 Sep 2020 00:33:59 +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 4B39A60366\n\tfor ;\n\tWed, 30 Sep 2020 00:33:58 +0200 (CEST)","by mail-lf1-x12f.google.com with SMTP id u8so7482435lff.1\n\tfor ;\n\tTue, 29 Sep 2020 15:33:58 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tx5sm3387915lfd.119.2020.09.29.15.33.56\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tTue, 29 Sep 2020 15:33:56 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"VhQVwQlk\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=zKxM7/7kmZRPbgy0CA+Z07owsIqFvUScqxCne0T768E=;\n\tb=VhQVwQlkw2nDMWCCH9+STFJppCSP0WZN6gADBoT/hdpxmpnTYZj6UpZAGegAykFSiK\n\t/yWXwohoctOjRJ7oF/K+IGYW64ILh7CWo9UrntSoSXfc+8P8y3mKIxwAPQMdfikhfqAP\n\t6FnVz8lfbKUk3Zl3v4GrK4TZsBu4E9yXDaaGaOljXLBXlH5y92+1xTY5GFFEO7xvVsiq\n\t39hRS3gUMhJQUHutxf4vp8a1CH3Ir+gSX9ursyZqKT3Atzu8omnEe95osLuODEvCvBUK\n\tRxASvggKjVCKO/SbZliN1zhggn9MYqaY2HfWwlkVfAS67ZZmtWBe+Fu8EUYiMiqo7rgj\n\tYEzg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=zKxM7/7kmZRPbgy0CA+Z07owsIqFvUScqxCne0T768E=;\n\tb=p83BWfr9JTcw8//kX+LUENsrxbA6B4PhUCUxhPONGb44I2z641m3TayOtgjg6IoEIY\n\tdOZgvpQKyEbtgyoPXImaVmKOy+LPQWNndCkmeHfEZ6rPKAauRB/LOBPw+ZYYFhtpzgiA\n\tlqsdF93OvjVKAkqEcMImOvEOVZ4azdjN3s9gwvpDmNyaO6ytpSqx0UbmYf2EGxnC8SGf\n\tpvQsADzO/hFfcXYyPH88lFC1u21LFaI7nxF+gLNNVldptv2cZqBmE8bOyNEajmcIqOxv\n\trM/ux3iXQB7kfEha/rIHJniks6YMnBNws9tWIW3ittE9rWpViDlLHJb7rsUQsfXr+Nme\n\t0Abg==","X-Gm-Message-State":"AOAM530hghjhTM2W8wh/HgUo0NX64NgZEsiokCqNdgXDf0mIsL8b/5El\n\t2Mw3uX3ZF044kbv5BTnWEggSzw==","X-Google-Smtp-Source":"ABdhPJw/8A75Jc0CdLxBtN5izbFBTfWfktmTo966fVB3+/xl7pyUyNwwQD/6YaWup95Qkmm5K6oK8g==","X-Received":"by 2002:a19:4bd6:: with SMTP id\n\ty205mr2056839lfa.182.1601418837453; \n\tTue, 29 Sep 2020 15:33:57 -0700 (PDT)","Date":"Wed, 30 Sep 2020 00:33:56 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"David Plowman ","Message-ID":"<20200929223356.GC1271798@oden.dyn.berto.se>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929164000.15429-7-david.plowman@raspberrypi.com>","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12885,"web_url":"https://patchwork.libcamera.org/comment/12885/","msgid":"","date":"2020-09-30T07:00:18","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Niklas\n\nThanks for the comments, and sorry for not addressing them sooner!\n\nOn Tue, 29 Sep 2020 at 23:33, Niklas Söderlund\n wrote:\n>\n> Hi David,\n>\n> Thanks for your patch.\n>\n> On 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> > This allows the user to request digital zoom by adding, for example,\n> > \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> > the middle of the image.\n> >\n> > Signed-off-by: David Plowman \n>\n> If I understand the cover letter correctly this is just a rebase so I\n\nCorrect, apart from a couple of minor mods (though one rather important one!)\n\n> understand not all comments from v2 are addressed. But just to reiterate\n> my comments from v2.\n>\n> > ---\n> > src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> > src/cam/capture.h | 2 +-\n> > src/cam/main.cpp | 3 +++\n> > src/cam/main.h | 1 +\n> > 4 files changed, 28 insertions(+), 3 deletions(-)\n> >\n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 5510c009..3b50c591 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -10,6 +10,9 @@\n> > #include \n> > #include \n> >\n> > +#include \n> > +#include \n> > +\n> > #include \"capture.h\"\n> > #include \"main.h\"\n> >\n> > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> >\n> > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> >\n> > - ret = capture(allocator);\n> > + ret = capture(allocator, options);\n> >\n> > if (options.isSet(OptFile)) {\n> > delete writer_;\n> > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > return ret;\n> > }\n> >\n> > -int Capture::capture(FrameBufferAllocator *allocator)\n> > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> > {\n> > int ret;\n> >\n> > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > requests.push_back(request);\n> > }\n> >\n> > + /* Set up digital zoom if it was requested on the command line. */\n> > + if (options.isSet(OptZoom)) {\n> > + std::string zoom = options[OptZoom].toString();\n> > + float x, y, width, height;\n> > +\n> > + if (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n>\n> This is the wrong place to parse the input from the user. If the zoom\n> options are global something similar like StreamKeyValueParser\n> (ZoomKeyValueParser?) should be created. If it is to be moved a stream\n> level StreamKeyValueParser should be extended.\n\nYes, I was kind of hoping not to get too involved with all that! How about this:\n\n1. Add an OptionFloat type\n\n2. Change the \"isArray\" option field to \"size\" or something, and make\nit an unsigned int. We'd make its default value 1, meaning \"not an\narray\" and edit any existing usage that sets it to False to use 1\ninstead.\n\n3. It seems to me that the current semantics for an \"array\" option is\nthat they're variable length, i.e. it accepts as many as you type in.\nPerhaps we could use zero size for such an array, and otherwise insist\nthat we get exactly the number that were wanted?\n\n4. Then in my case I could supply a size of 4, and get an array of 4 floats.\n\n5. It's true there is still some checking that needs to be done on the\nvalues to be sure they're sensible - my view here is that pipeline\nhandlers should deal with garbage as applications can easily submit\ngarbage, and here we have a way of checking that the pipeline handler\nactually does this correctly!\n\n>\n> > + Size sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> > + Rectangle crop(x * sensorArea.width,\n> > + y * sensorArea.height,\n> > + width * sensorArea.width,\n> > + height * sensorArea.height);\n> > +\n> > + requests.front()->controls().set(controls::IspCrop, crop);\n>\n> Do we wish to expose the ISP in the public API? If we do is the ISP\n> (from an application point of view) the correct level to operate on?\n> Would not setting this on a stream level cover more hardware?\n>\n> I'm thinking of an ISP that can apply different crop parameters to two\n> (or more) source pads from a single sink pad. Having a control that sets\n> a global crop on the ISP would that not limit such hardware or am I\n> missing something?\n\nIndeed you're right, there is a bit of a dilemma here. There was some\ndiscussion of this a while back, I think with Jacopo, unfortunately I\ndon't have any email references to hand.\n\nI think the feeling was that in all the real use cases that we can\nimagine, you generally want the same field of view on all your\noutputs. I think situations where something else would be useful are\ncertainly less \"mainstream\".\n\nSecondly, some ISPs will let you crop outputs differently, and some\nwon't. That's an awkward thing to handle - how would you know what the\nunderlying hardware does, and then would you write different code for\neach case? The pragmatic answer may just be to implement the \"usual\"\ncase here, and it becomes a question for the future whether we get\nper-stream crop controls, or whether a StreamConfiguration gets crop\nrectangles...\n\nMaybe Jacopo can comment if I've got anything wrong there!\n\nThanks again and best regards\nDavid\n\n>\n> > + } else {\n> > + std::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> > + }\n> > + }\n> > +\n> > ret = camera_->start();\n> > if (ret) {\n> > std::cout << \"Failed to start capture\" << std::endl;\n> > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > index 0aebdac9..52806164 100644\n> > --- a/src/cam/capture.h\n> > +++ b/src/cam/capture.h\n> > @@ -29,7 +29,7 @@ public:\n> >\n> > int run(const OptionsParser::Options &options);\n> > private:\n> > - int capture(libcamera::FrameBufferAllocator *allocator);\n> > + int capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> >\n> > void requestComplete(libcamera::Request *request);\n> >\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index 244720b4..b871d049 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > parser.addOption(OptStrictFormats, OptionNone,\n> > \"Do not allow requested stream format(s) to be adjusted\",\n> > \"strict-formats\");\n> > + parser.addOption(OptZoom, OptionString,\n> > + \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> > + \"zoom\", ArgumentRequired, \"zoom\");\n> >\n> > options_ = parser.parse(argc, argv);\n> > if (!options_.valid())\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index ea8dfd33..a1fd899f 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -17,6 +17,7 @@ enum {\n> > OptListProperties = 'p',\n> > OptMonitor = 'm',\n> > OptStream = 's',\n> > + OptZoom = 'z',\n> > OptListControls = 256,\n> > OptStrictFormats = 257,\n> > };\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel\n>\n> --\n> Regards,\n> Niklas Söderlund","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 52771C3B5C\n\tfor ;\n\tWed, 30 Sep 2020 07:00:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B1F4E62209;\n\tWed, 30 Sep 2020 09:00:32 +0200 (CEST)","from mail-oi1-x234.google.com (mail-oi1-x234.google.com\n\t[IPv6:2607:f8b0:4864:20::234])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 7D1CC60A0F\n\tfor ;\n\tWed, 30 Sep 2020 09:00:31 +0200 (CEST)","by mail-oi1-x234.google.com with SMTP id z26so590492oih.12\n\tfor ;\n\tWed, 30 Sep 2020 00:00:31 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"W5xNMnQV\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=pYKD0uRHa7bea7OCdAPqDwlCLdU/ElTGpZQC88kSYZs=;\n\tb=W5xNMnQV9mhBd7Q4wsOBj8Okih6diobcH/eNFDBFzKy5SO6t3/4uADMl0oI0FUy1uT\n\tjzvKoXvTNGsWCFPHrFDckUdKhUvpD8FOSDOa0dtU07fqa5Ru55vTb1R4nFeG5v6cRfys\n\tvVigsjkezFx9lQ83FhGqF/xRB3X0f5XFrt/DGnf2LKjVmfAGyTLjIpE28RX/he3gK5Ml\n\tpAkl1xJhujHsUN0yha7PIVe64oSu6+My6u8/Q+7Df9r577UHZskDRy067cF16UyWvsw+\n\tyR+izfSGtJjObDp6r/ZAeNfxWlPvcm+hbt2q/XcYjXO1CiHMjENO5RDp+sWgr7smcv1+\n\t3C9Q==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=pYKD0uRHa7bea7OCdAPqDwlCLdU/ElTGpZQC88kSYZs=;\n\tb=jvyABrtW/8QsEsRmHOpyJKUFpMddwsUC20gnK1FfrsmsQxxwssAnTaV9EHYf2GgfTb\n\tEzCTzzFR7mEjYmun27twINpAxxlhsA0tYJgIZuIOejUx50Wp8k55Kcj1FMbyJWjH9PYd\n\tAwD1smQebaqQjpU0jW9Qye3D5Lho9kVDj2uJ9vd7f/CptbiMdPvyEaJTHARiZJ+/uc4y\n\tVJKaQQF+VzctM0EQV0A30GE4V0GWETTz/727XrOzfX8nfpxor3NUr+5/spDHJY+Hm2at\n\tdo2s1zIux1AnOGE0emThHWd1e/Ck8D/Z+7xZ9k//5EGb02SjO5o2QOVHBLP/CiMQpGBr\n\to2pA==","X-Gm-Message-State":"AOAM530izIjOL8FWOHXdBbKmC70LfE1RJaTVxNRoM5kxxJHszr0rjF+4\n\tO4lWMnXuyapFbhiBCM4VtKZopsqingI7pSwpBgV6kg==","X-Google-Smtp-Source":"ABdhPJzv78A8XGt5rX/bpyhXRYOPStJRUr8uJgZpGcvRCs/NV3kCoUh1gjbkAs3lPH3x5Mhg7q4cs1gJLCtw4WjykJc=","X-Received":"by 2002:aca:5546:: with SMTP id j67mr681591oib.107.1601449229384;\n\tWed, 30 Sep 2020 00:00:29 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>\n\t<20200929223356.GC1271798@oden.dyn.berto.se>","In-Reply-To":"<20200929223356.GC1271798@oden.dyn.berto.se>","From":"David Plowman ","Date":"Wed, 30 Sep 2020 08:00:18 +0100","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12895,"web_url":"https://patchwork.libcamera.org/comment/12895/","msgid":"<20200930084228.nw75qcahulelkmyh@uno.localdomain>","date":"2020-09-30T08:42:28","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi David,\n\nOn Wed, Sep 30, 2020 at 08:00:18AM +0100, David Plowman wrote:\n> Hi Niklas\n>\n> Thanks for the comments, and sorry for not addressing them sooner!\n>\n> On Tue, 29 Sep 2020 at 23:33, Niklas Söderlund\n> wrote:\n> >\n> > Hi David,\n> >\n> > Thanks for your patch.\n> >\n> > On 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> > > This allows the user to request digital zoom by adding, for example,\n> > > \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> > > the middle of the image.\n> > >\n> > > Signed-off-by: David Plowman \n> >\n> > If I understand the cover letter correctly this is just a rebase so I\n>\n> Correct, apart from a couple of minor mods (though one rather important one!)\n>\n> > understand not all comments from v2 are addressed. But just to reiterate\n> > my comments from v2.\n> >\n> > > ---\n> > > src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> > > src/cam/capture.h | 2 +-\n> > > src/cam/main.cpp | 3 +++\n> > > src/cam/main.h | 1 +\n> > > 4 files changed, 28 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 5510c009..3b50c591 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -10,6 +10,9 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +#include \n> > > +\n> > > #include \"capture.h\"\n> > > #include \"main.h\"\n> > >\n> > > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > >\n> > > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > >\n> > > - ret = capture(allocator);\n> > > + ret = capture(allocator, options);\n> > >\n> > > if (options.isSet(OptFile)) {\n> > > delete writer_;\n> > > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > > return ret;\n> > > }\n> > >\n> > > -int Capture::capture(FrameBufferAllocator *allocator)\n> > > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> > > {\n> > > int ret;\n> > >\n> > > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > requests.push_back(request);\n> > > }\n> > >\n> > > + /* Set up digital zoom if it was requested on the command line. */\n> > > + if (options.isSet(OptZoom)) {\n> > > + std::string zoom = options[OptZoom].toString();\n> > > + float x, y, width, height;\n> > > +\n> > > + if (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n> >\n> > This is the wrong place to parse the input from the user. If the zoom\n> > options are global something similar like StreamKeyValueParser\n> > (ZoomKeyValueParser?) should be created. If it is to be moved a stream\n> > level StreamKeyValueParser should be extended.\n>\n> Yes, I was kind of hoping not to get too involved with all that! How about this:\n>\n> 1. Add an OptionFloat type\n>\n> 2. Change the \"isArray\" option field to \"size\" or something, and make\n> it an unsigned int. We'd make its default value 1, meaning \"not an\n> array\" and edit any existing usage that sets it to False to use 1\n> instead.\n>\n> 3. It seems to me that the current semantics for an \"array\" option is\n> that they're variable length, i.e. it accepts as many as you type in.\n> Perhaps we could use zero size for such an array, and otherwise insist\n> that we get exactly the number that were wanted?\n>\n> 4. Then in my case I could supply a size of 4, and get an array of 4 floats.\n>\n> 5. It's true there is still some checking that needs to be done on the\n> values to be sure they're sensible - my view here is that pipeline\n> handlers should deal with garbage as applications can easily submit\n> garbage, and here we have a way of checking that the pipeline handler\n> actually does this correctly!\n>\n> >\n> > > + Size sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> > > + Rectangle crop(x * sensorArea.width,\n> > > + y * sensorArea.height,\n> > > + width * sensorArea.width,\n> > > + height * sensorArea.height);\n> > > +\n> > > + requests.front()->controls().set(controls::IspCrop, crop);\n> >\n> > Do we wish to expose the ISP in the public API? If we do is the ISP\n> > (from an application point of view) the correct level to operate on?\n> > Would not setting this on a stream level cover more hardware?\n> >\n> > I'm thinking of an ISP that can apply different crop parameters to two\n> > (or more) source pads from a single sink pad. Having a control that sets\n> > a global crop on the ISP would that not limit such hardware or am I\n> > missing something?\n>\n> Indeed you're right, there is a bit of a dilemma here. There was some\n> discussion of this a while back, I think with Jacopo, unfortunately I\n> don't have any email references to hand.\n>\n> I think the feeling was that in all the real use cases that we can\n> imagine, you generally want the same field of view on all your\n> outputs. I think situations where something else would be useful are\n> certainly less \"mainstream\".\n>\n> Secondly, some ISPs will let you crop outputs differently, and some\n> won't. That's an awkward thing to handle - how would you know what the\n> underlying hardware does, and then would you write different code for\n> each case? The pragmatic answer may just be to implement the \"usual\"\n> case here, and it becomes a question for the future whether we get\n> per-stream crop controls, or whether a StreamConfiguration gets crop\n> rectangles...\n>\n> Maybe Jacopo can comment if I've got anything wrong there!\n\nYes, we've gone through that, and I think my intereptation was that\nyou're better positioned than me, having a large user base and knowing\nwhat they expect, to decide if it was worth blocking this feature\nuntil we have decided how to implement per-Stream controls and how to\nhandle the situation you have described here when a platform supports\nsuch a feature at the Camera level or at the Stream level.\n\nMy opinion is that digital zoom it's a feature it's worth having, even\nif in sub-optimal shape (same applies to the SensorOutputSize\nproperty, which ideally should be reported as part of the\nCameraConfiguration). The alternative is to post-pone this until we\nhave augmented CameraConfiguration with properties (relatively easy)\nand implemented per-Stream control (less easy if you ask me).\n\nI know Laurent wanted to have a look here, so let's see what his\nopinion is.\n\nThanks\n j\n\n\n>\n> Thanks again and best regards\n> David\n>\n> >\n> > > + } else {\n> > > + std::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> > > + }\n> > > + }\n> > > +\n> > > ret = camera_->start();\n> > > if (ret) {\n> > > std::cout << \"Failed to start capture\" << std::endl;\n> > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > index 0aebdac9..52806164 100644\n> > > --- a/src/cam/capture.h\n> > > +++ b/src/cam/capture.h\n> > > @@ -29,7 +29,7 @@ public:\n> > >\n> > > int run(const OptionsParser::Options &options);\n> > > private:\n> > > - int capture(libcamera::FrameBufferAllocator *allocator);\n> > > + int capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> > >\n> > > void requestComplete(libcamera::Request *request);\n> > >\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index 244720b4..b871d049 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > parser.addOption(OptStrictFormats, OptionNone,\n> > > \"Do not allow requested stream format(s) to be adjusted\",\n> > > \"strict-formats\");\n> > > + parser.addOption(OptZoom, OptionString,\n> > > + \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> > > + \"zoom\", ArgumentRequired, \"zoom\");\n> > >\n> > > options_ = parser.parse(argc, argv);\n> > > if (!options_.valid())\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index ea8dfd33..a1fd899f 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -17,6 +17,7 @@ enum {\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > OptStream = 's',\n> > > + OptZoom = 'z',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > };\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","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 224F2C3B5C\n\tfor ;\n\tWed, 30 Sep 2020 08:38:36 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADBFD6221A;\n\tWed, 30 Sep 2020 10:38:35 +0200 (CEST)","from relay12.mail.gandi.net (relay12.mail.gandi.net\n\t[217.70.178.232])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 84F7A6035F\n\tfor ;\n\tWed, 30 Sep 2020 10:38:34 +0200 (CEST)","from uno.localdomain (93-34-118-233.ip49.fastwebnet.it\n\t[93.34.118.233]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay12.mail.gandi.net (Postfix) with ESMTPSA id 1ADD820001E;\n\tWed, 30 Sep 2020 08:38:32 +0000 (UTC)"],"Date":"Wed, 30 Sep 2020 10:42:28 +0200","From":"Jacopo Mondi ","To":"David Plowman ","Message-ID":"<20200930084228.nw75qcahulelkmyh@uno.localdomain>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>\n\t<20200929223356.GC1271798@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12909,"web_url":"https://patchwork.libcamera.org/comment/12909/","msgid":"<20200930113241.GE1516931@oden.dyn.berto.se>","date":"2020-09-30T11:32:41","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi David,\n\nOn 2020-09-30 08:00:18 +0100, David Plowman wrote:\n> Hi Niklas\n> \n> Thanks for the comments, and sorry for not addressing them sooner!\n> \n> On Tue, 29 Sep 2020 at 23:33, Niklas Söderlund\n> wrote:\n> >\n> > Hi David,\n> >\n> > Thanks for your patch.\n> >\n> > On 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> > > This allows the user to request digital zoom by adding, for example,\n> > > \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> > > the middle of the image.\n> > >\n> > > Signed-off-by: David Plowman \n> >\n> > If I understand the cover letter correctly this is just a rebase so I\n> \n> Correct, apart from a couple of minor mods (though one rather important one!)\n> \n> > understand not all comments from v2 are addressed. But just to reiterate\n> > my comments from v2.\n> >\n> > > ---\n> > > src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> > > src/cam/capture.h | 2 +-\n> > > src/cam/main.cpp | 3 +++\n> > > src/cam/main.h | 1 +\n> > > 4 files changed, 28 insertions(+), 3 deletions(-)\n> > >\n> > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > index 5510c009..3b50c591 100644\n> > > --- a/src/cam/capture.cpp\n> > > +++ b/src/cam/capture.cpp\n> > > @@ -10,6 +10,9 @@\n> > > #include \n> > > #include \n> > >\n> > > +#include \n> > > +#include \n> > > +\n> > > #include \"capture.h\"\n> > > #include \"main.h\"\n> > >\n> > > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > >\n> > > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > >\n> > > - ret = capture(allocator);\n> > > + ret = capture(allocator, options);\n> > >\n> > > if (options.isSet(OptFile)) {\n> > > delete writer_;\n> > > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > > return ret;\n> > > }\n> > >\n> > > -int Capture::capture(FrameBufferAllocator *allocator)\n> > > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> > > {\n> > > int ret;\n> > >\n> > > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > requests.push_back(request);\n> > > }\n> > >\n> > > + /* Set up digital zoom if it was requested on the command line. */\n> > > + if (options.isSet(OptZoom)) {\n> > > + std::string zoom = options[OptZoom].toString();\n> > > + float x, y, width, height;\n> > > +\n> > > + if (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n> >\n> > This is the wrong place to parse the input from the user. If the zoom\n> > options are global something similar like StreamKeyValueParser\n> > (ZoomKeyValueParser?) should be created. If it is to be moved a stream\n> > level StreamKeyValueParser should be extended.\n> \n> Yes, I was kind of hoping not to get too involved with all that!\n\n:-)\n\n> How about this:\n> \n> 1. Add an OptionFloat type\n> \n> 2. Change the \"isArray\" option field to \"size\" or something, and make\n> it an unsigned int. We'd make its default value 1, meaning \"not an\n> array\" and edit any existing usage that sets it to False to use 1\n> instead.\n> \n> 3. It seems to me that the current semantics for an \"array\" option is\n> that they're variable length, i.e. it accepts as many as you type in.\n> Perhaps we could use zero size for such an array, and otherwise insist\n> that we get exactly the number that were wanted?\n> \n> 4. Then in my case I could supply a size of 4, and get an array of 4 floats.\n> \n> 5. It's true there is still some checking that needs to be done on the\n> values to be sure they're sensible - my view here is that pipeline\n> handlers should deal with garbage as applications can easily submit\n> garbage, and here we have a way of checking that the pipeline handler\n> actually does this correctly!\n\nI think cam is the wrong tool to test error injection into pipelines. I \nalso understand you wish to avoid hacking on the input parsers. I on the \nother hand enjoy that work so I would be more then happy to supply you \nwith a patch you could squash into this patch.\n\nOnly thing blocking me from writing one now is that I'm not sure this \nshould be a new --zoom option (ZoomKeyValueParser) or added to the \nexisting --stream option (extend StreamKeyValueParser). This is effect \nwhat we are discussing below so I will postpone until we have resolved \nthis.\n\n> \n> >\n> > > + Size sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> > > + Rectangle crop(x * sensorArea.width,\n> > > + y * sensorArea.height,\n> > > + width * sensorArea.width,\n> > > + height * sensorArea.height);\n> > > +\n> > > + requests.front()->controls().set(controls::IspCrop, crop);\n> >\n> > Do we wish to expose the ISP in the public API? If we do is the ISP\n> > (from an application point of view) the correct level to operate on?\n> > Would not setting this on a stream level cover more hardware?\n> >\n> > I'm thinking of an ISP that can apply different crop parameters to two\n> > (or more) source pads from a single sink pad. Having a control that sets\n> > a global crop on the ISP would that not limit such hardware or am I\n> > missing something?\n> \n> Indeed you're right, there is a bit of a dilemma here. There was some\n> discussion of this a while back, I think with Jacopo, unfortunately I\n> don't have any email references to hand.\n> \n> I think the feeling was that in all the real use cases that we can\n> imagine, you generally want the same field of view on all your\n> outputs. I think situations where something else would be useful are\n> certainly less \"mainstream\".\n\nI'm sure there are use cases for different fields of view. I have seen \nvideo conferencing gear that in the participant list zoom in on the \ncenter of the screen while the speakers video is a larger FoV. I thought \nthis was Google HO but a quick test proves me wrong. In any case I'm \nsure this is done in software without any support from the camera, but I \nthink there are use-cases for this.\n\nI also think the flavor of a zoom control is in the same area as a \nrotation control. And rotation is something we sure wish to have per\nstream. So I think no matter which controls go first we are reaching the \npoint we need to add support for per stream controls.\n\n> \n> Secondly, some ISPs will let you crop outputs differently, and some\n> won't. That's an awkward thing to handle - how would you know what the\n> underlying hardware does, and then would you write different code for\n> each case?\n\nApplications must be able to enumerate the features of the camera and \nthen enable or disable certain use-cases based on the enumerated \nfeatures. Of course this might lead to the applications rejecting a \ncamera as it don't support the features it needs (A raw capture \napplication would likely reject a camera that can't capture in a raw \nformat it knows how to process).\n\n> The pragmatic answer may just be to implement the \"usual\"\n> case here, and it becomes a question for the future whether we get\n> per-stream crop controls, or whether a StreamConfiguration gets crop\n> rectangles...\n\nI think we need to implement both the usual and special cases, but we \nneed to do so in a user-friendly way. One way to do that is to provide \nhelpers for applications.\n\nI'm thinking as we know controls like digital zoom and rotation \ndepending on the hardware capabilities can apply to individual or all \nstreams the low-level API must allow for the most complex setup, \ncontrols on the stream level. But as you say their are use-cases where \nan application just wish to zoom or rotate all streams and then we could \nadd helpers for that, maybe something simple like this would work?\n\n Request::setControlAllStreams(Control, Value);\n\nThen applications who want to rotate or zoom all streams use such \nhelpers while special case applications will enumerate the camera \nfeatures and detect if it's possible to rotate or zoom on indivudual \nstreams and if so do it.\n\nAs Jacopo points out, we know Laurent is working in this area so I'm \nsure he has more comments or at least a different viewpoint then mine \n;-)\n\n> \n> Maybe Jacopo can comment if I've got anything wrong there!\n> \n> Thanks again and best regards\n> David\n> \n> >\n> > > + } else {\n> > > + std::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> > > + }\n> > > + }\n> > > +\n> > > ret = camera_->start();\n> > > if (ret) {\n> > > std::cout << \"Failed to start capture\" << std::endl;\n> > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > index 0aebdac9..52806164 100644\n> > > --- a/src/cam/capture.h\n> > > +++ b/src/cam/capture.h\n> > > @@ -29,7 +29,7 @@ public:\n> > >\n> > > int run(const OptionsParser::Options &options);\n> > > private:\n> > > - int capture(libcamera::FrameBufferAllocator *allocator);\n> > > + int capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> > >\n> > > void requestComplete(libcamera::Request *request);\n> > >\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index 244720b4..b871d049 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > parser.addOption(OptStrictFormats, OptionNone,\n> > > \"Do not allow requested stream format(s) to be adjusted\",\n> > > \"strict-formats\");\n> > > + parser.addOption(OptZoom, OptionString,\n> > > + \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> > > + \"zoom\", ArgumentRequired, \"zoom\");\n> > >\n> > > options_ = parser.parse(argc, argv);\n> > > if (!options_.valid())\n> > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > index ea8dfd33..a1fd899f 100644\n> > > --- a/src/cam/main.h\n> > > +++ b/src/cam/main.h\n> > > @@ -17,6 +17,7 @@ enum {\n> > > OptListProperties = 'p',\n> > > OptMonitor = 'm',\n> > > OptStream = 's',\n> > > + OptZoom = 'z',\n> > > OptListControls = 256,\n> > > OptStrictFormats = 257,\n> > > };\n> > > --\n> > > 2.20.1\n> > >\n> > > _______________________________________________\n> > > libcamera-devel mailing list\n> > > libcamera-devel@lists.libcamera.org\n> > > https://lists.libcamera.org/listinfo/libcamera-devel\n> >\n> > --\n> > Regards,\n> > Niklas Söderlund","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 201E5C3B5B\n\tfor ;\n\tWed, 30 Sep 2020 11:32:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A1636220B;\n\tWed, 30 Sep 2020 13:32:45 +0200 (CEST)","from mail-lf1-x135.google.com (mail-lf1-x135.google.com\n\t[IPv6:2a00:1450:4864:20::135])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4B4AA60BD4\n\tfor ;\n\tWed, 30 Sep 2020 13:32:43 +0200 (CEST)","by mail-lf1-x135.google.com with SMTP id x69so1715379lff.3\n\tfor ;\n\tWed, 30 Sep 2020 04:32:43 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\t21sm159692lfg.263.2020.09.30.04.32.41\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 30 Sep 2020 04:32:41 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=ragnatech-se.20150623.gappssmtp.com\n\theader.i=@ragnatech-se.20150623.gappssmtp.com\n\theader.b=\"wUbL8UNK\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=ragnatech-se.20150623.gappssmtp.com; s=20150623;\n\th=date:from:to:cc:subject:message-id:references:mime-version\n\t:content-disposition:content-transfer-encoding:in-reply-to;\n\tbh=LNtpedjraXJB1uepWr0CdawoxPzmACZ0je/ehGDx1Yw=;\n\tb=wUbL8UNKD/ip3cOT68APb1qc4pRXcfx/J0f22jpQgpOIkF0HCf2xxB7yImOkd/VwoI\n\tVISED7zkuSfpdvyGGKTMM57fGGZdUXqRHvC2KwU3YHHyHWQ/1gI77amE1lDQXDqPQf3d\n\tu099x/ea6s0wC2wd7ztWgCuuREmiRc7wqNFMLy2XEXVu9HtvDEA2ZalI+h+9/NxVXAR+\n\tMTEBVGjshVg0ybl7pNiKuy6DcU1GQUwAXoIve6Bo8WVj1JmGO8ubSbkZI5rWGx0Ces/w\n\t24GK6JsibGeCgFF35PvvRY+7EXqoCwJkvaV+E+j2tsCoVlACZdPw/0Y/y9nLhJkvbu4Z\n\tvjBQ==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:date:from:to:cc:subject:message-id:references\n\t:mime-version:content-disposition:content-transfer-encoding\n\t:in-reply-to;\n\tbh=LNtpedjraXJB1uepWr0CdawoxPzmACZ0je/ehGDx1Yw=;\n\tb=ixM6RIyB6bvZboNoYfk3nQJCZ0s5WfrMl96ez7BlB4kdserCDbhjgMH6coqTO66HLB\n\tOao9jrrUdhZX9WJpyc1XKgOaMCSELIlqP+WrFMc4xJiDwiy0w6cdh8/5Bn4SvMypMycr\n\tAO7Zg6muZj+BPcOmZzNbTNt1/S6ZocH2EAZpEFGpVwnfSdl7/VG4rk0PST1BQTd2XKVl\n\tPwp0q8/f4dN3AVdlMru4l2LH3ekLKBL2imqfCNnfvkzeIxbooaoObsbU4Efqvbl3XhwF\n\toHVbkFW9asBLTsrniM/+r0mzhWdU9Ry7O83QbcBwmHkjyNRxp4wZR+DbXMbnzbE7mF6w\n\ta/Pw==","X-Gm-Message-State":"AOAM532luwNKj+MjkUn7ZDNF/hglr30b69kympkYZqJ9EqGDRCOXpnP3\n\tpQQ6oI9d3XX3eHtAB06wKS/6lkGB2jD5vQ==","X-Google-Smtp-Source":"ABdhPJzsLZ0MJlq1+fQ+Sq+VfjMSJyv2d/SmyrnYFR4++Q4Kq8v6HdNOFS1jhx4eXyDDY/rG7kqaFg==","X-Received":"by 2002:a19:44a:: with SMTP id 71mr772761lfe.403.1601465562491; \n\tWed, 30 Sep 2020 04:32:42 -0700 (PDT)","Date":"Wed, 30 Sep 2020 13:32:41 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"David Plowman ","Message-ID":"<20200930113241.GE1516931@oden.dyn.berto.se>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>\n\t<20200929223356.GC1271798@oden.dyn.berto.se>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"iso-8859-1\"","Content-Transfer-Encoding":"quoted-printable","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":12919,"web_url":"https://patchwork.libcamera.org/comment/12919/","msgid":"","date":"2020-10-01T14:00:30","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":42,"url":"https://patchwork.libcamera.org/api/people/42/","name":"David Plowman","email":"david.plowman@raspberrypi.com"},"content":"Hi Niklas\n\nThanks for your comments.\n\nOn Wed, 30 Sep 2020 at 12:32, Niklas Söderlund\n wrote:\n>\n> Hi David,\n>\n> On 2020-09-30 08:00:18 +0100, David Plowman wrote:\n> > Hi Niklas\n> >\n> > Thanks for the comments, and sorry for not addressing them sooner!\n> >\n> > On Tue, 29 Sep 2020 at 23:33, Niklas Söderlund\n> > wrote:\n> > >\n> > > Hi David,\n> > >\n> > > Thanks for your patch.\n> > >\n> > > On 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> > > > This allows the user to request digital zoom by adding, for example,\n> > > > \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> > > > the middle of the image.\n> > > >\n> > > > Signed-off-by: David Plowman \n> > >\n> > > If I understand the cover letter correctly this is just a rebase so I\n> >\n> > Correct, apart from a couple of minor mods (though one rather important one!)\n> >\n> > > understand not all comments from v2 are addressed. But just to reiterate\n> > > my comments from v2.\n> > >\n> > > > ---\n> > > > src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> > > > src/cam/capture.h | 2 +-\n> > > > src/cam/main.cpp | 3 +++\n> > > > src/cam/main.h | 1 +\n> > > > 4 files changed, 28 insertions(+), 3 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > > > index 5510c009..3b50c591 100644\n> > > > --- a/src/cam/capture.cpp\n> > > > +++ b/src/cam/capture.cpp\n> > > > @@ -10,6 +10,9 @@\n> > > > #include \n> > > > #include \n> > > >\n> > > > +#include \n> > > > +#include \n> > > > +\n> > > > #include \"capture.h\"\n> > > > #include \"main.h\"\n> > > >\n> > > > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > > >\n> > > > FrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > > >\n> > > > - ret = capture(allocator);\n> > > > + ret = capture(allocator, options);\n> > > >\n> > > > if (options.isSet(OptFile)) {\n> > > > delete writer_;\n> > > > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > > > return ret;\n> > > > }\n> > > >\n> > > > -int Capture::capture(FrameBufferAllocator *allocator)\n> > > > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> > > > {\n> > > > int ret;\n> > > >\n> > > > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > > > requests.push_back(request);\n> > > > }\n> > > >\n> > > > + /* Set up digital zoom if it was requested on the command line. */\n> > > > + if (options.isSet(OptZoom)) {\n> > > > + std::string zoom = options[OptZoom].toString();\n> > > > + float x, y, width, height;\n> > > > +\n> > > > + if (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n> > >\n> > > This is the wrong place to parse the input from the user. If the zoom\n> > > options are global something similar like StreamKeyValueParser\n> > > (ZoomKeyValueParser?) should be created. If it is to be moved a stream\n> > > level StreamKeyValueParser should be extended.\n> >\n> > Yes, I was kind of hoping not to get too involved with all that!\n>\n> :-)\n>\n> > How about this:\n> >\n> > 1. Add an OptionFloat type\n> >\n> > 2. Change the \"isArray\" option field to \"size\" or something, and make\n> > it an unsigned int. We'd make its default value 1, meaning \"not an\n> > array\" and edit any existing usage that sets it to False to use 1\n> > instead.\n> >\n> > 3. It seems to me that the current semantics for an \"array\" option is\n> > that they're variable length, i.e. it accepts as many as you type in.\n> > Perhaps we could use zero size for such an array, and otherwise insist\n> > that we get exactly the number that were wanted?\n> >\n> > 4. Then in my case I could supply a size of 4, and get an array of 4 floats.\n> >\n> > 5. It's true there is still some checking that needs to be done on the\n> > values to be sure they're sensible - my view here is that pipeline\n> > handlers should deal with garbage as applications can easily submit\n> > garbage, and here we have a way of checking that the pipeline handler\n> > actually does this correctly!\n>\n> I think cam is the wrong tool to test error injection into pipelines. I\n> also understand you wish to avoid hacking on the input parsers. I on the\n> other hand enjoy that work so I would be more then happy to supply you\n> with a patch you could squash into this patch.\n>\n> Only thing blocking me from writing one now is that I'm not sure this\n> should be a new --zoom option (ZoomKeyValueParser) or added to the\n> existing --stream option (extend StreamKeyValueParser). This is effect\n> what we are discussing below so I will postpone until we have resolved\n> this.\n\nWell, any help is always appreciated! As I said initially, I'm very\nhappy to drop this patch, and we could let another one replace it.\n>\n> >\n> > >\n> > > > + Size sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> > > > + Rectangle crop(x * sensorArea.width,\n> > > > + y * sensorArea.height,\n> > > > + width * sensorArea.width,\n> > > > + height * sensorArea.height);\n> > > > +\n> > > > + requests.front()->controls().set(controls::IspCrop, crop);\n> > >\n> > > Do we wish to expose the ISP in the public API? If we do is the ISP\n> > > (from an application point of view) the correct level to operate on?\n> > > Would not setting this on a stream level cover more hardware?\n> > >\n> > > I'm thinking of an ISP that can apply different crop parameters to two\n> > > (or more) source pads from a single sink pad. Having a control that sets\n> > > a global crop on the ISP would that not limit such hardware or am I\n> > > missing something?\n> >\n> > Indeed you're right, there is a bit of a dilemma here. There was some\n> > discussion of this a while back, I think with Jacopo, unfortunately I\n> > don't have any email references to hand.\n> >\n> > I think the feeling was that in all the real use cases that we can\n> > imagine, you generally want the same field of view on all your\n> > outputs. I think situations where something else would be useful are\n> > certainly less \"mainstream\".\n>\n> I'm sure there are use cases for different fields of view. I have seen\n> video conferencing gear that in the participant list zoom in on the\n> center of the screen while the speakers video is a larger FoV. I thought\n> this was Google HO but a quick test proves me wrong. In any case I'm\n> sure this is done in software without any support from the camera, but I\n> think there are use-cases for this.\n\nSo yes, I agree there are use cases, and indeed I've seen this kind of\nuse case before. But I've seen it done using software too, which kind\nof underlines the whole problem. Namely, are applications going to\nwrite different versions of code for underlying hardware with\ndifferent features? Unless the developer is targeting particular\nhardware known to have this feature they will try to avoid it.\n\nBut in any case, we have to expose what we regard as our minimum\nexpected hardware features, which would probably be a single zoom\ncontrol, and make that convenient to use.\n\n>\n> I also think the flavor of a zoom control is in the same area as a\n> rotation control. And rotation is something we sure wish to have per\n> stream. So I think no matter which controls go first we are reaching the\n> point we need to add support for per stream controls.\n\nAgreed, rotation (or transform) control is a case in point too. To be\nfair, we've followed a similar procedure there and allowed for a\nsingle per-camera transform (the CameraConfiguration has a field to\nrequest this transform). The question of per-stream transforms is\ntricky - many ISPs won't support it which limits its usefulness,\nothers may support them only at startup, others per-frame, and so that\nwhole question is deferred for the moment.\n\n>\n> >\n> > Secondly, some ISPs will let you crop outputs differently, and some\n> > won't. That's an awkward thing to handle - how would you know what the\n> > underlying hardware does, and then would you write different code for\n> > each case?\n>\n> Applications must be able to enumerate the features of the camera and\n> then enable or disable certain use-cases based on the enumerated\n> features. Of course this might lead to the applications rejecting a\n> camera as it don't support the features it needs (A raw capture\n> application would likely reject a camera that can't capture in a raw\n> format it knows how to process).\n>\n> > The pragmatic answer may just be to implement the \"usual\"\n> > case here, and it becomes a question for the future whether we get\n> > per-stream crop controls, or whether a StreamConfiguration gets crop\n> > rectangles...\n>\n> I think we need to implement both the usual and special cases, but we\n> need to do so in a user-friendly way. One way to do that is to provide\n> helpers for applications.\n>\n> I'm thinking as we know controls like digital zoom and rotation\n> depending on the hardware capabilities can apply to individual or all\n> streams the low-level API must allow for the most complex setup,\n> controls on the stream level. But as you say their are use-cases where\n> an application just wish to zoom or rotate all streams and then we could\n> add helpers for that, maybe something simple like this would work?\n>\n> Request::setControlAllStreams(Control, Value);\n>\n> Then applications who want to rotate or zoom all streams use such\n> helpers while special case applications will enumerate the camera\n> features and detect if it's possible to rotate or zoom on indivudual\n> streams and if so do it.\n\nWell, I'm happy to go with this approach too. For us, digital zoom is\nan important feature, so if we could come to some consensus that would\nbe very helpful!\n\nBest regards\nDavid\n\n>\n> As Jacopo points out, we know Laurent is working in this area so I'm\n> sure he has more comments or at least a different viewpoint then mine\n> ;-)\n>\n> >\n> > Maybe Jacopo can comment if I've got anything wrong there!\n> >\n> > Thanks again and best regards\n> > David\n> >\n> > >\n> > > > + } else {\n> > > > + std::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> > > > + }\n> > > > + }\n> > > > +\n> > > > ret = camera_->start();\n> > > > if (ret) {\n> > > > std::cout << \"Failed to start capture\" << std::endl;\n> > > > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > > > index 0aebdac9..52806164 100644\n> > > > --- a/src/cam/capture.h\n> > > > +++ b/src/cam/capture.h\n> > > > @@ -29,7 +29,7 @@ public:\n> > > >\n> > > > int run(const OptionsParser::Options &options);\n> > > > private:\n> > > > - int capture(libcamera::FrameBufferAllocator *allocator);\n> > > > + int capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> > > >\n> > > > void requestComplete(libcamera::Request *request);\n> > > >\n> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > > index 244720b4..b871d049 100644\n> > > > --- a/src/cam/main.cpp\n> > > > +++ b/src/cam/main.cpp\n> > > > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > > > parser.addOption(OptStrictFormats, OptionNone,\n> > > > \"Do not allow requested stream format(s) to be adjusted\",\n> > > > \"strict-formats\");\n> > > > + parser.addOption(OptZoom, OptionString,\n> > > > + \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> > > > + \"zoom\", ArgumentRequired, \"zoom\");\n> > > >\n> > > > options_ = parser.parse(argc, argv);\n> > > > if (!options_.valid())\n> > > > diff --git a/src/cam/main.h b/src/cam/main.h\n> > > > index ea8dfd33..a1fd899f 100644\n> > > > --- a/src/cam/main.h\n> > > > +++ b/src/cam/main.h\n> > > > @@ -17,6 +17,7 @@ enum {\n> > > > OptListProperties = 'p',\n> > > > OptMonitor = 'm',\n> > > > OptStream = 's',\n> > > > + OptZoom = 'z',\n> > > > OptListControls = 256,\n> > > > OptStrictFormats = 257,\n> > > > };\n> > > > --\n> > > > 2.20.1\n> > > >\n> > > > _______________________________________________\n> > > > libcamera-devel mailing list\n> > > > libcamera-devel@lists.libcamera.org\n> > > > https://lists.libcamera.org/listinfo/libcamera-devel\n> > >\n> > > --\n> > > Regards,\n> > > Niklas Söderlund\n>\n> --\n> Regards,\n> Niklas Söderlund","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 04A99C3B5C\n\tfor ;\n\tThu, 1 Oct 2020 14:00:46 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 7A00B622E7;\n\tThu, 1 Oct 2020 16:00:45 +0200 (CEST)","from mail-ot1-x32e.google.com (mail-ot1-x32e.google.com\n\t[IPv6:2607:f8b0:4864:20::32e])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 495736035F\n\tfor ;\n\tThu, 1 Oct 2020 16:00:44 +0200 (CEST)","by mail-ot1-x32e.google.com with SMTP id y5so5470585otg.5\n\tfor ;\n\tThu, 01 Oct 2020 07:00:44 -0700 (PDT)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=raspberrypi.com header.i=@raspberrypi.com\n\theader.b=\"th7ciYlJ\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=raspberrypi.com; s=google;\n\th=mime-version:references:in-reply-to:from:date:message-id:subject:to\n\t:cc:content-transfer-encoding;\n\tbh=YZ1moIsXTOVvpHrcIoU7bw/hqaxpsTgGpWj4lhS6Zpk=;\n\tb=th7ciYlJc9VPGda5GCY3KFEiBwELS7Vu7+dUNkb590nqk3i+jZspjZKnI9Wg4J8Z8P\n\t5LAzjLt1KpkypuwRb6f83kOC1E+F3+qRsBThY58wYOEkdUM7G6DLP3tG+dFRXYPPYlr4\n\tWGxDkerAPKD/TbjeNq4H/sYQPdHOf2+7liwM99dRGtwr+rNEwBLPIIBQomnXIR1ZFi9w\n\tjRhkg6gVXFQxV8YfYQfTf/IVC1Aw/lep9fs8Wnwq4XOVtNcIYdFCG0hrk7DbD6tr9dfC\n\tm1Pi86r3AwaTF78xuSTUsAaulANVXRl2b7hgcYASktn+xJXjcnzzCUT5u0wWraKEPJHs\n\twXZg==","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20161025;\n\th=x-gm-message-state:mime-version:references:in-reply-to:from:date\n\t:message-id:subject:to:cc:content-transfer-encoding;\n\tbh=YZ1moIsXTOVvpHrcIoU7bw/hqaxpsTgGpWj4lhS6Zpk=;\n\tb=Z8J8FuoEuBQ5V8ufAZ3MFmluK5RvjuSlq8+cFEb93Lb6P5uG8l/2OTVmtX8coAPB5X\n\tBWdDairhXpemiCHDwIBdx1nAOrJnQz9tVRqvi2JpILy5LUsIkRyHIvxIPM7VwBkTU99h\n\tvVNQaw8g+gAP07LG32a1aBD2VIfPl6CVj5hzrMIlJgAA2SPeA5Gng5o4nKo5yVBYJmXC\n\ttwjV15BDg2EWOlDE4Daz8q+jg9UzBclE7l3eb7WVhNTsGwdy365brRgY3TXbVlcBQv+C\n\tbmh2R4IYLV+uHt6Ewdak38/ZV20BqNmIkpaJcg5i6mKQzT4YQcZ16MI3aQrhuXePu+XL\n\tQ3hA==","X-Gm-Message-State":"AOAM533U+d5nQi5jZhSVlAz/MfJiDn0DD6eMfbtDZhfv8Uy2I04lAi16\n\tVAslBCORymJZY1GE9HxGGf7fy3Oegm4tF2evYxDCfQ==","X-Google-Smtp-Source":"ABdhPJxSrXjiPYZjEWzUwk8xRMRKyYD2D1x+GjX6Alwp45YkDhKZ4fPBOqNLDfe8DLmazIu0ELPmJBZp+bSAq+ruO6s=","X-Received":"by 2002:a9d:4c0a:: with SMTP id\n\tl10mr4952213otf.166.1601560842675; \n\tThu, 01 Oct 2020 07:00:42 -0700 (PDT)","MIME-Version":"1.0","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>\n\t<20200929223356.GC1271798@oden.dyn.berto.se>\n\t\n\t<20200930113241.GE1516931@oden.dyn.berto.se>","In-Reply-To":"<20200930113241.GE1516931@oden.dyn.berto.se>","From":"David Plowman ","Date":"Thu, 1 Oct 2020 15:00:30 +0100","Message-ID":"","To":"=?utf-8?q?Niklas_S=C3=B6derlund?= ","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":13161,"web_url":"https://patchwork.libcamera.org/comment/13161/","msgid":"<20201011025805.GG3939@pendragon.ideasonboard.com>","date":"2020-10-11T02:58:05","subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Niklas,\n\nOn Wed, Sep 30, 2020 at 12:33:56AM +0200, Niklas Söderlund wrote:\n> On 2020-09-29 17:40:00 +0100, David Plowman wrote:\n> > This allows the user to request digital zoom by adding, for example,\n> > \"-z 0.25,0.25,0.5,0.5\" which would give a 2x zoom, still centred in\n> > the middle of the image.\n> > \n> > Signed-off-by: David Plowman \n> \n> If I understand the cover letter correctly this is just a rebase so I \n> understand not all comments from v2 are addressed. But just to reiterate \n> my comments from v2.\n> \n> > ---\n> > src/cam/capture.cpp | 25 +++++++++++++++++++++++--\n> > src/cam/capture.h | 2 +-\n> > src/cam/main.cpp | 3 +++\n> > src/cam/main.h | 1 +\n> > 4 files changed, 28 insertions(+), 3 deletions(-)\n> > \n> > diff --git a/src/cam/capture.cpp b/src/cam/capture.cpp\n> > index 5510c009..3b50c591 100644\n> > --- a/src/cam/capture.cpp\n> > +++ b/src/cam/capture.cpp\n> > @@ -10,6 +10,9 @@\n> > #include \n> > #include \n> > \n> > +#include \n> > +#include \n> > +\n> > #include \"capture.h\"\n> > #include \"main.h\"\n> > \n> > @@ -58,7 +61,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > \n> > \tFrameBufferAllocator *allocator = new FrameBufferAllocator(camera_);\n> > \n> > -\tret = capture(allocator);\n> > +\tret = capture(allocator, options);\n> > \n> > \tif (options.isSet(OptFile)) {\n> > \t\tdelete writer_;\n> > @@ -70,7 +73,7 @@ int Capture::run(const OptionsParser::Options &options)\n> > \treturn ret;\n> > }\n> > \n> > -int Capture::capture(FrameBufferAllocator *allocator)\n> > +int Capture::capture(FrameBufferAllocator *allocator, const OptionsParser::Options &options)\n> > {\n> > \tint ret;\n> > \n> > @@ -120,6 +123,24 @@ int Capture::capture(FrameBufferAllocator *allocator)\n> > \t\trequests.push_back(request);\n> > \t}\n> > \n> > +\t/* Set up digital zoom if it was requested on the command line. */\n> > +\tif (options.isSet(OptZoom)) {\n> > +\t\tstd::string zoom = options[OptZoom].toString();\n> > +\t\tfloat x, y, width, height;\n> > +\n> > +\t\tif (sscanf(zoom.c_str(), \"%f,%f,%f,%f\", &x, &y, &width, &height) == 4) {\n> \n> This is the wrong place to parse the input from the user. If the zoom \n> options are global something similar like StreamKeyValueParser\n> (ZoomKeyValueParser?) should be created. If it is to be moved a stream \n> level StreamKeyValueParser should be extended.\n> \n> > +\t\t\tSize sensorArea = camera_->properties().get(properties::SensorOutputSize);\n> > +\t\t\tRectangle crop(x * sensorArea.width,\n> > +\t\t\t\t y * sensorArea.height,\n> > +\t\t\t\t width * sensorArea.width,\n> > +\t\t\t\t height * sensorArea.height);\n> > +\n> > +\t\t\trequests.front()->controls().set(controls::IspCrop, crop);\n> \n> Do we wish to expose the ISP in the public API? If we do is the ISP\n> (from an application point of view) the correct level to operate on?\n> Would not setting this on a stream level cover more hardware?\n> \n> I'm thinking of an ISP that can apply different crop parameters to two\n> (or more) source pads from a single sink pad. Having a control that sets\n> a global crop on the ISP would that not limit such hardware or am I\n> missing something?\n\n(Copied from a previous version, to centralize all discussions in this\nseries)\n\nThis is a very good question, and a very fundamental design decision we\nneed to make. I've started thinking that we need to define and document\nan explicit abstract pipeline model for cameras, striking the right\nbalance between flexibility and adaptability to different hardware\narchitectures, and ease of implementation for pipeline handlers and of\nuse for applications.\n\nIn this context, David mentioned in a previous version of this series\nthat, while different streams could indeed have different zoom levels,\nuse cases for this feature were not immediately apparent. We could thus\nleave this out of our pipeline model. If we ever want to support this\nfeature in the future, I think we will be able to do so with the help of\nper-stream controls in requests, using the same IspCrop control, so it\nappears we would have a way forward.\n\n> > +\t\t} else {\n> > +\t\t\tstd::cout << \"Invalid zoom values - ignoring\" << std::endl;\n> > +\t\t}\n> > +\t}\n> > +\n> > \tret = camera_->start();\n> > \tif (ret) {\n> > \t\tstd::cout << \"Failed to start capture\" << std::endl;\n> > diff --git a/src/cam/capture.h b/src/cam/capture.h\n> > index 0aebdac9..52806164 100644\n> > --- a/src/cam/capture.h\n> > +++ b/src/cam/capture.h\n> > @@ -29,7 +29,7 @@ public:\n> > \n> > \tint run(const OptionsParser::Options &options);\n> > private:\n> > -\tint capture(libcamera::FrameBufferAllocator *allocator);\n> > +\tint capture(libcamera::FrameBufferAllocator *allocator, const OptionsParser::Options &options);\n> > \n> > \tvoid requestComplete(libcamera::Request *request);\n> > \n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index 244720b4..b871d049 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -201,6 +201,9 @@ int CamApp::parseOptions(int argc, char *argv[])\n> > \tparser.addOption(OptStrictFormats, OptionNone,\n> > \t\t\t \"Do not allow requested stream format(s) to be adjusted\",\n> > \t\t\t \"strict-formats\");\n> > +\tparser.addOption(OptZoom, OptionString,\n> > +\t\t\t \"Zoom in on the image portion described by the x,y,width,height rectangle (fractions of the maximum FOV) e.g. 0.25,0.25,0.5,0.5\",\n> > +\t\t\t \"zoom\", ArgumentRequired, \"zoom\");\n> > \n> > \toptions_ = parser.parse(argc, argv);\n> > \tif (!options_.valid())\n> > diff --git a/src/cam/main.h b/src/cam/main.h\n> > index ea8dfd33..a1fd899f 100644\n> > --- a/src/cam/main.h\n> > +++ b/src/cam/main.h\n> > @@ -17,6 +17,7 @@ enum {\n> > \tOptListProperties = 'p',\n> > \tOptMonitor = 'm',\n> > \tOptStream = 's',\n> > +\tOptZoom = 'z',\n> > \tOptListControls = 256,\n> > \tOptStrictFormats = 257,\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 C7899BEEDF\n\tfor ;\n\tSun, 11 Oct 2020 02:58:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 496F060862;\n\tSun, 11 Oct 2020 04:58:52 +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 DE43A60358\n\tfor ;\n\tSun, 11 Oct 2020 04:58:50 +0200 (CEST)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 44A10528;\n\tSun, 11 Oct 2020 04:58:50 +0200 (CEST)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"o/Px0LUu\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1602385130;\n\tbh=oTob564gOf5Fw0ApdWqWZ/6qhteqlxq6ogdF2CTijnI=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=o/Px0LUuo3CEjuA/ix5IC2wmC/OOD5eCtH21j4FuF8nyp40p6G1/53RpD8BAE9iSm\n\tk13gB3IZTcKDXRTLZxMbOwgNniJ3G2CgPyst27yL8MXAUSK0pIFVa5+uMTEjjXrKFF\n\tZFgRqs7fNj7uz3NuNjkux67WfaS3feWheT185WQk=","Date":"Sun, 11 Oct 2020 05:58:05 +0300","From":"Laurent Pinchart ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20201011025805.GG3939@pendragon.ideasonboard.com>","References":"<20200929164000.15429-1-david.plowman@raspberrypi.com>\n\t<20200929164000.15429-7-david.plowman@raspberrypi.com>\n\t<20200929223356.GC1271798@oden.dyn.berto.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200929223356.GC1271798@oden.dyn.berto.se>","Subject":"Re: [libcamera-devel] [PATCH v3 6/6] cam: Add command line option\n\tto test IspCrop control","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera-devel@lists.libcamera.org","Content-Type":"text/plain; charset=\"utf-8\"","Content-Transfer-Encoding":"base64","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]