[{"id":27676,"web_url":"https://patchwork.libcamera.org/comment/27676/","msgid":"","date":"2023-08-21T13:48:55","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hello Gabby,\n\nThanks for your patch.\n\nOn Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n>\n> Identify and open the UVC metadata's video node. This will give us\n> access to metadata associated with the video stream of the uvc camera.\n> The user will not have access to this video node and will not need to\n> manage its data in any way.\n>\n> Signed-off-by: Gabby George \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 38f48a5d..dbe0cc8c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -49,10 +49,13 @@ public:\n> const std::string &id() const { return id_; }\n>\n> std::unique_ptr video_;\n> + std::unique_ptr metadata_;\n> Stream stream_;\n> std::map> formats_;\n>\n> private:\n> + int initMetadata(MediaDevice *media);\n> +\n> bool generateId();\n>\n> std::string id_;\n> @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> return true;\n> }\n>\n> +int UVCCameraData::initMetadata(MediaDevice *media)\n> +{\n> + int ret;\n> +\n> + const std::vector &entities = media->entities();\n> + std::string dev_node_name = video_->deviceNode();\n> + auto metadata = std::find_if(entities.begin(), entities.end(),\n> + [&dev_node_name](MediaEntity *e) {\n> + return e->type() == MediaEntity::Type::V4L2VideoDevice\n> + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> + });\n> +\n> + if (metadata == entities.end()) {\n> + return -ENODEV;\n> + }\n> +\n> + /* configure the metadata node */\n> + metadata_ = std::make_unique(*metadata);\n> + ret = metadata_->open();\n\nShould we check for validity of metadata_ with a simple if (metadata_)\n? or is it overengineering ?\n\n> + if (ret)\n> + return ret;\n> +\n> + if (!(metadata_->caps().isMeta())) {\n> + /*\n> + * UVC Devices are usually only anticipated to expose two\n> + * devices, so we assume the non-default device is the metadata\n> + * device node\n> + */\n\nThe comment seems misaligned in indentation, please fix it.\n\n> + return -EINVAL;\n> + }\n> + return 0;\n> +}\n> +\n> int UVCCameraData::init(MediaDevice *media)\n> {\n> int ret;\n> @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n>\n> controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>\n> + ret = initMetadata(media);\n> + if (ret) {\n> + metadata_ = nullptr;\n\nI think, you are leaking metadata_ if the initMetadata function\nreturns from failure of\n\n> + ret = metadata_->open();\n> + if (ret)\n> + return ret;\n\n\n> + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> + }\n> +\n> return 0;\n> }\n>\n> --\n> 2.34.1\n>\n\nRegards,\nVedant Paranjape\n\nOn Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n>\n> Identify and open the UVC metadata's video node. This will give us\n> access to metadata associated with the video stream of the uvc camera.\n> The user will not have access to this video node and will not need to\n> manage its data in any way.\n>\n> Signed-off-by: Gabby George \n> ---\n> src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> 1 file changed, 42 insertions(+)\n>\n> diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> index 38f48a5d..dbe0cc8c 100644\n> --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> @@ -49,10 +49,13 @@ public:\n> const std::string &id() const { return id_; }\n>\n> std::unique_ptr video_;\n> + std::unique_ptr metadata_;\n> Stream stream_;\n> std::map> formats_;\n>\n> private:\n> + int initMetadata(MediaDevice *media);\n> +\n> bool generateId();\n>\n> std::string id_;\n> @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> return true;\n> }\n>\n> +int UVCCameraData::initMetadata(MediaDevice *media)\n> +{\n> + int ret;\n> +\n> + const std::vector &entities = media->entities();\n> + std::string dev_node_name = video_->deviceNode();\n> + auto metadata = std::find_if(entities.begin(), entities.end(),\n> + [&dev_node_name](MediaEntity *e) {\n> + return e->type() == MediaEntity::Type::V4L2VideoDevice\n> + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> + });\n> +\n> + if (metadata == entities.end()) {\n> + return -ENODEV;\n> + }\n> +\n> + /* configure the metadata node */\n> + metadata_ = std::make_unique(*metadata);\n> + ret = metadata_->open();\n> + if (ret)\n> + return ret;\n> +\n> + if (!(metadata_->caps().isMeta())) {\n> + /*\n> + * UVC Devices are usually only anticipated to expose two\n> + * devices, so we assume the non-default device is the metadata\n> + * device node\n> + */\n> + return -EINVAL;\n> + }\n> + return 0;\n> +}\n> +\n> int UVCCameraData::init(MediaDevice *media)\n> {\n> int ret;\n> @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n>\n> controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>\n> + ret = initMetadata(media);\n> + if (ret) {\n> + metadata_ = nullptr;\n> + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> + }\n> +\n> return 0;\n> }\n>\n> --\n> 2.34.1\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id B4639BDE17\n\tfor ;\n\tMon, 21 Aug 2023 13:49:10 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 1234F627E0;\n\tMon, 21 Aug 2023 15:49:10 +0200 (CEST)","from mail-ot1-x336.google.com (mail-ot1-x336.google.com\n\t[IPv6:2607:f8b0:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F3C1F61E09\n\tfor ;\n\tMon, 21 Aug 2023 15:49:07 +0200 (CEST)","by mail-ot1-x336.google.com with SMTP id\n\t46e09a7af769-6bd0afbd616so2656607a34.0\n\tfor ;\n\tMon, 21 Aug 2023 06:49:07 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1692625750;\n\tbh=nwA4nNLNeFU8iJYSJ1uFiYkBq9BWAfDBL5biQNwn8GU=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=i1EXP15VpQz6/2iVozaqd7uEqfM9ZelBAK9T3nt3fMiFe3mHPkb47hYvoumgV4BiJ\n\t61Ee8f0huiNtKjooaM5olMKXeW8RqCparnyz//fgJr5MVAyh2y38DywjYTxNXOQbzz\n\tieUnLDCHFfd5frWbIvTq8UABh0lDDAnAyWjRhYX484SSvkXdkXAR7vL2pbcSx8rzw5\n\tU8+5y4upUS3jHWUTL0u8cc7lfC6hIscbNwASZyeWnuEoncY3YNktSs4JfnAj6ASVx/\n\t+TXUZbYoPj7/L4aUjV5+Nl2dVufBux6NoUqcHoJZOxrreRrUePZeZnmv35A9l/1Sua\n\trYZroDPY+i/mw==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20221208; t=1692625746; x=1693230546;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=GB5TThYeSMYXQMy7iEu80Vn0SuBXx+JVq3DCLVQuxyE=;\n\tb=GIEObooBKRP5K6IRPmObK5TNmD5YeLlVgCiW3G6f7dtkzeq80+6wd65U2OwNlGtV2l\n\tK6Q6Ua/PzDirUG4JwlfxeUUD6u3ic4aEHCnMBgtIS8MmaNVnUPHpd3vHf3WWhLei2hId\n\tzZGKr8f/FOxtewByrGbJd5Wnkhsd9Fsb7ozhLBsUV/ND9Ed/i6Zt6gn7ZvFl1J0gg8iN\n\trCR0u9lG0i8LcqZ/wf80nVzhHsy5wP+/IatgnT43nhd3IT/j+I5Lu5OdXMwnC83iBmm6\n\t5zx9UmLFWZLSnYNlX9lhIhlkdD2vtcjQwGbVeev3BIOYPv6hyUMfVosew0VPjyLUUVic\n\t8sGQ=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"GIEObooB\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1692625746; x=1693230546;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=GB5TThYeSMYXQMy7iEu80Vn0SuBXx+JVq3DCLVQuxyE=;\n\tb=LNcVEcUvJz7PyoppDBtdPu9g3h1JpvDmRRXLPdgPhYgmGDBh8kc8/PQDSsyumJZ4eJ\n\tLgMqdPqtrc9ggbZvRiTUAAyPGCwkae94lgqCSC37mDYS9BhPy0Ps/G10psV5jcMpufpF\n\tvcJ2451V+GODUrZZHRspznWDlcbs8GHHewRaIyhcgj00uq3PmmW5dROetl/yaOQe+oFk\n\tHth/n9x/MMm+ffEt7ZAnb2CvKMagzecD5yp6dElaHlV/m0Y/ZctrIGTuDkm6HQDjGB0/\n\tmZZaoFj8UyK2FnZgYo1hr6mwSBHqciFRDryJgqB4MBPwOJs/t3sYzDqnlHNikzF1GWac\n\tEkwg==","X-Gm-Message-State":"AOJu0YyUEX/kEADv/P9ozw9bHSiiBOQXJzdG5koiLnNoMaqPfBs0CQv3\n\tt3my0aAtnoHxuxb1aEYlusO3VdmZsjqNyeSB0ASOrg/rggYECA==","X-Google-Smtp-Source":"AGHT+IGhEkhqrG92wQdY3zXbwgiZJTIjbdDtvR9i/IkLG4ZVdlp4vN/m09R2EFvy2ho43HwZHi/Hh91kzcO25UkFbYU=","X-Received":"by 2002:a9d:6e86:0:b0:6b9:dc90:8a85 with SMTP id\n\ta6-20020a9d6e86000000b006b9dc908a85mr7083680otr.24.1692625746523;\n\tMon, 21 Aug 2023 06:49:06 -0700 (PDT)","MIME-Version":"1.0","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>","In-Reply-To":"<20230821131039.127370-2-gabbymg94@gmail.com>","Date":"Mon, 21 Aug 2023 19:18:55 +0530","Message-ID":"","To":"Gabby George ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Vedant Paranjape via libcamera-devel\n\t","Reply-To":"Vedant Paranjape ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27693,"web_url":"https://patchwork.libcamera.org/comment/27693/","msgid":"","date":"2023-08-27T04:29:39","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":160,"url":"https://patchwork.libcamera.org/api/people/160/","name":"Gabrielle George","email":"gabbymg94@gmail.com"},"content":"On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <\nvedantparanjape160201@gmail.com> wrote:\n\n> Hello Gabby,\n>\n> Thanks for your patch.\n>\n> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n> >\n> > Identify and open the UVC metadata's video node. This will give us\n> > access to metadata associated with the video stream of the uvc camera.\n> > The user will not have access to this video node and will not need to\n> > manage its data in any way.\n> >\n> > Signed-off-by: Gabby George \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> > 1 file changed, 42 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 38f48a5d..dbe0cc8c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -49,10 +49,13 @@ public:\n> > const std::string &id() const { return id_; }\n> >\n> > std::unique_ptr video_;\n> > + std::unique_ptr metadata_;\n> > Stream stream_;\n> > std::map> formats_;\n> >\n> > private:\n> > + int initMetadata(MediaDevice *media);\n> > +\n> > bool generateId();\n> >\n> > std::string id_;\n> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator\n> *enumerator)\n> > return true;\n> > }\n> >\n> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> > +{\n> > + int ret;\n> > +\n> > + const std::vector &entities = media->entities();\n> > + std::string dev_node_name = video_->deviceNode();\n> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n> > + [&dev_node_name](MediaEntity *e) {\n> > + return e->type() ==\n> MediaEntity::Type::V4L2VideoDevice\n> > + && !(e->flags()\n> & MEDIA_ENT_FL_DEFAULT);\n> > + });\n> > +\n> > + if (metadata == entities.end()) {\n> > + return -ENODEV;\n> > + }\n> > +\n> > + /* configure the metadata node */\n> > + metadata_ = std::make_unique(*metadata);\n> > + ret = metadata_->open();\n>\n> Should we check for validity of metadata_ with a simple if (metadata_)\n> ? or is it overengineering ?\n>\n\nGood call! I think we should.\n\n>\n> > + if (ret)\n> > + return ret;\n> > +\n> > + if (!(metadata_->caps().isMeta())) {\n> > + /*\n> > + * UVC Devices are usually only anticipated to expose two\n> > + * devices, so we assume the non-default device is the metadata\n> > + * device node\n> > + */\n>\n> The comment seems misaligned in indentation, please fix it.\n>\n\nI'll make sure it's correct in the next patch.\n\n>\n> > + return -EINVAL;\n> > + }\n> > + return 0;\n> > +}\n> > +\n> > int UVCCameraData::init(MediaDevice *media)\n> > {\n> > int ret;\n> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> >\n> > controlInfo_ = ControlInfoMap(std::move(ctrls),\n> controls::controls);\n> >\n> > + ret = initMetadata(media);\n> > + if (ret) {\n> > + metadata_ = nullptr;\n>\n> I think, you are leaking metadata_ if the initMetadata function\n> returns from failure of\n>\n\nI thought assigning null to smart pointers initiates the normal cleanup\nroutine of the objects they point to. Is this the leak you're talking\nabout, or should I clean up something else too?\n\n> \n> > + ret = metadata_->open();\n> > + if (ret)\n> > + return ret;\n> \n>\n> > + LOG(UVC, Error) << \"Could not find a metadata video\n> device.\";\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > --\n> > 2.34.1\n> >\n>\n> Regards,\n> Vedant Paranjape\n>\n\nThanks for the suggestions!\n\n>\n> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n> >\n> > Identify and open the UVC metadata's video node. This will give us\n> > access to metadata associated with the video stream of the uvc camera.\n> > The user will not have access to this video node and will not need to\n> > manage its data in any way.\n> >\n> > Signed-off-by: Gabby George \n> > ---\n> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> > 1 file changed, 42 insertions(+)\n> >\n> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > index 38f48a5d..dbe0cc8c 100644\n> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > @@ -49,10 +49,13 @@ public:\n> > const std::string &id() const { return id_; }\n> >\n> > std::unique_ptr video_;\n> > + std::unique_ptr metadata_;\n> > Stream stream_;\n> > std::map> formats_;\n> >\n> > private:\n> > + int initMetadata(MediaDevice *media);\n> > +\n> > bool generateId();\n> >\n> > std::string id_;\n> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator\n> *enumerator)\n> > return true;\n> > }\n> >\n> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> > +{\n> > + int ret;\n> > +\n> > + const std::vector &entities = media->entities();\n> > + std::string dev_node_name = video_->deviceNode();\n> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n> > + [&dev_node_name](MediaEntity *e) {\n> > + return e->type() ==\n> MediaEntity::Type::V4L2VideoDevice\n> > + && !(e->flags()\n> & MEDIA_ENT_FL_DEFAULT);\n> > + });\n> > +\n> > + if (metadata == entities.end()) {\n> > + return -ENODEV;\n> > + }\n> > +\n> > + /* configure the metadata node */\n> > + metadata_ = std::make_unique(*metadata);\n> > + ret = metadata_->open();\n> > + if (ret)\n> > + return ret;\n> > +\n> > + if (!(metadata_->caps().isMeta())) {\n> > + /*\n> > + * UVC Devices are usually only anticipated to expose two\n> > + * devices, so we assume the non-default device is the metadata\n> > + * device node\n> > + */\n> > + return -EINVAL;\n> > + }\n> > + return 0;\n> > +}\n> > +\n> > int UVCCameraData::init(MediaDevice *media)\n> > {\n> > int ret;\n> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> >\n> > controlInfo_ = ControlInfoMap(std::move(ctrls),\n> controls::controls);\n> >\n> > + ret = initMetadata(media);\n> > + if (ret) {\n> > + metadata_ = nullptr;\n> > + LOG(UVC, Error) << \"Could not find a metadata video\n> device.\";\n> > + }\n> > +\n> > return 0;\n> > }\n> >\n> > --\n> > 2.34.1\n> >\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id AE36BBD80A\n\tfor ;\n\tSun, 27 Aug 2023 04:29:54 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id B7E3E627E0;\n\tSun, 27 Aug 2023 06:29:53 +0200 (CEST)","from mail-ej1-x62c.google.com (mail-ej1-x62c.google.com\n\t[IPv6:2a00:1450:4864:20::62c])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 0B49E61DFF\n\tfor ;\n\tSun, 27 Aug 2023 06:29:52 +0200 (CEST)","by mail-ej1-x62c.google.com with SMTP id\n\ta640c23a62f3a-9a19bf6ab66so276631966b.3\n\tfor ;\n\tSat, 26 Aug 2023 21:29:51 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693110593;\n\tbh=RhA6a9v5fjRmMWCVq/CR0CqSHmsRjfXQovE8vCzPr4I=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=G525bzkNnikg0QTnExSgjuC54ddsBGYnwkpE8KLuwzc8NoaQiEwR5HYFxvKQguBws\n\tb/pCSXy0KkZY3hO/3lbdHqjF55zCPbjF7v6GfvQ2h0fQd9v+1mz/OAlDUx9ugrszWP\n\tAXDCWRMW9L4vDW+sxSbDADYBjxLOnJ9UozEJqm8kxzTmWWaM1o6wJcXqDAiJeQi2/F\n\t7kyenHwlrxkHwb40uAWAyQpLfD03xxQYBO/QUE+3cCeTgq+QYXAxU2JQ5p0jrwT+3l\n\tcaB58DwKV3OtnHBjNfNmNoTzlfNr8uA6fc6P4oY0j8Kj5iBjR9fOwpWzgX9pOXxbiL\n\tzj2W1b0xkQhNQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20221208; t=1693110591; x=1693715391;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=RCyih0Qr0TRf1TAL5Cas4vwUpVIOJJ7wmtg1leKcRr4=;\n\tb=g3A4ADAf3QnupVvRqdeX2G+osp+EnKv2cj5A5s0l/SjLO/kbMOfLKnwlJX2Z+OWI4r\n\toNFHUrYUr0hkC6V6xjUrxj3hNdoaMUj68XXEWqVIX1PEuuvBgM96gQQJSwPOjdltjNUH\n\tZcBfBSFAHYfxHhV5i081R7YWXMTL4+UqWLgMKC8P0rlVzU00tqge1/xjNFqM1aQu9fcd\n\trWrL8GFyj55tOnDbhebO7f0IwlxmHPCIpsthucBGrdjAJIM7HeaEf5oAcDfRKVXzkaaz\n\tWHImT0ObZzPxosBRUaesP4ZN2dUIWp9smO9QX580OS0VShsUqFSa+AHP8uyK/fudPiTq\n\tMVVA=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"g3A4ADAf\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1693110591; x=1693715391;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=RCyih0Qr0TRf1TAL5Cas4vwUpVIOJJ7wmtg1leKcRr4=;\n\tb=R3aCZ0tL25FdRSw1gkfRYIL7xpkeP1Ob/FMKR/H8umzuB5hDqcjjSnJXfnzX+3Rf3K\n\t8Yffxx0j/tZn78rojVChWfxAdssnjcvNX8utM7J2ZZm4htucLR5TAokEgBBnCjlJxGuo\n\tK4uHHvenGt1qGr1oWuCSdmWBQ0z+XQovP7s9952PI4mbaElk11Lz0RrGO/S5XzEiwlAa\n\tDC1SPOlN4UGzJ8/pJ6JnolCSuR2E4OAKa+h1S69mEAF2dPDwRlYJwsh9tuIhTVNN3rEs\n\trnyPbA8e0t9tgNh6JL1OnSQL1i1/pRzgx6WkFn07NQ9oeQFkTSeR80Y8L/BS5kKG72dv\n\tbGVA==","X-Gm-Message-State":"AOJu0YxfcC1W70UVQikEWDJBjgp2mVHZ/uVMJtnNrbkHj9diUSlrio5/\n\tyfod88+JVbbEm92kud2gNisay/1YmidtX8e4qdI=","X-Google-Smtp-Source":"AGHT+IHuAemJHc4eo/cZ4q3Xzbc3KqUoU+NUbyyDWekvONsAIRjkNOVTbJm30H7+r8P+NAjvHZvKFgO+Bs1gYGlEr0I=","X-Received":"by 2002:a17:907:b09:b0:9a2:474:4aa0 with SMTP id\n\th9-20020a1709070b0900b009a204744aa0mr6404674ejl.48.1693110591099;\n\tSat, 26 Aug 2023 21:29:51 -0700 (PDT)","MIME-Version":"1.0","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>\n\t","In-Reply-To":"","Date":"Sat, 26 Aug 2023 21:29:39 -0700","Message-ID":"","To":"Vedant Paranjape ","Content-Type":"multipart/alternative; boundary=\"0000000000005916f40603e00749\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Gabrielle George via libcamera-devel\n\t","Reply-To":"Gabrielle George ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27695,"web_url":"https://patchwork.libcamera.org/comment/27695/","msgid":"","date":"2023-08-27T18:58:49","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hello Gabby,\n\nOn Sun, Aug 27, 2023 at 9:59 AM Gabrielle George wrote:\n>\n>\n>\n> On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape wrote:\n>>\n>> Hello Gabby,\n>>\n>> Thanks for your patch.\n>>\n>> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n>> >\n>> > Identify and open the UVC metadata's video node. This will give us\n>> > access to metadata associated with the video stream of the uvc camera.\n>> > The user will not have access to this video node and will not need to\n>> > manage its data in any way.\n>> >\n>> > Signed-off-by: Gabby George \n>> > ---\n>> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n>> > 1 file changed, 42 insertions(+)\n>> >\n>> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > index 38f48a5d..dbe0cc8c 100644\n>> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > @@ -49,10 +49,13 @@ public:\n>> > const std::string &id() const { return id_; }\n>> >\n>> > std::unique_ptr video_;\n>> > + std::unique_ptr metadata_;\n>> > Stream stream_;\n>> > std::map> formats_;\n>> >\n>> > private:\n>> > + int initMetadata(MediaDevice *media);\n>> > +\n>> > bool generateId();\n>> >\n>> > std::string id_;\n>> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>> > return true;\n>> > }\n>> >\n>> > +int UVCCameraData::initMetadata(MediaDevice *media)\n>> > +{\n>> > + int ret;\n>> > +\n>> > + const std::vector &entities = media->entities();\n>> > + std::string dev_node_name = video_->deviceNode();\n>> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n>> > + [&dev_node_name](MediaEntity *e) {\n>> > + return e->type() == MediaEntity::Type::V4L2VideoDevice\n>> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n>> > + });\n>> > +\n>> > + if (metadata == entities.end()) {\n>> > + return -ENODEV;\n>> > + }\n>> > +\n>> > + /* configure the metadata node */\n>> > + metadata_ = std::make_unique(*metadata);\n>> > + ret = metadata_->open();\n>>\n>> Should we check for validity of metadata_ with a simple if (metadata_)\n>> ? or is it overengineering ?\n>\n>\n> Good call! I think we should.\n>>\n>>\n>> > + if (ret)\n>> > + return ret;\n>> > +\n>> > + if (!(metadata_->caps().isMeta())) {\n>> > + /*\n>> > + * UVC Devices are usually only anticipated to expose two\n>> > + * devices, so we assume the non-default device is the metadata\n>> > + * device node\n>> > + */\n>>\n>> The comment seems misaligned in indentation, please fix it.\n>\n>\n> I'll make sure it's correct in the next patch.\n>>\n>>\n>> > + return -EINVAL;\n>> > + }\n>> > + return 0;\n>> > +}\n>> > +\n>> > int UVCCameraData::init(MediaDevice *media)\n>> > {\n>> > int ret;\n>> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n>> >\n>> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>> >\n>> > + ret = initMetadata(media);\n>> > + if (ret) {\n>> > + metadata_ = nullptr;\n>>\n>> I think, you are leaking metadata_ if the initMetadata function\n>> returns from failure of\n>\n>\n> I thought assigning null to smart pointers initiates the normal cleanup routine of the objects they point to. Is this the leak you're talking about, or should I clean up something else too?\n\nhttps://godbolt.org/z/rhx886abY\n\nI tried doing something similar, check the godbolt link, assigning\nnull has no effect on the allocated memory. (cc: @Laurent Pinchart and\n@Kieran Bingham can you guys have a look once)\n\n>>\n>> \n>> > + ret = metadata_->open();\n>> > + if (ret)\n>> > + return ret;\n>> \n>>\n>> > + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n>> > + }\n>> > +\n>> > return 0;\n>> > }\n>> >\n>> > --\n>> > 2.34.1\n>> >\n>>\n>> Regards,\n>> Vedant Paranjape\n>\n>\n> Thanks for the suggestions!\n>>\n>>\n>> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n>> >\n>> > Identify and open the UVC metadata's video node. This will give us\n>> > access to metadata associated with the video stream of the uvc camera.\n>> > The user will not have access to this video node and will not need to\n>> > manage its data in any way.\n>> >\n>> > Signed-off-by: Gabby George \n>> > ---\n>> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n>> > 1 file changed, 42 insertions(+)\n>> >\n>> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > index 38f48a5d..dbe0cc8c 100644\n>> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n>> > @@ -49,10 +49,13 @@ public:\n>> > const std::string &id() const { return id_; }\n>> >\n>> > std::unique_ptr video_;\n>> > + std::unique_ptr metadata_;\n>> > Stream stream_;\n>> > std::map> formats_;\n>> >\n>> > private:\n>> > + int initMetadata(MediaDevice *media);\n>> > +\n>> > bool generateId();\n>> >\n>> > std::string id_;\n>> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n>> > return true;\n>> > }\n>> >\n>> > +int UVCCameraData::initMetadata(MediaDevice *media)\n>> > +{\n>> > + int ret;\n>> > +\n>> > + const std::vector &entities = media->entities();\n>> > + std::string dev_node_name = video_->deviceNode();\n>> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n>> > + [&dev_node_name](MediaEntity *e) {\n>> > + return e->type() == MediaEntity::Type::V4L2VideoDevice\n>> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n>> > + });\n>> > +\n>> > + if (metadata == entities.end()) {\n>> > + return -ENODEV;\n>> > + }\n>> > +\n>> > + /* configure the metadata node */\n>> > + metadata_ = std::make_unique(*metadata);\n>> > + ret = metadata_->open();\n>> > + if (ret)\n>> > + return ret;\n>> > +\n>> > + if (!(metadata_->caps().isMeta())) {\n>> > + /*\n>> > + * UVC Devices are usually only anticipated to expose two\n>> > + * devices, so we assume the non-default device is the metadata\n>> > + * device node\n>> > + */\n>> > + return -EINVAL;\n>> > + }\n>> > + return 0;\n>> > +}\n>> > +\n>> > int UVCCameraData::init(MediaDevice *media)\n>> > {\n>> > int ret;\n>> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n>> >\n>> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n>> >\n>> > + ret = initMetadata(media);\n>> > + if (ret) {\n>> > + metadata_ = nullptr;\n>> > + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n>> > + }\n>> > +\n>> > return 0;\n>> > }\n>> >\n>> > --\n>> > 2.34.1\n>> >\n\nRegards,\nVedant Paranjape","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 AA5A3BD80A\n\tfor ;\n\tSun, 27 Aug 2023 18:59:05 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id E99BE627DF;\n\tSun, 27 Aug 2023 20:59:04 +0200 (CEST)","from mail-ot1-x336.google.com (mail-ot1-x336.google.com\n\t[IPv6:2607:f8b0:4864:20::336])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id E9EC160377\n\tfor ;\n\tSun, 27 Aug 2023 20:59:02 +0200 (CEST)","by mail-ot1-x336.google.com with SMTP id\n\t46e09a7af769-6befdad890eso885223a34.3\n\tfor ;\n\tSun, 27 Aug 2023 11:59:02 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693162745;\n\tbh=jk5tvsvPqfC2+tGyDwDETEZ7hpjdf8EZon3m+4Ma0Qc=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=PsoiUQDh9JMG8t5i2ByNXJKkNRKDaAw6Zpv0Ua90YyZg5+eNUG832ft6W86i1WGek\n\tOmK9/qMXoEtEeO38N4C/9yPgrWhAd/T7Txbfer5p4LeBds2WO6i6JKfnmv4VcyIwr4\n\tDHUPgBWF/F1gtcDJ8sQzfjbj0zWv5UVNTNC66ZM5kEG37M45yyxygVlg/tGZZQk7vk\n\toH07a83LaZWnErzdLupUuUtlB0gMsn5J6kFdZCmsq+wBPCIkeCaedQXkgOayo4C0HW\n\tSicFad4KQancvEfS//vdEvfZzxfhz8HCz62tOMSmLzSqlFjyxhpKtxIi1ukK6YftZ4\n\tbH93lLzpIUxbQ==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20221208; t=1693162741; x=1693767541;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:from:to:cc:subject:date\n\t:message-id:reply-to;\n\tbh=jj87JNZ1jB5W8pkTHoYt5stKhn+whxEiqCiPUgenAUY=;\n\tb=KJp/GSdD/nw+kBMeTWFEF50poYDbiUwC999XHsJMIzQ/Jra55W504JsPV6SWDCHwTE\n\tevwth2im+S1ajfEHtOW5PPlDFUeiUfPNPz++FfszK+hG8pmrmCgF3bySeFidt8KuM7mI\n\tWwHac6rXNU1iOkA54v1f7wrD6Rv/xeSulU0HI3wSDTH7pr/4RZFoEELObV+0RNOj643J\n\tVq3YC0OG2wQf0/g8oQOL2I3Eudy2wVfM+aQSclmWVh+Qxe5lZ2SDMfk3xCE/CgRc9OIO\n\t/A42lHgOoDQqauXv8DPcGLt1ZI7C8ju5AzGR21/eCBynsNSLeUGfSj9eOmASm53fw1pu\n\tPj5g=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"KJp/GSdD\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1693162741; x=1693767541;\n\th=content-transfer-encoding:cc:to:subject:message-id:date:from\n\t:in-reply-to:references:mime-version:x-gm-message-state:from:to:cc\n\t:subject:date:message-id:reply-to;\n\tbh=jj87JNZ1jB5W8pkTHoYt5stKhn+whxEiqCiPUgenAUY=;\n\tb=NkL+I2JdHrSTx7kDlcrAX4nNW8QAEYZOHlrnriu8gMIIWUnPRL1BF6tQhhn8TXaUJ5\n\tPVVciVWeZx3OoFVCdW5m2WQYPTm3IhmfpOErY5apN1EqMmo+5jMLbUllNonZW2KW1hUZ\n\thxrTUh6xuI+3TgRKjdQEdwmOnbsZl1YyJyX1EQOMcyhM0GZvXSLmiicUxxawzHfapk9Y\n\tNsqJFzNJzg5qGZmER8fgdm+1zcoU3yFJSkjQrufuLn/4adAdgHmdElpyk7damUsa7YGd\n\tjMXKftPMN0UANDDq59JsAxrhmk04lUrGnxkiwQLnvuvoijt75Mcmb76EfWR6Akv1KNXW\n\tImXg==","X-Gm-Message-State":"AOJu0YwcoouLq3HuqbNxSP4/Ra9Btt++l+T8x/ApHmwWLmBDA+mbRBCL\n\taNScJtwXdJMdYdhcH1X+AO0j1s25VCgA4MV8hXA=","X-Google-Smtp-Source":"AGHT+IEcy76pGin0cnthZ7e15bN+Aiox/gtjx4vGWitBCG1DpVJ/hXe2zUBQdEOpNFP54w/CXr0EU3zvHnWIlp8QjVA=","X-Received":"by 2002:a9d:4e94:0:b0:6b7:1d93:72e0 with SMTP id\n\tv20-20020a9d4e94000000b006b71d9372e0mr12698819otk.32.1693162741459;\n\tSun, 27 Aug 2023 11:59:01 -0700 (PDT)","MIME-Version":"1.0","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>\n\t\n\t","In-Reply-To":"","Date":"Mon, 28 Aug 2023 00:28:49 +0530","Message-ID":"","To":"Gabrielle George , \n\tLaurent Pinchart ","Content-Type":"text/plain; charset=\"UTF-8\"","Content-Transfer-Encoding":"quoted-printable","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Vedant Paranjape via libcamera-devel\n\t","Reply-To":"Vedant Paranjape ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27696,"web_url":"https://patchwork.libcamera.org/comment/27696/","msgid":"<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","date":"2023-08-28T08:16:13","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":143,"url":"https://patchwork.libcamera.org/api/people/143/","name":"Jacopo Mondi","email":"jacopo.mondi@ideasonboard.com"},"content":"Hello\n\nOn Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via libcamera-devel wrote:\n> Hello Gabby,\n>\n> On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George wrote:\n> >\n> >\n> >\n> > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape wrote:\n> >>\n> >> Hello Gabby,\n> >>\n> >> Thanks for your patch.\n> >>\n> >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n> >> >\n> >> > Identify and open the UVC metadata's video node. This will give us\n> >> > access to metadata associated with the video stream of the uvc camera.\n> >> > The user will not have access to this video node and will not need to\n> >> > manage its data in any way.\n> >> >\n> >> > Signed-off-by: Gabby George \n> >> > ---\n> >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> >> > 1 file changed, 42 insertions(+)\n> >> >\n> >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > index 38f48a5d..dbe0cc8c 100644\n> >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > @@ -49,10 +49,13 @@ public:\n> >> > const std::string &id() const { return id_; }\n> >> >\n> >> > std::unique_ptr video_;\n> >> > + std::unique_ptr metadata_;\n> >> > Stream stream_;\n> >> > std::map> formats_;\n> >> >\n> >> > private:\n> >> > + int initMetadata(MediaDevice *media);\n> >> > +\n> >> > bool generateId();\n> >> >\n> >> > std::string id_;\n> >> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >> > return true;\n> >> > }\n> >> >\n> >> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> >> > +{\n> >> > + int ret;\n> >> > +\n> >> > + const std::vector &entities = media->entities();\n> >> > + std::string dev_node_name = video_->deviceNode();\n> >> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n> >> > + [&dev_node_name](MediaEntity *e) {\n> >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice\n> >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> >> > + });\n> >> > +\n> >> > + if (metadata == entities.end()) {\n> >> > + return -ENODEV;\n> >> > + }\n> >> > +\n> >> > + /* configure the metadata node */\n> >> > + metadata_ = std::make_unique(*metadata);\n> >> > + ret = metadata_->open();\n> >>\n> >> Should we check for validity of metadata_ with a simple if (metadata_)\n> >> ? or is it overengineering ?\n> >\n> >\n> > Good call! I think we should.\n> >>\n> >>\n> >> > + if (ret)\n> >> > + return ret;\n> >> > +\n> >> > + if (!(metadata_->caps().isMeta())) {\n> >> > + /*\n> >> > + * UVC Devices are usually only anticipated to expose two\n> >> > + * devices, so we assume the non-default device is the metadata\n> >> > + * device node\n> >> > + */\n> >>\n> >> The comment seems misaligned in indentation, please fix it.\n> >\n> >\n> > I'll make sure it's correct in the next patch.\n> >>\n> >>\n> >> > + return -EINVAL;\n> >> > + }\n> >> > + return 0;\n> >> > +}\n> >> > +\n> >> > int UVCCameraData::init(MediaDevice *media)\n> >> > {\n> >> > int ret;\n> >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> >> >\n> >> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >> >\n> >> > + ret = initMetadata(media);\n> >> > + if (ret) {\n> >> > + metadata_ = nullptr;\n> >>\n> >> I think, you are leaking metadata_ if the initMetadata function\n> >> returns from failure of\n> >\n> >\n> > I thought assigning null to smart pointers initiates the normal cleanup routine of the objects they point to. Is this the leak you're talking about, or should I clean up something else too?\n\nFrom https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D\n\n(3) unique_ptr& operator=( std::nullptr_t ) noexcept;\n 3) Effectively the same as calling reset().\n\nhttps://en.cppreference.com/w/cpp/memory/unique_ptr/reset\n If the old pointer was non-empty, deletes the previously managed\n object if (old_ptr) get_deleter()(old_ptr).\n\nif metadata_ is a unique_ptr, assigning nullptr to it will delete the object\nit previously managed.\n\n>\n> https://godbolt.org/z/rhx886abY\n>\n> I tried doing something similar, check the godbolt link, assigning\n> null has no effect on the allocated memory. (cc: @Laurent Pinchart and\n> @Kieran Bingham can you guys have a look once)\n>\n\nTry to use unique_ptr, shared_ptr is harder to follow as it has to do\nreference counting. With unique_ptr<> I see\n\n callq operator delete(void*)@PLT\n\nThanks\n j\n\n\n> >>\n> >> \n> >> > + ret = metadata_->open();\n> >> > + if (ret)\n> >> > + return ret;\n> >> \n> >>\n> >> > + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> >> > + }\n> >> > +\n> >> > return 0;\n> >> > }\n> >> >\n> >> > --\n> >> > 2.34.1\n> >> >\n> >>\n> >> Regards,\n> >> Vedant Paranjape\n> >\n> >\n> > Thanks for the suggestions!\n> >>\n> >>\n> >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n> >> >\n> >> > Identify and open the UVC metadata's video node. This will give us\n> >> > access to metadata associated with the video stream of the uvc camera.\n> >> > The user will not have access to this video node and will not need to\n> >> > manage its data in any way.\n> >> >\n> >> > Signed-off-by: Gabby George \n> >> > ---\n> >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> >> > 1 file changed, 42 insertions(+)\n> >> >\n> >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > index 38f48a5d..dbe0cc8c 100644\n> >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> >> > @@ -49,10 +49,13 @@ public:\n> >> > const std::string &id() const { return id_; }\n> >> >\n> >> > std::unique_ptr video_;\n> >> > + std::unique_ptr metadata_;\n> >> > Stream stream_;\n> >> > std::map> formats_;\n> >> >\n> >> > private:\n> >> > + int initMetadata(MediaDevice *media);\n> >> > +\n> >> > bool generateId();\n> >> >\n> >> > std::string id_;\n> >> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> >> > return true;\n> >> > }\n> >> >\n> >> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> >> > +{\n> >> > + int ret;\n> >> > +\n> >> > + const std::vector &entities = media->entities();\n> >> > + std::string dev_node_name = video_->deviceNode();\n> >> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n> >> > + [&dev_node_name](MediaEntity *e) {\n> >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice\n> >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> >> > + });\n> >> > +\n> >> > + if (metadata == entities.end()) {\n> >> > + return -ENODEV;\n> >> > + }\n> >> > +\n> >> > + /* configure the metadata node */\n> >> > + metadata_ = std::make_unique(*metadata);\n> >> > + ret = metadata_->open();\n> >> > + if (ret)\n> >> > + return ret;\n> >> > +\n> >> > + if (!(metadata_->caps().isMeta())) {\n> >> > + /*\n> >> > + * UVC Devices are usually only anticipated to expose two\n> >> > + * devices, so we assume the non-default device is the metadata\n> >> > + * device node\n> >> > + */\n> >> > + return -EINVAL;\n> >> > + }\n> >> > + return 0;\n> >> > +}\n> >> > +\n> >> > int UVCCameraData::init(MediaDevice *media)\n> >> > {\n> >> > int ret;\n> >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> >> >\n> >> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> >> >\n> >> > + ret = initMetadata(media);\n> >> > + if (ret) {\n> >> > + metadata_ = nullptr;\n> >> > + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> >> > + }\n> >> > +\n> >> > return 0;\n> >> > }\n> >> >\n> >> > --\n> >> > 2.34.1\n> >> >\n>\n> Regards,\n> Vedant Paranjape","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 70A82BE080\n\tfor ;\n\tMon, 28 Aug 2023 08:16:19 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 8B21D61E02;\n\tMon, 28 Aug 2023 10:16:18 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 73C0961E02\n\tfor ;\n\tMon, 28 Aug 2023 10:16:16 +0200 (CEST)","from ideasonboard.com (mob-5-91-19-240.net.vodafone.it\n\t[5.91.19.240])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 388702D8;\n\tMon, 28 Aug 2023 10:14:55 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693210578;\n\tbh=cPCway9+bdI6I+RAbNeNlmaKkmjggqin6sEeElDOxYE=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=SntD/Nre/zbd15ynUcZlJasEsuE0oEjWldN9CZ05fdOnpDLBNJb3FgFDg/f7e4Ll/\n\tZtfVyZM7elz/+zK0Yup+Pf0ckcv7aWOjg8cRL4DJm7ewsPiXK+qtIvoexZaGu/5mLA\n\t4TSESRc+Fn1W0x+lZAhin08Q40JlACWyru3N+UBM9Fh5CT+U3NulnuXZbW9TaXK3rM\n\t9hYQd7XBnY97X8icHjFHhNdghygPjjwuXCCmWMro/TFHCS5fBJsYKI6Qj/QMu0XSG5\n\t6HZhe3qxIaVvU07m2HEdtq42s39LNQhU4SlrZve53bGxFmjCvuk/EZrWucuD7sihdW\n\tY5DZf4duwvpyA==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693210495;\n\tbh=cPCway9+bdI6I+RAbNeNlmaKkmjggqin6sEeElDOxYE=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=DOMZSjR4sk0wbMqRYMHgcHmXlk5IXPHwAGdfwnqa8RS2Pa4L+JmJMm3UX8KiQxx85\n\tEGhgmkUVr2AUhtmLjcR+pJmu0gotkNivWImTbhKVL3/8kflDl2NkvhRcyLWc6mnWLF\n\tE7czE+XHWveTgGbwGTNIy35pFhh6fHqrQRnbGo0w="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"DOMZSjR4\"; dkim-atps=neutral","Date":"Mon, 28 Aug 2023 10:16:13 +0200","To":"Vedant Paranjape ","Message-ID":"<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>\n\t\n\t\n\t","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Jacopo Mondi via libcamera-devel ","Reply-To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27700,"web_url":"https://patchwork.libcamera.org/comment/27700/","msgid":"<20230828203337.GG17083@pendragon.ideasonboard.com>","date":"2023-08-28T20:33:37","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hello,\n\nOn Mon, Aug 28, 2023 at 10:16:13AM +0200, Jacopo Mondi wrote:\n> On Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via libcamera-devel wrote:\n> > On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George wrote:\n> > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape wrote:\n> > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George wrote:\n> > >> >\n> > >> > Identify and open the UVC metadata's video node. This will give us\n> > >> > access to metadata associated with the video stream of the uvc camera.\n\ns/uvc/UVC/ (as UVC is an acronym)\n\n> > >> > The user will not have access to this video node and will not need to\n> > >> > manage its data in any way.\n> > >> >\n> > >> > Signed-off-by: Gabby George \n> > >> > ---\n> > >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42 ++++++++++++++++++++\n> > >> > 1 file changed, 42 insertions(+)\n> > >> >\n> > >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > index 38f48a5d..dbe0cc8c 100644\n> > >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > @@ -49,10 +49,13 @@ public:\n> > >> > const std::string &id() const { return id_; }\n> > >> >\n> > >> > std::unique_ptr video_;\n> > >> > + std::unique_ptr metadata_;\n> > >> > Stream stream_;\n> > >> > std::map> formats_;\n> > >> >\n> > >> > private:\n> > >> > + int initMetadata(MediaDevice *media);\n> > >> > +\n> > >> > bool generateId();\n> > >> >\n> > >> > std::string id_;\n> > >> > @@ -411,6 +414,39 @@ bool PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > >> > return true;\n> > >> > }\n> > >> >\n> > >> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> > >> > +{\n> > >> > + int ret;\n\nYou can declare ret on first use.\n\n> > >> > +\n> > >> > + const std::vector &entities = media->entities();\n> > >> > + std::string dev_node_name = video_->deviceNode();\n\ndevNodeName. We use camelCase. And this should be a const reference to\navoid a copy.\n\n> > >> > + auto metadata = std::find_if(entities.begin(), entities.end(),\n> > >> > + [&dev_node_name](MediaEntity *e) {\n\nYou don't use dev_node_name anywhere.\n\n> > >> > + return e->type() == MediaEntity::Type::V4L2VideoDevice\n> > >> > + && !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> > >> > + });\n\nThat won't work correctly if the UVC device has multiple image capture\ndevice nodes. Only one of the image capture device nodes will be marked\nas default, the other one may get picked by this code.\n\nThere's nothing in the kernel uvcvideo driver that currently exposes the\nrelationship between the image capture device node and the metadata\ndevice node :-S This should be fixed, but that's out of scope for your\ncurrent work. Let's thus record this in a comment here:\n\n\t/*\n\t * \\todo The kernel doesn't expose the relationship between image\n\t * capture video devices and metadata capture video devices. Until this\n\t * gets fixed on the kernel side, do our best by picking one metadata\n\t * capture video device.\n\t */\n\nAs for how to find a metadata capture device node, you can simply check\nthe number of pads. The entities corresponding to metadata devices\ncurrently have no pad.\n\n> > >> > +\n> > >> > + if (metadata == entities.end()) {\n> > >> > + return -ENODEV;\n> > >> > + }\n\nNo need for curly braces.\n\n> > >> > +\n> > >> > + /* configure the metadata node */\n> > >> > + metadata_ = std::make_unique(*metadata);\n> > >> > + ret = metadata_->open();\n> > >>\n> > >> Should we check for validity of metadata_ with a simple if (metadata_)\n> > >> ? or is it overengineering ?\n> > >\n> > > Good call! I think we should.\n\nAs far as I can tell, std::make_unique<>() never returns a null\nstd::unique_ptr<>.\n\n> > >> > + if (ret)\n> > >> > + return ret;\n> > >> > +\n> > >> > + if (!(metadata_->caps().isMeta())) {\n\nNo need for the outer parentheses.\n\n> > >> > + /*\n> > >> > + * UVC Devices are usually only anticipated to expose two\n> > >> > + * devices, so we assume the non-default device is the metadata\n> > >> > + * device node\n> > >> > + */\n> > >>\n> > >> The comment seems misaligned in indentation, please fix it.\n> > >\n> > > I'll make sure it's correct in the next patch.\n\nThis should have been caught by checkstyle.py. Are you using it through\nthe git post-commit hook ?\n\n> > >> > + return -EINVAL;\n> > >> > + }\n> > >> > + return 0;\n> > >> > +}\n> > >> > +\n> > >> > int UVCCameraData::init(MediaDevice *media)\n> > >> > {\n> > >> > int ret;\n> > >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> > >> >\n> > >> > controlInfo_ = ControlInfoMap(std::move(ctrls), controls::controls);\n> > >> >\n> > >> > + ret = initMetadata(media);\n> > >> > + if (ret) {\n> > >> > + metadata_ = nullptr;\n> > >>\n> > >> I think, you are leaking metadata_ if the initMetadata function\n> > >> returns from failure of\n> > >\n> > > I thought assigning null to smart pointers initiates the normal\n> > > cleanup routine of the objects they point to. Is this the leak\n> > > you're talking about, or should I clean up something else too?\n> \n> From https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D\n> \n> (3) unique_ptr& operator=( std::nullptr_t ) noexcept;\n> 3) Effectively the same as calling reset().\n> \n> https://en.cppreference.com/w/cpp/memory/unique_ptr/reset\n> If the old pointer was non-empty, deletes the previously managed\n> object if (old_ptr) get_deleter()(old_ptr).\n> \n> if metadata_ is a unique_ptr, assigning nullptr to it will delete the object\n> it previously managed.\n\nCorrect. I would however move this to the initMetadata() function to\nmake it self-contained, it's not a good practice to make the caller\naware of the internal implementation of the callee.\n\n> > https://godbolt.org/z/rhx886abY\n> >\n> > I tried doing something similar, check the godbolt link, assigning\n> > null has no effect on the allocated memory. (cc: @Laurent Pinchart and\n> > @Kieran Bingham can you guys have a look once)\n> \n> Try to use unique_ptr, shared_ptr is harder to follow as it has to do\n> reference counting. With unique_ptr<> I see\n> \n> callq operator delete(void*)@PLT\n> \n> > >> \n> > >> > + ret = metadata_->open();\n> > >> > + if (ret)\n> > >> > + return ret;\n> > >> \n> > >>\n> > >> > + LOG(UVC, Error) << \"Could not find a metadata video device.\";\n> > >> > + }\n> > >> > +\n> > >> > return 0;\n> > >> > }\n> > >> >\n> > >\n> > > Thanks for the suggestions!","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 EABEFBE080\n\tfor ;\n\tMon, 28 Aug 2023 20:33:30 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 42F9261F18;\n\tMon, 28 Aug 2023 22:33:30 +0200 (CEST)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id F17A960375\n\tfor ;\n\tMon, 28 Aug 2023 22:33:27 +0200 (CEST)","from pendragon.ideasonboard.com\n\t(117.145-247-81.adsl-dyn.isp.belgacom.be [81.247.145.117])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 5596DAF2;\n\tMon, 28 Aug 2023 22:32:06 +0200 (CEST)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693254810;\n\tbh=+v5W2WKDVSEcjXFxtPwzNqPM4msqNiCCbYRPkNhvPN0=;\n\th=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=lUTiHEwMHEzxY7MH+bqR52I3OtBLbcMGN56b8HJvfc8pk5Y/pqRGedl1Iw34lyDzC\n\toyHqVOUlIqlLR9SOSP/FrI3RcA8bXcCkz7ocrAImAcsGCKXwdebdaZOX/3AWjsr8zO\n\tAilQ2ICmklvaD7bqxcCtuh2ASi4rnKdzQ63o6GdAPVp9mqEEwQmKB82V8hk7QUEJeB\n\tT5sUmd1gZ0mW67RSx1selB4C55kjgA3xJRXPZV0Zs74XA/p0xAGG7FRYgKbsZStfdv\n\tnhq5hUb+qZw3ga7jZDnsMWxJnbR1Xf0IXV8/FRIThmXZWuM5cpB73F24EBaD8wME6t\n\tjswxnpprJ3r7w==","v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1693254726;\n\tbh=+v5W2WKDVSEcjXFxtPwzNqPM4msqNiCCbYRPkNhvPN0=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=B/tcrmXmQSXpbia1BVyFk8kNSOWggqJ8myvTxuMPsfb+P4VGhVi1Id57+KYLQF2ku\n\tKQ6o0FBH2LvAdESNzyFxGyvWxRXhRFj3KCX7zZhrANB1DsxcRGiTTZ6O7eXk9MTBx5\n\tWksbmO9J5YsodhhVfMf18Zw4/jeMqNezJg0/5HRM="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (1024-bit key; \n\tunprotected) header.d=ideasonboard.com\n\theader.i=@ideasonboard.com\n\theader.b=\"B/tcrmXm\"; dkim-atps=neutral","Date":"Mon, 28 Aug 2023 23:33:37 +0300","To":"Jacopo Mondi ","Message-ID":"<20230828203337.GG17083@pendragon.ideasonboard.com>","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>\n\t\n\t\n\t\n\t<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","MIME-Version":"1.0","Content-Type":"text/plain; charset=utf-8","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Laurent Pinchart via libcamera-devel\n\t","Reply-To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org,\n\tVedant Paranjape ","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":27710,"web_url":"https://patchwork.libcamera.org/comment/27710/","msgid":"","date":"2023-08-29T17:59:23","subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","submitter":{"id":85,"url":"https://patchwork.libcamera.org/api/people/85/","name":"Vedant Paranjape","email":"vedantparanjape160201@gmail.com"},"content":"Hello Jacopo,\n\nThis is new, thanks for the info. Need to read the fineprint :)\n\nRegards,\nVedant Paranjape\n\nOn Mon, Aug 28, 2023 at 1:46 PM Jacopo Mondi \nwrote:\n\n> Hello\n>\n> On Mon, Aug 28, 2023 at 12:28:49AM +0530, Vedant Paranjape via\n> libcamera-devel wrote:\n> > Hello Gabby,\n> >\n> > On Sun, Aug 27, 2023 at 9:59 AM Gabrielle George \n> wrote:\n> > >\n> > >\n> > >\n> > > On Mon, Aug 21, 2023 at 6:49 AM Vedant Paranjape <\n> vedantparanjape160201@gmail.com> wrote:\n> > >>\n> > >> Hello Gabby,\n> > >>\n> > >> Thanks for your patch.\n> > >>\n> > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George \n> wrote:\n> > >> >\n> > >> > Identify and open the UVC metadata's video node. This will give us\n> > >> > access to metadata associated with the video stream of the uvc\n> camera.\n> > >> > The user will not have access to this video node and will not need\n> to\n> > >> > manage its data in any way.\n> > >> >\n> > >> > Signed-off-by: Gabby George \n> > >> > ---\n> > >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42\n> ++++++++++++++++++++\n> > >> > 1 file changed, 42 insertions(+)\n> > >> >\n> > >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > index 38f48a5d..dbe0cc8c 100644\n> > >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > @@ -49,10 +49,13 @@ public:\n> > >> > const std::string &id() const { return id_; }\n> > >> >\n> > >> > std::unique_ptr video_;\n> > >> > + std::unique_ptr metadata_;\n> > >> > Stream stream_;\n> > >> > std::map> formats_;\n> > >> >\n> > >> > private:\n> > >> > + int initMetadata(MediaDevice *media);\n> > >> > +\n> > >> > bool generateId();\n> > >> >\n> > >> > std::string id_;\n> > >> > @@ -411,6 +414,39 @@ bool\n> PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > >> > return true;\n> > >> > }\n> > >> >\n> > >> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> > >> > +{\n> > >> > + int ret;\n> > >> > +\n> > >> > + const std::vector &entities =\n> media->entities();\n> > >> > + std::string dev_node_name = video_->deviceNode();\n> > >> > + auto metadata = std::find_if(entities.begin(),\n> entities.end(),\n> > >> > + [&dev_node_name](MediaEntity\n> *e) {\n> > >> > + return e->type() ==\n> MediaEntity::Type::V4L2VideoDevice\n> > >> > + &&\n> !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> > >> > + });\n> > >> > +\n> > >> > + if (metadata == entities.end()) {\n> > >> > + return -ENODEV;\n> > >> > + }\n> > >> > +\n> > >> > + /* configure the metadata node */\n> > >> > + metadata_ = std::make_unique(*metadata);\n> > >> > + ret = metadata_->open();\n> > >>\n> > >> Should we check for validity of metadata_ with a simple if (metadata_)\n> > >> ? or is it overengineering ?\n> > >\n> > >\n> > > Good call! I think we should.\n> > >>\n> > >>\n> > >> > + if (ret)\n> > >> > + return ret;\n> > >> > +\n> > >> > + if (!(metadata_->caps().isMeta())) {\n> > >> > + /*\n> > >> > + * UVC Devices are usually only anticipated to\n> expose two\n> > >> > + * devices, so we assume the non-default device is the\n> metadata\n> > >> > + * device node\n> > >> > + */\n> > >>\n> > >> The comment seems misaligned in indentation, please fix it.\n> > >\n> > >\n> > > I'll make sure it's correct in the next patch.\n> > >>\n> > >>\n> > >> > + return -EINVAL;\n> > >> > + }\n> > >> > + return 0;\n> > >> > +}\n> > >> > +\n> > >> > int UVCCameraData::init(MediaDevice *media)\n> > >> > {\n> > >> > int ret;\n> > >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> > >> >\n> > >> > controlInfo_ = ControlInfoMap(std::move(ctrls),\n> controls::controls);\n> > >> >\n> > >> > + ret = initMetadata(media);\n> > >> > + if (ret) {\n> > >> > + metadata_ = nullptr;\n> > >>\n> > >> I think, you are leaking metadata_ if the initMetadata function\n> > >> returns from failure of\n> > >\n> > >\n> > > I thought assigning null to smart pointers initiates the normal\n> cleanup routine of the objects they point to. Is this the leak you're\n> talking about, or should I clean up something else too?\n>\n> From https://en.cppreference.com/w/cpp/memory/unique_ptr/operator%3D\n>\n> (3) unique_ptr& operator=( std::nullptr_t ) noexcept;\n> 3) Effectively the same as calling reset().\n>\n> https://en.cppreference.com/w/cpp/memory/unique_ptr/reset\n> If the old pointer was non-empty, deletes the previously managed\n> object if (old_ptr) get_deleter()(old_ptr).\n>\n> if metadata_ is a unique_ptr, assigning nullptr to it will delete the\n> object\n> it previously managed.\n>\n> >\n> > https://godbolt.org/z/rhx886abY\n> >\n> > I tried doing something similar, check the godbolt link, assigning\n> > null has no effect on the allocated memory. (cc: @Laurent Pinchart and\n> > @Kieran Bingham can you guys have a look once)\n> >\n>\n> Try to use unique_ptr, shared_ptr is harder to follow as it has to do\n> reference counting. With unique_ptr<> I see\n>\n> callq operator delete(void*)@PLT\n>\n> Thanks\n> j\n>\n>\n> > >>\n> > >> \n> > >> > + ret = metadata_->open();\n> > >> > + if (ret)\n> > >> > + return ret;\n> > >> \n> > >>\n> > >> > + LOG(UVC, Error) << \"Could not find a metadata video\n> device.\";\n> > >> > + }\n> > >> > +\n> > >> > return 0;\n> > >> > }\n> > >> >\n> > >> > --\n> > >> > 2.34.1\n> > >> >\n> > >>\n> > >> Regards,\n> > >> Vedant Paranjape\n> > >\n> > >\n> > > Thanks for the suggestions!\n> > >>\n> > >>\n> > >> On Mon, Aug 21, 2023 at 6:40 PM Gabby George \n> wrote:\n> > >> >\n> > >> > Identify and open the UVC metadata's video node. This will give us\n> > >> > access to metadata associated with the video stream of the uvc\n> camera.\n> > >> > The user will not have access to this video node and will not need\n> to\n> > >> > manage its data in any way.\n> > >> >\n> > >> > Signed-off-by: Gabby George \n> > >> > ---\n> > >> > src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 42\n> ++++++++++++++++++++\n> > >> > 1 file changed, 42 insertions(+)\n> > >> >\n> > >> > diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > index 38f48a5d..dbe0cc8c 100644\n> > >> > --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp\n> > >> > @@ -49,10 +49,13 @@ public:\n> > >> > const std::string &id() const { return id_; }\n> > >> >\n> > >> > std::unique_ptr video_;\n> > >> > + std::unique_ptr metadata_;\n> > >> > Stream stream_;\n> > >> > std::map> formats_;\n> > >> >\n> > >> > private:\n> > >> > + int initMetadata(MediaDevice *media);\n> > >> > +\n> > >> > bool generateId();\n> > >> >\n> > >> > std::string id_;\n> > >> > @@ -411,6 +414,39 @@ bool\n> PipelineHandlerUVC::match(DeviceEnumerator *enumerator)\n> > >> > return true;\n> > >> > }\n> > >> >\n> > >> > +int UVCCameraData::initMetadata(MediaDevice *media)\n> > >> > +{\n> > >> > + int ret;\n> > >> > +\n> > >> > + const std::vector &entities =\n> media->entities();\n> > >> > + std::string dev_node_name = video_->deviceNode();\n> > >> > + auto metadata = std::find_if(entities.begin(),\n> entities.end(),\n> > >> > + [&dev_node_name](MediaEntity\n> *e) {\n> > >> > + return e->type() ==\n> MediaEntity::Type::V4L2VideoDevice\n> > >> > + &&\n> !(e->flags() & MEDIA_ENT_FL_DEFAULT);\n> > >> > + });\n> > >> > +\n> > >> > + if (metadata == entities.end()) {\n> > >> > + return -ENODEV;\n> > >> > + }\n> > >> > +\n> > >> > + /* configure the metadata node */\n> > >> > + metadata_ = std::make_unique(*metadata);\n> > >> > + ret = metadata_->open();\n> > >> > + if (ret)\n> > >> > + return ret;\n> > >> > +\n> > >> > + if (!(metadata_->caps().isMeta())) {\n> > >> > + /*\n> > >> > + * UVC Devices are usually only anticipated to\n> expose two\n> > >> > + * devices, so we assume the non-default device is the\n> metadata\n> > >> > + * device node\n> > >> > + */\n> > >> > + return -EINVAL;\n> > >> > + }\n> > >> > + return 0;\n> > >> > +}\n> > >> > +\n> > >> > int UVCCameraData::init(MediaDevice *media)\n> > >> > {\n> > >> > int ret;\n> > >> > @@ -512,6 +548,12 @@ int UVCCameraData::init(MediaDevice *media)\n> > >> >\n> > >> > controlInfo_ = ControlInfoMap(std::move(ctrls),\n> controls::controls);\n> > >> >\n> > >> > + ret = initMetadata(media);\n> > >> > + if (ret) {\n> > >> > + metadata_ = nullptr;\n> > >> > + LOG(UVC, Error) << \"Could not find a metadata video\n> device.\";\n> > >> > + }\n> > >> > +\n> > >> > return 0;\n> > >> > }\n> > >> >\n> > >> > --\n> > >> > 2.34.1\n> > >> >\n> >\n> > Regards,\n> > Vedant Paranjape\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 59EFABE080\n\tfor ;\n\tTue, 29 Aug 2023 17:59:38 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 9A176627E0;\n\tTue, 29 Aug 2023 19:59:37 +0200 (CEST)","from mail-ot1-x32b.google.com (mail-ot1-x32b.google.com\n\t[IPv6:2607:f8b0:4864:20::32b])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id DFD6861E02\n\tfor ;\n\tTue, 29 Aug 2023 19:59:36 +0200 (CEST)","by mail-ot1-x32b.google.com with SMTP id\n\t46e09a7af769-6bd3317144fso3743473a34.1\n\tfor ;\n\tTue, 29 Aug 2023 10:59:36 -0700 (PDT)"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/simple; d=libcamera.org;\n\ts=mail; t=1693331977;\n\tbh=ICWGFmqlkDACWC0pr4zpO6XnKmMBDNDECEme8lypxYA=;\n\th=References:In-Reply-To:Date:To:Subject:List-Id:List-Unsubscribe:\n\tList-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc:\n\tFrom;\n\tb=eAyYOXjOje9ZRgi8ppHZXtxH9FT0YuAdu9a42ihxgU3jcqXxKMUEcGQ4iP15g8FaX\n\tdO2ChwPko+ffNuXxXWjyXapQAFsNhmUR5RqLh7m1mUynH+24uZkNioYTXKZf0Dy2Zx\n\thoJD8Hwu8QCwIqbyhnwaz8RLDLIgpL3ky7yXjfnGpTLgR1V0fAUBS5kxMhi96MpnQ/\n\tbijjC2dhBk8L914fQm8vBWETSrwrpP/RbSjn1d8/hXhz+Az/1ISjAZKRx/FS21YwA6\n\tlxbbF3c2oWsd/vAL9HlVaJtWFWIkrj9YKvdjzbY0LZwEOQyN7CpdTH5/+/qi33NXbv\n\tHD5xHHuVz/kYA==","v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=gmail.com; s=20221208; t=1693331975; x=1693936775;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:from:to:cc:subject:date:message-id:reply-to;\n\tbh=PuWm9AcRylhQC+mvBgYxjhNMlFrn8l7pH+w317pLaUA=;\n\tb=fW+Qxz7rywXFvm/euyG2hClxQB0mFQRE8Luw904+4JJg9UAC7i5Rg7BydJFqq9WtBs\n\t2FL1Oc3j1SxYA5r61fPmySN1btvJtCexhwudh4Ca4zX4ZKVUvV2jbjZcXXTTxzdAgV/n\n\tPRxIp6qn2mQcDQl1t1O8PQ8EGoSftWG9HUZCtJPKg7QdDmhi/TgIyupsHh9lu/+wn5jL\n\tFpJHifC0+gDRcwFGb0KuLjJD5age26cH+uu+WshBTguNYDMxKhZ5EkUj5/F0ZuNinYK8\n\tE4YZzDgLdqUBp0eBcasrsuHpiu9wljAUZJ4x43ZOvpu4tEboBpDed3kyMXt/FfhRT8FL\n\tNkgw=="],"Authentication-Results":"lancelot.ideasonboard.com; dkim=pass (2048-bit key; \n\tunprotected) header.d=gmail.com header.i=@gmail.com\n\theader.b=\"fW+Qxz7r\"; dkim-atps=neutral","X-Google-DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/relaxed;\n\td=1e100.net; s=20221208; t=1693331975; x=1693936775;\n\th=cc:to:subject:message-id:date:from:in-reply-to:references\n\t:mime-version:x-gm-message-state:from:to:cc:subject:date:message-id\n\t:reply-to;\n\tbh=PuWm9AcRylhQC+mvBgYxjhNMlFrn8l7pH+w317pLaUA=;\n\tb=SfidIjAzoZL3yzb29pJkwXoduRpYEH1mxJvHzo5xTSmK134B2Al23gnWg2OAn127fS\n\tV3DZwsGnDA9lC1Xipw1AWkQuiwVw1kvCQrwriza6g1pUXnLCL9kWSnXUTh9w7awm6VIL\n\tMruKXJ71L9ZUIEujwE5jDNmG4UuNc/reYj9vrWWjbpAmDQHKsZ3lDA3Y+aXzwN0SCx7V\n\tzQXku7JbMrKpWTrtckpM7ZgAsL6fIuFdQZnaNIbKMXHzhUWI3W9WJA2lRVkBMau2/T4h\n\tooti9UPsvPXNoB6bOAOgk3NzLg20CEyq2lAdjkRYIAmD2xuzGlEUfWEDeCrhb8l80rHZ\n\thTaw==","X-Gm-Message-State":"AOJu0Yx2YRWewuydzfxSKuqPf68EvA9D7HAjGpbiGc0q/dtDc4ouCSYR\n\tT3rZLg78hAngXleR9bCQDCMJyltD47vSryjauk4=","X-Google-Smtp-Source":"AGHT+IHQm+FQZFzPOmqnpyqy6ujU0nvyiUejZDe/uLnnrYwxnROASO/DiIMl8+22DmGYsjqp5ZHjhkRhqM3CwC1dNiE=","X-Received":"by 2002:a05:6870:6391:b0:1bf:a95:7a3f with SMTP id\n\tt17-20020a056870639100b001bf0a957a3fmr17493017oap.54.1693331975144;\n\tTue, 29 Aug 2023 10:59:35 -0700 (PDT)","MIME-Version":"1.0","References":"<20230821131039.127370-1-gabbymg94@gmail.com>\n\t<20230821131039.127370-2-gabbymg94@gmail.com>\n\t\n\t\n\t\n\t<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","In-Reply-To":"<4xhkpabnkl77tnlxyscmcd7zk2xk6xsm2mvoefggbj5n4msmoe@oh2vebhfv4kh>","Date":"Tue, 29 Aug 2023 23:29:23 +0530","Message-ID":"","To":"Jacopo Mondi ","Content-Type":"multipart/alternative; boundary=\"000000000000dd8c8106041392e0\"","Subject":"Re: [libcamera-devel] [RFC PATCH v2 1/5] libcamera: pipeline:\n\tuvcvideo: Add UVC metadata node","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","From":"Vedant Paranjape via libcamera-devel\n\t","Reply-To":"Vedant Paranjape ","Cc":"libcamera-devel@lists.libcamera.org","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]