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

3 files changed, 62 insertions, 30 deletions

diff --git a/gst-libs/gst/video/gstvideoaggregator.c b/gst-libs/gst/video/gstvideoaggregator.c
index 1616b2724..8310fd217 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-libs/gst/video/gstvideoaggregatorpad.h b/gst-libs/gst/video/gstvideoaggregatorpad.h
index 3360b5356..b2cc3e77e 100644
--- a/gst-libs/gst/video/gstvideoaggregatorpad.h
+++ b/gst-libs/gst/video/gstvideoaggregatorpad.h
@@ -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;
 
diff --git a/gst/compositor/compositor.c b/gst/compositor/compositor.c
index fab75c639..de3bf22d6 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;
   }