[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 #46 from Tim-Philipp Müller <t.i.m at zen.co.uk> 2013-11-05 00:04:08 UTC ---
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)

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