From 2a60a9f66fe3754ca1799eba5f08de28744572ec Mon Sep 17 00:00:00 2001 From: George Kiagiadakis Date: Tue, 4 Apr 2017 11:25:43 +0300 Subject: [PATCH] videoaggregator: delay using new caps from a sink pad until the next buffer in the queue is taken When caps changes while streaming, the new caps was getting processed immediately in videoaggregator, but the next buffer in the queue that corresponds to this new caps was not necessarily being used immediately, which resulted sometimes in using an old buffer with new caps. Of course there used to be a separate buffer_vinfo for mapping the buffer with its own caps, but in compositor the GstVideoConverter was still using wrong info and resulted in invalid reads and corrupt output. This approach here is more safe. We delay using the new caps until we actually select the next buffer in the queue for use. This way we also eliminate the need for buffer_vinfo, since the pad->info is always in sync with the format of the selected buffer. https://bugzilla.gnome.org/show_bug.cgi?id=780682 --- gst-libs/gst/video/gstvideoaggregator.c | 64 +++++++++++++++++++++++++++------ gst/compositor/compositor.c | 21 +++++------ 2 files changed, 62 insertions(+), 23 deletions(-) diff --git a/gst-libs/gst/video/gstvideoaggregator.c b/gst-libs/gst/video/gstvideoaggregator.c index 1616b27..8310fd2 100644 --- a/gst-libs/gst/video/gstvideoaggregator.c +++ b/gst-libs/gst/video/gstvideoaggregator.c @@ -75,6 +75,8 @@ struct _GstVideoAggregatorPadPrivate GstClockTime start_time; GstClockTime end_time; + + GstVideoInfo pending_vinfo; }; @@ -244,8 +246,7 @@ gst_video_aggregator_pad_prepare_frame (GstVideoAggregatorPad * pad, frame = g_slice_new0 (GstVideoFrame); - if (!gst_video_frame_map (frame, &pad->buffer_vinfo, pad->buffer, - GST_MAP_READ)) { + if (!gst_video_frame_map (frame, &pad->info, pad->buffer, GST_MAP_READ)) { GST_WARNING_OBJECT (vagg, "Could not map input buffer"); return FALSE; } @@ -886,8 +887,20 @@ gst_video_aggregator_pad_sink_setcaps (GstPad * pad, GstObject * parent, } } - vaggpad->info = info; - gst_pad_mark_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg)); + if (!vaggpad->info.finfo || + GST_VIDEO_INFO_FORMAT (&vaggpad->info) == GST_VIDEO_FORMAT_UNKNOWN) { + /* no video info was already set, so this is the first time + * that this pad is getting configured; configure immediately to avoid + * problems with the initial negotiation */ + vaggpad->info = info; + gst_pad_mark_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg)); + } else { + /* this pad already had caps but received new ones; keep the new caps + * pending until we pick the next buffer from the queue, otherwise we + * might use an old buffer with the new caps and crash */ + vaggpad->priv->pending_vinfo = info; + GST_DEBUG_OBJECT (pad, "delaying caps change"); + } ret = TRUE; GST_VIDEO_AGGREGATOR_UNLOCK (vagg); @@ -1098,6 +1111,7 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, GList *l; gboolean eos = TRUE; gboolean need_more_data = FALSE; + gboolean need_reconfigure = FALSE; /* get a set of buffers into pad->buffer that are within output_start_running_time * and output_end_running_time taking into account finished and unresponsive pads */ @@ -1108,7 +1122,6 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, GstSegment segment; GstAggregatorPad *bpad; GstBuffer *buf; - GstVideoInfo *vinfo; gboolean is_eos; bpad = GST_AGGREGATOR_PAD (pad); @@ -1131,8 +1144,6 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, return GST_FLOW_ERROR; } - vinfo = &pad->info; - /* FIXME: Make all this work with negative rates */ end_time = GST_BUFFER_DURATION (buf); @@ -1155,7 +1166,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, GST_DEBUG_OBJECT (pad, "buffer duration is -1, start_time < " "output_start_running_time. Discarding old buffer"); gst_buffer_replace (&pad->buffer, buf); - pad->buffer_vinfo = *vinfo; + if (pad->priv->pending_vinfo.finfo) { + pad->info = pad->priv->pending_vinfo; + need_reconfigure = TRUE; + pad->priv->pending_vinfo.finfo = NULL; + } gst_buffer_unref (buf); gst_aggregator_pad_drop_buffer (bpad); need_more_data = TRUE; @@ -1164,7 +1179,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, gst_buffer_unref (buf); buf = gst_aggregator_pad_steal_buffer (bpad); gst_buffer_replace (&pad->buffer, buf); - pad->buffer_vinfo = *vinfo; + if (pad->priv->pending_vinfo.finfo) { + pad->info = pad->priv->pending_vinfo; + need_reconfigure = TRUE; + pad->priv->pending_vinfo.finfo = NULL; + } /* FIXME: Set start_time and end_time to something here? */ gst_buffer_unref (buf); GST_DEBUG_OBJECT (pad, "buffer duration is -1"); @@ -1225,7 +1244,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, "Taking new buffer with start time %" GST_TIME_FORMAT, GST_TIME_ARGS (start_time)); gst_buffer_replace (&pad->buffer, buf); - pad->buffer_vinfo = *vinfo; + if (pad->priv->pending_vinfo.finfo) { + pad->info = pad->priv->pending_vinfo; + need_reconfigure = TRUE; + pad->priv->pending_vinfo.finfo = NULL; + } pad->priv->start_time = start_time; pad->priv->end_time = end_time; @@ -1239,7 +1262,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, eos = FALSE; } else { gst_buffer_replace (&pad->buffer, buf); - pad->buffer_vinfo = *vinfo; + if (pad->priv->pending_vinfo.finfo) { + pad->info = pad->priv->pending_vinfo; + need_reconfigure = TRUE; + pad->priv->pending_vinfo.finfo = NULL; + } pad->priv->start_time = start_time; pad->priv->end_time = end_time; GST_DEBUG_OBJECT (pad, @@ -1278,6 +1305,9 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg, } GST_OBJECT_UNLOCK (vagg); + if (need_reconfigure) + gst_pad_mark_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg)); + if (need_more_data) return GST_FLOW_NEEDS_DATA; if (eos) @@ -1515,6 +1545,7 @@ gst_video_aggregator_aggregate (GstAggregator * agg, gboolean timeout) GST_VIDEO_AGGREGATOR_LOCK (vagg); +restart: flow_ret = gst_video_aggregator_check_reconfigure (vagg, timeout); if (flow_ret != GST_FLOW_OK) { if (flow_ret == GST_FLOW_NEEDS_DATA) @@ -1572,6 +1603,17 @@ gst_video_aggregator_aggregate (GstAggregator * agg, gboolean timeout) goto unlock_and_return; } + /* It is possible that gst_video_aggregator_fill_queues() marked the pad + * for reconfiguration. In this case we have to reconfigure before continuing + * because we have picked a new buffer with different caps than before from + * one one of the sink pads and continuing here may lead to a crash. + * https://bugzilla.gnome.org/show_bug.cgi?id=780682 + */ + if (gst_pad_needs_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg))) { + GST_DEBUG_OBJECT (vagg, "Need reconfigure"); + goto restart; + } + GST_DEBUG_OBJECT (vagg, "Producing buffer for %" GST_TIME_FORMAT " to %" GST_TIME_FORMAT ", running time start %" GST_TIME_FORMAT ", running time end %" diff --git a/gst/compositor/compositor.c b/gst/compositor/compositor.c index fab75c6..de3bf22 100644 --- a/gst/compositor/compositor.c +++ b/gst/compositor/compositor.c @@ -381,7 +381,7 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad, /* There's three types of width/height here: * 1. GST_VIDEO_FRAME_WIDTH/HEIGHT: - * The frame width/height (same as pad->buffer_vinfo.height/width; + * The frame width/height (same as pad->info.height/width; * see gst_video_frame_map()) * 2. cpad->width/height: * The optional pad property for scaling the frame (if zero, the video is @@ -409,21 +409,20 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad, gst_video_converter_free (cpad->convert); cpad->convert = NULL; - colorimetry = - gst_video_colorimetry_to_string (&pad->buffer_vinfo.colorimetry); - chroma = gst_video_chroma_to_string (pad->buffer_vinfo.chroma_site); + colorimetry = gst_video_colorimetry_to_string (&pad->info.colorimetry); + chroma = gst_video_chroma_to_string (pad->info.chroma_site); wanted_colorimetry = gst_video_colorimetry_to_string (&cpad->conversion_info.colorimetry); wanted_chroma = gst_video_chroma_to_string (cpad->conversion_info.chroma_site); - if (GST_VIDEO_INFO_FORMAT (&pad->buffer_vinfo) != + if (GST_VIDEO_INFO_FORMAT (&pad->info) != GST_VIDEO_INFO_FORMAT (&cpad->conversion_info) || g_strcmp0 (colorimetry, wanted_colorimetry) || g_strcmp0 (chroma, wanted_chroma) - || width != GST_VIDEO_INFO_WIDTH (&pad->buffer_vinfo) - || height != GST_VIDEO_INFO_HEIGHT (&pad->buffer_vinfo)) { + || width != GST_VIDEO_INFO_WIDTH (&pad->info) + || height != GST_VIDEO_INFO_HEIGHT (&pad->info)) { GstVideoInfo tmp_info; gst_video_info_set_format (&tmp_info, cpad->conversion_info.finfo->format, @@ -438,11 +437,10 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad, tmp_info.interlace_mode = cpad->conversion_info.interlace_mode; GST_DEBUG_OBJECT (pad, "This pad will be converted from %d to %d", - GST_VIDEO_INFO_FORMAT (&pad->buffer_vinfo), + GST_VIDEO_INFO_FORMAT (&pad->info), GST_VIDEO_INFO_FORMAT (&tmp_info)); - cpad->convert = - gst_video_converter_new (&pad->buffer_vinfo, &tmp_info, NULL); + cpad->convert = gst_video_converter_new (&pad->info, &tmp_info, NULL); cpad->conversion_info = tmp_info; if (!cpad->convert) { @@ -522,8 +520,7 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad, frame = g_slice_new0 (GstVideoFrame); - if (!gst_video_frame_map (frame, &pad->buffer_vinfo, pad->buffer, - GST_MAP_READ)) { + if (!gst_video_frame_map (frame, &pad->info, pad->buffer, GST_MAP_READ)) { GST_WARNING_OBJECT (vagg, "Could not map input buffer"); return FALSE; } -- 2.7.4