Disable synchronous play in RialtoServer as it caused some race conditions (#501)

Summary: Disable synchronous play in RialtoServer as it caused some race
conditions
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-18286

Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>

Author: Koky2701

media/server/gstplayer/include/GstGenericPlayer.h

@@ -481,11 +481,6 @@ class GstGenericPlayer : public IGstGenericPlayer, public IGstGenericPlayerPriva
      * @brief The object used to check flushing state for all sources
      */
     std::unique_ptr<IFlushWatcher> m_flushWatcher;
-
-    /**
-     * @brief The ongoing state change operations counter
-     */
-    std::atomic<uint32_t> m_ongoingStateChangesNumber{0};
 };
 
 } // namespace firebolt::rialto::server

media/server/gstplayer/source/GstGenericPlayer.cpp

@@ -1659,30 +1659,15 @@ void GstGenericPlayer::cancelUnderflow(firebolt::rialto::MediaSourceType mediaSo
 
 void GstGenericPlayer::play(bool &async)
 {
-    if (0 == m_ongoingStateChangesNumber)
-    {
-        // Operation called on main thread, because PAUSED->PLAYING change is synchronous and needs to be done fast.
-        //
-        // m_context.pipeline can be used, because it's modified only in GstGenericPlayer
-        // constructor and destructor. GstGenericPlayer is created/destructed on main thread, so we won't have a crash here.
-        ++m_ongoingStateChangesNumber;
-        async = (changePipelineState(GST_STATE_PLAYING) == GST_STATE_CHANGE_ASYNC);
-        RIALTO_SERVER_LOG_MIL("State change to PLAYING requested");
-    }
-    else
+    async = true;
+    if (m_workerThread)
     {
-        ++m_ongoingStateChangesNumber;
-        async = true;
-        if (m_workerThread)
-        {
-            m_workerThread->enqueueTask(m_taskFactory->createPlay(*this));
-        }
+        m_workerThread->enqueueTask(m_taskFactory->createPlay(*this));
     }
 }
 
 void GstGenericPlayer::pause()
 {
-    ++m_ongoingStateChangesNumber;
     if (m_workerThread)
     {
         m_workerThread->enqueueTask(m_taskFactory->createPause(m_context, *this));
@@ -1691,7 +1676,6 @@ void GstGenericPlayer::pause()
 
 void GstGenericPlayer::stop()
 {
-    ++m_ongoingStateChangesNumber;
     if (m_workerThread)
     {
         m_workerThread->enqueueTask(m_taskFactory->createStop(m_context, *this));
@@ -1705,7 +1689,6 @@ GstStateChangeReturn GstGenericPlayer::changePipelineState(GstState newState)
         RIALTO_SERVER_LOG_ERROR("Change state failed - pipeline is nullptr");
         if (m_gstPlayerClient)
             m_gstPlayerClient->notifyPlaybackState(PlaybackState::FAILURE);
-        --m_ongoingStateChangesNumber;
         return GST_STATE_CHANGE_FAILURE;
     }
     m_context.flushOnPrerollController->setTargetState(newState);
@@ -1716,7 +1699,6 @@ GstStateChangeReturn GstGenericPlayer::changePipelineState(GstState newState)
         if (m_gstPlayerClient)
             m_gstPlayerClient->notifyPlaybackState(PlaybackState::FAILURE);
     }
-    --m_ongoingStateChangesNumber;
     return result;
 }
 

media/server/main/source/MediaPipelineServerInternal.cpp

@@ -341,7 +341,7 @@ bool MediaPipelineServerInternal::play(bool &async)
     bool result;
     auto task = [&]() { result = playInternal(async); };
 
-    m_mainThread->enqueuePriorityTaskAndWait(m_mainThreadClientId, task);
+    m_mainThread->enqueueTaskAndWait(m_mainThreadClientId, task);
     return result;
 }
 

tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerTest.cpp

@@ -157,15 +157,6 @@ TEST_F(GstGenericPlayerTest, shouldAllSourcesAttached)
 
 TEST_F(GstGenericPlayerTest, shouldPlayOnWorkerThread)
 {
-    // Pause first
-    std::unique_ptr<IPlayerTask> pauseTask{std::make_unique<StrictMock<PlayerTaskMock>>()};
-    EXPECT_CALL(dynamic_cast<StrictMock<PlayerTaskMock> &>(*pauseTask), execute());
-    EXPECT_CALL(m_taskFactoryMock, createPause(_, _)).WillOnce(Return(ByMove(std::move(pauseTask))));
-
-    m_sut->pause();
-
-    // ...
-
     bool async = false;
     std::unique_ptr<IPlayerTask> task{std::make_unique<StrictMock<PlayerTaskMock>>()};
     EXPECT_CALL(dynamic_cast<StrictMock<PlayerTaskMock> &>(*task), execute());
@@ -175,22 +166,6 @@ TEST_F(GstGenericPlayerTest, shouldPlayOnWorkerThread)
     EXPECT_TRUE(async);
 }
 
-TEST_F(GstGenericPlayerTest, shouldPlayImmediatelySynchronously)
-{
-    bool async = false;
-    EXPECT_CALL(*m_gstWrapperMock, gstElementSetState(_, GST_STATE_PLAYING)).WillOnce(Return(GST_STATE_CHANGE_SUCCESS));
-    m_sut->play(async);
-    EXPECT_FALSE(async);
-}
-
-TEST_F(GstGenericPlayerTest, shouldPlayImmediatelyAsynchronously)
-{
-    bool async = false;
-    EXPECT_CALL(*m_gstWrapperMock, gstElementSetState(_, GST_STATE_PLAYING)).WillOnce(Return(GST_STATE_CHANGE_ASYNC));
-    m_sut->play(async);
-    EXPECT_TRUE(async);
-}
-
 TEST_F(GstGenericPlayerTest, shouldPause)
 {
     std::unique_ptr<IPlayerTask> task{std::make_unique<StrictMock<PlayerTaskMock>>()};

tests/unittests/media/server/main/mediaPipeline/MiscellaneousFunctionsTest.cpp

@@ -45,7 +45,7 @@ TEST_F(RialtoServerMediaPipelineMiscellaneousFunctionsTest, PlaySuccess)
 {
     bool async{false};
     loadGstPlayer();
-    mainThreadWillEnqueuePriorityTaskAndWait();
+    mainThreadWillEnqueueTaskAndWait();
 
     EXPECT_CALL(*m_gstPlayerMock, play(_));
     EXPECT_TRUE(m_mediaPipeline->play(async));
@@ -57,7 +57,7 @@ TEST_F(RialtoServerMediaPipelineMiscellaneousFunctionsTest, PlaySuccess)
 TEST_F(RialtoServerMediaPipelineMiscellaneousFunctionsTest, PlayFailureDueToUninitializedPlayer)
 {
     bool async{false};
-    mainThreadWillEnqueuePriorityTaskAndWait();
+    mainThreadWillEnqueueTaskAndWait();
     EXPECT_FALSE(m_mediaPipeline->play(async));
 }
 

tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.cpp

@@ -82,13 +82,6 @@ void MediaPipelineTestBase::mainThreadWillEnqueueTaskAndWait()
         .RetiresOnSaturation();
 }
 
-void MediaPipelineTestBase::mainThreadWillEnqueuePriorityTaskAndWait()
-{
-    EXPECT_CALL(*m_mainThreadMock, enqueuePriorityTaskAndWait(m_kMainThreadClientId, _))
-        .WillOnce(Invoke([](uint32_t clientId, firebolt::rialto::server::IMainThread::Task task) { task(); }))
-        .RetiresOnSaturation();
-}
-
 void MediaPipelineTestBase::loadGstPlayer()
 {
     mainThreadWillEnqueueTaskAndWait();

tests/unittests/media/server/main/mediaPipeline/base/MediaPipelineTestBase.h

@@ -90,7 +90,6 @@ class MediaPipelineTestBase : public ::testing::Test
     void destroyMediaPipeline();
     void mainThreadWillEnqueueTask();
     void mainThreadWillEnqueueTaskAndWait();
-    void mainThreadWillEnqueuePriorityTaskAndWait();
     void loadGstPlayer();
     int attachSource(MediaSourceType sourceType, const std::string &mimeType);
     void setEos(MediaSourceType sourceType);