Feature/vplay 11316 cc fixes for federated #713

anshephe · 2025-11-27T11:54:00Z

Branch off of feature/RDK-59937_federated_2511 for testing, with addition of VPLAY-11846, VPLAY-11719, VPLAY-11707

* PrivateInstanceAAMP::SavePreferredTextLanguages() accepts "sub-type" preference, which StreamAbstractionAAMP_HLS:: SelectPreferredTextTrack() uses as part of its track selection scoring.

Reason for change: Subtiles not initially muted when SetCCStatus called prior to load Risks: Low Test Procedure: Test subtitles/closed captions are muted correctly, e.g. on tune Priority: P1 Signed-off-by: anshephe <[email protected]>

Co-authored-by: Copilot <[email protected]>

Fixed location of call to SetSubtitleMuteInternal() in SetCCStatusInternal() Updated utests

Co-authored-by: Copilot <[email protected]>

…e/VPLAY-11316_cc_fixes_for_federated

…nto feature/VPLAY-11316_cc_fixes_for_federated

Copilot

Pull request overview

This PR implements fixes for closed captioning (CC) functionality in a federated environment, addressing issues VPLAY-11846, VPLAY-11719, and VPLAY-11707. The changes focus on improving CC state management, text track selection logic, and thread safety.

Key changes:

Thread safety improvements by converting video_muted and subtitles_muted to atomic variables
Refactored CC status handling to properly differentiate between pre-tune and post-tune states
Enhanced text track selection algorithm with improved scoring and language preference handling
Better handling of edge cases like empty language strings and missing audio track languages

Reviewed changes

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

Show a summary per file

File	Description
priv_aamp.h	Added `FindTextTrackIndex` method declaration and converted `video_muted`/`subtitles_muted` to atomic types for thread safety
priv_aamp.cpp	Updated atomic variable usage with `.load()` calls, refactored `SetCCStatusInternal` logic, added `FindTextTrackIndex` helper, and improved language list sanitization
test/utests/tests/PrivAampTests/PrivAampTests.cpp	Renamed and restructured CC status tests to cover pre-tune, post-tune, and video mute interaction scenarios
fragmentcollector_hls.h	Added `NotifyTextTrackChanges` method declaration
fragmentcollector_hls.cpp	Improved audio track language fallback handling, refactored text track selection with enhanced scoring algorithm, and separated text track change notifications into dedicated method

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-27T11:58:35Z

fragmentcollector_hls.cpp

+				// Select preferred text track based on user language preferences
+				TextTrackInfo selectedTextTrack;
+				if (SelectPreferredTextTrack(selectedTextTrack))
+				{
+					AAMPLOG_INFO("Selected text track - lang:%s, name:%s, rendition:%s",
+								 selectedTextTrack.language.c_str(),
+								 selectedTextTrack.name.c_str(),
+								 selectedTextTrack.rendition.c_str());
+					aamp->SetPreferredTextTrack(std::move(selectedTextTrack));
+				}
+				else
+				{
+					AAMPLOG_WARN("No text track matched user preferences, will use default selection");
+				}
+
+				// Configure Subtitle track for the playback
+				ConfigureTextTrack();
+
+				// Check for text track changes and notify
+				NotifyTextTrackChanges();


[nitpick] The function SelectPreferredTextTrack is called before ConfigureTextTrack, and the selected track is stored via aamp->SetPreferredTextTrack(). However, ConfigureTextTrack() (line 3469) retrieves this track and converts it to currentTextTrackProfileIndex. Then NotifyTextTrackChanges() (line 3472) checks if the track changed and sends a notification.

The issue is that for a new tune (first time), aamp->mCurrentTextTrackIndex will be -1 (initial/uninitialized), and currentTextTrackProfileIndex will be set to the newly selected track index. According to the logic in NotifyTextTrackChanges() (line 7341), the condition aamp->mCurrentTextTrackIndex != -1 will be false, so no notification will be sent on the first tune. This means the first text track selection won't trigger a notification event, which may or may not be intended behavior. Consider if an initial text track selection should also trigger a notification to inform the application.

Copilot · 2025-11-27T11:58:35Z

