Feature/rdkemw 13310 by dp0000 · Pull Request #82 · rdkcentral/middleware-player-interface

dp0000 · 2026-02-23T09:04:43Z

No description provided.

Reason for change : telemetry marker implementation . Test Steps : . Signed-off by: Deepikasri Natarajan (deepikasri_n@comcast.com)

github-actions · 2026-02-23T09:04:55Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

Copilot

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 PlayerTelemetry2 class 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_REQUIRED flag

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.

Copilot · 2026-02-23T09:13:06Z

InterfacePlayerRDK.cpp

+#if 0
+            PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",
+            formatType,
+            protSystemId ? protSystemId : "",
+            mediaType,
+            initDataSize);
+#endif


These commented-out telemetry calls (lines 3291-3297) should either be implemented or removed. Leaving significant blocks of disabled code in the codebase reduces readability and maintainability. If this is intended for future use, consider moving it to a feature branch or tracking it through an issue.

Suggested change

#if 0

PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",

formatType,

protSystemId ? protSystemId : "",

mediaType,

initDataSize);

#endif

// No protection event is queued when initData is invalid or empty.

Copilot · 2026-02-23T09:13:06Z

InterfacePlayerRDK.cpp

+#if 0
+                        PlayerTelemetry2::send("MW_GST_ERROR",
+                        GST_OBJECT_NAME(msg->src),
+                        error->message,
+                        dbg_info ? dbg_info : "",
+                        privatePlayer->gstPrivateContext->rate);
+#endif


These commented-out telemetry calls (lines 4210-4216) should either be implemented or removed. Leaving significant blocks of disabled code in the codebase reduces readability and maintainability.

Suggested change

#if 0

PlayerTelemetry2::send("MW_GST_ERROR",

GST_OBJECT_NAME(msg->src),

error->message,

dbg_info ? dbg_info : "",

privatePlayer->gstPrivateContext->rate);

#endif

Copilot · 2026-02-23T09:13:06Z

InterfacePlayerRDK.cpp

+#if 0
+    	         PlayerTelemetry2::send("MW_BUFFERING_TIMEOUT",
+                 privatePlayer->gstPrivateContext->numberOfVideoBuffersSent,
+                 privatePlayer->gstPrivateContext->buffering_timeout_cnt,
+                 privatePlayer->gstPrivateContext->rate,
+                 isBufferingTimeoutConditionMet,
+                 isRateCorrectionDefaultOnPlaying,
+                 isPlayerReady);
+#endif


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.

Copilot · 2026-02-23T09:13:07Z

PlayerTelemetry2.cpp

+            std::string key = pair.first;
+            int value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        for (const auto& pair : stringData) {
+            std::string key = pair.first;
+            std::string value = pair.second;
+            cJSON_AddStringToObject(root, key.c_str(), value.c_str());
+        }
+
+        for (const auto& pair : floatData) {
+            std::string key = pair.first;
+            float value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);


The variables 'key' and 'value' are unnecessary local copies. The loop can directly use pair.first and pair.second in the function call, avoiding unnecessary string copies.

Suggested change

std::string key = pair.first;

int value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

}

for (const auto& pair : stringData) {

std::string key = pair.first;

std::string value = pair.second;

cJSON_AddStringToObject(root, key.c_str(), value.c_str());

}

for (const auto& pair : floatData) {

std::string key = pair.first;

float value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

cJSON_AddNumberToObject(root, pair.first.c_str(), pair.second);

}

for (const auto& pair : stringData) {

cJSON_AddStringToObject(root, pair.first.c_str(), pair.second.c_str());

}

for (const auto& pair : floatData) {

cJSON_AddNumberToObject(root, pair.first.c_str(), pair.second);

Copilot · 2026-02-23T09:13:07Z

PlayerTelemetry2.cpp

+bool PlayerTelemetry2::send( const std::string &markerName, const char *  data)
+{
+    bool bRet = false;
+    if(mInitializer.isInitialized()	&& NULL != data)


Inconsistent whitespace: there are tabs after 'isInitialized()' that should be removed for consistency.

Copilot · 2026-02-23T09:13:09Z

PlayerTelemetry2.cpp

+        T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);
+
+        if(T2ERROR_SUCCESS == t2Error)
+        {
+            bRet = true;
+        }
+        else
+        {
+            MW_LOG_ERR("t2_event_s map failed:%d ", t2Error);
+        }
+        cJSON_free(jsonString);
+        cJSON_Delete(root);
+    }
+
+    return bRet;
+}
+
+bool PlayerTelemetry2::send( const std::string &markerName, const char *  data)
+{
+    bool bRet = false;
+    if(mInitializer.isInitialized()	&& NULL != data)
+    {
+        MW_LOG_INFO("[S] Marker Name: %s value:%s", markerName.c_str(),data );
+        T2ERROR t2Error =  t2_event_s( (char *)markerName.c_str(),(char*)data );


Casting away const from markerName and data strings when calling t2_event_s is unsafe. If the t2_event_s function modifies these strings, it will cause undefined behavior. Verify that t2_event_s does not modify these parameters, or if the API requires non-const char*, consider using a defensive copy.

Copilot · 2026-02-23T09:13:09Z

PlayerTelemetry2.hpp

+#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 


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

Copilot · 2026-02-23T09:13:09Z

InterfacePlayerRDK.cpp

+#if 0
+        PlayerTelemetry2::send("MW_DECODE_ERROR",
+        GST_ELEMENT_NAME(object),
+        privatePlayer->gstPrivateContext->decodeErrorCBCount,
+        deltaMS,
+        privatePlayer->gstPrivateContext->rate);
+#endif


These commented-out telemetry calls (lines 4167-4173) should either be implemented or removed. Leaving significant blocks of disabled code in the codebase reduces readability and maintainability.

Suggested change

#if 0

PlayerTelemetry2::send("MW_DECODE_ERROR",

GST_ELEMENT_NAME(object),

privatePlayer->gstPrivateContext->decodeErrorCBCount,

deltaMS,

privatePlayer->gstPrivateContext->rate);

#endif

Copilot · 2026-02-23T09:13:10Z

InterfacePlayerRDK.cpp

+            PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",
+            formatType,
+            protSystemId ? protSystemId : "",
+            mediaType,
+            initDataSize);


The commented-out code calls PlayerTelemetry2::send as a static method, but the send methods are instance methods, not static methods. If these are intended to be enabled in the future, the API design needs to be corrected - either make send methods static or create instances before calling them.

Suggested change

PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",

formatType,

protSystemId ? protSystemId : "",

mediaType,

initDataSize);

