[Bug 597822] Add removesilence plugin

[GStreamer (bugzilla.gnome.org)]

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

Sebastian Dröge <slomo> changed:

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

--- Comment #18 from Sebastian Dröge <slomo at circular-chaos.org> 2011-05-31 08:38:24 UTC ---
Review of attachment 188893:
 --> (https://bugzilla.gnome.org/review?bug=597822&attachment=188893)

::: gst/removesilence/Makefile.am
@@ +3,3 @@
+
+libgstremovesilence_la_SOURCES = gstremovesilence.c vad_private.c
+libgstremovesilence_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_CFLAGS)

$(GST_BASE_CFLAGS) is missing here

@@ +4,3 @@
+libgstremovesilence_la_SOURCES = gstremovesilence.c vad_private.c
+libgstremovesilence_la_CFLAGS = $(GST_PLUGINS_BASE_CFLAGS) $(GST_CFLAGS)
+libgstremovesilence_la_LIBADD = $(GST_PLUGINS_BASE_LIBS) $(GST_BASE_LIBS)
$(GST_LIBS)

But you don't need GST_PLUGIN_BASE_{LIBS,CFLAGS} and GST_BASE_{LIBS,CFLAGS} at
all anyway

::: gst/removesilence/gstremovesilence.c
@@ +1,3 @@
+/* GStreamer
+ * Copyright (C) 2009 Tiago Katcipis <tiagokatcipis at gmail.com>
+ * Copyright (C) 2009 Paulo Pizarro  <paulo.pizarro at gmail.com>

We have 2011 now :)

@@ +63,3 @@
+    GST_STATIC_CAPS
+    ("audio/x-raw-int, "
+        "rate=[1, 2147483647], channels=1, "

You can use rate=[1, MAX]

Also it would be nice to have support for multiple channels, should be easy to
add

@@ +73,3 @@
+    ("audio/x-raw-int, "
+        "rate=[1, 2147483647], channels=1, "
+        "endianness= { " G_STRINGIFY (G_BYTE_ORDER)

You can use "endianness = BYTE_ORDER"

@@ +77,3 @@
+
+GST_BOILERPLATE (GstRemoveSilence, gst_remove_silence, GstElement,
+    GST_TYPE_ELEMENT);

Really try to use GstBaseTransform here

@@ +132,3 @@
+      g_param_spec_boolean ("remove", "Remove",
+          "Set to true to remove silence from the stream, false otherwhise",
+          FALSE, G_PARAM_READWRITE));

| G_PARAM_STATIC_STRINGS

@@ +138,3 @@
+          "Hysteresis",
+          "Set the hysteresis (on samples) used on the internal VAD",
+          1, G_MAXUINT64, DEFAULT_VAD_HYSTERESIS, G_PARAM_READWRITE));

| G_PARAM_STATIC_STRINGS

@@ +291,3 @@
+  if (filter->remove && (filter->segment_end != -1) &&
+      (GST_BUFFER_TIMESTAMP (inbuf) != GST_CLOCK_TIME_NONE) &&
+      (GST_BUFFER_TIMESTAMP (inbuf) > filter->segment_end)) {

Now I understand what you're doing here. You want to use
gst_audio_buffer_clip() here, it does what you're doing and clips the buffer in
other situations too

@@ +302,3 @@
+  frame_type =
+      vad_update (filter->vad, (short *) GST_BUFFER_DATA (inbuf),
+      GST_BUFFER_SIZE (inbuf) / sizeof (short));

Better use the GLib types everywhere, gint16 instead of short, etc. In this
file and in the vad_private.[ch]

@@ +356,3 @@
+      filter->segment_end =
+          (stop / sizeof (short)) / (filter->samplerate / GST_SECOND);
+    }

You should have a GstSegment member in your instance structure and use
gst_segment_set_newsegment_full() with the values of the event (or the
bytes->time transformed values). If you use GstBaseTransform as base class this
will already be done for you and you can use the segment member of
GstBaseTransform. And you don't need any segment handling at all anymore and
none of the gst_pad_set_*function() functions

-- 
Configure bugmail: https://bugzilla.gnome.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.