GNOME Bugzilla – Bug 747757
winks: add GstDeviceProvider implementations on Windows
Last modified: 2015-04-29 00:25:53 UTC
This is bad, obviously, as it prevents convenient enumeration of things like video capture devices. This should be easy to implement for winks, since it already enumerates existing devices for its own purposes.
Created attachment 301430 [details] [review] Tentative GstDeviceProvider implementation for Windows Kernel Streaming plugin gst_ks_device_provider_probe() is a no-braier, just runs ks_enumerate_devices() and reports the results. Monitoring is a bit more tricky. We have to create a dummy message-processing window and register device change notifications for it.
This is mostly me throwing things against a wall to see if they stick. attachment 301430 [details] [review], for example, has no device removal code, and it adds the same device multiple times (because a single device has multiple aliases, apparently). I am also unsure about thread safety of it all (is it safe to add devices from another thread?). And finally, this implementation does non-trivial stuff inside message processor, which is frowned upon (it can stall the system). Instead it should send guids of the devices that change into another thread (back into the main thread?) and let it do the processing there.
Created attachment 301434 [details] [review] GstDeviceProvider implementation for Windows Kernel Streaming plugin OK, this turned out to be easier than i thought. I've added GST_OBJECT_LOCK/UNLOCK everywhere, hopefully it's somewhat threadsafe now. Realized that notifications came in tiers, for example: USB device, then video device, then capture device. Also, turned out that devices can be enumerated with both GUIDs (devtype and category) being the same. So since we (eventually) get a notification for "capture device", this is what we should work with, and ignore the rest. Removal is even easier, as it's enough to just compare device paths. Cleaned up excessive if-nesting, separated debug logging into its own (locked) part. Note that there are some placeholder pieces of code for audio capture and audio playback here. Winks does not support these, but it might do so in time. Some kind of video playback via Kernel Streaming should be theoretically possible, but i've failed to google up any examples of it in the wild, hence no placeholders for it. I'm still unclear how to report device index to the base provider class, and whether it's a valid way to identify a device at all.
Review of attachment 301434 [details] [review]: Generally looks good. Did you test with gst-device-monitor-1.0 and "gst-device-monitor-1.0 -f" and plug/unplug a couple devices? ::: sys/winks/ksdeviceprovider.c @@ +490,3 @@ + GST_OBJECT_LOCK (self); + GST_DEBUG_OBJECT (self, "Exiting internal window thread: %p", + g_thread_self ()); Why take the lock here?
(In reply to Olivier Crête from comment #4) > Review of attachment 301434 [details] [review] [review]: > > Generally looks good. Did you test with gst-device-monitor-1.0 and > "gst-device-monitor-1.0 -f" and plug/unplug a couple devices? I only have one webcamera, and i tested *that*. I could probably hook it up to a laptop to test addition and removal of a second device, but, obviously, can't remove the built-in one at runtime. > > ::: sys/winks/ksdeviceprovider.c > @@ +490,3 @@ > + GST_OBJECT_LOCK (self); > + GST_DEBUG_OBJECT (self, "Exiting internal window thread: %p", > + g_thread_self ()); > > Why take the lock here? Well, i thought that you lock self whenever you use self. GST_DEBUG_OBJECT() takes self, so i lock.
(In reply to LRN from comment #5) > (In reply to Olivier Crête from comment #4) > > Review of attachment 301434 [details] [review] [review] [review]: > > > > Generally looks good. Did you test with gst-device-monitor-1.0 and > > "gst-device-monitor-1.0 -f" and plug/unplug a couple devices? > I only have one webcamera, and i tested *that*. I could probably hook it up > to a laptop to test addition and removal of a second device, but, obviously, > can't remove the built-in one at runtime. That's already a good test, thank you ! > > ::: sys/winks/ksdeviceprovider.c > > @@ +490,3 @@ > > + GST_OBJECT_LOCK (self); > > + GST_DEBUG_OBJECT (self, "Exiting internal window thread: %p", > > + g_thread_self ()); > > > > Why take the lock here? > > Well, i thought that you lock self whenever you use self. GST_DEBUG_OBJECT() > takes self, so i lock. For GST_DEBUG,etc_OBJECT() macros you don't need to take the lock.
Created attachment 301849 [details] [review] GstDeviceProvider implementation for Windows Kernel Streaming plugin * Don't lock provider object just to log * Flattened some nested constructs Tested this on a PC with 2 different cameras, gst-device-monitor -f correctly detected connections and disconnections. Problem: when 2 identical cameras are connected, they appear identical in the list that gst-device-monitor provides. They probably have different paths, but gst-device-monitor does not show them. Should we deliberately change their names to make them unique-ish, or just leave it as-is (given the probably rarity of the situation)? Still no audio support (and i have no particular desire or need to do audio support), still does processing in the message thread.
Problem: if the device is identified by the device-index parameter, the device that ksvideosrc ends up using varies between runs. Most likely because device index is not a property of the device, it's determined by the order in which devices are enumerated, and that order changes, apparently, between runs. I guess this is a leftover from the time when winks had no device provider, thus the only way to list devices was to make winks spew enumeration debug output, so identifying devices by index seemed like a good idea.
The device-index property is only really useful for debugging with the command line, when you don't know the device name. It shouldn't be used as a device identifier.
We can just remove the property if it doesn't make much sense or isn't useful, fwiw.
(In reply to Tim-Philipp Müller from comment #10) > We can just remove the property if it doesn't make much sense or isn't > useful, fwiw. For testing it's very useful, and it's property I miss in the Direct Show source for debugging . In linux the device name is usually a path to a file, which is easy to find/use, but in Windows it's a long string that's not so easy to figure out :)
Created attachment 302528 [details] [review] GstDeviceProvider implementation for WIN Kernel Streaming plugin Re-committed with gst-indent, now with style changes.
Review of attachment 302528 [details] [review]: The GstDeviceProvider itself seems to be missing from the patch. ::: sys/winks/gstksclock.c @@ -226,3 @@ GST_KS_CLOCK_LOCK (); } - Why remove all of the empty lines here and below? ::: sys/winks/gstksvideosrc.c @@ -468,3 @@ gst_ks_video_src_alloc_buffer, self); } - Why remove all the the empty lines again? @@ +1070,3 @@ + + GST_DEBUG_CATEGORY_INIT (gst_ks_debug, "ksvideosrc", + 0, "Kernel streaming video source"); Do the category init before the register, in case there are debug messages in the class init in the future
(In reply to Olivier Crête from comment #13) > Review of attachment 302528 [details] [review] [review]: > > The GstDeviceProvider itself seems to be missing from the patch. > > ::: sys/winks/gstksclock.c > @@ -226,3 @@ > GST_KS_CLOCK_LOCK (); > } > - > > Why remove all of the empty lines here and below? Because gst-indent told me so. > > ::: sys/winks/gstksvideosrc.c > @@ -468,3 @@ > gst_ks_video_src_alloc_buffer, self); > } > - > > Why remove all the the empty lines again? Because gst-indent told me so. It wouldn't let me commit unless i removed them. > > @@ +1070,3 @@ > + > + GST_DEBUG_CATEGORY_INIT (gst_ks_debug, "ksvideosrc", > + 0, "Kernel streaming video source"); > > Do the category init before the register, in case there are debug messages > in the class init in the future Roger.
Some kind of bug in --cuddle-else, it causes indent to eat a blank line that follows an `if' block.
If this is the only thing reported by gst-indent, I would say it's find to skip it (using -n option to commit). Make sure there is no space in these blank line, since this would be a different story.
Created attachment 302531 [details] [review] GstDeviceProvider implementation for WIN Kernel Streaming plugin v2: Re-committed with gst-indent, now with style changes. This time used a version of gst-indent that does not remove empty lines after `if' blocks when --cuddle-else option is specified. This time i also didn't forget to actually add the two new files :-/
Thank you, merged! commit a31855d61808dfb4393c939fc42160ee765501b0 Author: Руслан Ижбулатов <lrn1986@gmail.com> Date: Tue Apr 28 21:43:56 2015 +0000 GstDeviceProvider implementation for WIN Kernel Streaming plugin gst_ks_device_provider_probe() is a no-braier, just runs ks_enumerate_devices() and reports the results. Monitoring is a bit more tricky. We have to create a dummy message-processing window and register device change notifications for it. As kernel streaming can (and should) be used for audio capture and audio playback, this change also has certain placeholders for such. https://bugzilla.gnome.org/show_bug.cgi?id=747757