GNOME Bugzilla – Bug 797338
dshowvideosrc: add device-index, add GstDeviceMonitor, and several fixes
Last modified: 2018-11-03 14:37:00 UTC
Created attachment 374042 [details] [review] 0001 - dshowsrcwrapper: add debug category for general dshowsrcwrapper It seems the DirectShow source dshowvideosrc hasn't seen much attention lately. The plugin winks provides ksvideosrc which supports most modern video sources, that is devices supporting the USB Video Class (UVC). However there are still several sources which are stuck with DirectShow, which I happen to be stuck using, so I worked on it a bit to make it usable for me. They can be seen here: https://github.com/joshdoe/gst-plugins-bad/tree/dshowsrc There are 11 patches, should I upload them all here? I already did some squashing, but perhaps could go further to limit the number of patches.
Created attachment 374043 [details] [review] 0002 - dshowsrcwrapper: support building 32- and 64-bit with CMake
Created attachment 374044 [details] [review] 0003 - dshowsrcwrapper: update CMake build instructions
Created attachment 374045 [details] [review] 0004 - dshowsrcwrapper: add device-index property to sources
Created attachment 374046 [details] [review] 0005 - dshowsrcwrapper: add some helpful debug statements
Created attachment 374047 [details] [review] 0006 - dshowvideosrc: fix template caps to reflect actual supported caps
Created attachment 374048 [details] [review] 0007 - dshowvideosrc: delay selecting device until source is started
Created attachment 374049 [details] [review] 0008 - dshowvideosrc: handle empty strings for device and device-name
Created attachment 374050 [details] [review] 0009 - dshowsrcwrapper: add get_property implementation to sources
Created attachment 374051 [details] [review] 0010 - dshowsrcwrapper: refactor device selection, filter creation, and caps retrieval
Created attachment 374052 [details] [review] 0011 - dshowsrcwrapper: add implementation of GstDeviceMonitor
Review of attachment 374042 [details] [review]: lgtm
Review of attachment 374043 [details] [review]: We support building all of gstreamer with MSVC with Meson now, and we intend to remove these old CMake files. This will likely happen immmediately after the 1.16 release. Please try to move your workflow to Meson. :)
Review of attachment 374044 [details] [review]: lgtm, with the caveat that this will be removed after 1.16 is released.
Review of attachment 374045 [details] [review]: lgtm
Review of attachment 374046 [details] [review]: ::: sys/dshowsrcwrapper/gstdshowvideosrc.cpp @@ +486,3 @@ GstDshowVideoSrc *src = GST_DSHOWVIDEOSRC (bsrc); + GST_DEBUG_OBJECT (src, "start"); Is this a stray debug log? If not this should contain more information.
Review of attachment 374048 [details] [review]: ::: sys/dshowsrcwrapper/gstdshowvideosrc.cpp @@ +497,3 @@ + + gst_dshowvideosrc_create_capture_filter (src); + The device should indeed not be opened in get_caps, but it should also not be opened in start(), it should be opened in change_state() when going from NULL -> READY. See the winks src for an example. dshowsrcvideo also already has gst_dshowvideosrc_change_state() so this function need to just be moved there.
Review of attachment 374049 [details] [review]: It this required to handle cases where the user has set the property to "" instead of not setting it at all?
Review of attachment 374051 [details] [review]: ::: sys/dshowsrcwrapper/gstdshow.cpp @@ +34,3 @@ }; + Accidental whitespace? @@ +360,3 @@ + gchar *friendly_name = + g_utf16_to_utf8 ((const gunichar2 *) varFriendlyName.bstrVal, + wcslen (varFriendlyName.bstrVal), NULL, NULL, NULL); Maybe you want to refactor this into a utility function? I noticed that you were repeating this a lot in various files. :)
Review of attachment 374052 [details] [review]: Looks good overall! Can be pushed once the copyright header is fixed. ::: sys/dshowsrcwrapper/dshowdeviceprovider.c @@ +3,3 @@ + * + * ksdeviceprovider.c: Kernel Streaming device probing and monitoring + * Fix the author and the filename please. @@ +56,3 @@ + + GST_DEBUG ("class init"); + This is not needed. ::: sys/dshowsrcwrapper/dshowdeviceprovider.h @@ +3,3 @@ + * + * ksdeviceprovider.h: Kernel Streaming device probing and monitoring + * Same.
Review of attachment 374046 [details] [review]: Yes, stray debug log.
Review of attachment 374048 [details] [review]: I'm not clear on this one. What problems can arise by opening the device in start() as opposed to the NULL->READY state change? Looking through many source elements in -good, some seem to open devices in start(), and some during NULL->READY. Perhaps the documentation on start() needs to be improved, as it currently states "Subclasses should open resources and prepare to produce data". If I should move device opening to the NULL->READY state change, what do I leave in start()?
Review of attachment 374049 [details] [review]: Yes, several applications I've come across set empty strings instead of NULL, and looking across other elements, many handle them equivalently.
Review of attachment 374051 [details] [review]: Ok I'll fix these.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/gstreamer/gst-plugins-bad/issues/807.