RDKEMW-13310: Adding Telemetry markers in Player-Interface component

[dp0000]

No description provided.

[github-actions]

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]

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]

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]

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]

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

[Copilot]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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]

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 10 out of 10 changed files in this pull request and generated 14 comments.

Comments suppressed due to low confidence (1)

InterfacePlayerRDK.cpp:3603

• This branch is reached when validateStateWithMsTimeout() succeeded (state reached nextState), but it still logs "FAILED" and performs a retry/recovery sequence that can incorrectly flip retValue to false even though the transition already succeeded. The retry logic should run only on failure.

				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));
				
				/* Recovery: retry the state change once before reporting failure */
				MW_LOG_INFO("InterfacePlayerRDK_Pause - retrying state change to GstState %d", nextState);

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (12)

TelemetryMarkers.h:71

• The BUFFERING_/ERROR_/PIPELINE_STATE_CHANGE_FAILURE markers are defined twice, which can trigger macro redefinition warnings/errors (and makes the header harder to maintain). Remove the duplicated block (lines 61–71) and keep a single definition per marker.

/* ── 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:5

• <fstream> is included but not used in this translation unit, which can trigger -Wunused-include warnings and slows compilation slightly.

#include "PlayerTelemetry2.hpp"
#include <fstream>

#include <telemetry_busmessage_sender.h>

PlayerTelemetry2.cpp:36

• The default constructor creates a temporary PlayerTelemetry2("") instead of delegating, so mInitializer.Init() is never called and appName is left default-initialized. This also means later sendEvent() can call into T2 without initialization.

PlayerTelemetry2::PlayerTelemetry2() {
    PlayerTelemetry2("");
}

PlayerTelemetry2.cpp:144

• sendEvent() computes init but never uses it, and unconditionally calls t2_event_d() even when telemetry hasn't been initialized. This can lead to undefined behavior if t2_init() was not run.

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.hpp:7

• The include guard uses a double-underscore identifier (__PLAYER_TELEMETRY_2_H__), which is reserved for the implementation and can cause undefined behavior / collisions on some toolchains. Use a non-reserved macro name instead.

#ifndef __PLAYER_TELEMETRY_2_H__
#define __PLAYER_TELEMETRY_2_H__

PlayerTelemetry2.hpp:69

• Keep the #endif comment in sync with the renamed include guard macro to avoid confusion.

#endif // __PLAYER_TELEMETRY_2_H__

InterfacePlayerRDK.cpp:113

• The constructor emits the same INITIALIZED marker multiple times via two different telemetry APIs (and once with a payload), which will duplicate telemetry for a single constructor call. If the intent is just to ensure T2 is initialized, create an initializer once and emit the event only once (preferably via PlayerTelemetry).

#ifdef PLAYER_TELEMETRY_SUPPORT
	PlayerTelemetry2 telemetry;
	telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
	

InterfacePlayerRDK.cpp:401

• PLAYER_TELEMETRY_SUPPORT is a compile-time define, so logging it as "enabled at runtime" is misleading. Also the "Nitz :" prefix looks like a stray debug tag. Consider restoring the original log message without these extra lines.

            
#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
		MW_LOG_MIL("Nitz : Create pipeline %s (pipeline %p bus %p)", pipelineName, interfacePlayerPriv->gstPrivateContext->pipeline, interfacePlayerPriv->gstPrivateContext->bus);
		CreatePipeline(pipelineName, PipelinePriority); 		/*Create a new pipeline if pipeline or the message bus does not exist*/

InterfacePlayerRDK.cpp:602

• The seekPausedState branch contradicts the comment: it logs a PLAYING failure and sends a failure marker, but it never actually defers the PLAYING transition (no pendingPlayState/buffering_target_state update, and no explicit PAUSED enforcement). This changes playback behavior for keepPaused seeks.

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

InterfacePlayerRDK.cpp:623

• TELEMETRY_EVENT_PLAYBACK_STARTED is emitted before attempting the PLAYING state transition; if SetStateWithWarnings(...PLAYING) fails, telemetry will still show playback started. Emit PLAYBACK_STARTED only after a successful state change (and emit a failure marker on error).

#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)
			{
				MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
			}

InterfacePlayerRDK.cpp:1403

• Use the shared marker constant instead of a hard-coded string literal so the event name stays consistent with TelemetryMarkers.h and refactors don't miss this call site.

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

PlayerTelemetry2.cpp:19

• Player_TelemetryInitializer::Init() is not thread-safe: multiple threads can race on m_Initialized and call t2_init() more than once (there are many call sites that construct PlayerTelemetry2 in callbacks). Use std::call_once/std::once_flag (or equivalent) to guarantee single initialization per process.

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

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (10)

TelemetryMarkers.h:72

• Duplicate macro definitions (e.g., TELEMETRY_EVENT_BUFFERING_STARTED / ERROR / PIPELINE_STATE_CHANGE_FAILURE) are repeated later in the file. This will cause macro redefinition errors or hide accidental divergence between duplicates.