priv_aamp.cpp

+ * @param[in] tracks Vector of text tracks to search
+ * @param[in] target Track to find


[nitpick] The documentation for this function uses inconsistent parameter formatting. The @param[in] tags use different styles for the parameter descriptions:

Line 12326: "Vector of text tracks to search" (no hyphen)

Line 12327: "Track to find" (no hyphen)

while the declaration comment on line 12328 uses "Index of track (0-based), or -1 if not found".

For consistency with the rest of the codebase, consider using a consistent format. The function documentation in priv_aamp.h (lines 603-610) uses hyphens after parameter names, e.g., @param[in] tracks - Vector of text tracks...

Suggested change

* @param[in] tracks Vector of text tracks to search

* @param[in] target Track to find

* @param[in] tracks - Vector of text tracks to search

* @param[in] target - Track to find

Copilot · 2025-11-27T11:58:36Z

priv_aamp.cpp

+					{
+						// Skip normalization for empty strings
+						if (lang.empty())
+						{


[nitpick] The empty string check prevents calling Getiso639map_NormalizeLanguageCode with an empty string, which is good defensive programming. However, empty strings in the language list may indicate a data quality issue upstream. Consider logging a warning when an empty language string is encountered to help identify where invalid data is being introduced into the language list. This would make debugging configuration issues easier.

Suggested change

{

{

AAMPLOG_WARN("SanitizeLanguageList: Encountered empty language string in language list. This may indicate a data quality issue upstream.");

Copilot · 2025-11-27T11:58:36Z

priv_aamp.cpp

 				sink->SetVideoZoom(zoom_mode);
-				AAMPLOG_INFO("SetVideoMute video_muted %d mApplyCachedVideoMute %d", video_muted, mApplyCachedVideoMute);
-				sink->SetVideoMute(video_muted);
+				AAMPLOG_INFO("SetVideoMute video_muted %d mApplyCachedVideoMute %d", video_muted.load(), mApplyCachedVideoMute);


The logging statement includes mApplyCachedVideoMute in the message but this variable is only checked in the conditional below. If mApplyCachedVideoMute is false, the log message incorrectly suggests it's being considered when it's not actually being applied. Consider moving the log statement inside the if (mApplyCachedVideoMute) block or adjusting the message to clarify when the cached value is actually being applied.

Copilot · 2025-11-27T11:58:36Z

priv_aamp.cpp

 			{ // has sidecar data
 				mpStreamAbstractionAAMP->MuteSidecarSubtitles(mute_subtitles_applied);
 			}
+			SetSubtitleMuteInternal(mute_subtitles_applied);


The logic for calling SetSubtitleMuteInternal has been moved inside the OOB (out-of-band) subtitles branch. Previously, SetSubtitleMuteInternal was always called regardless of the subtitle type. Now it's only called for OOB subtitles when !isGstSubtecEnabled is false. This means that for inband CC (mIsInbandCC == true or !isGstSubtecEnabled == true), SetSubtitleMuteInternal is no longer being called. Verify this is intentional, as it could potentially break subtitle muting for inband CC scenarios.

Copilot · 2025-11-27T11:58:37Z

fragmentcollector_hls.cpp

+	if (bestScore == 0)
+	{
+		AAMPLOG_WARN("No suitable text track found");
+	}
+


The variable bestScore is initialized to 0, and the function returns (bestScore > 0). However, every track gets a base score of 1 (line 7607). This means that if there are any available tracks at all, bestScore will always be at least 1, making the check on line 7657 (if (bestScore == 0)) unreachable. The warning "No suitable text track found" will never be logged. Consider either removing this dead code or adjusting the logic if a score of 1 (base score only, with no preference matches) should be considered "no suitable track".

Suggested change

if (bestScore == 0)

{

AAMPLOG_WARN("No suitable text track found");

}

…type-pref' into feature/VPLAY-11316_cc_fixes_for_federated

Pull from Feature/rdk 59937 federated 2511

DomSyna and others added 30 commits November 19, 2025 11:25

VPLAY-11618 Handle empty language in HLS manifest

312b2bd

VPLAY-11618 Handle empty language in HLS manifest

c973919

VPLAY-11719 Accept IB/OOB subtitle preference

8eac239

* PrivateInstanceAAMP::SavePreferredTextLanguages() accepts "sub-type" preference, which StreamAbstractionAAMP_HLS:: SelectPreferredTextTrack() uses as part of its track selection scoring.

VPLAY-11618 Handle empty language in HLS manifest

43bf95c

VPLAY-11618 Handle empty language in HLS manifest

5c2c374

VPLAY-11719 Add temporary test code.

4a5fa53

Whitespace cleanup

1ec4e04

Co-authored-by: Copilot <[email protected]>

VPLAY-11618 Handle empty language in HLS manifest

ad79145

VPLAY-11618 Handle empty language in HLS manifest

f65b983

VPLAY-11618 Handle empty language in HLS manifest

5991094

VPLAY-11618 Handle empty language in HLS manifest

a6a0e3f

Apply suggestion from @Copilot

b650add

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

b88ce85

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

4d75445

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

ba9217f

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

e517fe1

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

d555416

Co-authored-by: Copilot <[email protected]>

Apply suggestion from @Copilot

5e1c92c

Co-authored-by: Copilot <[email protected]>

VPLAY-11618 Handle empty language in HLS manifest

9aaf020

Apply suggestion from @Copilot

a5f81d9

Co-authored-by: Copilot <[email protected]>

VPLAY-11719 test code

9fd722d

VPLAY-11618 Handle empty language in HLS manifest

ebcc7f0

VPLAY-11719 Typo

f0aea51

Added load() call to atomic variables

fbf9026

Fixed location of call to SetSubtitleMuteInternal() in SetCCStatusInternal() Updated utests

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11618-text_lang

e79d7f0

Merge branch 'dev_sprint_25_2' into feature/VPLAY-11707

4496533

Apply suggestions from code review

262a3d7

Co-authored-by: Copilot <[email protected]>

VPLAY-11618 Handle empty language in HLS manifest

af8b9bc

VPLAY-11618 Handle empty language in HLS manifest

be634e1

DomSyna and others added 8 commits November 24, 2025 15:42

VPLAY-11618 Handle empty language in HLS manifest

35fb263

Updated following review comments

a2d8fd5

VPLAY-11618 Handle empty language in HLS manifest

3dbeaf6

Update fragmentcollector_hls.cpp

d104595

Co-authored-by: Copilot <[email protected]>

VPLAY-11719 Add some logging

a87b616

VPLAY-11719 Remove testing code

6e4bd77

Merge remote-tracking branch 'origin/feature/VPLAY-11707' into featur…

b5aac2c

…e/VPLAY-11316_cc_fixes_for_federated

Merge remote-tracking branch 'origin/feature/VPLAY-11618-text_lang' i…

6d51328

…nto feature/VPLAY-11316_cc_fixes_for_federated

Copilot AI review requested due to automatic review settings November 27, 2025 11:54

Copilot started reviewing on behalf of anshephe November 27, 2025 11:54 View session

Copilot finished reviewing on behalf of anshephe November 27, 2025 11:57

Copilot AI reviewed Nov 27, 2025

View reviewed changes

kd-syna and others added 2 commits November 27, 2025 12:46

Merge remote-tracking branch 'origin/feature/VPLAY-11719_subtitle-sub…

13e9b79

…type-pref' into feature/VPLAY-11316_cc_fixes_for_federated

Merge pull request #739 from rdkcentral/feature/RDK-59937_federated_2511

06623c2

Pull from Feature/rdk 59937 federated 2511

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/vplay 11316 cc fixes for federated #713

Feature/vplay 11316 cc fixes for federated #713

anshephe commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Copilot AI Nov 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

		* @param[in] tracks Vector of text tracks to search
		* @param[in] target Track to find

	{
	{
	AAMPLOG_WARN("SanitizeLanguageList: Encountered empty language string in language list. This may indicate a data quality issue upstream.");

Feature/vplay 11316 cc fixes for federated #713

Are you sure you want to change the base?

Feature/vplay 11316 cc fixes for federated #713

Conversation

anshephe commented Nov 27, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 27, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants