videoaggregator: delay using new caps from a sink pad until the next buffer in the...

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

diff --git a/gst-libs/gst/video/gstvideoaggregator.c b/gst-libs/gst/video/gstvideoaggregator.c
index 1616b27..8310fd2 100644 (file)
--- 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 (file)
--- 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;
   }