Microsoft Playready support for Amazon Prime RDK app#449
Microsoft Playready support for Amazon Prime RDK app#449skywojciechowskim wants to merge 16 commits intomasterfrom
Conversation
|
Pull request must be merged with a description containing the required fields, Summary: If there is no jira releated to this change, please put 'Jira: NO-JIRA'. Description can be changed by editing the top comment on your pull request and making a new commit. |
|
tests/componenttests/server/tests/mediaKeys/MediaKeysTest.cpp:46:38: warning: The class 'MediaKeysTest' defines member variable with name 'm_kInitData' also defined in its parent class 'MediaKeysTestMethods'. [duplInheritedMember] |
There was a problem hiding this comment.
Pull request overview
This pull request refactors the DRM key session management to generalize Microsoft Playready support beyond Netflix, enabling support for Amazon Prime and other applications using Playready. The key architectural change is replacing Netflix-specific Playready detection with a generic "extended interface" concept that dynamically detects when Playready-specific APIs are used.
Changes:
- Removed the
isLDLparameter fromcreateKeySession()API - Added
ldlStateparameter togenerateRequest()API (with default value for backward compatibility) - Replaced
isNetflixPlayreadyKeySystem()method withisExtendedInterfaceUsed()to generalize Playready detection - Introduced dynamic detection of extended interface usage based on which APIs are called (setDrmHeader, selectKeyId, generateRequest with ldlState)
- Added support for queueing DRM headers before session construction
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| media/public/include/MediaCommon.h | Added LimitedDurationLicense enum |
| media/public/include/IMediaKeys.h | Updated API signatures for createKeySession and generateRequest |
| media/server/main/include/MediaKeySession.h | Added extended interface tracking and queued DRM header support |
| media/server/main/source/MediaKeySession.cpp | Implemented dynamic extended interface detection and queued header logic |
| media/server/service/source/CdmService.cpp | Updated to track extended interface usage per session |
| media/client/main/source/MediaKeys.cpp | Updated client API to match new signatures |
| proto/mediakeysmodule.proto | Added LimitedDurationLicense enum to GenerateRequestRequest |
| tests/unittests/media/server/main/mediaKeys/IsNetflixPlayreadyKeySystemTest.cpp | Removed Netflix-specific tests |
| tests/componenttests/server/tests/mediaKeys/* | Updated tests for new Playready workflow |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else | ||
| { | ||
| // Set the request flag for the onLicenseRequest callback | ||
| m_licenseRequested = true; | ||
| } |
There was a problem hiding this comment.
Logic issue: When ldlState is NOT_SPECIFIED and the session is already constructed, m_licenseRequested is set to true (line 121) but no license request is actually generated. The flag should only be set to true when a license request will actually be generated. Consider moving line 121 to be set only when !m_isSessionConstructed or when the extended interface will actually trigger a license request.
| if (m_extendedInterfaceInUse) | ||
| { | ||
| // Ocdm-playready does not notify onProcessChallenge when complete. | ||
| // Fetch the challenge manually. | ||
| getChallenge(ldlState); | ||
| } |
There was a problem hiding this comment.
Logic issue: When session construction fails (line 132-136), m_licenseRequested is set to false, but if m_extendedInterfaceInUse is true, the code will still call getChallenge() at line 158. The check at line 154 should also verify that the session was successfully constructed before calling getChallenge(), or return early if status is not OK.
| if (LimitedDurationLicense::NOT_SPECIFIED != ldlState) | ||
| { | ||
| m_sessionInfo[keySessionId].isExtendedInterfaceUsed = true; | ||
| } | ||
| return mediaKeysIter->second->generateRequest(keySessionId, initDataType, initData, ldlState); |
There was a problem hiding this comment.
Logic issue: At line 167, m_sessionInfo[keySessionId].isExtendedInterfaceUsed is accessed without checking if keySessionId exists in m_sessionInfo. If generateRequest is called with an invalid or not-yet-created keySessionId, this will create a new entry with default values in the map. The code should verify that the keySessionId exists in m_sessionInfo before updating it, or this should be done after the generateRequest call succeeds.
| if (LimitedDurationLicense::NOT_SPECIFIED != ldlState) | |
| { | |
| m_sessionInfo[keySessionId].isExtendedInterfaceUsed = true; | |
| } | |
| return mediaKeysIter->second->generateRequest(keySessionId, initDataType, initData, ldlState); | |
| MediaKeyErrorStatus status = mediaKeysIter->second->generateRequest(keySessionId, initDataType, initData, ldlState); | |
| if (MediaKeyErrorStatus::OK == status && LimitedDurationLicense::NOT_SPECIFIED != ldlState) | |
| { | |
| auto sessionInfoIter = m_sessionInfo.find(keySessionId); | |
| if (sessionInfoIter != m_sessionInfo.end()) | |
| { | |
| sessionInfoIter->second.isExtendedInterfaceUsed = true; | |
| } | |
| else | |
| { | |
| RIALTO_SERVER_LOG_ERROR("Session info for key session: %d does not exist", keySessionId); | |
| } | |
| } | |
| return status; |
| enum LimitedDurationLicense { | ||
| NOT_SPECIFIED = 0; ///< The license duration is not specified | ||
| ENABLED = 1; ///< The license has a limited duration | ||
| DISABLED = 2; ///< The license does not have a limited duration | ||
| }; | ||
|
|
||
| optional int32 media_keys_handle = 1 [default = -1]; | ||
| optional int32 key_session_id = 2 [default = -1]; | ||
| optional InitDataType init_data_type = 3; | ||
| repeated uint32 init_data = 4; | ||
|
|
||
| optional LimitedDurationLicense ldl_state = 5 [default = NOT_SPECIFIED]; |
There was a problem hiding this comment.
Documentation is incomplete: The function documentation at lines 189-207 does not mention the new ldl_state parameter that was added to the request. The documentation should be updated to include this parameter and its purpose.
| bool CdmService::isNetflixPlayreadyKeySystem(int32_t keySessionId) | ||
| bool CdmService::isExtendedInterfaceUsed(int32_t keySessionId) | ||
| { | ||
| RIALTO_SERVER_LOG_DEBUG("CdmService requested to check if key system is Netflix Playready, key session id: %d", |
There was a problem hiding this comment.
Log message is outdated: The log message still says "check if key system is Netflix Playready" but the function has been renamed to isExtendedInterfaceUsed and its purpose has been generalized. The log message should be updated to reflect the new purpose, e.g., "CdmService requested to check if extended interface is used, key session id: %d".
| RIALTO_SERVER_LOG_DEBUG("CdmService requested to check if key system is Netflix Playready, key session id: %d", | |
| RIALTO_SERVER_LOG_DEBUG("CdmService requested to check if extended interface is used, key session id: %d", |
|
|
||
| MediaKeyErrorStatus status; | ||
| auto task = [&]() { status = createKeySessionInternal(sessionType, client, isLDL, keySessionId); }; | ||
| auto task = [&]() { status = createKeySessionInternal(sessionType, client, false, keySessionId); }; |
There was a problem hiding this comment.
Maintainability issue: The lambda at line 219 passes a hardcoded false value for the isLDL parameter to createKeySessionInternal. Since this parameter is no longer used in the implementation (it's not passed forward to createMediaKeySession), consider refactoring createKeySessionInternal to remove this unused parameter in a follow-up change for cleaner code.
| protoLimitedDurationLicense = GenerateRequestRequest_LimitedDurationLicense_DISABLED; | ||
| break; | ||
| default: | ||
| RIALTO_CLIENT_LOG_WARN("Recieved unknown limited duration license state"); |
There was a problem hiding this comment.
Spelling error: "Recieved" should be "Received". The correct spelling has the 'e' before the 'i'.
media/public/include/MediaCommon.h
Outdated
| }; | ||
|
|
||
| /** | ||
| * @brief Struct containing current playback information. |
There was a problem hiding this comment.
Documentation error: The comment says "Struct containing current playback information" but this is an enum, not a struct. The documentation should say "Enum for Limited Duration License state" or similar.
| * @brief Struct containing current playback information. | |
| * @brief Enum for Limited Duration License state. |
|
Coverage statistics of your commit: |
|
Coverage statistics of your commit: |
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Summary: Microsoft Playready support for Amazon Prime RDK app
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: NO-JIRA