RDKEVD-8726: Audio Remains Muted and Volume Reports 0 After Standby Reboot When Panel Is Woken Up via FFV - #278
Conversation
There was a problem hiding this comment.
Pull request overview
This PR is scoped around adding diagnostic logging and adjusting log verbosity in the DeviceSettings audio and capability-loading paths, likely intended to help investigate RDKEVD-8726 (“Audio remains muted / volume reports 0 after standby reboot”).
Changes:
- Added several new debug/trace logs in
dsAudio.carounddsSetAudioLevelloading and ducking volume application. - Replaced DS logger macros (
INT_*) with directprintfcalls in capability loading and audio config code paths. - Changed DS logger defaults/mappings in
dslogger.h(default log level andINT_INFOseverity).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| rpc/srv/dsAudio.c | Adds extra logging around audio level initialization and ducking volume application. |
| ds/manager.cpp | Switches multiple log statements to printf in DL symbol loading and Manager lifecycle functions. |
| ds/include/dslogger.h | Changes default DS log level and remaps INT_INFO to log at DEBUG severity. |
| ds/audioOutputPortConfig.cpp | Adds printf diagnostics in supported-type enumeration and config dumping/loading paths. |
Suppressed comments (17)
ds/manager.cpp:108
printfusage here is inconsistent with the rest of the module’s logging (which usesINT_*/ds_log) and can bypass log routing (e.g.,DS_RegisterForLog). PreferINT_INFOfor these informational messages.
printf("Entering capabilityType = 0x%08X", capabilityType);
dlerror(); // clear old error
pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
if (nullptr == pDLHandle) {
ds/manager.cpp:193
- This
printfbypasses the DS logging infrastructure (INT_*/ds_log). PreferINT_INFOso the message follows the normal logging path and formatting.
printf("Exiting ...");
ds/manager.cpp:317
- These
printfcalls bypassds_log/INT_*and also omit newlines, which can make logs hard to correlate and may not flush promptly depending on stdout buffering. PreferINT_INFOfor consistent routing/formatting.
printf("Exiting ... with thread id %lu",pthread_self());
}
void Manager::load()
{
printf("Enter function");
ds/manager.cpp:346
- This
printfbypasses the DS logging infrastructure (INT_*/ds_log). PreferINT_INFOso logs include standard metadata (file/line/function) and go through the registered log callback.
printf("Entering ... count %d with thread id %lu",IsInitialized,pthread_self());
ds/manager.cpp:363
- This
printfbypasses the DS logging infrastructure (INT_*/ds_log). PreferINT_INFOfor consistent routing/formatting and to avoid stdout buffering issues.
printf("Exiting ... with thread %lu",pthread_self());
ds/audioOutputPortConfig.cpp:121
dumpconfig()switched fromINT_*logging toprintf, which bypasses the DS logging callback/formatting and writes to stdout (often undesired on embedded targets). Keeping these onINT_*preserves consistent log routing and severity control.
if (nullptr == config) {
printf("\nAudio config is NULL");
return;
}
if ( -1 == access("/opt/dsMgrDumpDeviceConfigs", F_OK) ) {
ds/audioOutputPortConfig.cpp:127
- This debug output should go through the DS logger (
INT_INFO) instead of stdout so it can be routed/filtered consistently (and includes file/line/function context).
printf("\n\n=============== Starting to Dump Audio Configs ===============\n");
ds/audioOutputPortConfig.cpp:137
- These
printfstatements should useINT_INFOso the output is consistent with other DS logs and can be redirected viaDS_RegisterForLog.
const dsAudioTypeConfig_t *typeCfg = &(config->pKConfigs[i]);
printf("\ntypeCfg->typeId = %d", typeCfg->typeId);
printf("\ntypeCfg->name = %s", typeCfg->name);
printf("\ntypeCfg->numSupportedEncodings = %zu", typeCfg->numSupportedEncodings);
printf("\ntypeCfg->numSupportedCompressions = %zu", typeCfg->numSupportedCompressions);
ds/audioOutputPortConfig.cpp:143
- Use
INT_ERRORhere for consistency with the rest of the module’s logging (and to avoid writing to stdout).
printf("\nkAudioConfigs is NULL");
ds/audioOutputPortConfig.cpp:152
- Use
INT_INFOhere so the port dump follows normal DS logging routing/formatting.
printf("\nportCfg->id.type = %d", portCfg->id.type);
printf("\nportCfg->id.index = %d", portCfg->id.index);
ds/audioOutputPortConfig.cpp:157
- Use
INT_ERRORhere for consistency with the rest of the module’s error logging (and to avoid writing to stdout).
printf("\nkAudioPorts is NULL");
ds/audioOutputPortConfig.cpp:160
- This completion message should use
INT_INFOrather thanprintfso it follows standard DS logging behavior and can be filtered/routed.
printf("\n\n=============== Dump Audio Configs done ===============\n");
ds/audioOutputPortConfig.cpp:168
- This should use DS logging (
INT_INFO) instead ofprintfto avoid stdout noise and keep consistent log routing/formatting.
printf("\nEnter function");
ds/audioOutputPortConfig.cpp:192
- This message should use
INT_INFOrather thanprintfso it can be routed/filtered consistently via the DS logger.
printf("\nUsing '%s' config", dynamicAudioConfigs ? "dynamic" : "static");
ds/audioOutputPortConfig.cpp:212
- Prefer
INT_INFOoverprintfso this diagnostic goes through the DS logger (and can be routed viaDS_RegisterForLog).
printf("\nAudio Config[%p] ConfigSize[%d] Ports[%p] PortSize[%d]",
configuration.pKConfigs,
configSize,
configuration.pKPorts,
portSize);
ds/audioOutputPortConfig.cpp:258
- These status messages should go through
INT_INFO/INT_ERROR(instead of stdout) to keep logging consistent and avoid mixing runtime logs with config dumps.
printf("\nAudio Configs loaded successfully");
}
else {
printf("\nAudio Configs loading failed");
}
ds/audioOutputPortConfig.cpp:263
- This function exit message should use
INT_INFO(notprintf) so it follows standard DS logging behavior.
printf("\nExit function");
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| #ifndef DS_LOG_LEVEL | ||
| #define DS_LOG_LEVEL ERROR_LEVEL | ||
| #define DS_LOG_LEVEL DEBUG_LEVEL | ||
| #endif | ||
|
|
||
| #define INT_INFO(FORMAT, ...) ds_log(INFO_LEVEL, fileName(__FILE__), __LINE__, __FUNCTION__, FORMAT, ##__VA_ARGS__ ) | ||
| #define INT_INFO(FORMAT, ...) ds_log(DEBUG_LEVEL, fileName(__FILE__), __LINE__, __FUNCTION__, FORMAT, ##__VA_ARGS__ ) |
| if ((nullptr == pDLHandle) || (nullptr == symbols)) { | ||
| INT_ERROR("Invalid DL Handle or symbolsPtr"); | ||
| printf("Invalid DL Handle or symbolsPtr"); | ||
| } | ||
| else { | ||
| INT_INFO("numberOfSymbols = %d",numberOfSymbols); | ||
| printf("numberOfSymbols = %d",numberOfSymbols); |
| for (std::vector<AudioOutputPortType>::const_iterator it = _aPortTypes.begin(); it != _aPortTypes.end(); it++) { | ||
| printf("\nYESH: supportedTypes _aPortTypes *it %s enabled %d", it->toString().c_str(), it->isEnabled()); | ||
| if (it->isEnabled()) { |
| #ifndef DS_LOG_LEVEL | ||
| #define DS_LOG_LEVEL ERROR_LEVEL | ||
| #define DS_LOG_LEVEL DEBUG_LEVEL | ||
| #endif |
ebbc9b5 to
c2dbb99
Compare
https://ccp.sys.comcast.net/browse/RDKEVD-6226 RDKEVD-8726:Audio Remains Muted and Volume Reports 0 After Standby Reboot When Panel Is Woken Up via FFV https://ccp.sys.comcast.net/browse/RDKEVD-8726
c2dbb99 to
0f1763e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces production-unfriendly debug/printf logging (including unprofessional markers) and alters logging severity semantics in a way that can disrupt log filtering and operational behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (5)
ds/include/dslogger.h:52
- INT_INFO is currently logged with DEBUG_LEVEL, and DS_LOG_LEVEL defaults to DEBUG_LEVEL. This mislabels Info messages as Debug (and can break severity-based filtering in DS_RegisterForLog), while also enabling debug logging by default in all builds that don't override DS_LOG_LEVEL.
#ifndef DS_LOG_LEVEL
#define DS_LOG_LEVEL DEBUG_LEVEL
#endif
#define INT_INFO(FORMAT, ...) ds_log(DEBUG_LEVEL, fileName(__FILE__), __LINE__, __FUNCTION__, FORMAT, ##__VA_ARGS__ )
ds/manager.cpp:79
- Using printf() here bypasses the dslogger callback/formatting, lacks newlines (likely to be buffered/concatenated), and can introduce build warnings/errors due to missing includes in this TU. Use the existing INT_* logging macros instead.
if ((nullptr == pDLHandle) || (nullptr == symbols)) {
printf("Invalid DL Handle or symbolsPtr");
}
else {
printf("numberOfSymbols = %d",numberOfSymbols);
ds/manager.cpp:108
- These printf() calls bypass the device settings logging framework and omit newlines, which can make logs hard to correlate and may be buffered/lost depending on stdout configuration. Prefer INT_INFO/INT_DEBUG so logs are routed consistently (and can be suppressed when needed).
This issue also appears on line 345 of the same file.
printf("Entering capabilityType = 0x%08X", capabilityType);
dlerror(); // clear old error
pDLHandle = dlopen(RDK_DSHAL_NAME, RTLD_LAZY);
if (nullptr == pDLHandle) {
ds/manager.cpp:349
- This block introduces additional printf() logging (no newline, bypasses dslogger) and also reduces readability via one-line lock scope and unbraced if statements. Using INT_INFO and adding braces keeps logging consistent and makes the deinit sequencing easier to audit.
{std::lock_guard<std::mutex> lock(gManagerInitMutex);
printf("Entering ... count %d with thread id %lu",IsInitialized,pthread_self());
if(IsInitialized>0)IsInitialized--;
if (0 == IsInitialized) {
ds/audioOutputPortConfig.cpp:123
- Use the existing dslogger macros instead of printf() so this informational message can be filtered/suppressed consistently and include a terminating newline for readability.
if ( -1 == access("/opt/dsMgrDumpDeviceConfigs", F_OK) ) {
printf("\nDumping of Device configs is disabled");
return;
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
| if (nullptr == config) { | ||
| INT_ERROR("Audio config is NULL"); | ||
| printf("\nAudio config is NULL"); | ||
| return; | ||
| } |
https://ccp.sys.comcast.net/browse/RDKEVD-8726