Merge branch 'bugfix/fix_gmf_task_running_flag_no_safe' into 'main'

Fixed a thread safety issue with the gmf_task running flag

See merge request adf/multimedia/esp-gmf!68

Author: jason-mao

elements/test_apps/main/CMakeLists.txt

@@ -1,5 +1,5 @@
 idf_component_register(SRC_DIRS "." "elements"
                        INCLUDE_DIRS "." "elements"
-                       REQUIRES unity esp_codec_dev system_common test_utils
+                       REQUIRES unity esp_codec_dev system_common test_utils esp_gdbstub
                        EMBED_FILES "hi_lexin.pcm"
                        WHOLE_ARCHIVE)

elements/test_apps/main/elements/gmf_audio_play_el_test.c

@@ -528,7 +528,7 @@ TEST_CASE("Audio Play, two in pipe use same task, [file->dec]->rb->[resample+IIS
     ESP_GMF_MEM_SHOW(TAG);
 }
 
-TEST_CASE("Audio Play, One Pipe, [HTTP->dec->resample->IIS]", "[ESP_GMF_POOL][ignore][leaks=10000]")
+TEST_CASE("Audio Play, One Pipe, [HTTP->dec->resample->IIS]", "[ESP_GMF_POOL][leaks=10000]")
 {
     esp_log_level_set("*", ESP_LOG_INFO);
     esp_log_level_set("ESP_GMF_PIPELINE", ESP_LOG_DEBUG);

gmf_core/CHANGELOG.md

@@ -16,6 +16,7 @@
 ### Bug Fixes
 
 - Fixed pause timeout caused by missing sync event when pause producer entered an error state
+- Fixed a thread safety issue with the gmf_task `running` flag
 
 ## v0.6.1
 

gmf_core/include/esp_gmf_element.h

@@ -97,26 +97,26 @@ typedef struct {
  * @brief  Structure representing a GMF element
  */
 typedef struct esp_gmf_element {
-    esp_gmf_obj_t                   base;           /*!< Base object */
-    esp_gmf_element_ops_t           ops;            /*!< Operations */
-    uint8_t                         job_mask;       /*!< Job mask */
+    esp_gmf_obj_t                base;           /*!< Base object */
+    esp_gmf_element_ops_t        ops;            /*!< Operations */
+    uint8_t                      job_mask;       /*!< Job mask */
 
-    esp_gmf_port_t                 *in;             /*!< Input port */
-    esp_gmf_element_port_attr_t     in_attr;        /*!< Input port attributes */
+    esp_gmf_port_t              *in;             /*!< Input port */
+    esp_gmf_element_port_attr_t  in_attr;        /*!< Input port attributes */
 
-    esp_gmf_port_t                 *out;            /*!< Output port */
-    esp_gmf_element_port_attr_t     out_attr;       /*!< Output port attributes */
+    esp_gmf_port_t              *out;            /*!< Output port */
+    esp_gmf_element_port_attr_t  out_attr;       /*!< Output port attributes */
 
     /* Properties */
-    esp_gmf_event_state_t           init_state;     /*!< Initial state */
-    esp_gmf_event_state_t           cur_state;      /*!< Current state */
-    esp_gmf_event_cb                event_func;     /*!< Event function */
-    esp_gmf_method_t               *method;         /*!< It can access the data members and member functions of the objects */
-    esp_gmf_cap_t                  *caps;           /*!< Element capabilities */
+    esp_gmf_event_state_t        init_state;     /*!< Initial state */
+    esp_gmf_event_state_t        cur_state;      /*!< Current state */
+    esp_gmf_event_cb             event_func;     /*!< Event function */
+    esp_gmf_method_t            *method;         /*!< It can access the data members and member functions of the objects */
+    esp_gmf_cap_t               *caps;           /*!< Element capabilities */
 
     /* Protect */
-    void                           *ctx;            /*!< User Context */
-    uint8_t                         dependency : 1; /*!< Indicates if the element depends on other information to open */
+    void                        *ctx;            /*!< User Context */
+    uint8_t                      dependency : 1; /*!< Indicates if the element depends on other information to open */
 } esp_gmf_element_t;
 
 /**

gmf_core/include/esp_gmf_pipeline.h

@@ -35,22 +35,22 @@ typedef esp_gmf_err_t (*esp_gmf_pipeline_prev_act)(void *handle);  /*!<  */
  * @brief  Structure representing a pipeline in GMF
  */
 typedef struct esp_gmf_pipeline {
-    esp_gmf_element_handle_t    head_el;        /*!< Handle of the first element in the pipeline */
-    esp_gmf_element_handle_t    last_el;        /*!< Handle of the last element in the pipeline */
-    esp_gmf_io_handle_t         in;             /*!< Handle of the input I/O port */
-    esp_gmf_io_handle_t         out;            /*!< Handle of the output I/O port */
-    esp_gmf_event_item_t       *evt_conveyor;   /*!< Event conveyor list */
-    esp_gmf_event_cb            evt_acceptor;   /*!< Event acceptor callback function */
-    esp_gmf_event_cb            user_cb;        /*!< User callback function */
-    void                       *user_ctx;       /*!< User context */
-    esp_gmf_event_state_t       state;          /*!< Current state of the pipeline */
-    esp_gmf_task_handle_t       thread;         /*!< Handle of the task associated with the pipeline */
-    esp_gmf_pipeline_prev_act   prev_run;       /*!< A pointer to the previous run callback */
-    esp_gmf_pipeline_prev_act   prev_stop;      /*!< A pointer to the previous stop callback */
-    void                       *prev_run_ctx;   /*!< The previous run context */
-    void                       *prev_stop_ctx;  /*!< The previous stop context */
-    uint8_t                     prev_state;     /*!< The previous action state */
-    void                       *lock;           /*!< Lock for thread synchronization */
+    esp_gmf_element_handle_t   head_el;        /*!< Handle of the first element in the pipeline */
+    esp_gmf_element_handle_t   last_el;        /*!< Handle of the last element in the pipeline */
+    esp_gmf_io_handle_t        in;             /*!< Handle of the input I/O port */
+    esp_gmf_io_handle_t        out;            /*!< Handle of the output I/O port */
+    esp_gmf_event_item_t      *evt_conveyor;   /*!< Event conveyor list */
+    esp_gmf_event_cb           evt_acceptor;   /*!< Event acceptor callback function */
+    esp_gmf_event_cb           user_cb;        /*!< User callback function */
+    void                      *user_ctx;       /*!< User context */
+    esp_gmf_event_state_t      state;          /*!< Current state of the pipeline */
+    esp_gmf_task_handle_t      thread;         /*!< Handle of the task associated with the pipeline */
+    esp_gmf_pipeline_prev_act  prev_run;       /*!< A pointer to the previous run callback */
+    esp_gmf_pipeline_prev_act  prev_stop;      /*!< A pointer to the previous stop callback */
+    void                      *prev_run_ctx;   /*!< The previous run context */
+    void                      *prev_stop_ctx;  /*!< The previous stop context */
+    uint8_t                    prev_state;     /*!< The previous action state */
+    void                      *lock;           /*!< Lock for thread synchronization */
 } esp_gmf_pipeline_t;
 
 /**

gmf_core/include/esp_gmf_task.h

@@ -43,31 +43,32 @@ typedef struct esp_gmf_task_config {
  *         Represents a GMF task, including its properties, configuration, and internal state.
  */
 typedef struct _esp_gmf_task {
-    struct esp_gmf_obj_     base;           /*!< Base object for GMF tasks */
-    esp_gmf_job_t          *working;        /*!< Currently executing job in the task */
-    esp_gmf_job_stack_t    *start_stack;    /*!< Stack for the start job */
+    struct esp_gmf_obj_    base;           /*!< Base object for GMF tasks */
+    esp_gmf_job_t         *working;        /*!< Currently executing job in the task */
+    esp_gmf_job_stack_t   *start_stack;    /*!< Stack for the start job */
 
     /* Properties */
-    esp_gmf_event_cb        event_func;     /*!< Callback function for task events */
-    esp_gmf_event_state_t   state;          /*!< Current state of the task */
+    esp_gmf_event_cb       event_func;     /*!< Callback function for task events */
+    esp_gmf_event_state_t  state;          /*!< Current state of the task */
 
     /* Protect */
-    esp_gmf_task_config_t   thread;         /*!< Configuration settings for the task */
-    void                   *ctx;            /*!< Context associated with the task */
+    esp_gmf_task_config_t  thread;         /*!< Configuration settings for the task */
+    void                  *ctx;            /*!< Context associated with the task */
 
     /* Private */
-    void                   *oal_thread;     /*!< Handle to the thread */
-    void                   *lock;           /*!< Mutex lock for task synchronization */
-    void                   *event_group;    /*!< Event group for wait events */
-    void                   *block_sem;      /*!< Semaphore for blocking tasks */
-    void                   *wait_sem;       /*!< Semaphore for task waiting */
-    int                     api_sync_time;  /*!< Timeout for synchronization */
-
-    uint8_t                 _task_run : 1;  /*!< Internal flag for task execution */
-    uint8_t                 _running  : 1;  /*!< Internal flag for task running state */
-    uint8_t                 _pause    : 1;  /*!< Internal flag for task pause state */
-    uint8_t                 _stop     : 1;  /*!< Internal flag for task stop state */
-    uint8_t                 _destroy  : 1;  /*!< Internal flag for task destruction */
+    void                  *oal_thread;     /*!< Handle to the thread */
+    void                  *lock;           /*!< Mutex lock for task synchronization */
+    void                  *event_group;    /*!< Event group for wait events */
+    void                  *block_sem;      /*!< Semaphore for blocking tasks */
+    void                  *wait_sem;       /*!< Semaphore for task waiting */
+    int                    api_sync_time;  /*!< Timeout for synchronization */
+
+    uint8_t                _task_run : 1;  /*!< Internal flag for task execution */
+    uint8_t                _running  : 1;  /*!< Internal flag for task running state */
+    uint8_t                _run      : 1;  /*!< Internal flag for task run API flag */
+    uint8_t                _pause    : 1;  /*!< Internal flag for task pause API flag */
+    uint8_t                _stop     : 1;  /*!< Internal flag for task stop API flag */
+    uint8_t                _destroy  : 1;  /*!< Internal flag for task destruction API flag */
 } esp_gmf_task_t;
 
 /**

gmf_core/src/esp_gmf_task.c

@@ -271,6 +271,10 @@ static void esp_gmf_thread_fun(void *pv)
                 ESP_LOGD(TAG, "Thread will be destroyed, [%s,%p]", OBJ_GET_TAG((esp_gmf_obj_handle_t)tsk), tsk);
                 goto ESP_GMF_THREAD_EXIT;
             }
+            if (tsk->_run == 1) {
+                tsk->_running = 1;
+                tsk->_run = 0;
+            }
         }
         int ret = esp_gmf_task_event_state_change_and_notify(tsk, ESP_GMF_EVENT_STATE_RUNNING);
         GMF_TASK_SET_STATE_BITS(tsk->event_group, GMF_TASK_RUN_BIT);
@@ -452,7 +456,7 @@ esp_gmf_err_t esp_gmf_task_run(esp_gmf_task_handle_t handle)
         ESP_LOGW(TAG, "No task for run, %s, [%s,%p]", esp_gmf_event_get_state_str(tsk->state), OBJ_GET_TAG((esp_gmf_obj_handle_t)tsk), tsk);
         return ESP_GMF_ERR_INVALID_STATE;
     }
-    tsk->_running = 1;
+    tsk->_run = 1;
     xSemaphoreGive(tsk->block_sem);
     // Wait for run finished
     if (GMF_TASK_WAIT_FOR_STATE_BITS(tsk->event_group, GMF_TASK_RUN_BIT, tsk->api_sync_time) == false) {