videoflip: fix possible crash when setting the video-direction while running

A classic case of not enough locking.

One interesting thing with this is the interaction between the
rotation value and caps negotiation.  i.e. the width/height of the caps
can be swapped depending on the video-direction property.  We can't lock
the entirety of the caps negotiation for obvious reasons so we need to
do something else.  This takes the approach of trying to use a single
rotation value throughout the entirety of the negotiation and then
subsequent output frame in a kind of latching sequence.

Fixes: https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/issues/792
Part-of: <https://gitlab.freedesktop.org/gstreamer/gst-plugins-good/-/merge_requests/847>

4 files changed, 211 insertions, 51 deletions

diff --git a/docs/gst_plugins_cache.json b/docs/gst_plugins_cache.json
index 01f68978b..c1267af7a 100644
--- a/docs/gst_plugins_cache.json
+++ b/docs/gst_plugins_cache.json
@@ -25542,7 +25542,7 @@
                         "construct-only": false,
                         "controllable": true,
                         "default": "none (0)",
-                        "mutable": "null",
+                        "mutable": "playing",
                         "readable": true,
                         "type": "GstVideoFlipMethod",
                         "writable": true
diff --git a/gst/videofilter/gstvideoflip.c b/gst/videofilter/gstvideoflip.c
index 6ee5125bf..f20e4edaf 100644
--- a/gst/videofilter/gstvideoflip.c
+++ b/gst/videofilter/gstvideoflip.c
@@ -135,6 +135,26 @@ gst_video_flip_transform_caps (GstBaseTransform * trans,
 
   ret = gst_caps_copy (caps);
 
+  GST_OBJECT_LOCK (videoflip);
+
+  if (videoflip->change_configuring_method) {
+    GEnumValue *configuring_method_enum, *method_enum;
+    GEnumClass *enum_class =
+        g_type_class_ref (GST_TYPE_VIDEO_ORIENTATION_METHOD);
+
+    configuring_method_enum =
+        g_enum_get_value (enum_class, videoflip->configuring_method);
+    method_enum = g_enum_get_value (enum_class, videoflip->proposed_method);
+    GST_LOG_OBJECT (videoflip,
+        "Changing configuring method from %s to proposed %s",
+        configuring_method_enum ? configuring_method_enum->value_nick : "(nil)",
+        method_enum ? method_enum->value_nick : "(nil)");
+    g_type_class_unref (enum_class);
+
+    videoflip->configuring_method = videoflip->proposed_method;
+  }
+  videoflip->change_configuring_method = FALSE;
+
   for (i = 0; i < gst_caps_get_size (ret); i++) {
     GstStructure *structure = gst_caps_get_structure (ret, i);
     gint par_n, par_d;
@@ -142,7 +162,7 @@ gst_video_flip_transform_caps (GstBaseTransform * trans,
     if (gst_structure_get_int (structure, "width", &width) &&
         gst_structure_get_int (structure, "height", &height)) {
 
-      switch (videoflip->active_method) {
+      switch (videoflip->configuring_method) {
         case GST_VIDEO_ORIENTATION_90R:
         case GST_VIDEO_ORIENTATION_90L:
         case GST_VIDEO_ORIENTATION_UL_LR:
@@ -177,6 +197,7 @@ gst_video_flip_transform_caps (GstBaseTransform * trans,
       }
     }
   }
+  GST_OBJECT_UNLOCK (videoflip);
 
   GST_DEBUG_OBJECT (videoflip, "transformed %" GST_PTR_FORMAT " to %"
       GST_PTR_FORMAT, caps, ret);
@@ -435,7 +456,7 @@ gst_video_flip_planar_yuv (GstVideoFlip * videoflip, GstVideoFrame * dest,
       }
       break;
     case GST_VIDEO_ORIENTATION_IDENTITY:
-      g_assert_not_reached ();
+      gst_video_frame_copy (dest, src);
       break;
     default:
       g_assert_not_reached ();
@@ -634,7 +655,7 @@ gst_video_flip_semi_planar_yuv (GstVideoFlip * videoflip, GstVideoFrame * dest,
       }
       break;
     case GST_VIDEO_ORIENTATION_IDENTITY:
-      g_assert_not_reached ();
+      gst_video_frame_copy (dest, src);
       break;
     default:
       g_assert_not_reached ();
@@ -735,7 +756,7 @@ gst_video_flip_packed_simple (GstVideoFlip * videoflip, GstVideoFrame * dest,
       }
       break;
     case GST_VIDEO_ORIENTATION_IDENTITY:
-      g_assert_not_reached ();
+      gst_video_frame_copy (dest, src);
       break;
     default:
       g_assert_not_reached ();
@@ -951,7 +972,7 @@ gst_video_flip_y422 (GstVideoFlip * videoflip, GstVideoFrame * dest,
       }
       break;
     case GST_VIDEO_ORIENTATION_IDENTITY:
-      g_assert_not_reached ();
+      gst_video_frame_copy (dest, src);
       break;
     default:
       g_assert_not_reached ();
@@ -959,13 +980,51 @@ gst_video_flip_y422 (GstVideoFlip * videoflip, GstVideoFrame * dest,
   }
 }
 
+static void
+gst_video_flip_configure_process (GstVideoFlip * vf)
+{
+  switch (vf->v_format) {
+    case GST_VIDEO_FORMAT_I420:
+    case GST_VIDEO_FORMAT_YV12:
+    case GST_VIDEO_FORMAT_Y444:
+      vf->process = gst_video_flip_planar_yuv;
+      break;
+    case GST_VIDEO_FORMAT_YUY2:
+    case GST_VIDEO_FORMAT_UYVY:
+    case GST_VIDEO_FORMAT_YVYU:
+      vf->process = gst_video_flip_y422;
+      break;
+    case GST_VIDEO_FORMAT_AYUV:
+    case GST_VIDEO_FORMAT_ARGB:
+    case GST_VIDEO_FORMAT_ABGR:
+    case GST_VIDEO_FORMAT_RGBA:
+    case GST_VIDEO_FORMAT_BGRA:
+    case GST_VIDEO_FORMAT_xRGB:
+    case GST_VIDEO_FORMAT_xBGR:
+    case GST_VIDEO_FORMAT_RGBx:
+    case GST_VIDEO_FORMAT_BGRx:
+    case GST_VIDEO_FORMAT_RGB:
+    case GST_VIDEO_FORMAT_BGR:
+    case GST_VIDEO_FORMAT_GRAY8:
+    case GST_VIDEO_FORMAT_GRAY16_BE:
+    case GST_VIDEO_FORMAT_GRAY16_LE:
+      vf->process = gst_video_flip_packed_simple;
+      break;
+    case GST_VIDEO_FORMAT_NV12:
+    case GST_VIDEO_FORMAT_NV21:
+      vf->process = gst_video_flip_semi_planar_yuv;
+      break;
+    default:
+      break;
+  }
+}
 
 static gboolean
 gst_video_flip_set_info (GstVideoFilter * vfilter, GstCaps * incaps,
     GstVideoInfo * in_info, GstCaps * outcaps, GstVideoInfo * out_info)
 {
   GstVideoFlip *vf = GST_VIDEO_FLIP (vfilter);
-  gboolean ret = FALSE;
+  gboolean ret = FALSE, need_reconfigure = FALSE;
 
   vf->process = NULL;
 
@@ -973,7 +1032,8 @@ gst_video_flip_set_info (GstVideoFilter * vfilter, GstCaps * incaps,
     goto invalid_caps;
 
   /* Check that they are correct */
-  switch (vf->active_method) {
+  GST_OBJECT_LOCK (vf);
+  switch (vf->configuring_method) {
     case GST_VIDEO_ORIENTATION_90R:
     case GST_VIDEO_ORIENTATION_90L:
     case GST_VIDEO_ORIENTATION_UL_LR:
@@ -987,8 +1047,6 @@ gst_video_flip_set_info (GstVideoFilter * vfilter, GstCaps * incaps,
       }
       break;
     case GST_VIDEO_ORIENTATION_IDENTITY:
-
-      break;
     case GST_VIDEO_ORIENTATION_180:
     case GST_VIDEO_ORIENTATION_HORIZ:
     case GST_VIDEO_ORIENTATION_VERT:
@@ -1007,42 +1065,32 @@ gst_video_flip_set_info (GstVideoFilter * vfilter, GstCaps * incaps,
 
   ret = TRUE;
 
-  switch (GST_VIDEO_INFO_FORMAT (in_info)) {
-    case GST_VIDEO_FORMAT_I420:
-    case GST_VIDEO_FORMAT_YV12:
-    case GST_VIDEO_FORMAT_Y444:
-      vf->process = gst_video_flip_planar_yuv;
-      break;
-    case GST_VIDEO_FORMAT_YUY2:
-    case GST_VIDEO_FORMAT_UYVY:
-    case GST_VIDEO_FORMAT_YVYU:
-      vf->process = gst_video_flip_y422;
-      break;
-    case GST_VIDEO_FORMAT_AYUV:
-    case GST_VIDEO_FORMAT_ARGB:
-    case GST_VIDEO_FORMAT_ABGR:
-    case GST_VIDEO_FORMAT_RGBA:
-    case GST_VIDEO_FORMAT_BGRA:
-    case GST_VIDEO_FORMAT_xRGB:
-    case GST_VIDEO_FORMAT_xBGR:
-    case GST_VIDEO_FORMAT_RGBx:
-    case GST_VIDEO_FORMAT_BGRx:
-    case GST_VIDEO_FORMAT_RGB:
-    case GST_VIDEO_FORMAT_BGR:
-    case GST_VIDEO_FORMAT_GRAY8:
-    case GST_VIDEO_FORMAT_GRAY16_BE:
-    case GST_VIDEO_FORMAT_GRAY16_LE:
-      vf->process = gst_video_flip_packed_simple;
-      break;
-    case GST_VIDEO_FORMAT_NV12:
-    case GST_VIDEO_FORMAT_NV21:
-      vf->process = gst_video_flip_semi_planar_yuv;
-      break;
-    default:
-      break;
+  {
+    GEnumValue *active_method_enum, *method_enum;
+    GEnumClass *enum_class =
+        g_type_class_ref (GST_TYPE_VIDEO_ORIENTATION_METHOD);
+
+    active_method_enum = g_enum_get_value (enum_class, vf->active_method);
+    method_enum = g_enum_get_value (enum_class, vf->configuring_method);
+    GST_LOG_OBJECT (vf, "Changing active method from %s to configuring %s",
+        active_method_enum ? active_method_enum->value_nick : "(nil)",
+        method_enum ? method_enum->value_nick : "(nil)");
+    g_type_class_unref (enum_class);
   }
+  vf->active_method = vf->configuring_method;
+  vf->change_configuring_method = TRUE;
+  if (vf->active_method != vf->proposed_method)
+    need_reconfigure = TRUE;
+
+  vf->v_format = GST_VIDEO_INFO_FORMAT (in_info);
+  gst_video_flip_configure_process (vf);
 
 beach:
+  GST_OBJECT_UNLOCK (vf);
+  if (need_reconfigure) {
+    gst_base_transform_reconfigure_src (GST_BASE_TRANSFORM (vf));
+  }
+
   return ret && (vf->process != NULL);
 
 invalid_caps:
@@ -1075,7 +1123,7 @@ gst_video_flip_set_method (GstVideoFlip * videoflip,
   else
     method = videoflip->method;
 
-  if (method != videoflip->active_method) {
+  if (method != videoflip->proposed_method) {
     GEnumValue *active_method_enum, *method_enum;
     GstBaseTransform *btrans = GST_BASE_TRANSFORM (videoflip);
     GEnumClass *enum_class =
@@ -1084,12 +1132,12 @@ gst_video_flip_set_method (GstVideoFlip * videoflip,
     active_method_enum =
         g_enum_get_value (enum_class, videoflip->active_method);
     method_enum = g_enum_get_value (enum_class, method);
-    GST_DEBUG_OBJECT (videoflip, "Changing method from %s to %s",
+    GST_LOG_OBJECT (videoflip, "Changing method from %s to %s",
         active_method_enum ? active_method_enum->value_nick : "(nil)",
         method_enum ? method_enum->value_nick : "(nil)");
     g_type_class_unref (enum_class);
 
-    videoflip->active_method = method;
+    videoflip->proposed_method = method;
 
     GST_OBJECT_UNLOCK (videoflip);
 
@@ -1123,26 +1171,46 @@ gst_video_flip_transform_frame (GstVideoFilter * vfilter,
     GstVideoFrame * in_frame, GstVideoFrame * out_frame)
 {
   GEnumClass *enum_class;
+  GstVideoOrientationMethod active, proposed;
   GEnumValue *active_method_enum;
   GstVideoFlip *videoflip = GST_VIDEO_FLIP (vfilter);
 
+  GST_OBJECT_LOCK (videoflip);
   if (G_UNLIKELY (videoflip->process == NULL))
     goto not_negotiated;
 
+  if (videoflip->configuring_method != videoflip->active_method) {
+    videoflip->active_method = videoflip->configuring_method;
+    gst_video_flip_configure_process (videoflip);
+  }
+
   enum_class = g_type_class_ref (GST_TYPE_VIDEO_ORIENTATION_METHOD);
   active_method_enum = g_enum_get_value (enum_class, videoflip->active_method);
-  GST_LOG_OBJECT (videoflip, "videoflip: flipping (%s)",
-      active_method_enum ? active_method_enum->value_nick : "(nil)");
+  GST_LOG_OBJECT (videoflip,
+      "videoflip: flipping (%s), input %ux%u output %ux%u",
+      active_method_enum ? active_method_enum->value_nick : "(nil)",
+      GST_VIDEO_FRAME_WIDTH (in_frame), GST_VIDEO_FRAME_HEIGHT (in_frame),
+      GST_VIDEO_FRAME_WIDTH (out_frame), GST_VIDEO_FRAME_HEIGHT (out_frame));
   g_type_class_unref (enum_class);
 
-  GST_OBJECT_LOCK (videoflip);
   videoflip->process (videoflip, out_frame, in_frame);
+
+  proposed = videoflip->proposed_method;
+  active = videoflip->active_method;
+  videoflip->change_configuring_method = TRUE;
   GST_OBJECT_UNLOCK (videoflip);
 
+  if (proposed != active) {
+    gst_base_transform_set_passthrough (GST_BASE_TRANSFORM (videoflip),
+        proposed == GST_VIDEO_ORIENTATION_IDENTITY);
+    gst_base_transform_reconfigure_src (GST_BASE_TRANSFORM (videoflip));
+  }
+
   return GST_FLOW_OK;
 
 not_negotiated:
   {
+    GST_OBJECT_UNLOCK (videoflip);
     GST_ERROR_OBJECT (videoflip, "Not negotiated yet");
     return GST_FLOW_NOT_NEGOTIATED;
   }
@@ -1168,6 +1236,7 @@ gst_video_flip_src_event (GstBaseTransform * trans, GstEvent * event)
       if (gst_structure_get_double (structure, "pointer_x", &x) &&
           gst_structure_get_double (structure, "pointer_y", &y)) {
         GST_DEBUG_OBJECT (vf, "converting %fx%f", x, y);
+        GST_OBJECT_LOCK (vf);
         switch (vf->active_method) {
           case GST_VIDEO_ORIENTATION_90R:
             new_x = y;
@@ -1202,6 +1271,7 @@ gst_video_flip_src_event (GstBaseTransform * trans, GstEvent * event)
             new_y = y;
             break;
         }
+        GST_OBJECT_UNLOCK (vf);
         GST_DEBUG_OBJECT (vf, "to %fx%f", new_x, new_y);
         gst_structure_set (structure, "pointer_x", G_TYPE_DOUBLE, new_x,
             "pointer_y", G_TYPE_DOUBLE, new_y, NULL);
@@ -1301,6 +1371,7 @@ gst_video_flip_class_init (GstVideoFlipClass * klass)
   GstElementClass *gstelement_class = (GstElementClass *) klass;
   GstBaseTransformClass *trans_class = (GstBaseTransformClass *) klass;
   GstVideoFilterClass *vfilter_class = (GstVideoFilterClass *) klass;
+  GParamSpec *pspec;
 
   GST_DEBUG_CATEGORY_INIT (video_flip_debug, "videoflip", 0, "videoflip");
 
@@ -1311,10 +1382,14 @@ gst_video_flip_class_init (GstVideoFlipClass * klass)
       g_param_spec_enum ("method", "method",
           "method (deprecated, use video-direction instead)",
           GST_TYPE_VIDEO_FLIP_METHOD, PROP_METHOD_DEFAULT,
-          GST_PARAM_CONTROLLABLE | G_PARAM_READWRITE | G_PARAM_CONSTRUCT |
-          G_PARAM_STATIC_STRINGS));
+          GST_PARAM_CONTROLLABLE | GST_PARAM_MUTABLE_PLAYING |
+          G_PARAM_READWRITE | G_PARAM_CONSTRUCT | G_PARAM_STATIC_STRINGS));
   g_object_class_override_property (gobject_class, PROP_VIDEO_DIRECTION,
       "video-direction");
+  /* override the overriden property's flags to include the mutable in playing
+   * flag */
+  pspec = g_object_class_find_property (gobject_class, "video-direction");
+  pspec->flags |= GST_PARAM_MUTABLE_PLAYING;
 
   gst_element_class_set_static_metadata (gstelement_class, "Video flipper",
       "Filter/Effect/Video",
@@ -1345,4 +1420,6 @@ gst_video_flip_init (GstVideoFlip * videoflip)
   /* AUTO is not valid for active method, this is just to ensure we setup the
    * method in gst_video_flip_set_method() */
   videoflip->active_method = GST_VIDEO_ORIENTATION_AUTO;
+  videoflip->proposed_method = GST_VIDEO_ORIENTATION_IDENTITY;
+  videoflip->configuring_method = GST_VIDEO_ORIENTATION_IDENTITY;
 }
diff --git a/gst/videofilter/gstvideoflip.h b/gst/videofilter/gstvideoflip.h
index 0fca8e93e..a82bbc479 100644
--- a/gst/videofilter/gstvideoflip.h
+++ b/gst/videofilter/gstvideoflip.h
@@ -75,8 +75,13 @@ struct _GstVideoFlip {
   GstVideoFilter videofilter;
 
   /* < private > */
+  GstVideoFormat v_format;
+
   GstVideoOrientationMethod method;
   GstVideoOrientationMethod tag_method;
+  GstVideoOrientationMethod proposed_method;
+  gboolean change_configuring_method;
+  GstVideoOrientationMethod configuring_method;
   GstVideoOrientationMethod active_method;
   void (*process) (GstVideoFlip *videoflip, GstVideoFrame *dest, const GstVideoFrame *src);
 };
diff --git a/tests/check/elements/videoflip.c b/tests/check/elements/videoflip.c
index 4a8d579de..2a9f8cba6 100644
--- a/tests/check/elements/videoflip.c
+++ b/tests/check/elements/videoflip.c
@@ -144,6 +144,82 @@ GST_START_TEST (test_change_method)
 
 GST_END_TEST;
 
+GST_START_TEST (test_change_method_twice_same_caps_different_method)
+{
+  GstHarness *flip = gst_harness_new ("videoflip");
+  GstVideoInfo in_info, out_info;
+  GstCaps *in_caps, *out_caps;
+  GstEvent *e;
+  GstBuffer *input, *output, *buf;
+  GstMapInfo in_map_info, out_map_info;
+
+  gst_video_info_set_format (&in_info, GST_VIDEO_FORMAT_RGBA, 4, 9);
+  in_caps = gst_video_info_to_caps (&in_info);
+
+  gst_harness_set_src_caps (flip, in_caps);
+
+  e = gst_harness_pull_event (flip);
+  fail_unless_equals_int (GST_EVENT_TYPE (e), GST_EVENT_STREAM_START);
+  gst_event_unref (e);
+  e = gst_harness_pull_event (flip);
+  fail_unless_equals_int (GST_EVENT_TYPE (e), GST_EVENT_CAPS);
+  gst_event_parse_caps (e, &out_caps);
+  fail_unless (gst_video_info_from_caps (&out_info, out_caps));
+  fail_unless_equals_int (GST_VIDEO_INFO_WIDTH (&in_info),
+      GST_VIDEO_INFO_WIDTH (&out_info));
+  fail_unless_equals_int (GST_VIDEO_INFO_HEIGHT (&in_info),
+      GST_VIDEO_INFO_HEIGHT (&out_info));
+  gst_event_unref (e);
+
+  e = gst_harness_pull_event (flip);
+  fail_unless_equals_int (GST_EVENT_TYPE (e), GST_EVENT_SEGMENT);
+  gst_event_unref (e);
+
+  buf = create_test_video_buffer_rgba8 (&in_info);
+  buf = gst_harness_push_and_pull (flip, buf);
+  fail_unless (buf != NULL);
+  gst_buffer_unref (buf);
+
+  g_object_set (flip->element, "video-direction", 1 /* 90r */ , NULL);
+  g_object_set (flip->element, "video-direction", 2 /* 180 */ , NULL);
+
+  input = create_test_video_buffer_rgba8 (&in_info);
+  fail_unless_equals_int (gst_harness_push (flip, gst_buffer_ref (input)),
+      GST_FLOW_OK);
+  /* caps will not change and basetransform won't send updated ones so we
+   * can't check for them */
+  output = gst_harness_pull (flip);
+  fail_unless (output != NULL);
+
+  fail_unless (gst_buffer_map (input, &in_map_info, GST_MAP_READ));
+  fail_unless (gst_buffer_map (output, &out_map_info, GST_MAP_READ));
+
+  {
+    gsize top_right = (GST_VIDEO_INFO_WIDTH (&in_info) - 1) * 4;
+    gsize bottom_left =
+        (GST_VIDEO_INFO_HEIGHT (&out_info) -
+        1) * GST_VIDEO_INFO_PLANE_STRIDE (&out_info, 0);
+
+    fail_unless_equals_int (in_map_info.data[top_right + 0],
+        out_map_info.data[bottom_left + 0]);
+    fail_unless_equals_int (in_map_info.data[top_right + 1],
+        out_map_info.data[bottom_left + 1]);
+    fail_unless_equals_int (in_map_info.data[top_right + 2],
+        out_map_info.data[bottom_left + 2]);
+    fail_unless_equals_int (in_map_info.data[top_right + 3],
+        out_map_info.data[bottom_left + 3]);
+  }
+
+  gst_buffer_unmap (input, &in_map_info);
+  gst_buffer_unmap (output, &out_map_info);
+
+  gst_buffer_unref (input);
+  gst_buffer_unref (output);
+
+  gst_harness_teardown (flip);
+}
+
+GST_END_TEST;
 GST_START_TEST (test_stress_change_method)
 {
   GstHarness *flip = gst_harness_new ("videoflip");
@@ -200,6 +276,8 @@ videoflip_suite (void)
   suite_add_tcase (s, tc_chain);
   tcase_add_test (tc_chain, test_passthrough);
   tcase_add_test (tc_chain, test_change_method);
+  tcase_add_test (tc_chain,
+      test_change_method_twice_same_caps_different_method);
   tcase_add_test (tc_chain, test_stress_change_method);
 
   return s;