[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 #47 from Olivier Crete (Tester) <olivier.crete at ocrete.ca> 2013-11-05 00:42:55 UTC ---
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..

-- 
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.