PlayerTelemetry2 telemetry;

telemetry.send("MW_PROTECTION_EVENT_FAILED",

formatType,

protSystemId ? protSystemId : "",

mediaType,

initDataSize);

Copilot · 2026-02-23T09:13:10Z

PlayerTelemetry2.hpp

+    /**
+     * @brief send  - Send the telemetry data to the telemetry bus
+     * @param[in] markerName - Name of the marker
+     * @param[in] data - Data to be sent


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

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

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.

Copilot · 2026-03-04T10:11:25Z

sync/aamp_middleware_sync.sh

+    echo ""
+    echo "Sync completed on: $current_date at $current_time"
+    echo "============================================"
+    save_state


Step 24 removes $STATE_FILE as part of successful completion, but then calls save_state at the end of the step, which recreates the state file. Drop the final save_state on success (or only call it on error) so the temp state file is actually cleaned up.

Suggested change

save_state

Copilot · 2026-03-04T10:11:25Z

PlayerTelemetry2.cpp

+PlayerTelemetry2::PlayerTelemetry2() {
+    PlayerTelemetry2("");


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("")

{

Copilot · 2026-03-04T10:11:26Z

InterfacePlayerRDK.cpp

+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");
+#else
+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");
+#endif
+		MW_LOG_MIL("Nitz : Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);


The PLAYER_TELEMETRY_SUPPORT messages are behind #ifdef, so they reflect build-time configuration (not runtime). Also the "Nitz :" prefix in the pipeline creation log looks like leftover debug text and makes logs harder to grep; consider removing/standardizing it.

Suggested change

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");

#else

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");

#endif

MW_LOG_MIL("Nitz : Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled in this build");

#else

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled in this build");

#endif

MW_LOG_MIL("Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);

Copilot · 2026-03-04T10:11:26Z

drm/helper/VerimatrixHelper.cpp

 	else
 		MW_LOG_WARN("unknown mediaFormat %d", mDrmInfo.mediaFormat);


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;

}

Copilot · 2026-03-04T10:11:26Z

test/utests/run.sh

 }

+# "corrupt arc tag"
+(find . -name "*.gcda" -print0 | xargs -0 rm) || true


find ... | xargs rm will invoke rm with no arguments on some platforms when there are no .gcda files, producing noisy "missing operand" output (even though you ignore the exit code). Consider using xargs -r rm or find ... -delete to avoid spurious errors.

Suggested change

(find . -name "*.gcda" -print0 | xargs -0 rm) || true

(find . -name "*.gcda" -print0 | xargs -0 -r rm) || true

Copilot · 2026-03-04T10:11:27Z

sync/filter_middleware_patch.py

+
+import re
+import sys
+from typing import List, TextIO


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

Copilot · 2026-03-04T10:11:27Z

PlayerTelemetry2.cpp

+        //lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.
+        char *jsonString = cJSON_PrintUnformatted(root);
+
+        MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(),jsonString);
+
+        T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);
+


cJSON_PrintUnformatted(root) may return nullptr on allocation failure; MW_LOG_INFO/t2_event_s then dereference/use it. Add a null-check for jsonString (and also handle root == nullptr) to avoid crashes under OOM conditions.

Copilot · 2026-03-04T10:11:27Z

InterfacePlayerRDK.cpp

+	// Integer values
+	i["vid"] = isVideo ? 1 : 0;
+	i["aud"] = isAudioSink ? 1 : 0;
+
+	// Float values


i["vid"]/i["aud"] are populated before isVideo / isAudioSink are computed (those booleans are still false here), so telemetry will always report 0/0. Move telemetry population/sending to after the sink/decoder classification logic so it reports correct values.

Copilot · 2026-03-04T10:11:28Z

test/utests/run.sh

+PLAYER_BUILD_GCNO=""
+
+if [ "$build_coverage" -eq "1" ]; then
+    #Find where player .gcno files get put when player-cli is built via install-middleware.sh -c
+    A_GCNO=$(find ${PLAYERDIR}/build -name 'AampConfig*gcno' -print -quit)


PLAYER_BUILD_GCNO is computed when -c is enabled, but it is never used anywhere else in the script (only assigned). Either use it in the later lcov/genhtml steps or remove this block to avoid dead code/confusion.

Copilot

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.

Copilot · 2026-03-05T09:25:40Z

sync/aamp_middleware_sync.sh

+    # Clean up state file on successful completion
+    if [ -f "$STATE_FILE" ]; then
+        rm "$STATE_FILE"
+        print_status "Removed state file $STATE_FILE after successful completion"
+    fi


In the success path you remove the state file (rm "$STATE_FILE"), but later in the same step the script calls save_state, which will recreate the .temp file even after a successful run. Drop the final save_state on success (or only call it before deletion) so the cleanup is effective.

Copilot · 2026-03-05T09:25:41Z

sync/filter_middleware_patch.py

+        print("No 'diff --git' found in the patch file")
+        return


When no diff --git is found, the script prints a message and returns successfully without creating the output file. Since this script is used in automation, it should exit non-zero in this case (e.g., raise an exception or sys.exit(1)) so callers can reliably detect failure.

Suggested change

print("No 'diff --git' found in the patch file")

return

print("No 'diff --git' found in the patch file", file=sys.stderr)

sys.exit(1)

Copilot · 2026-03-05T09:25:41Z

InterfacePlayerRDK.cpp

 	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);


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).

Copilot · 2026-03-05T09:25:41Z

InterfacePlayerRDK.cpp

+	else
+	{
+#if 0
+            PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",
+            formatType,
+            protSystemId ? protSystemId : "",
+            mediaType,
+            initDataSize);
+#endif
+	}


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.

Copilot · 2026-03-05T09:25:41Z

test/utests/run.sh

+PLAYER_BUILD_GCNO=""
+
+if [ "$build_coverage" -eq "1" ]; then
+    #Find where player .gcno files get put when player-cli is built via install-middleware.sh -c
+    A_GCNO=$(find ${PLAYERDIR}/build -name 'AampConfig*gcno' -print -quit)
+
+    if [ -z "$A_GCNO" ]; then
+        echo "ERROR need to run 'install-middleware.sh -c' first to get baseline list of middleware files for coverage"
+        exit 1
+    fi
+    PLAYER_BUILD_GCNO=$(dirname $A_GCNO)
+fi


PLAYER_BUILD_GCNO is computed but never used later in the script. If coverage needs this directory, wire it into the subsequent lcov capture; otherwise, remove the variable and the lookup to keep the script maintainable.

Copilot · 2026-03-05T09:25:42Z

PlayerTelemetry2.cpp

+    if(false == m_Initialized)
+    {
+        m_Initialized = true;
+        t2_init((char *)"mwplayer");
+        MW_LOG_MIL("t2_init done ");


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.

Copilot

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.

Copilot · 2026-03-06T09:15:20Z

InterfacePlayerRDK.cpp

+		f["rate"] = privatePlayer->gstPrivateContext->rate;
+
+		PlayerTelemetry2 telemetry;
+		telemetry.send("MW_BUFFER_UNDERFLOW", i, s, f);
+#endif


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.

Copilot · 2026-03-06T09:15:20Z

PlayerTelemetry2.cpp

+    bool bRet = false;
+    if(mInitializer.isInitialized()	)
+    {
+        cJSON *root = cJSON_CreateObject();


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;

}

Copilot · 2026-03-06T09:15:21Z

PlayerTelemetry2.hpp

+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;


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;

Copilot · 2026-03-06T09:15:21Z

InterfacePlayerRDK.cpp

+	else
+	{
+#if 0
+            PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",
+            formatType,
+            protSystemId ? protSystemId : "",
+            mediaType,
+            initDataSize);
+#endif
+	}


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

}

Copilot · 2026-03-06T09:15:21Z

InterfacePlayerRDK.cpp

+#if 0 
+	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["cnt"] = privatePlayer->gstPrivateContext->decodeErrorCBCount;
+
+	// Float values
+	f["delta"] = static_cast<float>(deltaMS);
+	f["rate"]  = privatePlayer->gstPrivateContext->rate;
+
+	PlayerTelemetry2 telemetry;
+	telemetry.send("MW_DECODE_ERROR", i, s, f);
+#endif


This #if 0 telemetry block is dead code. Please remove it or hook it up behind PLAYER_TELEMETRY_SUPPORT so it stays buildable/testable.

Copilot · 2026-03-06T09:15:22Z

InterfacePlayerRDK.cpp

+#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


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.

Copilot · 2026-03-06T09:15:22Z

PlayerTelemetry2.cpp

+void Player_TelemetryInitializer::Init()
+{
+    if(false == m_Initialized)
+    {
+        m_Initialized = true;
+        t2_init((char *)"mwplayer");
+        MW_LOG_MIL("t2_init done ");
+    }


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.

Copilot · 2026-03-06T09:15:22Z

PlayerTelemetry2.hpp

+// 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. 


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.

Copilot · 2026-03-06T09:15:23Z

InterfacePlayerRDK.cpp

+#if 0 
+	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);
+#endif


This #if 0 telemetry block is dead code and will not be compiled/tested. Please remove it or put it behind PLAYER_TELEMETRY_SUPPORT (and ensure it compiles) to prevent bit-rot.

Copilot · 2026-03-06T09:15:23Z

InterfacePlayerRDK.cpp

+#if 0
+    	         PlayerTelemetry2::send("MW_BUFFERING_TIMEOUT",
+                 privatePlayer->gstPrivateContext->numberOfVideoBuffersSent,
+                 privatePlayer->gstPrivateContext->buffering_timeout_cnt,
+                 privatePlayer->gstPrivateContext->rate,
+                 isBufferingTimeoutConditionMet,
+                 isRateCorrectionDefaultOnPlaying,
+                 isPlayerReady);
+#endif


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.

Copilot

Pull request overview

Copilot reviewed 4 out of 4 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.

Copilot · 2026-03-06T17:18:37Z

CMakeLists.txt

+	set(LIBPLAYERGSTINTERFACE_SOURCES "${LIBPLAYERGSTINTERFACE_SOURCES}" PlayerTelemetry2.cpp)
+	set(LIBPLAYERGSTINTERFACE_DEFINES "${LIBPLAYERGSTINTERFACE_DEFINES} -DPLAYER_TELEMETRY_SUPPORT=1")
+	set(LIBPLAYERGSTINTERFACE_DEPENDS ${LIBPLAYERGSTINTERFACE_DEPENDS} "-ltelemetry_msgsender")


Appending PlayerTelemetry2.cpp to LIBPLAYERGSTINTERFACE_SOURCES via set(LIBPLAYERGSTINTERFACE_SOURCES "${LIBPLAYERGSTINTERFACE_SOURCES}" PlayerTelemetry2.cpp) can accidentally turn a list into a single semicolon-delimited string and behaves differently from other source additions in this CMakeLists. Prefer list(APPEND LIBPLAYERGSTINTERFACE_SOURCES PlayerTelemetry2.cpp) (or unquoted ${LIBPLAYERGSTINTERFACE_SOURCES}) for consistent list handling.

Suggested change

set(LIBPLAYERGSTINTERFACE_SOURCES "${LIBPLAYERGSTINTERFACE_SOURCES}" PlayerTelemetry2.cpp)

set(LIBPLAYERGSTINTERFACE_DEFINES "${LIBPLAYERGSTINTERFACE_DEFINES} -DPLAYER_TELEMETRY_SUPPORT=1")

set(LIBPLAYERGSTINTERFACE_DEPENDS ${LIBPLAYERGSTINTERFACE_DEPENDS} "-ltelemetry_msgsender")

list(APPEND LIBPLAYERGSTINTERFACE_SOURCES PlayerTelemetry2.cpp)

set(LIBPLAYERGSTINTERFACE_DEFINES ${LIBPLAYERGSTINTERFACE_DEFINES} -DPLAYER_TELEMETRY_SUPPORT=1)

set(LIBPLAYERGSTINTERFACE_DEPENDS ${LIBPLAYERGSTINTERFACE_DEPENDS} -ltelemetry_msgsender)

Copilot · 2026-03-06T17:18:38Z

PlayerTelemetry2.cpp

+void Player_TelemetryInitializer::Init()
+{
+    if(false == m_Initialized)
+    {
+        m_Initialized = true;
+        t2_init((char *)"mwplayer");
+        MW_LOG_MIL("t2_init done ");
+    }
+}


Player_TelemetryInitializer::Init() is not thread-safe: m_Initialized is a plain bool accessed without synchronization, so concurrent construction of PlayerTelemetry2 (e.g., from different GStreamer threads) can race and cause a data race / multiple t2_init() calls. Consider guarding initialization with std::once_flag/std::call_once (or a mutex/atomic) and make m_Initialized updates synchronized.

Copilot · 2026-03-06T17:18:38Z

PlayerTelemetry2.cpp

+        t2_init((char *)"mwplayer");
+        MW_LOG_MIL("t2_init done ");


Casting a string literal to char* ((char*)"mwplayer") is undefined behavior if the callee ever writes to it. Prefer passing a const char* if the API supports it, or copy into a mutable buffer before calling t2_init().

Copilot · 2026-03-06T17:18:38Z

PlayerTelemetry2.cpp

+        cJSON *root = cJSON_CreateObject();
+
+        cJSON_AddStringToObject(root, "app", appName.c_str());
+
+        for (const auto& pair : intData) {
+            std::string key = pair.first;
+            int value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        for (const auto& pair : stringData) {
+            std::string key = pair.first;
+            std::string value = pair.second;
+            cJSON_AddStringToObject(root, key.c_str(), value.c_str());
+        }
+
+        for (const auto& pair : floatData) {
+            std::string key = pair.first;
+            float value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        //lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.
+        char *jsonString = cJSON_PrintUnformatted(root);
+
+        MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(),jsonString);
+
+        T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);
+


cJSON_CreateObject() / cJSON_PrintUnformatted() can return NULL on allocation failure. The current code unconditionally dereferences/prints jsonString (%s) and uses root without checks, which can crash. Add null checks and ensure cleanup happens on all failure paths before calling t2_event_s().

Copilot · 2026-03-06T17:18:38Z

PlayerTelemetry2.cpp

+    if(mInitializer.isInitialized()	&& NULL != data)
+    {
+        MW_LOG_INFO("[S] Marker Name: %s value:%s", markerName.c_str(),data );
+        T2ERROR t2Error =  t2_event_s( (char *)markerName.c_str(),(char*)data );
+


send(const std::string&, const char*) casts away const on both markerName.c_str() and data when calling t2_event_s(). If t2_event_s() writes to either buffer, this is undefined behavior. Prefer an API that takes const char*, or make an explicit mutable copy before calling into the telemetry library.

Copilot · 2026-03-06T17:18:39Z

PlayerTelemetry2.hpp

+#define __PLAYER_TELEMETRY_2_H__
+
+#include <cjson/cJSON.h>
+#include <iostream>


#include <iostream> in this header appears unused (the implementation uses logging macros instead). Removing unused includes reduces compile time and avoids pulling extra headers transitively.

Suggested change

#include <iostream>

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 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.

Copilot · 2026-03-11T07:15:54Z

PlayerTelemetry2.cpp

+}
+
+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 ", markerName.c_str());


This logs the marker name at error level for every telemetry send (MW_LOG_ERR), which will pollute error logs during normal operation. Consider lowering this to INFO/DEBUG (or only logging on failure) to keep error logs actionable.

Suggested change

MW_LOG_ERR("[M] Marker Name: %s ", markerName.c_str());

MW_LOG_INFO("[M] Marker Name: %s ", markerName.c_str());

Copilot · 2026-03-11T07:15:55Z

PlayerTelemetry2.cpp

+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 ", markerName.c_str());
+    bool bRet = false;
+    if(mInitializer.isInitialized()	)
+    {
+        cJSON *root = cJSON_CreateObject();
+
+        cJSON_AddStringToObject(root, "app", appName.c_str());
+
+        for (const auto& pair : intData) {
+            std::string key = pair.first;
+            int value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        for (const auto& pair : stringData) {
+            std::string key = pair.first;
+            std::string value = pair.second;
+            cJSON_AddStringToObject(root, key.c_str(), value.c_str());
+        }
+
+        for (const auto& pair : floatData) {
+            std::string key = pair.first;
+            float value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        //lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.
+        char *jsonString = cJSON_PrintUnformatted(root);
+
+        MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(),jsonString);
+
+        T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);
+
+        if(T2ERROR_SUCCESS == t2Error)
+        {
+            bRet = true;
+        }
+        else
+        {
+            MW_LOG_ERR("t2_event_s map failed:%d ", t2Error);
+        }
+        cJSON_free(jsonString);
+        cJSON_Delete(root);
+    }
+
+    return bRet;


New telemetry JSON construction + send behavior is introduced here, but there are no unit tests validating (1) JSON contents for mixed int/string/float maps and (2) behavior when cJSON_* allocations fail or when telemetry isn't initialized. Given the repo has a utest framework, consider adding tests with a mock/stub for the t2_event_s API to lock down expected payloads and error paths.

Copilot · 2026-03-11T07:15:55Z

PlayerTelemetry2.cpp

+        cJSON_AddStringToObject(root, "app", appName.c_str());
+
+        for (const auto& pair : intData) {
+            std::string key = pair.first;
+            int value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        for (const auto& pair : stringData) {
+            std::string key = pair.first;
+            std::string value = pair.second;
+            cJSON_AddStringToObject(root, key.c_str(), value.c_str());
+        }
+
+        for (const auto& pair : floatData) {
+            std::string key = pair.first;
+            float value = pair.second;
+            cJSON_AddNumberToObject(root, key.c_str(), value);
+        }
+
+        //lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.
+        char *jsonString = cJSON_PrintUnformatted(root);
+
+        MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(),jsonString);
+
+        T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);
+
+        if(T2ERROR_SUCCESS == t2Error)
+        {
+            bRet = true;
+        }
+        else
+        {
+            MW_LOG_ERR("t2_event_s map failed:%d ", t2Error);
+        }
+        cJSON_free(jsonString);
+        cJSON_Delete(root);


cJSON_CreateObject() and cJSON_PrintUnformatted() return pointers that can be NULL on allocation failure. The current code dereferences root immediately and logs jsonString with %s, which can crash in low-memory scenarios. Add null checks and ensure you only call cJSON_Delete/cJSON_free when the allocations succeeded.

Suggested change

cJSON_AddStringToObject(root, "app", appName.c_str());

for (const auto& pair : intData) {

std::string key = pair.first;

int value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

}

for (const auto& pair : stringData) {

std::string key = pair.first;

std::string value = pair.second;

cJSON_AddStringToObject(root, key.c_str(), value.c_str());

}

for (const auto& pair : floatData) {

std::string key = pair.first;

float value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

}

//lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.

char *jsonString = cJSON_PrintUnformatted(root);

MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(),jsonString);

T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(),jsonString);

if(T2ERROR_SUCCESS == t2Error)

{

bRet = true;

}

else

{

MW_LOG_ERR("t2_event_s map failed:%d ", t2Error);

}

cJSON_free(jsonString);

cJSON_Delete(root);

if (root == NULL)

{

MW_LOG_ERR("Failed to create cJSON root object for marker: %s", markerName.c_str());

}

else

{

cJSON_AddStringToObject(root, "app", appName.c_str());

for (const auto& pair : intData) {

std::string key = pair.first;

int value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

}

for (const auto& pair : stringData) {

std::string key = pair.first;

std::string value = pair.second;

cJSON_AddStringToObject(root, key.c_str(), value.c_str());

}

for (const auto& pair : floatData) {

std::string key = pair.first;

float value = pair.second;

cJSON_AddNumberToObject(root, key.c_str(), value);

}

//lets use cJSON_PrintUnformatted , cJSON_Print is formated adds whitespace n hence takes more memory also eats up more logs if logged.

char *jsonString = cJSON_PrintUnformatted(root);

if (jsonString == NULL)

{

MW_LOG_ERR("Failed to serialize cJSON object for marker: %s", markerName.c_str());

}

else

{

MW_LOG_INFO("[M] Marker Name: %s value:%s", markerName.c_str(), jsonString);

T2ERROR t2Error = t2_event_s( (char *)markerName.c_str(), jsonString);

if(T2ERROR_SUCCESS == t2Error)

{

bRet = true;

}

else

{

MW_LOG_ERR("t2_event_s map failed:%d ", t2Error);

}

cJSON_free(jsonString);

}

cJSON_Delete(root);

}

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 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.

Copilot · 2026-03-12T07:36:23Z

PlayerTelemetry2.cpp

+        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());
+
+    bool init = mInitializer.isInitialized();
+    if(init)
+    {
+        MW_LOG_ERR("[M] Inside initializer block");
+
+        cJSON *root = cJSON_CreateObject();
+        if(!root)
+        {
+            MW_LOG_ERR("[M] cJSON_CreateObject failed");
+            return false;
+        }
+
+        MW_LOG_ERR("[M] JSON object created");
+
+        cJSON_AddStringToObject(root, "app", appName.c_str());
+        MW_LOG_ERR("[M] appName added: %s", appName.c_str());
+
+        for (const auto& pair : intData)
+        {
+            MW_LOG_ERR("[M] int key=%s value=%d", pair.first.c_str(), pair.second);
+            cJSON_AddNumberToObject(root, pair.first.c_str(), pair.second);
+        }
+
+        for (const auto& pair : stringData)
+        {
+            MW_LOG_ERR("[M] string key=%s value=%s", pair.first.c_str(), pair.second.c_str());
+            cJSON_AddStringToObject(root, pair.first.c_str(), pair.second.c_str());
+        }
+
+        for (const auto& pair : floatData)
+        {
+            MW_LOG_ERR("[M] float key=%s value=%f", pair.first.c_str(), pair.second);
+            cJSON_AddNumberToObject(root, pair.first.c_str(), pair.second);
+        }
+
+        char *jsonString = cJSON_PrintUnformatted(root);
+        if(!jsonString)
+        {
+            MW_LOG_ERR("[M] cJSON_PrintUnformatted failed");
+            cJSON_Delete(root);
+            return false;
+        }
+
+        MW_LOG_ERR("[M] Marker Name: %s | JSON: %s", markerName.c_str(), jsonString);
+
+        T2ERROR t2Error = t2_event_s((char *)markerName.c_str(), jsonString);
+        MW_LOG_ERR("[M] t2_event_s returned: %d", t2Error);
+
+        if(T2ERROR_SUCCESS == t2Error)
+        {
+            MW_LOG_ERR("[M] Telemetry event sent successfully");
+            bRet = true;
+        }
+        else
+        {
+            MW_LOG_ERR("[M] Telemetry event failed: %d", t2Error);
+        }
+
+        cJSON_free(jsonString);
+        cJSON_Delete(root);
+    }
+    else
+    {
+        MW_LOG_ERR("[M] Telemetry initializer not ready");
+    }
+
+    MW_LOG_ERR("[M] Exiting send() | marker: %s", markerName.c_str());
+    return bRet;


Debug/development log statements left in production code: the send() method is filled with MW_LOG_ERR calls (lines 43, 47-48, 53, 62, 65, 69, 75, 81, 93, 96, 100, 105, 113, 116) that log at ERROR level for what is essentially debug/trace information. These should either be removed or downgraded to an appropriate log level (e.g., MW_LOG_DEBUG or MW_LOG_INFO).

Copilot · 2026-03-12T07:36:23Z

InterfacePlayerRDK.cpp

+#if 0
+            PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",
+            formatType,
+            protSystemId ? protSystemId : "",
+            mediaType,
+            initDataSize);
+#endif


The #if 0 block calls PlayerTelemetry2::send(...) as a static method with arguments that don't match any declared overload of send. The send methods are non-static instance methods. Even though this is currently disabled, it won't compile if enabled. Similarly, the #if 0 block at buffering_timeout (line 4697) calls send as static with wrong arguments. Either fix these call signatures or remove the dead code.

Suggested change

#if 0

PlayerTelemetry2::send("MW_PROTECTION_EVENT_FAILED",

formatType,

protSystemId ? protSystemId : "",

mediaType,

initDataSize);

#endif

// Telemetry for protection event failures is currently disabled.

// If re-enabled in the future, ensure PlayerTelemetry2 is used via a valid instance method.

Copilot · 2026-03-12T07:36:24Z

InterfacePlayerRDK.cpp

+#ifdef PLAYER_TELEMETRY_SUPPORT /** verifying telemetry support*/
+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");
+#else
+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");
+#endif


