From 1b417192e6ccf8f47563d40694c33dd898cc2071 Mon Sep 17 00:00:00 2001 From: Yves Lefebvre Date: Fri, 13 Oct 2006 13:27:46 +0000 Subject: [PATCH] gst/gstelement.h: Clarify _NO_PREROLL a bit more. Original commit message from CVS: * gst/gstelement.h: Clarify _NO_PREROLL a bit more. * gst/gstevent.c: Fix docs. * gst/gstpad.c: (gst_pad_link_check_hierarchy), (gst_pad_get_caps_unlocked), (gst_pad_save_thyself), (handle_pad_block), (gst_pad_push_event), (gst_pad_send_event): Patch by: Yves Lefebvre Fix possible deadlock due to wrong locking order. Fixes #361769. Remove some redundant/misplaced checks in pad_block. * libs/gst/base/gstbasesink.c: (gst_base_sink_get_position): For negative rates, count backwards from the duration. --- ChangeLog | 18 ++++++++ gst/gstelement.h | 7 ++- gst/gstevent.c | 2 +- gst/gstpad.c | 92 +++++++++++++++++++------------------ libs/gst/base/gstbasesink.c | 13 +++++- 5 files changed, 83 insertions(+), 49 deletions(-) diff --git a/ChangeLog b/ChangeLog index 5f46d5a87a..65db1dfa03 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,21 @@ +2006-10-13 Wim Taymans + + * gst/gstelement.h: + Clarify _NO_PREROLL a bit more. + + * gst/gstevent.c: + Fix docs. + + * gst/gstpad.c: (gst_pad_link_check_hierarchy), + (gst_pad_get_caps_unlocked), (gst_pad_save_thyself), + (handle_pad_block), (gst_pad_push_event), (gst_pad_send_event): + Patch by: Yves Lefebvre Fix possible deadlock + due to wrong locking order. Fixes #361769. + Remove some redundant/misplaced checks in pad_block. + + * libs/gst/base/gstbasesink.c: (gst_base_sink_get_position): + For negative rates, count backwards from the duration. + 2006-10-13 Tim-Philipp Müller * gst/gsterror.c: (_gst_library_errors_init): diff --git a/gst/gstelement.h b/gst/gstelement.h index 879605b67d..fec28ed667 100644 --- a/gst/gstelement.h +++ b/gst/gstelement.h @@ -77,9 +77,12 @@ G_BEGIN_DECLS * @GST_STATE_CHANGE_FAILURE : the state change failed * @GST_STATE_CHANGE_SUCCESS : the state change succeeded * @GST_STATE_CHANGE_ASYNC : the state change will happen asynchronously - * @GST_STATE_CHANGE_NO_PREROLL: the state change cannot be prerolled + * @GST_STATE_CHANGE_NO_PREROLL: the state change succeeded but the element cannot + * produce data in PAUSED. This typically happens + * with live sources. * - * the possible return values from a state change function. + * The possible return values from a state change function. Only + * @GST_STATE_CHANGE_FAILURE is a real failure. */ typedef enum { GST_STATE_CHANGE_FAILURE = 0, diff --git a/gst/gstevent.c b/gst/gstevent.c index 6a58169d28..7826ec2559 100644 --- a/gst/gstevent.c +++ b/gst/gstevent.c @@ -492,7 +492,7 @@ gst_event_parse_new_segment (GstEvent * event, gboolean * update, * * @start cannot be -1, @stop can be -1. If there * is a valid @stop given, it must be greater or equal the @start, including - * when the indicated playback @rate is < 0 + * when the indicated playback @rate is < 0. * * The @applied_rate value provides information about any rate adjustment that * has already been made to the timestamps and content on the buffers of the diff --git a/gst/gstpad.c b/gst/gstpad.c index dc0ff1964d..6e02742e9e 100644 --- a/gst/gstpad.c +++ b/gst/gstpad.c @@ -1665,43 +1665,50 @@ static gboolean gst_pad_link_check_hierarchy (GstPad * src, GstPad * sink) { GstObject *psrc, *psink; - gboolean res = TRUE; psrc = GST_OBJECT_PARENT (src); psink = GST_OBJECT_PARENT (sink); /* if one of the pads has no parent, we allow the link */ - if (psrc && psink) { - /* if the parents are the same, we have a loop */ - if (G_UNLIKELY (psrc == psink)) - goto same_parents; + if (G_UNLIKELY (psrc == NULL || psink == NULL)) + goto no_parent; - /* if they both have a parent, we check the grandparents */ - psrc = gst_object_get_parent (psrc); - psink = gst_object_get_parent (psink); + /* if the parents are the same, we have a loop */ + if (G_UNLIKELY (psrc == psink)) + goto same_parents; - if (G_UNLIKELY (psrc != psink)) { - /* if they have grandparents but they are not the same */ - GST_CAT_DEBUG (GST_CAT_CAPS, - "pads have different grandparents %" GST_PTR_FORMAT " and %" - GST_PTR_FORMAT, psrc, psink); - res = FALSE; - } - if (psrc) - gst_object_unref (psrc); - if (psink) - gst_object_unref (psink); - } -done: - return res; + /* if they both have a parent, we check the grandparents. We can not lock + * the parent because we hold on the child (pad) and the locking order is + * parent >> child. */ + psrc = GST_OBJECT_PARENT (psrc); + psink = GST_OBJECT_PARENT (psink); + + /* if they have grandparents but they are not the same */ + if (G_UNLIKELY (psrc != psink)) + goto wrong_grandparents; + + return TRUE; /* ERRORS */ +no_parent: + { + GST_CAT_DEBUG (GST_CAT_CAPS, + "one of the pads has no parent %" GST_PTR_FORMAT " and %" + GST_PTR_FORMAT, psrc, psink); + return TRUE; + } same_parents: { GST_CAT_DEBUG (GST_CAT_CAPS, "pads have same parent %" GST_PTR_FORMAT, psrc); - res = FALSE; - goto done; + return FALSE; + } +wrong_grandparents: + { + GST_CAT_DEBUG (GST_CAT_CAPS, + "pads have different grandparents %" GST_PTR_FORMAT " and %" + GST_PTR_FORMAT, psrc, psink); + return FALSE; } } @@ -3203,8 +3210,8 @@ gst_ghost_pad_save_thyself (GstPad * pad, xmlNodePtr parent) * the pad block happens. * * During the actual blocking state, the GST_PAD_BLOCKING flag is set. - * The GST_PAD_BLOCKING flag is unset when the GST_PAD_FLUSHING flag is - * unset. This is to know whether the pad was blocking when GST_PAD_FLUSHING + * The GST_PAD_BLOCKING flag is unset when the pad was unblocked without a + * flush. This is to know whether the pad was blocking when GST_PAD_FLUSHING * was set. * * MT safe. @@ -3242,6 +3249,10 @@ handle_pad_block (GstPad * pad) GST_OBJECT_UNLOCK (pad); callback (pad, TRUE, user_data); GST_OBJECT_LOCK (pad); + + /* we released the lock, recheck flushing */ + if (GST_PAD_IS_FLUSHING (pad)) + goto flushing; } else { /* no callback, signal the thread that is doing a GCond wait * if any. */ @@ -3252,9 +3263,6 @@ handle_pad_block (GstPad * pad) * then could have made the pad unblock so we need to check the blocking * condition again. */ while (GST_PAD_IS_BLOCKED (pad)) { - if (GST_PAD_IS_FLUSHING (pad)) - goto flushing; - /* now we block the streaming thread. It can be unlocked when we * deactivate the pad (which will also set the FLUSHING flag) or * when the pad is unblocked. A flushing event will also unblock @@ -3267,11 +3275,11 @@ handle_pad_block (GstPad * pad) /* see if we got unlocked by a flush or not */ if (GST_PAD_IS_FLUSHING (pad)) goto flushing; - } - /* If we made it here we either never blocked, or were unblocked because we - * weren't flushing, it is therefore safe to remove the BLOCKING flag */ - GST_OBJECT_FLAG_UNSET (pad, GST_PAD_BLOCKING); + /* If we made it here we were unblocked and not flushing, remove the + * blocking flag now. */ + GST_OBJECT_FLAG_UNSET (pad, GST_PAD_BLOCKING); + } GST_CAT_LOG_OBJECT (GST_CAT_SCHEDULING, pad, "got unblocked"); @@ -3883,24 +3891,21 @@ gst_pad_push_event (GstPad * pad, GstEvent * event) case GST_EVENT_FLUSH_STOP: GST_PAD_UNSET_FLUSHING (pad); - /* If pad was blocking on something when the pad received flush-start, we - * don't forward the flush-stop event either. */ + /* If pad was blocking on something when the pad received flush-start, the + * BLOCKING flag was never cleared. we don't forward the flush-stop event + * either then but unset the blocking flag. */ if (G_UNLIKELY (GST_PAD_IS_BLOCKING (pad))) { GST_OBJECT_FLAG_UNSET (pad, GST_PAD_BLOCKING); GST_LOG_OBJECT (pad, "Pad was previously blocking, not forwarding flush-stop"); goto flushed; } - GST_OBJECT_FLAG_UNSET (pad, GST_PAD_BLOCKING); break; default: - if (G_UNLIKELY (GST_PAD_IS_BLOCKED (pad))) { - if (GST_PAD_IS_FLUSHING (pad)) + while (G_UNLIKELY (GST_PAD_IS_BLOCKED (pad))) { + /* block the event as long as the pad is blocked */ + if (handle_pad_block (pad) != GST_FLOW_OK) goto flushed; - while (GST_PAD_IS_BLOCKED (pad)) - /* else block the event as long as the pad is blocked */ - if (handle_pad_block (pad) != GST_FLOW_OK) - goto flushed; } break; } @@ -3942,7 +3947,6 @@ not_linked: GST_OBJECT_UNLOCK (pad); return FALSE; } - flushed: { GST_DEBUG_OBJECT (pad, @@ -4049,7 +4053,7 @@ gst_pad_send_event (GstPad * pad, GstEvent * event) goto flushing; if (serialized) { - /* lock order: STREAM_LOCK, LOCK */ + /* lock order: STREAM_LOCK, LOCK, recheck flushing. */ GST_OBJECT_UNLOCK (pad); GST_PAD_STREAM_LOCK (pad); need_unlock = TRUE; diff --git a/libs/gst/base/gstbasesink.c b/libs/gst/base/gstbasesink.c index c3a0e66420..52fda62618 100644 --- a/libs/gst/base/gstbasesink.c +++ b/libs/gst/base/gstbasesink.c @@ -2326,7 +2326,7 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format, case GST_FORMAT_TIME: { GstClockTime now, base; - gint64 time, accum; + gint64 time, accum, duration; gdouble rate; gint64 last; @@ -2361,6 +2361,11 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format, else time = 0; + if (GST_CLOCK_TIME_IS_VALID (basesink->segment.stop)) + duration = basesink->segment.stop - basesink->segment.start; + else + duration = 0; + base = GST_ELEMENT_CAST (basesink)->base_time; accum = basesink->segment.accum; rate = basesink->segment.rate * basesink->segment.applied_rate; @@ -2379,7 +2384,11 @@ gst_base_sink_get_position (GstBaseSink * basesink, GstFormat format, * rate and applied rate. */ base += accum; base = MIN (now, base); - /* for negative rate this will count back from the segment time */ + + /* for negative rates we need to count back from from the segment + * duration. */ + if (rate < 0.0) + time += duration; *cur = time + gst_guint64_to_gdouble (now - base) * rate; /* never report more than last seen position */