GNOME Bugzilla – Bug 732283
dshowvideosrc: Port to 1.0
Last modified: 2014-09-18 09:37:22 UTC
I'm starting to miss dshowvideosrc now that I've ported my apps to 1.0. Kind of surprised this hasn't been ported yet (supposedly due to the smallish Windows user base), so I wanted to file this bug in case anyone can enlighten me on any special considerations involved in porting this over.
Note that DirectShow is deprecated on Windows since a long long time. That might be another reason why nobody ported it yet. Ideally we would write new elements based on MediaFoundation.
It may be technically deprecated, but I'm not aware of any capture cards that have added support for Windows Media Foundation (WMF), all the ones I've used or checked (Blackmagic, Euresys, Active Silicon, AJA, Matrox) still only provide DirectShow filters. And honestly, from some brief research it seems uptake on WMF is incredibly slow. That being said, I think it's worth porting dshowvideosrc, and I just hope I can find the time to do it this summer.
Did you start this ? If not I'll give it a try. I need to capture video on Windows and I'm not fond of getting back to 0.10.
Go for it. This keeps getting pushed back, I probably wouldn't do it for another couple months at best. I'd be happy to test it out though.
Okay! I actually already took a look but I'm confused by the build. I'll probably ask some questions on the mailing list.
Created attachment 285731 [details] [review] Port of dshowsrcwrapper to 1.0 Hope it's OK; the SubmittingPatches page does not exist on the Wiki...
Note: I removed the property probe stuff. Support for the new GstDeviceMonitor stuff will come in another patch.
Review of attachment 285731 [details] [review]: Thanks, looks generally good :) Just some comments below ::: sys/dshowsrcwrapper/BUILD.txt @@ +18,3 @@ +installed in <SDK>/Samples/multimedia/directshow/baseclasses. Just +open the SLN and build both Debug and Release (NOT the MBCS versions +though). Why can't we build them together with the plugin directly? Also previously we had a visual studio project to build this plugin :) ::: sys/dshowsrcwrapper/gstdshowaudiosrc.cpp @@ +40,3 @@ + " }, " + "rate = " GST_AUDIO_RATE_RANGE ", " + "channels = " GST_AUDIO_CHANNELS_RANGE) Previously this only had channels=[1,2], not the full range @@ +98,3 @@ GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_get_property); + gstbasesrc_class->query = GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_query); Why don't you use get_caps() here? @@ +253,3 @@ + gst_query_parse_caps (query, &filter); + if (filter) { + GstCaps *temp = gst_caps_intersect (caps, filter); gst_caps_intersect_full(filter, caps, GST_CAPS_INTERSECT_FIRST) @@ +567,2 @@ error: + // Don't restart the graph, we're out anyway. Don't use C99 comments, just use /* bla */ @@ +643,3 @@ + GstClock *clock = gst_element_get_clock (GST_ELEMENT (asrc)); + *timestamp = gst_clock_get_time(clock); + gst_object_unref(clock); You can keep *timestamp unset (i.e. GST_CLOCK_TIME_NONE as initialized by the base class). This here is only useful if you can get timestamps from the device/driver somehow @@ +726,3 @@ wavformat->wBitsPerSample, "depth", G_TYPE_INT, wavformat->wBitsPerSample, "endianness", G_TYPE_INT, G_BYTE_ORDER, "signed", G_TYPE_BOOLEAN, TRUE, "channels", G_TYPE_INT, Ideally use GstAudioInfo and create the caps from that with gst_audio_info_from_caps(). Also there are no depth, width, endianness and signed fields in 1.x ::: sys/dshowsrcwrapper/gstdshowvideosrc.cpp @@ +43,3 @@ + "height = " GST_VIDEO_SIZE_RANGE ", " + "framerate = " GST_VIDEO_FPS_RANGE ", " + "systemstream = (boolean) FALSE; " The systemstream field was only for video/x-dv, not video/x-raw @@ +121,3 @@ gstbasesrc_class->unlock_stop = GST_DEBUG_FUNCPTR (gst_dshowvideosrc_unlock_stop); + gstbasesrc_class->query = GST_DEBUG_FUNCPTR (gst_dshowvideosrc_query); Why don't you use get_caps()? @@ +177,3 @@ CoInitializeEx (NULL, COINIT_MULTITHREADED); + gst_pad_set_event_function (GST_BASE_SRC_PAD (src), GST_DEBUG_FUNCPTR (gst_dshowvideosrc_event)); And here you could use the event vfunc @@ -288,3 @@ - if (src->buffer_cond) { - g_cond_free (src->buffer_cond); - src->buffer_cond = NULL; You should clear the cond and mutex here @@ +304,3 @@ + + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_CAPS: You can use the set_caps vfunc here @@ +471,3 @@ + if (src->media_filter) { + // Setting this to TRUE because set_caps may be invoked before + // Run() returns. No C99 comments @@ +1035,3 @@ GST_BUFFER_DURATION (buf) = duration; + GstMapInfo info; Put variable declarations at the beginning of blocks
(In reply to comment #8) > Review of attachment 285731 [details] [review]: > > Thanks, looks generally good :) Just some comments below > > ::: sys/dshowsrcwrapper/BUILD.txt > @@ +18,3 @@ > +installed in <SDK>/Samples/multimedia/directshow/baseclasses. Just > +open the SLN and build both Debug and Release (NOT the MBCS versions > +though). > > Why can't we build them together with the plugin directly? Well, it's a third party static library. I can build it as part of the plugin but it kind of defeats the purpose of a library. > Also previously we > had a visual studio project to build this plugin :) I noticed that. Actually, I noticed three different Visual solutions with some missing project files, none of them being intended for the version of Visual I use. The big advantage of CMake IMO is that you can generate a solution for whatever version of Visual you intend to use, and only have to maintain a text file. That, and avoid messing with the IDE, which is a PITA. > ::: sys/dshowsrcwrapper/gstdshowaudiosrc.cpp > @@ +40,3 @@ > + " }, " > + "rate = " GST_AUDIO_RATE_RANGE ", " > + "channels = " GST_AUDIO_CHANNELS_RANGE) > > Previously this only had channels=[1,2], not the full range Right. But I just noticed DirectShow knows about audio with more than 2 channels, so I'll implement that instead. > @@ +98,3 @@ > GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_get_property); > > + gstbasesrc_class->query = GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_query); > > Why don't you use get_caps() here? I was under the impression that caps negotiation used queries and events now, and that get/set caps methods were deprecated. If not so, I'm fixing this. > > @@ +253,3 @@ > + gst_query_parse_caps (query, &filter); > + if (filter) { > + GstCaps *temp = gst_caps_intersect (caps, filter); > > gst_caps_intersect_full(filter, caps, GST_CAPS_INTERSECT_FIRST) Thanks, fixing > > @@ +567,2 @@ > error: > + // Don't restart the graph, we're out anyway. > > Don't use C99 comments, just use /* bla */ Too much C++ lately :) > > @@ +643,3 @@ > + GstClock *clock = gst_element_get_clock (GST_ELEMENT (asrc)); > + *timestamp = gst_clock_get_time(clock); > + gst_object_unref(clock); > > You can keep *timestamp unset (i.e. GST_CLOCK_TIME_NONE as initialized by the > base class). > > This here is only useful if you can get timestamps from the device/driver > somehow OK I was wondering since I had timestamping errors, but they probably came from something else > > @@ +726,3 @@ > wavformat->wBitsPerSample, "depth", G_TYPE_INT, > wavformat->wBitsPerSample, "endianness", G_TYPE_INT, G_BYTE_ORDER, > "signed", G_TYPE_BOOLEAN, TRUE, "channels", G_TYPE_INT, > > Ideally use GstAudioInfo and create the caps from that with > gst_audio_info_from_caps(). Also there are no depth, width, endianness and > signed fields in 1.x OK > > ::: sys/dshowsrcwrapper/gstdshowvideosrc.cpp > @@ +43,3 @@ > + "height = " GST_VIDEO_SIZE_RANGE ", " > + "framerate = " GST_VIDEO_FPS_RANGE ", " > + "systemstream = (boolean) FALSE; " > > The systemstream field was only for video/x-dv, not video/x-raw OK > > @@ +121,3 @@ > gstbasesrc_class->unlock_stop = > GST_DEBUG_FUNCPTR (gst_dshowvideosrc_unlock_stop); > + gstbasesrc_class->query = GST_DEBUG_FUNCPTR (gst_dshowvideosrc_query); > > Why don't you use get_caps()? Same as above :) > > @@ +177,3 @@ > CoInitializeEx (NULL, COINIT_MULTITHREADED); > > + gst_pad_set_event_function (GST_BASE_SRC_PAD (src), GST_DEBUG_FUNCPTR > (gst_dshowvideosrc_event)); > > And here you could use the event vfunc If I use the set_caps vfunc there will be no need for an event method any more isn't it ? > > @@ -288,3 @@ > - if (src->buffer_cond) { > - g_cond_free (src->buffer_cond); > - src->buffer_cond = NULL; > > You should clear the cond and mutex here Oops > > @@ +304,3 @@ > + > + switch (GST_EVENT_TYPE (event)) { > + case GST_EVENT_CAPS: > > You can use the set_caps vfunc here OK > > @@ +471,3 @@ > + if (src->media_filter) { > + // Setting this to TRUE because set_caps may be invoked before > + // Run() returns. > > No C99 comments OK > > @@ +1035,3 @@ > GST_BUFFER_DURATION (buf) = duration; > > + GstMapInfo info; > > Put variable declarations at the beginning of blocks OK
(In reply to comment #9) > (In reply to comment #8) > > Review of attachment 285731 [details] [review] [details]: > > > > Thanks, looks generally good :) Just some comments below > > > > ::: sys/dshowsrcwrapper/BUILD.txt > > @@ +18,3 @@ > > +installed in <SDK>/Samples/multimedia/directshow/baseclasses. Just > > +open the SLN and build both Debug and Release (NOT the MBCS versions > > +though). > > > > Why can't we build them together with the plugin directly? > > Well, it's a third party static library. I can build it as part of the plugin > but > it kind of defeats the purpose of a library. Sure, but it requires to build stuff manually via msbuild.exe or visual studio :) > > Also previously we > > had a visual studio project to build this plugin :) > > I noticed that. Actually, I noticed three different Visual solutions with some > missing project > files, none of them being intended for the version of Visual I use. The big > advantage of CMake > IMO is that you can generate a solution for whatever version of Visual you > intend to use, and > only have to maintain a text file. That, and avoid messing with the IDE, which > is a PITA. Agreed > > ::: sys/dshowsrcwrapper/gstdshowaudiosrc.cpp > > @@ +40,3 @@ > > + " }, " > > + "rate = " GST_AUDIO_RATE_RANGE ", " > > + "channels = " GST_AUDIO_CHANNELS_RANGE) > > > > Previously this only had channels=[1,2], not the full range > > Right. But I just noticed DirectShow knows about audio with more than 2 > channels, so I'll implement that instead. For more than 2 channels you'll have to worry about setting the correct channel-mask though. I would do the >2 channel support in a separate patch later. > > @@ +98,3 @@ > > GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_get_property); > > > > + gstbasesrc_class->query = GST_DEBUG_FUNCPTR (gst_dshowaudiosrc_query); > > > > Why don't you use get_caps() here? > > I was under the impression that caps negotiation used queries and events now, > and that get/set caps methods were deprecated. If not so, I'm fixing this. They're not deprecated :) The only difference here to 0.10 is that all these are internally implemented as queries/events > > @@ +177,3 @@ > > CoInitializeEx (NULL, COINIT_MULTITHREADED); > > > > + gst_pad_set_event_function (GST_BASE_SRC_PAD (src), GST_DEBUG_FUNCPTR > > (gst_dshowvideosrc_event)); > > > > And here you could use the event vfunc > > If I use the set_caps vfunc there will be no need for an event method any more > isn't it ? Yes
Created attachment 286292 [details] [review] Port of dshowsrcwrapper to 1.0 (v2) Here you are. Support for more than 2 channels is added to my todo list :) I tried including the base classes project as a subproject, but it brings more problems than it solves; the file must be upgraded by VS the first time you open, but default permissions of the SDK don't allow it... Then the configurations don't match. I think it's simpler to build the sample once and for all after installing the SDK.
Review of attachment 286292 [details] [review]: Looks almost good, good work :) ::: sys/dshowsrcwrapper/gstdshowaudiosrc.cpp @@ +247,3 @@ + + switch (GST_QUERY_TYPE (query)) { + case GST_QUERY_CAPS: Use GstBaseSrc::get_caps() for this too @@ +738,3 @@ + break; + case 16: + format = GST_AUDIO_FORMAT_S16; gst_audio_format_build_integer() might be useful here
Created attachment 286356 [details] [review] Port of dshowsrcwrapper to 1.0 (v3) Done. Forgot to remove the unused query function :(
commit 4f60ecdd98194cd8a80f4d0637fb4d984bbbeee3 Author: Jerome Laheurte <jlaheurte@quividi.com> Date: Wed Sep 17 12:24:39 2014 +0200 dshowsrcwrapper: Port to 1.0 https://bugzilla.gnome.org/show_bug.cgi?id=732283