mirrors/gstreamer
mirrors/gstreamer
1
1
Fork 0
mirror of https://gitlab.freedesktop.org/gstreamer/gstreamer.git synced 2024-10-02 16:52:42 +00:00
Code Issues 1 Projects Releases Wiki Activity

videoaggregator: delay using new caps from a sink pad until the next buffer in the queue is taken

Browse source 
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
This commit is contained in:
George Kiagiadakis 2017-04-04 11:25:43 +03:00 committed by Olivier Crête
parent d2335bce9b
commit 39d2526c34
3 changed files with 62 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

64
gst-libs/gst/video/gstvideoaggregator.c
View file

@ -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 %"

7
gst-libs/gst/video/gstvideoaggregatorpad.h
View file

@ -61,13 +61,6 @@ struct _GstVideoAggregatorPad
GstVideoInfo info;
GstBuffer *buffer;
/* The caps on the pad may not match the buffer above because of two reasons:
* 1) When caps change, the info above will get updated, but the buffer might
* not since it might be pending on the GstAggregatorPad
* 2) We might reject the new buffer in fill_queues() and reuse a previous
* buffer which has older GstVideoInfo
* Hence, we need to maintain a GstVideoInfo for mapping buffers separately */
GstVideoInfo buffer_vinfo;
GstVideoFrame *aggregated_frame;

21
gst/compositor/compositor.c
View file

@ -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;
}