RDKEMW-13310: Adding Telemetry markers in Player-Interface component - #82
Open
dp0000 wants to merge 57 commits into
Open
RDKEMW-13310: Adding Telemetry markers in Player-Interface component#82dp0000 wants to merge 57 commits into
dp0000 wants to merge 57 commits into
Conversation
Reason for change : telemetry marker implementation . Test Steps : . Signed-off by: Deepikasri Natarajan (deepikasri_n@comcast.com)
|
All contributors have signed the CLA ✍️ ✅ |
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request introduces RDK Telemetry 2.0 support to the player middleware, allowing the system to send telemetry events for monitoring and diagnostics purposes. However, the PR contains a critical unresolved merge conflict in the test script that must be addressed before merging.
Changes:
- Added new
PlayerTelemetry2class to provide telemetry support with initialization/deinitialization lifecycle management - Integrated telemetry event reporting at key failure points in the player pipeline (state change failures, buffer underflows)
- Updated CMake build configuration to conditionally compile telemetry support based on
CMAKE_TELEMETRY_2_0_REQUIREDflag
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| PlayerTelemetry2.hpp | New header file defining telemetry classes with initialization management and send methods |
| PlayerTelemetry2.cpp | Implementation of telemetry support with JSON serialization and T2 event bus integration |
| InterfacePlayerRDK.cpp | Integration of telemetry reporting at pipeline failure points, with multiple commented-out telemetry calls |
| CMakeLists.txt | Build configuration to conditionally compile telemetry sources and link telemetry library |
| test/utests/run.sh | Contains unresolved merge conflict in coverage report generation section |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4662
to
+4670
| #if 0 | ||
| PlayerTelemetry2::send("MW_BUFFERING_TIMEOUT", | ||
| privatePlayer->gstPrivateContext->numberOfVideoBuffersSent, | ||
| privatePlayer->gstPrivateContext->buffering_timeout_cnt, | ||
| privatePlayer->gstPrivateContext->rate, | ||
| isBufferingTimeoutConditionMet, | ||
| isRateCorrectionDefaultOnPlaying, | ||
| isPlayerReady); | ||
| #endif |
There was a problem hiding this comment.
These commented-out telemetry calls (lines 4662-4670) should either be implemented or removed. Leaving significant blocks of disabled code in the codebase reduces readability and maintainability.
| bool PlayerTelemetry2::send( const std::string &markerName, const char * data) | ||
| { | ||
| bool bRet = false; | ||
| if(mInitializer.isInitialized() && NULL != data) |
There was a problem hiding this comment.
Inconsistent whitespace: there are tabs after 'isInitialized()' that should be removed for consistency.
| #include "PlayerLogManager.h" | ||
|
|
||
| // Note that RDK telemetry 2.0 support is per process basic, | ||
| // this class is created to take care of un initialization of telemetry but having object as global variable |
There was a problem hiding this comment.
Spelling error: 'un initialization' should be 'uninitialization' (one word).
Suggested change
| // this class is created to take care of un initialization of telemetry but having object as global variable | |
| // this class is created to take care of uninitialization of telemetry but having object as global variable |
| /** | ||
| * @brief send - Send the telemetry data to the telemetry bus | ||
| * @param[in] markerName - Name of the marker | ||
| * @param[in] data - Data to be sent |
There was a problem hiding this comment.
The documentation for the second send method is missing a @return tag to describe what the boolean return value means, while the first send method includes this information. This is inconsistent documentation.
Suggested change
| * @param[in] data - Data to be sent | |
| * @param[in] data - Data to be sent | |
| * @return bool - true if success, false otherwise |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 17 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+34
to
+35
| PlayerTelemetry2::PlayerTelemetry2() { | ||
| PlayerTelemetry2(""); |
There was a problem hiding this comment.
PlayerTelemetry2::PlayerTelemetry2() constructs a temporary PlayerTelemetry2("") instead of delegating/initializing the current instance, so mInitializer.Init() is never called for default-constructed objects and appName remains unchanged. Use a delegating constructor initializer list (or call mInitializer.Init() directly) so telemetry is initialized for the actual object.
Suggested change
| PlayerTelemetry2::PlayerTelemetry2() { | |
| PlayerTelemetry2(""); | |
| PlayerTelemetry2::PlayerTelemetry2() | |
| : PlayerTelemetry2("") | |
| { |
Comment on lines
98
to
99
| else | ||
| MW_LOG_WARN("unknown mediaFormat %d", mDrmInfo.mediaFormat); |
There was a problem hiding this comment.
In createInitData(), the else branch only logs "unknown mediaFormat" but does not set init; initData.assign(init, ...) then uses an uninitialized pointer (UB/crash). Set a safe default string (or return/throw) in the unknown-format case before assigning.
Suggested change
| else | |
| MW_LOG_WARN("unknown mediaFormat %d", mDrmInfo.mediaFormat); | |
| else | |
| { | |
| MW_LOG_WARN("unknown mediaFormat %d", mDrmInfo.mediaFormat); | |
| initData.clear(); | |
| return; | |
| } |
|
|
||
| import re | ||
| import sys | ||
| from typing import List, TextIO |
There was a problem hiding this comment.
TextIO is imported from typing but never used. Removing the unused import avoids lint noise and keeps the script minimal.
Suggested change
| from typing import List, TextIO | |
| from typing import List |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 17 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
4128
to
+4147
| bool isVideo = false; | ||
| bool isAudioSink = false; | ||
| #ifdef PLAYER_TELEMETRY_SUPPORT | ||
| std::map<std::string, int> i; | ||
| std::map<std::string, std::string> s; | ||
| std::map<std::string, float> f; | ||
|
|
||
| // String values | ||
| s["elem"] = GST_ELEMENT_NAME(object); | ||
|
|
||
| // Integer values | ||
| i["vid"] = isVideo ? 1 : 0; | ||
| i["aud"] = isAudioSink ? 1 : 0; | ||
|
|
||
| // Float values | ||
| f["pts"] = static_cast<float>(privatePlayer->gstPrivateContext->lastKnownPTS); | ||
| f["ptsUpd"] = static_cast<float>(privatePlayer->gstPrivateContext->ptsUpdatedTimeMS); | ||
|
|
||
| PlayerTelemetry2 telemetry; | ||
| telemetry.send("MW_PTS_ERROR", i, s, f); |
There was a problem hiding this comment.
Telemetry in GstPlayer_OnGstPtsErrorCb records vid/aud before isVideo / isAudioSink are computed, so it will always send 0 for both. Move the telemetry block to after the isVideo / isAudioSink detection (or populate the maps after those booleans are set).
Comment on lines
+3289
to
+3298
| else | ||
| { | ||
| #if 0 | ||
| PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED", | ||
| formatType, | ||
| protSystemId ? protSystemId : "", | ||
| mediaType, | ||
| initDataSize); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
There are newly added #if 0 blocks with commented-out PlayerTelemetry2::send(...) calls. Keeping dead code in-tree makes it harder to maintain and can drift from the real API. Either remove these blocks or implement/guard them behind #ifdef PLAYER_TELEMETRY_SUPPORT if they’re intended for production use.
Comment on lines
+13
to
+17
| if(false == m_Initialized) | ||
| { | ||
| m_Initialized = true; | ||
| t2_init((char *)"mwplayer"); | ||
| MW_LOG_MIL("t2_init done "); |
There was a problem hiding this comment.
This code casts away const from string literals / std::string::c_str() when calling t2_init / t2_event_s. If the telemetry library writes to those buffers, this is undefined behavior; even if it doesn’t, it will typically trigger warnings. Prefer passing mutable buffers (e.g., std::string marker = markerName; t2_event_s(marker.data(), ...)) or use const_cast<char*> only when the API is known to treat inputs as read-only.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+4078
to
+4082
| f["rate"] = privatePlayer->gstPrivateContext->rate; | ||
|
|
||
| PlayerTelemetry2 telemetry; | ||
| telemetry.send("MW_BUFFER_UNDERFLOW", i, s, f); | ||
| #endif |
There was a problem hiding this comment.
New telemetry emission is added here under PLAYER_TELEMETRY_SUPPORT, but there are no tests covering that build configuration or verifying the telemetry payload/marker. Consider adding a unit/integration test (or a small injectable wrapper around t2_event_s) so regressions in the telemetry path are caught.
| bool bRet = false; | ||
| if(mInitializer.isInitialized() ) | ||
| { | ||
| cJSON *root = cJSON_CreateObject(); |
There was a problem hiding this comment.
cJSON_CreateObject() can return null (OOM). The code immediately dereferences root via cJSON_AddStringToObject, which would crash. Add a null check for root and return false (or log) if allocation fails.
Suggested change
| cJSON *root = cJSON_CreateObject(); | |
| cJSON *root = cJSON_CreateObject(); | |
| if (root == NULL) | |
| { | |
| MW_LOG_ERR("Failed to create cJSON root object for telemetry event: %s", markerName.c_str()); | |
| return false; | |
| } |
Comment on lines
+19
to
+32
| class Player_TelemetryInitializer { | ||
| private: | ||
| bool m_Initialized = false; | ||
| public: | ||
| Player_TelemetryInitializer(); | ||
| void Init(); | ||
| bool isInitialized() const; | ||
| ~Player_TelemetryInitializer(); | ||
| }; | ||
|
|
||
|
|
||
| class PlayerTelemetry2 { | ||
| private: | ||
| static Player_TelemetryInitializer mInitializer; |
There was a problem hiding this comment.
The class name Player_TelemetryInitializer is inconsistent with the surrounding codebase’s class naming (mostly CamelCase without underscores). Renaming to something like PlayerTelemetryInitializer will make it easier to discover and keep naming consistent.
Suggested change
| class Player_TelemetryInitializer { | |
| private: | |
| bool m_Initialized = false; | |
| public: | |
| Player_TelemetryInitializer(); | |
| void Init(); | |
| bool isInitialized() const; | |
| ~Player_TelemetryInitializer(); | |
| }; | |
| class PlayerTelemetry2 { | |
| private: | |
| static Player_TelemetryInitializer mInitializer; | |
| class PlayerTelemetryInitializer { | |
| private: | |
| bool m_Initialized = false; | |
| public: | |
| PlayerTelemetryInitializer(); | |
| void Init(); | |
| bool isInitialized() const; | |
| ~PlayerTelemetryInitializer(); | |
| }; | |
| class PlayerTelemetry2 { | |
| private: | |
| static PlayerTelemetryInitializer mInitializer; |
Comment on lines
+3289
to
+3298
| else | ||
| { | ||
| #if 0 | ||
| PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED", | ||
| formatType, | ||
| protSystemId ? protSystemId : "", | ||
| mediaType, | ||
| initDataSize); | ||
| #endif | ||
| } |
There was a problem hiding this comment.
This #if 0 block inside the new else branch leaves dead code and an empty runtime path. Please remove the else/disabled block, or enable it properly behind a real feature flag if the telemetry is required.
Suggested change
| else | |
| { | |
| #if 0 | |
| PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED", | |
| formatType, | |
| protSystemId ? protSystemId : "", | |
| mediaType, | |
| initDataSize); | |
| #endif | |
| } |
Comment on lines
+1293
to
+1308
| #if 0 | ||
| std::map<std::string, int> i; | ||
| std::map<std::string, std::string> s; | ||
| std::map<std::string, float> f; | ||
|
|
||
| s["elem"] = SafeName(element); | ||
| s["cur"] = gst_element_state_get_name(current); | ||
| s["pen"] = gst_element_state_get_name(pending); | ||
|
|
||
| // GstState is an enum; transmit numeric value (stable for decoding on the backend) | ||
| i["tgt"] = static_cast<int>(targetState); | ||
|
|
||
| PlayerTelemetry2 telemetry; | ||
| telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f); | ||
|
|
||
| #endif |
There was a problem hiding this comment.
This #if 0 block adds permanently disabled telemetry code. It increases maintenance burden and can silently rot. Please either remove it, or wire it up behind PLAYER_TELEMETRY_SUPPORT (and ensure it compiles) if it’s intended to be available.
Comment on lines
+11
to
+18
| void Player_TelemetryInitializer::Init() | ||
| { | ||
| if(false == m_Initialized) | ||
| { | ||
| m_Initialized = true; | ||
| t2_init((char *)"mwplayer"); | ||
| MW_LOG_MIL("t2_init done "); | ||
| } |
There was a problem hiding this comment.
Player_TelemetryInitializer::Init() is not thread-safe: m_Initialized is a plain bool accessed without synchronization, so concurrent calls can race (UB) and potentially call t2_init() multiple times. Consider protecting initialization with std::once_flag/std::call_once or a mutex + atomic flag.
Comment on lines
+15
to
+17
| // Note that RDK telemetry 2.0 support is per process basic, | ||
| // this class is created to take care of un initialization of telemetry but having object as global variable | ||
| // when process goes down, destructor of this class will be called and it will uninitialize the telemetry. |
There was a problem hiding this comment.
The file-level comment explaining the per-process init/uninit is hard to parse (e.g., “per process basic”, “un initialization”, “but having object as global variable”). Please rewrite it to clearly describe the lifecycle/ownership model and when init/uninit occurs.
Suggested change
| // Note that RDK telemetry 2.0 support is per process basic, | |
| // this class is created to take care of un initialization of telemetry but having object as global variable | |
| // when process goes down, destructor of this class will be called and it will uninitialize the telemetry. | |
| // RDK telemetry 2.0 is initialized once per process. | |
| // This helper class encapsulates the initialization/uninitialization logic and is intended to be used | |
| // via a global/static instance: its constructor initializes telemetry at process startup, and its | |
| // destructor automatically uninitializes telemetry when the process shuts down. |
Comment on lines
+4696
to
+4704
| #if 0 | ||
| PlayerTelemetry2::send("MW_BUFFERING_TIMEOUT", | ||
| privatePlayer->gstPrivateContext->numberOfVideoBuffersSent, | ||
| privatePlayer->gstPrivateContext->buffering_timeout_cnt, | ||
| privatePlayer->gstPrivateContext->rate, | ||
| isBufferingTimeoutConditionMet, | ||
| isRateCorrectionDefaultOnPlaying, | ||
| isPlayerReady); | ||
| #endif |
There was a problem hiding this comment.
This #if 0 block in buffering_timeout is dead code and will not be maintained by CI. Please remove it or enable it behind a supported feature flag if needed.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (7)
TelemetryMarkers.h:72
- TelemetryMarkers.h defines TELEMETRY_EVENT_BUFFERING_* / TELEMETRY_EVENT_* (error) / TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE twice, which will cause macro redefinition warnings/errors depending on compiler flags.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
TelemetryMarkers.h:79
- WidevineDrmHelper.cpp emits TELEMETRY_EVENT_DRM_KEY_MISMATCH, but TelemetryMarkers.h does not define it, which will fail compilation.
#define TELEMETRY_EVENT_DRM_HELPER_NOT_FOUND "DRM_HELPER_NOT_FOUND" /**< No DRM helper found for the content protection system */
#define TELEMETRY_EVENT_DRM_PSSH_PARSE_FAILED "DRM_PSSH_PARSE_FAILED" /**< Failed to parse PSSH data from DRM init data */
#define TELEMETRY_EVENT_DRM_SESSION_CREATE_FAILED "DRM_SESSION_CREATE_FAILED" /**< DRM session creation returned null / invalid params */
#define TELEMETRY_EVENT_DRM_SESSION_INIT_FAILED "DRM_SESSION_INIT_FAILED" /**< DRM session OCDM initialisation failed */
#define TELEMETRY_EVENT_OCDM_SYSTEM_CREATE_FAILED "OCDM_SYSTEM_CREATE_FAILED" /**< opencdm_create_system() returned null */
PlayerTelemetry2.cpp:36
- PlayerTelemetry2 default constructor does not delegate; it creates a temporary PlayerTelemetry2 and leaves this->appName uninitialized (and may skip Init depending on optimization).
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:144
- PlayerTelemetry2::sendEvent() computes
initbut never uses it, and calls t2_event_d() even if telemetry was not initialized. This can also trigger -Wunused-but-set-variable warnings in stricter builds.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
InterfacePlayerRDK.cpp:399
- This block logs PLAYER_TELEMETRY_SUPPORT status at runtime every time the pipeline is (re)created, which is noisy and not actionable in production logs.
#ifdef PLAYER_TELEMETRY_SUPPORT /** verifying telemetry support*/
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at compile time");
#else
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at compile time");
#endif
InterfacePlayerRDK.cpp:621
- The seekPausedState branch claims to defer the PLAYING transition, but the current code logs a PLAYING failure and does not set pendingPlayState / buffering_target_state or ensure the pipeline remains PAUSED. This changes behavior and can break seek-with-keepPaused handling.
if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
{
MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
{
TelemetryPayload playingFailPayload;
PlayerTelemetry2.cpp:30
- Player_TelemetryInitializer::~Player_TelemetryInitializer() calls t2_uninit() even if initialization never happened; guard uninit to avoid double-uninit / undefined behavior in partial init scenarios.
Player_TelemetryInitializer::~Player_TelemetryInitializer()
{
t2_uninit();
MW_LOG_MIL("t2_uninit done ");
}
Comment on lines
+247
to
+262
| #ifdef PLAYER_TELEMETRY_SUPPORT | ||
| { | ||
| std::map<std::string, int> intMetrics; | ||
| std::map<std::string, std::string> stringMetrics; | ||
| std::map<std::string, float> floatMetrics; | ||
|
|
||
| intMetrics["isUuidFormat"] = isUuidFormat ? 1 : 0; | ||
| intMetrics["keyIDCount"] = (int)mKeyIDs.size(); | ||
| intMetrics["cencDataSize"] = (int)cencData.size(); | ||
| stringMetrics["cencData"] = cencData; | ||
| stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID); | ||
| stringMetrics["source"] = "setDefaultKeyID_noMatch"; | ||
|
|
||
| PlayerTelemetry2 telemetry; | ||
| telemetry.send(TELEMETRY_EVENT_DRM_KEY_MISMATCH, intMetrics, stringMetrics, floatMetrics); | ||
| } |
Comment on lines
+190
to
+193
| // Telemetry: log cencData format to check if UUID-to-binary conversion is needed | ||
| bool isUuidFormat = (cencData.size() == 36 && cencData[8] == '-' && cencData[13] == '-' && cencData[18] == '-' && cencData[23] == '-'); | ||
| MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s", | ||
| cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str()); |
Comment on lines
+241
to
+263
| MW_LOG_ERR("setDefaultKeyID: TELEMETRY - no key match for cencData=%s isUuidFormat=%d keyIDCount=%zu", | ||
| cencData.c_str(), isUuidFormat, mKeyIDs.size()); | ||
| for (const auto& it : mKeyIDs) | ||
| { | ||
| MW_LOG_ERR("setDefaultKeyID: TELEMETRY - available keyID[%d]=%s", it.first, PlayerLogManager::getHexDebugStr(it.second).c_str()); | ||
| } | ||
| #ifdef PLAYER_TELEMETRY_SUPPORT | ||
| { | ||
| std::map<std::string, int> intMetrics; | ||
| std::map<std::string, std::string> stringMetrics; | ||
| std::map<std::string, float> floatMetrics; | ||
|
|
||
| intMetrics["isUuidFormat"] = isUuidFormat ? 1 : 0; | ||
| intMetrics["keyIDCount"] = (int)mKeyIDs.size(); | ||
| intMetrics["cencDataSize"] = (int)cencData.size(); | ||
| stringMetrics["cencData"] = cencData; | ||
| stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID); | ||
| stringMetrics["source"] = "setDefaultKeyID_noMatch"; | ||
|
|
||
| PlayerTelemetry2 telemetry; | ||
| telemetry.send(TELEMETRY_EVENT_DRM_KEY_MISMATCH, intMetrics, stringMetrics, floatMetrics); | ||
| } | ||
| #endif |
| MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s", | ||
| cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str()); | ||
|
|
||
| #if 0 //dn808 |
| for(auto& it : mKeyIDs) | ||
| { | ||
| if(defaultKeyID == it.second || defaultKeyIDBinary == it.second) | ||
| if(defaultKeyID == it.second ) |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (10)
TelemetryMarkers.h:71
- Duplicate TELEMETRY_EVENT_* macros are defined twice in this header (BUFFERING_, ERROR_, PIPELINE_STATE_CHANGE_FAILURE). This can trigger macro redefinition warnings/errors (often treated as build failures with -Werror) and makes the marker list harder to maintain.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
TelemetryMarkers.h:78
- TELEMETRY_EVENT_DRM_KEY_MISMATCH is referenced from WidevineDrmHelper.cpp but is not defined in TelemetryMarkers.h, which will fail to compile once that code is enabled.
/* ── DRM / Content protection events ─────────────────────────────────────── */
#define TELEMETRY_EVENT_DRM_HELPER_NOT_FOUND "DRM_HELPER_NOT_FOUND" /**< No DRM helper found for the content protection system */
#define TELEMETRY_EVENT_DRM_PSSH_PARSE_FAILED "DRM_PSSH_PARSE_FAILED" /**< Failed to parse PSSH data from DRM init data */
#define TELEMETRY_EVENT_DRM_SESSION_CREATE_FAILED "DRM_SESSION_CREATE_FAILED" /**< DRM session creation returned null / invalid params */
#define TELEMETRY_EVENT_DRM_SESSION_INIT_FAILED "DRM_SESSION_INIT_FAILED" /**< DRM session OCDM initialisation failed */
drm/helper/WidevineDrmHelper.cpp:34
- This file uses TELEMETRY_EVENT_DRM_KEY_MISMATCH but does not include TelemetryMarkers.h, so the build will fail with an undefined identifier (even though the send is under PLAYER_TELEMETRY_SUPPORT, the macro still must be defined).
#include "PlayerLogManager.h"
#include "DrmConstants.h"
#include "PlayerTelemetry2.hpp"
PlayerTelemetry2.cpp:36
- The default constructor constructs a temporary PlayerTelemetry2(""), it does not delegate to the other constructor. As a result, mInitializer.Init() is never called for default-constructed instances (and you create default instances in this PR).
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:144
- sendEvent() computes mInitializer.isInitialized() but does not use it, and still calls t2_event_d even when telemetry was never initialized (and also triggers an unused-variable warning). This can lead to events being dropped or undefined behavior depending on the telemetry library.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
InterfacePlayerRDK.cpp:621
- The seekPausedState branch no longer defers the PLAYING transition (as indicated by the comment) and instead logs a PLAYING failure without updating pendingPlayState/buffering_target_state or forcing the pipeline back to PAUSED. This changes behavior and can break the seek-with-keepPaused flow.
if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
{
MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
{
TelemetryPayload playingFailPayload;
InterfacePlayerRDK.cpp:1417
- This telemetry send hard-codes the marker string instead of using the TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE constant, which risks drift if the marker name changes.
PlayerTelemetry2 telemetry;
telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);
InterfacePlayerRDK.cpp:3640
- This success-path log message still says "FAILED expected", which is misleading now that it executes when the expected state is reached.
MW_LOG_INFO("InterfacePlayerRDK_Pause - validateStateWithMsTimeout - FAILED expected %s", gst_element_state_get_name(nextState));
InterfacePlayerRDK.cpp:118
- This PR introduces both PlayerTelemetry (string key/value payload) and PlayerTelemetry2 (JSON payload) and emits the same marker multiple times (e.g., TELEMETRY_EVENT_INITIALIZED is sent via PlayerTelemetry2::sendEvent, PlayerTelemetry::sendEvent with payload, and PlayerTelemetry2::send). This will inflate telemetry volume and produce inconsistent event schemas for the same marker.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 telemetry;
telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
TelemetryPayload initPayload;
initPayload.add("component", "InterfacePlayerRDK");
initPayload.add("action", "constructor");
initPayload.add("isRialto", isRialto ? 1 : 0);
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_INITIALIZED, initPayload);
PlayerTelemetry.h:123
- PlayerTelemetry directly calls t2_event_d/t2_event_s but does not initialize telemetry (t2_init) anywhere in this abstraction. In this PR, several call sites use PlayerTelemetry without ever constructing PlayerTelemetry2 (the only place that currently calls t2_init), so events may be dropped or the telemetry library may misbehave.
static void sendEvent(const std::string& eventName)
{
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (14)
TelemetryMarkers.h:71
- Duplicate macro definitions (BUFFERING_, ERROR_, PIPELINE_STATE_CHANGE_FAILURE) will trigger redefinition warnings/errors when this header is included. Remove the duplicate block instead of defining the same markers twice.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
PlayerTelemetry2.cpp:144
sendEvent()computesinitbut ignores it and always callst2_event_d(). This can attempt to emit telemetry before initialization and also leaves an unused variable.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
InterfacePlayerRDK.cpp:627
- The
seekPausedStatebranch claims it will defer the PLAYING transition, but the current code logs a PLAYING failure and does not actually setpendingPlayState/ keep the pipeline in PAUSED. This changes behavior and can leave playback stuck or misreported.
/* If a seek-with-keepPaused is active we must not race into PLAYING.
* Defer the PLAYING transition and leave pipeline in PAUSED until
* an explicit resume (Pause(false)) clears `seekPausedState`.
*/
if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
{
MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
{
TelemetryPayload playingFailPayload;
playingFailPayload.add("fromState", "PAUSED");
playingFailPayload.add("toState", "PLAYING");
playingFailPayload.add("context", "ConfigurePipeline");
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE, playingFailPayload);
}
}
InterfacePlayerRDK.cpp:3641
- In the success path of
validateStateWithMsTimeout, the log message still says "FAILED expected" which is misleading when the transition succeeded.
PlayerTelemetry::sendEvent(pause ? TELEMETRY_EVENT_PLAYBACK_PAUSED : TELEMETRY_EVENT_PLAYBACK_RESUMED);
GstState current, pending;
MW_LOG_INFO("InterfacePlayerRDK_Pause - validateStateWithMsTimeout - FAILED expected %s", gst_element_state_get_name(nextState));
PlayerTelemetry2.cpp:36
- The default constructor constructs a temporary
PlayerTelemetry2("")instead of delegating, soappNameremains default-initialized andmInitializer.Init()is not called for the constructed object.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
InterfacePlayerRDK.cpp:112
- This emits the same initialization marker multiple times (via
PlayerTelemetry2::sendEvent,PlayerTelemetry::sendEventwith payload, andPlayerTelemetry2::send). That will inflate metrics / create duplicates; pick one emission path and keep the others only for initialization if needed.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 telemetry;
telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
PlayerTelemetry2.cpp:49
PlayerTelemetry2::send()logs every call and every key/value at ERR level. This is likely to spam production logs and add significant overhead on hot paths.
bool PlayerTelemetry2::send( const std::string &markerName, const std::map<std::string, int>& intData, const std::map<std::string, std::string>& stringData, const std::map<std::string, float>& floatData ) {
MW_LOG_ERR("[M] Marker Name: %s %d", markerName.c_str(), mInitializer.isInitialized());
bool bRet = false;
// Log entry and initializer status
MW_LOG_ERR("[M] Entered send() | marker: %s | initializer: %d",
markerName.c_str(), mInitializer.isInitialized());
drm/helper/WidevineDrmHelper.cpp:34
TELEMETRY_EVENT_DRM_KEY_MISMATCHis used below butTelemetryMarkers.his not included, so builds withPLAYER_TELEMETRY_SUPPORTwill fail with an undefined macro.
#include "WidevineDrmHelper.h"
#include "DrmUtils.h"
#include "PlayerLogManager.h"
#include "DrmConstants.h"
#include "PlayerTelemetry2.hpp"
drm/helper/WidevineDrmHelper.cpp:199
- UUID-to-binary conversion is currently disabled (
#if 0), butmKeyIDsare stored as binary bytes from PSSH. This makessetDefaultKeyID()unable to match UUID-string inputs and will often fall back to the first slot.
#if 0 //dn808
// Also convert UUID string (e.g. "f3dff538-b8c9-58e4-e8cd-96cf811d32dc") to 16-byte binary
// for comparison against binary keyIDs parsed from PSSH
std::vector<uint8_t> defaultKeyIDBinary;
drm/helper/WidevineDrmHelper.cpp:233
- After enabling UUID conversion, the match should also compare against
defaultKeyIDBinary(otherwise UUID-string inputs still won't match binary PSSH key IDs).
for(auto& it : mKeyIDs)
{
if(defaultKeyID == it.second )
{
mDefaultKeySlot = it.first;
drm/helper/WidevineDrmHelper.cpp:194
- These logs dump the raw key ID / CENC data bytes at WARN/ERR level. Key IDs can be considered sensitive and this will also create very noisy logs; prefer logging only sizes/counts (or guard behind a debug flag).
MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s",
cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str());
drm/helper/WidevineDrmHelper.cpp:247
- Avoid logging each available key ID value in production logs; if needed, gate this behind a debug-only flag.
for (const auto& it : mKeyIDs)
{
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - available keyID[%d]=%s", it.first, PlayerLogManager::getHexDebugStr(it.second).c_str());
}
CMakeLists.txt:157
- Using
set(... "${VAR}" newItem)to append to list-like variables is fragile and hard to maintain (quoting turns the previous list into a single element). Preferlist(APPEND ...)for sources/depends and avoid quoting raw-l...linker flags.
if(CMAKE_TELEMETRY_2_0_REQUIRED)
message("CMAKE_TELEMETRY_2_0_REQUIRED set")
set(LIBPLAYERGSTINTERFACE_SOURCES "${LIBPLAYERGSTINTERFACE_SOURCES}" PlayerTelemetry2.cpp)
set(LIBPLAYERGSTINTERFACE_DEFINES "${LIBPLAYERGSTINTERFACE_DEFINES} -DPLAYER_TELEMETRY_SUPPORT=1")
set(LIBPLAYERGSTINTERFACE_DEPENDS ${LIBPLAYERGSTINTERFACE_DEPENDS} "-ltelemetry_msgsender")
drm/helper/WidevineDrmHelper.cpp:243
- Avoid logging the raw
cencDataand all available key IDs at ERR level; this can leak key identifiers and explode log volume. Log only counts/sizes, and rely on telemetry/debug builds for deep diagnostics.
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - no key match for cencData=%s isUuidFormat=%d keyIDCount=%zu",
cencData.c_str(), isUuidFormat, mKeyIDs.size());
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (12)
TelemetryMarkers.h:74
- The buffering/error sections are duplicated, redefining TELEMETRY_EVENT_BUFFERING_STARTED/ENDED, TELEMETRY_EVENT_ERROR/DECODE_ERROR/NETWORK_ERROR, and TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE. This will trigger macro redefinition warnings/errors and can break builds depending on compiler flags.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
/* ── DRM / Content protection events ─────────────────────────────────────── */
PlayerTelemetry2.cpp:30
- Destructor always calls t2_uninit() even when telemetry was never initialized. If Init() was never called, this can cause undefined behavior depending on the telemetry library implementation; it also makes logs misleading.
Player_TelemetryInitializer::~Player_TelemetryInitializer()
{
t2_uninit();
MW_LOG_MIL("t2_uninit done ");
}
PlayerTelemetry2.cpp:36
- The default constructor creates a temporary PlayerTelemetry2(""), which does not initialize the current object (and is easy to misread as delegating). Use a delegating constructor so initialization is performed on this instance.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:144
- sendEvent() computes init but doesn't use it and calls t2_event_d() unconditionally. This both triggers an unused-variable warning (potentially failing -Werror builds) and can attempt to emit telemetry before initialization.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
PlayerTelemetry2.cpp:4
- is included but not used in this translation unit, adding unnecessary compile overhead and potentially triggering -Wunused-include warnings under stricter builds.
#include "PlayerTelemetry2.hpp"
#include <fstream>
#include <telemetry_busmessage_sender.h>
InterfacePlayerRDK.cpp:627
- The seekPausedState branch comment says PLAYING should be deferred, but the current implementation only logs a failure and does not set pendingPlayState/buffering_target_state or ensure the pipeline stays PAUSED. This likely breaks the intended "seek-with-keepPaused" behavior and leaves state flags inconsistent.
/* If a seek-with-keepPaused is active we must not race into PLAYING.
* Defer the PLAYING transition and leave pipeline in PAUSED until
* an explicit resume (Pause(false)) clears `seekPausedState`.
*/
if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
{
MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
{
TelemetryPayload playingFailPayload;
playingFailPayload.add("fromState", "PAUSED");
playingFailPayload.add("toState", "PLAYING");
playingFailPayload.add("context", "ConfigurePipeline");
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE, playingFailPayload);
}
}
InterfacePlayerRDK.cpp:3641
- In the success path (validateStateWithMsTimeout matched nextState), the log message still says "FAILED expected" which is misleading and will confuse debugging/operations.
PlayerTelemetry::sendEvent(pause ? TELEMETRY_EVENT_PLAYBACK_PAUSED : TELEMETRY_EVENT_PLAYBACK_RESUMED);
GstState current, pending;
MW_LOG_INFO("InterfacePlayerRDK_Pause - validateStateWithMsTimeout - FAILED expected %s", gst_element_state_get_name(nextState));
InterfacePlayerRDK.cpp:1418
- Telemetry event name is hard-coded as a string literal ("MW_PIPELINE_STATE_CHANGE_FAILURE") instead of using the shared marker constant. This makes refactors error-prone and risks drift if the marker is renamed.
PlayerTelemetry2 telemetry;
telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);
drm/helper/WidevineDrmHelper.cpp:200
- This log prints the full default key ID (derived from cencData) as hex. In a DRM context this is sensitive material and should not be logged at WARN level (or at all) in production builds.
// Telemetry: log cencData format to check if UUID-to-binary conversion is needed
bool isUuidFormat = (cencData.size() == 36 && cencData[8] == '-' && cencData[13] == '-' && cencData[18] == '-' && cencData[23] == '-');
MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s",
cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str());
drm/helper/WidevineDrmHelper.cpp:268
- The added error logging + telemetry payload include raw cencData and key IDs (and iterates logging all available keyIDs). This can leak DRM identifiers/key material into logs/telemetry backends. Prefer only non-sensitive aggregates (sizes/counts/flags) or a one-way hash if correlation is needed.
// Telemetry: log when no match found - indicates UUID binary comparison may be needed
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - no key match for cencData=%s isUuidFormat=%d keyIDCount=%zu",
cencData.c_str(), isUuidFormat, mKeyIDs.size());
for (const auto& it : mKeyIDs)
{
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - available keyID[%d]=%s", it.first, PlayerLogManager::getHexDebugStr(it.second).c_str());
}
#ifdef PLAYER_TELEMETRY_SUPPORT
{
std::map<std::string, int> intMetrics;
std::map<std::string, std::string> stringMetrics;
std::map<std::string, float> floatMetrics;
intMetrics["isUuidFormat"] = isUuidFormat ? 1 : 0;
intMetrics["keyIDCount"] = (int)mKeyIDs.size();
intMetrics["cencDataSize"] = (int)cencData.size();
stringMetrics["cencData"] = cencData;
stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID);
stringMetrics["source"] = "setDefaultKeyID_noMatch";
PlayerTelemetry2 telemetry;
telemetry.send(TELEMETRY_EVENT_DRM_KEY_MISMATCH, intMetrics, stringMetrics, floatMetrics);
}
InterfacePlayerRDK.cpp:130
- When PLAYER_TELEMETRY_SUPPORT is enabled, this constructor emits TELEMETRY_EVENT_INITIALIZED multiple times (PlayerTelemetry2::sendEvent, PlayerTelemetry::sendEvent with payload, and PlayerTelemetry2::send with metrics). This will double/triple-count the same marker in telemetry backends and makes dashboards ambiguous. Consider standardizing on a single emitter/payload format per marker.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 telemetry;
telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
TelemetryPayload initPayload;
initPayload.add("component", "InterfacePlayerRDK");
initPayload.add("action", "constructor");
initPayload.add("isRialto", isRialto ? 1 : 0);
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_INITIALIZED, initPayload);
#ifdef PLAYER_TELEMETRY_SUPPORT
std::map<std::string, int> intMetrics;
std::map<std::string, std::string> stringMetrics;
std::map<std::string, float> floatMetrics;
intMetrics["isRialto"] = isRialto ? 1 : 0;
stringMetrics["component"] = "InterfacePlayerRDK";
stringMetrics["action"] = "constructor";
telemetry.send(TELEMETRY_EVENT_INITIALIZED, intMetrics, stringMetrics, floatMetrics);
#endif
InterfacePlayerRDK.cpp:635
- When PLAYER_TELEMETRY_SUPPORT is enabled, TELEMETRY_EVENT_PLAYBACK_STARTED is emitted via both PlayerTelemetry2 and PlayerTelemetry. This will likely create duplicate events for a single playback start and skew metrics.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 Telemetry;
Telemetry.sendEvent(TELEMETRY_EVENT_PLAYBACK_STARTED);
#endif
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_PLAYBACK_STARTED);
if (SetStateWithWarnings(interfacePlayerPriv->gstPrivateContext->pipeline, GST_STATE_PLAYING) == GST_STATE_CHANGE_FAILURE)
for watermarking telemetry condition added
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (9)
PlayerTelemetry2.cpp:143
- sendEvent() computes an unused
initvariable and calls t2_event_d() even when the telemetry initializer is not ready. This can emit events before t2_init() and silently fail or misbehave.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
PlayerTelemetry2.cpp:36
- The default constructor constructs a temporary PlayerTelemetry2(""), leaving this instance uninitialized (mInitializer.Init() not called) and appName unset. This can cause send()/sendEvent() to run without telemetry being initialized.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.hpp:7
- Header guard macro names beginning with double underscores are reserved to the implementation in C/C++. Using
__PLAYER_TELEMETRY_2_H__risks undefined behavior or collisions with toolchain headers.
#ifndef __PLAYER_TELEMETRY_2_H__
#define __PLAYER_TELEMETRY_2_H__
InterfacePlayerRDK.cpp:617
- When seekPausedState is active, this branch logs "GST_STATE_PLAYING failed" and emits a failure telemetry event without actually attempting the PLAYING transition or setting pendingPlayState. This contradicts the comment above (defer PLAYING and keep pipeline PAUSED) and can leave the pipeline in an unexpected state.
/* If a seek-with-keepPaused is active we must not race into PLAYING.
* Defer the PLAYING transition and leave pipeline in PAUSED until
* an explicit resume (Pause(false)) clears `seekPausedState`.
*/
if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
drm/helper/WidevineDrmHelper.cpp:199
- This log prints the full defaultKeyID (derived from cencData) in hex. DRM key IDs can be sensitive; avoid logging the raw identifier in production logs.
// Telemetry: log cencData format to check if UUID-to-binary conversion is needed
bool isUuidFormat = (cencData.size() == 36 && cencData[8] == '-' && cencData[13] == '-' && cencData[18] == '-' && cencData[23] == '-');
MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s",
cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str());
TelemetryMarkers.h:65
- Duplicate TELEMETRY_EVENT_* macro definitions (BUFFERING_, ERROR_, PIPELINE_STATE_CHANGE_FAILURE) will trigger macro redefinition warnings/errors and can break builds when warnings are treated as errors. Remove the repeated block so each marker is defined once.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
drm/helper/WidevineDrmHelper.cpp:251
- On mismatch, the code logs the raw cencData string and dumps every available keyID value. This risks leaking DRM identifiers into logs; prefer logging only counts/lengths (or a non-reversible hash) instead of full values.
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - no key match for cencData=%s isUuidFormat=%d keyIDCount=%zu",
cencData.c_str(), isUuidFormat, mKeyIDs.size());
for (const auto& it : mKeyIDs)
{
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - available keyID[%d]=%s", it.first, PlayerLogManager::getHexDebugStr(it.second).c_str());
drm/helper/WidevineDrmHelper.cpp:264
- Telemetry payload includes raw
cencDataanddefaultKeyHex. These look like DRM identifiers and may be sensitive; consider removing them or replacing with a non-reversible hash/truncated form to avoid data exposure via telemetry.
intMetrics["isUuidFormat"] = isUuidFormat ? 1 : 0;
intMetrics["keyIDCount"] = (int)mKeyIDs.size();
intMetrics["cencDataSize"] = (int)cencData.size();
stringMetrics["cencData"] = cencData;
stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID);
stringMetrics["source"] = "setDefaultKeyID_noMatch";
InterfacePlayerRDK.cpp:634
- TELEMETRY_EVENT_PLAYBACK_STARTED is emitted before attempting to set the pipeline to PLAYING. If SetStateWithWarnings() fails, telemetry will incorrectly report playback started. Emit the event only after a successful transition.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 Telemetry;
Telemetry.sendEvent(TELEMETRY_EVENT_PLAYBACK_STARTED);
#endif
PlayerTelemetry::sendEvent(TELEMETRY_EVENT_PLAYBACK_STARTED);
Comment on lines
+229
to
+245
| if (width == 0 || height == 0) | ||
| { | ||
|
|
||
| std::map<std::string, int> intMetrics; | ||
| std::map<std::string, std::string> stringMetrics; | ||
| std::map<std::string, float> floatMetrics; | ||
|
|
||
| intMetrics["width"] = width; | ||
| intMetrics["height"] = height; | ||
| intMetrics["isRialto"] = isRialto ? 1 : 0; // keep only if isRialto is available in scope | ||
|
|
||
| stringMetrics["component"] = "AampLicenseManager"; | ||
| stringMetrics["action"] = "setVideoWindowSize"; | ||
| stringMetrics["reason"] = "watermark_enabled_zero_dimension"; | ||
|
|
||
| telemetry.send("TELEMETRY_WATERMARK_ZERO_DIMENSION", intMetrics, stringMetrics, floatMetrics); | ||
| } |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (7)
PlayerTelemetry2.cpp:36
- The default constructor constructs a temporary PlayerTelemetry2("") instead of delegating/initializing this instance, so mInitializer.Init() is never called for default-constructed objects. This prevents send() from ever sending (initializer stays false) and can leave telemetry uninitialized.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:144
- sendEvent() computes an unused 'init' local (potential -Wunused-variable) and calls t2_event_d() even if telemetry was never initialized. Either initialize here (idempotently) or return early when not initialized.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
TelemetryMarkers.h:71
- Duplicate TELEMETRY_EVENT_* macro definitions (BUFFERING_, ERROR_, PIPELINE_STATE_CHANGE_FAILURE) are redefined later in this header, which can cause macro-redefinition warnings/errors (and break builds if warnings are treated strictly). Remove the duplicated block so each marker is defined once.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
drm/DrmSessionManager.cpp:239
- This block will not compile: it uses std::map without including , references 'isRialto' which is not in scope in this method, and calls telemetry.send(...) but no 'telemetry' object exists in the function/class. Consider using the existing PlayerTelemetry + TelemetryPayload helper here (no extra includes/objects needed).
if (width == 0 || height == 0)
{
std::map<std::string, int> intMetrics;
std::map<std::string, std::string> stringMetrics;
drm/helper/WidevineDrmHelper.cpp:264
- Telemetry currently sends the raw cencData string and a hex representation of the default key ID. These values can be sensitive DRM identifiers; consider removing or redacting them (e.g., send only sizes/booleans) to avoid leaking DRM-related data into telemetry pipelines.
intMetrics["isUuidFormat"] = isUuidFormat ? 1 : 0;
intMetrics["keyIDCount"] = (int)mKeyIDs.size();
intMetrics["cencDataSize"] = (int)cencData.size();
stringMetrics["cencData"] = cencData;
stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID);
stringMetrics["source"] = "setDefaultKeyID_noMatch";
CMakeLists.txt:151
- In CMake, quoting LIBPLAYERGSTINTERFACE_SOURCES here can collapse the existing semicolon-separated list into a single string element, which can drop sources on some generators. Append to the list without quotes (as done elsewhere in this file).
set(LIBPLAYERGSTINTERFACE_SOURCES "${LIBPLAYERGSTINTERFACE_SOURCES}" PlayerTelemetry2.cpp)
InterfacePlayerRDK.cpp:4436
- In GstPlayer_OnGstPtsErrorCb(), the telemetry payload sets i["vid"]/i["aud"] from isVideo/isAudioSink before those booleans are computed, so telemetry always reports 0/0. Populate these fields using the same helper logic (or move the telemetry send after the sink-type detection).
/** Integer values */
i["vid"] = isVideo ? 1 : 0;
i["aud"] = isAudioSink ? 1 : 0;
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (10)
TelemetryMarkers.h:72
- TelemetryMarkers.h defines several TELEMETRY_EVENT_* macros twice (e.g., BUFFERING_STARTED/ENDED, ERROR/DECODE_ERROR/NETWORK_ERROR, PIPELINE_STATE_CHANGE_FAILURE). This will trigger macro redefinition warnings/errors depending on compiler flags.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
PlayerTelemetry2.cpp:36
- The default constructor constructs a temporary PlayerTelemetry2 instance ("PlayerTelemetry2("")") instead of delegating, so mInitializer.Init() is not guaranteed to run for default-constructed objects.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:143
- sendEvent() ignores the initializer state (and the local 'init' is unused). Calling t2_event_d() without successful t2_init() can lead to undefined behavior or dropped events.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
InterfacePlayerRDK.cpp:112
- TELEMETRY_EVENT_INITIALIZED is emitted multiple times (PlayerTelemetry2::sendEvent, PlayerTelemetry::sendEvent with payload, and PlayerTelemetry2::send). This will create duplicate telemetry records for a single initialization and increases overhead.
#ifdef PLAYER_TELEMETRY_SUPPORT
PlayerTelemetry2 telemetry;
telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
InterfacePlayerRDK.cpp:399
- The compile-time PLAYER_TELEMETRY_SUPPORT log and the "Nitz :" prefix add noisy/non-actionable logging in a hot path. Consider keeping a single consistent log line without personal/debug prefixes.
#ifdef PLAYER_TELEMETRY_SUPPORT /** verifying telemetry support*/
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at compile time");
#else
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at compile time");
#endif
InterfacePlayerRDK.cpp:1388
- A string literal is used for the pipeline-state-change-failure marker even though TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE is defined. Using the macro avoids drift if the marker name changes.
telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);
InterfacePlayerRDK.cpp:4436
- The telemetry payload is populated with isVideo/isAudioSink before those flags are computed, so 'vid'/'aud' will always be reported as 0 in MW_PTS_ERROR events.
/** Integer values */
i["vid"] = isVideo ? 1 : 0;
i["aud"] = isAudioSink ? 1 : 0;
drm/helper/WidevineDrmHelper.cpp:199
- This log prints the (hex-encoded) default key ID derived from cencData. Even though it's not the decryption key, key IDs/content identifiers can still be sensitive; consider logging only sizes/flags unless debug logging is explicitly enabled.
MW_LOG_WARN("setDefaultKeyID: cencData size=%zu isUuidFormat=%d data=%s",
cencData.size(), isUuidFormat, PlayerLogManager::getHexDebugStr(defaultKeyID).c_str());
drm/helper/WidevineDrmHelper.cpp:251
- These error logs include raw cencData and dump all available key IDs. This can expose DRM/content identifiers and can be very noisy in production; prefer redacted logging (sizes/counts) or guard the full dump behind a debug flag.
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - no key match for cencData=%s isUuidFormat=%d keyIDCount=%zu",
cencData.c_str(), isUuidFormat, mKeyIDs.size());
for (const auto& it : mKeyIDs)
{
MW_LOG_ERR("setDefaultKeyID: TELEMETRY - available keyID[%d]=%s", it.first, PlayerLogManager::getHexDebugStr(it.second).c_str());
drm/DrmSessionManager.cpp:46
- This file includes TelemetryMarkers.h and PlayerTelemetry.h twice (once unconditionally and again after the PLAYER_TELEMETRY_SUPPORT block). This is redundant and makes include ordering harder to reason about.
#ifdef PLAYER_TELEMETRY_SUPPORT
#include "PlayerTelemetry2.hpp"
#endif
#include "TelemetryMarkers.h"
#include "PlayerTelemetry.h"
Comment on lines
+251
to
+255
| stringMetrics["component"] = "AampLicenseManager"; | ||
| stringMetrics["action"] = "setVideoWindowSize"; | ||
| stringMetrics["reason"] = "watermark_enabled_zero_dimension"; | ||
|
|
||
| telemetry.send("TELEMETRY_WATERMARK_ZERO_DIMENSION", intMetrics, stringMetrics, floatMetrics); |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (9)
TelemetryMarkers.h:72
- TelemetryMarkers.h defines several TELEMETRY_EVENT_* macros twice (e.g., BUFFERING_STARTED/ENDED, ERROR/DECODE_ERROR/NETWORK_ERROR, PIPELINE_STATE_CHANGE_FAILURE). This can trigger macro redefinition warnings (often treated as errors) and makes the marker list harder to maintain. Remove the duplicate block so each marker is defined once.
/* ── Media / buffering events ─────────────────────────────────────────────── */
#define TELEMETRY_EVENT_BUFFERING_STARTED "BUFFERING_STARTED" /**< Pre-roll buffering begins */
#define TELEMETRY_EVENT_BUFFERING_ENDED "BUFFERING_ENDED" /**< Sufficient frames buffered; pipeline unpaused */
/* ── Error events ─────────────────────────────────────────────────────────── */
#define TELEMETRY_EVENT_ERROR "ERROR" /**< Generic GStreamer pipeline error (GST_MESSAGE_ERROR) */
#define TELEMETRY_EVENT_DECODE_ERROR "DECODE_ERROR" /**< Decoder reported a decode-error-callback */
#define TELEMETRY_EVENT_NETWORK_ERROR "NETWORK_ERROR" /**< Resource/stream error that indicates a network fault */
/* ── Pipeline state change failure ───────────────────────────────────────── */
#define TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE "MW_PIPELINE_STATE_CHANGE_FAILURE" /**< gst_element_set_state() returned GST_STATE_CHANGE_FAILURE */
PlayerTelemetry2.cpp:36
- The default constructor creates a temporary PlayerTelemetry2 instead of delegating/initializing this instance, so appName stays default-initialized and (more importantly) mInitializer.Init() is never called. This means
PlayerTelemetry2 telemetry; telemetry.send...will never send anything.
PlayerTelemetry2::PlayerTelemetry2() {
PlayerTelemetry2("");
}
PlayerTelemetry2.cpp:30
- Player_TelemetryInitializer unconditionally calls t2_uninit() in the destructor even if Init() was never called. Guarding on m_Initialized avoids uninitializing an uninitialized telemetry library (and makes intent explicit).
Player_TelemetryInitializer::~Player_TelemetryInitializer()
{
t2_uninit();
MW_LOG_MIL("t2_uninit done ");
}
PlayerTelemetry2.cpp:144
- sendEvent() ignores the initializer state (and leaves an unused local
init). If telemetry isn't initialized, this will still call t2_event_d(). Add an initialization guard and remove the unused variable.
void PlayerTelemetry2::sendEvent(const std::string& eventName)
{
bool init = mInitializer.isInitialized();
MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
t2_event_d(const_cast<char*>(eventName.c_str()), 1);
}
InterfacePlayerRDK.cpp:399
- This block looks like leftover debug/verification logging, and the log message includes a personal tag ("Nitz :"). It will add noise to logs in a common path. Consider reverting to the original single "Create pipeline" log line without the compile-time verification messages/personal tag.
#ifdef PLAYER_TELEMETRY_SUPPORT /** verifying telemetry support*/
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at compile time");
#else
MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at compile time");
#endif
InterfacePlayerRDK.cpp:1389
- Use the shared marker constant instead of a hard-coded string so the event name stays consistent if the marker list changes (and so it’s obvious this matches TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE).
PlayerTelemetry2 telemetry;
telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);
InterfacePlayerRDK.cpp:4437
- MW_PTS_ERROR telemetry currently always reports vid=0/aud=0 because isVideo/isAudioSink are still false when the map is populated (they’re computed after the telemetry send). Compute the values for telemetry before setting i["vid"]/i["aud"].
/** Integer values */
i["vid"] = isVideo ? 1 : 0;
i["aud"] = isAudioSink ? 1 : 0;
drm/helper/WidevineDrmHelper.cpp:264
- This telemetry event includes raw DRM-related identifiers (cencData and defaultKeyHex). These values can be sensitive and may be considered content-identifying data; consider redacting them and only sending non-sensitive metadata (sizes, counts, format flags).
stringMetrics["cencData"] = cencData;
stringMetrics["defaultKeyHex"] = PlayerLogManager::getHexDebugStr(defaultKeyID);
stringMetrics["source"] = "setDefaultKeyID_noMatch";
drm/DrmSessionManager.cpp:46
- TelemetryMarkers.h / PlayerTelemetry.h are included twice in this file (once before and once after the PLAYER_TELEMETRY_SUPPORT block). This is redundant and makes include order harder to follow; keep a single include block.
#include "TelemetryMarkers.h"
#include "PlayerTelemetry.h"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.