Skip to content

Fix pipeline setup async requests executing at pipeline teardown #563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
smudri85 wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Fix pipeline setup async requests executing at pipeline teardown #563

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
26cf49a
Added ref/unref for GST element for DeepElementAdded task.
Jul 15, 2026
9817d97
Added ref/unref for GST element for UpdatePlaybackGroup task.
Jul 16, 2026
8a757cc
Added UTs following given changes.
Jul 16, 2026
51db707
Added changes suggested by copilot.
Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions media/server/gstplayer/include/tasks/IGenericPlayerTaskFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,13 +408,13 @@ class IGenericPlayerTaskFactory
* @param[in] context : The GstGenericPlayer context
* @param[in] player : The GstGenericPlayer instance
* @param[in] typefind : The typefind element.
* @param[in] caps : The GstCaps of added element
* @param[in] caps : The GstCaps of added element.
Comment thread
smudri85 marked this conversation as resolved.
*
Comment thread
smudri85 marked this conversation as resolved.
* @retval the new UpdatePlaybackGroup task instance.
*/
virtual std::unique_ptr<IPlayerTask> createUpdatePlaybackGroup(GenericPlayerContext &context,
IGstGenericPlayerPrivate &player,
GstElement *typefind, const GstCaps *caps) const = 0;
GstElement *typefind, GstCaps *caps) const = 0;

