From f9c85a53dc8e18d7d1151dae3980a8835329f30f Mon Sep 17 00:00:00 2001 From: Wim Taymans Date: Tue, 20 Mar 2012 13:14:55 +0100 Subject: [PATCH] pad: improve docs of get/pull_range Improve the docs of the get/pull_range functions, define the lifetime of the buffer in case of errors and short reads. Make sure the code does what the docs say. --- gst/gstpad.c | 83 ++++++++++++++++++++++++------------ libs/gst/base/gstbasesrc.c | 45 ++++++++++++------- plugins/elements/gstqueue2.c | 17 +++++--- 3 files changed, 97 insertions(+), 48 deletions(-) diff --git a/gst/gstpad.c b/gst/gstpad.c index 6d38440d70..4fe7a1d1d0 100644 --- a/gst/gstpad.c +++ b/gst/gstpad.c @@ -3818,6 +3818,7 @@ gst_pad_get_range_unchecked (GstPad * pad, guint64 offset, guint size, GstFlowReturn ret; GstPadGetRangeFunction getrangefunc; GstObject *parent; + GstBuffer *res_buf; GST_PAD_STREAM_LOCK (pad); @@ -3831,11 +3832,13 @@ gst_pad_get_range_unchecked (GstPad * pad, guint64 offset, guint size, if (G_UNLIKELY ((ret = check_sticky (pad))) != GST_FLOW_OK) goto events_error; + res_buf = *buffer; + /* when one of the probes returns DROPPED, probe_stopped will be called - * and we skip calling the getrange function, *buffer should then contain a + * and we skip calling the getrange function, res_buf should then contain a * valid result buffer */ PROBE_PULL (pad, GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BLOCK, - *buffer, offset, size, probe_stopped); + res_buf, offset, size, probe_stopped); ACQUIRE_PARENT (pad, parent, no_parent); GST_OBJECT_UNLOCK (pad); @@ -3848,7 +3851,7 @@ gst_pad_get_range_unchecked (GstPad * pad, guint64 offset, guint size, G_GUINT64_FORMAT ", size %u", GST_DEBUG_FUNCPTR_NAME (getrangefunc), offset, size); - ret = getrangefunc (pad, parent, offset, size, buffer); + ret = getrangefunc (pad, parent, offset, size, &res_buf); RELEASE_PARENT (parent); @@ -3859,11 +3862,13 @@ gst_pad_get_range_unchecked (GstPad * pad, guint64 offset, guint size, GST_OBJECT_LOCK (pad); probed_data: PROBE_PULL (pad, GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BUFFER, - *buffer, offset, size, probe_stopped_unref); + res_buf, offset, size, probe_stopped_unref); GST_OBJECT_UNLOCK (pad); GST_PAD_STREAM_UNLOCK (pad); + *buffer = res_buf; + return ret; /* ERRORS */ @@ -3910,7 +3915,7 @@ probe_stopped: GST_CAT_LOG_OBJECT (GST_CAT_SCHEDULING, pad, "probe returned %s", gst_flow_get_name (ret)); if (ret == GST_FLOW_CUSTOM_SUCCESS) { - if (*buffer) { + if (res_buf) { /* the probe filled the buffer and asks us to not call the getrange * anymore, we continue with the post probes then. */ GST_DEBUG_OBJECT (pad, "handled buffer"); @@ -3936,14 +3941,13 @@ probe_stopped_unref: ret = GST_FLOW_EOS; GST_OBJECT_UNLOCK (pad); GST_PAD_STREAM_UNLOCK (pad); - gst_buffer_unref (*buffer); - *buffer = NULL; + if (*buffer == NULL) + gst_buffer_unref (res_buf); return ret; } get_range_failed: { GST_PAD_STREAM_UNLOCK (pad); - *buffer = NULL; GST_CAT_LEVEL_LOG (GST_CAT_SCHEDULING, (ret >= GST_FLOW_EOS) ? GST_LEVEL_INFO : GST_LEVEL_WARNING, pad, "getrange failed, flow: %s", gst_flow_get_name (ret)); @@ -3967,8 +3971,22 @@ get_range_failed: * installed (see gst_pad_set_getrange_function()) this function returns * #GST_FLOW_NOT_SUPPORTED. * - * @buffer must point to a variable holding NULL or a variable that - * points to a #GstBuffer that will be filled with the result data. + * If @buffer points to a variable holding NULL, a valid new #GstBuffer will be + * placed in @buffer when this function returns #GST_FLOW_OK. The new buffer + * must be freed with gst_buffer_unref() after usage. + * + * When @buffer points to a variable that points to a valid #GstBuffer, the + * buffer will be filled with the result data when this function returns + * #GST_FLOW_OK. If the provided buffer is larger than @size, only + * @size bytes will be filled in the result buffer and its size will be updated + * accordingly. + * + * Note that less than @size bytes can be returned in @buffer when, for example, + * an EOS condition is near or when @buffer is not large enough to hold @size + * bytes. The caller should check the result buffer size to get the result size. + * + * When this function returns any other result value than #GST_FLOW_OK, @buffer + * will be unchanged. * * This is a lowlevel function. Usualy gst_pad_pull_range() is used. * @@ -3997,7 +4015,7 @@ gst_pad_get_range (GstPad * pad, guint64 offset, guint size, * @buffer: (out callee-allocates): a pointer to hold the #GstBuffer, returns * GST_FLOW_ERROR if %NULL. * - * Pulls a @buffer from the peer pad. + * Pulls a @buffer from the peer pad or fills up a provided buffer. * * This function will first trigger the pad block signal if it was * installed. @@ -4007,14 +4025,23 @@ gst_pad_get_range (GstPad * pad, guint64 offset, guint size, * See gst_pad_get_range() for a list of return values and for the * semantics of the arguments of this function. * - * @buffer must point to a variable holding NULL or a variable that - * points to a #GstBuffer that will be filled with the result data. + * If @buffer points to a variable holding NULL, a valid new #GstBuffer will be + * placed in @buffer when this function returns #GST_FLOW_OK. The new buffer + * must be freed with gst_buffer_unref() after usage. When this function + * returns any other result value, @buffer will still point to NULL. + * + * When @buffer points to a variable that points to a valid #GstBuffer, the + * buffer will be filled with the result data when this function returns + * #GST_FLOW_OK. When this function returns any other result value, + * @buffer will be unchanged. If the provided buffer is larger than @size, only + * @size bytes will be filled in the result buffer and its size will be updated + * accordingly. + * + * Note that less than @size bytes can be returned in @buffer when, for example, + * an EOS condition is near or when @buffer is not large enough to hold @size + * bytes. The caller should check the result buffer size to get the result size. * * Returns: a #GstFlowReturn from the peer pad. - * When this function returns #GST_FLOW_OK, @buffer will contain a valid - * #GstBuffer that should be freed with gst_buffer_unref() after usage. - * @buffer may not be used or freed when any other return value than - * #GST_FLOW_OK is returned. * * MT safe. */ @@ -4024,6 +4051,7 @@ gst_pad_pull_range (GstPad * pad, guint64 offset, guint size, { GstPad *peer; GstFlowReturn ret; + GstBuffer *res_buf; g_return_val_if_fail (GST_IS_PAD (pad), GST_FLOW_ERROR); g_return_val_if_fail (GST_PAD_IS_SINK (pad), GST_FLOW_ERROR); @@ -4038,11 +4066,13 @@ gst_pad_pull_range (GstPad * pad, guint64 offset, guint size, if (G_UNLIKELY (GST_PAD_MODE (pad) != GST_PAD_MODE_PULL)) goto wrong_mode; + res_buf = *buffer; + /* when one of the probes returns DROPPED, probe_stopped will be * called and we skip calling the peer getrange function. *buffer should then * contain a valid buffer */ PROBE_PULL (pad, GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BLOCK, - *buffer, offset, size, probe_stopped); + res_buf, offset, size, probe_stopped); if (G_UNLIKELY ((peer = GST_PAD_PEER (pad)) == NULL)) goto not_linked; @@ -4051,7 +4081,7 @@ gst_pad_pull_range (GstPad * pad, guint64 offset, guint size, pad->priv->using++; GST_OBJECT_UNLOCK (pad); - ret = gst_pad_get_range_unchecked (peer, offset, size, buffer); + ret = gst_pad_get_range_unchecked (peer, offset, size, &res_buf); gst_object_unref (peer); @@ -4068,10 +4098,12 @@ gst_pad_pull_range (GstPad * pad, guint64 offset, guint size, probed_data: PROBE_PULL (pad, GST_PAD_PROBE_TYPE_PULL | GST_PAD_PROBE_TYPE_BUFFER, - *buffer, offset, size, probe_stopped_unref); + res_buf, offset, size, probe_stopped_unref); GST_OBJECT_UNLOCK (pad); + *buffer = res_buf; + return ret; /* ERROR recovery here */ @@ -4094,7 +4126,7 @@ probe_stopped: GST_CAT_LOG_OBJECT (GST_CAT_SCHEDULING, pad, "pre probe returned %s", gst_flow_get_name (ret)); if (ret == GST_FLOW_CUSTOM_SUCCESS) { - if (*buffer) { + if (res_buf) { /* the probe filled the buffer and asks us to not forward to the peer * anymore, we continue with the post probes then */ GST_DEBUG_OBJECT (pad, "handled buffer"); @@ -4118,7 +4150,6 @@ not_linked: } pull_range_failed: { - *buffer = NULL; GST_OBJECT_UNLOCK (pad); GST_CAT_LEVEL_LOG (GST_CAT_SCHEDULING, (ret >= GST_FLOW_EOS) ? GST_LEVEL_INFO : GST_LEVEL_WARNING, @@ -4131,12 +4162,10 @@ probe_stopped_unref: "post probe returned %s", gst_flow_get_name (ret)); GST_OBJECT_UNLOCK (pad); /* if we drop here, it signals EOS */ - if (ret == GST_FLOW_CUSTOM_SUCCESS) { + if (ret == GST_FLOW_CUSTOM_SUCCESS) ret = GST_FLOW_EOS; - gst_buffer_unref (*buffer); - } else if (ret == GST_FLOW_OK) - gst_buffer_unref (*buffer); - *buffer = NULL; + if (*buffer == NULL) + gst_buffer_unref (res_buf); return ret; } } diff --git a/libs/gst/base/gstbasesrc.c b/libs/gst/base/gstbasesrc.c index 347d655eb2..fdd3a4d8d1 100644 --- a/libs/gst/base/gstbasesrc.c +++ b/libs/gst/base/gstbasesrc.c @@ -1379,6 +1379,7 @@ gst_base_src_default_create (GstBaseSrc * src, guint64 offset, { GstBaseSrcClass *bclass; GstFlowReturn ret; + GstBuffer *res_buf; bclass = GST_BASE_SRC_GET_CLASS (src); @@ -1390,18 +1391,22 @@ gst_base_src_default_create (GstBaseSrc * src, guint64 offset, if (*buffer == NULL) { /* downstream did not provide us with a buffer to fill, allocate one * ourselves */ - ret = bclass->alloc (src, offset, size, buffer); + ret = bclass->alloc (src, offset, size, &res_buf); if (G_UNLIKELY (ret != GST_FLOW_OK)) goto alloc_failed; + } else { + res_buf = *buffer; } if (G_LIKELY (size > 0)) { /* only call fill when there is a size */ - ret = bclass->fill (src, offset, size, *buffer); + ret = bclass->fill (src, offset, size, res_buf); if (G_UNLIKELY (ret != GST_FLOW_OK)) goto not_ok; } + *buffer = res_buf; + return GST_FLOW_OK; /* ERRORS */ @@ -1419,7 +1424,8 @@ not_ok: { GST_DEBUG_OBJECT (src, "fill returned %d (%s)", ret, gst_flow_get_name (ret)); - gst_buffer_unref (*buffer); + if (*buffer == NULL) + gst_buffer_unref (res_buf); return ret; } } @@ -2242,6 +2248,7 @@ gst_base_src_get_range (GstBaseSrc * src, guint64 offset, guint length, GstFlowReturn ret; GstBaseSrcClass *bclass; GstClockReturn status; + GstBuffer *res_buf; bclass = GST_BASE_SRC_GET_CLASS (src); @@ -2287,15 +2294,17 @@ again: "calling create offset %" G_GUINT64_FORMAT " length %u, time %" G_GINT64_FORMAT, offset, length, src->segment.time); - ret = bclass->create (src, offset, length, buf); + res_buf = *buf; + + ret = bclass->create (src, offset, length, &res_buf); /* The create function could be unlocked because we have a pending EOS. It's * possible that we have a valid buffer from create that we need to * discard when the create function returned _OK. */ if (G_UNLIKELY (g_atomic_int_get (&src->priv->pending_eos))) { if (ret == GST_FLOW_OK) { - gst_buffer_unref (*buf); - *buf = NULL; + if (*buf == NULL) + gst_buffer_unref (res_buf); } goto eos; } @@ -2305,14 +2314,14 @@ again: /* no timestamp set and we are at offset 0, we can timestamp with 0 */ if (offset == 0 && src->segment.time == 0 - && GST_BUFFER_TIMESTAMP (*buf) == -1 && !src->is_live) { + && GST_BUFFER_TIMESTAMP (res_buf) == -1 && !src->is_live) { GST_DEBUG_OBJECT (src, "setting first timestamp to 0"); - *buf = gst_buffer_make_writable (*buf); - GST_BUFFER_TIMESTAMP (*buf) = 0; + res_buf = gst_buffer_make_writable (res_buf); + GST_BUFFER_TIMESTAMP (res_buf) = 0; } /* now sync before pushing the buffer */ - status = gst_base_src_do_sync (src, *buf); + status = gst_base_src_do_sync (src, res_buf); /* waiting for the clock could have made us flushing */ if (G_UNLIKELY (src->priv->flushing)) @@ -2331,8 +2340,9 @@ again: /* this case is triggered when we were waiting for the clock and * it got unlocked because we did a state change. In any case, get rid of * the buffer. */ - gst_buffer_unref (*buf); - *buf = NULL; + if (*buf == NULL) + gst_buffer_unref (res_buf); + if (!src->live_running) { /* We return WRONG_STATE when we are not running to stop the dataflow also * get rid of the produced buffer. */ @@ -2352,11 +2362,14 @@ again: GST_ELEMENT_ERROR (src, CORE, CLOCK, (_("Internal clock error.")), ("clock returned unexpected return value %d", status)); - gst_buffer_unref (*buf); - *buf = NULL; + if (*buf == NULL) + gst_buffer_unref (res_buf); ret = GST_FLOW_ERROR; break; } + if (G_LIKELY (ret == GST_FLOW_OK)) + *buf = res_buf; + return ret; /* ERROR */ @@ -2396,8 +2409,8 @@ reached_num_buffers: flushing: { GST_DEBUG_OBJECT (src, "we are flushing"); - gst_buffer_unref (*buf); - *buf = NULL; + if (*buf == NULL) + gst_buffer_unref (res_buf); return GST_FLOW_FLUSHING; } eos: diff --git a/plugins/elements/gstqueue2.c b/plugins/elements/gstqueue2.c index deb47d9d6f..e5b9e3cac9 100644 --- a/plugins/elements/gstqueue2.c +++ b/plugins/elements/gstqueue2.c @@ -1161,7 +1161,11 @@ gst_queue2_create_read (GstQueue2 * queue, guint64 offset, guint length, GstFlowReturn ret = GST_FLOW_OK; /* allocate the output buffer of the requested size */ - buf = gst_buffer_new_allocate (NULL, length, NULL); + if (*buffer == NULL) + buf = gst_buffer_new_allocate (NULL, length, NULL); + else + buf = *buffer; + gst_buffer_map (buf, &info, GST_MAP_WRITE); data = info.data; @@ -1289,21 +1293,24 @@ hit_eos: { GST_DEBUG_OBJECT (queue, "EOS hit and we don't have any requested data"); gst_buffer_unmap (buf, &info); - gst_buffer_unref (buf); + if (*buffer == NULL) + gst_buffer_unref (buf); return GST_FLOW_EOS; } out_flushing: { GST_DEBUG_OBJECT (queue, "we are flushing"); gst_buffer_unmap (buf, &info); - gst_buffer_unref (buf); + if (*buffer == NULL) + gst_buffer_unref (buf); return GST_FLOW_FLUSHING; } read_error: { GST_DEBUG_OBJECT (queue, "we have a read error"); gst_buffer_unmap (buf, &info); - gst_buffer_unref (buf); + if (*buffer == NULL) + gst_buffer_unref (buf); return ret; } } @@ -1322,7 +1329,7 @@ gst_queue2_read_item_from_file (GstQueue2 * queue) queue->starting_segment = NULL; } else { GstFlowReturn ret; - GstBuffer *buffer; + GstBuffer *buffer = NULL; guint64 reading_pos; reading_pos = queue->current->reading_pos;