GNOME Bugzilla – Bug 678402
Device discovery/listing replacement for GstPropertyProbe
Last modified: 2014-03-17 00:43:25 UTC
Since GstPropertyProbe was removed in 0.11, we need to come up with a replacement. I'm guessing that some of the core developers have already thought of how to implement this, which is my I'm creating this bug. For my needs it would return a list of devices, each with a friendly name, an internal name, and associated caps (fixed and not). The internal name or some other identifier would then be used to set the device.
Indeed. I have to find some time to write down my thoughts / requirements for this. I think there are a couple of issues we need to / want to solve: - generic device discovery - device-specific information, with things like caps, display-friendly names, more type info (is it a camera or a TV capture card or ..?) - not necessarily bound to a particular API (like e.g. v4l2 or alsa), but the whole API should be driven from what a possibly cross-platform application would typically want, e.g.: give me a list of cameras - should create the element suitably-configured from the discovered device info This is all still a bit vague I guess, but the way I see it it's a bit like a mini-HAL with device-specific API instead of something super-generic. There are only so many different types of device classes people are interested in for multimedia stuff. There was more, but I can't remember right now. Perhaps I can find time at GUADEC to hack together some prototype.
Also, moving to gst-plugins-base, since I don't think this needs to be in core. Oh, and the way I see it the device probes/discoverers would still be part of the API-specific plugins though, perhaps as some kind of new element factory (which would need core support then, but let's save that for later), so all the v4l2-specific code would be in the v4l2 plugin etc.
Tim's write-up mostly matches what I had in mind. What other categories of devices than cameras do we care about?
> What other categories of devices than cameras do we care about? CD/DVD drives, audio inputs, (audio outputs?), (video outputs?)
We have been using GstPropertyProbe also to e.g. constrain property ranges one a device got opened. (E.g. constrain the enums a property actually suppports or e.g. 'disable' the Hue property in videobalance). We could also just register a could of qdata flags and provide some gst_object_ api wrappers for elements to set them and apps to query them.
I have cases where there may be multiple devices, each of which have multiple interfaces, however this probably doesn't need any special treatment. More challenging, is that sometimes I also have to specify some configuration parameter(s), usually in the form of a configuration file or an enum, to even be able to get usable data. A good example of this would be decklinksrc, which has device instance (physical PCIe board), video mode (NTSC/PAL/etc), audio source (analog/AES), and connection (SDI/HDMI/etc). Perhaps the properties that configure the device (as opposed to something like num-buffers) could be listed by the device discovery API. Also, an indication if a given device is known to be providing valid data. In other words, whether a camera is actually connected to a given board/port. Could use enums like _CONNECTED, _NOT_CONNECTED, and _UNKNOWN (too expensive to check if connected, will have to try manually opening).
Note to self from bug #565873: for alsa we should use snd_device_name_hint() for enumerating the devices as the snd_card_* functions only list the kernel devices and no user space devices. http://0pointer.de/blog/projects/guide-to-sound-apis.html
To add to that requirement list, from the experience of Cheese and Empathy. It needs to be a device monitor, not just a one-time enumeration, modern devices are hot-pluggable.
I've done a little prototype of some API that I think makes sense, I've also done an implementation for V4L2. Missing features are: - Being able to change the target of a running source/sink (pulseaudio can do that), also an API to know where it's possible and where it's not - Some way to know if a GstDeviceMonitor supports enumeration or not API suggestions are welcome! GstDeviceMonitor: (+GstDeviceMonitorFactory) - It's a basic type like a GstElement that is in plugins methods: GPtrArray * gst_device_monitor_probe (GstDeviceMonitor * monitor); - List devices gboolean gst_device_monitor_start (GstDeviceMonitor * monitor); void gst_device_monitor_stop (GstDeviceMonitor * monitor); - Starts and stop emitting the added and removed signals signals: "added" and "removed with argument (GstDevice *) Created with gst_device_monitor_factory_make() / gst_device_monitor_factory_create() just like elements. GstDevice: methods: GstElement * gst_device_create_element (GstDevice * device, const gchar * name); - creates an element configured to use this device GstCaps * gst_device_get_caps (GstDevice * device); - The caps for the device.. should be a superset (or equal) to whatever gst_pad_query_caps() will return once the device is opened. gchar * gst_device_get_device_name (GstDevice * device); - Returns a user-friendly name (not sure if we can offer translations, I think not) signals: "removed" properties: "caps" and "device-name" .. same as the functions GstGlobalDeviceMonitor: - List devices from all possible monitors that match the listed types methods: GPtrArray * gst_global_device_monitor_probe (GstGlobalDeviceMonitor * monitor); gboolean gst_global_device_monitor_start (GstGlobalDeviceMonitor * monitor); void gst_global_device_monitor_stop (GstGlobalDeviceMonitor * monitor); - Same as GstDeviceMonitor, but does it all monitors void gst_global_device_monitor_set_type_filter ( GstGlobalDeviceMonitor * monitor, GstDeviceMonitorFactoryListType type); GstDeviceMonitorFactoryListType gst_global_device_monitor_get_type_filter ( GstGlobalDeviceMonitor * monitor); - Set/Get a filter to not get all monitors (so the type could be GST_DEVICE_MONITOR_FACTORY_TYPE_SRC | GST_DEVICE_MONITOR_FACTORY_TYPE_MEDIA_VIDEO) void gst_global_device_monitor_set_caps_filter ( GstGlobalDeviceMonitor * monitor, GstCaps * caps); GstCaps * gst_global_device_monitor_get_caps_filter ( GstGlobalDeviceMonitor * monitor); - Set/Get caps that are used to filter possible devices, devices that don't match these caps are ignored (so we can request only devices that can produce video/x-h264 or video/x-raw, etc..) Created with gst_global_device_monitor_new()
Created attachment 233031 [details] [review] devicemonitor: Add GstDeviceMonitor and related Also add GstDevice and GstDeviceMonitorFactory And add code to the registry to save them
Created attachment 233032 [details] [review] globaldevicemonitor: Add device monitor aggregator
Created attachment 233033 [details] [review] v4l2: De-duplicate caps probing between src and sink
Created attachment 233034 [details] [review] v4l2object: Allow user to not be a GstElement
Created attachment 233035 [details] [review] v4l2: Implement GstDeviceMonitor subclass
By "Some way to know if a GstDeviceMonitor supports enumeration or not", I meant... Some way to know if a GstDeviceMonitor supports monitoring or only enumeration. I should also mention that the various plugins would implement subclasses of GstDeviceMonitor and GstDevice (see the v4l2 example).
Nice work Olivier! This would be really useful API that would enable me to drop CheeseCameraDeviceMonitor in Cheese. The API seems fine to me on first glance, and is more generic than the custom monitor that Cheese has. I wonder if the name property on GstDevice is something that would be unique and be preserved across replugging? Currently, Cheese stores the path to the active V4L2 device in GSettings, so that the choice can be preserved across application restarts, and although I could do the same thing with a GStreamer device monitor, I wonder if a GstDevice should directly provide some kind of UUID or persistent identifier. Anyway, this could be added at some later time, and the API already seems really useful. I will see if I can find some time to work on some patches for Cheese as an example consumer.
We should probably add some kind of persistent string, my current thinking is probename:unique, some for v4l2src, I'd do v4l2src:sysfspath/blah/device
Other missing feature: - In pulsesrc, the pulseaudio can change the device from under our feet (let's say if the usb mic is unplugged), we need to notify the app of that. Should these also be messages on the main loop? - Tim suggests using GstMessages for changes so they end up on the application's main loop.
@amigadave: I assume that you ideally want to re-use the same device even if it has been plugged into a different USB port? So maybe the sysfs path is too restrictive and you just want to use the "name" property (which is also the user-presentable name).
(In reply to comment #19) Yes, the name should be good enough for Cheese, and indeed the sysfs path might be too restrictive in the changing-USB-port case. If a user has multiple webcams with the same name then this might be a problem, but I do not see that as a big issue.
Comment on attachment 233033 [details] [review] v4l2: De-duplicate caps probing between src and sink Cleanup not directly related to this bug, lets push it now. commit 48caa1712a76e04522348ed209cb248e0de166e0 Author: Olivier Crête <[email protected]> Date: Mon Oct 22 17:58:07 2012 -0400 v4l2: De-duplicate caps probing between src and sink
Created attachment 252654 [details] [review] message: Add GST_MESSAGE_EXTENDED This is requires because we're running out of GstMessageTypes
Created attachment 252655 [details] [review] devicemonitor: Add GstDeviceMonitor and related Also add GstDevice and GstDeviceMonitorFactory And add code to the registry to save them
Created attachment 252656 [details] [review] globaldevicemonitor: Add device monitor aggregator
Created attachment 252657 [details] [review] v4l2object: Allow user to not be a GstElement
Created attachment 252658 [details] [review] v4l2: Implement GstDeviceMonitor subclass
Created attachment 252659 [details] [review] v4l2: Remove GstPropertyProbe leftovers
Created attachment 252660 [details] [review] pulse: Make gst_pulse_format_info_to_caps() shared
Created attachment 252661 [details] [review] pulse: Add device monitors
Created attachment 252662 [details] [review] pulsedevicemonitor: PulseSrc can be reconfigured at runtime This depends on the patches in bug #590768 to be able t actually reconfigure the source
New set of patches, it addresses all of the previously stated concerns: 1. gst_device_monitor_can_monitor() to know if a monitor can live-monitor 2. gst_device_reconfigure_element() to reconfigure an element in the PLAYING state (with pulsesrc implementation) 3. The signals where replaced by the DEVICE message 4. The "name" property is what apps should save to get more or less the same device on restarts (as it's basically the only thing we can track across USB disconnect/reconnect) The only missing thing is having some element-agnostic way to know if the device changed from under our feet (for pulseaudio), I was thinking of letting elements emit a DEVICE message with CHANGED. Do we care about that enough ?
Created attachment 252788 [details] [review] pulse: Add device monitors Bug #590768 was fixed, so pulsesrc/pulsesink can now change the device live, so this is also in, no need for a separate patch now.
Is any of this ready for "prime time"?
I think we should get this into GIT master ASAP, get some real world testing and refine the API later if needed
I would like to review this in more detail before it goes in. Will try to do so soon.
I have a couple of needs for this...one of which is to integrate in to Ekiga. I am more than willing to test when you place in to Git.
@Robert Krakora: Would the API described in comment #9 amended by commment #31 fit your needs?
Yes, quite nicely. (In reply to comment #37) > @Robert Krakora: Would the API described in comment #9 amended by commment #31 > fit your needs?
I'm interested in this API too, needed bye WebKit for WebRTC :)
You have two willing testers who can provide you with valuable feedback now. :-)
(In reply to comment #37) > @Robert Krakora: Would the API described in comment #9 amended by commment #31 > fit your needs? I also think this API would be ok for our use-cases in WebKit.
@Note to self: Write gtk-doc before merging
Comment on attachment 252660 [details] [review] pulse: Make gst_pulse_format_info_to_caps() shared Guess this can go in already?
Comment on attachment 252657 [details] [review] v4l2object: Allow user to not be a GstElement This looks like it could go in already as well
Sorry for the delay in looking at this. In general I think this goes in the right direction, and you've done most of the hard work already. The thing that bugs me most at this point is that I think it's not really nice enough for a high-level application API IMHO, or I think it could be made nicer anyway. What this boils down to is for one the re-use of the GstElementFactory GstFactoryListType system, which I don't think is very nice. I think what I had in mind was something similar that's hierarchical and "stronger-typed". There are only so many different types of devices we're interested in really (possibly in combination with some tags with extra info). Maybe with dedicated GstFooDevice sub-classes with public API in -base. I suspect there will be more things we want to expose for different types of sources in future (e.g. media stuff for disc drives). Random drive by comments on nit-picks: - in gst_registry_chunks_{load,save}_feature() use 'dmf' instead of 'tff' as variable name for DeviceMonitorFactory (copy'n'pasted from typefinder) - 'Since: 1.2' markers need to be updated to 'Since: 1.4'; some headers are missing since markers for new doc chunks - multiple places: why use GPtrArray over GList ? Most of our other API uses GLists I think - overview docs are missing GstDevice - gstdevice.c: include gst_private.h before any other glib/gst header - should GstDeviceClass::reconfigure_element have a GError as well? [not entirely clear how reconfigure is supposed to be used though] - gst_device_get_caps() - need signalling somewhere whether these caps are likely to be the final probed caps or just template caps (not always possible to probe for the actual caps) - gst_device_get_device_name() -> gst_device_get_name() or gst_device_get_display_name(): do we add the 'class' of device (e.g. "Camera") here or just show the name (e.g. "Built-in iSight")? Need to make very clear what string it is that will be shown to the user in a UI dialog. GstDeviceMonitor: - maybe instead of _device_added/removed() just an _add/remove() and then let the GstDeviceMonitor class keep track of the device list for convience? (would one ever not want that?) - may probe() block or does it never block? If it blocks, should there be an async version? (for the benefit of whoever needs to poll) Should it take a GError * for error reporting? Or maybe we should just get rid of the function entirely and leave it up to the implementation to block in a separate thread if necessary) GstDeviceMonitorFactory: - where are the keys defined? what is it good for? re.: const gchar * gst_device_monitor_factory_get_metadata (GstDeviceMonitorFactory *factory, const gchar *key); gchar ** gst_device_monitor_factory_get_metadata_keys (GstDeviceMonitorFactory *factory); - do we need longname/description/author ? I don't think anyone would ever show a list of monitors to the user? And we don't have those things for typefinders either, for example. Get rid of all this IMHO - I think using the GstElementFactory klass system is not a good idea here, see GstGlobalDeviceMonitor comments GstGlobalDeviceMonitor: - not nice enough / high-level enough for my taste - I think using the GstElementFactory klass system is not a good idea here, it's not precise enough, I think we need something that's (a) "stronger-typed" and (b) hierarchical (so we can differentiate different types of video inputs, for example) [although maybe that applies more to GstDevice than the monitor factory, e.g. v4l2 might have both webcams and video capture devices) - if the bus is private move it into GstGlobalDeviceMonitorPrivate ? - should it be a singleton? (should we assume that one can always create multiple monitor factories of the same kind?) - not yet convinced by the filter options - one migh want to track different kinds of devices with the same monitor, or is the idea to use different monitors for that? - maybe allow setting a filter function for more flexibility too? - how about some kind of _add_filter (monitor, type, caps or NULL) ? - if a monitor doesn't support monitoring, we need to poll, so maybe we should also have API to set the poll intervall ? (Generally I think it's nicer to let the global device monitor poll and figure out if anything changed, than letting the application do the polling). - gst_global_device_monitor_probe() returns array/list of devices - I think the name should indicate what it returns (probe_devices?), but also in the old property probe interface we had the 'probing' step and the 'get_list' as separate steps (with a needs_probing API on top). Not saying that's how we should do it again, just making sure it was done on purpose like that. - leak in gst_global_device_monitor_set_type_filter() - monitor_get_bus() returns a ref GstMessage - GST_DEVICE_OPERATION_TYPE_{ADDED,REMOVED} - do we need/want the _TYPE_ in the enum as well? - what about medium/disc inserted/ejected? - is 'operation' the right word here? maybe it should be DEVICE_EVENT ? - or maybe we should have separate DEVICE_{ADDED,REMOVED,CHANGED} messages Pulse patches: - pulsedevicemonitor contains copy'n'pasted bits mentioning v4l2 - why separate soure + sink monitors - gst_pulse_device_monitor_probe(): unlock_and_fail label is misnamed (no unlock)
Thanks for doing a thorough review! (In reply to comment #46) > What this boils down to is for one the re-use of the GstElementFactory > GstFactoryListType system, which I don't think is very nice. We could always extend the klass system to have more classes ? Source|Camera vs Source|Tuner ? Having a hierarchical mode may also be a good idea. That said, having it as GObject classes may not be a great idea as we probably want to filter both monitors and actual devices, as we don't want to run the v4l2src monitor if we want to list audio sinks. > - multiple places: why use GPtrArray over GList ? Most of our other API uses > GLists I think Because you can return a ref without doing a copy. But it could also be a GList. > GstDevice > > - should GstDeviceClass::reconfigure_element have a GError as well? [not > entirely clear how reconfigure is supposed to be used though] The idea of reconfigure is that maybe you can re-use an existing element without stopping it. If reconfigure returns FALSE, then you must throw away the element and create a new one. The main use-case for reconfigure is changing the input/output of pulsesrc/pulsesink without having to start/stop them. > - gst_device_get_caps() - need signalling somewhere whether these > caps are likely to be the final probed caps or just template > caps (not always possible to probe for the actual caps) In my mind these were always probed. I'm not sure in which case they aren't? > - gst_device_get_device_name() -> gst_device_get_name() or > gst_device_get_display_name(): do we add the 'class' of device > (e.g. "Camera") here or just show the name (e.g. "Built-in iSight")? > Need to make very clear what string it is that will be shown to the > user in a UI dialog. I guess gst_device_get_display_name() it is then. > GstDeviceMonitor: > > - maybe instead of _device_added/removed() just an _add/remove() and then > let the GstDeviceMonitor class keep track of the device list for convience? > (would one ever not want that?) Good idea, I suppose that means we can also only call ->probe in the subclass if the monitor is not running (otherwise the base class will contain the list). > - may probe() block or does it never block? If it blocks, should there be > an async version? (for the benefit of whoever needs to poll) Should it > take a GError * for error reporting? Or maybe we should just get rid of > the function entirely and leave it up to the implementation to block in > a separate thread if necessary) > GstDeviceMonitorFactory: Current idea is that it never blocks. It is just to return all devices that are currently connected. > - where are the keys defined? what is it good for? re.: > const gchar * gst_device_monitor_factory_get_metadata > (GstDeviceMonitorFactory *factory, const gchar *key); > gchar ** gst_device_monitor_factory_get_metadata_keys > (GstDeviceMonitorFactory *factory); It's there on GstElementFactory, not sure why? > GstGlobalDeviceMonitor: > > - not nice enough / high-level enough for my taste Anything else than the class system you don't like ? > - should it be a singleton? (should we assume that one can always create > multiple monitor factories of the same kind?) No, we want to be able to run separate monitors for lets say camera, audio source, audio sink. It's easier for the application devs if they can just receive the right events. > - not yet convinced by the filter options - one migh want to track different > kinds of devices with the same monitor, or is the idea to use different > monitors for that? The idea is to use different monitors. > - maybe allow setting a filter function for more flexibility too? Can't the application just filter itself if it wants finer grained? But that's easy to add. > - how about some kind of _add_filter (monitor, type, caps or NULL) ? And get_filter(monitor, &type, &caps) ? > - if a monitor doesn't support monitoring, we need to poll, so maybe we > should also have API to set the poll intervall ? (Generally I think it's > nicer to let the global device monitor poll and figure out if anything > changed, than letting the application do the polling). I think if it doesn't support monitoring, it means the devices won't change (for example, PCI cards or SoC cameras). I don't think we should have global polling. If for some case we have an API where the devices can change but there is no notification, the specific monitor should do its own polling. > - gst_global_device_monitor_probe() returns array/list of devices - I think > the name should indicate what it returns (probe_devices?), but also in the > old property probe interface we had the 'probing' step and the 'get_list' > as separate steps (with a needs_probing API on top). Not saying that's how > we should do it again, just making sure it was done on purpose like that. Probing should be non-blocking, so no need for separate probe and list. > GstMessage > > - GST_DEVICE_OPERATION_TYPE_{ADDED,REMOVED} - do we need/want the _TYPE_ in > the enum as well? This is how it is done for GstProgressType, GstStreamStatusType, GstStructureChangeType, etc > - what about medium/disc inserted/ejected? How is that different from added/removed ? We may want to have separate device types or even separate monitors for disks drives and disks? > - is 'operation' the right word here? maybe it should be DEVICE_EVENT ? Isn't "event" a bit too generic? (and meaningless?).. And well EVENT already means something else in GSt. > - or maybe we should have separate DEVICE_{ADDED,REMOVED,CHANGED} messages What would changed mean? > Pulse patches: > > - why separate source + sink monitors The idea is that applications probably don't care about both, only sources or sinks. So having separate monitors means that we only get the right events. We could also add filtering inside each GstDeviceMonitor (instead of just the global one), but that didn't seem so useful to me. > ... Everything else, I agree with..
> We could always extend the klass system to have more classes ? Source|Camera vs > Source|Tuner ? Having a hierarchical mode may also be a good idea. That said, > having it as GObject classes may not be a great idea as we probably want to > filter both monitors and actual devices, as we don't want to run the v4l2src > monitor if we want to list audio sinks. The rationale for exposing actual API for derived GstDevices wasn't primarily for filtering, but because I think people will want additional information from devices sooner or later, additional information which is probably closely tied to the kind of device that you're dealing with, and doing this via 'anonymous' GObject API is not very nice. For example, one might want medium status / type on a disc drive, mount/unmount or eject operations. On a video source or sink one might want to query available video norms if available, or if it's a tunable device. And the same probably applies to other devices too. I agree that we want to filter monitors. But having a hierarchical description of the possible device categories does not preclude that, you just have to register a list of hierarchy roots in the monitor "klass" rather than a collection of tags, for example. > > - multiple places: why use GPtrArray over GList ? Most of our other API uses > > GLists I think > > Because you can return a ref without doing a copy. But it could also be a > GList. True, but you have to return a copy anyway because of writability semantics (some other thread of the monitor might ask the monitor to add/remove a device while some other code is using the reffed array), no? Not that it really matters, was just wondering. > The idea of reconfigure is that maybe you can re-use an existing element > without stopping it. If reconfigure returns FALSE, then you must throw away the > element and create a new one. The main use-case for reconfigure is changing the > input/output of pulsesrc/pulsesink without having to start/stop them. Ok, makes sense. (would public API on GstElement be more suitable, even if it just ended up calling the same vfunc?) > > - gst_device_get_caps() - need signalling somewhere whether these > > caps are likely to be the final probed caps or just template > > caps (not always possible to probe for the actual caps) > > In my mind these were always probed. I'm not sure in which case they aren't? You seem to assume you can always probe things, and with well-behaved APIs that's the case. But there may be others. You might only be able to probe a device if it's currently not in use, for example. Of course then one could just return NULL which is just as well. > > GstDeviceMonitor: > Current idea is that [_probe()] never blocks. It is just to return > all devices that are currently connected. ok. > > - where are the keys defined? what is it good for? re.: > > const gchar * gst_device_monitor_factory_get_metadata > > (GstDeviceMonitorFactory *factory, const gchar *key); > > gchar ** gst_device_monitor_factory_get_metadata_keys > > (GstDeviceMonitorFactory *factory); > > It's there on GstElementFactory, not sure why? That's so element factories can store additional information there, e.g. something they have probed once. I don't think we need all that stuff for the monitor factory, we should get rid of it (along with the long name, description and author) until we have a use case for it IMHO (KISS). > > GstGlobalDeviceMonitor: > > > > - not nice enough / high-level enough for my taste > > Anything else than the class system you don't like ? That, and the naming :) It's not really 'global' is it? IMHO GstGlobalDeviceMonitor should really be called GstDeviceMonitor, but then I don't know what GstDeviceMonitor should be called instead - maybe GstDeviceProbe or something. > > - should it be a singleton? (should we assume that one can always create > > multiple monitor factories of the same kind?) > > No, we want to be able to run separate monitors for lets say camera, audio > source, audio sink. It's easier for the application devs if they can just > receive the right events. Making it a singleton doesn't mean you can't have individual filters. What one would need to ensure is that different pieces of code in the same process can both use such a singleton monitor / device pool independently. This is so that libraries such as farsight can use it under the hood, while the main application itself also uses it. The way I pictured it was that the global device monitor (singleton or not) loads the device monitors that match the current filter requirements, and then it is basically a broker/filter that forwards the devices it gets from the various monitors to the 1-N listeners, applying the respective filters for each. That would require a slightly different API I guess, like passing a bus to each with the filters, or a callback. > > - not yet convinced by the filter options - one migh want to track different > > kinds of devices with the same monitor, or is the idea to use different > > monitors for that? > > The idea is to use different monitors. But why? It doesn't seem to make much sense to me. Why not just one monitor per API? It makes very optimistic assumptions about the various APIs involved IMHO, and reading through the pulse and v4l2 implementations it seems that having one monitor would simplify the code a lot, and not introduce any additional overhead apart from creating a few devices that aren't needed, which is negligible. > > - maybe allow setting a filter function for more flexibility too? > > Can't the application just filter itself if it wants finer grained? But that's > easy to add. > > > - how about some kind of _add_filter (monitor, type, caps or NULL) ? > > And get_filter(monitor, &type, &caps) ? Why would you ever need to get the filter? I was thinking of it more like a watch - you add it, and then you can remove it if you don't need it any more, but there's no need to query the parameters - the app knows those anyway. > > - if a monitor doesn't support monitoring, we need to poll, so maybe we > > should also have API to set the poll intervall ? (Generally I think it's > > nicer to let the global device monitor poll and figure out if anything > > changed, than letting the application do the polling). > > I think if it doesn't support monitoring, it means the devices won't change > (for example, PCI cards or SoC cameras). I don't think we should have global > polling. If for some case we have an API where the devices can change but there > is no notification, the specific monitor should do its own polling. Ok, so we just leave the polling interval to the implementation in question then. That would work. It's not so important anyway, could still be added later if more control was needed. > Probing should be non-blocking, so no need for separate probe and list. Ack. > > - GST_DEVICE_OPERATION_TYPE_{ADDED,REMOVED} - do we need/want the _TYPE_ in > > the enum as well? > > This is how it is done for GstProgressType, GstStreamStatusType, > GstStructureChangeType, etc Ok, guess it's a bit inconsistent (e.g. Gst{Query,Message}Type -> GST_QUERY_FOO) > > - what about medium/disc inserted/ejected? > > How is that different from added/removed ? We may want to have separate device > types or even separate monitors for disks drives and disks? How would you express this? Does the inserted medium show up as a new device? Not sure that maps well to how applications would want to use this. I think it could be done as "device X changed: changetype = medium_inserted/ejected" or similar.. > > - is 'operation' the right word here? maybe it should be DEVICE_EVENT ? > > Isn't "event" a bit too generic? (and meaningless?).. And well EVENT already > means something else in Gst. Well, it would be an event, just a device event. As I see it, an "operation" is some action that's to be triggered or going on over a longer period of time, and device-added/removed does not seem like an operation to me. Seems easier to just differentiate it using different message types, if we're not restricted by the number of flags any more with the extended type. > > - or maybe we should have separate DEVICE_{ADDED,REMOVED,CHANGED} messages > > What would changed mean? See above, e.g. disc medium inserted, but could be anything else too. Maybe some setting on an external device like a camera was changed on the device itself. > > Pulse patches: > > > > - why separate source + sink monitors > > The idea is that applications probably don't care about both, only sources or > sinks. So having separate monitors means that we only get the right events. We > could also add filtering inside each GstDeviceMonitor (instead of just the > global one), but that didn't seem so useful to me. I think it would simplify things to just have one monitor for all devices for any given API, unless there are good reasons to have separate ones (e.g. separate ways of querying the different types of devices).
(In reply to comment #48) > The rationale for exposing actual API for derived GstDevices wasn't primarily > for filtering, but because I think people will want additional information from > devices sooner or later, additional information which is probably closely tied > to the kind of device that you're dealing with, and doing this via 'anonymous' > GObject API is not very nice. If we want to add additional information later, we can always create subclasses, I don't think this is required now. > For example, one might want medium status / type on a disc drive, mount/unmount > or eject operations. On a video source or sink one might want to query > available video norms if available, or if it's a tunable device. And the same > probably applies to other devices too. Most of those things could just be properties? Operations like eject might make sense, but nothing prevents us from adding GstDevice subclasses later. > I agree that we want to filter monitors. But having a hierarchical description > of the possible device categories does not preclude that, you just have to > register a list of hierarchy roots in the monitor "klass" rather than a > collection of tags, for example. We could make the "klass" into a "path".. Use use a g_str_prefix() has a filter. So we could have klasses like "/Source/Video/Camera/V4l2/" which would match "/Source/" or "/Source/Video/". And I guess we could put the klass into both the filter and the gstdevice itself.. That clashes a bit with your other suggestion.. if you have a single monitor for both pulsesrc and pulsesink, what klass would it be ? > > > - multiple places: why use GPtrArray over GList ? Most of our other API uses > > > GLists I think > > > > Because you can return a ref without doing a copy. But it could also be a > > GList. > > True, but you have to return a copy anyway because of writability semantics > (some other thread of the monitor might ask the monitor to add/remove a device > while some other code is using the reffed array), no? Not that it really > matters, was just wondering. Good point, GList it is. > > > - gst_device_get_caps() - need signalling somewhere whether these > > > caps are likely to be the final probed caps or just template > > > caps (not always possible to probe for the actual caps) > > > > In my mind these were always probed. I'm not sure in which case they aren't? > > You seem to assume you can always probe things, and with well-behaved APIs > that's the case. But there may be others. You might only be able to probe a > device if it's currently not in use, for example. Of course then one could just > return NULL which is just as well. I guess if probing is impossible it might return some kind of template caps. I think even probed caps can possibly be a superset of what is actually possible right now. > > > - where are the keys defined? what is it good for? re.: > > > const gchar * gst_device_monitor_factory_get_metadata > > > (GstDeviceMonitorFactory *factory, const gchar *key); > > > gchar ** gst_device_monitor_factory_get_metadata_keys > > > (GstDeviceMonitorFactory *factory); > > > > It's there on GstElementFactory, not sure why? > > That's so element factories can store additional information there, e.g. > something they have probed once. I don't think we need all that stuff for the > monitor factory, we should get rid of it (along with the long name, description > and author) until we have a use case for it IMHO (KISS). I'd like to keep long name/desc/author so we can add an option to list monitors to gst-inspect. But metadata can do, I guess we can re-add it later. > > > GstGlobalDeviceMonitor: > > > > > > - not nice enough / high-level enough for my taste > > > > Anything else than the class system you don't like ? > > That, and the naming :) It's not really 'global' is it? IMHO > GstGlobalDeviceMonitor should really be called GstDeviceMonitor, but then I > don't know what GstDeviceMonitor should be called instead - maybe > GstDeviceProbe or something. I'm not a super fan of the name. I don't think Probe is good either (as it's not only probing!). Maybe the Global one could be a GstAggregatedDeviceMonitor ? > > > - should it be a singleton? (should we assume that one can always create > > > multiple monitor factories of the same kind?) > > > > No, we want to be able to run separate monitors for lets say camera, audio > > source, audio sink. It's easier for the application devs if they can just > > receive the right events. > > Making it a singleton doesn't mean you can't have individual filters. You mean have a: GstBus * gst_singleton_global_device_monitor_add_filter(caps, klass, filter_func, user_data); ? This kind of API makes it harder to extend the filtering later. If the Aggregator is an object, then more functions/properties can be added later for more types of filtering. I'm not sure what the benefits of having a singleton are ? > The way I pictured it was that the global device monitor (singleton or not) > loads the device monitors that match the current filter requirements, and then > it is basically a broker/filter that forwards the devices it gets from the > various monitors to the 1-N listeners, applying the respective filters for > each. That would require a slightly different API I guess, like passing a bus > to each with the filters, or a callback. Why have a single one? Maybe it makes sense to have each user create it's own "monitor aggregator" with appropriate filters, but that the actual monitors each be singleton that is running as long as any aggregator is running. It might make sense if each monitor is a singleton though, if we only do the filtering in the "global" one. Maybe this could be done with some help from the factory (having a flag that makes the factory always return the same one if possible). That said, I'm not sure there is a huge benefit in having anything be singleton. Except maybe the probing part. > > > - not yet convinced by the filter options - one migh want to track different > > > kinds of devices with the same monitor, or is the idea to use different > > > monitors for that? > > > > The idea is to use different monitors. > > But why? It doesn't seem to make much sense to me. Why not just one monitor per > API? It makes very optimistic assumptions about the various APIs involved IMHO, > and reading through the pulse and v4l2 implementations it seems that having one > monitor would simplify the code a lot, and not introduce any additional > overhead apart from creating a few devices that aren't needed, which is > negligible. You mean have one monitor for sinks and sources ? I could be convinced of that, but it doesn't match well with the hierarchical model well as mentioned earlier. > > > - how about some kind of _add_filter (monitor, type, caps or NULL) ? > > > > And get_filter(monitor, &type, &caps) ? > > Why would you ever need to get the filter? I was thinking of it more like a > watch - you add it, and then you can remove it if you don't need it any more, > but there's no need to query the parameters - the app knows those anyway. I just like getters to match setters. > > > - what about medium/disc inserted/ejected? > > > > How is that different from added/removed ? We may want to have separate device > > types or even separate monitors for disks drives and disks? > > How would you express this? Does the inserted medium show up as a new device? > Not sure that maps well to how applications would want to use this. I think it > could be done as "device X changed: changetype = medium_inserted/ejected" or > similar.. I would have the disk show up as it's own GstDevice, of a type different from the drive device. I think GstDevice objects property should never change. Changing properties means applications need to remember the old ones and do diffs, and that's painful. > > > - is 'operation' the right word here? maybe it should be DEVICE_EVENT ? > > > > Isn't "event" a bit too generic? (and meaningless?).. And well EVENT already > > means something else in Gst. > > Well, it would be an event, just a device event. As I see it, an "operation" is > some action that's to be triggered or going on over a longer period of time, > and device-added/removed does not seem like an operation to me. Seems easier to > just differentiate it using different message types, if we're not restricted by > the number of flags any more with the extended type. Yeah, maybe we need a better name than operation or event.
> If we want to add additional information later, we can always create > subclasses, I don't think this is required now. Agreed. > > medium status / type on a disc drive, mount/unmount or eject operations. > > video source or sink one might want to query available video norms (..) > > Most of those things could just be properties? Operations like eject might make > sense, but nothing prevents us from adding GstDevice subclasses later. Yeah, but then you have properties on anonymous GstDevice subclasses that can't be inspected in gst-inspect, and will be documented in weird locations in the docs, if at all. But yes, we can figure that out later. > We could make the "klass" into a "path".. Use use a g_str_prefix() has a > filter. So we could have klasses like "/Source/Video/Camera/V4l2/" which would > match "/Source/" or "/Source/Video/". And I guess we could put the klass into > both the filter and the gstdevice itself.. Right, that was the idea. > That clashes a bit with your other suggestion.. if you have a single monitor > for both pulsesrc and pulsesink, what klass would it be ? We could attach multiple paths/klasses to a monitor, or just make it a comma-separated list of paths/klasses. > I guess if probing is impossible it might return some kind of template caps. I > think even probed caps can possibly be a superset of what is actually possible > right now. One might want to differentiate the two cases, because applications will likely assume that if they use a capsfilter that is compatible with the probed caps, stuff will work. Can still figure this out later of course, was just wondering. > > we should get rid of that stuff (along with the long name, description > > and author) until we have a use case for it IMHO (KISS). > > I'd like to keep long name/desc/author so we can add an option to list monitors > to gst-inspect. But metadata can do, I guess we can re-add it later. We can list them in gst-inspect even without a long name, description or author. Those things only make sense for elements IMHO, where you want to show things to users in a GUI and have them select them. Anyway, not so important. > I'm not a super fan of the name. I don't think Probe is good either (as it's > not only probing!). Maybe the Global one could be a GstAggregatedDeviceMonitor > ? Agreed that DeviceProbe is not very good either. I prefer Global to Aggregated. I suppose GstDeviceMonitorMonitor is out too ? ;-) Will see if other people have some ideas. > > Making it a singleton doesn't mean you can't have individual filters. > > You mean have a: > GstBus * gst_singleton_global_device_monitor_add_filter(caps, klass, > filter_func, user_data); ? > This kind of API makes it harder to extend the filtering later. If the > Aggregator is an object, then more functions/properties can be added later for > more types of filtering. One could make a filter helper object if you think we need to make filtering more extensive in future. I suspect in practice apps and libraries will just do any additional filtering in their own functions anyway though. You could also require the caller to pass/set a bus instead of returning one (not that that's directly related). > I'm not sure what the benefits of having a singleton are ? It just seemed the most natural setup to me. The benefits are that you only ever have one monitor instantiated for each API type, which means you avoid unnecessary threads/connections/overhead when you have multiple libs and the app all monitoring devices. I'm not married to the idea though. > Why have a single one? Maybe it makes sense to have each user create it's own > "monitor aggregator" with appropriate filters, but that the actual monitors > each be singleton that is running as long as any aggregator is running. > > It might make sense if each monitor is a singleton though, if we only do the > filtering in the "global" one. Maybe this could be done with some help from the > factory (having a flag that makes the factory always return the same one if > possible). Right, that would achieve the same. > You mean have one monitor for sinks and sources ? I could be convinced of that, > but it doesn't match well with the hierarchical model well as mentioned > earlier. See remarks above, I don' think a monitor needs to have only one single klass/path, it can cover multiple ones. > > > > - what about medium/disc inserted/ejected? > I would have the disk show up as it's own GstDevice, of a type different from > the drive device. I think that's a bit weird, but I guess it would work. > I think GstDevice objects property should never change. Changing properties > means applications need to remember the old ones and do diffs, and that's painful. Why does it mean apps have to remember the old ones? Usually they will just update their internal state to the new one and not care about the old value, no? Not sure if the goal of device object properties never changing will be practical, but we can see how it goes I guess. > > > > - is 'operation' the right word here? maybe it should be DEVICE_EVENT ? > > > (...) > Yeah, maybe we need a better name than operation or event. Still think making it separate message is easiest, but maybe we can find a better name too.
Created attachment 261206 [details] [review] message: Add GST_MESSAGE_EXTENDED
Created attachment 261207 [details] [review] devicemonitor: Add GstDeviceMonitor and related Also add GstDevice and GstDeviceMonitorFactory And add code to the registry to save them
Created attachment 261208 [details] [review] globaldevicemonitor: Add device monitor aggregator
Created attachment 261209 [details] [review] v4l2: Implement GstDeviceMonitor subclass
Created attachment 261210 [details] [review] pulse: Add device monitors
Changes since last patchset: - fixed Since: docstrings - Replaced GPtrArray with GList in API - fixed gst_registry_chunks_{load,save}_feature() to use 'dmf' instead of 'tff' - in gstdevice.c: moved include gst_private.h before any other glib/gst header - renamed gst_device_get_device_name to .._get_display_name(), also renamed the matching property to "display-name" - documented that _probe never blocks - GstBus in Monitor and GlobalMonitor moved to private struct - replaced gst_device_monitor_device_added/removed with add/remove and keep list in GstDeviceMonitor itself. Only call _probe in the subclass if its not running. - Renamed gst_device_monitor_probe() to _get_devices(), same for the global one - Now parenting the GstDevice to the DeviceMonitor that created it while they are kept in the list.. But not for those that are from _get_devices (since the enduser owns them). I guess for the user it's the same, they just unref them all. - Now keeping track of start/stop count in GstDeviceMonitor, so it can be shared. Need to call _stop() as many times as _start() was called to really stop - Separated the device message into device-added and device-removed - Made GstDeviceMemory into a singleton, the GstDeviceMonitorFactory already returns the same one - Don't emit all possible devices on _start(), the user should do _start(); _get_devices() to get the initial ones The metadata keys are actually used, they are used like for GstElementFactory. I'm actually reusing the GstElement keys. Should we make a second copy of the defines? The more I think about it, the more I think that having separate "tags" instead of a path or hierarchy for the "klass" system is a good idea. Using flags though probably isn't, I guess matching strings should be enough. So pulsemonitor would declare "Audio/Source/Sink", but the GstDevice it would produce would only have "Audio/Source" or "Audio/Sink", could even have "Audio/Sink/Headphones" vs "Audio/Sink/Speakers". We should define a canonical list of tags in the doc. We'd probalby have a DVD drive be "DVD/CD/Drive" and a Bluray drive be "DVD/CD/Bluray/Drive", but the disk would only be "CD/CDAudio/Audio/Disk" or "CD/CDROM/Disk" or "DVD/DVDVideo/Video/Disk" ? The drive monitor might have "CD/DVD/Bluray/Disk/Drive/CDROM/CDAudio/Audio/Video/etc/etc/etc". The "global" monitor would then have a _set_classes_filter(gchar*) function. One could pass for example "Video/Disk", and then it would return any GstDevice that's a DVD or Bluray or CDVideo. I'm not sure what to for the case where we have both pulsemonitor and alsamonitor, in that case, we probably only want one monitor loaded. Maybe using ranks? So we would order monitors by rank and only picks those with the top rank, and only go to the second rank if _start() failed for every top-rank ones, etc. And maybe somehow let the application select which monitors go into the global one? Left todo: - New name for GstGlobalDeviceMonitor - Write doc - Implement device class filtering (tags or hierarchy?) - Merge source and sink monitors - Add listing to gst-inspect - Add device listing/monitoring command-line tool
Another use-case to consider for aggregation is the uvch264src vs v4l2src case. When you have 2 cameras, one is a regular v4l2 (non-encoding) camera and one is a v4l2 uvc h264 encoding one. v4l2src will expose both, but uvch264src will only expose the h264 encoding one. We need the GstDevice from uvch264src to shadow the v4l2src GstDevice for the same device, but let other v4l2src GstDevices for other devices through.
Good point.
Hello, This is a friendly bump since I found out this bug is affecting Pidgin voice and video implementation for Win32. To quote: >| gst 0.10 is unstable on win32 and not maintained anymore, but 1.0 doesn’ have any replacement for GstPropertyProbe. Is there any milestone planned for it's implementation as a plugin and/or into the core of gst? As soon as we can get that the development team over at Pidgin could begin working on a build for Windows. Thanks.
I took the time to ask on irc, and __tim told me the issue would be discussed at a hackfest 14-16 march ; there is still hope it will end up in 1.4.
Great, glad the developers are being diligent with this feature. Don't want something that is half-baked. :)
Comment on attachment 261206 [details] [review] message: Add GST_MESSAGE_EXTENDED Committed, but done a bit differently again after discussion with Wim and others at the hackfest.
Excellent! When will it be available in a release?
"Soon" - we will start doing 1.3.x pre-releases in the next 1-2 weeks hopefully, and then it will be in 1.4.0 when it comes out in a month or so.
Open issues: - class filtering - merging sink & source monitors - uvch264src vs wrapperv4l2src case Pulse monitors not pushed for now, olivier will merge sink+src first. Video4Linux patches don't apply and need updating.
The GstBus message filtering also needs to be fixed for the new extended IDs (Jan is looking at that).
Comment on attachment 252659 [details] [review] v4l2: Remove GstPropertyProbe leftovers Push the relevant half of this one, the other half had already been done.
Comment on attachment 252657 [details] [review] v4l2object: Allow user to not be a GstElement This one is now obsolete.
I committed updated versions of the v4l2 and pulse patches, I think the goal of this bug has been reached, let's open more targeted bugs for new issues. commit cfa58778992aef5ab229ab6a2421db6f86c0dd29 Author: Olivier Crête <[email protected]> Date: Fri Nov 2 13:33:13 2012 +0100 v4l2: Implement GstDeviceMonitor subclass https://bugzilla.gnome.org/show_bug.cgi?id=678402 commit 019a0009afa5ccb8bc5d5d78d3bafc3dbf46d811 Author: Olivier Crête <[email protected]> Date: Mon Aug 12 11:49:21 2013 -0400 pulse: Add device monitors https://bugzilla.gnome.org/show_bug.cgi?id=678402 commit fdceedb77c2f224422f242f3e87c7bb55edcd380 Author: Olivier Crête <[email protected]> Date: Sun Mar 16 19:24:26 2014 -0400 v4l2: Remove GstPropertyProbe leftovers And in the core: commit e743fac26bd5f36268bf1035f33ab900d956b1e3 Author: Olivier Crête <[email protected]> Date: Sun Mar 16 15:56:59 2014 -0400 device: Add "klass" to GstDevices commit b8078e26566faf5045fd6e9addc12e77ec12b917 Author: Olivier Crête <[email protected]> Date: Sun Mar 16 18:02:56 2014 -0400 devicemonitor: Make classes into pure strings Instead of having strings & flags, make them just strings