The compile-time #ifdef / #else check at lines 364-368 logs "enabled at runtime" / "NOT enabled at runtime", but this is a compile-time check, not a runtime check. The messages are misleading. Also, these appear to be debug-only diagnostics that should be removed before merging.

Suggested change

#ifdef PLAYER_TELEMETRY_SUPPORT /** verifying telemetry support*/

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");

#else

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");

#endif

Copilot · 2026-03-12T07:36:24Z

InterfacePlayerRDK.cpp

+#if 0
+    	         PlayerTelemetry2::send("MW_BUFFERING_TIMEOUT",
+                 privatePlayer->gstPrivateContext->numberOfVideoBuffersSent,
+                 privatePlayer->gstPrivateContext->buffering_timeout_cnt,
+                 privatePlayer->gstPrivateContext->rate,
+                 isBufferingTimeoutConditionMet,
+                 isRateCorrectionDefaultOnPlaying,
+                 isPlayerReady);
+#endif


The #if 0 dead code block and unreachable code after return on line 4695. This telemetry call (lines 4696-4704) is placed after a return statement and will never execute even if the #if 0 were changed to #if 1.

Copilot

Pull request overview

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

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

Copilot · 2026-03-31T07:41:23Z

PlayerTelemetry2.cpp

+Player_TelemetryInitializer::~Player_TelemetryInitializer()
+{
+    t2_uninit();
+    MW_LOG_MIL("t2_uninit done ");
+}


