llama-17925 : TextToSpeech crash, followed by black screen#6454
llama-17925 : TextToSpeech crash, followed by black screen#6454kkarup694 wants to merge 1 commit into
Conversation
Reason for change: fix for parallel pipeline set state Test Procedure: Mentioned in ticket Risks: Low version: minor Signed-off-by: kumar <kumar_Karuppiahyogarajah@comcast.com>
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a crash and black screen issue in TextToSpeech by adding mutex protection around GStreamer pipeline state changes. The changes introduce a new mutex (m_pipelineMutex) and a wrapper method (setPipelineState) that protects calls to gst_element_set_state.
Changes:
- Added
m_pipelineMutexmember variable andsetPipelineStatemethod declaration in TTSSpeaker.h - Implemented
setPipelineStatemethod with mutex protection - Replaced all direct
gst_element_set_statecalls withsetPipelineState
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| TextToSpeech/impl/TTSSpeaker.h | Added mutex member and setPipelineState method declaration |
| TextToSpeech/impl/TTSSpeaker.cpp | Implemented setPipelineState with mutex, replaced all gst_element_set_state calls |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if(m_pipeline) | ||
| { | ||
| std::lock_guard<std::mutex> lock(m_pipelineMutex); | ||
| gst_element_set_state(m_pipeline, state); | ||
| } | ||
| else | ||
| { |
There was a problem hiding this comment.
The setPipelineState function has a time-of-check-time-of-use (TOCTOU) race condition. The m_pipeline pointer is checked for NULL before acquiring the mutex lock, but m_pipeline could be set to NULL by another thread (e.g., in destroyPipeline at line 975 or createPipeline at line 910) between the check and the lock acquisition. The NULL check should be performed after acquiring the mutex lock to ensure thread safety.
| if(m_pipeline) | |
| { | |
| std::lock_guard<std::mutex> lock(m_pipelineMutex); | |
| gst_element_set_state(m_pipeline, state); | |
| } | |
| else | |
| { | |
| std::lock_guard<std::mutex> lock(m_pipelineMutex); | |
| if (m_pipeline) { | |
| gst_element_set_state(m_pipeline, state); | |
| } else { |
| if(m_pipeline) { | ||
| gst_element_set_state(m_pipeline, GST_STATE_NULL); | ||
| setPipelineState(GST_STATE_NULL); | ||
| waitForStatus(GST_STATE_NULL, 1*1000); | ||
| g_source_remove(m_busWatch); | ||
| gst_object_unref(m_pipeline); |
There was a problem hiding this comment.
The gst_object_unref operation on m_pipeline is not protected by the m_pipelineMutex. This creates a race condition where another thread could be calling setPipelineState and accessing m_pipeline while it's being unreferenced and destroyed here. The unref operation and the subsequent NULL assignment should be protected by the m_pipelineMutex to ensure thread safety.
| } | ||
| else | ||
| { | ||
| TTSLOG_ERROR("Pipeline error occured"); |
There was a problem hiding this comment.
Spelling error: "occured" should be "occurred" (with two 'r's).
Reason for change: fix for parallel pipeline set state
Test Procedure: Mentioned in ticket
Risks: Low
version: minor