[{"id":11688,"web_url":"https://patchwork.libcamera.org/comment/11688/","msgid":"<20200729081459.wrdlfexa3a5lx7np@uno.localdomain>","date":"2020-07-29T08:14:59","subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: pipeline: uvcvideo:\n\tGenerate unique camera names","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n\nOn Wed, Jul 29, 2020 at 01:42:24AM +0200, Niklas Söderlund wrote:\n> Generate camera names that are unique and persistent between system\n> resets. The name is constructed from the USB vendor and product\n> information that is stored on USB device itself and the USB bus and\n> device numbers where the hardware is plugged in.\n>\n> Before this change example of camera names:\n>\n> Venus USB2.0 Camera: Venus USB2\n> Logitech Webcam C930e\n>\n> After this change the same cameras are:\n>\n> 0ac8:3420:3:10\n> 046d:0843:3:4\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++-\n> 1 file changed, 33 insertions(+), 1 deletion(-)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 93e3dc17e3a7105e..72f3f5d9f7c414d0 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -6,6 +6,7 @@\n> */\n>\n> #include \n> +#include \n> #include \n> #include \n> #include \n> @@ -81,6 +82,9 @@ public:\n> \tbool match(DeviceEnumerator *enumerator) override;\n>\n> private:\n> +\tstd::string generateName(const MediaDevice *media,\n> +\t\t\t\t const std::string path);\n> +\n> \tint processControl(ControlList *controls, unsigned int id,\n> \t\t\t const ControlValue &value);\n> \tint processControls(UVCCameraData *data, Request *request);\n> @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> \treturn 0;\n> }\n>\n> +std::string PipelineHandlerUVC::generateName(const MediaDevice *media,\n\nIs media used ?\n\n> +\t\t\t\t\t const std::string path)\n\nI would have passed the CameraData instance and got the path from\nthere.\n\n> +{\n> +\tstatic const std::vector files = { \"idVendor\", \"idProduct\", \"busnum\", \"devnum\" };\n\nless than 120 cols ?\n\n> +\tstd::vector values;\n> +\tstd::string value;\n> +\n> +\tfor (const std::string &name : files) {\n> +\t\tstd::ifstream file(path + \"/../\" + name);\n> +\n> +\t\tif (!file.is_open())\n> +\t\t\treturn \"\";\n\nReturn or just skip that file ? If you want to return is this worth an\nerror message ?\n> +\n> +\t\tstd::getline(file, value);\n> +\t\tfile.close();\n\nThis applies to the other patches as well, reading\nstd::ifstream::close documentation I see:\n\n\"Note that any open file is automatically closed when the ifstream object is destroyed.\"\n\nDo you need to call close ? I got suspicious as it felt weird that an\nSTL library opens a file on construction but requires manual closure..\n\n> +\n> +\t\tvalues.push_back(value);\n> +\t}\n> +\n> +\treturn utils::join(values, \":\");\n> +}\n> +\n> bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> {\n> \tMediaDevice *media;\n> @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> \t\treturn false;\n>\n> \t/* Create and register the camera. */\n> +\tstd::string name = generateName(media, data->video_->devicePath());\n> +\tif (name.empty()) {\n> +\t\tLOG(UVC, Error) << \"Failed to generate camera name\";\n> +\t\treturn false;\n> +\t}\n\nOh the error is here, and is better here than in the function. But\nthen you have to fail if you don't find one of the files... I think\nwhat you have here is right then...\n\n> +\n> \tstd::set streams{ &data->stream_ };\n> -\tstd::shared_ptr camera = Camera::create(this, media->model(), streams);\n> +\tstd::shared_ptr camera = Camera::create(this, name, streams);\n\nnits apart\nReviewed-by: Jacopo Mondi \n\nThanks\n j\n\n> \tregisterCamera(std::move(camera), std::move(data));\n>\n> \t/* Enable hot-unplug notifications. */\n> --\n> 2.27.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 71C09BD86F\n\tfor ;\n\tWed, 29 Jul 2020 08:11:21 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 11F7461176;\n\tWed, 29 Jul 2020 10:11:21 +0200 (CEST)","from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 873AA605B2\n\tfor ;\n\tWed, 29 Jul 2020 10:11:19 +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 relay11.mail.gandi.net (Postfix) with ESMTPSA id D9836100009;\n\tWed, 29 Jul 2020 08:11:18 +0000 (UTC)"],"Date":"Wed, 29 Jul 2020 10:14:59 +0200","From":"Jacopo Mondi ","To":"Niklas =?utf-8?q?S=C3=B6derlund?= ","Message-ID":"<20200729081459.wrdlfexa3a5lx7np@uno.localdomain>","References":"<20200728233744.3503740-1-niklas.soderlund@ragnatech.se>\n\t<20200728234225.3505868-1-niklas.soderlund@ragnatech.se>\n\t<20200728234225.3505868-4-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200728234225.3505868-4-niklas.soderlund@ragnatech.se>","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: pipeline: uvcvideo:\n\tGenerate unique camera names","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":11695,"web_url":"https://patchwork.libcamera.org/comment/11695/","msgid":"<20200729090545.GC273308@oden.dyn.berto.se>","date":"2020-07-29T09:05:45","subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: pipeline: uvcvideo:\n\tGenerate unique camera names","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Jacopo,\n\nThanks for your comments.\n\nOn 2020-07-29 10:14:59 +0200, Jacopo Mondi wrote:\n> Hi Niklas,\n> \n> On Wed, Jul 29, 2020 at 01:42:24AM +0200, Niklas Söderlund wrote:\n> > Generate camera names that are unique and persistent between system\n> > resets. The name is constructed from the USB vendor and product\n> > information that is stored on USB device itself and the USB bus and\n> > device numbers where the hardware is plugged in.\n> >\n> > Before this change example of camera names:\n> >\n> > Venus USB2.0 Camera: Venus USB2\n> > Logitech Webcam C930e\n> >\n> > After this change the same cameras are:\n> >\n> > 0ac8:3420:3:10\n> > 046d:0843:3:4\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 34 +++++++++++++++++++-\n> > 1 file changed, 33 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 93e3dc17e3a7105e..72f3f5d9f7c414d0 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -6,6 +6,7 @@\n> > */\n> >\n> > #include \n> > +#include \n> > #include \n> > #include \n> > #include \n> > @@ -81,6 +82,9 @@ public:\n> > \tbool match(DeviceEnumerator *enumerator) override;\n> >\n> > private:\n> > +\tstd::string generateName(const MediaDevice *media,\n> > +\t\t\t\t const std::string path);\n> > +\n> > \tint processControl(ControlList *controls, unsigned int id,\n> > \t\t\t const ControlValue &value);\n> > \tint processControls(UVCCameraData *data, Request *request);\n> > @@ -379,6 +383,28 @@ int PipelineHandlerUVC::queueRequestDevice(Camera *camera, Request *request)\n> > \treturn 0;\n> > }\n> >\n> > +std::string PipelineHandlerUVC::generateName(const MediaDevice *media,\n> \n> Is media used ?\n> \n> > +\t\t\t\t\t const std::string path)\n> \n> I would have passed the CameraData instance and got the path from\n> there.\n\nI have no strong preference so I will make it so in v4.\n\n\n> \n> > +{\n> > +\tstatic const std::vector files = { \"idVendor\", \"idProduct\", \"busnum\", \"devnum\" };\n> \n> less than 120 cols ?\n> \n> > +\tstd::vector values;\n> > +\tstd::string value;\n> > +\n> > +\tfor (const std::string &name : files) {\n> > +\t\tstd::ifstream file(path + \"/../\" + name);\n> > +\n> > +\t\tif (!file.is_open())\n> > +\t\t\treturn \"\";\n> \n> Return or just skip that file ? If you want to return is this worth an\n> error message ?\n\nAll files are mandatory so its all pass or we should fail.\n\n> > +\n> > +\t\tstd::getline(file, value);\n> > +\t\tfile.close();\n> \n> This applies to the other patches as well, reading\n> std::ifstream::close documentation I see:\n> \n> \"Note that any open file is automatically closed when the ifstream object is destroyed.\"\n> \n> Do you need to call close ? I got suspicious as it felt weird that an\n> STL library opens a file on construction but requires manual closure..\n\nI like to explicitly close the file, specially here as we loop I think \nit makes the code more readable.\n\n> \n> > +\n> > +\t\tvalues.push_back(value);\n> > +\t}\n> > +\n> > +\treturn utils::join(values, \":\");\n> > +}\n> > +\n> > bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > {\n> > \tMediaDevice *media;\n> > @@ -405,8 +431,14 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > \t\treturn false;\n> >\n> > \t/* Create and register the camera. */\n> > +\tstd::string name = generateName(media, data->video_->devicePath());\n> > +\tif (name.empty()) {\n> > +\t\tLOG(UVC, Error) << \"Failed to generate camera name\";\n> > +\t\treturn false;\n> > +\t}\n> \n> Oh the error is here, and is better here than in the function. But\n> then you have to fail if you don't find one of the files... I think\n> what you have here is right then...\n> \n> > +\n> > \tstd::set streams{ &data->stream_ };\n> > -\tstd::shared_ptr camera = Camera::create(this, media->model(), streams);\n> > +\tstd::shared_ptr camera = Camera::create(this, name, streams);\n> \n> nits apart\n> Reviewed-by: Jacopo Mondi \n\nThanks.\n\n> \n> Thanks\n> j\n> \n> > \tregisterCamera(std::move(camera), std::move(data));\n> >\n> > \t/* Enable hot-unplug notifications. */\n> > --\n> > 2.27.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 19B6CBD878\n\tfor ;\n\tWed, 29 Jul 2020 09:05:50 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id A87BA613C6;\n\tWed, 29 Jul 2020 11:05:49 +0200 (CEST)","from mail-lj1-x241.google.com (mail-lj1-x241.google.com\n\t[IPv6:2a00:1450:4864:20::241])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4A909605B2\n\tfor ;\n\tWed, 29 Jul 2020 11:05:48 +0200 (CEST)","by mail-lj1-x241.google.com with SMTP id t6so11271619ljk.9\n\tfor ;\n\tWed, 29 Jul 2020 02:05:48 -0700 (PDT)","from localhost (h-209-203.A463.priv.bahnhof.se. [155.4.209.203])\n\tby smtp.gmail.com with ESMTPSA id\n\tf9sm316053lfc.43.2020.07.29.02.05.46\n\t(version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256);\n\tWed, 29 Jul 2020 02:05:46 -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=\"aGAos3X+\"; 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=MZymOd2RWw1besVi0RKfR0+nElNksfUzS3+e2ZUNaeU=;\n\tb=aGAos3X+4bFcKm392NRAOZ0m03yeTvF4MWGPXBeGoJLMHkXSG5FIVrlPiJQ5lNsyim\n\tOD+7t7X08eOyrRA1OuFkJi6tZI9EiqYuj2ILFyytJsvkdTboREJGyuAX+SzvHiFOLUsW\n\tJZydtOO89/1MdQ2wYX0GldPzoiASKzdVIY9QyRf7uRd3cSxQdjMi8oQ2b0H9rJvj/Azg\n\tPxshYec4XiYSLUTj8MzEPquH+7+cnczAbwhVaSrg98+BLsmli+9EKMLdTRVefjSIqv+X\n\tKmarvYO1P2JsaMKzL7bijPV8V4SIpEhbvb2fLHDRdwre/OQNL13Vc4Ny8VOlikFrOsgW\n\tRwnA==","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=MZymOd2RWw1besVi0RKfR0+nElNksfUzS3+e2ZUNaeU=;\n\tb=P2dnJ+AbnT+SJdjhEUre/3yOOCh+KHCy9VJOu9yGlpIq6UiARzjnADIpTjhtw2whMX\n\t0H0bprTTLd/dZtyUOiEhQ0Ug3yIYRay70ocCCMud/IHs84TRa/SKF5R2LN0pSynLg/D8\n\t1DS3bKSAz+tdy6fZX+GWfUhWshxP4D6bC3B1JwuFgoTMbuo401sigiYLdoqMHFc62Jfp\n\to3DefQtlz/gqFSDeSfVhiVJKRSXq9V8kmZZiVpFJmPwIYl24h40Et2IoQHl4K72eygp/\n\th7zhPQeQNkuqCyca3vZQgbdUTDMqIn88xKhYFpWhG2j0pTOZ3benV6WkzKaz3Y/LZP39\n\tRgQQ==","X-Gm-Message-State":"AOAM532najYTXo5WxXaph/cNwW3XlEb+xCJInxUKWNS3OTp2cMoELC8V\n\tsVY6b6J47/zr2YvyPcZy5eAnWA==","X-Google-Smtp-Source":"ABdhPJxkW5Slk00AHTfMhsFfrHfCj+fJu4KLwBxRkk2gypIdUptOFDJQ94/OewvOq/al13iERCSw0Q==","X-Received":"by 2002:a2e:7a10:: with SMTP id\n\tv16mr8603977ljc.188.1596013547555; \n\tWed, 29 Jul 2020 02:05:47 -0700 (PDT)","Date":"Wed, 29 Jul 2020 11:05:45 +0200","From":"Niklas =?iso-8859-1?q?S=F6derlund?= ","To":"Jacopo Mondi ","Message-ID":"<20200729090545.GC273308@oden.dyn.berto.se>","References":"<20200728233744.3503740-1-niklas.soderlund@ragnatech.se>\n\t<20200728234225.3505868-1-niklas.soderlund@ragnatech.se>\n\t<20200728234225.3505868-4-niklas.soderlund@ragnatech.se>\n\t<20200729081459.wrdlfexa3a5lx7np@uno.localdomain>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20200729081459.wrdlfexa3a5lx7np@uno.localdomain>","Subject":"Re: [libcamera-devel] [PATCH v3 4/5] libcamera: pipeline: uvcvideo:\n\tGenerate unique camera names","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\" "}}]