Player_TelemetryInitializer::~Player_TelemetryInitializer() calls t2_uninit() unconditionally. If Init() was never called (or t2_init() failed), uninitializing may be unsafe. Guard uninit with m_Initialized (and consider tracking t2_init success).

Copilot · 2026-03-31T07:41:23Z

PlayerTelemetry2.cpp

+void Player_TelemetryInitializer::Init()
+{
+    if(false == m_Initialized)
+    {
+        m_Initialized = true;
+        t2_init((char *)"mwplayer");
+        MW_LOG_MIL("t2_init done ");
+    }
+}


Player_TelemetryInitializer::Init() is not thread-safe: multiple threads can race on m_Initialized (data race) and call t2_init() multiple times. Consider std::once_flag/std::call_once or an atomic + mutex to make initialization safe.

Copilot · 2026-03-31T07:41:25Z

PlayerTelemetry2.cpp

+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());
+


This send() implementation logs many normal control-flow messages using MW_LOG_ERR, which will treat expected telemetry sends as errors and can flood error logs in production. Consider lowering these to DEBUG/TRACE (or removing) and reserving ERROR for actual failures.

Copilot · 2026-03-31T07:41:28Z

InterfacePlayerRDK.cpp

+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");
+#else
+    MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");
+#endif
+		MW_LOG_MIL("Nitz : Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);


These log lines say telemetry support is "enabled at runtime", but PLAYER_TELEMETRY_SUPPORT is a compile-time define. Also, the "Nitz :" prefix looks like a leftover debug tag and should be removed or replaced with a meaningful message.

Suggested change

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is enabled at runtime");

#else

MW_LOG_MIL("PLAYER_TELEMETRY_SUPPORT is NOT enabled at runtime");

#endif

MW_LOG_MIL("Nitz : Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);

MW_LOG_MIL("Telemetry support enabled (PLAYER_TELEMETRY_SUPPORT defined at compile time)");

#else

MW_LOG_MIL("Telemetry support disabled (PLAYER_TELEMETRY_SUPPORT not defined at compile time)");

#endif

MW_LOG_MIL("Creating pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);

Copilot · 2026-03-31T07:41:30Z

InterfacePlayerRDK.cpp

+	MW_LOG_ERR("GstPlayer_OnGstPtsErrorCb: Got PTS error message from %s", GST_ELEMENT_NAME(object));
 	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);
+#endif


Telemetry for PTS errors is emitted before isVideo / isAudioSink are computed, so vid/aud are always reported as 0. Move the telemetry block after determining whether the element is video/audio (or populate these fields afterward).

Copilot · 2026-03-31T07:41:32Z

InterfacePlayerRDK.cpp

+				else
+				{
+					busEvent.dbg_info[0] = '\0';
+				}


When dbg_info is null, this writes to busEvent.dbg_info[0] even though busEvent.dbg_info is an empty std::string here, which is undefined behavior. Use busEvent.dbg_info.clear() or busEvent.dbg_info = "" instead.

Copilot · 2026-03-31T07:41:33Z

PlayerTelemetry.h

+/**
+ * @class PlayerTelemetry
+ * @brief Static helper for emitting named telemetry events with optional payload data.
+ *
+ * Active implementation compiled when PLAYER_TELEMETRY_SUPPORT is defined.
+ */
+class PlayerTelemetry
+{
+public:
+    /**
+     * @brief Emit a telemetry event with no additional payload.
+     * @param[in] eventName  One of the TELEMETRY_EVENT_* markers from TelemetryMarkers.h.
+     */
+    static void sendEvent(const std::string& eventName)
+    {
+        MW_LOG_MIL("[TELEMETRY] event=%s", eventName.c_str());
+    }


The documentation and TelemetryMarkers.h comments describe markers as being emitted via PlayerTelemetry::sendEvent(), and PLAYER_TELEMETRY_SUPPORT implies real telemetry emission. However, the enabled implementation currently only logs to MW_LOG_MIL. If telemetry 2.0 is required, consider routing sendEvent() to the telemetry bus (e.g., via PlayerTelemetry2) or update the docs/flag naming to avoid implying events are being sent when they are not.

Copilot · 2026-03-31T07:41:34Z

InterfacePlayerRDK.cpp

+				i["tgt"]  = static_cast<int>(targetState);
+
+				 PlayerTelemetry2 telemetry;
+				 telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);


Hard-coded marker string "MW_PIPELINE_STATE_CHANGE_FAILURE" duplicates TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE from TelemetryMarkers.h. Using the macro here avoids drift/typos and keeps markers consistent across PlayerTelemetry/PlayerTelemetry2 call sites.

Suggested change

telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);

telemetry.send(TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE, i, s, f);

Deepikasri1234 added 6 commits February 17, 2026 16:26

RDKEMW-13310: Telemetry T2 markers for Player-interface component

313035d

Reason for change : telemetry marker implementation . Test Steps : . Signed-off by: Deepikasri Natarajan (deepikasri_n@comcast.com)

T2 changes

beae9af

resovling compilation issues

71a8b63

underflow msg added

b9d1c24

static code usage is removed

7c20fb0

Merge branch 'develop' into feature/RDKEMW-13310

aaa77d4

dp0000 requested a review from a team as a code owner February 23, 2026 09:04

Copilot AI review requested due to automatic review settings February 23, 2026 09:04

Copilot started reviewing on behalf of dp0000 February 23, 2026 09:05 View session

resolving conflicts

64366e1

Copilot AI reviewed Feb 23, 2026

View reviewed changes

Deepikasri1234 added 2 commits March 4, 2026 15:25

added error PTS conditions

f404e6e

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

7339125

Copilot AI review requested due to automatic review settings March 4, 2026 09:59

Copilot started reviewing on behalf of dp0000 March 4, 2026 10:00 View session

Copilot AI reviewed Mar 4, 2026

View reviewed changes

Deepikasri1234 added 2 commits March 5, 2026 00:02

resolved compilation isseus

e4d686e

resolved compilation issues

9148d7e

Copilot AI review requested due to automatic review settings March 5, 2026 09:17

Copilot started reviewing on behalf of dp0000 March 5, 2026 09:17 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

b0ace39

dp0000 changed the base branch from develop to feature/dev_sprint_pli March 6, 2026 08:58

removed additional logic handled

1692915

Copilot AI review requested due to automatic review settings March 6, 2026 09:07

Copilot started reviewing on behalf of dp0000 March 6, 2026 09:08 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

added prints

3a28ef6

fixed compilation issues

0ccd026

Copilot AI review requested due to automatic review settings March 6, 2026 17:13

Copilot started reviewing on behalf of dp0000 March 6, 2026 17:14 View session

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Deepikasri1234 added 2 commits March 9, 2026 23:50

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

6107254

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

b67c5a6

Copilot AI review requested due to automatic review settings March 11, 2026 07:09

Copilot started reviewing on behalf of dp0000 March 11, 2026 07:09 View session

Copilot AI reviewed Mar 11, 2026

View reviewed changes

Deepikasri1234 added 2 commits March 12, 2026 13:02

added debug

3c90d8a

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

fe2013c

Copilot AI review requested due to automatic review settings March 12, 2026 07:34

Copilot started reviewing on behalf of dp0000 March 12, 2026 07:34 View session

Copilot AI reviewed Mar 12, 2026

View reviewed changes

Deepikasri1234 added 2 commits March 25, 2026 11:27

added few telemetry markers

16117e1

added new set of markers

4f6db42

Copilot AI review requested due to automatic review settings March 31, 2026 07:34

Copilot started reviewing on behalf of dp0000 March 31, 2026 07:35 View session

Merge branch 'feature/dev_sprint_pli' into feature/RDKEMW-13310

c13064e

Copilot AI reviewed Mar 31, 2026

View reviewed changes

	// 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

	* @param[in] data - Data to be sent
	* @param[in] data - Data to be sent
	* @return bool - true if success, false otherwise

		else
		MW_LOG_WARN("unknown mediaFormat %d", mDrmInfo.mediaFormat);

	(find . -name "*.gcda" -print0 \| xargs -0 rm) \|\| true
	(find . -name "*.gcda" -print0 \| xargs -0 -r rm) \|\| true

-        print("No 'diff --git' found in the patch file")
-        return
+        print("No 'diff --git' found in the patch file", file=sys.stderr)
+        sys.exit(1)

-// 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.

	MW_LOG_ERR("[M] Marker Name: %s ", markerName.c_str());
	MW_LOG_INFO("[M] Marker Name: %s ", markerName.c_str());

	telemetry.send("MW_PIPELINE_STATE_CHANGE_FAILURE", i, s, f);
	telemetry.send(TELEMETRY_EVENT_PIPELINE_STATE_CHANGE_FAILURE, i, s, f);

Conversation

dp0000 commented Feb 23, 2026

Uh oh!

github-actions bot commented Feb 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 4, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

github-actions bot commented Feb 23, 2026 •

edited

Loading