[Bug 678402] Device discovery/listing replacement for GstPropertyProbe

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=678402
  GStreamer | gst-plugins-base | git

--- Comment #48 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2013-11-06 22:49:45 UTC ---
> 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).

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.
You are the assignee for the bug.