Skip to content

Conversation

@anjali-syna
Copy link
Contributor

@anjali-syna anjali-syna commented Jan 16, 2026

VPLAY-11816 [Linear] Crash occurred when multiple times Toggling Subtitles and Audio Language During Linear Playback

Reason for change: Audio language change results in configuring the gstremer pipeline, which causes state transition inside gstremer. Sometimes, in the middle of this state change operation, MonitorAvTimerCallback() wakes up and calls GetVideoPlaybackQuality() function in aamp, which queries gstreamer to get some stats data from video decoder. This call is getting hung and then leads to crash.

Changes:

  • Modified GetVideoPlaybackQuality() to avoid querying gstreamer for video decoder stats when state transition is in progress.

Test Procedure: Refer JIRA ticket

Signed-off-by: Anjali Jacob <anjjacob@synamedia.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
…itles and Audio Language During Linear Playback

Reason for change: Audio language change results in configuring the gstremer pipeline, which causes state transition inside gstremer. Sometimes, in the middle of this state change operation, MonitorAvTimerCallback() wake up and calls GetVideoPlaybackQuality() function in aamp, which queries gstreamer to get some statics data from video decoder. This call is getting hung and then leads to crash.

Changes:
* Modified GetVideoPlaybackQuality() to avoid querying gstreamer for video decoder statics when state transition is in progress.

Test Procedure: Refer JIRA ticket

Risks: low

Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
@anjali-syna anjali-syna changed the title Adding debug logs to understand the issue VPLAY-11816 [Linear] Crash occurred when Toggling Subtitles and Audio Language During Linear Playback Jan 22, 2026
@anjali-syna anjali-syna marked this pull request as ready for review January 22, 2026 06:18
@anjali-syna anjali-syna requested a review from a team as a code owner January 22, 2026 06:18
Copilot AI review requested due to automatic review settings January 22, 2026 06:18
@anjali-syna anjali-syna requested a review from jfagunde January 22, 2026 06:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a crash that occurs when toggling subtitles and audio language during linear playback. The crash was caused by GetVideoPlaybackQuality() being called by MonitorAvTimerCallback() while GStreamer was in the middle of a state transition (triggered by audio language changes). The function would hang when querying GStreamer for video decoder statistics during these transitions.

Changes:

  • Added pipeline state checking before querying GStreamer for playback quality statistics
  • Enhanced logging to include state change return codes for better debugging

Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Copy link

@dnfaulkner dnfaulkner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of minor comments.
Plus in the PR description, you say:
"... queries gstreamer to get some statics data..." and
"...querying gstreamer for video decoder statics..."
Shouldn't "statics" be "stats"?
Also, I think that:
"...MonitorAvTimerCallback() wake up..."
"wake" should be "wakes"

@anjali-syna anjali-syna requested a review from DomSyna January 22, 2026 10:32
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Copilot AI review requested due to automatic review settings January 23, 2026 10:06
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 9 comments.

Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Copilot AI review requested due to automatic review settings January 23, 2026 13:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.

Copy link

@dnfaulkner dnfaulkner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Signed-off-by: anjali-syna <206662904+anjali-syna@users.noreply.github.com>
Copilot AI review requested due to automatic review settings January 23, 2026 15:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent initialization syntax within the same test. Lines 1159-1160 use uniform initialization (braces), while lines 1161-1162 use traditional empty braces. For consistency and adherence to modern C++ guidelines, all GstElement declarations should use uniform initialization syntax.

Suggested change
GstElement video_dec = {};
GstElement video_dec{};

Copilot uses AI. Check for mistakes.
Comment on lines +1216 to +1218
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent initialization syntax. Lines 1214-1215 use uniform initialization (braces), while lines 1216-1217 use traditional empty braces. For consistency and adherence to modern C++ guidelines, all GstElement declarations should use uniform initialization syntax.

Suggested change
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
GstElement video_dec{};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline{};

Copilot uses AI. Check for mistakes.
Comment on lines +1270 to +1272
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent initialization syntax. Lines 1268-1269 use uniform initialization (braces), while lines 1270-1271 use traditional empty braces. For consistency and adherence to modern C++ guidelines, all GstElement declarations should use uniform initialization syntax.

