| Message ID | 20181222230041.29999-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, as I've commented on 00/12, I think the DeviceMatcher should be replaced by a MediaDeviceDesc and have the MediaDevice do the matching on it. Nonethless, I have some questions here, as they might apply generally to all our code base. On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > Provide a DeviceMatch class which represents all properties of a media > device a pipeline hander can specify when searching for a device to use > in its pipeline. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > src/libcamera/include/deviceenumerator.h | 17 ++++++++ > 2 files changed, 71 insertions(+) > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp > index 7c44a65b45472ef3..17b6874d229c791c 100644 > --- a/src/libcamera/deviceenumerator.cpp > +++ b/src/libcamera/deviceenumerator.cpp > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const > return true; > } > > +/* ----------------------------------------------------------------------------- > + * DeviceMatch > + */ > + > +DeviceMatch::DeviceMatch(const std::string &driver) > + : driver_(driver) > +{ > +} Is it worth placing constructors with list initializers only in the .cpp file? > + > +void DeviceMatch::add(const std::string &entity) > +{ > + entities_.push_back(std::string(entity)); > +} > + > +bool DeviceMatch::match(const DeviceInfo *info) const > +{ > + std::vector<std::string> entities; That's weird. This is clearly unused, but the compiler does not complain. Actually I can keep it or remove it without any message from g++. Was it here intentionally? > + > + if (!matchInfo(info->info())) > + return false; > + > + if (!matchEntities(info->entities())) > + return false; > + > + return true; > +} > + > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const > +{ > + /* TODO: Add more optinal matching pairs from struct media_device_info */ optional > + /* TODO: Allow for empty driver in DeviceMatch */ > + return driver_ == info.driver; > +} > + > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) const > +{ > + for (const std::string &name : entities_) { > + bool found = false; > + > + for (const std::string &entity : entities) { > + nit: unnecessary empty line > + if (name == entity) { > + found = true; > + break; > + } > + } > + > + if (!found) > + return false; > + } > + > + return true; > +} > + > } /* namespace libcamera */ > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h > index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 > --- a/src/libcamera/include/deviceenumerator.h > +++ b/src/libcamera/include/deviceenumerator.h > @@ -39,6 +39,23 @@ private: > std::map<std::string, std::string> entities_; > }; > > +class DeviceMatch > +{ > +public: > + DeviceMatch(const std::string &driver); > + > + void add(const std::string &entity); > + > + bool match(const DeviceInfo *info) const; > + > +private: > + std::string driver_; > + std::vector<std::string> entities_; > + > + bool matchInfo(const struct media_device_info &info) const; > + bool matchEntities(const std::vector<std::string> &entities) const; > +}; > + > } /* namespace libcamera */ > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :) > -- > 2.20.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo, Thanks for your feedback, On 2018-12-24 11:55:30 +0100, Jacopo Mondi wrote: > Hi Niklas, > as I've commented on 00/12, I think the DeviceMatcher should be > replaced by a MediaDeviceDesc and have the MediaDevice do the matching > on it. Nonethless, I have some questions here, as they might apply > generally to all our code base. See comment in 00/12. > > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > > Provide a DeviceMatch class which represents all properties of a media > > device a pipeline hander can specify when searching for a device to use > > in its pipeline. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > > src/libcamera/include/deviceenumerator.h | 17 ++++++++ > > 2 files changed, 71 insertions(+) > > > > diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp > > index 7c44a65b45472ef3..17b6874d229c791c 100644 > > --- a/src/libcamera/deviceenumerator.cpp > > +++ b/src/libcamera/deviceenumerator.cpp > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const > > return true; > > } > > > > +/* ----------------------------------------------------------------------------- > > + * DeviceMatch > > + */ > > + > > +DeviceMatch::DeviceMatch(const std::string &driver) > > + : driver_(driver) > > +{ > > +} > > Is it worth placing constructors with list initializers only in the > .cpp file? Why not? > > > + > > +void DeviceMatch::add(const std::string &entity) > > +{ > > + entities_.push_back(std::string(entity)); > > +} > > + > > +bool DeviceMatch::match(const DeviceInfo *info) const > > +{ > > + std::vector<std::string> entities; > > That's weird. This is clearly unused, but the compiler does not > complain. Actually I can keep it or remove it without any message from > g++. Was it here intentionally? My bad it was part of a previous version of the code and should be removed. Great catch I wonder why g++ do not complain about it. > > > + > > + if (!matchInfo(info->info())) > > + return false; > > + > > + if (!matchEntities(info->entities())) > > + return false; > > + > > + return true; > > +} > > + > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const > > +{ > > + /* TODO: Add more optinal matching pairs from struct media_device_info */ > optional Thanks, this is fixed in the documentation patch but no reason to not be correct in the first place. > > + /* TODO: Allow for empty driver in DeviceMatch */ > > + return driver_ == info.driver; > > +} > > + > > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) const > > +{ > > + for (const std::string &name : entities_) { > > + bool found = false; > > + > > + for (const std::string &entity : entities) { > > + > nit: unnecessary empty line Thanks. > > > + if (name == entity) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return false; > > + } > > + > > + return true; > > +} > > + > > } /* namespace libcamera */ > > diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h > > index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 > > --- a/src/libcamera/include/deviceenumerator.h > > +++ b/src/libcamera/include/deviceenumerator.h > > @@ -39,6 +39,23 @@ private: > > std::map<std::string, std::string> entities_; > > }; > > > > +class DeviceMatch > > +{ > > +public: > > + DeviceMatch(const std::string &driver); > > + > > + void add(const std::string &entity); > > + > > + bool match(const DeviceInfo *info) const; > > + > > +private: > > + std::string driver_; > > + std::vector<std::string> entities_; > > + > > + bool matchInfo(const struct media_device_info &info) const; > > + bool matchEntities(const std::vector<std::string> &entities) const; > > +}; > > + > > } /* namespace libcamera */ > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :) Yes, thanks for spotting this. > > -- > > 2.20.1 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote: > Hi Niklas, > as I've commented on 00/12, I think the DeviceMatcher should be > replaced by a MediaDeviceDesc and have the MediaDevice do the matching > on it. Nonethless, I have some questions here, as they might apply > generally to all our code base. How about doing it the other way around, having the match() function in the MediaDeviceDesc class ? That would keep MediaDevice focused on handling media devices, and MediaDeviceDesc on handling matching. We could perhaps rename MediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name as such, but I don't really like abbreviating Desc). > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > > Provide a DeviceMatch class which represents all properties of a media > > device a pipeline hander can specify when searching for a device to use > > in its pipeline. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > > src/libcamera/include/deviceenumerator.h | 17 ++++++++ > > 2 files changed, 71 insertions(+) > > > > diff --git a/src/libcamera/deviceenumerator.cpp > > b/src/libcamera/deviceenumerator.cpp index > > 7c44a65b45472ef3..17b6874d229c791c 100644 > > --- a/src/libcamera/deviceenumerator.cpp > > +++ b/src/libcamera/deviceenumerator.cpp > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, > > std::string &devnode) const > > return true; > > } > > > > +/* ---------------------------------------------------------------------- > > + * DeviceMatch > > + */ > > + > > +DeviceMatch::DeviceMatch(const std::string &driver) > > + : driver_(driver) > > +{ > > +} > > Is it worth placing constructors with list initializers only in the > .cpp file? > > > + > > +void DeviceMatch::add(const std::string &entity) > > +{ > > + entities_.push_back(std::string(entity)); Doesn't entities_.push_back(entity); work as expected ? > > +} > > + > > +bool DeviceMatch::match(const DeviceInfo *info) const > > +{ > > + std::vector<std::string> entities; > > That's weird. This is clearly unused, but the compiler does not > complain. Actually I can keep it or remove it without any message from > g++. Was it here intentionally? > > > + > > + if (!matchInfo(info->info())) > > + return false; > > + > > + if (!matchEntities(info->entities())) > > + return false; > > + > > + return true; Given how simple the code is I would inline both matchInfo() and matchEntities() here. > > +} > > + > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const > > +{ > > + /* TODO: Add more optinal matching pairs from struct media_device_info > > */ > > optional > > > + /* TODO: Allow for empty driver in DeviceMatch */ > > + return driver_ == info.driver; > > +} > > + > > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) > > const > > +{ > > + for (const std::string &name : entities_) { > > + bool found = false; > > + > > + for (const std::string &entity : entities) { > > + > > nit: unnecessary empty line > > > + if (name == entity) { > > + found = true; > > + break; > > + } > > + } > > + > > + if (!found) > > + return false; > > + } > > + > > + return true; > > +} > > + > > > > } /* namespace libcamera */ > > > > diff --git a/src/libcamera/include/deviceenumerator.h > > b/src/libcamera/include/deviceenumerator.h index > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 > > --- a/src/libcamera/include/deviceenumerator.h > > +++ b/src/libcamera/include/deviceenumerator.h > > @@ -39,6 +39,23 @@ private: > > std::map<std::string, std::string> entities_; > > }; > > > > +class DeviceMatch > > +{ > > +public: > > + DeviceMatch(const std::string &driver); > > + > > + void add(const std::string &entity); > > + > > + bool match(const DeviceInfo *info) const; > > + > > +private: > > + std::string driver_; > > + std::vector<std::string> entities_; > > + > > + bool matchInfo(const struct media_device_info &info) const; > > + bool matchEntities(const std::vector<std::string> &entities) const; > > +}; > > + > > > > } /* namespace libcamera */ > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :)
Hi Laurent, Thanks for your feedback. On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote: > On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote: > > Hi Niklas, > > as I've commented on 00/12, I think the DeviceMatcher should be > > replaced by a MediaDeviceDesc and have the MediaDevice do the matching > > on it. Nonethless, I have some questions here, as they might apply > > generally to all our code base. > > How about doing it the other way around, having the match() function in the > MediaDeviceDesc class ? That would keep MediaDevice focused on handling media > devices, and MediaDeviceDesc on handling matching. We could perhaps rename > MediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name > as such, but I don't really like abbreviating Desc). I agree it would be good if the MedaDevice could focus on operating media device and have the enumeration and matching in other objects. > > > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > > > Provide a DeviceMatch class which represents all properties of a media > > > device a pipeline hander can specify when searching for a device to use > > > in its pipeline. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > > > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > > > src/libcamera/include/deviceenumerator.h | 17 ++++++++ > > > 2 files changed, 71 insertions(+) > > > > > > diff --git a/src/libcamera/deviceenumerator.cpp > > > b/src/libcamera/deviceenumerator.cpp index > > > 7c44a65b45472ef3..17b6874d229c791c 100644 > > > --- a/src/libcamera/deviceenumerator.cpp > > > +++ b/src/libcamera/deviceenumerator.cpp > > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, > > > std::string &devnode) const > > > return true; > > > } > > > > > > +/* ---------------------------------------------------------------------- > > > + * DeviceMatch > > > + */ > > > + > > > +DeviceMatch::DeviceMatch(const std::string &driver) > > > + : driver_(driver) > > > +{ > > > +} > > > > Is it worth placing constructors with list initializers only in the > > .cpp file? > > > > > + > > > +void DeviceMatch::add(const std::string &entity) > > > +{ > > > + entities_.push_back(std::string(entity)); > > Doesn't entities_.push_back(entity); work as expected ? It does, this is a mistake on my side. Thanks for pointing it out. > > > > +} > > > + > > > +bool DeviceMatch::match(const DeviceInfo *info) const > > > +{ > > > + std::vector<std::string> entities; > > > > That's weird. This is clearly unused, but the compiler does not > > complain. Actually I can keep it or remove it without any message from > > g++. Was it here intentionally? > > > > > + > > > + if (!matchInfo(info->info())) > > > + return false; > > > + > > > + if (!matchEntities(info->entities())) > > > + return false; > > > + > > > + return true; > > Given how simple the code is I would inline both matchInfo() and > matchEntities() here. OK I yield :-) Will inline in v2. > > > > +} > > > + > > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const > > > +{ > > > + /* TODO: Add more optinal matching pairs from struct media_device_info > > > */ > > > > optional > > > > > + /* TODO: Allow for empty driver in DeviceMatch */ > > > + return driver_ == info.driver; > > > +} > > > + > > > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) > > > const > > > +{ > > > + for (const std::string &name : entities_) { > > > + bool found = false; > > > + > > > + for (const std::string &entity : entities) { > > > + > > > > nit: unnecessary empty line > > > > > + if (name == entity) { > > > + found = true; > > > + break; > > > + } > > > + } > > > + > > > + if (!found) > > > + return false; > > > + } > > > + > > > + return true; > > > +} > > > + > > > > > > } /* namespace libcamera */ > > > > > > diff --git a/src/libcamera/include/deviceenumerator.h > > > b/src/libcamera/include/deviceenumerator.h index > > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 > > > --- a/src/libcamera/include/deviceenumerator.h > > > +++ b/src/libcamera/include/deviceenumerator.h > > > @@ -39,6 +39,23 @@ private: > > > std::map<std::string, std::string> entities_; > > > }; > > > > > > +class DeviceMatch > > > +{ > > > +public: > > > + DeviceMatch(const std::string &driver); > > > + > > > + void add(const std::string &entity); > > > + > > > + bool match(const DeviceInfo *info) const; > > > + > > > +private: > > > + std::string driver_; > > > + std::vector<std::string> entities_; > > > + > > > + bool matchInfo(const struct media_device_info &info) const; > > > + bool matchEntities(const std::vector<std::string> &entities) const; > > > +}; > > > + > > > > > > } /* namespace libcamera */ > > > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :) > > -- > Regards, > > Laurent Pinchart > > >
Hi Laurent, Niklas, On Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote: > Hi Laurent, > > Thanks for your feedback. > > On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote: > > On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote: > > > Hi Niklas, > > > as I've commented on 00/12, I think the DeviceMatcher should be > > > replaced by a MediaDeviceDesc and have the MediaDevice do the matching > > > on it. Nonethless, I have some questions here, as they might apply > > > generally to all our code base. > > > > How about doing it the other way around, having the match() function in the > > MediaDeviceDesc class ? That would keep MediaDevice focused on handling media > > devices, and MediaDeviceDesc on handling matching. We could perhaps rename > > MediaDeviceDesc to MediaDeviceMatch (I don't dislike the MediaDeviceDesc name > > as such, but I don't really like abbreviating Desc). > > I agree it would be good if the MedaDevice could focus on operating > media device and have the enumeration and matching in other objects. I'm sorry, I don't agree. I struggled to keep the MediaDevice as opaque as possible and having a (friend?) class that goes and look into the media device to see if it matches a description goes against this direction. Matching a media graphs at the moment means matching (part of) it's media_device_info and a list of entities names. I really feel this is something the MediaDevice should do, otherwise those informations are exposed to the extern, which makes then possible for all components in the system access the single entities (as you might notice, in my proposed implementation 'getEntityByName()' is private) and the internal device informations as returned by MEDIA_IOC_DEVICE_INFO. I really would like to keep the MediaDevice self-contained, exposing a minimal API to handle linking/unlinking and in future retrieve entities to access their subdevices to set/get formats, controls and all V4L2 operations from the pipeline manager. But it my opinion that's should be it. Everything else should be opaque and incapsulated by the MediaDevice class. I fail to see why creating a dependency on the MediaDevice interface (how you access the media_device iformation, how you retrieve entities etc) in another object is better than define a 'description' object along side the MediaDevice definition itself, and have the MediaDevice consume it and match with it internals informations. For the current patch series this is not a big deal though, but it's a discussion I would like to continue in future. Thanks j > > > > > On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > > > > Provide a DeviceMatch class which represents all properties of a media > > > > device a pipeline hander can specify when searching for a device to use > > > > in its pipeline. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > > > > > src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > > > > src/libcamera/include/deviceenumerator.h | 17 ++++++++ > > > > 2 files changed, 71 insertions(+) > > > > > > > > diff --git a/src/libcamera/deviceenumerator.cpp > > > > b/src/libcamera/deviceenumerator.cpp index > > > > 7c44a65b45472ef3..17b6874d229c791c 100644 > > > > --- a/src/libcamera/deviceenumerator.cpp > > > > +++ b/src/libcamera/deviceenumerator.cpp > > > > @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, > > > > std::string &devnode) const > > > > return true; > > > > } > > > > > > > > +/* ---------------------------------------------------------------------- > > > > + * DeviceMatch > > > > + */ > > > > + > > > > +DeviceMatch::DeviceMatch(const std::string &driver) > > > > + : driver_(driver) > > > > +{ > > > > +} > > > > > > Is it worth placing constructors with list initializers only in the > > > .cpp file? > > > > > > > + > > > > +void DeviceMatch::add(const std::string &entity) > > > > +{ > > > > + entities_.push_back(std::string(entity)); > > > > Doesn't entities_.push_back(entity); work as expected ? > > It does, this is a mistake on my side. Thanks for pointing it out. > > > > > > > +} > > > > + > > > > +bool DeviceMatch::match(const DeviceInfo *info) const > > > > +{ > > > > + std::vector<std::string> entities; > > > > > > That's weird. This is clearly unused, but the compiler does not > > > complain. Actually I can keep it or remove it without any message from > > > g++. Was it here intentionally? > > > > > > > + > > > > + if (!matchInfo(info->info())) > > > > + return false; > > > > + > > > > + if (!matchEntities(info->entities())) > > > > + return false; > > > > + > > > > + return true; > > > > Given how simple the code is I would inline both matchInfo() and > > matchEntities() here. > > OK I yield :-) Will inline in v2. > > > > > > > +} > > > > + > > > > +bool DeviceMatch::matchInfo(const struct media_device_info &info) const > > > > +{ > > > > + /* TODO: Add more optinal matching pairs from struct media_device_info > > > > */ > > > > > > optional > > > > > > > + /* TODO: Allow for empty driver in DeviceMatch */ > > > > + return driver_ == info.driver; > > > > +} > > > > + > > > > +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) > > > > const > > > > +{ > > > > + for (const std::string &name : entities_) { > > > > + bool found = false; > > > > + > > > > + for (const std::string &entity : entities) { > > > > + > > > > > > nit: unnecessary empty line > > > > > > > + if (name == entity) { > > > > + found = true; > > > > + break; > > > > + } > > > > + } > > > > + > > > > + if (!found) > > > > + return false; > > > > + } > > > > + > > > > + return true; > > > > +} > > > > + > > > > > > > > } /* namespace libcamera */ > > > > > > > > diff --git a/src/libcamera/include/deviceenumerator.h > > > > b/src/libcamera/include/deviceenumerator.h index > > > > 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 > > > > --- a/src/libcamera/include/deviceenumerator.h > > > > +++ b/src/libcamera/include/deviceenumerator.h > > > > @@ -39,6 +39,23 @@ private: > > > > std::map<std::string, std::string> entities_; > > > > }; > > > > > > > > +class DeviceMatch > > > > +{ > > > > +public: > > > > + DeviceMatch(const std::string &driver); > > > > + > > > > + void add(const std::string &entity); > > > > + > > > > + bool match(const DeviceInfo *info) const; > > > > + > > > > +private: > > > > + std::string driver_; > > > > + std::vector<std::string> entities_; > > > > + > > > > + bool matchInfo(const struct media_device_info &info) const; > > > > + bool matchEntities(const std::vector<std::string> &entities) const; > > > > +}; > > > > + > > > > > > > > } /* namespace libcamera */ > > > > > > > > #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */ > > > > > > This is less than nit, but shouldn't this be DEVICEENUMERATOR ? :) > > > > -- > > Regards, > > > > Laurent Pinchart > > > > > > > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On Saturday, 29 December 2018 13:09:04 EET Jacopo Mondi wrote: > On Sat, Dec 29, 2018 at 02:06:24AM +0100, Niklas S??derlund wrote: > > On 2018-12-29 00:02:41 +0200, Laurent Pinchart wrote: > >> On Monday, 24 December 2018 12:55:30 EET Jacopo Mondi wrote: > >>> Hi Niklas, > >>> > >>> as I've commented on 00/12, I think the DeviceMatcher should be > >>> replaced by a MediaDeviceDesc and have the MediaDevice do the matching > >>> on it. Nonethless, I have some questions here, as they might apply > >>> generally to all our code base. > >> > >> How about doing it the other way around, having the match() function in > >> the MediaDeviceDesc class ? That would keep MediaDevice focused on > >> handling media devices, and MediaDeviceDesc on handling matching. We > >> could perhaps rename MediaDeviceDesc to MediaDeviceMatch (I don't > >> dislike the MediaDeviceDesc name as such, but I don't really like > >> abbreviating Desc). > > > > I agree it would be good if the MedaDevice could focus on operating > > media device and have the enumeration and matching in other objects. > > I'm sorry, I don't agree. > > I struggled to keep the MediaDevice as opaque as possible and having a > (friend?) class that goes and look into the media device to see if it > matches a description goes against this direction. But why does it need to be as opaque as possible ? I see no reason not to expose the full list of entities for instance. Let's not artificially restrict what the MediaDevice users can do. > Matching a media graphs at the moment means matching (part of) it's > media_device_info and a list of entities names. I really feel this is > something the MediaDevice should do, otherwise those informations are > exposed to the extern, which makes then possible for all components in > the system access the single entities (as you might notice, in my > proposed implementation 'getEntityByName()' is private) and the > internal device informations as returned by MEDIA_IOC_DEVICE_INFO. > > I really would like to keep the MediaDevice self-contained, exposing a > minimal API to handle linking/unlinking and in future retrieve entities > to access their subdevices to set/get formats, controls and all V4L2 > operations from the pipeline manager. But it my opinion that's should > be it. Everything else should be opaque and incapsulated by the > MediaDevice class. > > I fail to see why creating a dependency on the MediaDevice interface > (how you access the media_device iformation, how you retrieve entities > etc) in another object is better than define a 'description' object > along side the MediaDevice definition itself, and have the MediaDevice > consume it and match with it internals informations. > > For the current patch series this is not a big deal though, but it's a > discussion I would like to continue in future. As we've discussed on IRC, we will have code that will need to access entities without much a priori knowledge of the graph. Two such examples are generic helpers that are not device-specific, and pipeline handlers that have no a priori knowledge of which sensor(s) can be connected to ISPs. > >>> On Sun, Dec 23, 2018 at 12:00:33AM +0100, Niklas S??derlund wrote: > >>>> Provide a DeviceMatch class which represents all properties of a > >>>> media device a pipeline hander can specify when searching for a > >>>> device to use in its pipeline. > >>>> > >>>> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > >>>> --- > >>>> > >>>> src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ > >>>> src/libcamera/include/deviceenumerator.h | 17 ++++++++ > >>>> 2 files changed, 71 insertions(+) [snip]
diff --git a/src/libcamera/deviceenumerator.cpp b/src/libcamera/deviceenumerator.cpp index 7c44a65b45472ef3..17b6874d229c791c 100644 --- a/src/libcamera/deviceenumerator.cpp +++ b/src/libcamera/deviceenumerator.cpp @@ -75,4 +75,58 @@ bool DeviceInfo::lookup(const std::string &name, std::string &devnode) const return true; } +/* ----------------------------------------------------------------------------- + * DeviceMatch + */ + +DeviceMatch::DeviceMatch(const std::string &driver) + : driver_(driver) +{ +} + +void DeviceMatch::add(const std::string &entity) +{ + entities_.push_back(std::string(entity)); +} + +bool DeviceMatch::match(const DeviceInfo *info) const +{ + std::vector<std::string> entities; + + if (!matchInfo(info->info())) + return false; + + if (!matchEntities(info->entities())) + return false; + + return true; +} + +bool DeviceMatch::matchInfo(const struct media_device_info &info) const +{ + /* TODO: Add more optinal matching pairs from struct media_device_info */ + /* TODO: Allow for empty driver in DeviceMatch */ + return driver_ == info.driver; +} + +bool DeviceMatch::matchEntities(const std::vector<std::string> &entities) const +{ + for (const std::string &name : entities_) { + bool found = false; + + for (const std::string &entity : entities) { + + if (name == entity) { + found = true; + break; + } + } + + if (!found) + return false; + } + + return true; +} + } /* namespace libcamera */ diff --git a/src/libcamera/include/deviceenumerator.h b/src/libcamera/include/deviceenumerator.h index 0136ed6ea23bf65e..fb412b8840cb2ffe 100644 --- a/src/libcamera/include/deviceenumerator.h +++ b/src/libcamera/include/deviceenumerator.h @@ -39,6 +39,23 @@ private: std::map<std::string, std::string> entities_; }; +class DeviceMatch +{ +public: + DeviceMatch(const std::string &driver); + + void add(const std::string &entity); + + bool match(const DeviceInfo *info) const; + +private: + std::string driver_; + std::vector<std::string> entities_; + + bool matchInfo(const struct media_device_info &info) const; + bool matchEntities(const std::vector<std::string> &entities) const; +}; + } /* namespace libcamera */ #endif /* __LIBCAMERA_DEVICEENUMERATE_H__ */
Provide a DeviceMatch class which represents all properties of a media device a pipeline hander can specify when searching for a device to use in its pipeline. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/deviceenumerator.cpp | 54 ++++++++++++++++++++++++ src/libcamera/include/deviceenumerator.h | 17 ++++++++ 2 files changed, 71 insertions(+)