GNOME Bugzilla – Bug 317438
[speed] fix 0.9 port
Last modified: 2005-10-19 11:13:57 UTC
# # Please take a special look in the functions (speed_src_event, # gst_speed_convert and speed_src_query) to see if it has been really fixed # # diff -urNaw gst-plugins-bad.orig gst-plugins-bad > gst-plugins-bad.patch # the contents of gst-plugins-bad.patch follows below # diff -urNaw gst-plugins-bad.orig/ChangeLog gst-plugins-bad/ChangeLog --- gst-plugins-bad.orig/ChangeLog 2005-09-26 12:19:38.000000000 -0300 +++ gst-plugins-bad/ChangeLog 2005-09-28 10:53:26.852942288 -0300 @@ -1,3 +1,13 @@ +2005-09-28 Edgard Lima <edgard.lima@indt.org.br> + + * gst/speed/gstspeed.c: (speed_chain), (speed_setcaps), + (speed_parse_caps), (speed_src_event), (speed_sink_event), + (speed_src_query), (speed_init), (speed_set_property), + (speed_change_state), (gst_speed_convert): + Fixed speed - the previous version, 1.38, has been ported to 0.9 + from a wrong version, 1.37 (from 1.36). That fix already includes + the changes done in 1.36.2.4. + 2005-09-26 Christian Schaller <uraeus@gnome.org> * configure.ac: add speed and rfb where needed diff -urNaw gst-plugins-bad.orig/gst/speed/gstspeed.c gst-plugins-bad/gst/speed/gstspeed.c --- gst-plugins-bad.orig/gst/speed/gstspeed.c 2005-09-08 12:56:38.000000000 -0300 +++ gst-plugins-bad/gst/speed/gstspeed.c 2005-09-28 10:53:50.977274832 -0300 @@ -87,13 +87,15 @@ static GstFlowReturn speed_chain (GstPad * pad, GstBuffer * buf); -static GstStateChangeReturn speed_change_state (GstElement * element, - GstStateChange transition); +static GstStateChangeReturn speed_change_state (GstElement * element, GstStateChange transition); + +static gboolean speed_sink_event (GstPad * pad, GstEvent * event); +static gboolean speed_src_event (GstPad * pad, GstEvent * event); static GstElementClass *parent_class; /* NULL */ static gboolean -gst_speed_setcaps (GstPad * pad, GstCaps * caps) +speed_setcaps (GstPad * pad, GstCaps * caps) { GstSpeed *filter; GstPad *otherpad; @@ -103,10 +105,12 @@ g_return_val_if_fail (GST_IS_SPEED (filter), GST_PAD_LINK_REFUSED); otherpad = (pad == filter->srcpad) ? filter->sinkpad : filter->srcpad; - if (!speed_parse_caps (filter, caps)) - return GST_PAD_LINK_REFUSED; + if (!speed_parse_caps (filter, caps)) { + return FALSE; + } return gst_pad_set_caps (otherpad, caps); + } static gboolean @@ -114,7 +118,7 @@ { const gchar *mimetype; GstStructure *structure; - gboolean ret; + gint rate, chans, width, buffer_frames; g_return_val_if_fail (filter != NULL, FALSE); g_return_val_if_fail (caps != NULL, FALSE); @@ -126,15 +130,24 @@ filter->format = GST_SPEED_FORMAT_FLOAT; else if (strcmp (mimetype, "audio/x-raw-int") == 0) filter->format = GST_SPEED_FORMAT_INT; - else + else { return FALSE; + } - ret = gst_structure_get_int (structure, "rate", &filter->rate); - ret &= gst_structure_get_int (structure, "channels", &filter->channels); - ret &= gst_structure_get_int (structure, "width", &filter->width); + if (!gst_structure_get_int (structure, "rate", &rate) + || !gst_structure_get_int (structure, "width", &width) + || !gst_structure_get_int (structure, "channels", &chans)) { + return FALSE; + } + + filter->rate = rate; + filter->width = width; + filter->channels = chans; + if (gst_structure_get_int (structure, "buffer-frames", &buffer_frames)) + filter->buffer_frames = buffer_frames; + else filter->buffer_frames = 0; - gst_structure_get_int (structure, "buffer-frames", &filter->buffer_frames); if (filter->format == GST_SPEED_FORMAT_FLOAT) { filter->sample_size = filter->channels * filter->width / 8; @@ -143,7 +156,7 @@ filter->sample_size = filter->channels * filter->width / 8; } - return ret; + return TRUE; } GType @@ -182,7 +195,111 @@ } static gboolean +speed_src_event (GstPad * pad, GstEvent * event) +{ + GstSpeed *filter; + GstEvent *newevent; + + filter = GST_SPEED (gst_pad_get_parent (pad)); + + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_SEEK: + { + gdouble rate; + GstFormat format; + GstSeekFlags flags; + GstSeekType start_type, stop_type; + gint64 start, stop; + + gst_event_parse_seek (event, &rate, &format, &flags, + &start_type, &start, &stop_type, &stop); + + newevent = gst_event_new_seek (rate, format, flags, start_type, start * filter->speed, stop_type, stop); + + gst_event_unref (event); + event = newevent; + return gst_pad_send_event (GST_PAD_PEER (filter->sinkpad), event); + break; + } + default: + break; + } + + return gst_pad_event_default (pad, event); +} + +static gboolean +gst_speed_convert (GstPad * pad, GstFormat src_format, gint64 src_value, + GstFormat * dest_format, gint64 * dest_value) +{ + gboolean res = TRUE; + guint scale = 1; + GstSpeed *filter; + + if ( src_format == *dest_format ) { + *dest_value = src_value; + return TRUE; + } + + filter = GST_SPEED (gst_pad_get_parent (pad)); + + switch (src_format) { + case GST_FORMAT_BYTES: + switch (*dest_format) { + case GST_FORMAT_DEFAULT: + if (filter->sample_size == 0) + return FALSE; + *dest_value = src_value / filter->sample_size; + break; + case GST_FORMAT_TIME: + { + gint byterate = filter->sample_size * filter->rate; + + if (byterate == 0) + return FALSE; + *dest_value = src_value * GST_SECOND / byterate; + break; + } + default: + res = FALSE; + } + break; + case GST_FORMAT_DEFAULT: + switch (*dest_format) { + case GST_FORMAT_BYTES: + *dest_value = src_value * filter->sample_size; + break; + case GST_FORMAT_TIME: + if (filter->rate == 0) + return FALSE; + *dest_value = src_value * GST_SECOND / filter->rate; + break; + default: + res = FALSE; + } + break; + case GST_FORMAT_TIME: + switch (*dest_format) { + case GST_FORMAT_BYTES: + scale = filter->sample_size; + /* fallthrough */ + case GST_FORMAT_DEFAULT: + *dest_value = + src_value * scale * filter->rate / GST_SECOND; + break; + default: + res = FALSE; + } + break; + default: + res = FALSE; + } + return res; +} + +static gboolean speed_src_query (GstPad * pad, GstQuery * query) + { gboolean res = TRUE; GstSpeed *filter; @@ -192,7 +309,140 @@ switch (GST_QUERY_TYPE (query)) { case GST_QUERY_POSITION: { + GstFormat format; + GstFormat rformat; + gint64 cur, end; + GstPad *peer; + + /* save requested format */ + gst_query_parse_position (query, &format, NULL, NULL); + + /* query peer for total length in bytes */ + gst_query_set_position (query, GST_FORMAT_BYTES, -1, -1); + + if ((peer = gst_pad_get_peer (filter->sinkpad)) == NULL) + goto error; + + if (!gst_pad_query (peer, query)) { + GST_LOG_OBJECT (filter, "query on peer pad failed"); + goto error; + } + gst_object_unref (peer); + + /* get the returned format */ + gst_query_parse_position (query, &rformat, NULL, &end); + if (rformat == GST_FORMAT_BYTES) + GST_LOG_OBJECT (filter, "peer pad returned total=%lld bytes", + end); + else if (rformat == GST_FORMAT_TIME) + GST_LOG_OBJECT (filter, "peer pad returned time=%lld", end); + + /* Check if requested format is returned format */ + /* + if (format == rformat) { + cur = (gint64) (((gdouble) cur) / filter->speed); + gst_query_set_position (query, format, cur, total); + return TRUE; + } + */ + + GstFormat conv_format = GST_FORMAT_TIME; + + /* convert to time format */ + if ( ! gst_speed_convert (pad, rformat, cur, &conv_format, &cur) ) { + return FALSE; + } + /* convert to time format */ + if ( ! gst_speed_convert (pad, rformat, end, &conv_format, &end) ) { + return FALSE; + } + + /* adjust for speed factor */ + cur /= filter->speed; + end /= filter->speed; + + /* convert to time format */ + if ( ! gst_speed_convert (pad, conv_format, cur, &format, &cur) ) { + return FALSE; + } + /* convert to time format */ + if ( ! gst_speed_convert (pad, conv_format, end, &format, &end) ) { + return FALSE; + } + + gst_query_set_position (query, format, cur, end); + + GST_LOG_OBJECT (filter, + "position query: peer returned total: %llu - we return %llu (format %u)", + end, cur, format); + + break; + } + default: + res = FALSE; + break; + } + + return res; + +error: + + GST_DEBUG ("error handling query"); + return FALSE; +} + +#if 0 +speed_src_query (GstPad * pad, GstQueryType type, + GstFormat * format, gint64 * val) +{ + gboolean res = TRUE; + GstSpeed *filter; + + filter = GST_SPEED (gst_pad_get_parent (pad)); + + switch (type) { + case GST_QUERY_POSITION: + { + switch (*format) { + case GST_FORMAT_BYTES: + case GST_FORMAT_DEFAULT: + case GST_FORMAT_TIME: + { + gint64 peer_value; + const GstFormat *peer_formats; + res = FALSE; + + peer_formats = gst_pad_get_formats (GST_PAD_PEER (filter->sinkpad)); + + while (peer_formats && *peer_formats && !res) { + + GstFormat peer_format = *peer_formats; + + /* do the probe */ + if (gst_pad_query (GST_PAD_PEER (filter->sinkpad), type, + &peer_format, &peer_value)) { + GstFormat conv_format; + + /* convert to TIME */ + conv_format = GST_FORMAT_TIME; + res = gst_pad_convert (filter->sinkpad, + peer_format, peer_value, &conv_format, val); + + /* adjust for speed factor */ + *val = (gint64) (((gdouble) * val) / filter->speed); + + /* and to final format */ + res &= gst_pad_convert (pad, GST_FORMAT_TIME, *val, format, val); + } + peer_formats++; + } + break; + } + default: + res = FALSE; + break; + } break; } default: @@ -201,6 +451,7 @@ } return res; } +#endif static void speed_base_init (gpointer g_class) @@ -237,24 +488,25 @@ filter->sinkpad = gst_pad_new_from_template (gst_static_pad_template_get (&gst_speed_sink_template), "sink"); - gst_pad_set_setcaps_function (filter->sinkpad, gst_speed_setcaps); + gst_pad_set_setcaps_function (filter->sinkpad, speed_setcaps); gst_pad_set_chain_function (filter->sinkpad, speed_chain); - gst_pad_set_getcaps_function (filter->sinkpad, gst_pad_proxy_getcaps); gst_element_add_pad (GST_ELEMENT (filter), filter->sinkpad); + gst_pad_set_event_function (filter->sinkpad, speed_sink_event); filter->srcpad = gst_pad_new_from_template (gst_static_pad_template_get (&gst_speed_src_template), "src"); - /*gst_pad_set_link_function (filter->srcpad, speed_link); */ - gst_pad_set_setcaps_function (filter->srcpad, gst_speed_setcaps); - gst_pad_set_getcaps_function (filter->srcpad, gst_pad_proxy_getcaps); + gst_pad_set_setcaps_function (filter->srcpad, speed_setcaps); gst_pad_set_query_type_function (filter->srcpad, speed_get_query_types); gst_pad_set_query_function (filter->srcpad, speed_src_query); gst_element_add_pad (GST_ELEMENT (filter), filter->srcpad); + gst_pad_set_event_function (filter->srcpad, speed_src_event); + filter->offset = 0; filter->timestamp = 0; filter->sample_size = 0; + } static inline guint @@ -323,64 +575,103 @@ return j; } -static GstFlowReturn -speed_chain (GstPad * pad, GstBuffer * buf) + +static gboolean speed_sink_event (GstPad * pad, GstEvent * event) { + + + switch (GST_EVENT_TYPE (event)) { + case GST_EVENT_NEWSEGMENT: { - GstBuffer *out_buf; GstSpeed *filter; - guint c, in_samples, out_samples, out_size; - GstFlowReturn result = GST_FLOW_OK; + gint64 timestamp, offset; + gdouble rate; + GstFormat format; + gint64 start_value, stop_value, base; + GstEvent *newevent; -/* - g_return_if_fail (pad != NULL); - g_return_if_fail (GST_IS_PAD (pad)); - g_return_if_fail (data != NULL); -*/ filter = GST_SPEED (GST_OBJECT_PARENT (pad)); - /*g_return_if_fail (GST_IS_SPEED (filter)); */ + g_return_val_if_fail (GST_IS_SPEED (filter), FALSE); - if (GST_IS_EVENT (buf)) { - switch (GST_EVENT_TYPE (GST_EVENT (buf))) { - case GST_EVENT_NEWSEGMENT: - { /* - gint64 timestamp, offset; - GstFormat format = GST_FORMAT_UNDEFINED; - gint64 value, end_value; + gst_event_parse_newsegment (event, &rate, &format, &start_value, &stop_value, &base); - gst_event_parse_newsegment (GST_EVENT (buf), NULL, &format, &value, &end_value, NULL); if (format == GST_FORMAT_TIME) { - filter->timestamp = timestamp; - filter->offset = timestamp * filter->rate / GST_SECOND; + + g_assert (filter->speed > 0); + + timestamp = start_value; + + filter->timestamp = timestamp / filter->speed; + filter->offset = filter->timestamp * filter->rate / GST_SECOND; + + newevent = gst_event_new_newsegment (rate, format, filter->timestamp, stop_value, base); + + } else if ( format == GST_FORMAT_BYTES ) { + + g_assert (filter->speed > 0); + if ( filter->rate == 0 ) { + filter->rate = 1.0; } - if (format == GST_FORMAT_BYTES) { - filter->offset = offset; - filter->timestamp = offset * GST_SECOND / filter->rate; + + offset = start_value; + + filter->offset = offset / filter->speed; + filter->timestamp = filter->offset * GST_SECOND / filter->rate; + + newevent = gst_event_new_newsegment (rate, format, filter->offset, stop_value, base); + } - */ + gst_event_unref (event); + event = newevent; + break; } default: break; } - gst_pad_event_default (pad, GST_EVENT (buf)); - return GST_FLOW_OK; + + return gst_pad_event_default (pad, event); + } - out_size = ceil ((gfloat) GST_BUFFER_SIZE (buf) / filter->speed); +static GstFlowReturn +speed_chain (GstPad * pad, GstBuffer * in_buf) +{ + GstBuffer *out_buf; + GstSpeed *filter; + guint c, in_samples, out_samples, out_size; + GstFlowReturn result = GST_FLOW_OK; + + g_return_val_if_fail (pad != NULL, GST_FLOW_ERROR); + g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); + + filter = GST_SPEED (GST_OBJECT_PARENT (pad)); + g_return_val_if_fail (GST_IS_SPEED (filter), GST_FLOW_ERROR); + + + /* buffersize has to be aligned by samplesize */ + out_size = ceil ((gfloat) GST_BUFFER_SIZE (in_buf) / filter->speed); + out_size = (filter->sample_size * (out_size + filter->sample_size - 1)) / + filter->sample_size; + result = gst_pad_alloc_buffer (filter->srcpad, -1, out_size, GST_PAD_CAPS (filter->srcpad), &out_buf); - in_samples = GST_BUFFER_SIZE (buf) / filter->sample_size; + if ( result != GST_FLOW_OK ) { + return result; + } + + in_samples = GST_BUFFER_SIZE (in_buf) / filter->sample_size; out_samples = 0; for (c = 0; c < filter->channels; ++c) { if (filter->format == GST_SPEED_FORMAT_INT) { - out_samples = speed_chain_int16 (filter, buf, out_buf, c, in_samples); + out_samples = speed_chain_int16 (filter, in_buf, out_buf, c, in_samples); } else { - out_samples = speed_chain_float32 (filter, buf, out_buf, c, in_samples); + out_samples = + speed_chain_float32 (filter, in_buf, out_buf, c, in_samples); } } @@ -397,8 +688,8 @@ gst_pad_push (filter->srcpad, out_buf); - return result; - /*gst_buffer_unref (in_buf); */ + return GST_FLOW_OK; + } static void @@ -414,6 +705,7 @@ filter->speed = g_value_get_float (value); break; default: + G_OBJECT_WARN_INVALID_PROPERTY_ID(object, prop_id, pspec); break; } } @@ -442,12 +734,9 @@ GstSpeed *speed = GST_SPEED (element); switch (transition) { - case GST_STATE_CHANGE_PAUSED_TO_READY: - break; case GST_STATE_CHANGE_READY_TO_PAUSED: speed->offset = 0; speed->timestamp = 0; - speed->sample_size = 0; break; default: break;
Some more comments: * There are still references to the old buffer-frames stuff: - in GST_SPEED_AUDIO_CAPS - in the _GstSpeed structure in gstspeed.h - in speed_parse_caps() * in speed_setcaps(): - This line filter = GST_SPEED (gst_pad_get_parent (pad)); should be filter = GST_SPEED (GST_PAD_PARENT (pad)); as _get_parent() increases the refcount in 0.9 - These two assertions are not really necessary: g_return_val_if_fail (filter != NULL, GST_PAD_LINK_REFUSED); g_return_val_if_fail (GST_IS_SPEED (filter), GST_PAD_LINK_REFUSED); as GST_SPEED() will already throw a warning if filter isn't a GstSpeed object * stylistic issue in gst_speed_parse_caps(): - This can be written nicer in 0.9: mimetype = gst_structure_get_name (structure); if (strcmp (mimetype, "audio/x-raw-float") == 0) can be replaced with if (gst_structure_has_name (structure, "audio/x-raw-float")) * in speed_src_event(): - gst_pad_get_parent => GST_PAD_PARENT - you should only rewrite seek events as you do for formats you know, ie. GST_FORMAT_DEFAULT, GST_FORMAT_TIME, GST_FORMAT_BYTES. For other formats, either unref the event and return FALSE or pray and send it on unmodified (example: you have cddasrc ! speed ! sink and the seek events seeks to CD track N - you don't really want to modify that). - you need to adjust stop_value as well if, and only if, stop_stype is not SEEK_TYPE_NONE. - break after return is not needed in the case statement - instead of event = newevent; return gst_pad_send_event (foopad, event); just do return gst_pad_send_event (foopad, newevent); * in gst_speed_convert(): - gst_pad_get_parent => GST_PAD_PARENT * in speed_src_query(): - gst_pad_get_parent => GST_PAD_PARENT - must unref peer if gst_pad_query() fails as well - 'cur' is used uninitialised (no idea why the compiler doesn't warn about it in this case) - all variable declarations should be at the beginning of the code block (here: conv_format) - why do you query the peer in BYTES format and then convert to TIME format, instead of querying directly in TIME format? - you can use gst_pad_query_position() directly on the peer pad, without modifying the query object (less confusing maybe). - why don't you retrieve the current position when calling gst_query_parse_position()? * speed_change_state() isn't thread-safe, see porting guide in Plugin Writer's Guide appendix for details (first handle upwards state changes, then chain up to parent class, then handle downwards state changes) * gst_speed_chain() - still leaks the input buffer - the code to align out_size to sample_size is wrong, which is not your fault though, but it's in 0.8 as well * speed_sink_event(): - should handle and forward EOS (taking the STREAM_LOCK) - take the STREAM_LOCK while handling and forwarding the newsegment event - stop_value in event should be adjusted as well - should handle GST_FORMAT_DEFAULT (=samples) That's all I could find for now :) Hope you're not discouraged by the long list, most of it is trivial stuff or style issues after all. Btw, next time please add the patch as attachement in bugzilla (easier to download). Cheers -Tim
Created attachment 53207 [details] [review] patch to apply changes proposed by tim
Looks fine, please commit to CVS, if there's anything else that can always be fixed later. Here's two more issues: - at the end of the chain function you should probably replace gst_pad_push (filter->srcpad, out_buf); gst_buffer_unref (in_buf); return GST_FLOW_OK; with result = gst_pad_push (filter->srcpad, out_buf); gst_buffer_unref (in_buf); return result; - generally there aren't any locks anywhere when speed->foo members are accessed (e.g. in the set/get_property functions, in the chain function and in the query+event functions). These can be accessed concurrently from multiple threads though, so you should guard against by surrounding those critical sections with GST_LOCK (speed) and GST_UNLOCK (speed). Cheers -Tim
All suggestions applied and commit, thanks very much!