Add support for 'first-audio-frame-callback' signal#552
Conversation
Koky2701
commented
Jun 30, 2026
|
Pull request must be merged with a description containing the required fields, Summary: If there is no jira releated to this change, please put 'Jira: NO-JIRA'. Description can be changed by editing the top comment on your pull request and making a new commit. |
There was a problem hiding this comment.
Pull request overview
This PR extends Rialto’s existing “first video frame received” notification pipeline to also support the first audio frame, including support for the first-audio-frame-callback signal and a fallback sink-pad probe when the callback signal is unavailable, while reusing the existing FirstFrameReceivedEvent / notifyFirstFrameReceived(...) contract end-to-end.
Changes:
- Add audio first-frame detection during GStreamer element setup (signal hookup + sink fallback probe) and schedule the existing first-frame task path using
MediaSourceType::AUDIO. - Add lifecycle cleanup/reset for audio first-frame state (attach/reattach/flush/stop/term).
- Add/update unit + component tests and add spec-driven documentation (openspec change).
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/media/server/mocks/gstplayer/GstGenericPlayerPrivateMock.h | Extends private-player mock with audio first-frame scheduling/probe APIs. |
| tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/SetupElementTest.cpp | Adds UT for first-audio-frame signal hookup behavior. |
| tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/FirstFrameReceivedTest.cpp | Adds UT coverage for notifying first frame received for audio. |
| tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerPrivateTest.cpp | Adds UT for scheduling first audio frame received task. |
| tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsContext.h | Adds audio first-frame callback storage for UT harness. |
| tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.h | Adds UT helpers for audio first-frame signal/trigger paths. |
| tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp | Implements UT helper expectations and triggers for audio first-frame. |
| tests/componenttests/server/tests/mediaPipeline/FirstFrameNotificationTest.cpp | Adds server CT covering end-to-end audio first-frame notification. |
| tests/componenttests/client/tests/mse/FirstFrameNotificationTest.cpp | Extends client CT to validate both video and audio first-frame notifications. |
| tests/componenttests/client/tests/base/MediaPipelineTestMethods.h | Adds client CT helper declarations for audio first-frame notification. |
| tests/componenttests/client/tests/base/MediaPipelineTestMethods.cpp | Implements client CT helpers to expect/send audio first-frame events. |
| openspec/changes/audio-first-frame/tasks.md | Adds implementation/validation task checklist for the change. |
| openspec/changes/audio-first-frame/specs/audio-first-frame/spec.md | Adds formal requirements for audio first-frame detection + one-shot guard. |
| openspec/changes/audio-first-frame/proposal.md | Documents motivation and scope of the audio first-frame change. |
| openspec/changes/audio-first-frame/design.md | Documents detection strategy, fallback probing, and one-shot emission design. |
| openspec/changes/audio-first-frame/.openspec.yaml | Adds openspec metadata for the new change package. |
| media/server/gstplayer/source/Utils.cpp | Extends first-frame signal name detection set to include audio signals. |
| media/server/gstplayer/source/tasks/generic/Stop.cpp | Resets audio first-frame state and clears fallback probe on stop. |
| media/server/gstplayer/source/tasks/generic/SetupElement.cpp | Wires first-audio-frame signal callback + installs sink fallback probe when needed. |
| media/server/gstplayer/source/tasks/generic/AttachSource.cpp | Resets audio first-frame state on (re)attach. |
| media/server/gstplayer/source/GstGenericPlayer.cpp | Implements scheduling guard + probe state management/cleanup for audio first-frame. |
| media/server/gstplayer/include/IGstGenericPlayerPrivate.h | Extends private interface for audio first-frame scheduling and probe lifecycle control. |
| media/server/gstplayer/include/GstGenericPlayer.h | Declares new private-interface methods on the concrete player. |
| media/server/gstplayer/include/GenericPlayerContext.h | Adds audio first-frame one-shot flag + fallback probe state to context. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fe8574e to
11dc49b
Compare
11dc49b to
a4be17b
Compare
a4be17b to
f276929
Compare
|
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp:456:0: style: The function 'expectFirstAudioFrameSignalConnection' is never used. [unusedFunction] |
f276929 to
3dc70f5
Compare
3dc70f5 to
5bef55c
Compare
|
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp:795:0: style: The function 'shouldSetupAudioSinkElementWithPendingLowLatency' is never used. [unusedFunction] |
5bef55c to
e015fde
Compare
e015fde to
ba877b1
Compare
ba877b1 to
a9c8627
Compare
a9c8627 to
3e3e9f1
Compare
|
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp:750:0: style: The function 'shouldSetupAudioDecoderElementWithFirstAudioFrameCallback' is never used. [unusedFunction] |
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
| EXPECT_CALL(*m_glibWrapperMock, gSignalQuery(m_signals[0], _)) | ||
| .WillRepeatedly(Invoke([&](guint signal_id, GSignalQuery *query) | ||
| { query->signal_name = "first-video-frame-callback"; })); | ||
| .WillRepeatedly(Invoke([&](guint signal_id, GSignalQuery *query) { query->signal_name = signalName; })); | ||
| EXPECT_CALL(*m_glibWrapperMock, gFree(m_signals)).Times(2); |
| if (!m_firstVideoFrameCallback || !m_firstVideoFrameData) | ||
| { | ||
| return; | ||
| } |
|
Coverage statistics of your commit: |
| if (m_context.firstAudioFrameReceived) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("First audio frame notification already sent"); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (m_gstPlayerClient) | ||
| { | ||
| if (m_sourceType == MediaSourceType::AUDIO) | ||
| { | ||
| m_context.firstAudioFrameReceived = true; | ||
| } | ||
| m_gstPlayerClient->notifyFirstFrameReceived(m_sourceType); | ||
| } |
| #include <gst/gst.h> | ||
|
|
||
| #include <functional> | ||
|
|
|
Coverage statistics of your commit: |
| guint m_signals[1]{123}; | ||
| GCallback m_audioUnderflowCallback; | ||
| GCallback m_videoUnderflowCallback; | ||
| GCallback m_firstVideoFrameCallback; | ||
| GCallback m_firstAudioFrameCallback; | ||
| GstPadProbeCallback m_firstAudioFrameProbeCallback{}; | ||
| GCallback m_childAddedCallback; | ||
| GCallback m_childRemovedCallback; |
| if (!m_firstVideoFrameCallback || !m_firstVideoFrameData) | ||
| { | ||
| return; | ||
| } |
|
Coverage statistics of your commit: |
| if (m_gstPlayerClient) | ||
| { | ||
| if (m_sourceType == MediaSourceType::AUDIO) | ||
| { | ||
| m_context.firstAudioFrameReceived = true; | ||
| } | ||
| m_gstPlayerClient->notifyFirstFrameReceived(m_sourceType); | ||
| } |
|
Coverage statistics of your commit: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
media/server/gstplayer/source/GstGenericPlayer.cpp:1520
pushSampleIfRequirednow takes a free-formtypeStr, but the function uses it for behavioral logic (audioGstSegmentPositionupdate) rather than just logging. This is brittle (callers must pass exactlyconvertMediaSourceType(...)) and contradicts the header doc that says it’s only for logging. Prefer passingMediaSourceTypeand deriving the string locally, then compare the enum for the audio-only branch.
void GstGenericPlayer::pushSampleIfRequired(GstElement *source, const std::string &typeStr)
{
auto initialPosition = m_context.initialPositions.find(source);
if (m_context.initialPositions.end() == initialPosition)
{
| if (typeStr == common::convertMediaSourceType(MediaSourceType::AUDIO)) | ||
| { | ||
| m_context.audioGstSegmentPosition = position; | ||
| } |
| else if (isAudioSink(*m_gstWrapper, m_element)) | ||
| { | ||
| GstPad *sinkPad = m_gstWrapper->gstElementGetStaticPad(m_element, "sink"); | ||
| if (sinkPad) | ||
| { | ||
| gulong probeId = m_gstWrapper->gstPadAddProbe(sinkPad, GST_PAD_PROBE_TYPE_BUFFER, | ||
| firstAudioFrameProbeCallback, &m_player, nullptr); | ||
|
|
||
| if (probeId != 0) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("Installed first audio frame fallback probe on sink"); | ||
| m_player.setAudioFirstFrameFallbackProbe(sinkPad, probeId); | ||
| } | ||
| else | ||
| { | ||
| m_gstWrapper->gstObjectUnref(sinkPad); | ||
| } | ||
| } |
There was a problem hiding this comment.
That's true, apply it.
|
Coverage statistics of your commit: |
1 similar comment
|
Coverage statistics of your commit: |
|
tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerPrivateTest.cpp:357:45: style: Parameter 'context' can be declared as reference to const [constParameterReference] |
| if (!m_firstVideoFrameCallback || !m_firstVideoFrameData) | ||
| { | ||
| return; | ||
| } |
| void getSyncFailure() | ||
| { | ||
| auto req{createGetSyncRequest(m_sessionId)}; | ||
| auto req{createGetSyncRequest(m_sessionId + 1)}; |
| // Step 8: Fail to get Sync | ||
| willFailToGetSink(); | ||
| getSyncFailure(); | ||
|
|
| EXPECT_CALL(*testContext->m_gstWrapper, | ||
| gstElementFactoryListIsType(testContext->m_elementFactory, GST_ELEMENT_FACTORY_TYPE_SINK)) | ||
| .WillOnce(Return(TRUE)); | ||
| gstElementFactoryListIsType(testContext->m_elementFactory, GST_ELEMENT_FACTORY_TYPE_DECODER)) | ||
| .WillRepeatedly(Return(TRUE)); |
| auto req{createGetSyncRequest(m_sessionId + 1)}; | ||
| ConfigureAction<GetSync>(m_clientStub).send(req).expectFailure(); | ||
| waitWorker(); |
| // Step 8: Fail to get Sync | ||
| willFailToGetSink(); | ||
| getSyncFailure(); | ||
|
|
|
Coverage statistics of your commit: |
1 similar comment
|
Coverage statistics of your commit: |
| /** | ||
| * @brief Schedules first audio frame received task. Called by the GStreamer thread. | ||
| */ | ||
| virtual void scheduleFirstAudioFrameReceived() = 0; |
There was a problem hiding this comment.
If I see correctly, thr Copilot is right here, you probably should remove this virtual method
| GstElement *appSrc = nullptr; | ||
| if (m_attachedSource->getType() == MediaSourceType::AUDIO) | ||
| { | ||
| m_context.firstAudioFrameReceived = false; |
There was a problem hiding this comment.
it's still not resolved :)
| else if (isAudioSink(*m_gstWrapper, m_element)) | ||
| { | ||
| GstPad *sinkPad = m_gstWrapper->gstElementGetStaticPad(m_element, "sink"); | ||
| if (sinkPad) | ||
| { | ||
| gulong probeId = m_gstWrapper->gstPadAddProbe(sinkPad, GST_PAD_PROBE_TYPE_BUFFER, | ||
| firstAudioFrameProbeCallback, &m_player, nullptr); | ||
|
|
||
| if (probeId != 0) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("Installed first audio frame fallback probe on sink"); | ||
| m_player.setAudioFirstFrameFallbackProbe(sinkPad, probeId); | ||
| } | ||
| else | ||
| { | ||
| m_gstWrapper->gstObjectUnref(sinkPad); | ||
| } | ||
| } |
There was a problem hiding this comment.
That's true, apply it.
|
Coverage statistics of your commit: |
| const char *underflowSignals[]{"buffer-underflow-callback", "vidsink-underflow-callback", "underrun-callback"}; | ||
| const char *firstFrameSignals[]{"first-video-frame-callback"}; | ||
| const char *firstFrameSignals[]{"first-video-frame-callback", "first-audio-frame", "first-audio-frame-callback"}; | ||
|
|
| GCallback m_audioUnderflowCallback; | ||
| GCallback m_videoUnderflowCallback; | ||
| GCallback m_firstVideoFrameCallback; | ||
| GCallback m_firstAudioFrameCallback; | ||
| GstPadProbeCallback m_firstAudioFrameProbeCallback{}; |
|
Coverage statistics of your commit: |
| if (!m_firstVideoFrameCallback || !m_firstVideoFrameData) | ||
| { | ||
| return; | ||
| } |
| if (!m_firstFrameCallback || !m_firstFrameData) | ||
| { | ||
| return; | ||
| } |
Summary: Fixing review comments
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-17882
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
media/server/gstplayer/source/GstGenericPlayer.cpp:1526
pushSampleIfRequiredconstructskTypeStrbefore the early-return check. This allocates astd::stringeven when there is no initial position entry (the common no-op path). Consider moving the string construction after the early return to avoid the unnecessary allocation.
const std::string kTypeStr{common::convertMediaSourceType(mediaSourceType)};
auto initialPosition = m_context.initialPositions.find(source);
if (m_context.initialPositions.end() == initialPosition)
{
// Sending initial sample not needed
return;
}
| #include "WorkerThread.h" | ||
| #include "tasks/generic/FirstFrameReceived.h" | ||
| #include "tasks/generic/GenericPlayerTaskFactory.h" |
|
Coverage statistics of your commit: |