/**
* @brief Creates a RenderFrame task.
Expand Down
2 changes: 1 addition & 1 deletion media/server/gstplayer/include/tasks/generic/GenericPlayerTaskFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ class GenericPlayerTaskFactory : public IGenericPlayerTaskFactory
MediaSourceType sourceType) const override;
std::unique_ptr<IPlayerTask> createUpdatePlaybackGroup(GenericPlayerContext &context,
IGstGenericPlayerPrivate &player, GstElement *typefind,
const GstCaps *caps) const override;
GstCaps *caps) const override;
std::unique_ptr<IPlayerTask> createRenderFrame(GenericPlayerContext &context,
IGstGenericPlayerPrivate &player) const override;
std::unique_ptr<IPlayerTask> createPing(std::unique_ptr<IHeartbeatHandler> &&heartbeatHandler) const override;
Expand Down
4 changes: 2 additions & 2 deletions media/server/gstplayer/include/tasks/generic/UpdatePlaybackGroup.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ class UpdatePlaybackGroup : public IPlayerTask
UpdatePlaybackGroup(GenericPlayerContext &context, IGstGenericPlayerPrivate &player,
const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
const std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> &glibWrapper,
GstElement *typefind, const GstCaps *caps);
GstElement *typefind, GstCaps *caps);
~UpdatePlaybackGroup() override;
void execute() const override;

Expand All @@ -46,7 +46,7 @@ class UpdatePlaybackGroup : public IPlayerTask
std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> m_gstWrapper;
std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> m_glibWrapper;
GstElement *m_typefind;
const GstCaps *m_caps;
GstCaps *m_caps;
Comment thread
smudri85 marked this conversation as resolved.
Comment thread
smudri85 marked this conversation as resolved.
};
} // namespace firebolt::rialto::server::tasks::generic

Expand Down
8 changes: 7 additions & 1 deletion media/server/gstplayer/source/GstGenericPlayer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ void GstGenericPlayer::deepElementAdded(GstBin *pipeline, GstBin *bin, GstElemen
RIALTO_SERVER_LOG_DEBUG("Deep element %s added to the pipeline", GST_ELEMENT_NAME(element));
if (self->m_workerThread)
{
self->m_gstWrapper->gstObjectRef(element);
self->m_workerThread->enqueueTask(
self->m_taskFactory->createDeepElementAdded(self->m_context, *self, pipeline, bin, element));
}
Comment thread
smudri85 marked this conversation as resolved.
Expand Down Expand Up @@ -2760,7 +2761,12 @@ void GstGenericPlayer::handleBusMessage(GstMessage *message)

void GstGenericPlayer::updatePlaybackGroup(GstElement *typefind, const GstCaps *caps)
{
m_workerThread->enqueueTask(m_taskFactory->createUpdatePlaybackGroup(m_context, *this, typefind, caps));
if (m_workerThread)
{
m_gstWrapper->gstObjectRef(typefind);
GstCaps *ownedCaps{caps ? m_gstWrapper->gstCapsCopy(caps) : nullptr};
m_workerThread->enqueueTask(m_taskFactory->createUpdatePlaybackGroup(m_context, *this, typefind, ownedCaps));
}
Comment thread
smudri85 marked this conversation as resolved.
}

void GstGenericPlayer::addAutoVideoSinkChild(GObject *object)
Expand Down
1 change: 1 addition & 0 deletions media/server/gstplayer/source/tasks/generic/DeepElementAdded.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ DeepElementAdded::~DeepElementAdded()
{
RIALTO_SERVER_LOG_DEBUG("DeepElementAdded finished");
m_glibWrapper->gFree(m_elementName);
m_gstWrapper->gstObjectUnref(m_element);
}

void DeepElementAdded::execute() const
Expand Down
3 changes: 1 addition & 2 deletions media/server/gstplayer/source/tasks/generic/GenericPlayerTaskFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,8 +282,7 @@ std::unique_ptr<IPlayerTask> GenericPlayerTaskFactory::createFirstFrameReceived(

std::unique_ptr<IPlayerTask> GenericPlayerTaskFactory::createUpdatePlaybackGroup(GenericPlayerContext &context,
IGstGenericPlayerPrivate &player,
GstElement *typefind,
const GstCaps *caps) const
GstElement *typefind, GstCaps *caps) const
{
return std::make_unique<tasks::generic::UpdatePlaybackGroup>(context, player, m_gstWrapper, m_glibWrapper, typefind,
caps);
Expand Down
7 changes: 6 additions & 1 deletion media/server/gstplayer/source/tasks/generic/UpdatePlaybackGroup.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ namespace firebolt::rialto::server::tasks::generic
UpdatePlaybackGroup::UpdatePlaybackGroup(GenericPlayerContext &context, IGstGenericPlayerPrivate &player,
const std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> &gstWrapper,
const std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> &glibWrapper,
GstElement *typefind, const GstCaps *caps)
GstElement *typefind, GstCaps *caps)
: m_context{context}, m_player{player}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_typefind{typefind},
m_caps{caps}
{
Expand All @@ -35,6 +35,11 @@ UpdatePlaybackGroup::UpdatePlaybackGroup(GenericPlayerContext &context, IGstGene
UpdatePlaybackGroup::~UpdatePlaybackGroup()
{
RIALTO_SERVER_LOG_DEBUG("UpdatePlaybackGroup finished");
m_gstWrapper->gstObjectUnref(m_typefind);
if (m_caps)
{
m_gstWrapper->gstCapsUnref(m_caps);
}
Comment thread
smudri85 marked this conversation as resolved.
}

void UpdatePlaybackGroup::execute() const
Expand Down
17 changes: 16 additions & 1 deletion tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerPrivateTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1846,14 +1846,29 @@ TEST_F(GstGenericPlayerPrivateTest, shouldUpdatePlaybackGroup)
{
GstElement typefind;
GstCaps caps;
GstCaps copiedCaps;
std::unique_ptr<IPlayerTask> task{std::make_unique<StrictMock<PlayerTaskMock>>()};
EXPECT_CALL(dynamic_cast<StrictMock<PlayerTaskMock> &>(*task), execute());
EXPECT_CALL(m_taskFactoryMock, createUpdatePlaybackGroup(_, _, &typefind, &caps))
EXPECT_CALL(*m_gstWrapperMock, gstObjectRef(&typefind));
EXPECT_CALL(*m_gstWrapperMock, gstCapsCopy(&caps)).WillOnce(Return(&copiedCaps));
EXPECT_CALL(m_taskFactoryMock, createUpdatePlaybackGroup(_, _, &typefind, &copiedCaps))
.WillOnce(Return(ByMove(std::move(task))));

m_sut->updatePlaybackGroup(&typefind, &caps);
}

TEST_F(GstGenericPlayerPrivateTest, shouldUpdatePlaybackGroupWithNullCaps)
{
GstElement typefind;
std::unique_ptr<IPlayerTask> task{std::make_unique<StrictMock<PlayerTaskMock>>()};
EXPECT_CALL(dynamic_cast<StrictMock<PlayerTaskMock> &>(*task), execute());
EXPECT_CALL(*m_gstWrapperMock, gstObjectRef(&typefind));
EXPECT_CALL(m_taskFactoryMock, createUpdatePlaybackGroup(_, _, &typefind, nullptr))
.WillOnce(Return(ByMove(std::move(task))));

m_sut->updatePlaybackGroup(&typefind, nullptr);
}

TEST_F(GstGenericPlayerPrivateTest, shouldAddAutoVideoSinkChildSink)
{
const GenericPlayerContext *context = getPlayerContext();
Expand Down
1 change: 1 addition & 0 deletions tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ TEST_F(GstGenericPlayerTest, shouldAddDeepElement)
GstElement element{};
std::unique_ptr<IPlayerTask> task{std::make_unique<StrictMock<PlayerTaskMock>>()};
EXPECT_CALL(dynamic_cast<StrictMock<PlayerTaskMock> &>(*task), execute());
EXPECT_CALL(*m_gstWrapperMock, gstObjectRef(&element));
EXPECT_CALL(m_taskFactoryMock, createDeepElementAdded(_, _, _, _, &element)).WillOnce(Return(ByMove(std::move(task))));

triggerDeepElementAdded(&element);
Expand Down
10 changes: 10 additions & 0 deletions tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2081,6 +2081,8 @@ void GenericTasksTestsBase::shouldNotRegisterCallbackWhenPtrsAreNotEqual()

void GenericTasksTestsBase::constructDeepElementAdded()
{
// The element reference taken in GstGenericPlayer::deepElementAdded is released in the task destructor.
EXPECT_CALL(*testContext->m_gstWrapper, gstObjectUnref(testContext->m_element));
firebolt::rialto::server::tasks::generic::DeepElementAdded task{testContext->m_context,
testContext->m_gstPlayer,
testContext->m_gstWrapper,
Expand Down Expand Up @@ -2163,6 +2165,8 @@ void GenericTasksTestsBase::shouldSetTypefindElement()

void GenericTasksTestsBase::triggerDeepElementAdded()
{
// The element reference taken in GstGenericPlayer::deepElementAdded is released in the task destructor.
EXPECT_CALL(*testContext->m_gstWrapper, gstObjectUnref(testContext->m_element));
firebolt::rialto::server::tasks::generic::DeepElementAdded task{testContext->m_context,
testContext->m_gstPlayer,
testContext->m_gstWrapper,
Expand Down Expand Up @@ -2327,6 +2331,8 @@ void GenericTasksTestsBase::checkAudioSinkPlaybackGroupAdded()

void GenericTasksTestsBase::triggerUpdatePlaybackGroupNoCaps()
{
// The typefind reference taken in GstGenericPlayer::updatePlaybackGroup is released in the task destructor.
EXPECT_CALL(*testContext->m_gstWrapper, gstObjectUnref(testContext->m_element));
firebolt::rialto::server::tasks::generic::UpdatePlaybackGroup task{testContext->m_context,
testContext->m_gstPlayer,
testContext->m_gstWrapper,
Expand All @@ -2349,6 +2355,10 @@ void GenericTasksTestsBase::shouldReturnNullCaps()

void GenericTasksTestsBase::triggerUpdatePlaybackGroup()
{
// The typefind and caps references (owned copy) taken in GstGenericPlayer::updatePlaybackGroup are released in the
// task destructor.
EXPECT_CALL(*testContext->m_gstWrapper, gstObjectUnref(testContext->m_element));
EXPECT_CALL(*testContext->m_gstWrapper, gstCapsUnref(&testContext->m_gstCaps1));
firebolt::rialto::server::tasks::generic::UpdatePlaybackGroup task{testContext->m_context,
testContext->m_gstPlayer,
testContext->m_gstWrapper,
Expand Down
10 changes: 8 additions & 2 deletions ...nittests/media/server/gstplayer/genericPlayer/tasksTests/GenericPlayerTaskFactoryTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,14 @@ TEST_F(GenericPlayerTaskFactoryTest, ShouldCreateAttachSource)

TEST_F(GenericPlayerTaskFactoryTest, ShouldCreateDeepElementAdded)
{
GstElement element{};
EXPECT_CALL(*m_gstWrapper, gstObjectParent(_)).WillOnce(Return(nullptr));
EXPECT_CALL(*m_gstWrapper, gstObjectCast(_)).WillOnce(Return(nullptr));
EXPECT_CALL(*m_gstWrapper, gstElementGetName(_)).WillOnce(Return(nullptr));
EXPECT_CALL(*m_glibWrapper, gFree(nullptr));
auto task = m_sut.createDeepElementAdded(m_context, m_gstPlayer, nullptr, nullptr, nullptr);
// The task destructor releases the element reference taken when the task is scheduled.
EXPECT_CALL(*m_gstWrapper, gstObjectUnref(&element));
auto task = m_sut.createDeepElementAdded(m_context, m_gstPlayer, nullptr, nullptr, &element);
EXPECT_NE(task, nullptr);
Comment thread
smudri85 marked this conversation as resolved.
EXPECT_NO_THROW(dynamic_cast<firebolt::rialto::server::tasks::generic::DeepElementAdded &>(*task));
}
Expand Down Expand Up @@ -320,7 +323,10 @@ TEST_F(GenericPlayerTaskFactoryTest, ShouldCreateSetPlaybackRate)

TEST_F(GenericPlayerTaskFactoryTest, ShouldCreateUpdatePlaybackGroup)
{
auto task = m_sut.createUpdatePlaybackGroup(m_context, m_gstPlayer, nullptr, nullptr);
GstElement typefind{};
// The task destructor releases the typefind reference taken when the task is scheduled.
EXPECT_CALL(*m_gstWrapper, gstObjectUnref(&typefind));
auto task = m_sut.createUpdatePlaybackGroup(m_context, m_gstPlayer, &typefind, nullptr);
EXPECT_NE(task, nullptr);
Comment thread
smudri85 marked this conversation as resolved.
EXPECT_NO_THROW(dynamic_cast<firebolt::rialto::server::tasks::generic::UpdatePlaybackGroup &>(*task));
}
Expand Down
3 changes: 1 addition & 2 deletions tests/unittests/media/server/mocks/gstplayer/GenericPlayerTaskFactoryMock.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,7 @@ class GenericPlayerTaskFactoryMock : public IGenericPlayerTaskFactory
MediaSourceType sourceType),
(const, override));
MOCK_METHOD(std::unique_ptr<IPlayerTask>, createUpdatePlaybackGroup,
(GenericPlayerContext & context, IGstGenericPlayerPrivate &player, GstElement *typefind,
const GstCaps *caps),
(GenericPlayerContext & context, IGstGenericPlayerPrivate &player, GstElement *typefind, GstCaps *caps),
(const, override));
MOCK_METHOD(std::unique_ptr<IPlayerTask>, createRenderFrame,
(GenericPlayerContext & context, IGstGenericPlayerPrivate &player), (const, override));
Expand Down
Loading