[NTS] Fast Playback test case failures - #523
Conversation
|
Pull request title must follow the pattern: Pull request description must follow the Commit message format for RDK-E:
< JIRA TICKET >: < one line summary of change less than 65 characters > |
There was a problem hiding this comment.
Pull request overview
Fixes premature audio/video underflow reporting in the GStreamer generic player setup by ignoring underflow callbacks while the sink/decoder still reports buffered frames (non-zero FIFO depth). This aligns underflow signaling with actual depletion and prevents fast-playback-related test failures.
Changes:
- Guard audio/video underflow callbacks to no-op when
fifoDepth > 0. - Add unit tests asserting underflow is not scheduled when
fifoDepthis non-zero for both audio and video.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
media/server/gstplayer/source/tasks/generic/SetupElement.cpp |
Ignores underflow callbacks when FIFO depth is non-zero to prevent premature underflow scheduling. |
tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/SetupElementTest.cpp |
Adds unit coverage for the new “ignore underflow when fifoDepth > 0” behavior for audio and video. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d11ae39 to
e38c32a
Compare
|
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp:263:5: performance: Variable 'testContext' is assigned in constructor body. Consider performing initialization in initialization list. [useInitializationList] |
| TEST_F(SetupElementTest, shouldNotReportVideoUnderflowWhenFifoDepthIsNonZero) | ||
| { | ||
| shouldSetupVideoDecoderElementOnly(); | ||
| triggerSetupElement(); | ||
|
|
||
| ASSERT_TRUE(testContext->m_videoUnderflowCallback); | ||
| EXPECT_CALL(testContext->m_gstPlayer, scheduleVideoUnderflow()).Times(0); | ||
|
|
f48c93a to
0d2a258
Compare
| auto videoUnderflowCallback = reinterpret_cast<void (*)(GstElement *, guint, gpointer, gpointer)>( | ||
| testContext->m_videoUnderflowCallback); | ||
| videoUnderflowCallback(testContext->m_element, 1, nullptr, &testContext->m_gstPlayer); | ||
| TEST_F(SetupElementTest, shouldReportFirstVideoFrame) |
|
tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/SetupElementTest.cpp:174:1: error: Unmatched '{'. Configuration: ''. [syntaxError] |
b56c353 to
1450d87
Compare
1450d87 to
599a0a5
Compare
| ASSERT_TRUE(testContext->m_videoUnderflowCallback); | ||
| EXPECT_CALL(testContext->m_gstPlayer, scheduleVideoUnderflow()).Times(0); | ||
|
|
||
| triggerVideoUnderflowCallback(); | ||
| } |
| ASSERT_TRUE(testContext->m_audioUnderflowCallback); | ||
| EXPECT_CALL(testContext->m_gstPlayer, scheduleAudioUnderflow()).Times(0); | ||
|
|
||
| triggerAudioUnderflowCallback(); | ||
| } |
| #include <memory> | ||
| #include <string> | ||
|
|
||
| #include "GenericTasksTestsContext.h" | ||
|
|
1c5fafb to
ddce17b
Compare
| if (fifoDepth > 0) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Ignoring audio underflow callback - fifoDepth is %u", fifoDepth); | ||
| return; | ||
| } |
| if (fifoDepth > 0) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Ignoring video underflow callback - fifoDepth is %u", fifoDepth); | ||
| return; | ||
| } |
|
Coverage statistics of your commit: |
ddce17b to
e832f8d
Compare
|
Coverage statistics of your commit: |
1 similar comment
|
Coverage statistics of your commit: |
bdbe3ed to
5cb1188
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Suppressed comments (4)
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:73
videoUnderflowCallbackignores thequeueDepthargument, so it will still schedule an underflow whenfifoDepth == 0butqueueDepth > 0(which means there are still frames queued). This also makes the new queue-depth unit test impossible to satisfy.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
tests/unittests/media/server/gstplayer/genericPlayer/tasksTests/SetupElementTest.cpp:197
- This test passes a non-null
queueDepthpointer and expects underflow not to be scheduled, but the callback path will also callisVideoHandleSet()when depth is non-zero. Becausem_gstPlayeris aStrictMock, the test should set an expectation forisVideoHandleSet()(and returntrue).
ASSERT_TRUE(testContext->m_videoUnderflowCallback);
EXPECT_CALL(testContext->m_gstPlayer, scheduleVideoUnderflow()).Times(0);
guint queueDepth = 1;
auto videoUnderflowCallback = reinterpret_cast<void (*)(GstElement *, guint, gpointer, gpointer)>(
testContext->m_videoUnderflowCallback);
videoUnderflowCallback(testContext->m_element, 0, &queueDepth, &testContext->m_gstPlayer);
media/server/gstplayer/source/GstGenericPlayer.cpp:1889
GstGenericPlayer::isVideoHandleSet()is defined here, but there is no corresponding method declaration/override inmedia/server/gstplayer/include/GstGenericPlayer.h. As written, this out-of-line definition will not compile, and the class also won’t satisfy the new pure-virtual method onIGstGenericPlayerPrivate.
bool GstGenericPlayer::isVideoHandleSet() const
{
return m_context.isVideoHandleSet;
}
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:52
audioUnderflowCallbackalso ignores thequeueDepthargument; if GStreamer reportsfifoDepth == 0but the queue depth is still non-zero, this will still schedule an underflow prematurely.
This issue also appears on line 70 of the same file.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
| ASSERT_TRUE(testContext->m_audioUnderflowCallback); | ||
| EXPECT_CALL(testContext->m_gstPlayer, scheduleAudioUnderflow()).Times(0); | ||
|
|
||
| auto audioUnderflowCallback = reinterpret_cast<void (*)(GstElement *, guint, gpointer, gpointer)>( | ||
| testContext->m_audioUnderflowCallback); |
acff4de to
844084a
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:74
- Video underflow suppression currently only checks fifoDepth (and also gates it on
isVideoHandleSet()), but the signal also provides queueDepth. As written, underflow will still be scheduled when fifoDepth == 0 but queueDepth > 0 (buffer still has queued frames). Underflow should only be scheduled when both fifoDepth and queueDepth are zero.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:52
- The underflow callback ignores the provided queueDepth and only suppresses underflow when
isVideoHandleSet()is true. This means underflow can still be scheduled even when the sink reports buffered data (fifoDepth/queueDepth > 0), and it also introduces a StrictMock failure in unit tests when fifoDepth > 0 (uninterestingisVideoHandleSet()call). Consider treating underflow as real only when both fifoDepth and queueDepth are zero, independent of subtitle/video-handle state.
This issue also appears on line 70 of the same file.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
844084a to
24ea018
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:73
videoUnderflowCallbackignores thequeueDepthargument, so a non-zero queue depth will still schedule underflow (contradicting the new testshouldNotReportVideoUnderflowWhenQueueDepthIsNonZero). IncorporatequeueDepthinto the early-return condition so underflow is not reported while either fifo or queue still contains data.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:49
audioUnderflowCallbacksuppresses underflow only whenfifoDepth > 0andisVideoHandleSet()is true. This (a) still reports underflow whenqueueDepthis non-zero, and (b) callsisVideoHandleSet()for audio underflow (which also breaks the new unit test expecting no additional mock calls whenfifoDepthis non-zero). Consider treating any non-zero fifo/queue depth as “not underflow” for audio, without consulting the video-handle state.
This issue also appears on line 70 of the same file.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
24ea018 to
6ae62ab
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (3)
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.h:33
- GenericTasksTestsBase.h includes GenericTasksTestsContext.h but also forward-declares GenericTasksTestsContext. Since the header only stores a std::shared_ptr, the forward declaration is sufficient and the include can be dropped to avoid pulling the full context into every test translation unit.
#include "GenericTasksTestsContext.h"
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:73
- videoUnderflowCallback ignores the queueDepth argument. The updated unit test passes fifoDepth=0 with non-zero queueDepth and expects underflow NOT to be scheduled; with the current code scheduleVideoUnderflow() will still be called.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:50
- audioUnderflowCallback currently calls isVideoHandleSet() and only suppresses underflow when that returns true. This makes audio-underflow suppression depend on a video-specific flag and also triggers an unexpected StrictMock call in unit tests. It should ignore underflow whenever fifoDepth (or queueDepth) indicates buffered data, without consulting isVideoHandleSet().
This issue also appears on line 70 of the same file.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
Summary: Fix premature underflow signal when buffer is still rendering frames
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-15056
6ae62ab to
ec72e75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:74
- videoUnderflowCallback ignores the queueDepth argument. This means underflow can be scheduled even when the queue still contains data (queueDepth > 0), and the new unit test that passes fifoDepth==0 with queueDepth==1 would fail. Incorporate queueDepth into the suppression condition so that non-zero fifoDepth OR queueDepth prevents scheduling underflow (when isVideoHandleSet() indicates the relevant handle is set).
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
media/server/gstplayer/source/tasks/generic/SetupElement.cpp:50
- audioUnderflowCallback currently gates the early-return on isVideoHandleSet(). This makes audio underflow reporting depend on a video/subtitle handle state and will still schedule audio underflow when fifoDepth > 0 in audio-only cases (and also causes unexpected isVideoHandleSet() calls in StrictMock tests). The callback should suppress underflow whenever fifoDepth (or queueDepth) is non-zero, without consulting isVideoHandleSet().
This issue also appears on line 70 of the same file.
if (fifoDepth > 0 && player && player->isVideoHandleSet())
{
return;
}
This reverts commit ec72e75.
Summary: Checking timestamps to verify whether there are skipped frames
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-15056
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Suppressed comments (2)
media/server/gstplayer/source/GstGenericPlayer.cpp:1439
- This adds an INFO log for every buffer pushed. In steady-state playback this can be extremely chatty and may impact performance. Also,
GST_BUFFER_{PTS,DTS,DURATION}can beGST_CLOCK_TIME_NONE(i.e., -1); casting touint64_twill log a very large number, which is misleading during debugging.
const auto pts = static_cast<uint64_t>(GST_BUFFER_PTS(buffer));
const auto dts = static_cast<uint64_t>(GST_BUFFER_DTS(buffer));
const auto duration = static_cast<uint64_t>(GST_BUFFER_DURATION(buffer));
RIALTO_SERVER_LOG_INFO(
"Pushing %s buffer to gst pipeline: pts=%" PRIu64 " dts=%" PRIu64 " duration=%" PRIu64,
common::convertMediaSourceType(mediaType), pts, dts, duration);
media/server/gstplayer/source/GstGenericPlayer.cpp:1439
- PR description says this is a functional fix for a premature underflow signal, but the only change here is adding per-buffer logging. If the underflow fix is implemented elsewhere, it isn't reflected in this PR diff; otherwise the PR description should be updated to match (or the actual underflow fix added).
RIALTO_SERVER_LOG_INFO(
"Pushing %s buffer to gst pipeline: pts=%" PRIu64 " dts=%" PRIu64 " duration=%" PRIu64,
common::convertMediaSourceType(mediaType), pts, dts, duration);
Summary: Fix premature underflow signal when buffer is still rendering frames
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-15056