[{"id":80,"web_url":"https://patchwork.libcamera.org/comment/80/","msgid":"<20181224105530.GD2364@archlinux>","date":"2018-12-24T10:55:30","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Niklas,\n as I've commented on 00/12, I think the DeviceMatcher should be\nreplaced by a MediaDeviceDesc and have the MediaDevice do the matching\non it. Nonethless, I have some questions here, as they might apply\ngenerally to all our code base.\n\nOn Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> Provide a DeviceMatch class which represents all properties of a media\n> device a pipeline hander can specify when searching for a device to use\n> in its pipeline.\n>\n> Signed-off-by: Niklas Söderlund \n> ---\n> src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> 2 files changed, 71 insertions(+)\n>\n> diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> index 7c44a65b45472ef3..17b6874d229c791c 100644\n> --- a/src/libcamera/deviceenumerator.cpp\n> +++ b/src/libcamera/deviceenumerator.cpp\n> @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const\n> \treturn true;\n> }\n>\n> +/* -----------------------------------------------------------------------------\n> + * DeviceMatch\n> + */\n> +\n> +DeviceMatch::DeviceMatch(const std::string &driver)\n> +\t: driver_(driver)\n> +{\n> +}\n\nIs it worth placing constructors with list initializers only in the\n.cpp file?\n\n> +\n> +void DeviceMatch::add(const std::string &entity)\n> +{\n> +\tentities_.push_back(std::string(entity));\n> +}\n> +\n> +bool DeviceMatch::match(const DeviceInfo *info) const\n> +{\n> +\tstd::vector entities;\n\nThat's weird. This is clearly unused, but the compiler does not\ncomplain. Actually I can keep it or remove it without any message from\ng++. Was it here intentionally?\n\n> +\n> +\tif (!matchInfo(info->info()))\n> +\t\treturn false;\n> +\n> +\tif (!matchEntities(info->entities()))\n> +\t\treturn false;\n> +\n> +\treturn true;\n> +}\n> +\n> +bool DeviceMatch::matchInfo(const struct media_device_info &info) const\n> +{\n> +\t/* TODO: Add more optinal matching pairs from struct media_device_info */\noptional\n> +\t/* TODO: Allow for empty driver in DeviceMatch */\n> +\treturn driver_ == info.driver;\n> +}\n> +\n> +bool DeviceMatch::matchEntities(const std::vector &entities) const\n> +{\n> +\tfor (const std::string &name : entities_) {\n> +\t\tbool found = false;\n> +\n> +\t\tfor (const std::string &entity : entities) {\n> +\nnit: unnecessary empty line\n\n> +\t\t\tif (name == entity) {\n> +\t\t\t\tfound = true;\n> +\t\t\t\tbreak;\n> +\t\t\t}\n> +\t\t}\n> +\n> +\t\tif (!found)\n> +\t\t\treturn false;\n> +\t}\n> +\n> +\treturn true;\n> +}\n> +\n> } /* namespace libcamera */\n> diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644\n> --- a/src/libcamera/include/deviceenumerator.h\n> +++ b/src/libcamera/include/deviceenumerator.h\n> @@ -39,6 +39,23 @@ private:\n> \tstd::map entities_;\n> };\n>\n> +class DeviceMatch\n> +{\n> +public:\n> +\tDeviceMatch(const std::string &driver);\n> +\n> +\tvoid add(const std::string &entity);\n> +\n> +\tbool match(const DeviceInfo *info) const;\n> +\n> +private:\n> +\tstd::string driver_;\n> +\tstd::vector entities_;\n> +\n> +\tbool matchInfo(const struct media_device_info &info) const;\n> +\tbool matchEntities(const std::vector &entities) const;\n> +};\n> +\n> } /* namespace libcamera */\n>\n> #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n\nThis is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)\n> --\n> 2.20.1\n>\n> _______________________________________________\n> libcamera-devel mailing list\n> libcamera-devel@lists.libcamera.org\n> https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net\n\t[217.70.183.196])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 03F8E60B0B\n\tfor ;\n\tMon, 24 Dec 2018 11:55:31 +0100 (CET)","from archlinux (host54-51-dynamic.16-87-r.retail.telecomitalia.it\n\t[87.16.51.54]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 373A6E000D;\n\tMon, 24 Dec 2018 10:55:31 +0000 (UTC)"],"X-Originating-IP":"87.16.51.54","Date":"Mon, 24 Dec 2018 11:55:30 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181224105530.GD2364@archlinux>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-5-niklas.soderlund@ragnatech.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"jL2BoiuKMElzg3CS\"","Content-Disposition":"inline","In-Reply-To":"<20181222230041.29999-5-niklas.soderlund@ragnatech.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Mon, 24 Dec 2018 10:55:32 -0000"}},{"id":87,"web_url":"https://patchwork.libcamera.org/comment/87/","msgid":"<20181228031813.GQ19796@bigcity.dyn.berto.se>","date":"2018-12-28T03:18:14","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","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 feedback,\n\nOn 2018-12-24 11:55:30 +0100, Jacopo Mondi wrote:\n> Hi Niklas,\n> as I've commented on 00/12, I think the DeviceMatcher should be\n> replaced by a MediaDeviceDesc and have the MediaDevice do the matching\n> on it. Nonethless, I have some questions here, as they might apply\n> generally to all our code base.\n\nSee comment in 00/12.\n\n> \n> On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> > Provide a DeviceMatch class which represents all properties of a media\n> > device a pipeline hander can specify when searching for a device to use\n> > in its pipeline.\n> >\n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> > src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> > 2 files changed, 71 insertions(+)\n> >\n> > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp\n> > index 7c44a65b45472ef3..17b6874d229c791c 100644\n> > --- a/src/libcamera/deviceenumerator.cpp\n> > +++ b/src/libcamera/deviceenumerator.cpp\n> > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const\n> > \treturn true;\n> > }\n> >\n> > +/* -----------------------------------------------------------------------------\n> > + * DeviceMatch\n> > + */\n> > +\n> > +DeviceMatch::DeviceMatch(const std::string &driver)\n> > +\t: driver_(driver)\n> > +{\n> > +}\n> \n> Is it worth placing constructors with list initializers only in the\n> .cpp file?\n\nWhy not?\n\n> \n> > +\n> > +void DeviceMatch::add(const std::string &entity)\n> > +{\n> > +\tentities_.push_back(std::string(entity));\n> > +}\n> > +\n> > +bool DeviceMatch::match(const DeviceInfo *info) const\n> > +{\n> > +\tstd::vector entities;\n> \n> That's weird. This is clearly unused, but the compiler does not\n> complain. Actually I can keep it or remove it without any message from\n> g++. Was it here intentionally?\n\n My bad it was part of a previous version of the code and should be \n removed. Great catch I wonder why g++ do not complain about it.\n\n> \n> > +\n> > +\tif (!matchInfo(info->info()))\n> > +\t\treturn false;\n> > +\n> > +\tif (!matchEntities(info->entities()))\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const\n> > +{\n> > +\t/* TODO: Add more optinal matching pairs from struct media_device_info */\n> optional\n\nThanks, this is fixed in the documentation patch but no reason to not be \ncorrect in the first place.\n\n> > +\t/* TODO: Allow for empty driver in DeviceMatch */\n> > +\treturn driver_ == info.driver;\n> > +}\n> > +\n> > +bool DeviceMatch::matchEntities(const std::vector &entities) const\n> > +{\n> > +\tfor (const std::string &name : entities_) {\n> > +\t\tbool found = false;\n> > +\n> > +\t\tfor (const std::string &entity : entities) {\n> > +\n> nit: unnecessary empty line\n\nThanks.\n\n\n> \n> > +\t\t\tif (name == entity) {\n> > +\t\t\t\tfound = true;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!found)\n> > +\t\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > } /* namespace libcamera */\n> > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h\n> > index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644\n> > --- a/src/libcamera/include/deviceenumerator.h\n> > +++ b/src/libcamera/include/deviceenumerator.h\n> > @@ -39,6 +39,23 @@ private:\n> > \tstd::map entities_;\n> > };\n> >\n> > +class DeviceMatch\n> > +{\n> > +public:\n> > +\tDeviceMatch(const std::string &driver);\n> > +\n> > +\tvoid add(const std::string &entity);\n> > +\n> > +\tbool match(const DeviceInfo *info) const;\n> > +\n> > +private:\n> > +\tstd::string driver_;\n> > +\tstd::vector entities_;\n> > +\n> > +\tbool matchInfo(const struct media_device_info &info) const;\n> > +\tbool matchEntities(const std::vector &entities) const;\n> > +};\n> > +\n> > } /* namespace libcamera */\n> >\n> > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> \n> This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)\n\nYes, thanks for spotting this.\n\n\n> > --\n> > 2.20.1\n> >\n> > _______________________________________________\n> > libcamera-devel mailing list\n> > libcamera-devel@lists.libcamera.org\n> > https://lists.libcamera.org/listinfo/libcamera-devel","headers":{"Return-Path":"","Received":["from mail-lf1-x141.google.com (mail-lf1-x141.google.com\n\t[IPv6:2a00:1450:4864:20::141])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id B8CE8600CC\n\tfor ;\n\tFri, 28 Dec 2018 04:18:15 +0100 (CET)","by mail-lf1-x141.google.com with SMTP id p6so13828430lfc.1\n\tfor ;\n\tThu, 27 Dec 2018 19:18:15 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\ta2-v6sm8462240lji.13.2018.12.27.19.18.14\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tThu, 27 Dec 2018 19:18:14 -0800 (PST)"],"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\t:user-agent; bh=8E4KYITKHQKlbOVG7HVEX0xtaOJep+q4DNbj6DbNQRc=;\n\tb=MHCWabaVhJ8occSxYHryht+TJhhnwcMf+t8zNyRBba0Dx5LjjeqP3yr8I0LT3L678c\n\tgmtDT2Mdmwds+CaAx4ERzdEOQTgHlDq3qFXxiTKmeOlVdZR4Rn7LqAW0mY9Rk2cuYgpY\n\t3dIjXR9p9XYwgpD5CEYpSXFJWJz6FlWDF1KB4E65IMotIFoGQ4itiThFQRKqhcFNYlM9\n\tExHr37sWJZnt0EXli6BWnBvL2OCSKD9b8bcMGPVmM+PNayziZ/cj0Xmv0+uA0MYyumRg\n\tV9+86HE/0DwcLoadg7r33JX877WdIEmf7LCSypf5GKoZd7v0wIMwJnduxJ+w/5NV7rur\n\t9/0Q==","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:user-agent;\n\tbh=8E4KYITKHQKlbOVG7HVEX0xtaOJep+q4DNbj6DbNQRc=;\n\tb=U5YOIj+1PZJmLIjbe2mkrl2qDEqG1Z66O4H7sGBJV7pGqEsopilFb9dWm5IPPVFnW6\n\tgi+2ZiKoAagAeAsMa+CDz9UQ2fwvnRMyIgy2sVf5IXZtmmzDF93qXtla/Hx7iW5VWuy9\n\tU7dm6h74cAeD/fsU/JJ439bqEJv1MCNUr3ytvbcaTLy4LUfsjylCbH967asBY4y9Kkwj\n\tNAhMAJfxon7AwwgoUGZRA0GOP2LVE9Gz9aA+57RmRwt8FB+uT40R9ZLuDO67A8eksbzv\n\t5SbF1P/JTEQFzG95Y0IlV8eDQR9c2bAYDFS4OaChAF6hWWCTwiUKZ2PF32QGPVliFGkt\n\tPkpg==","X-Gm-Message-State":"AA+aEWbvX32DciuQS4mnBfb8Ylr05oKo5WAIDMdyTBY3oLLLXFR73ydG\n\tUwiQ/vgVE6N0qGH/DlMY0L2oJZ17juM=","X-Google-Smtp-Source":"AFSGD/WTh3eQOdL0ZS+D7Pm5PPbmWGQPPdS/HLlHJOEe3+y+YdLoEmSCaJbNl0iAN81UQlbiqD2/Og==","X-Received":"by 2002:ac2:4343:: with SMTP id\n\to3mr13604818lfl.129.1545967094972; \n\tThu, 27 Dec 2018 19:18:14 -0800 (PST)","Date":"Fri, 28 Dec 2018 04:18:14 +0100","From":"Niklas S??derlund ","To":"Jacopo Mondi ","Cc":"libcamera-devel@lists.libcamera.org","Message-ID":"<20181228031813.GQ19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-5-niklas.soderlund@ragnatech.se>\n\t<20181224105530.GD2364@archlinux>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<20181224105530.GD2364@archlinux>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 03:18:16 -0000"}},{"id":105,"web_url":"https://patchwork.libcamera.org/comment/105/","msgid":"<3747490.e0YOlbGKG1@avalon>","date":"2018-12-28T22:02:41","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote:\n> Hi Niklas,\n> as I've commented on 00/12, I think the DeviceMatcher should be\n> replaced by a MediaDeviceDesc and have the MediaDevice do the matching\n> on it. Nonethless, I have some questions here, as they might apply\n> generally to all our code base.\n\nHow about doing it the other way around, having the match() function in the \nMediaDeviceDesc class ? That would keep MediaDevice focused on handling media \ndevices, and MediaDeviceDesc on handling matching. We could perhaps rename \nMediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name \nas such, but I don't really like abbreviating Desc).\n\n> On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> > Provide a DeviceMatch class which represents all properties of a media\n> > device a pipeline hander can specify when searching for a device to use\n> > in its pipeline.\n> > \n> > Signed-off-by: Niklas Söderlund \n> > ---\n> > \n> > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> > src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> > 2 files changed, 71 insertions(+)\n> > \n> > diff --git a/src/libcamera/deviceenumerator.cpp\n> > b/src/libcamera/deviceenumerator.cpp index\n> > 7c44a65b45472ef3..17b6874d229c791c 100644\n> > --- a/src/libcamera/deviceenumerator.cpp\n> > +++ b/src/libcamera/deviceenumerator.cpp\n> > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name,\n> > std::string &devnode) const\n> > \treturn true;\n> > }\n> > \n> > +/* ----------------------------------------------------------------------\n> > + * DeviceMatch\n> > + */\n> > +\n> > +DeviceMatch::DeviceMatch(const std::string &driver)\n> > +\t: driver_(driver)\n> > +{\n> > +}\n> \n> Is it worth placing constructors with list initializers only in the\n> .cpp file?\n> \n> > +\n> > +void DeviceMatch::add(const std::string &entity)\n> > +{\n> > +\tentities_.push_back(std::string(entity));\n\nDoesn't entities_.push_back(entity); work as expected ?\n\n> > +}\n> > +\n> > +bool DeviceMatch::match(const DeviceInfo *info) const\n> > +{\n> > +\tstd::vector entities;\n> \n> That's weird. This is clearly unused, but the compiler does not\n> complain. Actually I can keep it or remove it without any message from\n> g++. Was it here intentionally?\n> \n> > +\n> > +\tif (!matchInfo(info->info()))\n> > +\t\treturn false;\n> > +\n> > +\tif (!matchEntities(info->entities()))\n> > +\t\treturn false;\n> > +\n> > +\treturn true;\n\nGiven how simple the code is I would inline both matchInfo() and \nmatchEntities() here.\n\n> > +}\n> > +\n> > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const\n> > +{\n> > +\t/* TODO: Add more optinal matching pairs from struct media_device_info\n> > */\n> \n> optional\n> \n> > +\t/* TODO: Allow for empty driver in DeviceMatch */\n> > +\treturn driver_ == info.driver;\n> > +}\n> > +\n> > +bool DeviceMatch::matchEntities(const std::vector &entities)\n> > const\n> > +{\n> > +\tfor (const std::string &name : entities_) {\n> > +\t\tbool found = false;\n> > +\n> > +\t\tfor (const std::string &entity : entities) {\n> > +\n> \n> nit: unnecessary empty line\n> \n> > +\t\t\tif (name == entity) {\n> > +\t\t\t\tfound = true;\n> > +\t\t\t\tbreak;\n> > +\t\t\t}\n> > +\t\t}\n> > +\n> > +\t\tif (!found)\n> > +\t\t\treturn false;\n> > +\t}\n> > +\n> > +\treturn true;\n> > +}\n> > +\n> > \n> > } /* namespace libcamera */\n> > \n> > diff --git a/src/libcamera/include/deviceenumerator.h\n> > b/src/libcamera/include/deviceenumerator.h index\n> > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644\n> > --- a/src/libcamera/include/deviceenumerator.h\n> > +++ b/src/libcamera/include/deviceenumerator.h\n> > @@ -39,6 +39,23 @@ private:\n> > \tstd::map entities_;\n> > };\n> > \n> > +class DeviceMatch\n> > +{\n> > +public:\n> > +\tDeviceMatch(const std::string &driver);\n> > +\n> > +\tvoid add(const std::string &entity);\n> > +\n> > +\tbool match(const DeviceInfo *info) const;\n> > +\n> > +private:\n> > +\tstd::string driver_;\n> > +\tstd::vector entities_;\n> > +\n> > +\tbool matchInfo(const struct media_device_info &info) const;\n> > +\tbool matchEntities(const std::vector &entities) const;\n> > +};\n> > +\n> > \n> > } /* namespace libcamera */\n> > \n> > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> \n> This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)","headers":{"Return-Path":"","Received":["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 43CAF60B2E\n\tfor ;\n\tFri, 28 Dec 2018 23:01:45 +0100 (CET)","from avalon.localnet (unknown\n\t[IPv6:2a02:2788:66a:3eb:2624:a446:f4b7:b19d])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B5D7ADF;\n\tFri, 28 Dec 2018 23:01:44 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546034504;\n\tbh=dtK5A4aNLGNsctih0g7JqJV25gOxuSrqUEsjR6Wfwrk=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=ZIHbX/2ry6uD3lUrZklbp0FWt6da4jqc6v+L52/AVJgcsQTPBtmUqi9YI+/cFzBtK\n\tVa6+NDEQpxvFWpEufKZ72tsNUEdT1L+dF37DXsXYYWIWqTJ2TTf25F66GRsEO9j1l5\n\tf8K3h6au5yh6U7a+EwmwjXvrC5wf5GC4W4OtynAk=","From":"Laurent Pinchart ","To":"libcamera-devel@lists.libcamera.org","Date":"Sat, 29 Dec 2018 00:02:41 +0200","Message-ID":"<3747490.e0YOlbGKG1@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181224105530.GD2364@archlinux>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-5-niklas.soderlund@ragnatech.se>\n\t<20181224105530.GD2364@archlinux>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Fri, 28 Dec 2018 22:01:45 -0000"}},{"id":114,"web_url":"https://patchwork.libcamera.org/comment/114/","msgid":"<20181229010624.GA19796@bigcity.dyn.berto.se>","date":"2018-12-29T01:06:24","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","submitter":{"id":5,"url":"https://patchwork.libcamera.org/api/people/5/","name":"Niklas Söderlund","email":"niklas.soderlund@ragnatech.se"},"content":"Hi Laurent,\n\nThanks for your feedback.\n\nOn 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote:\n> On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote:\n> > Hi Niklas,\n> > as I've commented on 00/12, I think the DeviceMatcher should be\n> > replaced by a MediaDeviceDesc and have the MediaDevice do the matching\n> > on it. Nonethless, I have some questions here, as they might apply\n> > generally to all our code base.\n> \n> How about doing it the other way around, having the match() function in the \n> MediaDeviceDesc class ? That would keep MediaDevice focused on handling media \n> devices, and MediaDeviceDesc on handling matching. We could perhaps rename \n> MediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name \n> as such, but I don't really like abbreviating Desc).\n\nI agree it would be good if the MedaDevice could focus on operating \nmedia device and have the enumeration and matching in other objects.\n\n> \n> > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> > > Provide a DeviceMatch class which represents all properties of a media\n> > > device a pipeline hander can specify when searching for a device to use\n> > > in its pipeline.\n> > > \n> > > Signed-off-by: Niklas Söderlund \n> > > ---\n> > > \n> > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> > > src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> > > 2 files changed, 71 insertions(+)\n> > > \n> > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > b/src/libcamera/deviceenumerator.cpp index\n> > > 7c44a65b45472ef3..17b6874d229c791c 100644\n> > > --- a/src/libcamera/deviceenumerator.cpp\n> > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name,\n> > > std::string &devnode) const\n> > > \treturn true;\n> > > }\n> > > \n> > > +/* ----------------------------------------------------------------------\n> > > + * DeviceMatch\n> > > + */\n> > > +\n> > > +DeviceMatch::DeviceMatch(const std::string &driver)\n> > > +\t: driver_(driver)\n> > > +{\n> > > +}\n> > \n> > Is it worth placing constructors with list initializers only in the\n> > .cpp file?\n> > \n> > > +\n> > > +void DeviceMatch::add(const std::string &entity)\n> > > +{\n> > > +\tentities_.push_back(std::string(entity));\n> \n> Doesn't entities_.push_back(entity); work as expected ?\n\nIt does, this is a mistake on my side. Thanks for pointing it out.\n\n> \n> > > +}\n> > > +\n> > > +bool DeviceMatch::match(const DeviceInfo *info) const\n> > > +{\n> > > +\tstd::vector entities;\n> > \n> > That's weird. This is clearly unused, but the compiler does not\n> > complain. Actually I can keep it or remove it without any message from\n> > g++. Was it here intentionally?\n> > \n> > > +\n> > > +\tif (!matchInfo(info->info()))\n> > > +\t\treturn false;\n> > > +\n> > > +\tif (!matchEntities(info->entities()))\n> > > +\t\treturn false;\n> > > +\n> > > +\treturn true;\n> \n> Given how simple the code is I would inline both matchInfo() and \n> matchEntities() here.\n\nOK I yield :-) Will inline in v2.\n\n> \n> > > +}\n> > > +\n> > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const\n> > > +{\n> > > +\t/* TODO: Add more optinal matching pairs from struct media_device_info\n> > > */\n> > \n> > optional\n> > \n> > > +\t/* TODO: Allow for empty driver in DeviceMatch */\n> > > +\treturn driver_ == info.driver;\n> > > +}\n> > > +\n> > > +bool DeviceMatch::matchEntities(const std::vector &entities)\n> > > const\n> > > +{\n> > > +\tfor (const std::string &name : entities_) {\n> > > +\t\tbool found = false;\n> > > +\n> > > +\t\tfor (const std::string &entity : entities) {\n> > > +\n> > \n> > nit: unnecessary empty line\n> > \n> > > +\t\t\tif (name == entity) {\n> > > +\t\t\t\tfound = true;\n> > > +\t\t\t\tbreak;\n> > > +\t\t\t}\n> > > +\t\t}\n> > > +\n> > > +\t\tif (!found)\n> > > +\t\t\treturn false;\n> > > +\t}\n> > > +\n> > > +\treturn true;\n> > > +}\n> > > +\n> > > \n> > > } /* namespace libcamera */\n> > > \n> > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > b/src/libcamera/include/deviceenumerator.h index\n> > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644\n> > > --- a/src/libcamera/include/deviceenumerator.h\n> > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > @@ -39,6 +39,23 @@ private:\n> > > \tstd::map entities_;\n> > > };\n> > > \n> > > +class DeviceMatch\n> > > +{\n> > > +public:\n> > > +\tDeviceMatch(const std::string &driver);\n> > > +\n> > > +\tvoid add(const std::string &entity);\n> > > +\n> > > +\tbool match(const DeviceInfo *info) const;\n> > > +\n> > > +private:\n> > > +\tstd::string driver_;\n> > > +\tstd::vector entities_;\n> > > +\n> > > +\tbool matchInfo(const struct media_device_info &info) const;\n> > > +\tbool matchEntities(const std::vector &entities) const;\n> > > +};\n> > > +\n> > > \n> > > } /* namespace libcamera */\n> > > \n> > > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> > \n> > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)\n> \n> -- \n> Regards,\n> \n> Laurent Pinchart\n> \n> \n>","headers":{"Return-Path":"","Received":["from mail-lf1-x142.google.com (mail-lf1-x142.google.com\n\t[IPv6:2a00:1450:4864:20::142])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id C5D7B60B30\n\tfor ;\n\tSat, 29 Dec 2018 02:06:26 +0100 (CET)","by mail-lf1-x142.google.com with SMTP id i26so15494499lfc.0\n\tfor ;\n\tFri, 28 Dec 2018 17:06:26 -0800 (PST)","from localhost (89-233-230-99.cust.bredband2.com. [89.233.230.99])\n\tby smtp.gmail.com with ESMTPSA id\n\tj12-v6sm8770318ljh.66.2018.12.28.17.06.25\n\t(version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256);\n\tFri, 28 Dec 2018 17:06:25 -0800 (PST)"],"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\t:user-agent; bh=zOwRX44buZaB8oMOSGl5cF4q9kfp9PWKk3v/jzLFI6A=;\n\tb=qLrGzzEtmYhHL2euR0Gm/0ZtAwRS4NWP3IYDoqlPA7VB0H9DlmqOttv6VrpCJIf+QM\n\thryTwvXnL7dtW7+CHNvyI8ZkMp3ZFzeajwDo/+K7kqLMVHSubGgpFZU8oXbdrKdrdxfC\n\tfbg6MBYMX9wXWrQcFldpPFCmTTf/5eoOX8SwvGI2ORBgPgf0AQG4OS1rNkSwJdZ9XYVZ\n\tRlK2z1PJmokALfk+fvUkQRcnZzVALlGe2pqOCLcUxwPJBMn5sKE/+I1r6XNPRY9eGfXZ\n\tlWgwND/D99z+3fpbSzuEIICiDOwc05ETLNwxsneV09g1mKQRXRmnuhBWHKTHAVWFfMq3\n\tofvg==","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:user-agent;\n\tbh=zOwRX44buZaB8oMOSGl5cF4q9kfp9PWKk3v/jzLFI6A=;\n\tb=OwRJjbD0naoyZbSaWoj/ZYu85B5bqiQiP/xN/VwIAfBT/RSayhHNYa72jQYudkG6kR\n\t5DKxCoCWdBB6KEV6K/dniM7Kt6X6vdgBKDM2yWBXUuywMjuhTT914VYRzbau+6Hw7shh\n\tXz8zPPwJf8P316nOKjXpBAC6YDyDLjdTztc+X+rLRvenTMdD4lFN3uokvy4jdH79TlH7\n\tSnxUkvDu8Ii093B4akoEzzhieD4lf7ofPd4L1r3DO+/uYc8KiCmJ5bHI6LwniY8+TWC2\n\tFRayhKrfN/vcJOumyp9iVA5mpNQL1ohD/3OFeT7fjh+1JQ1zHa35KBlvhOkXk7FjoWAb\n\tx8dg==","X-Gm-Message-State":"AA+aEWZ9j4hKe4uEKYhZH+0kHKYsFVls2JGBDYTKngiqYSGr2GPJRd1u\n\tU+/nCjLt2KizMZYb7SAELTPJLKjV99Q=","X-Google-Smtp-Source":"AFSGD/WqoYOYPPS14pdWwnegDO3nhAGdycfuTC/1icM/B89/SuxjJJTBt5rK1Ge10o1WOjtjT6yEXA==","X-Received":"by 2002:a19:d486:: with SMTP id\n\tl128mr14030480lfg.114.1546045586077; \n\tFri, 28 Dec 2018 17:06:26 -0800 (PST)","Date":"Sat, 29 Dec 2018 02:06:24 +0100","From":"Niklas S??derlund ","To":"Laurent Pinchart ","Cc":"libcamera-devel@lists.libcamera.org, Jacopo Mondi ","Message-ID":"<20181229010624.GA19796@bigcity.dyn.berto.se>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-5-niklas.soderlund@ragnatech.se>\n\t<20181224105530.GD2364@archlinux> <3747490.e0YOlbGKG1@avalon>","MIME-Version":"1.0","Content-Type":"text/plain; charset=iso-8859-1","Content-Disposition":"inline","Content-Transfer-Encoding":"8bit","In-Reply-To":"<3747490.e0YOlbGKG1@avalon>","User-Agent":"Mutt/1.10.1 (2018-07-13)","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Dec 2018 01:06:27 -0000"}},{"id":122,"web_url":"https://patchwork.libcamera.org/comment/122/","msgid":"<20181229110904.GB6300@uno.localdomain>","date":"2018-12-29T11:09:04","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","submitter":{"id":3,"url":"https://patchwork.libcamera.org/api/people/3/","name":"Jacopo Mondi","email":"jacopo@jmondi.org"},"content":"Hi Laurent, Niklas,\n\nOn Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote:\n> Hi Laurent,\n>\n> Thanks for your feedback.\n>\n> On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote:\n> > On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote:\n> > > Hi Niklas,\n> > > as I've commented on 00/12, I think the DeviceMatcher should be\n> > > replaced by a MediaDeviceDesc and have the MediaDevice do the matching\n> > > on it. Nonethless, I have some questions here, as they might apply\n> > > generally to all our code base.\n> >\n> > How about doing it the other way around, having the match() function in the\n> > MediaDeviceDesc class ? That would keep MediaDevice focused on handling media\n> > devices, and MediaDeviceDesc on handling matching. We could perhaps rename\n> > MediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name\n> > as such, but I don't really like abbreviating Desc).\n>\n> I agree it would be good if the MedaDevice could focus on operating\n> media device and have the enumeration and matching in other objects.\n\nI'm sorry, I don't agree.\n\nI struggled to keep the MediaDevice as opaque as possible and having a\n(friend?) class that goes and look into the media device to see if it\nmatches a description goes against this direction.\n\nMatching a media graphs at the moment means matching (part of) it's\nmedia_device_info and a list of entities names. I really feel this is\nsomething the MediaDevice should do, otherwise those informations are\nexposed to the extern, which makes then possible for all components in\nthe system access the single entities (as you might notice, in my\nproposed implementation 'getEntityByName()' is private) and the\ninternal device informations as returned by MEDIA_IOC_DEVICE_INFO.\n\nI really would like to keep the MediaDevice self-contained, exposing a\nminimal API to handle linking/unlinking and in future retrieve entities\nto access their subdevices to set/get formats, controls and all V4L2\noperations from the pipeline manager. But it my opinion that's should\nbe it. Everything else should be opaque and incapsulated by the\nMediaDevice class.\n\nI fail to see why creating a dependency on the MediaDevice interface\n(how you access the media_device iformation, how you retrieve entities\netc) in another object is better than define a 'description' object\nalong side the MediaDevice definition itself, and have the MediaDevice\nconsume it and match with it internals informations.\n\nFor the current patch series this is not a big deal though, but it's a\ndiscussion I would like to continue in future.\n\nThanks\n j\n\n> >\n> > > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> > > > Provide a DeviceMatch class which represents all properties of a media\n> > > > device a pipeline hander can specify when searching for a device to use\n> > > > in its pipeline.\n> > > >\n> > > > Signed-off-by: Niklas Söderlund \n> > > > ---\n> > > >\n> > > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> > > > src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> > > > 2 files changed, 71 insertions(+)\n> > > >\n> > > > diff --git a/src/libcamera/deviceenumerator.cpp\n> > > > b/src/libcamera/deviceenumerator.cpp index\n> > > > 7c44a65b45472ef3..17b6874d229c791c 100644\n> > > > --- a/src/libcamera/deviceenumerator.cpp\n> > > > +++ b/src/libcamera/deviceenumerator.cpp\n> > > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name,\n> > > > std::string &devnode) const\n> > > > \treturn true;\n> > > > }\n> > > >\n> > > > +/* ----------------------------------------------------------------------\n> > > > + * DeviceMatch\n> > > > + */\n> > > > +\n> > > > +DeviceMatch::DeviceMatch(const std::string &driver)\n> > > > +\t: driver_(driver)\n> > > > +{\n> > > > +}\n> > >\n> > > Is it worth placing constructors with list initializers only in the\n> > > .cpp file?\n> > >\n> > > > +\n> > > > +void DeviceMatch::add(const std::string &entity)\n> > > > +{\n> > > > +\tentities_.push_back(std::string(entity));\n> >\n> > Doesn't entities_.push_back(entity); work as expected ?\n>\n> It does, this is a mistake on my side. Thanks for pointing it out.\n>\n> >\n> > > > +}\n> > > > +\n> > > > +bool DeviceMatch::match(const DeviceInfo *info) const\n> > > > +{\n> > > > +\tstd::vector entities;\n> > >\n> > > That's weird. This is clearly unused, but the compiler does not\n> > > complain. Actually I can keep it or remove it without any message from\n> > > g++. Was it here intentionally?\n> > >\n> > > > +\n> > > > +\tif (!matchInfo(info->info()))\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\tif (!matchEntities(info->entities()))\n> > > > +\t\treturn false;\n> > > > +\n> > > > +\treturn true;\n> >\n> > Given how simple the code is I would inline both matchInfo() and\n> > matchEntities() here.\n>\n> OK I yield :-) Will inline in v2.\n>\n> >\n> > > > +}\n> > > > +\n> > > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const\n> > > > +{\n> > > > +\t/* TODO: Add more optinal matching pairs from struct media_device_info\n> > > > */\n> > >\n> > > optional\n> > >\n> > > > +\t/* TODO: Allow for empty driver in DeviceMatch */\n> > > > +\treturn driver_ == info.driver;\n> > > > +}\n> > > > +\n> > > > +bool DeviceMatch::matchEntities(const std::vector &entities)\n> > > > const\n> > > > +{\n> > > > +\tfor (const std::string &name : entities_) {\n> > > > +\t\tbool found = false;\n> > > > +\n> > > > +\t\tfor (const std::string &entity : entities) {\n> > > > +\n> > >\n> > > nit: unnecessary empty line\n> > >\n> > > > +\t\t\tif (name == entity) {\n> > > > +\t\t\t\tfound = true;\n> > > > +\t\t\t\tbreak;\n> > > > +\t\t\t}\n> > > > +\t\t}\n> > > > +\n> > > > +\t\tif (!found)\n> > > > +\t\t\treturn false;\n> > > > +\t}\n> > > > +\n> > > > +\treturn true;\n> > > > +}\n> > > > +\n> > > >\n> > > > } /* namespace libcamera */\n> > > >\n> > > > diff --git a/src/libcamera/include/deviceenumerator.h\n> > > > b/src/libcamera/include/deviceenumerator.h index\n> > > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644\n> > > > --- a/src/libcamera/include/deviceenumerator.h\n> > > > +++ b/src/libcamera/include/deviceenumerator.h\n> > > > @@ -39,6 +39,23 @@ private:\n> > > > \tstd::map entities_;\n> > > > };\n> > > >\n> > > > +class DeviceMatch\n> > > > +{\n> > > > +public:\n> > > > +\tDeviceMatch(const std::string &driver);\n> > > > +\n> > > > +\tvoid add(const std::string &entity);\n> > > > +\n> > > > +\tbool match(const DeviceInfo *info) const;\n> > > > +\n> > > > +private:\n> > > > +\tstd::string driver_;\n> > > > +\tstd::vector entities_;\n> > > > +\n> > > > +\tbool matchInfo(const struct media_device_info &info) const;\n> > > > +\tbool matchEntities(const std::vector &entities) const;\n> > > > +};\n> > > > +\n> > > >\n> > > > } /* namespace libcamera */\n> > > >\n> > > > #endif\t/* __LIBCAMERA_DEVICEENUMERATE_H__ */\n> > >\n> > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)\n> >\n> > --\n> > Regards,\n> >\n> > Laurent Pinchart\n> >\n> >\n> >\n>\n> --\n> Regards,\n> Niklas Söderlund","headers":{"Return-Path":"","Received":["from relay11.mail.gandi.net (relay11.mail.gandi.net\n\t[217.70.178.231])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4F94E60B2F\n\tfor ;\n\tSat, 29 Dec 2018 12:09:05 +0100 (CET)","from uno.localdomain (2-224-242-101.ip172.fastwebnet.it\n\t[2.224.242.101]) (Authenticated sender: jacopo@jmondi.org)\n\tby relay11.mail.gandi.net (Postfix) with ESMTPSA id EB8AD100004;\n\tSat, 29 Dec 2018 11:09:03 +0000 (UTC)"],"Date":"Sat, 29 Dec 2018 12:09:04 +0100","From":"Jacopo Mondi ","To":"Niklas S??derlund ","Cc":"Laurent Pinchart ,\n\tlibcamera-devel@lists.libcamera.org","Message-ID":"<20181229110904.GB6300@uno.localdomain>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181222230041.29999-5-niklas.soderlund@ragnatech.se>\n\t<20181224105530.GD2364@archlinux> <3747490.e0YOlbGKG1@avalon>\n\t<20181229010624.GA19796@bigcity.dyn.berto.se>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"V0207lvV8h4k8FAm\"","Content-Disposition":"inline","In-Reply-To":"<20181229010624.GA19796@bigcity.dyn.berto.se>","User-Agent":"Mutt/1.11.1 (2018-12-01)","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Sat, 29 Dec 2018 11:09:05 -0000"}},{"id":174,"web_url":"https://patchwork.libcamera.org/comment/174/","msgid":"<4661779.hVBZRcpGnY@avalon>","date":"2019-01-02T10:28:50","subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Jacopo,\n\nOn Saturday, 29 December 2018 13:09:04 EET Jacopo Mondi wrote:\n> On Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote:\n> > On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote:\n> >> On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote:\n> >>> Hi Niklas,\n> >>> \n> >>> as I've commented on 00/12, I think the DeviceMatcher should be\n> >>> replaced by a MediaDeviceDesc and have the MediaDevice do the matching\n> >>> on it. Nonethless, I have some questions here, as they might apply\n> >>> generally to all our code base.\n> >> \n> >> How about doing it the other way around, having the match() function in\n> >> the MediaDeviceDesc class ? That would keep MediaDevice focused on\n> >> handling media devices, and MediaDeviceDesc on handling matching. We\n> >> could perhaps rename MediaDeviceDesc to MediaDeviceMatch (I don't\n> >> dislike the MediaDeviceDesc name as such, but I don't really like\n> >> abbreviating Desc).\n> > \n> > I agree it would be good if the MedaDevice could focus on operating\n> > media device and have the enumeration and matching in other objects.\n> \n> I'm sorry, I don't agree.\n> \n> I struggled to keep the MediaDevice as opaque as possible and having a\n> (friend?) class that goes and look into the media device to see if it\n> matches a description goes against this direction.\n\nBut why does it need to be as opaque as possible ? I see no reason not to \nexpose the full list of entities for instance. Let's not artificially restrict \nwhat the MediaDevice users can do.\n\n> Matching a media graphs at the moment means matching (part of) it's\n> media_device_info and a list of entities names. I really feel this is\n> something the MediaDevice should do, otherwise those informations are\n> exposed to the extern, which makes then possible for all components in\n> the system access the single entities (as you might notice, in my\n> proposed implementation 'getEntityByName()' is private) and the\n> internal device informations as returned by MEDIA_IOC_DEVICE_INFO.\n> \n> I really would like to keep the MediaDevice self-contained, exposing a\n> minimal API to handle linking/unlinking and in future retrieve entities\n> to access their subdevices to set/get formats, controls and all V4L2\n> operations from the pipeline manager. But it my opinion that's should\n> be it. Everything else should be opaque and incapsulated by the\n> MediaDevice class.\n> \n> I fail to see why creating a dependency on the MediaDevice interface\n> (how you access the media_device iformation, how you retrieve entities\n> etc) in another object is better than define a 'description' object\n> along side the MediaDevice definition itself, and have the MediaDevice\n> consume it and match with it internals informations.\n> \n> For the current patch series this is not a big deal though, but it's a\n> discussion I would like to continue in future.\n\nAs we've discussed on IRC, we will have code that will need to access entities \nwithout much a priori knowledge of the graph. Two such examples are generic \nhelpers that are not device-specific, and pipeline handlers that have no a \npriori knowledge of which sensor(s) can be connected to ISPs.\n\n> >>> On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote:\n> >>>> Provide a DeviceMatch class which represents all properties of a\n> >>>> media device a pipeline hander can specify when searching for a\n> >>>> device to use in its pipeline.\n> >>>> \n> >>>> Signed-off-by: Niklas Söderlund \n> >>>> ---\n> >>>> \n> >>>> src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++\n> >>>> src/libcamera/include/deviceenumerator.h | 17 ++++++++\n> >>>> 2 files changed, 71 insertions(+)\n\n[snip]","headers":{"Return-Path":"","Received":["from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 70DF560B30\n\tfor ;\n\tWed, 2 Jan 2019 11:27:50 +0100 (CET)","from avalon.localnet (dfj612ybrt5fhg77mgycy-3.rev.dnainternet.fi\n\t[IPv6:2001:14ba:21f5:5b00:2e86:4862:ef6a:2804])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id DA22E505;\n\tWed, 2 Jan 2019 11:27:49 +0100 (CET)"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1546424870;\n\tbh=H9JTmxdn/ces01sUDkeOqDo5WQqY2x1oxqQzfY7saS8=;\n\th=From:To:Cc:Subject:Date:In-Reply-To:References:From;\n\tb=T/ewv75FGRIhPBilKT097GAyqn10+jPMkvYj1XADwSPW55sVU7SC/d1dGPmmGE2hY\n\t+5Q+DF9f6wufb0KxhadBrU6m27FFJ/46U+bnM+G+6zG1VxoJ60gct+fWstxUrLlRO3\n\tjDiVg/2+zACzTknbrIdkfORsYnUiO6UgAUeaYWdc=","From":"Laurent Pinchart ","To":"Jacopo Mondi ","Cc":"Niklas S??derlund ,\n\tlibcamera-devel@lists.libcamera.org","Date":"Wed, 02 Jan 2019 12:28:50 +0200","Message-ID":"<4661779.hVBZRcpGnY@avalon>","Organization":"Ideas on Board Oy","In-Reply-To":"<20181229110904.GB6300@uno.localdomain>","References":"<20181222230041.29999-1-niklas.soderlund@ragnatech.se>\n\t<20181229010624.GA19796@bigcity.dyn.berto.se>\n\t<20181229110904.GB6300@uno.localdomain>","MIME-Version":"1.0","Content-Transfer-Encoding":"quoted-printable","Content-Type":"text/plain; charset=\"iso-8859-1\"","Subject":"Re: [libcamera-devel] [PATCH 04/12] libcamera: deviceenumerator:\n\tadd DeviceMatch class","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","X-List-Received-Date":"Wed, 02 Jan 2019 10:27:50 -0000"}}]