[Bug 732283] dshowvideosrc: Port to 1.0

[GStreamer (bugzilla.gnome.org)]

https://bugzilla.gnome.org/show_bug.cgi?id=732283
  GStreamer | gst-plugins-bad | git

--- Comment #9 from Jérôme Laheurte <jerome-bugzilla at jeromelaheurte.net> 2014-09-16 10:06:19 UTC ---
(In reply to comment #8)
> Review of attachment 285731 [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.

> 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

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