[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

Sebastian Dröge (slomo) <slomo> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #285731|none                        |needs-work
             status|                            |

--- Comment #8 from Sebastian Dröge (slomo) <slomo at coaxion.net> 2014-09-16 08:19:43 UTC ---
Review of attachment 285731:
 --> (https://bugzilla.gnome.org/review?bug=732283&attachment=285731)

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

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