Suggested change
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
GstElement video_dec{};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline{};

Copilot uses AI. Check for mistakes.
Comment on lines +1323 to +1325
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent initialization syntax. Lines 1321-1322 use uniform initialization (braces), while lines 1323-1324 use traditional empty braces. For consistency and adherence to modern C++ guidelines, all GstElement declarations should use uniform initialization syntax.

Suggested change
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
GstElement video_dec{};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline{};
gst_element_pipeline.object.name = (gchar *)"testpipeline";

Copilot uses AI. Check for mistakes.
if( element )
GstState current{};
GstState pending{};
GstClockTime timeout = 0;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timeout variable is set to 0 and never changes. Consider using constexpr for compile-time constants that improve code clarity and signal intent. This is a modern C++ best practice for values that are known at compile time and won't change.

Copilot generated this review using guidance from repository custom instructions.
Copilot AI review requested due to automatic review settings January 23, 2026 17:01
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Comment on lines +1323 to +1324
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of brace initialization for GstElement. On line 1321, video_sink uses brace initialization {}, but on line 1323, video_dec uses the old-style empty initialization = {}. For consistency with modern C++ practices and the rest of the new test code, both should use uniform brace initialization without the equals sign.

Copilot generated this review using guidance from repository custom instructions.
Comment on lines +1319 to +1364
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_PausedState)
{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PAUSED};
GstStructure valid_stats{0x5678};
GValue rendered_value{};
GValue dropped_value{};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PAUSED),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.WillOnce(DoAll(
SetArgPointee<2>(&valid_stats),
Return()
));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("rendered")))
.WillOnce(Return(&rendered_value));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("dropped")))
.WillOnce(Return(&dropped_value));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&rendered_value))
.WillOnce(Return(2000ULL));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&dropped_value))
.WillOnce(Return(10ULL));

EXPECT_CALL(*g_mockGStreamer, gst_structure_free(&valid_stats));

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_NE(result, nullptr);
EXPECT_EQ(result->rendered, 2000ULL);
EXPECT_EQ(result->dropped, 10ULL);
}
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for the edge case where both gst_structure_get_value calls return NULL for both "rendered" and "dropped" fields. In this scenario (lines 2626-2638), the function would still return a pointer to playbackQuality but with potentially stale/uninitialized values from previous calls.

A test should verify this behavior: when stats structure exists but contains no valid "rendered" or "dropped" values, the function should either return nullptr or ensure the returned structure has well-defined values (e.g., zeros or previous valid values).

Copilot uses AI. Check for mistakes.
Comment on lines 998 to +1005
TEST_F(InterfacePlayerTests, GstGetVideoPlaybackQuality_StatsNull)
{
GstElement video_sink = {};
GstStructure stats = {1};
GstElement video_sink{};
GstStructure stats{1};
mPlayerContext->video_sink = &video_sink;
GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result,nullptr);
EXPECT_EQ(result, nullptr);
}
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test GstGetVideoPlaybackQuality_StatsNull is incomplete and doesn't actually test the scenario implied by its name. The test sets up a video_sink and creates a stats structure, but doesn't mock gst_element_get_state to return a valid state (PLAYING or PAUSED). Without this mock, the function will likely fail at the state check and return nullptr, but this doesn't test the "stats null" scenario.

This test should either be updated to properly test the case where g_object_get returns a null stats pointer (after passing all state checks), or it should be renamed to reflect what it actually tests (e.g., missing pipeline causing early return).