/* ── 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 (it does not delegate), so this instance never sets appName or calls mInitializer.Init(). As a result, call sites using PlayerTelemetry2 telemetry; will emit events without initialization.

PlayerTelemetry2::PlayerTelemetry2() {
    PlayerTelemetry2("");
}

PlayerTelemetry2.cpp:144

• sendEvent() ignores initialization status and also has an unused init local. If telemetry init is required by the backend, this can emit events before t2_init() and will produce an unused-variable warning.

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:130

• The InterfacePlayerRDK constructor emits the same INITIALIZED marker multiple times via two different telemetry wrappers. This will inflate metrics and make backend analysis ambiguous; initialize telemetry once, then emit a single event (ideally with payload).

#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:612

• The seekPausedState branch no longer defers the PLAYING transition (it logs an error but never sets pendingPlayState/buffering_target_state or forces PAUSED). This changes behavior described by the comment and can leave state flags inconsistent after a paused seek.

		/* 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:3613

• In the ASYNC state-change success path, the code logs "FAILED" and then unconditionally retries SetStateWithWarnings() again. This is contradictory and can cause extra transitions and misleading logs/telemetry.

				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));
				
				/* Recovery: retry the state change once before reporting failure */

InterfacePlayerRDK.cpp:3603

• When validateStateWithMsTimeout() fails, retValue is never set to false, so Pause() can report success even though the pipeline did not reach the requested state.

				MW_LOG_ERR("InterfacePlayerRDK_Pause - validateStateWithMsTimeout - FAILED GstState %d", nextState);
				{
					TelemetryPayload pauseTimeoutPayload;
					pauseTimeoutPayload.add("toState", pause ? "PAUSED" : "PLAYING");
					pauseTimeoutPayload.add("context", "Pause_timeout");

InterfacePlayerRDK.cpp:1402

• Use the shared marker constant instead of a hard-coded string so the event name stays consistent with TelemetryMarkers.h (and future renames don't miss this site).

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

PlayerTelemetry2.hpp:7

• The header guard macro __PLAYER_TELEMETRY_2_H__ uses a reserved identifier (double underscore + leading underscore). This is undefined behavior territory in C/C++; use a non-reserved guard name.

#ifndef __PLAYER_TELEMETRY_2_H__
#define __PLAYER_TELEMETRY_2_H__

InterfacePlayerRDK.cpp:623

• PLAYBACK_STARTED telemetry is emitted before attempting the actual state transition to PLAYING. If SetStateWithWarnings() fails, telemetry will still report a successful start.

#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)
			{
				MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
			}

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (7)

TelemetryMarkers.h:72

• Duplicate macro definitions (BUFFERING_, ERROR_, PIPELINE_STATE_CHANGE_FAILURE) are present later in this header, which will trigger macro redefinition warnings/errors and risks accidental divergence if one copy is edited.

/* ── 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:30

• Player_TelemetryInitializer unconditionally calls t2_uninit() even if Init() was never called; that can lead to unbalanced init/uninit (and potential issues depending on the telemetry library implementation).

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

PlayerTelemetry2.cpp:36

• The default constructor constructs a temporary PlayerTelemetry2(""), it does not delegate to the other constructor. As a result, this instance's appName is left default-initialized and mInitializer.Init() is not called (which breaks current call sites that use the default ctor).

PlayerTelemetry2::PlayerTelemetry2() {
    PlayerTelemetry2("");
}

PlayerTelemetry2.cpp:144

• sendEvent() computes init but doesn't use it and always calls t2_event_d(). With the current default-ctor bug this can call into T2 before initialization. Consider self-initializing (Init() is idempotent) and bailing out if initialization still isn't ready.

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:606

• In the seekPausedState branch, the code comment says we must defer the PLAYING transition, but the implementation now only logs/sends a failure marker and does not set pendingPlayState/buffering_target_state or ensure the pipeline stays PAUSED. This changes behavior and can break seek-with-keepPaused flows.

		if (interfacePlayerPriv->gstPrivateContext->seekPausedState)
		{
			MW_LOG_ERR("InterfacePlayerRDK: GST_STATE_PLAYING failed");
			{
				TelemetryPayload playingFailPayload;

InterfacePlayerRDK.cpp:398

• This block emits runtime log lines to report a compile-time #ifdef, and also adds a stray "Nitz :" prefix in the pipeline creation log. Both add noise to logs and the name prefix looks unintended.

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

InterfacePlayerRDK.cpp:113

• When PLAYER_TELEMETRY_SUPPORT is enabled, TELEMETRY_EVENT_INITIALIZED is emitted multiple times here (PlayerTelemetry2::sendEvent, PlayerTelemetry::sendEvent with payload, and PlayerTelemetry2::send). This can inflate metrics and make downstream interpretation ambiguous.

#ifdef PLAYER_TELEMETRY_SUPPORT
	PlayerTelemetry2 telemetry;
	telemetry.sendEvent(TELEMETRY_EVENT_INITIALIZED);
#endif
	

[Copilot]

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 init but 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 ");
}