GNOME Bugzilla – Bug 597822
Add removesilence plugin
Last modified: 2011-10-18 16:58:47 UTC
See: https://bugzilla.gnome.org/show_bug.cgi?id=597715 I hope this time i have done everything right :-). Here goes the git format-patch and the source code. I ran autogen and make and the plugin i added compiled successfully.
im not being able to upload he patch, it keeps telling me that the file does not exist or that i dont have permission to read it....but it does exists and i can read it. So im pasting here the patch content: From 4d87bc4d7a15212acf7c568af25379b4cc15193b Mon Sep 17 00:00:00 2001 From: Tiago Katcipis <katcipis@katcipis-laptop.(none)> Date: Thu, 8 Oct 2009 12:04:18 -0300 Subject: [PATCH] adding removesilence plugin --- configure.ac | 2 + gst/removesilence/Makefile.am | 12 ++ gst/removesilence/gstremovesilence.c | 345 ++++++++++++++++++++++++++++++++++ gst/removesilence/gstremovesilence.h | 70 +++++++ gst/removesilence/vad_private.c | 134 +++++++++++++ gst/removesilence/vad_private.h | 66 +++++++ 6 files changed, 629 insertions(+), 0 deletions(-) create mode 100644 gst/removesilence/Makefile.am create mode 100644 gst/removesilence/gstremovesilence.c create mode 100644 gst/removesilence/gstremovesilence.h create mode 100644 gst/removesilence/vad_private.c create mode 100644 gst/removesilence/vad_private.h diff --git a/configure.ac b/configure.ac index 7c32666..55b05d9 100644 --- a/configure.ac +++ b/configure.ac @@ -285,6 +285,7 @@ AG_GST_CHECK_PLUGIN(pnm) AG_GST_CHECK_PLUGIN(qtmux) AG_GST_CHECK_PLUGIN(rawparse) AG_GST_CHECK_PLUGIN(real) +AG_GST_CHECK_PLUGIN(removesilence) AG_GST_CHECK_PLUGIN(rtpmux) AG_GST_CHECK_PLUGIN(scaletempo) AG_GST_CHECK_PLUGIN(sdp) @@ -1767,6 +1768,7 @@ gst/pnm/Makefile gst/qtmux/Makefile gst/rawparse/Makefile gst/real/Makefile +gst/removesilence/Makefile gst/rtpmux/Makefile gst/scaletempo/Makefile gst/sdp/Makefile diff --git a/gst/removesilence/Makefile.am b/gst/removesilence/Makefile.am new file mode 100644 index 0000000..700df47 --- /dev/null +++ b/gst/removesilence/Makefile.am @@ -0,0 +1,12 @@ + +plugin_LTLIBRARIES = libgstremovesilence.la + +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) +libgstremovesilence_la_LDFLAGS = $(GST_PLUGIN_LDFLAGS) +libgstremovesilence_la_LIBTOOLFLAGS = --tag=disable-static + +noinst_HEADERS = \ + gstremovesilence.h \ + vad_private.h diff --git a/gst/removesilence/gstremovesilence.c b/gst/removesilence/gstremovesilence.c new file mode 100644 index 0000000..625bf8f --- /dev/null +++ b/gst/removesilence/gstremovesilence.c @@ -0,0 +1,345 @@ +/* + * GStreamer + * @author Tiago Katcipis <tiagokatcipis@gmail.com> + * @author Paulo Pizarro <paulo.pizarro@gmail.com> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Alternatively, the contents of this file may be used under the + * GNU Lesser General Public License Version 2.1 (the "LGPL"), in + * which case the following provisions apply instead of the ones + * mentioned above: + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 02111-1307, USA. + */ + +/** + * SECTION:element-removesilence + * + * Removes all silence periods from an audio stream, fixing timestamp discontinuities. + * + * <refsect2> + * <title>Example launch line</title> + * |[ + * gst-launch -v -m filesrc location="audiofile" ! decodebin ! removesilence ! autoaudiosink + * ]| + * </refsect2> + */ +#ifdef HAVE_CONFIG_H +# include "config.h" +#endif + +#include <gst/gst.h> +#include "gstremovesilence.h" +#include "vad_private.h" + +GST_DEBUG_CATEGORY_STATIC (gst_remove_silence_debug); +#define GST_CAT_DEFAULT gst_remove_silence_debug + +struct _GstRemoveSilence +{ + GstElement element; + GstPad *sinkpad, *srcpad; + VADFilter* vad; + gboolean remove; + gint64 segment_end; + gint samplerate; + GstClockTime last_timestamp; +}; + +struct _GstRemoveSilenceClass +{ + GstElementClass parent_class; +}; + +/* Filter signals and args */ +enum +{ + /* FILL ME */ + LAST_SIGNAL +}; + +enum +{ + PROP_0, + PROP_REMOVE +}; + + +static GstStaticPadTemplate sink_factory = GST_STATIC_PAD_TEMPLATE ("sink", + GST_PAD_SINK, + GST_PAD_ALWAYS, + GST_STATIC_CAPS ("audio/x-raw-int, rate=[1, 2147483647], channels=1, endianness=1234, width=16, depth=16, signed=true") + ); + +static GstStaticPadTemplate src_factory = GST_STATIC_PAD_TEMPLATE ("src", + 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") + ); + +GST_BOILERPLATE (GstRemoveSilence, gst_remove_silence, GstElement, GST_TYPE_ELEMENT); + +static void gst_remove_silence_set_property (GObject * object, guint prop_id, + const GValue * value, GParamSpec * pspec); +static void gst_remove_silence_get_property (GObject * object, guint prop_id, + GValue * value, GParamSpec * pspec); + +static gboolean gst_remove_silence_set_caps (GstPad * pad, GstCaps * caps); +static GstFlowReturn gst_remove_silence_chain (GstPad * pad, GstBuffer * buf); +static gboolean gst_remove_silence_event (GstPad * pad, GstEvent * event); + +/* GObject vmethod implementations */ + +static void +gst_remove_silence_base_init (gpointer gclass) +{ + GstElementClass *element_class = GST_ELEMENT_CLASS (gclass); + + gst_element_class_set_details_simple(element_class, + "RemoveSilence", + "Filter/Effect/Audio", + "Removes all the silence periods from the stream.", + "Tiago Katcipis <tiagokatcipis@gmail.com>\n \ + Paulo Pizarro <paulo.pizarro@gmail.com>"); + + gst_element_class_add_pad_template (element_class, + gst_static_pad_template_get (&src_factory)); + gst_element_class_add_pad_template (element_class, + gst_static_pad_template_get (&sink_factory)); +} + +/* initialize the removesilence's class */ +static void +gst_remove_silence_class_init (GstRemoveSilenceClass * klass) +{ + GObjectClass *gobject_class; + GstElementClass *gstelement_class; + + gobject_class = (GObjectClass *) klass; + gstelement_class = (GstElementClass *) klass; + + gobject_class->set_property = gst_remove_silence_set_property; + gobject_class->get_property = gst_remove_silence_get_property; + + g_object_class_install_property (gobject_class, PROP_REMOVE, + g_param_spec_boolean ("remove", "Remove", "Set to true to remove silence from the stream, false otherwhise", + FALSE, G_PARAM_READWRITE)); +} + +/* initialize the new element + * instantiate pads and add them to element + * set pad calback functions + * initialize instance structure + */ +static void +gst_remove_silence_init (GstRemoveSilence * filter, + GstRemoveSilenceClass * gclass) +{ + filter->sinkpad = gst_pad_new_from_static_template (&sink_factory, "sink"); + gst_pad_set_setcaps_function (filter->sinkpad, + GST_DEBUG_FUNCPTR(gst_remove_silence_set_caps)); + gst_pad_set_getcaps_function (filter->sinkpad, + GST_DEBUG_FUNCPTR(gst_pad_proxy_getcaps)); + gst_pad_set_chain_function (filter->sinkpad, + GST_DEBUG_FUNCPTR(gst_remove_silence_chain)); + + filter->srcpad = gst_pad_new_from_static_template (&src_factory, "src"); + gst_pad_set_getcaps_function (filter->srcpad, + GST_DEBUG_FUNCPTR(gst_pad_proxy_getcaps)); + + gst_pad_set_event_function (filter->sinkpad, gst_remove_silence_event); + + gst_element_add_pad (GST_ELEMENT (filter), filter->sinkpad); + gst_element_add_pad (GST_ELEMENT (filter), filter->srcpad); + + filter->remove = FALSE; + filter->last_timestamp = 0; + filter->vad = vad_new(); + if(!filter->vad){ + GST_DEBUG("Error initializing VAD !!"); + return; + } +} + +static void +gst_remove_silence_set_property (GObject * object, guint prop_id, + const GValue * value, GParamSpec * pspec) +{ + GstRemoveSilence *filter = GST_REMOVESILENCE (object); + + switch (prop_id) { + case PROP_REMOVE: + filter->remove = g_value_get_boolean (value); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } +} + +static void +gst_remove_silence_get_property (GObject * object, guint prop_id, + GValue * value, GParamSpec * pspec) +{ + GstRemoveSilence *filter = GST_REMOVESILENCE (object); + + switch (prop_id) { + case PROP_REMOVE: + g_value_set_boolean (value, filter->remove); + break; + default: + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, prop_id, pspec); + break; + } +} + +/* this function handles the link with other elements */ +static gboolean +gst_remove_silence_set_caps (GstPad * pad, GstCaps * caps) +{ + GstRemoveSilence *filter; + GstPad *otherpad; + GstStructure *structure = gst_caps_get_structure(caps, 0); + + filter = GST_REMOVESILENCE (gst_pad_get_parent (pad)); + otherpad = (pad == filter->srcpad) ? filter->sinkpad : filter->srcpad; + + if(!gst_pad_set_caps (otherpad, caps)) + return FALSE; + + gst_structure_get_int (structure, "rate", &filter->samplerate); + return TRUE; +} + +static GstFlowReturn +gst_remove_silence_chain (GstPad * pad, GstBuffer * inbuf) +{ + GstRemoveSilence *filter = NULL; + int frame_type; + filter = GST_REMOVESILENCE (GST_OBJECT_PARENT (pad)); + + /* + Validate timestamp, some elements does not control the segment being played + using timestamp, if you are looping on a segment with silence a skew can be generated + because the stream will emit segment end based on the audio played not the timestamp. + */ + if(filter->remove){ + if(GST_BUFFER_CAST(inbuf)->timestamp > filter->segment_end){ + //Reached the end of the segment + GST_DEBUG("EOS reached!"); + GST_DEBUG("buffer timestamp[%lli], segment_end[%lli]", GST_BUFFER_CAST(inbuf)->timestamp, filter->segment_end); + gst_buffer_unref(inbuf); + return GST_FLOW_UNEXPECTED; + } + } + frame_type = vad_update(filter->vad, (short *) GST_BUFFER_DATA(inbuf), GST_BUFFER_SIZE(inbuf) / sizeof(short)); + if (frame_type == VAD_SILENCE) { + GST_DEBUG("Silence detected!"); + if(filter->remove){ + GST_DEBUG("Removing silence!"); + gst_buffer_unref(inbuf); + return GST_FLOW_OK; + } + } + + GST_DEBUG("Voice!"); + GST_BUFFER_CAST(inbuf)->timestamp = filter->last_timestamp; + filter->last_timestamp += GST_BUFFER_DURATION(inbuf); + + return gst_pad_push (filter->srcpad, inbuf); +} + + +static gboolean gst_remove_handle_new_segment_event(GstRemoveSilence* filter, GstEvent* event) +{ + gdouble rate = 0.0; + gboolean update = FALSE; + gint64 start,stop,position; + GstFormat format; + + gst_event_parse_new_segment(event, &update, &rate, &format, + &start, &stop, &position); + + GST_DEBUG("Updating segment: update[%d], rate[%f], format[%d], start[%lli], stop[%lli], position[%lli]", + update, rate, format, start, stop, position); + + if(format == GST_FORMAT_TIME){ + GST_DEBUG("Getting new segment in time"); + filter->segment_end = stop; + filter->last_timestamp = start; + return TRUE; + } + + if(format == GST_FORMAT_BYTES){ + GST_DEBUG("Getting new segment in bytes"); + filter->segment_end = (stop / sizeof(short)) / (filter->samplerate / GST_SECOND); + filter->last_timestamp = (start / sizeof(short)) / (filter->samplerate / GST_SECOND); + return TRUE; + } + + GST_DEBUG("Unable to get the segment being played"); + return FALSE; +} + +static gboolean gst_remove_silence_event (GstPad * pad, GstEvent * event) +{ + GstRemoveSilence* filter = GST_REMOVESILENCE (GST_OBJECT_PARENT (pad)); + gboolean ret = FALSE; + + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_NEWSEGMENT: + if(!gst_remove_handle_new_segment_event(filter, event)){ + return FALSE; + } + ret = gst_pad_push_event (filter->srcpad, event); + break; + default: + ret = gst_pad_event_default(pad, event); + break; + } + return ret; +} + +/*Plugin init functions*/ +static gboolean +plugin_init (GstPlugin * plugin) +{ + return gst_element_register (plugin, "removesilence", GST_RANK_NONE, gst_remove_silence_get_type()); +} + +GST_PLUGIN_DEFINE (GST_VERSION_MAJOR, + GST_VERSION_MINOR, + "removesilence", + "Removes silence from an audio stream", + plugin_init, VERSION, "LGPL", GST_PACKAGE_NAME, GST_PACKAGE_ORIGIN); + diff --git a/gst/removesilence/gstremovesilence.h b/gst/removesilence/gstremovesilence.h new file mode 100644 index 0000000..276b963 --- /dev/null +++ b/gst/removesilence/gstremovesilence.h @@ -0,0 +1,70 @@ +/* + * GStreamer + * @author Tiago Katcipis <tiagokatcipis@gmail.com> + * @author Paulo Pizarro <paulo.pizarro@gmail.com> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Alternatively, the contents of this file may be used under the + * GNU Lesser General Public License Version 2.1 (the "LGPL"), in + * which case the following provisions apply instead of the ones + * mentioned above: + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 02111-1307, USA. + */ + +#ifndef __GST_REMOVESILENCE_H__ +#define __GST_REMOVESILENCE_H__ + +#include <gst/gst.h> + +G_BEGIN_DECLS + +#define GST_TYPE_REMOVESILENCE \ + (gst_remove_silence_get_type()) +#define GST_REMOVESILENCE(obj) \ + (G_TYPE_CHECK_INSTANCE_CAST((obj),GST_TYPE_REMOVESILENCE,GstRemoveSilence)) +#define GST_REMOVESILENCE_CLASS(klass) \ + (G_TYPE_CHECK_CLASS_CAST((klass),GST_TYPE_REMOVESILENCE,GstRemoveSilenceClass)) +#define GST_IS_REMOVESILENCE(obj) \ + (G_TYPE_CHECK_INSTANCE_TYPE((obj),GST_TYPE_REMOVESILENCE)) +#define GST_IS_REMOVESILENCE_CLASS(klass) \ + (G_TYPE_CHECK_CLASS_TYPE((klass),GST_TYPE_REMOVESILENCE)) + +typedef struct _GstRemoveSilence GstRemoveSilence; +typedef struct _GstRemoveSilenceClass GstRemoveSilenceClass; + +GType gst_remove_silence_get_type (void); + +G_END_DECLS + +#endif /* __GST_REMOVESILENCE_H__ */ diff --git a/gst/removesilence/vad_private.c b/gst/removesilence/vad_private.c new file mode 100644 index 0000000..54cbf21 --- /dev/null +++ b/gst/removesilence/vad_private.c @@ -0,0 +1,134 @@ +/* + * GStreamer + * @author Tiago Katcipis <tiagokatcipis@gmail.com> + * @author Paulo Pizarro <paulo.pizarro@gmail.com> + * @author Rogério Santos <rogerio.santos@digitro.com.br> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Alternatively, the contents of this file may be used under the + * GNU Lesser General Public License Version 2.1 (the "LGPL"), in + * which case the following provisions apply instead of the ones + * mentioned above: + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 02111-1307, USA. + */ + +#include "vad_private.h" +#include <string.h> + +union pgen { + unsigned long a; + void *v; + unsigned long *l; + unsigned char *b; + unsigned short *w; + short *s; +}; + +struct _cqueue_s { + union pgen base; + union pgen tail; + union pgen head; + int size; +}; + +typedef struct _cqueue_s cqueue_t; + +struct _vad_s { + short vad_buffer[VAD_BUFFER_SIZE]; + cqueue_t cqueue; + int vad_state; + unsigned long vad_samples; + unsigned long vad_power; + long vad_zcr; +}; + +VADFilter* vad_new() +{ + VADFilter* vad = malloc(sizeof(VADFilter)); + memset(vad, 0, sizeof(vad)); + vad->cqueue.base.s = vad->vad_buffer; + vad->cqueue.tail.a = vad->cqueue.head.a = 0; + vad->cqueue.size = VAD_BUFFER_SIZE; + return vad; +} + +int vad_update(struct _vad_s *p, short *data, int len) +{ + unsigned long tail; + int frame_type; + short sample; + int i; + + for (i = 0; i < len; i++) { + p->vad_power = VAD_POWER_ALPHA * ((data[i] * data[i] >> 14) & 0xFFFF) + \ + (0xFFFF - VAD_POWER_ALPHA) * (p->vad_power >> 16) + \ + ((0xFFFF - VAD_POWER_ALPHA) * (p->vad_power & 0xFFFF) >> 16); + /* Update VAD buffer */ + p->cqueue.base.s[p->cqueue.head.a] = data[i]; + p->cqueue.head.a = ++p->cqueue.head.a & (p->cqueue.size - 1); + if (p->cqueue.head.a == p->cqueue.tail.a) + p->cqueue.tail.a = ++p->cqueue.tail.a & (p->cqueue.size - 1); + } + + tail = p->cqueue.tail.a; + p->vad_zcr = 0; + for (;;) { + sample = p->cqueue.base.s[tail]; + tail = (tail + 1) & (p->cqueue.size - 1); + if (tail == p->cqueue.head.a) break; + p->vad_zcr += ((sample & 0x8000) != (p->cqueue.base.s[tail] & 0x8000)) ? 1 : -1; + } + + frame_type = (p->vad_power > VAD_POWER_THRESHOLD && p->vad_zcr < VAD_ZCR_THRESHOLD) ? VAD_VOICE : VAD_SILENCE; + + if (p->vad_state != frame_type) { + /* Voice to silence transition */ + if (p->vad_state == VAD_VOICE) { + p->vad_samples += len; + if (p->vad_samples >= VAD_HYSTERESIS) { + p->vad_state = frame_type; + p->vad_samples = 0; + } + } + else { + p->vad_state = frame_type; + p->vad_samples = 0; + } + } + else { + p->vad_samples = 0; + } + + return p->vad_state; +} diff --git a/gst/removesilence/vad_private.h b/gst/removesilence/vad_private.h new file mode 100644 index 0000000..e2d0f70 --- /dev/null +++ b/gst/removesilence/vad_private.h @@ -0,0 +1,66 @@ +/* + * GStreamer + * @author Tiago Katcipis <tiagokatcipis@gmail.com> + * @author Paulo Pizarro <paulo.pizarro@gmail.com> + * @author Rogério Santos <rogerio.santos@digitro.com.br> + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Alternatively, the contents of this file may be used under the + * GNU Lesser General Public License Version 2.1 (the "LGPL"), in + * which case the following provisions apply instead of the ones + * mentioned above: + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Library General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Library General Public License for more details. + * + * You should have received a copy of the GNU Library General Public + * License along with this library; if not, write to the + * Free Software Foundation, Inc., 59 Temple Place - Suite 330, + * Boston, MA 02111-1307, USA. + */ + +#ifndef __VAD_FILTER_H__ +#define __VAD_FILTER_H__ + +#define VAD_SILENCE 0 +#define VAD_VOICE 1 + +#define VAD_POWER_ALPHA 0x0800 /* Q16 */ +#define VAD_POWER_THRESHOLD 0x000010C7 /* -60 dB (square wave) */ +#define VAD_ZCR_THRESHOLD 0 +#define VAD_BUFFER_SIZE 256 +#define VAD_HYSTERESIS 480 /* 60 mseg */ + +#define DEFAULT_ADAPTATIVE_TIMER 100 /* mseg */ + +typedef struct _vad_s VADFilter; + +int vad_update(VADFilter *p, short *data, int len); + +VADFilter* vad_new(); + +#endif /* __VAD_FILTER__ */ -- 1.6.0.4
Created attachment 145078 [details] [review] removesilence patch Now at home the patches are going :-)
Have my patch been accepted? if don't...what i have to do so it can be accepted?
I'm not wanting to make pressure, i just want to know if there is something i can do to make the patch acceptable, is there something wrong with the patch? the plugin is not useful? when i wanted to extract only voice from a phone call it has been quite useful. best regards, Katcipis
Review of attachment 145078 [details] [review]: ::: gst/removesilence/gstremovesilence.c @@ +1,3 @@ +/* + * GStreamer + * @author Tiago Katcipis <tiagokatcipis@gmail.com> this looks a bit differnt in gstreamer files usualy @@ +107,3 @@ + GST_STATIC_CAPS ("audio/x-raw-int, rate=[1, 2147483647], channels=1, endianness=1234, width=16, depth=16, signed=true") + ); + not critical fro getting it in, but supporting more formats natively would be nice (e.g. width/depth). Also do you have any thoughts on how this would work with e.g. stereo or more channels? @@ +185,3 @@ + filter->remove = FALSE; + filter->last_timestamp = 0; + filter->vad = vad_new(); Don't you need to reset this during state changes (e.g. to make it work if the element is used a 2nd time)? @@ +196,3 @@ + const GValue * value, GParamSpec * pspec) +{ + GstRemoveSilence *filter = GST_REMOVESILENCE (object); The macro should be called GST_REMOVE_SILENCE. ::: gst/removesilence/vad_private.h @@ +56,3 @@ +#define VAD_HYSTERESIS 480 /* 60 mseg */ + +#define DEFAULT_ADAPTATIVE_TIMER 100 /* mseg */ wouldn't it make sense to have some of those as gobject properties?
>> this looks a bit different in gstreamer files usually Well i can fix that to make more "Gstreamer like" :-). >> not critical fro getting it in, but supporting more formats natively would be >> nice (e.g. width/depth). Also do you have any thoughts on how this would work >> with e.g. stereo or more channels? The VAD code is pretty simple and only works with this width and depth, the only thing that is flexible is the samplerate. The VAD only works with one channel. Since the main application is to remove silence from phone calls i did not care with stereo or more channels, neither with the depth and width, but if someone thinks on a good way to implement it, it would be good :-). >> Don't you need to reset this during state changes (e.g. to make it work if >>the >> element is used a 2nd time)? Well i am already using it on a product and it is working as it is... but I'm really newbie at Gstreamer, if you think it must be reset on state change i can implement it with no problem, but i cant say for sure if it is really needed. >> The macro should be called GST_REMOVE_SILENCE. This one i can fix :-). >> wouldn't it make sense to have some of those as gobject properties? Since the VAD code was not developed by me i didn't changed this values, but i suppose some of them can be configured, like the hysteresis. well I'm going to perform some of this changes and send a new patch here. Thanks for the help Stefan.
Im doing the changes that you requested, at least the ones i can do :-). And i got a little confused with this one: >> Don't you need to reset this during state changes (e.g. to make it work if the element is used a 2nd time)? All state changes? or some state changes? i think that reseting it when the events GST_EVENT_NEWSEGMENT, GST_EVENT_FLUSH_START and GST_EVENT_EOS makes sense....but all events it would not make sense...i think. I realized that i have to define a finalize to the element too, so i can free the vad struct...it would leak memory the way it is. Thanks for the help Stefan.
Created attachment 151499 [details] [review] New patch to insert the remove silence plugin The following changes have been made: - Fixed the headers to be more GStreamer like. - Added missing change_state function. - Now reset during state change READY->NULL. - Fixed the macro name. - Added the property hysteresis. - Fixed missing GST_DEBUG_INIT. - Fixed memory leak because the VAD struct was never being freed (added finalize method).
Well...is there anything more i can do to make the plugin acceptable?
"now that we got core, -base and -good releases out of the way, here's the plan for the next few weeks: - gst-plugins-ugly and -bad git will freeze for releases very soon; if there is anything you want to get in or bugs that you think should be looked at before the release, comment on them now, or mark them as blocker if they are blockers." Tim-Philipp Müller sent this email to the list, so im commenting on my patch before the release to see if someone will add it.
We are using this element on one of our final products and it is working fine and proved to be useful when hearing a long phone conversation searching for something is particular... there is no interest at all on adding it to gstreamer? =/
Seems like a reasonable addition. I have no idea why this got dropped.
Thanks David, if there is anything that i can do to make the plugin better before adding it let me know.
Review of attachment 151499 [details] [review]: ::: 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
> Wrong order of flags, first PLUGINS_BASE, then BASE then GST (for LIBS and CFLAGS) I'm going to fix that > I think you want BYTE_ORDER instead of endianness=1234 here Ill fix that too. > 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. This was one of my first plugins, i didn't knew GstBaseTransform yet. I will rewrite it as a gstAudioFilter. > Don't use long/ulong in public API, better use guint64 here I will change it to guint64. > 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 On the time i wrote it i didn't have idea that was possible. I'm going to fix that. This was made to avoid a problem that i had, the objective of the plugin is to remove the silence periods, but produce a continuous stream (no gaps on the timestamps), if a buffer is silence, it is dropped, but the drop will not appear on downstream elements because the plugin corrects the timestamps on the buffers, so it seems that no buffer was dropped and the audio is perfect, only without the silences. Using GstAudioFilter and returning GST_BASE_TRANSFORM_FLOW_DROPPED will give this same behaviour ? > 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. This would give me the behaviour that i described before? the idea was to exactly fool the downstream elements, so they think that no buffer was dropped. > Why do you keep track of the timestamps? To create the behaviour i described before, but based on the corrections you proposed it seems that it is a better way to do this (i never liked this timestamp stuff, but i didn't think on other idea that behaved like i wanted) > Please add the struct definitions to the header to make gtk-doc happy i will. > Please try to use GLib types here, gint16 instead of short, etc. Same for vad_private.c Thanks for all your suggestions, i will apply them ASAP.
(In reply to comment #15) > > 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 > > On the time i wrote it i didn't have idea that was possible. I'm going to fix > that. This was made to avoid a problem that i had, the objective of the plugin > is to remove the silence periods, but produce a continuous stream (no gaps on > the timestamps), if a buffer is silence, it is dropped, but the drop will not > appear on downstream elements because the plugin corrects the timestamps on the > buffers, so it seems that no buffer was dropped and the audio is perfect, only > without the silences. Using GstAudioFilter and returning > GST_BASE_TRANSFORM_FLOW_DROPPED will give this same behaviour ? No, this will only drop the buffer. You still have to adjust the timestamps and/or durations yourself > > 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. > > This would give me the behaviour that i described before? the idea was to > exactly fool the downstream elements, so they think that no buffer was dropped. Ok, then don't set that flag :) > > Why do you keep track of the timestamps? > > To create the behaviour i described before, but based on the corrections you > proposed it seems that it is a better way to do this (i never liked this > timestamp stuff, but i didn't think on other idea that behaved like i wanted) I think this is how a removesilence element should work, what I've described would be more a detectsilence element.
Created attachment 188893 [details] [review] Revised patch This patch applies all changes suggested by Sebastian, except the ones that would change the plugin behaviour.
Review of attachment 188893 [details] [review]: ::: 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@gmail.com> + * Copyright (C) 2009 Paulo Pizarro <paulo.pizarro@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
> But you don't need GST_PLUGIN_BASE_{LIBS,CFLAGS} and GST_BASE_{LIBS,CFLAGS} at all anyway ill remove them. > We have 2011 now :) The initial proposal was made on 2009, since them little change has been made on the code, until now of course :-). > You can use rate=[1, MAX] It definitely better. > Also it would be nice to have support for multiple channels, should be easy to add I think for now mono is fine (i never had the need for multiple channels), if people starts to use it and there is a need for multichannel ill do it. > You can use "endianness = BYTE_ORDER" I just copied the code from another plugin, ill fix it. > Really try to use GstBaseTransform here I will, but are you sure i will be able to have the same behaviour using GstBaseTransform ? > 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 I will take a look at gst_audio_buffer_clip. > Better use the GLib types everywhere, gint16 instead of short, etc. In this file and in the vad_private.[ch] i did it, but i forgot some non GLIB types, i will re review it :-). > 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 I'm going to use GstBaseTransform. Thanks a lot for the help Sebastian, I'm learning a lot at each patch, the next one might take some time but the important thing is that it is improving.
Created attachment 190077 [details] [review] new removesilence plugin patch Applied all suggestions made. Instead of changing the timestamp i use GstBaseTransform and GST_BASE_TRANSFORM_FLOW_DROPPED to drop the silence buffers. It gives the desired behaviour with much less code.
Review of attachment 190077 [details] [review]: Looks good in general but: ::: gst/removesilence/gstremovesilence.c @@ +240,3 @@ + if (filter->remove) { + GST_DEBUG ("Removing silence"); + return GST_BASE_TRANSFORM_FLOW_DROPPED; Note that this doesn't result in continous timestamps/durations. You're just dropping the buffer here while (from what you've written before) you want the next buffer to replace it. In that case you have to adjust all following timestamps after dropping the first buffer by the accumulated duration of all buffers.
> Note that this doesn't result in continous timestamps/durations. You're just dropping the buffer here while (from what you've written before) you want the next buffer to replace it. In that case you have to adjust all following timestamps after dropping the first buffer by the accumulated duration of all buffers. It seemed to me that this behaviour is more appropriate, it is up to the elements downstream in the pipeline to decide what to do with the gaps. For example, on the application I'm working the end of the pipeline has an appsink, with sync=false, so i just ignore the gaps generated by the removesilence element and stream the audio without silence periods. But if you link the removesilence element to the pulsesink, the pulsesink element will replace the gaps with silence, which would be useless in this case. Another example would be the following: gst-launch filesrc location=audio.wav ! decodebin2 ! removesilence remove=true ! wavenc ! filesink location=no_silence.wav The no_silence.wav will have no silence periods on it. But if you execute the following: gst-launch filesrc location=audio.wav ! decodebin2 ! removesilence remove=true ! wavenc ! pulsesink the pulsesink element will fill the gaps with silence.
Both use cases are valid: removing silence and adjusting timestamps, and replacing silence with gaps. The property name should reflect that difference, "remove" does not really convey that meaning. Anyway, this is pushed to -bad. Would like to see more patches to improve the element. commit 382346a6f39190ca72368c4a9e2bb2ef563a9842 Author: Tiago César Katcipis <tiagokatcipis@gmail.com> Date: Thu Jun 16 17:19:49 2011 -0300 removesilence: new plugin Fixes: #597822. Signed-off-by: David Schleef <ds@schleef.org>
(In reply to comment #23) > Both use cases are valid: removing silence and adjusting timestamps, and > replacing silence with gaps. The property name should reflect that difference, > "remove" does not really convey that meaning. Using "src ! removesilence remove=true ! audiorate ! sink" would achieve the "removing silence and adjusting timestamps" no ? This way, i think it would suffice having only the "remove silence replacing them with gaps" option. What would be a better name for this property ? remove-with-gaps? > > Anyway, this is pushed to -bad. Would like to see more patches to improve the > element. > > commit 382346a6f39190ca72368c4a9e2bb2ef563a9842 > Author: Tiago César Katcipis <tiagokatcipis@gmail.com> > Date: Thu Jun 16 17:19:49 2011 -0300 > > removesilence: new plugin > > Fixes: #597822. > > Signed-off-by: David Schleef <ds@schleef.org>
(In reply to comment #24) > (In reply to comment #23) > > Both use cases are valid: removing silence and adjusting timestamps, and > > replacing silence with gaps. The property name should reflect that difference, > > "remove" does not really convey that meaning. > > Using "src ! removesilence remove=true ! audiorate ! sink" would achieve the > "removing silence and adjusting timestamps" no ? > > This way, i think it would suffice having only the "remove silence replacing > them with gaps" option. > > What would be a better name for this property ? remove-with-gaps? > that would be a mode like in a "noise-gate".