[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 #151499|none                        |needs-work
             status|                            |

--- Comment #14 from Sebastian Dröge <slomo at circular-chaos.org> 2011-05-23 11:07:14 UTC ---
Review of attachment 151499:
 --> (https://bugzilla.gnome.org/review?bug=597822&attachment=151499)

::: configure.ac
@@ +346,3 @@
+if test "x$HAVE_SYS_SOCKET_H" != "xyes"; then
+  AG_GST_DISABLE_PLUGIN(librfb)
+fi

Unrelated change

@@ +1446,3 @@
        AC_SUBST(ACMENC_CFLAGS)
        AC_SUBST(ACMMP3DEC_CFLAGS)
+      ], [HAVE_ACM="no"])

Here too

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

Wrong order of flags, first PLUGINS_BASE, then BASE then GST (for LIBS and
CFLAGS)

::: gst/removesilence/gstremovesilence.c
@@ +84,3 @@
+    GST_PAD_SRC,
+    GST_PAD_ALWAYS,
+    GST_STATIC_CAPS ("audio/x-raw-int, rate=[1, 2147483647], channels=1,
endianness=1234, width=16, depth=16, signed=true")

I think you want BYTE_ORDER instead of endianness=1234 here

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

Why don't you use GstBaseTransform or GstAudioFilter as base class? You can
drop buffers by returning GST_BASE_TRANSFORM_FLOW_DROPPED from the
transform/transform_ip vfunc and it will handle many things for you and make
everything easier.

@@ +143,3 @@
+
+  g_object_class_install_property (gobject_class, PROP_HYSTERESIS,
+      g_param_spec_ulong ("hysteresis", "Hysteresis", "Set the hysteresis (on
samples) used on the internal VAD",

Don't use long/ulong in public API, better use guint64 here

@@ +147,3 @@
+
+
+  gstelement_class->change_state = gst_remove_silence_change_state;

= GST_DEBUG_FUNCPTR (gst_remove_silence_change_state)

@@ +296,3 @@
+  */
+  if(filter->remove){
+      if(GST_BUFFER_CAST(inbuf)->timestamp > filter->segment_end){

Not every buffer has timestamps, it can also be GST_CLOCK_TIME_NONE (i.e. -1).
Also I think this code doesn't make much sense

@@ +309,3 @@
+    if(filter->remove){
+      GST_DEBUG("Removing silence!");
+      gst_buffer_unref(inbuf);

You should set the DISCONT flag on the next buffer that is pushed downstream.
Also you need to send filler newsegment events downstream if there's a too big
duration of silence.

Also, an optional mode to just set the GAP flag on silence buffers instead of
dropping them would be good

@@ +316,3 @@
+  }
+  GST_BUFFER_CAST(inbuf)->timestamp = filter->last_timestamp;
+  filter->last_timestamp += GST_BUFFER_DURATION(inbuf);

Why do you keep track of the timestamps?

@@ +334,3 @@
+    GST_DEBUG("Updating segment: update[%d], rate[%f], format[%d],
start[%lli], stop[%lli], position[%lli]",
+              update, rate, format, start, stop, position);
+

The stop position can be -1 too, which means no stop at all. Also I don't think
this segment stuff is necessary at all. One thing that you could do though
would be to use gst_audio_buffer_clip() in the chain function, look at the
audiosegmentclip element in gst-plugins-bad for an example how to use it.

::: gst/removesilence/gstremovesilence.h
@@ +39,3 @@
+
+typedef struct _GstRemoveSilence      GstRemoveSilence;
+typedef struct _GstRemoveSilenceClass GstRemoveSilenceClass;

Please add the struct definitions to the header to make gtk-doc happy

::: gst/removesilence/vad_private.h
@@ +32,3 @@
+int vad_update(VADFilter *p, short *data, int len);
+
+void vad_set_hysteresis(VADFilter *p, unsigned long hysteresis);

Please try to use GLib types here, gint16 instead of short, etc. Same for
vad_private.c

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