Copilot uses AI. Check for mistakes.
Comment on lines +1015 to 1422
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_NoStatsCallDuringStateTransition)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PAUSED};
GstState pending_state{GST_STATE_PLAYING};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(pending_state),
Return(GST_STATE_CHANGE_ASYNC)
));
EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_NoStatsCallAtReadyState
* @brief Verify that stats query is skipped when pipeline is in READY state
*
* Pipeline in READY state is not suitable for playback quality metrics.
* The function should return NULL without calling g_object_get for stats.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_NoStatsCallAtReadyState)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_READY};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_READY),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_NoStatsCallAtNullState
* @brief Verify that stats query is skipped when pipeline is in NULL state
*
* Pipeline in NULL state means the pipeline is stopped.
* The function should return NULL without calling g_object_get for stats.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_NoStatsCallAtNullState)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_NULL};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_NULL),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_NoStatsCallAtVoidPendingState
* @brief Verify that stats query is skipped when pipeline is in VOID_PENDING state
*
* Pipeline in VOID_PENDING state is in an invalid or uninitialized state.
* The function should return NULL without calling g_object_get for stats.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_NoStatsCallAtVoidPendingState)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_VOID_PENDING};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_VOID_PENDING),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_NoStatsCallWhenElementIsNull
* @brief Verify that stats query is skipped when element is NULL
*
* Pipeline in GST_STATE_PLAYING with GST_STATE_CHANGE_SUCCESS is a valid state.
* Still the function should return NULL without calling g_object_get for stats,
* since element is NULL.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_NoStatsCallWhenElementIsNull)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;
//Do not set video_dec, so that element will be NULL when GetVideoPlaybackQuality() reads it

GstState current_state{GST_STATE_PLAYING};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_VOID_PENDING),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_PlayingState
* @brief Verify successful stats retrieval when pipeline is in PLAYING state
*
* When pipeline is in PLAYING state with valid stats available,
* should successfully retrieve and return quality metrics.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_PlayingState)
{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PLAYING};
GstStructure valid_stats{0x1234};
GstStructure *stats = &valid_stats;
GValue rendered_value{};
GValue dropped_value{};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PLAYING),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.WillOnce(DoAll(
SetArgPointee<2>(stats),
Return()
));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("rendered")))
.WillOnce(Return(&rendered_value));
EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("dropped")))
.WillOnce(Return(&dropped_value));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&rendered_value))
.WillOnce(Return(1000ULL));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&dropped_value))
.WillOnce(Return(5ULL));

EXPECT_CALL(*g_mockGStreamer, gst_structure_free(&valid_stats));

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_NE(result, nullptr);
EXPECT_EQ(result->rendered, 1000ULL);
EXPECT_EQ(result->dropped, 5ULL);
}

