mirror of
https://gitlab.freedesktop.org/gstreamer/gstreamer.git
synced 2024-12-25 09:40:37 +00:00
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
This commit is contained in:
parent
cbafb022aa
commit
2a60a9f66f
2 changed files with 62 additions and 23 deletions
|
@ -75,6 +75,8 @@ struct _GstVideoAggregatorPadPrivate
|
||||||
|
|
||||||
GstClockTime start_time;
|
GstClockTime start_time;
|
||||||
GstClockTime end_time;
|
GstClockTime end_time;
|
||||||
|
|
||||||
|
GstVideoInfo pending_vinfo;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
|
||||||
|
@ -244,8 +246,7 @@ gst_video_aggregator_pad_prepare_frame (GstVideoAggregatorPad * pad,
|
||||||
|
|
||||||
frame = g_slice_new0 (GstVideoFrame);
|
frame = g_slice_new0 (GstVideoFrame);
|
||||||
|
|
||||||
if (!gst_video_frame_map (frame, &pad->buffer_vinfo, pad->buffer,
|
if (!gst_video_frame_map (frame, &pad->info, pad->buffer, GST_MAP_READ)) {
|
||||||
GST_MAP_READ)) {
|
|
||||||
GST_WARNING_OBJECT (vagg, "Could not map input buffer");
|
GST_WARNING_OBJECT (vagg, "Could not map input buffer");
|
||||||
return FALSE;
|
return FALSE;
|
||||||
}
|
}
|
||||||
|
@ -886,8 +887,20 @@ gst_video_aggregator_pad_sink_setcaps (GstPad * pad, GstObject * parent,
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
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;
|
vaggpad->info = info;
|
||||||
gst_pad_mark_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg));
|
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;
|
ret = TRUE;
|
||||||
|
|
||||||
GST_VIDEO_AGGREGATOR_UNLOCK (vagg);
|
GST_VIDEO_AGGREGATOR_UNLOCK (vagg);
|
||||||
|
@ -1098,6 +1111,7 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg,
|
||||||
GList *l;
|
GList *l;
|
||||||
gboolean eos = TRUE;
|
gboolean eos = TRUE;
|
||||||
gboolean need_more_data = FALSE;
|
gboolean need_more_data = FALSE;
|
||||||
|
gboolean need_reconfigure = FALSE;
|
||||||
|
|
||||||
/* get a set of buffers into pad->buffer that are within output_start_running_time
|
/* 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 */
|
* 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;
|
GstSegment segment;
|
||||||
GstAggregatorPad *bpad;
|
GstAggregatorPad *bpad;
|
||||||
GstBuffer *buf;
|
GstBuffer *buf;
|
||||||
GstVideoInfo *vinfo;
|
|
||||||
gboolean is_eos;
|
gboolean is_eos;
|
||||||
|
|
||||||
bpad = GST_AGGREGATOR_PAD (pad);
|
bpad = GST_AGGREGATOR_PAD (pad);
|
||||||
|
@ -1131,8 +1144,6 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg,
|
||||||
return GST_FLOW_ERROR;
|
return GST_FLOW_ERROR;
|
||||||
}
|
}
|
||||||
|
|
||||||
vinfo = &pad->info;
|
|
||||||
|
|
||||||
/* FIXME: Make all this work with negative rates */
|
/* FIXME: Make all this work with negative rates */
|
||||||
end_time = GST_BUFFER_DURATION (buf);
|
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 < "
|
GST_DEBUG_OBJECT (pad, "buffer duration is -1, start_time < "
|
||||||
"output_start_running_time. Discarding old buffer");
|
"output_start_running_time. Discarding old buffer");
|
||||||
gst_buffer_replace (&pad->buffer, buf);
|
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_buffer_unref (buf);
|
||||||
gst_aggregator_pad_drop_buffer (bpad);
|
gst_aggregator_pad_drop_buffer (bpad);
|
||||||
need_more_data = TRUE;
|
need_more_data = TRUE;
|
||||||
|
@ -1164,7 +1179,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg,
|
||||||
gst_buffer_unref (buf);
|
gst_buffer_unref (buf);
|
||||||
buf = gst_aggregator_pad_steal_buffer (bpad);
|
buf = gst_aggregator_pad_steal_buffer (bpad);
|
||||||
gst_buffer_replace (&pad->buffer, buf);
|
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? */
|
/* FIXME: Set start_time and end_time to something here? */
|
||||||
gst_buffer_unref (buf);
|
gst_buffer_unref (buf);
|
||||||
GST_DEBUG_OBJECT (pad, "buffer duration is -1");
|
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,
|
"Taking new buffer with start time %" GST_TIME_FORMAT,
|
||||||
GST_TIME_ARGS (start_time));
|
GST_TIME_ARGS (start_time));
|
||||||
gst_buffer_replace (&pad->buffer, buf);
|
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->start_time = start_time;
|
||||||
pad->priv->end_time = end_time;
|
pad->priv->end_time = end_time;
|
||||||
|
|
||||||
|
@ -1239,7 +1262,11 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg,
|
||||||
eos = FALSE;
|
eos = FALSE;
|
||||||
} else {
|
} else {
|
||||||
gst_buffer_replace (&pad->buffer, buf);
|
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->start_time = start_time;
|
||||||
pad->priv->end_time = end_time;
|
pad->priv->end_time = end_time;
|
||||||
GST_DEBUG_OBJECT (pad,
|
GST_DEBUG_OBJECT (pad,
|
||||||
|
@ -1278,6 +1305,9 @@ gst_video_aggregator_fill_queues (GstVideoAggregator * vagg,
|
||||||
}
|
}
|
||||||
GST_OBJECT_UNLOCK (vagg);
|
GST_OBJECT_UNLOCK (vagg);
|
||||||
|
|
||||||
|
if (need_reconfigure)
|
||||||
|
gst_pad_mark_reconfigure (GST_AGGREGATOR_SRC_PAD (vagg));
|
||||||
|
|
||||||
if (need_more_data)
|
if (need_more_data)
|
||||||
return GST_FLOW_NEEDS_DATA;
|
return GST_FLOW_NEEDS_DATA;
|
||||||
if (eos)
|
if (eos)
|
||||||
|
@ -1515,6 +1545,7 @@ gst_video_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
|
||||||
|
|
||||||
GST_VIDEO_AGGREGATOR_LOCK (vagg);
|
GST_VIDEO_AGGREGATOR_LOCK (vagg);
|
||||||
|
|
||||||
|
restart:
|
||||||
flow_ret = gst_video_aggregator_check_reconfigure (vagg, timeout);
|
flow_ret = gst_video_aggregator_check_reconfigure (vagg, timeout);
|
||||||
if (flow_ret != GST_FLOW_OK) {
|
if (flow_ret != GST_FLOW_OK) {
|
||||||
if (flow_ret == GST_FLOW_NEEDS_DATA)
|
if (flow_ret == GST_FLOW_NEEDS_DATA)
|
||||||
|
@ -1572,6 +1603,17 @@ gst_video_aggregator_aggregate (GstAggregator * agg, gboolean timeout)
|
||||||
goto unlock_and_return;
|
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,
|
GST_DEBUG_OBJECT (vagg,
|
||||||
"Producing buffer for %" GST_TIME_FORMAT " to %" GST_TIME_FORMAT
|
"Producing buffer for %" GST_TIME_FORMAT " to %" GST_TIME_FORMAT
|
||||||
", running time start %" GST_TIME_FORMAT ", running time end %"
|
", running time start %" GST_TIME_FORMAT ", running time end %"
|
||||||
|
|
|
@ -381,7 +381,7 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad,
|
||||||
|
|
||||||
/* There's three types of width/height here:
|
/* There's three types of width/height here:
|
||||||
* 1. GST_VIDEO_FRAME_WIDTH/HEIGHT:
|
* 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())
|
* see gst_video_frame_map())
|
||||||
* 2. cpad->width/height:
|
* 2. cpad->width/height:
|
||||||
* The optional pad property for scaling the frame (if zero, the video is
|
* 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);
|
gst_video_converter_free (cpad->convert);
|
||||||
cpad->convert = NULL;
|
cpad->convert = NULL;
|
||||||
|
|
||||||
colorimetry =
|
colorimetry = gst_video_colorimetry_to_string (&pad->info.colorimetry);
|
||||||
gst_video_colorimetry_to_string (&pad->buffer_vinfo.colorimetry);
|
chroma = gst_video_chroma_to_string (pad->info.chroma_site);
|
||||||
chroma = gst_video_chroma_to_string (pad->buffer_vinfo.chroma_site);
|
|
||||||
|
|
||||||
wanted_colorimetry =
|
wanted_colorimetry =
|
||||||
gst_video_colorimetry_to_string (&cpad->conversion_info.colorimetry);
|
gst_video_colorimetry_to_string (&cpad->conversion_info.colorimetry);
|
||||||
wanted_chroma =
|
wanted_chroma =
|
||||||
gst_video_chroma_to_string (cpad->conversion_info.chroma_site);
|
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)
|
GST_VIDEO_INFO_FORMAT (&cpad->conversion_info)
|
||||||
|| g_strcmp0 (colorimetry, wanted_colorimetry)
|
|| g_strcmp0 (colorimetry, wanted_colorimetry)
|
||||||
|| g_strcmp0 (chroma, wanted_chroma)
|
|| g_strcmp0 (chroma, wanted_chroma)
|
||||||
|| width != GST_VIDEO_INFO_WIDTH (&pad->buffer_vinfo)
|
|| width != GST_VIDEO_INFO_WIDTH (&pad->info)
|
||||||
|| height != GST_VIDEO_INFO_HEIGHT (&pad->buffer_vinfo)) {
|
|| height != GST_VIDEO_INFO_HEIGHT (&pad->info)) {
|
||||||
GstVideoInfo tmp_info;
|
GstVideoInfo tmp_info;
|
||||||
|
|
||||||
gst_video_info_set_format (&tmp_info, cpad->conversion_info.finfo->format,
|
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;
|
tmp_info.interlace_mode = cpad->conversion_info.interlace_mode;
|
||||||
|
|
||||||
GST_DEBUG_OBJECT (pad, "This pad will be converted from %d to %d",
|
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));
|
GST_VIDEO_INFO_FORMAT (&tmp_info));
|
||||||
|
|
||||||
cpad->convert =
|
cpad->convert = gst_video_converter_new (&pad->info, &tmp_info, NULL);
|
||||||
gst_video_converter_new (&pad->buffer_vinfo, &tmp_info, NULL);
|
|
||||||
cpad->conversion_info = tmp_info;
|
cpad->conversion_info = tmp_info;
|
||||||
|
|
||||||
if (!cpad->convert) {
|
if (!cpad->convert) {
|
||||||
|
@ -522,8 +520,7 @@ gst_compositor_pad_prepare_frame (GstVideoAggregatorPad * pad,
|
||||||
|
|
||||||
frame = g_slice_new0 (GstVideoFrame);
|
frame = g_slice_new0 (GstVideoFrame);
|
||||||
|
|
||||||
if (!gst_video_frame_map (frame, &pad->buffer_vinfo, pad->buffer,
|
if (!gst_video_frame_map (frame, &pad->info, pad->buffer, GST_MAP_READ)) {
|
||||||
GST_MAP_READ)) {
|
|
||||||
GST_WARNING_OBJECT (vagg, "Could not map input buffer");
|
GST_WARNING_OBJECT (vagg, "Could not map input buffer");
|
||||||
return FALSE;
|
return FALSE;
|
||||||
}
|
}
|
||||||
|
|
Loading…
Reference in a new issue