Feature/RDKEMW-16874#476
Conversation
…tead of reporting a generic error Type: Feature Test Plan: UT/CT, Fullstack Jira: RDKEMW-16874 Signed-off-by: Varatharajan_Narayanan <Varatharajan_Narayanan@comcast.com>
|
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. |
There was a problem hiding this comment.
Pull request overview
This PR adds an API to retrieve DRM key status end-to-end (session → server internal → CDM service → decryptor) so that decrypt failures can be reported with a more specific reason than a generic error.
Changes:
- Introduces
getKeyStatus(...)onIMediaKeySession,IMediaKeysServerInternal, andIDecryptionService, with concrete implementations inMediaKeySession,MediaKeysServerInternal, andCdmService. - Updates the GStreamer decryptor to query key status after decrypt failures and include/log it.
- Extends unit-test mocks and adds/updates unit tests to cover the new key-status retrieval path.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unittests/media/server/mocks/main/MediaKeysServerInternalMock.h | Adds getKeyStatus mock for server-internal API. |
| tests/unittests/media/server/mocks/main/MediaKeySessionMock.h | Adds getKeyStatus mock for key session API. |
| tests/unittests/media/server/mocks/main/DecryptionServiceMock.h | Adds getKeyStatus mock for decryptor service dependency. |
| tests/unittests/media/server/main/mediaKeySession/DecryptBufferTest.cpp | Adds UT verifying MediaKeySession::getKeyStatus returns and propagates KeyStatus. |
| tests/unittests/media/server/gstplayer/decryptor/DecryptTest.cpp | Updates decryptor UT to expect key-status query on decrypt failure. |
| media/server/service/source/CdmService.h | Declares CdmService::getKeyStatus to satisfy IDecryptionService. |
| media/server/service/source/CdmService.cpp | Implements CdmService::getKeyStatus and routes to IMediaKeysServerInternal. |
| media/server/main/source/MediaKeysServerInternal.cpp | Adds main-thread marshalled getKeyStatus + internal implementation. |
| media/server/main/source/MediaKeySession.cpp | Implements MediaKeySession::getKeyStatus using OCDM session status. |
| media/server/main/interface/IMediaKeysServerInternal.h | Adds server-internal getKeyStatus API to interface. |
| media/server/main/interface/IDecryptionService.h | Adds getKeyStatus to the decryptor-facing service interface. |
| media/server/main/include/MediaKeysServerInternal.h | Declares getKeyStatus and getKeyStatusInternal. |
| media/server/main/include/MediaKeySession.h | Declares MediaKeySession::getKeyStatus. |
| media/server/main/include/IMediaKeySession.h | Adds getKeyStatus to key-session interface. |
| media/server/gstplayer/source/GstDecryptor.cpp | Attempts to fetch/log/propagate key status after decrypt failure (currently has correctness issues). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Notify dropped frame upstream as a non-fatal message | ||
| std::string message = "Failed to decrypt buffer, dropping frame and continuing"; | ||
| if (keyId.empty()) | ||
| { | ||
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | ||
| } | ||
|
|
||
| if (!keyId.empty()) | ||
| { | ||
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | ||
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | ||
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | ||
| { | ||
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | ||
| message.append("[%s]", toString(keyStatus)); | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); | ||
| } |
There was a problem hiding this comment.
protectionData comes from the GstBuffer meta (GstProtectionMetadataHelper::getProtectionMetadataData returns a pointer into the meta). After a decrypt failure, this code calls removeProtectionMetadata(buffer) earlier, which removes the meta and frees/unrefs protectionData->key. Using protectionData->key here to extract keyId (and later using protectionData->keySessionId) is therefore a use-after-free risk. Fix by extracting/caching keySessionId and keyId (or ref-counting the key buffer) before removing the protection metadata, or delay removeProtectionMetadata until after the failure handling finishes.
| // Notify dropped frame upstream as a non-fatal message | |
| std::string message = "Failed to decrypt buffer, dropping frame and continuing"; | |
| if (keyId.empty()) | |
| { | |
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | |
| } | |
| if (!keyId.empty()) | |
| { | |
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | |
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | |
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | |
| { | |
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | |
| message.append("[%s]", toString(keyStatus)); | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | |
| } | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); | |
| } | |
| // Notify dropped frame upstream as a non-fatal message. | |
| // Do not access protectionData here because the protection metadata may | |
| // already have been removed on the failure path, making protectionData | |
| // and its members invalid. | |
| std::string message = "Failed to decrypt buffer, dropping frame and continuing"; |
| if (mediaKeysHandleIter == m_sessionInfo.end()) | ||
| { | ||
| RIALTO_SERVER_LOG_ERROR("Media keys handle for mksId: %d does not exists", keySessionId); | ||
| return MediaKeyErrorStatus::FAIL; |
There was a problem hiding this comment.
Grammar in the log message: "does not exists" should be "does not exist".
| TEST_F(RialtoServerDecryptorPrivateDecryptTest, DecryptionServiceDecryptFailure) | ||
| { | ||
| expectGetInfoFromProtectionMeta(); | ||
| expectWidevineKeySystem(); | ||
| expectAddGstProtectionMeta(true); | ||
| expectGetKeyStatus(KeyStatus::OUTPUT_RESTRICTED); | ||
| expectDecryptWarning(); | ||
|
|
There was a problem hiding this comment.
This test exercises the new getKeyStatus call, but it still accepts any warning/error message (gErrorNewLiteral(..., _)). Since the PR’s purpose is to surface the specific decryption failure reason, add an assertion that the posted message (and/or the gErrorNewLiteral text) includes the key status when getKeyStatus succeeds (and does not when it fails). This will prevent regressions where the status is queried but never propagated to the user-visible error.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!keyId.empty()) | ||
| { | ||
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | ||
| if (firebolt::rialto::MediaKeyErrorStatus::OK == |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
media/server/gstplayer/source/GstDecryptor.cpp:232:1: error: Unmatched '{'. Configuration: ''. [syntaxError] |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (keyId.empty()) | ||
| { | ||
| if (m_decryptionService) | ||
| { | ||
| if (keyId.empty()) |
There was a problem hiding this comment.
protectionData comes from the GstBuffer meta (GstProtectionMetadataHelper::getProtectionMetadataData returns a pointer to protectionMetadata->data). This function removes that meta before this block (removeProtectionMetadata(buffer)), which triggers rialto_eme_protection_metadata_free and unrefs protectionMeta->data.key. Using protectionData->key here can therefore be a use-after-free. Capture keySessionId and extract/copy the keyId (or gst_buffer_ref the key buffer) before removing the metadata, or move metadata removal until after the warning message is generated.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| void expectGetKeyStatus(KeyStatus keyStatus) | ||
| { | ||
| EXPECT_CALL(*m_gstWrapperMock, gstBufferMap(&m_key, _, GST_MAP_READ)) | ||
| .WillOnce(DoAll(SetArgPointee<1>(m_keyMap), Return(TRUE))); | ||
| EXPECT_CALL(*m_gstWrapperMock, gstBufferUnmap(&m_key, _)); | ||
| EXPECT_CALL(*m_decryptionServiceMock, getKeyStatus(m_keySessionId, m_keyVector, _)) | ||
| .WillOnce(DoAll(SetArgReferee<2>(keyStatus), Return(MediaKeyErrorStatus::OK))); | ||
| } |
There was a problem hiding this comment.
The new key-status behavior in GstRialtoDecryptorPrivate::decrypt is meant to surface a specific reason for decrypt failures, but this unit test only asserts that getKeyStatus() is called and still accepts any warning/error message (_). Consider tightening the expectation to verify the posted warning message includes the key status string (e.g., "[output-restricted]") so regressions in the new user-visible error text are caught.
| if (keyId.empty()) | ||
| { | ||
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | ||
| } | ||
|
|
||
| if (!keyId.empty()) | ||
| { | ||
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | ||
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | ||
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | ||
| { | ||
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | ||
| message += "["; | ||
| message += toString(keyStatus); | ||
| message += "]"; | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); |
There was a problem hiding this comment.
In the decrypt-failure path this code reads protectionData->key/protectionData->keySessionId, but protectionData is a pointer to GstRialtoProtectionMetadata::data (owned by the GstMeta). Earlier in this function the meta is removed via m_metadataWrapper->removeProtectionMetadata(buffer), which can invalidate protectionData and lead to a use-after-free. Capture the needed values (e.g., keySessionId and keyId/key buffer) before removing the metadata, or move the key-status retrieval/logging before removeProtectionMetadata.
| if (keyId.empty()) | |
| { | |
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | |
| } | |
| if (!keyId.empty()) | |
| { | |
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | |
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | |
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | |
| { | |
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | |
| message += "["; | |
| message += toString(keyStatus); | |
| message += "]"; | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | |
| } | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); | |
| if (!keyId.empty()) | |
| { | |
| GST_WARNING_OBJECT(self, | |
| "Decrypt failure for key id %s; key status lookup skipped because protection " | |
| "metadata is no longer available", | |
| keyId.c_str()); | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, | |
| "Failed to decrypt buffer and no key id was captured before protection metadata " | |
| "was removed"); |
|
Coverage statistics of your commit: |
Signed-off-by: Varatharajan_Narayanan <Varatharajan_Narayanan@comcast.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| else if (warningContains(err, debug, "OUTPUT_RESTRICTED")) | ||
| { | ||
| rialtoError = PlaybackError::HDCPPROTECTION; | ||
| } | ||
| else if (warningContains(err, debug, "INTERNAL_ERROR")) |
There was a problem hiding this comment.
The substring checks here look for "OUTPUT_RESTRICTED" and "INTERNAL_ERROR", but the decryptor’s appended key-status string is currently "output-restricted" / "internal-error" (see media/server/gstplayer/source/GstDecryptor.cpp). As a result these branches won’t trigger and decrypt failures will still be reported as DECRYPTION. Align the tokens between producer and consumer (e.g., change the searched substrings to match the actual message, or emit enum-name style strings from GstDecryptor).
| else if (warningContains(err, debug, "OUTPUT_RESTRICTED")) | |
| { | |
| rialtoError = PlaybackError::HDCPPROTECTION; | |
| } | |
| else if (warningContains(err, debug, "INTERNAL_ERROR")) | |
| else if (warningContains(err, debug, "output-restricted") || | |
| warningContains(err, debug, "OUTPUT_RESTRICTED")) | |
| { | |
| rialtoError = PlaybackError::HDCPPROTECTION; | |
| } | |
| else if (warningContains(err, debug, "internal-error") || | |
| warningContains(err, debug, "INTERNAL_ERROR")) |
| if (warningContains(err, debug, "expired")) | ||
| { | ||
| rialtoError = PlaybackError::KEY; | ||
| } | ||
| else if (warningContains(err, debug, "OUTPUT_RESTRICTED")) | ||
| { | ||
| rialtoError = PlaybackError::HDCPPROTECTION; | ||
| } | ||
| else if (warningContains(err, debug, "INTERNAL_ERROR")) | ||
| { | ||
| rialtoError = PlaybackError::DRM; | ||
| } | ||
| else | ||
| { | ||
| rialtoError = PlaybackError::DECRYPTION; | ||
| } |
There was a problem hiding this comment.
New playback-error classification logic is introduced here (KEY/HDCPPROTECTION/DRM based on message substrings), but the existing HandleBusMessage unit tests still only assert DECRYPTION for decrypt warnings. Please extend the HandleBusMessage tests to cover each new branch by setting err->message/debug to include the expected token and asserting the corresponding PlaybackError value.
| private: | ||
| bool allSourcesEos() const; | ||
| static bool warningContains(const GError *error, const gchar *debugMessage, | ||
| const char *value); |
There was a problem hiding this comment.
This declaration introduces tab indentation (before const char *value) which is inconsistent with the surrounding spacing in this header and makes formatting harder to read/maintain. Replace tabs with spaces and align the wrapped parameters consistently.
| const char *value); | |
| const char *value); |
| { | ||
| return firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPCOMPLIANCE; | ||
| } | ||
| case firebolt::rialto::PlaybackError::DRM: |
There was a problem hiding this comment.
There’s a tab-indented case label here (and the indentation doesn’t match the rest of the switch). Please replace the tab with spaces to keep formatting consistent and avoid clang-format / lint noise.
| case firebolt::rialto::PlaybackError::DRM: | |
| case firebolt::rialto::PlaybackError::DRM: |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPPROTECTION: | ||
| playbackError = PlaybackError::HDCPPROTECTION; | ||
| break; | ||
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPCOMPLIANCE: | ||
| playbackError = PlaybackError::HDCPCOMPLIANCE; | ||
| break; | ||
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_DRM: |
There was a problem hiding this comment.
Several case labels here are tab-indented, which is inconsistent with the surrounding switch formatting. Please replace tabs with spaces to keep formatting consistent and avoid formatting/lint issues.
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPPROTECTION: | |
| playbackError = PlaybackError::HDCPPROTECTION; | |
| break; | |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPCOMPLIANCE: | |
| playbackError = PlaybackError::HDCPCOMPLIANCE; | |
| break; | |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_DRM: | |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPPROTECTION: | |
| playbackError = PlaybackError::HDCPPROTECTION; | |
| break; | |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_HDCPCOMPLIANCE: | |
| playbackError = PlaybackError::HDCPCOMPLIANCE; | |
| break; | |
| case firebolt::rialto::PlaybackErrorEvent_PlaybackError_DRM: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (keyId.empty()) | ||
| { | ||
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | ||
| } | ||
|
|
||
| if (!keyId.empty()) | ||
| { | ||
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | ||
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | ||
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | ||
| { | ||
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | ||
| message += "["; | ||
| message += toString(keyStatus); | ||
| message += "]"; | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | ||
| } | ||
| } | ||
| else | ||
| { | ||
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); |
There was a problem hiding this comment.
protectionData is a pointer into the GstMeta returned by getProtectionMetadataData(), but the meta is removed via m_metadataWrapper->removeProtectionMetadata(buffer) before this block. Accessing protectionData->key/keySessionId here can become a use-after-free once the meta is removed. Capture the needed fields (e.g., keySessionId and a safe copy/ref of the key buffer or keyId) before removing the meta, or defer removing the meta until after the warning message is posted.
| if (keyId.empty()) | |
| { | |
| keyId = extractKeyId(m_gstWrapper, protectionData->key); | |
| } | |
| if (!keyId.empty()) | |
| { | |
| firebolt::rialto::KeyStatus keyStatus{firebolt::rialto::KeyStatus::INTERNAL_ERROR}; | |
| if (firebolt::rialto::MediaKeyErrorStatus::OK == | |
| m_decryptionService->getKeyStatus(protectionData->keySessionId, keyId, keyStatus)) | |
| { | |
| GST_WARNING_OBJECT(self, "Key status after decrypt failure: %s", toString(keyStatus)); | |
| message += "["; | |
| message += toString(keyStatus); | |
| message += "]"; | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to get key status after decrypt failure"); | |
| } | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to extract key id after decrypt failure"); | |
| if (!keyId.empty()) | |
| { | |
| GST_WARNING_OBJECT(self, "Decrypt failure reported for key id"); | |
| } | |
| else | |
| { | |
| GST_WARNING_OBJECT(self, "Failed to determine key id after decrypt failure"); |
| if (warningContains(err, debug, "expired")) | ||
| { | ||
| rialtoError = PlaybackError::KEY; | ||
| } | ||
| else if (warningContains(err, debug, "OUTPUT_RESTRICTED")) | ||
| { | ||
| rialtoError = PlaybackError::HDCPPROTECTION; |
There was a problem hiding this comment.
New branches mapping decrypt warnings to PlaybackError::KEY / HDCPPROTECTION / DRM aren’t covered by the existing HandleBusMessageTest decrypt-warning tests (they currently only validate the default DECRYPTION path). Please add unit tests that set err->message or debug to include the key-status tokens so each new mapping is exercised.
Summary: Retrieved the specific reason for decryption failure instead of reporting a generic error
Type: Feature
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-16874