| Message ID | 20230415205742.268255-1-pobrn@protonmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Barnabás On Sat, Apr 15, 2023 at 09:03:40PM +0000, Barnabás Pőcze via libcamera-devel wrote: > Previously, after `addV4L2Device()` had seen all dependecies, it would > remove the `MediaDeviceDeps` object from the `pending_` list, which > would result in it being destroyed. However, there would still be > (dangling) pointers to this object in `devMap_` that were added in > `addUdevDevice()` (line 103). So remove the entry with the given > devnum when it is removed from the corresponding `MediaDeviceDeps` > object. > > Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> > --- > src/libcamera/device_enumerator_udev.cpp | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > index 5317afbd..a63cd360 100644 > --- a/src/libcamera/device_enumerator_udev.cpp > +++ b/src/libcamera/device_enumerator_udev.cpp > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > * enumerator. > */ > deps->deps_.erase(devnum); > + devMap_.erase(it); Shouldn't this be done only when all the dependencies are actually satisfied ? > > if (deps->deps_.empty()) { Likely inside this if() clause ? Otherwise, if there still are dependencies to meet for the devnum at hand, the next time addV4L2Device(devnum) is called it won't find any item on the devMap_ for that key. > LOG(DeviceEnumerator, Debug) > -- > 2.40.0 > >
Hi 2023. április 17., hétfő 8:30 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > [...] > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > index 5317afbd..a63cd360 100644 > > --- a/src/libcamera/device_enumerator_udev.cpp > > +++ b/src/libcamera/device_enumerator_udev.cpp > > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > * enumerator. > > */ > > deps->deps_.erase(devnum); > > + devMap_.erase(it); > > Shouldn't this be done only when all the dependencies are actually > satisfied ? > Every dependency has its own entry in `devMap_` with a pointer to the corresponding `MediaDeviceDeps` object. I think when one dependency appears it should be removed from the map, just like it is removed from `MediaDeviceDeps::deps_` (in the previous line). > > > > if (deps->deps_.empty()) { > > Likely inside this if() clause ? `deps->deps_` is empty at this point, so we cannot efficiently find all entries in `devMap_` that belong to `deps`. > > Otherwise, if there still are dependencies to meet for the devnum at > hand, the next time addV4L2Device(devnum) is called it won't find any item > on the devMap_ for that key. My reading of the code suggests that devnum is unique and each v4l2 device can be a dependency of at most 1 media device. Is that not correct? > > > LOG(DeviceEnumerator, Debug) > > -- > > 2.40.0 > > > > > Regards, Barnabás Pőcze
Hello Barnabás, On Mon, Apr 17, 2023 at 05:41:58PM +0000, Barnabás Pőcze via libcamera-devel wrote: > 2023. április 17., hétfő 8:30 keltezéssel, Jacopo Mondi írta: > > [...] > > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > > index 5317afbd..a63cd360 100644 > > > --- a/src/libcamera/device_enumerator_udev.cpp > > > +++ b/src/libcamera/device_enumerator_udev.cpp > > > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > > * enumerator. > > > */ > > > deps->deps_.erase(devnum); > > > + devMap_.erase(it); > > > > Shouldn't this be done only when all the dependencies are actually > > satisfied ? > > Every dependency has its own entry in `devMap_` with a pointer to > the corresponding `MediaDeviceDeps` object. I think when one dependency > appears it should be removed from the map, just like it is removed > from `MediaDeviceDeps::deps_` (in the previous line). > > > > if (deps->deps_.empty()) { > > > > Likely inside this if() clause ? > > `deps->deps_` is empty at this point, so we cannot efficiently find all > entries in `devMap_` that belong to `deps`. We could iterate over the whole map to remove all entries that point to deps, but I don't think that's needed. > > Otherwise, if there still are dependencies to meet for the devnum at > > hand, the next time addV4L2Device(devnum) is called it won't find any item > > on the devMap_ for that key. > > My reading of the code suggests that devnum is unique and each v4l2 device > can be a dependency of at most 1 media device. Is that not correct? Your assumption is correct. The only issue I can see would be if the V4L2 device is removed and re-added without the media device being removed too. addV4L2Device() wouldn't find the dependency in that case, but this is a completely unsupported use case that would cause other problems anyway. I think this patch is fine. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > LOG(DeviceEnumerator, Debug)
Hi Barnabás On Mon, Apr 17, 2023 at 05:41:58PM +0000, Barnabás Pőcze via libcamera-devel wrote: > Hi > > > 2023. április 17., hétfő 8:30 keltezéssel, Jacopo Mondi <jacopo.mondi@ideasonboard.com> írta: > > > [...] > > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > > > index 5317afbd..a63cd360 100644 > > > --- a/src/libcamera/device_enumerator_udev.cpp > > > +++ b/src/libcamera/device_enumerator_udev.cpp > > > @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > > > * enumerator. > > > */ > > > deps->deps_.erase(devnum); > > > + devMap_.erase(it); > > > > Shouldn't this be done only when all the dependencies are actually > > satisfied ? > > > > Every dependency has its own entry in `devMap_` with a pointer to > the corresponding `MediaDeviceDeps` object. I think when one dependency > appears it should be removed from the map, just like it is removed > from `MediaDeviceDeps::deps_` (in the previous line). > > > > > > > > if (deps->deps_.empty()) { > > > > Likely inside this if() clause ? > > `deps->deps_` is empty at this point, so we cannot efficiently find all > entries in `devMap_` that belong to `deps`. > > > > > > Otherwise, if there still are dependencies to meet for the devnum at > > hand, the next time addV4L2Device(devnum) is called it won't find any item > > on the devMap_ for that key. > > My reading of the code suggests that devnum is unique and each v4l2 device > can be a dependency of at most 1 media device. Is that not correct? > Indeed, from a superficial read I thought devMap_ was associating media devices with their dependencies while instead, as you said, each dependency has its own entry in devMap_. Sorry for the noise, it makes sense to me now Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Thanks j > > > > > > LOG(DeviceEnumerator, Debug) > > > -- > > > 2.40.0 > > > > > > > > > > > Regards, > Barnabás Pőcze
diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 5317afbd..a63cd360 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -315,6 +315,7 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) * enumerator. */ deps->deps_.erase(devnum); + devMap_.erase(it); if (deps->deps_.empty()) { LOG(DeviceEnumerator, Debug)
Previously, after `addV4L2Device()` had seen all dependecies, it would remove the `MediaDeviceDeps` object from the `pending_` list, which would result in it being destroyed. However, there would still be (dangling) pointers to this object in `devMap_` that were added in `addUdevDevice()` (line 103). So remove the entry with the given devnum when it is removed from the corresponding `MediaDeviceDeps` object. Signed-off-by: Barnabás Pőcze <pobrn@protonmail.com> --- src/libcamera/device_enumerator_udev.cpp | 1 + 1 file changed, 1 insertion(+) -- 2.40.0