From e7ebd3e9e2deabb673474f9c6ea19e66618cf531 Mon Sep 17 00:00:00 2001
From: Stefan Kieszkowski <85728496+stefankiesz@users.noreply.github.com>
Date: Wed, 6 Dec 2023 16:34:38 -0800
Subject: [PATCH] Fix KVSSINK State Transition Deadlock (#1113)

* Change parent class stat change position

* Add failure check

Co-authored-by: Niyati Maheshwari <niyatim23@gmail.com>

* Testing on mac os 13

* Revert ci to mac os 12

* Add comments explaining state transition guidelines

---------

Co-authored-by: Niyati Maheshwari <niyatim23@gmail.com>
---
 src/gstreamer/gstkvssink.cpp | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/src/gstreamer/gstkvssink.cpp b/src/gstreamer/gstkvssink.cpp
index f5dfefcc..c1d3b77a 100644
--- a/src/gstreamer/gstkvssink.cpp
+++ b/src/gstreamer/gstkvssink.cpp
@@ -1572,12 +1572,25 @@ init_track_data(GstKvsSink *kvssink) {
 
 static GstStateChangeReturn
 gst_kvs_sink_change_state(GstElement *element, GstStateChange transition) {
+    /*
+        The below state transition cases are separated into two switch blocks:
+        one for upward (NULL->READY->PAUSED->PLAYING) transitions and one for
+        downward (PLAYING->PAUSED->READY->NULL) transitions. It is necessary to transition
+        an element's parent class state after any of the element's upward transitions but
+        before any downward transitions. As per GStreamer documentation,
+        "this is necessary in order to safely handle concurrent access by multiple threads."
+        
+        https://gstreamer.freedesktop.org/documentation/plugin-development/basics/states.
+        html?gi-language=c#:~:text=Note%20that%20upwards,destroying%20allocated%20resources.
+    */
+    
     GstStateChangeReturn ret = GST_STATE_CHANGE_SUCCESS;
     GstKvsSink *kvssink = GST_KVS_SINK (element);
     auto data = kvssink->data;
     string err_msg = "";
     ostringstream oss;
 
+    // Upward transitions
     switch (transition) {
         case GST_STATE_CHANGE_NULL_TO_READY:
             if(kvssink->log_config_path != NULL) {
@@ -1609,6 +1622,18 @@ gst_kvs_sink_change_state(GstElement *element, GstStateChange transition) {
         case GST_STATE_CHANGE_READY_TO_PAUSED:
             gst_collect_pads_start (kvssink->collect);
             break;
+        default:
+            break;
+    }
+
+    // Parent class transition
+    ret = GST_ELEMENT_CLASS (parent_class)->change_state(element, transition);
+    if (ret == GST_STATE_CHANGE_FAILURE) {
+        goto CleanUp;
+    }
+
+    // Downward transitions
+    switch (transition) {
         case GST_STATE_CHANGE_PAUSED_TO_READY:
             LOG_INFO("Stopping kvssink for " << kvssink->stream_name);
             gst_collect_pads_stop (kvssink->collect);
@@ -1631,8 +1656,6 @@ gst_kvs_sink_change_state(GstElement *element, GstStateChange transition) {
             break;
     }
 
-    ret = GST_ELEMENT_CLASS (parent_class)->change_state(element, transition);
-
 CleanUp:
 
     if (ret != GST_STATE_CHANGE_SUCCESS) {