Camera::new() modifies global state to do discovery.

[de-vri-es]

arv_camera_new() internally does camera discovery. This modifies global state inside Aravis, so it's probably not thread safe.

The method needs to acquire a global lock. Either we can re-use the Aravis struct, or switch to an actual global lock.

[de-vri-es]

Actually, distinct Aravis objects should already be safe to use from different threads according to:

https://aravisproject.github.io/docs/aravis-0.8/thread-safety.html

Aravis is not thread safe, which means one can not use the same object simultaneously from different threads, without using mutexes. But it is perfectly fine to use different aravis objects in different threads.

Additionally, the global state used for camera discovery appears to be protected by a mutex:
https://github.com/AravisProject/aravis/blob/c2f21a085dff256fa5d1aa8da225ac6b4d22eb17/src/arvgvinterface.c#L665-L666

There may still be race conditions in performing discovery and enumerating the results without additional locking though. That needs a bit more inspection.

[EmmanuelP]

Hi Maarten,

Additionally, the global state used for camera discovery appears to be protected by a mutex:
https://github.com/AravisProject/aravis/blob/c2f21a085dff256fa5d1aa8da225ac6b4d22eb17/src/arvgvinterface.c#L665-L666

There may still be race conditions in performing discovery and enumerating the results without additional locking though. That needs a bit more inspection.

arv_camera_new calls arv_open_device, which uses the global mutex to avoid a modification of the discovered device list when searching for the requested one. So in principle, arv_camera_new should be thread safe.

But the rest of the API in arvsystem.c is not, and should only be used from one thread if not protected by a user mutex, and if arv_camera_new is used, it should also be called from the same thread to not mess with the other functions.

[de-vri-es]

Hey Emmanuel,

Thanks for the clarification. I figured as much. We don't want other threads to interleave their arv_update_device_list(), arv_get_n_devices() and arv_get_device_*() calls with our own. It could be nice to have a function that returns the device info in an owned list, but it's also not much trouble to add a small wrapper here (which we already have anyway).

To be clear, if we've used the above functions properly, and then call arv_camera_new() later or on a different thread with information acquired earlier, that's no problem right? I'm assuming that since we're just passing an IP/ID/name rather than an index to arv_camera_new(), it's no problem if someone else called arv_update_device_list() in the mean time. Plus, from my understanding, arv_camera_new() will potentially call arv_update_device_list() itself.

---

The next bit is mostly at myself, so I don't forget:

I think the thread safety wrapper I added is a bit broken. The intent was to have a global token that can only be instantiated once, which would not be shareable with other threads. However, looking back at the code, I don't see anything that prevents sharing it with other threads. The token can not be cloned, but nothing prevents you from moving it into an Arc (shared pointer) and sharing that with other threads.

I'm thinking it's probably better to just use a global mutex instead. That also means users don't need to worry about how and when they obtain the token, at the cost of potentially blocking in a multi-threaded environment.

[EmmanuelP]

To be clear, if we've used the above functions properly, and then call arv_camera_new() later or on a different thread with information acquired earlier, that's no problem right? I'm assuming that since we're just passing an IP/ID/name rather than an index to arv_camera_new(), it's no problem if someone else called arv_update_device_list() in the mean time. Plus, from my understanding, arv_camera_new() will potentially call arv_update_device_list() itself.

Yes, arv_camera_new() will allways work (well, it is meant to work) from any thread. And if you want to use arv_update_device_list(), arv_get_n_devices() and arv_get_device_*() API, that should only happen from one thread, and any call to arv_camera_new() should also happen from this thread.

[de-vri-es]

Ah, so arv_update_device_list() from another thread does interfere with arv_camera_new(). Then I need to add more locking :)

[EmmanuelP]

It is the other way around, arv_camera_new() perturbates the sequence of arv_update_device_list(), arv_get_n_devices() and arv_get_device_*() calls in another thread.

[de-vri-es]

Ah, ok. Either way, more locking :)

Thank again for the extra information.