From 359b2a2a2b56953fc133cd08182341571d31c243 Mon Sep 17 00:00:00 2001 From: rekha kandhavelan Date: Tue, 27 Oct 2026 12:27:55 +0530 Subject: [PATCH] Reason for change: This fix addresses a CC sinkbin teardown issue where clearing a stale sinkbin reference could bypass the Rialto teardown sequence and leave its server-side session active. The teardown paths now request GST_STATE_NULL, confirm the transition with bounded state polling, remove the sinkbin from the pipeline, and then clear retained references. Test Procedure: updated in ticket Risks: medium Priority: P1 Signed-off-by: Rekha Kandhavelan rekha_kandhavelan@comcast.com --- InterfacePlayerPriv.h | 7 ++ InterfacePlayerRDK.cpp | 106 +++++++++++++++++- .../InterfacePlayerFunctionTests.cpp | 10 +- 3 files changed, 120 insertions(+), 3 deletions(-) diff --git a/InterfacePlayerPriv.h b/InterfacePlayerPriv.h index b2fc8f43..81f9911b 100755 --- a/InterfacePlayerPriv.h +++ b/InterfacePlayerPriv.h @@ -252,6 +252,13 @@ struct GstPlayerPriv GstHandlerControl syncControl; GstHandlerControl callbackControl; + + /* Guards ConfigurePipeline()/CreatePipeline() against running on this shared pipeline + * object while a TearDownStream() from a prior tune is still releasing resources. */ + std::mutex mTeardownMutex; + std::condition_variable mTeardownCV; + bool mTeardownInProgress = false; + bool filterAudioDemuxBuffers; /**< flag to filter audio demux buffers */ double seekPosition; /**< the position to seek the pipeline to in seconds */ GstPlayerPriv(); diff --git a/InterfacePlayerRDK.cpp b/InterfacePlayerRDK.cpp index 919a8073..2aafa88b 100644 --- a/InterfacePlayerRDK.cpp +++ b/InterfacePlayerRDK.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include "PlayerLogManager.h" #include "GstUtils.h" #include @@ -336,6 +337,16 @@ void InterfacePlayerRDK::ConfigurePipeline(int format, int audioFormat, int subF bool bESChangeStatus, bool setReadyAfterPipelineCreation, bool isSubEnable, int32_t trackId, gint rate, const char *pipelineName, int PipelinePriority, bool FirstFrameFlag, std::string manifestUrl, bool enableLiveLatency) { + { + /* Don't let a new tune start reconfiguring this shared pipeline object while a prior + * tune's TearDownStream() (on another thread) is still confirming resource release. */ + std::unique_lock teardownLock(interfacePlayerPriv->gstPrivateContext->mTeardownMutex); + if (!interfacePlayerPriv->gstPrivateContext->mTeardownCV.wait_for(teardownLock, std::chrono::milliseconds(3000), + [this]{ return !interfacePlayerPriv->gstPrivateContext->mTeardownInProgress; })) + { + MW_LOG_ERR("InterfacePlayerRDK::ConfigurePipeline: timed out waiting for prior TearDownStream to complete"); + } + } StreamCodecInfo streamInfo; streamInfo.video.mCodecFormat = static_cast(format); streamInfo.audio.mCodecFormat = static_cast(audioFormat); @@ -1419,6 +1430,10 @@ static GstStateChangeReturn SetStateWithWarnings(GstElement *element, GstState t void InterfacePlayerRDK::TearDownStream(int type) { + { + std::lock_guard teardownLock(interfacePlayerPriv->gstPrivateContext->mTeardownMutex); + interfacePlayerPriv->gstPrivateContext->mTeardownInProgress = true; + } tearDownCb(true, type); gst_media_stream* stream = &interfacePlayerPriv->gstPrivateContext->stream[type]; RemoveProbe(type); @@ -1441,6 +1456,27 @@ void InterfacePlayerRDK::TearDownStream(int type) { MW_LOG_ERR("InterfacePlayerRDK::TearDownStream: Failed to set NULL state for sinkbin"); } + else + { + /* A SUCCESS/ASYNC return here only reflects local GStreamer bookkeeping; for + * server-backed sinks (e.g. Rialto) the decoder/session release happens over IPC + * and is not guaranteed to be complete when this call returns. Always confirm + * NULL is actually reached (bounded) before a new decoder tries to acquire the + * same resource, instead of only doing so when the return is ASYNC. */ + GstState current, pending; + gint retryCnt = GST_ELEMENT_GET_STATE_RETRY_CNT_MAX; + do + { + (void)gst_element_get_state(GST_ELEMENT(stream->sinkbin), ¤t, &pending, 100 * GST_MSECOND); + } + while ((current != GST_STATE_NULL) && (--retryCnt > 0)); + if (current != GST_STATE_NULL) + { + MW_LOG_ERR("InterfacePlayerRDK::TearDownStream: sinkbin stopped abruptly, did not reach NULL (current=%s) - forcing synchronous reset before next decoder configuration", gst_element_state_get_name(current)); + gst_element_set_state(GST_ELEMENT(stream->sinkbin), GST_STATE_NULL); + } + } + if (!gst_bin_remove(GST_BIN(interfacePlayerPriv->gstPrivateContext->pipeline), GST_ELEMENT(stream->sinkbin))) /* Removes the sinkbin element from the pipeline */ { MW_LOG_ERR("InterfacePlayerRDK::TearDownStream: Unable to remove sinkbin from pipeline"); @@ -1485,7 +1521,37 @@ void InterfacePlayerRDK::TearDownStream(int type) pthread_mutex_lock(&stream->sourceLock); if (stream->sinkbin) { - MW_LOG_WARN("InterfacePlayerRDK::TearDownStream: CC sinkbin still assigned, clearing"); + /* This sinkbin was skipped by the main teardown block above (format was already + * INVALID, or usingClosedCaptionsControl was already cleared by another media + * type's teardown in this same call sequence) - clearing the reference without + * first requesting GST_STATE_NULL never runs the Rialto sink's teardown handler, + * so the underlying RialtoServer session for it is leaked. Tear it down properly + * before dropping the reference. */ + MW_LOG_WARN("InterfacePlayerRDK::TearDownStream: CC sinkbin still assigned, tearing down before clearing"); + + if (interfacePlayerPriv->gstPrivateContext->pipeline) + { + GstStateChangeReturn nullRc = SetStateWithWarnings(GST_ELEMENT(stream->sinkbin), GST_STATE_NULL); + if (GST_STATE_CHANGE_FAILURE != nullRc) + { + GstState current, pending; + gint retryCnt = GST_ELEMENT_GET_STATE_RETRY_CNT_MAX; + do + { + gst_element_get_state(GST_ELEMENT(stream->sinkbin), ¤t, &pending, 100 * GST_MSECOND); + } + while ((current != GST_STATE_NULL) && (--retryCnt > 0)); + if (current != GST_STATE_NULL) + { + MW_LOG_ERR("InterfacePlayerRDK::TearDownStream: CC sinkbin did not reach NULL (current=%s), forcing", gst_element_state_get_name(current)); + gst_element_set_state(GST_ELEMENT(stream->sinkbin), GST_STATE_NULL); + } + } + if (!gst_bin_remove(GST_BIN(interfacePlayerPriv->gstPrivateContext->pipeline), GST_ELEMENT(stream->sinkbin))) + { + MW_LOG_INFO("InterfacePlayerRDK::TearDownStream: Unable to remove CC sinkbin from pipeline"); + } + } g_clear_object(&stream->sinkbin); } if (stream->source) @@ -1496,6 +1562,12 @@ void InterfacePlayerRDK::TearDownStream(int type) pthread_mutex_unlock(&stream->sourceLock); } tearDownCb(false, mediaType); + { + std::lock_guard teardownLock(interfacePlayerPriv->gstPrivateContext->mTeardownMutex); + interfacePlayerPriv->gstPrivateContext->mTeardownInProgress = false; + } + interfacePlayerPriv->gstPrivateContext->mTeardownCV.notify_all(); + MW_LOG_MIL("InterfacePlayerRDK::TearDownStream: exit mediaType = %d", mediaType); } @@ -2296,7 +2368,37 @@ void InterfacePlayerRDK::SetupClosedCaptionControlStream() // Check elements are not already assigned if (stream->sinkbin) { - MW_LOG_ERR("Sinkbin already assigned"); + /* A stale sinkbin here means a previous TearDownStream()/teardown path left this + * element attached to the pipeline without requesting GST_STATE_NULL first - clearing + * the reference alone would leak the underlying RialtoServer session and leave a + * dangling child element in the pipeline bin. Tear it down properly before clearing. */ + MW_LOG_WARN("Sinkbin already assigned, tearing down before clearing"); + if (privatePlayer->gstPrivateContext->pipeline) + { + GstStateChangeReturn nullRc = SetStateWithWarnings(GST_ELEMENT(stream->sinkbin), GST_STATE_NULL); + + if (GST_STATE_CHANGE_FAILURE != nullRc) + { + GstState current, pending; + gint retryCnt = GST_ELEMENT_GET_STATE_RETRY_CNT_MAX; + do + { + gst_element_get_state(GST_ELEMENT(stream->sinkbin), ¤t, &pending, 100 * GST_MSECOND); + } + while ((current != GST_STATE_NULL) && (--retryCnt > 0)); + + if (current != GST_STATE_NULL) + { + MW_LOG_ERR("Sinkbin did not reach NULL (current=%s), forcing", gst_element_state_get_name(current)); + gst_element_set_state(GST_ELEMENT(stream->sinkbin), GST_STATE_NULL); + } + } + if (!gst_bin_remove(GST_BIN(privatePlayer->gstPrivateContext->pipeline), GST_ELEMENT(stream->sinkbin))) + { + MW_LOG_INFO("Unable to remove stale sinkbin from pipeline"); + } + } + g_clear_object(&stream->sinkbin); } if (privatePlayer->gstPrivateContext->subtitle_sink) diff --git a/test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp b/test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp index db362abc..1937faa1 100644 --- a/test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp +++ b/test/utests/tests/InterfacePlayerTests/InterfacePlayerFunctionTests.cpp @@ -370,9 +370,17 @@ TEST_F(InterfacePlayerTests, TearDownStreamTest_successvideo) EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_sinkbin, _,_,0)) - .WillOnce(Return(GST_STATE_CHANGE_SUCCESS)); + .WillOnce(DoAll( + SetArgPointee<1>(GST_STATE_PLAYING), + SetArgPointee<2>(GST_STATE_NULL), + Return(GST_STATE_CHANGE_SUCCESS))); EXPECT_CALL(*g_mockGStreamer, gst_element_set_state(&gst_sinkbin, GST_STATE_NULL)) .WillOnce(Return(GST_STATE_CHANGE_SUCCESS)); + EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_sinkbin, _, _, 100 * GST_MSECOND)) + .WillOnce(DoAll( + SetArgPointee<1>(GST_STATE_NULL), + SetArgPointee<2>(GST_STATE_VOID_PENDING), + Return(GST_STATE_CHANGE_SUCCESS))); mInterfaceGstPlayer->TearDownStream(eGST_MEDIATYPE_VIDEO);