RDKEMW-11877: Metrics Collection for Player Interface Public APIs - Phase1 by nejuma1 · Pull Request #66 · rdkcentral/middleware-player-interface

nejuma1 · 2026-01-21T11:17:28Z

RDKEMW-11877 : Metrics Collection for Player Interface Public APIs - Phase1
Reason for change : Integrating code for metrics collection
Test Procedure : Enable info logs to see metrics related logs
Priority : P1
Risks : None
Signed-off-by : nejumathoppil_nazar@comcast.com

Merge pull request #52 from rdkcentral/feature/RDKEMW-9809

Reason for change : Integrating code for metrics collection Test Procedure : Enable info logs to see metrics related logs Priority : P1 Risks : None Signed-off-by : nejumathoppil_nazar@comcast.com

Copilot

Pull request overview

This pull request implements Phase 1 of metrics collection for Player Interface Public APIs by introducing a profiling infrastructure and comprehensive test coverage for DRM key management functionality.

Changes:

Added performance profiling infrastructure with MW_PROFILE_FUNCTION() macro
Implemented extensive test suites for PlayerUtils (RawKeyToKeyId) and DrmSessionManager (multi-key validation)
Updated base64_Decode API signature across codebase to include explicit length parameter

Reviewed changes

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

Show a summary per file

File	Description
PerfProfiler.h/cpp	New profiling infrastructure using scoped timers for function execution tracking
InterfacePlayerRDK.cpp	Added profiling macros to 40+ public API functions
CMakeLists.txt	Added ENABLE_MW_PROFILING option and build configuration
test/utests/tests/PlayerUtilsTests/*	New comprehensive tests for RawKeyToKeyId hex conversion function
test/utests/tests/DrmSessionManagerTests/*	New tests for multi-key DRM scenarios with real PSSH data
test/utests/tests/OCDMSessionAdapter/KeyUpdateTests.cpp	New tests for OCDM key update functionality
drm/ocdm/opencdmsessionadapter.cpp	Refactored keyUpdateOCDM to handle usable keys tracking consistently
drm/DrmSessionManager.cpp	Enhanced ValidateMultiKeySlot with improved key normalization logic
baseConversion/_base64.*	Removed overloaded base64_Decode, unified signature with explicit length
externals/contentsecuritymanager/*	Fixed spelling errors and added profiling macros
test/utests/mocks/*	Updated mocks and added new MockDrmSessionFactory
test/utests/fakes/*	Added fake implementations for ContentSecurityManager components

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

Copilot · 2026-01-21T11:24:50Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	// Get all key IDs from helper
+	std::map<int, std::vector<uint8_t>> keyIDs;
+	drmHelper->getKeys(keyIDs);
+	ASSERT_EQ(keyIDs.size(), 3); // Should have 3 key IDs (duplicates)


The comment states "Should have 3 key IDs (duplicates)" but Widevine helpers typically deduplicate keys. This comment should be verified against actual Widevine helper behavior. If keys are deduplicated, the expected value should be 1, not 3, and the comment should reflect that.

Copilot · 2026-01-21T11:24:50Z

CMakeLists.txt

+	PerfProfiler.h
 )
+if(ENABLE_MW_PROFILING)
+    list(APPEND LIBPLAYERGSTINTERFACE_HEADERS PerfProfiler.h)
+endif()


PerfProfiler.h is added to the LIBPLAYERGSTINTERFACE_HEADERS list twice: once unconditionally on line 169, and then conditionally on lines 171-173 if ENABLE_MW_PROFILING is enabled. This will result in duplicate entries when profiling is enabled. The unconditional addition on line 169 should be removed since the conditional block already handles adding it when needed.

Copilot · 2026-01-21T11:24:50Z

CMakeLists.txt

+	PerfProfiler.h
 	DESTINATION include)
+if(ENABLE_MW_PROFILING)
+    install(FILES PerfProfiler.h DESTINATION include)
+endif()


PerfProfiler.h is listed in the install FILES command twice: once unconditionally on line 210, and then conditionally on lines 212-214 if ENABLE_MW_PROFILING is enabled. This will result in duplicate installation entries when profiling is enabled. The unconditional addition on line 210 should be removed since the conditional block already handles installing it when needed.

Copilot · 2026-01-21T11:24:50Z

CMakeLists.txt

+	PerfProfiler.cpp
 )
+if(ENABLE_MW_PROFILING)
+    list(APPEND SOURCES PerfProfiler.cpp)
+endif()


PerfProfiler.cpp is added to the SOURCES list twice: once unconditionally on line 228, and then conditionally on lines 230-232 if ENABLE_MW_PROFILING is enabled. This will result in duplicate source file entries when profiling is enabled, potentially causing build errors. The unconditional addition on line 228 should be removed since the conditional block already handles adding it when needed.

Copilot · 2026-01-21T11:24:51Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	// Get extracted key IDs
+	std::map<int, std::vector<uint8_t>> keyIDs;
+	drmHelper->getKeys(keyIDs);
+	ASSERT_EQ(keyIDs.size(), 2); // Should have exactly 1 key (content ID should not be counted)


The comment says "Should have exactly 1 key (content ID should not be counted)" but the assertion expects size to be 2. This is inconsistent. Based on the comment, either the assertion should be ASSERT_EQ(keyIDs.size(), 1), or the comment should be updated to clarify that there is "1 key ID + 1 content ID = 2 total entries".

Copilot · 2026-01-21T11:24:51Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	drmHelper->getKeys(keyIDs);
+
+	// Should have 3 key IDs + 1 content ID = 4 total entries
+	ASSERT_EQ(keyIDs.size(), 4);


The comment says "Should have 3 key IDs + 1 content ID = 4 total entries" but the assertion expects size to be 4. The math is correct (3 key IDs + 1 content ID = 4), so this is accurate. However, earlier in line 960, the comment says "Should have 3 key IDs + 1 content ID = 4 total entries" which should be verified for consistency with the actual data structure.

Copilot · 2026-01-21T11:24:51Z

test/utests/mocks/MockDrmSessionFactory.h

+* If not stated otherwise in this file or this component's license file the
+* following copyright and licenses apply:
+*
+* Copyright 2024 RDK Management


The copyright year in this file header is 2024, but other new files in this PR use 2025. For consistency with the rest of the PR, this should be updated to 2025.

Suggested change

* Copyright 2024 RDK Management

* Copyright 2025 RDK Management

Copilot · 2026-01-21T11:24:51Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	// Get all key IDs from helper
+	std::map<int, std::vector<uint8_t>> keyIDs;
+	drmHelper->getKeys(keyIDs);
+	ASSERT_EQ(keyIDs.size(), 6); // Should have 5 key IDs


The comment says "Should have 5 key IDs" but the assertion checks for 6. This inconsistency between the comment and the code logic should be corrected. Either the comment should be updated to "Should have 6 key IDs (5 key IDs + 1 content ID)" or the expected value in the assertion should match the comment.

Copilot · 2026-01-21T11:24:52Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	// Get key IDs
+	std::map<int, std::vector<uint8_t>> keyIDs;
+	drmHelper->getKeys(keyIDs);
+	ASSERT_EQ(keyIDs.size(), 2); // Should have 1 key ID


The comment states "Should have 1 key ID" but the assertion checks for 2 entries. This inconsistency should be corrected. Based on the context in other tests where content IDs are also counted, the comment should likely be updated to clarify that there is "1 key ID + 1 content ID = 2 total entries".

github-actions · 2026-01-29T11:14:40Z

Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

1 out of 2 committers have signed the CLA.
✅ (narenr94)[https://github.com/narenr94]
❌ @nejuma1
_{You can retrigger this bot by commenting recheck in this Pull Request.}_{Posted by the CLA Assistant Lite bot.}

Copilot

Pull request overview

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

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

Copilot · 2026-01-29T11:26:44Z

CMakeLists.txt

+if(ENABLE_MW_PROFILING)
+    install(FILES PerfProfiler.h DESTINATION include)
+endif()

 set(SOURCES
 	InterfacePlayerRDK.cpp


PerfProfiler.h is already part of the install(FILES ...) list, but it’s installed again under if(ENABLE_MW_PROFILING). This duplicates the install rule; keep a single install path for the header.

Suggested change

if(ENABLE_MW_PROFILING)

install(FILES PerfProfiler.h DESTINATION include)

endif()

set(SOURCES

InterfacePlayerRDK.cpp

set(SOURCES

InterfacePlayerRDK.cpp

InterfacePlayerRDK.cpp

Copilot · 2026-01-29T11:26:45Z

test/utests/fakes/FakeDrmSessionFactory.cpp

+MockDrmSessionFactory* g_MockDrmSessionFactory = nullptr;
+
+/**
+ * @brief Fake implementation of GetDrmSession
+ * 
+ * If g_MockDrmSessionFactory is set, delegates to the mock.
+ * Otherwise returns nullptr.
+ */
+DrmSession* DrmSessionFactory::GetDrmSession(DrmHelperPtr drmHelper, DrmCallbacks* drmCallbacks)
+{
+	if (g_MockDrmSessionFactory)
+	{
+		return g_MockDrmSessionFactory->GetDrmSession(drmHelper, drmCallbacks);


The mock global is declared as g_mockDrmSessionFactory in MockDrmSessionFactory.h, but this fake defines/uses g_MockDrmSessionFactory. With the current mismatch, code including the header can’t set the pointer that DrmSessionFactory::GetDrmSession() checks. Rename to a single consistent global (and ensure it’s declared in the header used by tests).

Suggested change

MockDrmSessionFactory* g_MockDrmSessionFactory = nullptr;

/**

* @brief Fake implementation of GetDrmSession

*

* If g_MockDrmSessionFactory is set, delegates to the mock.

* Otherwise returns nullptr.

*/

DrmSession* DrmSessionFactory::GetDrmSession(DrmHelperPtr drmHelper, DrmCallbacks* drmCallbacks)

{

if (g_MockDrmSessionFactory)

{

return g_MockDrmSessionFactory->GetDrmSession(drmHelper, drmCallbacks);

MockDrmSessionFactory* g_mockDrmSessionFactory = nullptr;

/**

* @brief Fake implementation of GetDrmSession

*

* If g_mockDrmSessionFactory is set, delegates to the mock.

* Otherwise returns nullptr.

*/

DrmSession* DrmSessionFactory::GetDrmSession(DrmHelperPtr drmHelper, DrmCallbacks* drmCallbacks)

{

if (g_mockDrmSessionFactory)

{

return g_mockDrmSessionFactory->GetDrmSession(drmHelper, drmCallbacks);

Copilot · 2026-01-29T11:26:45Z

test/utests/tests/OcdmBasicSessionAdapterTests/FunctionalTests.cpp

+		// Create mocks before using them
 		g_mockOpenCdmSessionAdapter = new NiceMock<MockOpenCdmSessionAdapter>();
+		g_mockopencdm = new NiceMock<MockOpenCdm>();
+		g_mockMemorySystem = new NiceMock<MockDrmMemorySystem>();


g_mockopencdm is allocated earlier in SetUp(), and then allocated again here without deleting the first instance. This leaks the first mock and can drop any expectations on it. Please allocate it only once (or delete before reassigning).

Copilot · 2026-01-29T11:26:45Z

drm/helper/WidevineDrmHelper.cpp

 			{
 				mDefaultKeySlot = it.first;
-				MW_LOG_WARN("setDefaultKeyID : %s slot : %d", cencData.c_str(), mDefaultKeySlot);
+				MW_LOG_WARN("setDefaultKeyID : %s slot : %d", PlayerLogManager::getHexDebugStr(defaultKeyID).c_str(), mDefaultKeySlot);


setDefaultKeyID() logs the (hex) key ID at WARN. If this is expected to run in production, consider lowering the log level or gating it behind a debug/profiling flag to avoid noisy logs and potential identifier exposure.

Copilot · 2026-01-29T11:26:45Z

drm/helper/WidevineDrmHelper.cpp

+	// Print all key IDs for debugging
+	for (const auto& keyPair : mKeyIDs) {
+		std::string keyStr = PlayerLogManager::getHexDebugStr(keyPair.second);
+		MW_LOG_DEBUG("Key ID [%d]: %s", keyPair.first, keyStr.c_str());
+	}


getKey() now logs every Key ID on every call. This can become very noisy and adds work to a frequently-called method. Consider removing the per-call loop, logging only on demand, or guarding it behind a debug/profiling flag.

Suggested change

// Print all key IDs for debugging

for (const auto& keyPair : mKeyIDs) {

std::string keyStr = PlayerLogManager::getHexDebugStr(keyPair.second);

MW_LOG_DEBUG("Key ID [%d]: %s", keyPair.first, keyStr.c_str());

}

Copilot · 2026-01-29T11:26:46Z

test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp

+	std::map<int, std::vector<uint8_t>> keyIDs;
+	drmHelper->getKeys(keyIDs);
+	ASSERT_EQ(keyIDs.size(), 2); // Should have exactly 1 key (content ID should not be counted)
+


Same inconsistency here: ASSERT_EQ(keyIDs.size(), 2) but the comment says there should be exactly 1 key and that content-id should not be counted. Please update the expectation/comment to match what WidevineDrmHelper::getKeys() actually returns in this scenario.

Copilot · 2026-01-29T11:26:46Z

CMakeLists.txt

+option(ENABLE_MW_PROFILING "Enable middleware profiling" ON)
+
+if(ENABLE_MW_PROFILING)
+  add_definitions(-DENABLE_MW_PROFILING)
+endif()


ENABLE_MW_PROFILING is defaulted to ON, which makes profiling instrumentation (timers + MW_LOG_INFO per call) enabled for all builds by default. Given the number of MW_PROFILE_FUNCTION() call sites in hot paths (e.g., InterfacePlayerRDK), this can introduce significant runtime overhead and log volume unless explicitly turned off. Consider defaulting this option to OFF (or gating it by build type) and enabling it only for profiling builds.

Copilot · 2026-01-29T11:26:46Z

CMakeLists.txt

+if(ENABLE_MW_PROFILING)
+    list(APPEND LIBPLAYERGSTINTERFACE_HEADERS PerfProfiler.h)
+endif()


PerfProfiler.h is already listed in LIBPLAYERGSTINTERFACE_HEADERS, but it’s appended again under if(ENABLE_MW_PROFILING), which duplicates the entry. Keep it only once (either unconditional, or only under the option).

Suggested change

if(ENABLE_MW_PROFILING)

list(APPEND LIBPLAYERGSTINTERFACE_HEADERS PerfProfiler.h)

endif()

Copilot · 2026-01-29T11:26:47Z

CMakeLists.txt

+if(ENABLE_MW_PROFILING)
+    list(APPEND SOURCES PerfProfiler.cpp)
+endif()


PerfProfiler.cpp is included in SOURCES unconditionally and then appended again under if(ENABLE_MW_PROFILING). This duplicates the source entry and can result in redundant build rules; include it only once.

Suggested change

if(ENABLE_MW_PROFILING)

list(APPEND SOURCES PerfProfiler.cpp)

endif()

Copilot · 2026-01-29T11:26:47Z

drm/ocdm/opencdmsessionadapter.cpp

 {
 	std::lock_guard<std::mutex> lock(m_usableKeysMutex);
 	return m_usableKeys;
-}
+}


getUsableKeys() takes the mutex but returns a reference to m_usableKeys after the lock is released. Callers will iterate the returned reference without holding the mutex, so concurrent updates in keyUpdateOCDM() can still race and invalidate iterators. Consider returning a copy (snapshot) or changing the API to provide a thread-safe way to access the usable-keys list.

nrames759 and others added 8 commits December 15, 2025 13:37

sync

c90d369

Merge pull request #58 from rdkcentral/main

e3e2442

Merge pull request #52 from rdkcentral/feature/RDKEMW-9809

sync

18c73ec

RDKEMW-11877 : Metrics Collection for Player Interface Public APIs

11e2321

Reason for change : Integrating code for metrics collection Test Procedure : Enable info logs to see metrics related logs Priority : P1 Risks : None Signed-off-by : nejumathoppil_nazar@comcast.com

Adding necessary headers

c564720

Adding option for conditional compilation

d257d45

Fixing compilation issues

650f5bf

Adding profiling function to public APIs

e579971

nejuma1 requested a review from a team as a code owner January 21, 2026 11:17

Copilot AI review requested due to automatic review settings January 21, 2026 11:17

Copilot started reviewing on behalf of nejuma1 January 21, 2026 11:17 View session

Copilot AI reviewed Jan 21, 2026

View reviewed changes

Update PlayerExternalsInterface.cpp to include PerfProfiler.h

2618f4d

Update ContentSecurityManager.cpp to include PerfProfiler.h

7161fc8

Copilot AI review requested due to automatic review settings January 29, 2026 11:15

Copilot started reviewing on behalf of nejuma1 January 29, 2026 11:16 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

Merge branch 'feature/RDKEMW-11881' into feature/RDKEMW-11877

d9627b3

	* Copyright 2024 RDK Management
	* Copyright 2025 RDK Management

	if(ENABLE_MW_PROFILING)
	list(APPEND LIBPLAYERGSTINTERFACE_HEADERS PerfProfiler.h)
	endif()

	if(ENABLE_MW_PROFILING)
	list(APPEND SOURCES PerfProfiler.cpp)
	endif()

Conversation

nejuma1 commented Jan 21, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 29, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants