VPLAY-11719 Honor subtitles/CC "sub-type" preference #735
Conversation
* PrivateInstanceAAMP::SavePreferredTextLanguages() accepts "sub-type" preference, which StreamAbstractionAAMP_HLS:: SelectPreferredTextTrack() uses as part of its track selection scoring.
kd-syna
changed the title
There was a problem hiding this comment.
Pull request overview
This PR adds support for honoring the "sub-type" preference when selecting subtitle/closed caption tracks in HLS streams. When applications specify a preference for either "SUBTITLES" (out-of-band) or "CLOSED-CAPTIONS" (in-band), the player will now prioritize matching tracks accordingly.
Key Changes
- Added caching of the "sub-type" preference from JSON configuration
- Enhanced HLS text track selection to score tracks based on sub-type match
- Fixed incorrect comment for
preferredTextLabelStringmember variable
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| priv_aamp.h | Added new member variable preferredTextSubTypeString to store the sub-type preference; corrected comment for preferredTextLabelString |
| priv_aamp.cpp | Implemented parsing of "sub-type" field from JSON preferences and caching it in the new member variable |
| fragmentcollector_hls.cpp | Enhanced SelectPreferredTextTrack() to score tracks based on sub-type match (SUBTITLES vs CLOSED-CAPTIONS) using AAMP_TYPE_SCORE; added sub-type to preference logging and CC status to track selection logging |
| AampDefine.h | Code formatting improvement (alignment of score constant definitions) |
| // Only if it IS specified AND matches, score sub-type match. | ||
| // If a preference is not set (or is not recognised) it doesn't match. | ||
| if ( (!track.isCC && aamp->preferredTextSubTypeString == "SUBTITLES") | ||
| || ( track.isCC && aamp->preferredTextSubTypeString == "CLOSED-CAPTIONS")) | ||
| { | ||
| // This is intentional re-use of the AAMP_TYPE_SCORE weighting, as | ||
| // specifying name AND sub-type in the same preference profile is not supported. | ||
| score += AAMP_TYPE_SCORE; | ||
| } |
There was a problem hiding this comment.
The new sub-type matching logic lacks test coverage. The existing test in test/utests/tests/StreamAbstractionAAMP_HLS/FunctionalTests.cpp::SelectPreferredTextTrack should be extended to verify that:
- When
preferredTextSubTypeStringis set to "SUBTITLES", subtitle tracks (isCC=false) receive the AAMP_TYPE_SCORE bonus - When
preferredTextSubTypeStringis set to "CLOSED-CAPTIONS", CC tracks (isCC=true) receive the AAMP_TYPE_SCORE bonus - When
preferredTextSubTypeStringis empty or has an unrecognized value, no bonus is awarded
This is important to ensure the scoring behavior works correctly with various preference combinations.
| std::string preferredTextTypeString; /**< String value for text type */ | ||
| std::string preferredTextLabelString; /**< String value for text type */ | ||
| std::string preferredTextLabelString; /**< String value for label */ | ||
| std::string preferredTextSubTypeString; /**< String value for sub-type */ |
There was a problem hiding this comment.
The comment for preferredTextSubTypeString could be more descriptive to clarify the expected values. Consider:
std::string preferredTextSubTypeString; /**< String value for sub-type ("SUBTITLES" or "CLOSED-CAPTIONS") */This helps developers understand the valid values for this preference.
| std::string preferredTextSubTypeString; /**< String value for sub-type */ | |
| std::string preferredTextSubTypeString; /**< String value for text track sub-type (e.g., "SUBTITLES" or "CLOSED-CAPTIONS") */ |
| // This is intentional re-use of the AAMP_TYPE_SCORE weighting, as | ||
| // specifying name AND sub-type in the same preference profile is not supported. | ||
| score += AAMP_TYPE_SCORE; |
There was a problem hiding this comment.
[nitpick] While the comment states that "specifying name AND sub-type in the same preference profile is not supported," the code doesn't prevent this scenario. Both name (line 7643-7648) and sub-type (7652-7658) can add AAMP_TYPE_SCORE, potentially allowing both to score if the configuration includes both preferences. Consider if there should be mutual exclusion logic or if the comment should be updated to reflect that both can contribute to scoring if configured (though this might not be the intended use case).
Reason for Change: When an app specifies the CC/subtitles "sub-type" preference, use that to choose the most appropriate text track.
Summary of Changes:
SavePreferredTextLanguage() will cache the value of the "sub-type" preference from the JSON preferences document.
SelectPreferredTextTrack() will then prefer an in-band CC track when "sub-type" is "CLOSED-CAPTIONS" and prefer an out-of-band subtitle track when "sub-type" is "SUBTITLES". If the "sub-type" is neither of those values, it is treated as if it was not specified.
Risk: Low