RDKEMW-22812: Add persistent DRM session support in Rialto - #578
Open
varatharajan568 wants to merge 3 commits into
Open
RDKEMW-22812: Add persistent DRM session support in Rialto#578varatharajan568 wants to merge 3 commits into
varatharajan568 wants to merge 3 commits into
Conversation
Summary: Implement persistent-license session support in Rialto-OCDM for Widevine and PlayReady DRM. This enables applications to create, store, restore, and reuse persistent DRM sessions across application restarts in compliance with EME persistent-license requirements. Type: Feature Test Plan: Verify persistent-license session creation for Widevine & PlayReady DRM. Signed-off-by: Varatharajan_Narayanan <Varatharajan_Narayanan@comcast.com>
|
Pull request title must follow the pattern: Pull request description must follow the Commit message format for RDK-E:
< JIRA TICKET >: < one line summary of change less than 65 characters > |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces support for passing optional CDM-specific data through Rialto’s MediaKeys/MediaKeySession request pipeline down to the OpenCDM session construction, as a building block for persistent-license session support (Widevine/PlayReady) and session restoration across restarts.
Changes:
- Extend session construction and
generateRequestAPIs end-to-end to carry optionalcdmData(client → IPC/proto → server/service → session wrapper). - Update protobuf (
GenerateRequestRequest) and IPC marshalling/unmarshalling to transportcdm_data. - Adjust unit/component tests and mocks to reflect the new method signatures.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| wrappers/source/OcdmSession.cpp | Passes optional CDM data into opencdm_construct_session. |
| wrappers/interface/IOcdmSession.h | Extends IOcdm session construction API to accept optional CDM data. |
| wrappers/include/OcdmSession.h | Updates wrapper session header to match new constructSession signature. |
| proto/mediakeysmodule.proto | Adds cdm_data field to GenerateRequestRequest. |
| media/client/main/include/MediaKeys.h | Extends public client implementation to accept optional cdmData. |
| media/client/main/source/MediaKeys.cpp | Forwards cdmData through client MediaKeys to IPC. |
| media/client/ipc/include/MediaKeysIpc.h | Extends IPC interface to accept optional cdmData. |
| media/client/ipc/source/MediaKeysIpc.cpp | Serializes cdmData into the GenerateRequest protobuf request. |
| media/public/include/IMediaKeys.h | Extends public IMediaKeys API with optional cdmData. |
| media/server/ipc/source/MediaKeysModuleService.cpp | Deserializes cdm_data from protobuf and forwards to service. |
| media/server/service/include/ICdmService.h | Extends service interface to accept optional cdmData. |
| media/server/service/source/CdmService.h | Extends service API signature and defaults to support optional cdmData. |
| media/server/service/source/CdmService.cpp | Forwards cdmData into server MediaKeys request path. |
| media/server/main/include/IMediaKeySession.h | Extends server session interface to accept optional cdmData. |
| media/server/main/include/MediaKeySession.h | Updates MediaKeySession API to accept optional cdmData. |
| media/server/main/source/MediaKeySession.cpp | Uses cdmData (or queued DRM header) during OCDM session construction. |
| media/server/main/include/MediaKeysServerInternal.h | Extends internal server API to accept optional cdmData. |
| media/server/main/source/MediaKeysServerInternal.cpp | Forwards cdmData into per-session generateRequest. |
| tests/common/externalLibraryMocks/OcdmSessionMock.h | Updates OCDM session mock signature for constructSession. |
| tests/unittests/media/interface/mocks/MediaKeysMock.h | Updates IMediaKeys mock signature for generateRequest. |
| tests/unittests/media/server/mocks/main/MediaKeySessionMock.h | Updates MediaKeySession mock signature for generateRequest. |
| tests/unittests/media/server/mocks/main/MediaKeysServerInternalMock.h | Updates internal server mock signature for generateRequest. |
| tests/unittests/media/server/mocks/service/CdmServiceMock.h | Updates CdmService mock signature for generateRequest. |
| tests/unittests/media/server/service/cdmService/CdmServiceTestsFixture.cpp | Updates expectations to include the new cdmData parameter. |
| tests/unittests/media/server/main/mediaKeys/GenerateRequestTest.cpp | Updates tests to include cdmData argument (currently empty). |
| tests/unittests/media/server/main/mediaKeySession/GenerateRequestTest.cpp | Updates session tests to include cdmData (currently empty) and new constructSession args. |
| tests/unittests/media/server/main/mediaKeySession/base/MediaKeySessionTestBase.cpp | Updates test base utilities for new cdmData parameter. |
| tests/unittests/media/server/ipc/mediaKeysModuleService/MediaKeysModuleServiceTestsFixture.cpp | Updates IPC service tests to include cdmData in expectations. |
| tests/unittests/media/client/main/mediaKeys/KeySessionTest.cpp | Updates client-side tests for new cdmData argument. |
| tests/componenttests/server/tests/mediaKeys/MediaKeysTestMethods.cpp | Updates component-test expectations for new constructSession args. |
| tests/componenttests/server/tests/mediaKeys/MediaKeysTest.cpp | Updates component-test expectations for new constructSession args. |
| tests/componenttests/server/tests/mediaKeys/SessionReadyForDecryptionTest.cpp | Updates component-test expectations/lambdas for new constructSession args. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+137
to
+138
| status = m_ocdmSession->constructSession(m_kSessionType, initDataType, &initData[0], initData.size(), cdmDataPtr, | ||
| cdmDataSize); |
Comment on lines
168
to
+172
| if (LimitedDurationLicense::NOT_SPECIFIED != ldlState && m_sessionInfo.find(keySessionId) != m_sessionInfo.end()) | ||
| { | ||
| m_sessionInfo[keySessionId].isExtendedInterfaceUsed = true; | ||
| } | ||
| return mediaKeysIter->second->generateRequest(keySessionId, initDataType, initData, ldlState); | ||
| return mediaKeysIter->second->generateRequest(keySessionId, initDataType, initData, cdmData, ldlState); |
| @@ -46,11 +46,14 @@ class IOcdmSession | |||
| * @param[in] initDataType : The init data type. | |||
| * @param[in] initData : The init data pointer. | |||
| * @param[in] initDataSize : The size of the init data pointed too by initData. | |||
Comment on lines
+133
to
+137
| const std::vector<uint8_t> &cdmDataToUse = cdmData.empty() ? m_queuedDrmHeader : cdmData; | ||
| const uint8_t *cdmDataPtr = cdmDataToUse.empty() ? nullptr : cdmDataToUse.data(); | ||
| const uint32_t cdmDataSize = cdmDataToUse.size(); | ||
|
|
||
| status = m_ocdmSession->constructSession(m_kSessionType, initDataType, &initData[0], initData.size(), cdmDataPtr, |
Comment on lines
144
to
148
| else | ||
| { | ||
| m_isSessionConstructed = true; | ||
| if (!m_queuedDrmHeader.empty()) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("Setting queued drm header after session construction"); | ||
| setDrmHeader(m_queuedDrmHeader); | ||
| m_queuedDrmHeader.clear(); | ||
| } | ||
| m_queuedDrmHeader.clear(); | ||
| } |
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 no new comments.
Suppressed comments (6)
media/server/main/source/MediaKeySession.cpp:138
initDatais passed toconstructSessionas&initData[0], which is undefined behavior wheninitDatais empty. UseinitData.data()(or nullptr when empty) to avoid out-of-bounds access for valid zero-length init data.
const std::vector<uint8_t> &cdmDataToUse = cdmData.empty() ? m_queuedDrmHeader : cdmData;
const uint8_t *cdmDataPtr = cdmDataToUse.empty() ? nullptr : cdmDataToUse.data();
const uint32_t cdmDataSize = cdmDataToUse.size();
status = m_ocdmSession->constructSession(m_kSessionType, initDataType, &initData[0], initData.size(), cdmDataPtr,
cdmDataSize);
media/server/main/source/MediaKeySession.cpp:137
- New
cdmDatahandling is introduced here, but the unit tests only ever pass an emptycdmDatavector and don’t assert that a non-empty value is forwarded intoIOcdmSession::constructSession(or that a queued DRM header is used whencdmDatais empty). Adding coverage for both cases would prevent regressions in the persistent-session path.
const std::vector<uint8_t> &cdmDataToUse = cdmData.empty() ? m_queuedDrmHeader : cdmData;
const uint8_t *cdmDataPtr = cdmDataToUse.empty() ? nullptr : cdmDataToUse.data();
const uint32_t cdmDataSize = cdmDataToUse.size();
status = m_ocdmSession->constructSession(m_kSessionType, initDataType, &initData[0], initData.size(), cdmDataPtr,
media/public/include/IMediaKeys.h:133
- The new Doxygen
@paramlines forcdmDataare mis-indented compared to the rest of the comment block, which can break or misformat generated documentation.
* @param[in] keySessionId : The key session id for the session.
* @param[in] initDataType : The init data type.
* @param[in] initData : The init data.
* @param[in] cdmData : Optional CDM data.
* @param[in] ldlState : The Limited Duration License state. Most of key systems do not need this parameter,
media/server/main/include/IMediaKeySession.h:87
- The new Doxygen
@paramline forcdmDatais mis-indented relative to the rest of the comment block, which can break or misformat generated documentation.
* @brief Generates a licence request.
*
* @param[in] initDataType : The init data type.
* @param[in] initData : The init data.
* @param[in] cdmData : Optional CDM data.
* @param[in] ldlState : The Limited Duration License state. Most of key systems do not need this parameter.
wrappers/interface/IOcdmSession.h:50
- The new Doxygen
@paramlines forcdmData/cdmDataSizeare mis-indented compared to the rest of the comment block, which can break or misformat generated documentation.
media/public/include/IMediaKeys.h:141 - Changing the signature of the public virtual
IMediaKeys::generateRequestmethod is an ABI-breaking change even with default arguments (downstream implementations and binaries must be rebuilt). If ABI stability is required, consider keeping the existing virtual method and introducing a new API (e.g., a new virtualgenerateRequestEx(...)or a non-virtual wrapper) instead.
virtual MediaKeyErrorStatus
generateRequest(int32_t keySessionId, InitDataType initDataType, const std::vector<uint8_t> &initData,
const std::vector<uint8_t> &cdmData = std::vector<uint8_t>{},
const LimitedDurationLicense &ldlState = LimitedDurationLicense::NOT_SPECIFIED) = 0;
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 no new comments.
Suppressed comments (7)
wrappers/source/OcdmSession.cpp:162
- The new logging prints raw
cdmDatabytes. CDM/session data can be sensitive (DRM headers / persistent session data) and logging it (especially at ERROR level) risks leaking secrets and may generate very large logs. Remove this logging (or at minimum redact/truncate and gate behind a debug-only build flag).
media/server/main/source/MediaKeySession.cpp:138 &initData[0]is undefined behavior wheninitDatais empty. Even if empty initData is not expected, usingdata()/nullptr avoids a hard-to-debug crash and keeps the API robust.
status = m_ocdmSession->constructSession(m_kSessionType, initDataType, &initData[0], initData.size(), cdmDataPtr,
cdmDataSize);
media/server/main/source/MediaKeySession.cpp:136
- New behavior selects
cdmDataToUsefrom either the request parameter orm_queuedDrmHeaderand forwards it intoconstructSession, but the existing unit tests only exercise the empty-cdmDatapath and don't assert what is passed toIOcdmSession::constructSession. Adding a focused unit test for non-emptycdmData(and for the queued-header fallback) would protect this persistent-session pathway from regressions.
const std::vector<uint8_t> &cdmDataToUse = cdmData.empty() ? m_queuedDrmHeader : cdmData;
const uint8_t *cdmDataPtr = cdmDataToUse.empty() ? nullptr : cdmDataToUse.data();
const uint32_t cdmDataSize = cdmDataToUse.size();
wrappers/interface/IOcdmSession.h:50
- Doc comment typo/formatting: "pointed too" should be "pointed to", and the new
cdmData/cdmDataSizeparam lines should be aligned with the other*lines.
wrappers/CMakeLists.txt:126 RialtoLoggingis newly linked intoRialtoWrappers, but the only usage in this target is the rawcdmDatalogging added inOcdmSession.cpp. If that logging is removed/redacted, this link dependency can be dropped to avoid pulling logging into the wrappers library unnecessarily.
RialtoLogging
wrappers/CMakeLists.txt:116
- These include directories appear to be added only to support the new
RialtoClientLogging.husage inOcdmSession.cpp. If the sensitivecdmDatalogging is removed, please drop these to avoid an unnecessary dependency fromRialtoWrappersonto client/logging targets.
This issue also appears on line 126 of the same file.
$<TARGET_PROPERTY:RialtoClientCommon,INTERFACE_INCLUDE_DIRECTORIES>
$<TARGET_PROPERTY:RialtoLogging,INTERFACE_INCLUDE_DIRECTORIES>
wrappers/source/OcdmSession.cpp:28
- After removing the raw
cdmDatalogging, these includes become unused and can be dropped to avoid introducing a new logging dependency into the wrappers library.
This issue also appears on line 158 of the same file.
|
Coverage statistics of your commit: |
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.
Summary:
Implement persistent-license session support in Rialto-OCDM for Widevine and PlayReady DRM. This enables applications to create, store, restore, and reuse persistent DRM sessions across application restarts in compliance with EME persistent-license requirements.
Type: Feature
Test Plan: Verify persistent-license session creation for Widevine & PlayReady DRM.