[{"id":15774,"web_url":"https://patchwork.libcamera.org/comment/15774/","msgid":"","date":"2021-03-20T20:55:07","subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nThank you for the patch.\n\nOn Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:\n> In preparation to register the Location property only if the firware\n> interface provides it, do not assume it is available and build the\n> camera name using the camera sensor model as a fallback.\n> \n> Signed-off-by: Jacopo Mondi \n> ---\n> src/cam/main.cpp | 32 ++++++++++++++++++++------------\n> 1 file changed, 20 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index e01be63a7058..c087cdd97332 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)\n> \tconst ControlList &props = camera->properties();\n> \tstd::string name;\n> \n> -\tswitch (props.get(properties::Location)) {\n> -\tcase properties::CameraLocationFront:\n> -\t\tname = \"Internal front camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationBack:\n> -\t\tname = \"Internal back camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationExternal:\n> -\t\tname = \"External camera\";\n> -\t\tif (props.contains(properties::Model))\n> -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> -\t\tbreak;\n> +\tif (props.contains(properties::Location)) {\n> +\t\tswitch (props.get(properties::Location)) {\n> +\t\tcase properties::CameraLocationFront:\n> +\t\t\tname = \"Internal front camera\";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationBack:\n> +\t\t\tname = \"Internal back camera\";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationExternal:\n> +\t\t\tname = \"External camera\";\n> +\t\t\tif (props.contains(properties::Model))\n> +\t\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t} else if (props.contains(properties::Model)) {\n> +\t\t/*\n> +\t\t * If the camera location is not availble use the camera model\n> +\t\t * to build the camera name.\n> +\t\t */\n> +\t\tname = \"'\" + props.get(properties::Model) + \"'\";\n> \t}\n> \n> \tname += \" (\" + camera->id() + \")\";\n\nIf none of the conditions above is true, there will be an extra space.\nI initially thought utils::join() could be useful here, but that's not a\npublic API :-S This could be an option, I'm sure there are other equally\nvalid or better solutions.\n\ndiff --git a/src/cam/main.cpp b/src/cam/main.cpp\nindex e01be63a7058..3adfe2c34969 100644\n--- a/src/cam/main.cpp\n+++ b/src/cam/main.cpp\n@@ -377,23 +377,34 @@ int CamApp::run()\n std::string const CamApp::cameraName(const Camera *camera)\n {\n \tconst ControlList &props = camera->properties();\n+\tbool addModel = true;\n \tstd::string name;\n\n-\tswitch (props.get(properties::Location)) {\n-\tcase properties::CameraLocationFront:\n-\t\tname = \"Internal front camera\";\n-\t\tbreak;\n-\tcase properties::CameraLocationBack:\n-\t\tname = \"Internal back camera\";\n-\t\tbreak;\n-\tcase properties::CameraLocationExternal:\n-\t\tname = \"External camera\";\n-\t\tif (props.contains(properties::Model))\n-\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n-\t\tbreak;\n+\t/*\n+\t * Construct the name from the camera location, model and ID. The model\n+\t * is only used if the location isn't present or is set to External.\n+\t */\n+\n+\tif (props.contains(properties::Location)) {\n+\t\tswitch (props.get(properties::Location)) {\n+\t\tcase properties::CameraLocationFront:\n+\t\t\tname = \"Internal front camera \";\n+\t\t\taddModel = false;\n+\t\t\tbreak;\n+\t\tcase properties::CameraLocationBack:\n+\t\t\tname = \"Internal back camera \";\n+\t\t\taddModel = false;\n+\t\t\tbreak;\n+\t\tcase properties::CameraLocationExternal:\n+\t\t\tname = \"External camera \";\n+\t\t\tbreak;\n+\t\t}\n \t}\n\n-\tname += \" (\" + camera->id() + \")\";\n+\tif (addModel && props.contains(properties::Model))\n+\t\tname += \"'\" + props.get(properties::Model) + \"' \";\n+\n+\tname += \"(\" + camera->id() + \")\";\n\n \treturn name;\n }\n\nIt's a detail, and regardless of what option you pick,\n\nReviewed-by: Laurent Pinchart ","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 31C3DBD80C\n\tfor ;\n\tSat, 20 Mar 2021 20:55:49 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9387568D60;\n\tSat, 20 Mar 2021 21:55:48 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B27FC602DB\n\tfor ;\n\tSat, 20 Mar 2021 21:55:47 +0100 (CET)","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 2382D8D3;\n\tSat, 20 Mar 2021 21:55:47 +0100 (CET)"],"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=\"A0KMfHij\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616273747;\n\tbh=ZngJG7Bj8TGss2ZKunbv7Y9IeXM3NHWZfYqIl9MWjmg=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=A0KMfHijPlQBv/egax6O+LkplhOEctutZda1BfQ7YkeqI3l9ift8KLNt9VQgYrxxm\n\tK9/fQiS0HTfps7hLnMGE3JSIASHDm/o68qkwCvnB0QVkHLcdQRdT0mBTnclAKznbMc\n\tNsu8E9OtCyBtfLFpQRP2RcLVlITIq/R6JyjrhFsc=","Date":"Sat, 20 Mar 2021 22:55:07 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210319130120.141563-1-jacopo@jmondi.org>\n\t<20210319130120.141563-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210319130120.141563-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15776,"web_url":"https://patchwork.libcamera.org/comment/15776/","msgid":"<20210321133622.sj43ga4yepltfofk@uno.localdomain>","date":"2021-03-21T13:36:22","subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> Thank you for the patch.\n>\n> On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:\n> > In preparation to register the Location property only if the firware\n> > interface provides it, do not assume it is available and build the\n> > camera name using the camera sensor model as a fallback.\n> >\n> > Signed-off-by: Jacopo Mondi \n> > ---\n> > src/cam/main.cpp | 32 ++++++++++++++++++++------------\n> > 1 file changed, 20 insertions(+), 12 deletions(-)\n> >\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index e01be63a7058..c087cdd97332 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)\n> > \tconst ControlList &props = camera->properties();\n> > \tstd::string name;\n> >\n> > -\tswitch (props.get(properties::Location)) {\n> > -\tcase properties::CameraLocationFront:\n> > -\t\tname = \"Internal front camera\";\n> > -\t\tbreak;\n> > -\tcase properties::CameraLocationBack:\n> > -\t\tname = \"Internal back camera\";\n> > -\t\tbreak;\n> > -\tcase properties::CameraLocationExternal:\n> > -\t\tname = \"External camera\";\n> > -\t\tif (props.contains(properties::Model))\n> > -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > -\t\tbreak;\n> > +\tif (props.contains(properties::Location)) {\n> > +\t\tswitch (props.get(properties::Location)) {\n> > +\t\tcase properties::CameraLocationFront:\n> > +\t\t\tname = \"Internal front camera\";\n> > +\t\t\tbreak;\n> > +\t\tcase properties::CameraLocationBack:\n> > +\t\t\tname = \"Internal back camera\";\n> > +\t\t\tbreak;\n> > +\t\tcase properties::CameraLocationExternal:\n> > +\t\t\tname = \"External camera\";\n> > +\t\t\tif (props.contains(properties::Model))\n> > +\t\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > +\t\t\tbreak;\n> > +\t\t}\n> > +\t} else if (props.contains(properties::Model)) {\n> > +\t\t/*\n> > +\t\t * If the camera location is not availble use the camera model\n> > +\t\t * to build the camera name.\n> > +\t\t */\n> > +\t\tname = \"'\" + props.get(properties::Model) + \"'\";\n> > \t}\n> >\n> > \tname += \" (\" + camera->id() + \")\";\n>\n> If none of the conditions above is true, there will be an extra space.\n\nIsn't the Model property always registered by the CameraSensor class ?\n\n> I initially thought utils::join() could be useful here, but that's not a\n> public API :-S This could be an option, I'm sure there are other equally\n> valid or better solutions.\n>\n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index e01be63a7058..3adfe2c34969 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -377,23 +377,34 @@ int CamApp::run()\n> std::string const CamApp::cameraName(const Camera *camera)\n> {\n> \tconst ControlList &props = camera->properties();\n> +\tbool addModel = true;\n> \tstd::string name;\n>\n> -\tswitch (props.get(properties::Location)) {\n> -\tcase properties::CameraLocationFront:\n> -\t\tname = \"Internal front camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationBack:\n> -\t\tname = \"Internal back camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationExternal:\n> -\t\tname = \"External camera\";\n> -\t\tif (props.contains(properties::Model))\n> -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> -\t\tbreak;\n> +\t/*\n> +\t * Construct the name from the camera location, model and ID. The model\n> +\t * is only used if the location isn't present or is set to External.\n> +\t */\n> +\n> +\tif (props.contains(properties::Location)) {\n> +\t\tswitch (props.get(properties::Location)) {\n> +\t\tcase properties::CameraLocationFront:\n> +\t\t\tname = \"Internal front camera \";\n> +\t\t\taddModel = false;\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationBack:2\n> +\t\t\tname = \"Internal back camera \";\n> +\t\t\taddModel = false;\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationExternal:\n> +\t\t\tname = \"External camera \";\n> +\t\t\tbreak;\n> +\t\t}\n> \t}\n>\n> -\tname += \" (\" + camera->id() + \")\";\n> +\tif (addModel && props.contains(properties::Model))\n> +\t\tname += \"'\" + props.get(properties::Model) + \"' \";\n> +\n> +\tname += \"(\" + camera->id() + \")\";\n>\n> \treturn name;\n> }\n>\n> It's a detail, and regardless of what option you pick,\n\nAnyway, using an addModel flag would make the code more readable so\nI'll refactor.\n\n>\n> Reviewed-by: Laurent Pinchart \n>\n\nThanks\n j\n\n> --\n> Regards,\n>\n> Laurent Pinchart","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 2356CBD80C\n\tfor ;\n\tSun, 21 Mar 2021 13:35:52 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8070368D60;\n\tSun, 21 Mar 2021 14:35:51 +0100 (CET)","from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net\n\t[217.70.183.198])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C74EA6084F\n\tfor ;\n\tSun, 21 Mar 2021 14:35:49 +0100 (CET)","from uno.localdomain (host-82-63-7-72.business.telecomitalia.it\n\t[82.63.7.72]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay6-d.mail.gandi.net (Postfix) with ESMTPSA id E6A4CC0002;\n\tSun, 21 Mar 2021 13:35:48 +0000 (UTC)"],"X-Originating-IP":"82.63.7.72","Date":"Sun, 21 Mar 2021 14:36:22 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210321133622.sj43ga4yepltfofk@uno.localdomain>","References":"<20210319130120.141563-1-jacopo@jmondi.org>\n\t<20210319130120.141563-2-jacopo@jmondi.org>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15777,"web_url":"https://patchwork.libcamera.org/comment/15777/","msgid":"","date":"2021-03-21T13:47:26","subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Sun, Mar 21, 2021 at 02:36:22PM +0100, Jacopo Mondi wrote:\n> On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote:\n> > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:\n> > > In preparation to register the Location property only if the firware\n> > > interface provides it, do not assume it is available and build the\n> > > camera name using the camera sensor model as a fallback.\n> > >\n> > > Signed-off-by: Jacopo Mondi \n> > > ---\n> > > src/cam/main.cpp | 32 ++++++++++++++++++++------------\n> > > 1 file changed, 20 insertions(+), 12 deletions(-)\n> > >\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index e01be63a7058..c087cdd97332 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)\n> > > \tconst ControlList &props = camera->properties();\n> > > \tstd::string name;\n> > >\n> > > -\tswitch (props.get(properties::Location)) {\n> > > -\tcase properties::CameraLocationFront:\n> > > -\t\tname = \"Internal front camera\";\n> > > -\t\tbreak;\n> > > -\tcase properties::CameraLocationBack:\n> > > -\t\tname = \"Internal back camera\";\n> > > -\t\tbreak;\n> > > -\tcase properties::CameraLocationExternal:\n> > > -\t\tname = \"External camera\";\n> > > -\t\tif (props.contains(properties::Model))\n> > > -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > > -\t\tbreak;\n> > > +\tif (props.contains(properties::Location)) {\n> > > +\t\tswitch (props.get(properties::Location)) {\n> > > +\t\tcase properties::CameraLocationFront:\n> > > +\t\t\tname = \"Internal front camera\";\n> > > +\t\t\tbreak;\n> > > +\t\tcase properties::CameraLocationBack:\n> > > +\t\t\tname = \"Internal back camera\";\n> > > +\t\t\tbreak;\n> > > +\t\tcase properties::CameraLocationExternal:\n> > > +\t\t\tname = \"External camera\";\n> > > +\t\t\tif (props.contains(properties::Model))\n> > > +\t\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > +\t} else if (props.contains(properties::Model)) {\n> > > +\t\t/*\n> > > +\t\t * If the camera location is not availble use the camera model\n> > > +\t\t * to build the camera name.\n> > > +\t\t */\n> > > +\t\tname = \"'\" + props.get(properties::Model) + \"'\";\n> > > \t}\n> > >\n> > > \tname += \" (\" + camera->id() + \")\";\n> >\n> > If none of the conditions above is true, there will be an extra space.\n> \n> Isn't the Model property always registered by the CameraSensor class ?\n\nGood question. I have assumed it isn't, as you call\nprops.contains(Properties::Model) :-) I may have been wrong. Note that\nit's not just about the CameraSensor class, as not all pipeline handlers\nuse it (most notably the UVC pipeline handler). What matters is if the\nproperty is documented as required or not.\n\n> > I initially thought utils::join() could be useful here, but that's not a\n> > public API :-S This could be an option, I'm sure there are other equally\n> > valid or better solutions.\n> >\n> > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > index e01be63a7058..3adfe2c34969 100644\n> > --- a/src/cam/main.cpp\n> > +++ b/src/cam/main.cpp\n> > @@ -377,23 +377,34 @@ int CamApp::run()\n> > std::string const CamApp::cameraName(const Camera *camera)\n> > {\n> > \tconst ControlList &props = camera->properties();\n> > +\tbool addModel = true;\n> > \tstd::string name;\n> >\n> > -\tswitch (props.get(properties::Location)) {\n> > -\tcase properties::CameraLocationFront:\n> > -\t\tname = \"Internal front camera\";\n> > -\t\tbreak;\n> > -\tcase properties::CameraLocationBack:\n> > -\t\tname = \"Internal back camera\";\n> > -\t\tbreak;\n> > -\tcase properties::CameraLocationExternal:\n> > -\t\tname = \"External camera\";\n> > -\t\tif (props.contains(properties::Model))\n> > -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > -\t\tbreak;\n> > +\t/*\n> > +\t * Construct the name from the camera location, model and ID. The model\n> > +\t * is only used if the location isn't present or is set to External.\n> > +\t */\n> > +\n> > +\tif (props.contains(properties::Location)) {\n> > +\t\tswitch (props.get(properties::Location)) {\n> > +\t\tcase properties::CameraLocationFront:\n> > +\t\t\tname = \"Internal front camera \";\n> > +\t\t\taddModel = false;\n> > +\t\t\tbreak;\n> > +\t\tcase properties::CameraLocationBack:2\n> > +\t\t\tname = \"Internal back camera \";\n> > +\t\t\taddModel = false;\n> > +\t\t\tbreak;\n> > +\t\tcase properties::CameraLocationExternal:\n> > +\t\t\tname = \"External camera \";\n> > +\t\t\tbreak;\n> > +\t\t}\n> > \t}\n> >\n> > -\tname += \" (\" + camera->id() + \")\";\n> > +\tif (addModel && props.contains(properties::Model))\n> > +\t\tname += \"'\" + props.get(properties::Model) + \"' \";\n> > +\n> > +\tname += \"(\" + camera->id() + \")\";\n> >\n> > \treturn name;\n> > }\n> >\n> > It's a detail, and regardless of what option you pick,\n> \n> Anyway, using an addModel flag would make the code more readable so\n> I'll refactor.\n> \n> >\n> > Reviewed-by: Laurent Pinchart ","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 4D29AC32E1\n\tfor ;\n\tSun, 21 Mar 2021 13:48:09 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9D89968D60;\n\tSun, 21 Mar 2021 14:48:08 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E4DB76084F\n\tfor ;\n\tSun, 21 Mar 2021 14:48:06 +0100 (CET)","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 53F5051D;\n\tSun, 21 Mar 2021 14:48:06 +0100 (CET)"],"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=\"X3S6gG8B\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616334486;\n\tbh=P0xpTM1bmukJsj8gO5VSv7UAzd/OnN6Cut1hRL8y8gw=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=X3S6gG8BrJuxQpHAhUrKApbMH78kYwWH6dO/s3apP5ki8RGm0MaeDa+zK9AHmuKbM\n\t1Vbh4jJgx7DMXRl9ZFEfVouTaccufwDZDqb6tgGmLQQEyxwhOH7/IIzGFZidDz4Hd1\n\tigRhfbMUvTGM+yzI1Z8KqrRuiuzRodsG5TsMEa9c=","Date":"Sun, 21 Mar 2021 15:47:26 +0200","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Message-ID":"","References":"<20210319130120.141563-1-jacopo@jmondi.org>\n\t<20210319130120.141563-2-jacopo@jmondi.org>\n\t\n\t<20210321133622.sj43ga4yepltfofk@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210321133622.sj43ga4yepltfofk@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15778,"web_url":"https://patchwork.libcamera.org/comment/15778/","msgid":"<20210321140952.3naxn6rbqw2ep6l2@uno.localdomain>","date":"2021-03-21T14:09:52","subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent,\n\nOn Sun, Mar 21, 2021 at 03:47:26PM +0200, Laurent Pinchart wrote:\n> Hi Jacopo,\n>\n> On Sun, Mar 21, 2021 at 02:36:22PM +0100, Jacopo Mondi wrote:\n> > On Sat, Mar 20, 2021 at 10:55:07PM +0200, Laurent Pinchart wrote:\n> > > On Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:\n> > > > In preparation to register the Location property only if the firware\n> > > > interface provides it, do not assume it is available and build the\n> > > > camera name using the camera sensor model as a fallback.\n> > > >\n> > > > Signed-off-by: Jacopo Mondi \n> > > > ---\n> > > > src/cam/main.cpp | 32 ++++++++++++++++++++------------\n> > > > 1 file changed, 20 insertions(+), 12 deletions(-)\n> > > >\n> > > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > > index e01be63a7058..c087cdd97332 100644\n> > > > --- a/src/cam/main.cpp\n> > > > +++ b/src/cam/main.cpp\n> > > > @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)\n> > > > \tconst ControlList &props = camera->properties();\n> > > > \tstd::string name;\n> > > >\n> > > > -\tswitch (props.get(properties::Location)) {\n> > > > -\tcase properties::CameraLocationFront:\n> > > > -\t\tname = \"Internal front camera\";\n> > > > -\t\tbreak;\n> > > > -\tcase properties::CameraLocationBack:\n> > > > -\t\tname = \"Internal back camera\";\n> > > > -\t\tbreak;\n> > > > -\tcase properties::CameraLocationExternal:\n> > > > -\t\tname = \"External camera\";\n> > > > -\t\tif (props.contains(properties::Model))\n> > > > -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > > > -\t\tbreak;\n> > > > +\tif (props.contains(properties::Location)) {\n> > > > +\t\tswitch (props.get(properties::Location)) {\n> > > > +\t\tcase properties::CameraLocationFront:\n> > > > +\t\t\tname = \"Internal front camera\";\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase properties::CameraLocationBack:\n> > > > +\t\t\tname = \"Internal back camera\";\n> > > > +\t\t\tbreak;\n> > > > +\t\tcase properties::CameraLocationExternal:\n> > > > +\t\t\tname = \"External camera\";\n> > > > +\t\t\tif (props.contains(properties::Model))\n> > > > +\t\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > > > +\t\t\tbreak;\n> > > > +\t\t}\n> > > > +\t} else if (props.contains(properties::Model)) {\n> > > > +\t\t/*\n> > > > +\t\t * If the camera location is not availble use the camera model\n> > > > +\t\t * to build the camera name.\n> > > > +\t\t */\n> > > > +\t\tname = \"'\" + props.get(properties::Model) + \"'\";\n> > > > \t}\n> > > >\n> > > > \tname += \" (\" + camera->id() + \")\";\n> > >\n> > > If none of the conditions above is true, there will be an extra space.\n> >\n> > Isn't the Model property always registered by the CameraSensor class ?\n>\n> Good question. I have assumed it isn't, as you call\n> props.contains(Properties::Model) :-) I may have been wrong. Note that\n\nAnd I call props.contains() as it was there already :)\n\n> it's not just about the CameraSensor class, as not all pipeline handlers\n> use it (most notably the UVC pipeline handler). What matters is if the\n> property is documented as required or not.\n>\n\nThe current implementation of the UVC pipeline handler registers\nModel, but as it's not documented as mandatory I woudl stay safe and\ncontinue checking for its presence.\n\nI'll take your suggestion in and push!\n\nThanks\n j\n\n\n> > > I initially thought utils::join() could be useful here, but that's not a\n> > > public API :-S This could be an option, I'm sure there are other equally\n> > > valid or better solutions.\n> > >\n> > > diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> > > index e01be63a7058..3adfe2c34969 100644\n> > > --- a/src/cam/main.cpp\n> > > +++ b/src/cam/main.cpp\n> > > @@ -377,23 +377,34 @@ int CamApp::run()\n> > > std::string const CamApp::cameraName(const Camera *camera)\n> > > {\n> > > \tconst ControlList &props = camera->properties();\n> > > +\tbool addModel = true;\n> > > \tstd::string name;\n> > >\n> > > -\tswitch (props.get(properties::Location)) {\n> > > -\tcase properties::CameraLocationFront:\n> > > -\t\tname = \"Internal front camera\";\n> > > -\t\tbreak;\n> > > -\tcase properties::CameraLocationBack:\n> > > -\t\tname = \"Internal back camera\";\n> > > -\t\tbreak;\n> > > -\tcase properties::CameraLocationExternal:\n> > > -\t\tname = \"External camera\";\n> > > -\t\tif (props.contains(properties::Model))\n> > > -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> > > -\t\tbreak;\n> > > +\t/*\n> > > +\t * Construct the name from the camera location, model and ID. The model\n> > > +\t * is only used if the location isn't present or is set to External.\n> > > +\t */\n> > > +\n> > > +\tif (props.contains(properties::Location)) {\n> > > +\t\tswitch (props.get(properties::Location)) {\n> > > +\t\tcase properties::CameraLocationFront:\n> > > +\t\t\tname = \"Internal front camera \";\n> > > +\t\t\taddModel = false;\n> > > +\t\t\tbreak;\n> > > +\t\tcase properties::CameraLocationBack:2\n> > > +\t\t\tname = \"Internal back camera \";\n> > > +\t\t\taddModel = false;\n> > > +\t\t\tbreak;\n> > > +\t\tcase properties::CameraLocationExternal:\n> > > +\t\t\tname = \"External camera \";\n> > > +\t\t\tbreak;\n> > > +\t\t}\n> > > \t}\n> > >\n> > > -\tname += \" (\" + camera->id() + \")\";\n> > > +\tif (addModel && props.contains(properties::Model))\n> > > +\t\tname += \"'\" + props.get(properties::Model) + \"' \";\n> > > +\n> > > +\tname += \"(\" + camera->id() + \")\";\n> > >\n> > > \treturn name;\n> > > }\n> > >\n> > > It's a detail, and regardless of what option you pick,\n> >\n> > Anyway, using an addModel flag would make the code more readable so\n> > I'll refactor.\n> >\n> > >\n> > > Reviewed-by: Laurent Pinchart \n>\n> --\n> Regards,\n>\n> Laurent Pinchart","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 2A01ABD80C\n\tfor ;\n\tSun, 21 Mar 2021 14:09:23 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8786068D54;\n\tSun, 21 Mar 2021 15:09:22 +0100 (CET)","from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net\n\t[217.70.183.194])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 3547B6084F\n\tfor ;\n\tSun, 21 Mar 2021 15:09:21 +0100 (CET)","from uno.localdomain (host-82-63-7-72.business.telecomitalia.it\n\t[82.63.7.72]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay2-d.mail.gandi.net (Postfix) with ESMTPSA id 43F4A40006;\n\tSun, 21 Mar 2021 14:09:19 +0000 (UTC)"],"X-Originating-IP":"82.63.7.72","Date":"Sun, 21 Mar 2021 15:09:52 +0100","From":"Jacopo Mondi ","To":"Laurent Pinchart ","Message-ID":"<20210321140952.3naxn6rbqw2ep6l2@uno.localdomain>","References":"<20210319130120.141563-1-jacopo@jmondi.org>\n\t<20210319130120.141563-2-jacopo@jmondi.org>\n\t\n\t<20210321133622.sj43ga4yepltfofk@uno.localdomain>\n\t","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15779,"web_url":"https://patchwork.libcamera.org/comment/15779/","msgid":"<20210322054712.GA3413@pyrite.rasen.tech>","date":"2021-03-22T05:47:12","subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","submitter":{"id":17,"url":"https://patchwork.libcamera.org/api/people/17/","name":"Paul Elder","email":"paul.elder@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Fri, Mar 19, 2021 at 02:01:18PM +0100, Jacopo Mondi wrote:\n> In preparation to register the Location property only if the firware\n> interface provides it, do not assume it is available and build the\n> camera name using the camera sensor model as a fallback.\n> \n> Signed-off-by: Jacopo Mondi \n\nReviewed-by: Paul Elder \n\n> ---\n> src/cam/main.cpp | 32 ++++++++++++++++++++------------\n> 1 file changed, 20 insertions(+), 12 deletions(-)\n> \n> diff --git a/src/cam/main.cpp b/src/cam/main.cpp\n> index e01be63a7058..c087cdd97332 100644\n> --- a/src/cam/main.cpp\n> +++ b/src/cam/main.cpp\n> @@ -379,18 +379,26 @@ std::string const CamApp::cameraName(const Camera *camera)\n> \tconst ControlList &props = camera->properties();\n> \tstd::string name;\n> \n> -\tswitch (props.get(properties::Location)) {\n> -\tcase properties::CameraLocationFront:\n> -\t\tname = \"Internal front camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationBack:\n> -\t\tname = \"Internal back camera\";\n> -\t\tbreak;\n> -\tcase properties::CameraLocationExternal:\n> -\t\tname = \"External camera\";\n> -\t\tif (props.contains(properties::Model))\n> -\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> -\t\tbreak;\n> +\tif (props.contains(properties::Location)) {\n> +\t\tswitch (props.get(properties::Location)) {\n> +\t\tcase properties::CameraLocationFront:\n> +\t\t\tname = \"Internal front camera\";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationBack:\n> +\t\t\tname = \"Internal back camera\";\n> +\t\t\tbreak;\n> +\t\tcase properties::CameraLocationExternal:\n> +\t\t\tname = \"External camera\";\n> +\t\t\tif (props.contains(properties::Model))\n> +\t\t\t\tname += \" '\" + props.get(properties::Model) + \"'\";\n> +\t\t\tbreak;\n> +\t\t}\n> +\t} else if (props.contains(properties::Model)) {\n> +\t\t/*\n> +\t\t * If the camera location is not availble use the camera model\n> +\t\t * to build the camera name.\n> +\t\t */\n> +\t\tname = \"'\" + props.get(properties::Model) + \"'\";\n> \t}\n> \n> \tname += \" (\" + camera->id() + \")\";\n> -- \n> 2.30.0\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 D673AC32E1\n\tfor ;\n\tMon, 22 Mar 2021 05:47:33 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 3892968D58;\n\tMon, 22 Mar 2021 06:47:33 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C3801602D7\n\tfor ;\n\tMon, 22 Mar 2021 06:47:20 +0100 (CET)","from pyrite.rasen.tech (unknown\n\t[IPv6:2400:4051:61:600:2c71:1b79:d06d:5032])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 1E8028CE;\n\tMon, 22 Mar 2021 06:47:18 +0100 (CET)"],"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=\"QSfKuPmC\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1616392040;\n\tbh=d9sKKCLJRbm5rGMF/cicJje6SZFFGy5RHq9IYE/jW+U=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=QSfKuPmCEp9H31L1kLQ6zbI9DVamKDE2reHNFmxZmIEYnxBV1RajWLrHxJ9agnfyI\n\tSdTiAdYfP+cwZABXQPCM2f8qZEWFOPJAH7E3gf58PTQygRxRSP02otDzcg2Aj5Bl9y\n\typm5fXDvl0G02E14Xp/Wsb48DdmmnSx8Le5T0DXw=","Date":"Mon, 22 Mar 2021 14:47:12 +0900","From":"paul.elder@ideasonboard.com","To":"Jacopo Mondi ","Message-ID":"<20210322054712.GA3413@pyrite.rasen.tech>","References":"<20210319130120.141563-1-jacopo@jmondi.org>\n\t<20210319130120.141563-2-jacopo@jmondi.org>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210319130120.141563-2-jacopo@jmondi.org>","Subject":"Re: [libcamera-devel] [PATCH v2 1/3] cam: Do not assume Location is\n\tavailable","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=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]