/**
* @test GetVideoPlaybackQuality_PlayingStateWithRenderedNull
* @brief Verify successful retrieval of dropped stats when
* pipeline is in PLAYING state and rendered stats returns NULL
*
* When pipeline is in PLAYING state but rendered stats returns NULL,
* should successfully retrieve dropped stats and return quality metrics.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_PlayingStateWithRenderedNull)
{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PLAYING};
GstStructure valid_stats{0x1234};
GstStructure *stats = &valid_stats;
GValue rendered_value{};
GValue dropped_value{};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PLAYING),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.WillOnce(DoAll(
SetArgPointee<2>(stats),
Return()
));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("rendered")))
.WillOnce(Return((GValue*)NULL));
EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("dropped")))
.WillOnce(Return(&dropped_value));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&rendered_value))
.Times(0);

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&dropped_value))
.WillOnce(Return(5ULL));

EXPECT_CALL(*g_mockGStreamer, gst_structure_free(&valid_stats));

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_NE(result, nullptr);
EXPECT_EQ(result->dropped, 5ULL);
}

/**
* @test GetVideoPlaybackQuality_PlayingStateWithDroppedNull
* @brief Verify successful retrieval of rendered stats when
* pipeline is in PLAYING state and dropped stats returns NULL
*
* When pipeline is in PLAYING state but dropped stats returns NULL,
* should successfully retrieve rendered stats and return quality metrics.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_PlayingStateWithDroppedNull)
{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PLAYING};
GstStructure valid_stats{0x1234};
GstStructure *stats = &valid_stats;
GValue rendered_value{};
GValue dropped_value{};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PLAYING),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.WillOnce(DoAll(
SetArgPointee<2>(stats),
Return()
));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("rendered")))
.WillOnce(Return(&rendered_value));
EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("dropped")))
.WillOnce(Return((GValue *)NULL));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&dropped_value))
.Times(0);

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&rendered_value))
.WillOnce(Return(1000ULL));

EXPECT_CALL(*g_mockGStreamer, gst_structure_free(&valid_stats));

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_NE(result, nullptr);
EXPECT_EQ(result->rendered, 1000ULL);
}

/**
* @test GetVideoPlaybackQuality_PausedState
* @brief Verify successful stats retrieval when pipeline is in PAUSED state
*
* When pipeline is in PAUSED state with valid stats available,
* should successfully retrieve and return quality metrics.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_PausedState)
{
GstElement video_sink{};
mPlayerContext->video_sink = &video_sink;
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PAUSED};
GstStructure valid_stats{0x5678};
GValue rendered_value{};
GValue dropped_value{};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PAUSED),
Return(GST_STATE_CHANGE_SUCCESS)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.WillOnce(DoAll(
SetArgPointee<2>(&valid_stats),
Return()
));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("rendered")))
.WillOnce(Return(&rendered_value));

EXPECT_CALL(*g_mockGStreamer, gst_structure_get_value(&valid_stats, StrEq("dropped")))
.WillOnce(Return(&dropped_value));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&rendered_value))
.WillOnce(Return(2000ULL));

EXPECT_CALL(*g_mockGStreamer, g_value_get_uint64(&dropped_value))
.WillOnce(Return(10ULL));

EXPECT_CALL(*g_mockGStreamer, gst_structure_free(&valid_stats));

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_NE(result, nullptr);
EXPECT_EQ(result->rendered, 2000ULL);
EXPECT_EQ(result->dropped, 10ULL);
}

/**
* @test GetVideoPlaybackQuality_StateChangeFailure
* @brief Verify that stats query is skipped when state change returns FAILURE
*
* When gst_element_get_state() returns GST_STATE_CHANGE_FAILURE,
* indicating a failed state transition, the function should return NULL
* without attempting to access stats property.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_StateChangeFailure)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_NULL};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_NULL),
Return(GST_STATE_CHANGE_FAILURE)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}

/**
* @test GetVideoPlaybackQuality_StateChangeNoPreroll
* @brief Verify that stats query is skipped when state change returns NO_PREROLL
*
* When gst_element_get_state() returns GST_STATE_CHANGE_NO_PREROLL,
* indicating the pipeline cannot produce data in PAUSED state (typical for live sources),
* the function should return NULL without attempting to access stats property.
*/
TEST_F(InterfacePlayerTests, GetVideoPlaybackQuality_StateChangeNoPreroll)
{
GstElement gst_element_pipeline = {.object = {.name = (gchar *)"testpipeline"}};
mPlayerContext->pipeline = &gst_element_pipeline;

GstState current_state{GST_STATE_PAUSED};

EXPECT_CALL(*g_mockGStreamer, gst_element_get_state(&gst_element_pipeline, _, _, _))
.WillOnce(DoAll(
SetArgPointee<1>(current_state),
SetArgPointee<2>(GST_STATE_PAUSED),
Return(GST_STATE_CHANGE_NO_PREROLL)
));

EXPECT_CALL(*g_mockGLib, g_object_get(_, StrEq("stats"), Matcher<GstStructure **>(_)))
.Times(0);

GstPlaybackQualityStruct* result = mInterfaceGstPlayer->GetVideoPlaybackQuality();
EXPECT_EQ(result, nullptr);
}
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for the case where g_object_get is called successfully but returns a null stats pointer. In the production code (lines 2622-2643), if g_object_get sets stats to nullptr, the code logs "Failed to get sink stats" and returns nullptr. This is an important error path that should be covered by a test case.

Add a test that:

  1. Sets up valid pipeline state (PLAYING/PAUSED with GST_STATE_CHANGE_SUCCESS)
  2. Sets up valid video_sink or video_dec element
  3. Mocks g_object_get to leave stats as nullptr (do not use SetArgPointee)
  4. Verifies the function returns nullptr without calling gst_structure_get_value

Copilot uses AI. Check for mistakes.
Comment on lines +1161 to +1162
GstElement video_dec = {};
mPlayerContext->video_dec = &video_dec;
Copy link

Copilot AI Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inconsistent use of brace initialization for GstElement. On line 1159, video_sink uses brace initialization {}, but on line 1161, video_dec uses the old-style empty initialization = {}. For consistency with modern C++ practices and the rest of the new test code, both should use uniform brace initialization without the equals sign.

Copilot generated this review using guidance from repository custom instructions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants