VPAAMP-867 DirectRialto - Changes required - #220
Open
jfagunde wants to merge 8 commits into
Open
Conversation
Reason for Change: Changes required for DirectRialto Test Procedure: Build and run AAMP with DirectRialto enabled Priority: P1 Risks: Medium
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new closed-captions manager for the “direct-rialto” backend and updates DRM session plumbing to support safer key access patterns and injectable DRM session creation.
Changes:
- Add
PlayerDirectRialtoCCManager(GStreamer-free CC control viaIDirectRialtoCC) plus a new L1 unit test suite. - Change
DrmSession::getUsableKeys()to return a snapshot copy (by value) and update mocks/tests accordingly. - Add
DrmSessionCreatorinjection support inDrmSessionManagerand adjust build sources/includes (including removingdrm/DrmSession.cpp).
Reviewed changes
Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTests.cpp | Adds gtest main for the new direct-rialto CC manager test binary |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp | Adds L1 test cases validating track mapping and mute/unmute orchestration |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerBaseStubs.cpp | Provides CC base stubs to support isolated unit testing |
| test/utests/tests/PlayerDirectRialtoCCManagerTests/CMakeLists.txt | Builds and registers the new direct-rialto CC manager unit test target |
| test/utests/tests/OCDMSessionAdapter/FunctionalTests.cpp | Replaces a brittle gmock internal expectation with a standard EXPECT_CALL |
| test/utests/tests/OCDMSessionAdapter/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/OcdmBasicSessionAdapterTests/FunctionalTests.cpp | Updates usable-keys mocking for new by-value return type |
| test/utests/tests/OcdmBasicSessionAdapterTests/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmTests/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmSessionManagerTests/DrmSessionManagerTestCases.cpp | Updates getUsableKeys() expectations from ReturnRef to Return |
| test/utests/tests/DrmSessionManagerTests/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmSecureClient/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmOcdmTests/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmHelperTests/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/DrmAes/CMakeLists.txt | Removes DrmSession.cpp from test build sources |
| test/utests/tests/CMakeLists.txt | Adds the new PlayerDirectRialtoCCManagerTests subdirectory |
| test/utests/mocks/MockOpenCdmSessionAdapter.h | Changes getUsableKeys() mock to return by value |
| test/utests/fakes/Fakeopencdmsessionadapter.cpp | Updates fake adapter getUsableKeys() signature and default behavior |
| test/utests/fakes/FakeDRMSessionManager.cpp | Updates fake DrmSessionManager ctor signature to include creator injection |
| test/utests/fakes/FakeDRMSession.cpp | Removes now-unneeded fake DrmSession::getKeySystem() definition |
| test/utests/fakes/CMakeLists.txt | Adds include path for Rialto-related test fakes |
| drm/ocdm/opencdmsessionadapter.h | Updates getUsableKeys() override to return by value |
| drm/ocdm/opencdmsessionadapter.cpp | Returns a snapshot copy of usable keys under mutex |
| drm/DrmSessionManager.h | Adds DrmSessionCreator type and ctor parameter; stores creator in manager |
| drm/DrmSessionManager.cpp | Adds creator-based DrmSession instantiation path; adds guard for missing AcquireLicenseCb |
| drm/DrmSession.h | Moves key behaviors inline (incl. getUsableKeys default), changes usable-keys API, and adjusts drm session interface |
| drm/DrmSession.cpp | Deletes the source file (previously held lifecycle-guard method implementations) |
| CMakeLists.txt | Removes drm/DrmSession.cpp from library sources; adds direct-rialto CC manager to subtitle sources and include paths |
| closedcaptions/PlayerCCManager.h | Extends SetRialto API to support direct-rialto selection; replaces boolean with enum type |
| closedcaptions/PlayerCCManager.cpp | Instantiates DirectRialto CC manager when selected; updates SetRialto logic |
| closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.h | Adds new CC manager class for direct-rialto backend |
| closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp | Implements direct-rialto CC track mapping + mute control via IDirectRialtoCC |
| closedcaptions/direct-rialto/IDirectRialtoCC.h | Introduces narrow CC-control interface for direct-rialto path |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+94
to
+97
| DrmSession(const string &keySystem) : | ||
| m_keySystem(keySystem), | ||
| m_OutputProtectionEnabled(false), | ||
| mContentSecurityManagerSession() { }; |
Comment on lines
116
to
118
| /** | ||
| * @fn AcquireForUse | ||
| * @brief Must be called by any external caller (e.g. the GStreamer |
Comment on lines
+30
to
+33
| #include <gtest/gtest.h> | ||
| #include <gmock/gmock.h> | ||
| #include <string> | ||
|
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
drm/DrmSession.h:97
- DrmSession::mActiveOperations and mMarkedForDestruction are not initialized in the new inline constructor. These fields are used by the lifecycle guard (AcquireForUse/ReleaseAfterUse/PrepareForDestruction) and leaving them uninitialized is undefined behavior.
DrmSession(const string &keySystem) :
m_keySystem(keySystem),
m_OutputProtectionEnabled(false),
mContentSecurityManagerSession() { };
Comment on lines
243
to
246
| set(LIBPLAYERGSTINTERFACE_DRM_SOURCES drm/PlayerHlsDrmSessionInterface.cpp | ||
| drm/DrmSessionManager.cpp | ||
| drm/DrmSession.cpp | ||
| drm/DrmSessionFactory.cpp | ||
| drm/helper/DrmHelper.cpp |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 8 comments.
Suppressed comments (4)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:32
- This test file uses std::make_unique/std::unique_ptr but does not include . Depending on the standard library headers pulled in by gtest/gmock is not reliable and can break compilation on some toolchains.
drm/DrmSession.h:97 - The inline DrmSession constructor only initializes m_keySystem/m_OutputProtectionEnabled/mContentSecurityManagerSession; it leaves mActiveOperations and mMarkedForDestruction uninitialized (UB) and also conflicts with the existing out-of-line constructor in drm/DrmSession.cpp if that translation unit is built. Prefer declaring the ctor here and keeping the full member initialization in DrmSession.cpp (or fully inline all methods and delete the .cpp).
DrmSession(const string &keySystem) :
m_keySystem(keySystem),
m_OutputProtectionEnabled(false),
mContentSecurityManagerSession() { };
CMakeLists.txt:246
- drm/DrmSession.cpp is removed from the library build sources, but it is the only translation unit that defines DrmSession methods like AcquireForUse(), PrepareForDestruction(), getKeySystem(), and the default decrypt() implementations. As-is, this will cause unresolved symbols at link time (unless those methods are moved inline, which they are not).
set(LIBPLAYERGSTINTERFACE_DRM_SOURCES drm/PlayerHlsDrmSessionInterface.cpp
drm/DrmSessionManager.cpp
drm/DrmSessionFactory.cpp
drm/helper/DrmHelper.cpp
test/utests/tests/DrmSessionManagerTests/CMakeLists.txt:66
- This target's PLAYER_SOURCES no longer includes ${PLAYER_ROOT}/drm/DrmSession.cpp, but DrmSession.cpp provides the only out-of-line definitions for DrmSession methods used by DrmSessionManager (e.g., PrepareForDestruction(), getKeySystem()). This will fail to link if DrmSession.cpp is not compiled into the test executable.
set(PLAYER_SOURCES
${PLAYER_ROOT}/drm/DrmSessionManager.cpp
${PLAYER_ROOT}/drm/helper/DrmHelper.cpp
${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp
${PLAYER_ROOT}/drm/helper/WidevineDrmHelper.cpp
)
Comment on lines
51
to
54
| set(PLAYER_SOURCES ${PLAYER_ROOT}/drm/ocdm/opencdmsessionadapter.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/DrmUtils.cpp | ||
| ${PLAYER_ROOT}/externals/PlayerExternalsInterface.cpp |
Comment on lines
47
to
50
| set(PLAYER_SOURCES ${PLAYER_ROOT}/drm/ocdm/OcdmBasicSessionAdapter.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/playerLogManager/PlayerLogManager.cpp | ||
| ${PLAYER_ROOT}/playerLogManager/PlayerLogManager.cpp |
Comment on lines
81
to
85
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSessionManager.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/DrmJsonObject.cpp | ||
| ${PLAYER_ROOT}/drm/ocdm/opencdmsessionadapter.cpp |
Comment on lines
65
to
69
| ${PLAYER_ROOT}/drm/DrmSessionFactory.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSessionManager.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/ocdm/opencdmsessionadapter.cpp |
Comment on lines
68
to
72
| ${PLAYER_ROOT}/drm/DrmSessionFactory.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSessionManager.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/DrmJsonObject.cpp |
Comment on lines
71
to
75
| ${PLAYER_ROOT}/drm/DrmSessionFactory.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSessionManager.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/DrmJsonObject.cpp |
Comment on lines
64
to
68
| ${PLAYER_ROOT}/drm/DrmSessionFactory.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelper.cpp | ||
| ${PLAYER_ROOT}/drm/helper/DrmHelperFactory.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSessionManager.cpp | ||
| ${PLAYER_ROOT}/drm/DrmSession.cpp | ||
| ${PLAYER_ROOT}/drm/DrmJsonObject.cpp |
Reason for Change: Fix AAMP compilation failure after rebase Summary of Changes: - Restore the file DrmSession.cpp - Revert some of the changes made to DrmSession.h - Restore fakes, CMake files and references to DrmSession.cpp Test Procedure: Build AAMP-CLI and AAMP L1 tests Priority: P1 Risks: Low
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (2)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:33
- This test uses std::unique_ptr / std::make_unique but does not include . Relying on transitive includes is brittle and can fail with different standard library / build configurations.
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <string>
test/utests/fakes/FakeDRMSessionManager.cpp:27
- The fake DrmSessionManager constructor ignores all parameters (including the new 'creator' parameter). If unit tests are built with warnings-as-errors, this can fail due to unused-parameter warnings.
DrmSessionManager::DrmSessionManager(int maxDrmSessions, void *player, std::function<void(uint32_t, uint32_t, const std::string&)> watermarkSessionUpdateCallback, DrmSessionCreator creator)
{
}
IsKeyIdProcessed() only held cachedKeyMutex, so it could observe a slot another thread had just claimed (before createDrmSession() finished acquiring the license) and report it as already succeeded. It also only checked the per-entry isFailedKeyId flag, missing whole-slot failures tracked in isFailedKeyEntries. Now also takes mDrmSessionLock (same order createDrmSession() already uses) so it blocks until any in-flight session creation for that key has resolved, and checks isFailedKeyEntries too. Add regression tests to DrmSessionManagerTestCases.cpp.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:32
- This test uses std::unique_ptr and std::make_unique but does not include , which can break compilation depending on transitive includes.
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <string>
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:152
- IDirectRialtoCC::setCCMute returns a bool, but StartRendering() ignores it, so an unmute failure will be silent.
m_control->setCCMute(false);
MW_LOG_INFO("EXIT");
}
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:164
- IDirectRialtoCC::setCCMute returns a bool, but StopRendering() ignores it, so a mute failure will be silent.
m_control->setCCMute(true);
MW_LOG_INFO("EXIT");
}
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:139
- IDirectRialtoCC::setTextTrackIdentifier returns a bool, but the result is currently ignored, so failures will be silent and the manager will still think the track has been applied. Consider checking the return value and logging (or propagating an error) when it fails.
This issue also appears in the following locations of the same file:
- line 150
- line 162
const std::string identifier = mapTrackIdentifier(track, format);
MW_LOG_INFO("setTextTrackIdentifier=\"%s\"", identifier.c_str());
m_control->setTextTrackIdentifier(identifier);
return 0;
Comment on lines
+921
to
+929
| if (m_sessionCreator) | ||
| { | ||
| auto owned = m_sessionCreator(drmHelper, Instance); | ||
| drmSessionContexts[sessionSlot].drmSession = owned.release(); | ||
| } | ||
| else | ||
| { | ||
| drmSessionContexts[sessionSlot].drmSession = DrmSessionFactory::GetDrmSession(drmHelper, Instance); | ||
| } |
…sion status" This reverts commit 14edcaa.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 24 out of 24 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:33
- This test uses std::unique_ptr and std::make_unique but does not include , which can cause a compile failure depending on indirect includes.
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <string>
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:163
- IDirectRialtoCC::setCCMute() returns a bool, but StopRendering() ignores it. Logging failures would make CC mute/unmute issues diagnosable.
m_control->setCCMute(true);
MW_LOG_INFO("EXIT");
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:151
- IDirectRialtoCC::setCCMute() returns a bool, but StartRendering() ignores it. Logging failures would make CC mute/unmute issues diagnosable.
This issue also appears on line 162 of the same file.
m_control->setCCMute(false);
MW_LOG_INFO("EXIT");
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:139
- IDirectRialtoCC::setTextTrackIdentifier() returns a bool, but the result is ignored. If the Rialto pipeline rejects the identifier, the failure will be silent and hard to diagnose.
const std::string identifier = mapTrackIdentifier(track, format);
MW_LOG_INFO("setTextTrackIdentifier=\"%s\"", identifier.c_str());
m_control->setTextTrackIdentifier(identifier);
return 0;
True when NAL units are length-prefixed (AVCC/HVCC, e.g. avcC/hvcC sample entries per ISO/IEC 14496-15); false for Annex-B (start-code delimited) bitstreams such as HLS-TS ES output. Meaningless for non-NAL-unit codecs (audio/subtitle), where it stays false.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (4)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:33
- This test file uses std::make_unique and std::unique_ptr but does not include , which can cause a build failure depending on transitive includes.
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <string>
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:150
- IDirectRialtoCC::setCCMute() returns a success boolean, but StartRendering() ignores the result, so unmute failures are silent.
m_control->setCCMute(false);
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:162
- IDirectRialtoCC::setCCMute() returns a success boolean, but StopRendering() ignores the result, so mute failures are silent.
m_control->setCCMute(true);
closedcaptions/direct-rialto/PlayerDirectRialtoCCManager.cpp:139
- IDirectRialtoCC::setTextTrackIdentifier() returns a success boolean, but the result is ignored here. This makes CC track selection failures silent and harder to diagnose.
This issue also appears in the following locations of the same file:
- line 150
- line 162
const std::string identifier = mapTrackIdentifier(track, format);
MW_LOG_INFO("setTextTrackIdentifier=\"%s\"", identifier.c_str());
m_control->setTextTrackIdentifier(identifier);
return 0;
PlayerCCManagerBase::GetId()/Release() implements a usage-counter pattern that is correct for PlayerSubtecCCManager's shared, out-of-process connection, but was reused as-is for the Rialto and direct-Rialto backends, where the "handle" is actually a per-pipeline object owned by a single playback session. In multi-pipeline mode this can leave a dangling handle in the singleton after its owning session is destroyed, since the refcount only tracks session count, not the validity of a specific handle. - Add PlayerCCManagerBase::InvalidateHandle(void*), a no-op by default, that a handle owner can call to clear its handle from the singleton ahead of Release(), independent of the GetId()/Release() count. - Add PlayerCCManager::HasInstance() so callers can check for an existing singleton without creating one. - Override InvalidateHandle() in PlayerDirectRialtoCCManager and PlayerRialtoCCManager to clear m_control/mSubtitleControlHandle only when it matches the given handle. This is a minimal mitigation (Option A); wiring InvalidateHandle() into PlayerRialtoCCManager's GStreamer-side handle owner, and a larger structural fix (shared preferences singleton + per-session state), are tracked separately as follow-up work.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 28 out of 28 changed files in this pull request and generated no new comments.
Suppressed comments (1)
test/utests/tests/PlayerDirectRialtoCCManagerTests/PlayerDirectRialtoCCManagerTestCases.cpp:36
- This test file uses std::unique_ptr and std::make_unique but does not include , which will fail to compile on standard toolchains.
#include <gtest/gtest.h>
#include <gmock/gmock.h>
#include <string>
#include "PlayerDirectRialtoCCManager.h"
#include "IDirectRialtoCC.h"
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reason for Change: Changes required for DirectRialto
Test Procedure: Build and run AAMP with DirectRialto enabled
Priority: P1
Risks: Medium