Conversation
Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>
Co-authored-by: Marcin Wojciechowski <105790697+skywojciechowskim@users.noreply.github.com>
Summary: SetPlaybackRate modification.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: DELIA-70055
|
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 pull request refactors the Limited Duration License (LDL) handling in the media keys implementation and modifies the SetPlaybackRate task to support decoder-specific event handling. The main changes include:
Changes:
- Removed the
isLDLparameter fromcreateKeySessionAPI and replaced it with dynamic detection of extended interface usage based on which APIs are called - Added
ldlStateparameter togenerateRequestAPI with a default value for backward compatibility - Modified SetPlaybackRate to send rate change events directly to broadcom video/audio decoders when present, instead of always sending to the pipeline
- Renamed
isNetflixPlayreadyKeySystemtoisExtendedInterfaceUsedto better reflect its purpose - Added
gstEventRefandgstEventUnrefwrapper methods - Added new library dependency
rdkgstreamerutilsplatform
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| wrappers/interface/IGstWrapper.h | Added gstEventRef and gstEventUnref methods to wrapper interface |
| wrappers/include/GstWrapper.h | Implemented gstEventRef and gstEventUnref wrapper methods |
| wrappers/CMakeLists.txt | Added rdkgstreamerutilsplatform library dependency |
| media/server/gstplayer/source/tasks/generic/SetPlaybackRate.cpp | Modified to send rate change events to decoders directly for broadcom decoders; added player parameter for decoder access |
| media/server/gstplayer/include/tasks/generic/SetPlaybackRate.h | Added player member and updated constructor signature |
| media/server/gstplayer/include/tasks/generic/GenericPlayerTaskFactory.h | Updated createSetPlaybackRate signature to include player parameter |
| media/server/gstplayer/source/tasks/generic/GenericPlayerTaskFactory.cpp | Updated createSetPlaybackRate implementation |
| media/server/gstplayer/source/GstGenericPlayer.cpp | Updated setPlaybackRate call to pass player instance |
| media/server/gstplayer/include/IGenericPlayerTaskFactory.h | Updated createSetPlaybackRate interface signature |
| media/server/gstplayer/include/IGstGenericPlayerPrivate.h | Added getDecoder method to interface |
| media/server/gstplayer/source/GstDecryptor.cpp | Updated to use isExtendedInterfaceUsed instead of isNetflixPlayreadyKeySystem |
| media/server/service/source/CdmService.h | Renamed isNetflixPlayready to isExtendedInterfaceUsed in MediaKeySessionInfo |
| media/server/service/source/CdmService.cpp | Implemented dynamic extended interface detection; renamed method; removed isLDL from createKeySession |
| media/server/service/include/ICdmService.h | Updated interface signatures for createKeySession and generateRequest |
| media/server/main/source/MediaKeysServerInternal.cpp | Removed isLDL parameter and isNetflixPlayreadyKeySystem method |
| media/server/main/source/MediaKeySession.cpp | Added extendedInterfaceInUse flag; implemented queued DRM header logic; removed isLDL member; updated getChallenge to use ldlState |
| media/server/main/include/MediaKeysServerInternal.h | Updated interface signatures |
| media/server/main/include/MediaKeySession.h | Removed isLDL member; added extendedInterfaceInUse and queuedDrmHeader members |
| media/server/main/include/IMediaKeySession.h | Updated generateRequest signature; removed isNetflixPlayreadyKeySystem |
| media/server/main/interface/IMediaKeysServerInternal.h | Removed isNetflixPlayreadyKeySystem method |
| media/server/main/interface/IDecryptionService.h | Renamed isNetflixPlayreadyKeySystem to isExtendedInterfaceUsed |
| media/server/ipc/source/MediaKeysModuleService.cpp | Added LDL conversion function; updated createKeySession and generateRequest calls |
| media/public/include/MediaCommon.h | Added LimitedDurationLicense enum |
| media/public/include/IMediaKeys.h | Removed isLDL from createKeySession; added ldlState with default to generateRequest |
| media/client/main/source/MediaKeys.cpp | Updated method signatures |
| media/client/main/include/MediaKeys.h | Updated method signatures |
| media/client/ipc/source/MediaKeysIpc.cpp | Removed isLDL handling; added ldlState conversion and handling |
| media/client/ipc/include/MediaKeysIpc.h | Updated method signatures |
| proto/mediakeysmodule.proto | Added LimitedDurationLicense enum; removed is_ldl field; added ldl_state field |
| tests/* (multiple files) | Updated all tests to match new signatures; removed isNetflixPlayreadyKeySystem tests; added LDL-related tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| else | ||
| { | ||
| success = m_gstWrapper->gstElementSendEvent(m_context.pipeline, instantRateChangeEvent); |
There was a problem hiding this comment.
Memory leak: When gstElementSendEvent to the pipeline fails at line 123, the instantRateChangeEvent is not unreferenced. According to GStreamer documentation, when gst_element_send_event returns FALSE, the event is not consumed and the caller must unref it. Add an unref call for the failure case, similar to how it's handled at lines 104 and 115.
| success = m_gstWrapper->gstElementSendEvent(m_context.pipeline, instantRateChangeEvent); | |
| success = m_gstWrapper->gstElementSendEvent(m_context.pipeline, instantRateChangeEvent); | |
| if (!success) | |
| { | |
| m_gstWrapper->gstEventUnref(instantRateChangeEvent); | |
| } |
| GstElement *audioDecoder = m_player.getDecoder(firebolt::rialto::MediaSourceType::AUDIO); | ||
| GstElement *videoDecoder = m_player.getDecoder(firebolt::rialto::MediaSourceType::VIDEO); |
There was a problem hiding this comment.
Memory leak: The decoder elements obtained from getDecoder() are never unreferenced. Since getDecoder() returns a referenced GstElement (via gstObjectRef at line 502 of GstGenericPlayer.cpp), both audioDecoder and videoDecoder must be unreferenced with gstObjectUnref() before the function returns. This should be done at the end of the execute() method, similar to how audioSink is unreferenced at line 136.
| if (m_sessionInfo.find(keySessionId) != m_sessionInfo.end()) | ||
| { | ||
| m_sessionInfo[keySessionId].isExtendedInterfaceUsed = true; | ||
| } |
There was a problem hiding this comment.
Redundant check: The session existence check at line 538 is redundant. The session existence was already verified at line 532-537, and if it doesn't exist, the function returns early. The check at line 538 can be removed, and line 540 can simply use the existing mediaKeysHandleIter iterator.
| if (m_sessionInfo.find(keySessionId) != m_sessionInfo.end()) | |
| { | |
| m_sessionInfo[keySessionId].isExtendedInterfaceUsed = true; | |
| } | |
| mediaKeysHandleIter->second.isExtendedInterfaceUsed = true; |
| bool isLDL, int32_t &keySessionId) = 0; | ||
| int32_t &keySessionId) = 0; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Breaking API change: The removal of the 'isLDL' parameter from createKeySession() is a breaking change to the public API. Any existing code calling this function with the isLDL parameter will fail to compile. This should be clearly documented in the PR description and release notes. Consider whether this breaking change is acceptable for your versioning strategy.
| /** | |
| /** | |
| * @brief Creates a session and returns the session id (legacy overload with isLDL flag). | |
| * | |
| * This overload is kept for backwards compatibility with older clients that used the isLDL | |
| * parameter. The isLDL value is ignored; use the three-parameter overload instead. | |
| * | |
| * @deprecated Use createKeySession(KeySessionType, std::weak_ptr<IMediaKeysClient>, int32_t &) instead. | |
| * | |
| * @param[in] sessionType : The session type. | |
| * @param[in] client : Client object for callbacks | |
| * @param[out] keySessionId: The key session id | |
| * @param[in] isLDL : Legacy Limited Duration License flag (ignored). | |
| * | |
| * @retval an error status. | |
| */ | |
| [[deprecated("Use createKeySession(KeySessionType, std::weak_ptr<IMediaKeysClient>, int32_t &) instead.")]] | |
| MediaKeyErrorStatus createKeySession(KeySessionType sessionType, std::weak_ptr<IMediaKeysClient> client, | |
| int32_t &keySessionId, bool isLDL) | |
| { | |
| (void)isLDL; | |
| return createKeySession(sessionType, client, keySessionId); | |
| } | |
| /** |
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
Summary: SetPlaybackRate modification.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: DELIA-70055
Summary: Removed unused library.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: DELIA-70055
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 70 out of 70 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| GstEvent *gstEventRef(GstEvent *event) const override { return gst_event_ref(event); } | ||
|
|
||
| void gstEventUnref(GstEvent *event) const override { gst_event_unref(event); }; |
There was a problem hiding this comment.
There is an extra trailing semicolon after the inline gstEventUnref definition ({ ... };). While most compilers accept this, it’s unnecessary and can trigger style warnings. Consider removing the trailing semicolon for consistency with the other inline methods.
| * | ||
| * @param[in] mediaSourceType : the source type to obtain the decoder for | ||
| * | ||
| * @retval The decoder, NULL if not found |
There was a problem hiding this comment.
getDecoder() returns a GstElement* but the comment doesn’t specify ownership/refs (unlike getSink() which explicitly mentions unref). Since the implementation returns a ref-counted element (gstObjectRef), please document that the caller must unref the returned element to avoid leaks.
| * @retval The decoder, NULL if not found | |
| * @retval The decoder, NULL if not found. Please call getObjectUnref() if it's non-null |
| ACTION_P(memcpyChallenge, vec) | ||
| { | ||
| memcpy(arg1, &vec[0], vec.size()); | ||
| } |
There was a problem hiding this comment.
memcpyChallenge uses memcpy but this header doesn't include <cstring> (or <string.h>), which can cause build failures due to memcpy being undeclared. Add the proper header (and consider using std::memcpy) so the symbol is guaranteed to be available.
| GstElement *audioDecoder = m_player.getDecoder(firebolt::rialto::MediaSourceType::AUDIO); | ||
| GstElement *videoDecoder = m_player.getDecoder(firebolt::rialto::MediaSourceType::VIDEO); | ||
|
|
There was a problem hiding this comment.
getDecoder() returns a ref-counted GstElement* (it calls gstObjectRef). The acquired audioDecoder/videoDecoder are never unreffed here, which will leak references each time playback rate is set. Ensure these elements are unreffed on all paths once you're done with them (including early-exit paths after the send).
| if ((videoDecoder && (0 == strcmp(GST_ELEMENT_NAME(videoDecoder), "brcmvideodecoder"))) || | ||
| (audioDecoder && (0 == strcmp(GST_ELEMENT_NAME(audioDecoder), "brcmaudiodecoder")))) |
There was a problem hiding this comment.
This code uses strcmp but the file doesn't include <cstring>/<string.h>, which can result in strcmp being undeclared depending on include order. Also, comparing GST_ELEMENT_NAME(decoder) for exact equality is brittle because element instance names are often auto-suffixed (e.g., "brcmvideodecoder0"); consider checking the element factory name or using a prefix match if the intent is to detect the Broadcom decoder type.
| if ((videoDecoder && (0 == strcmp(GST_ELEMENT_NAME(videoDecoder), "brcmvideodecoder"))) || | |
| (audioDecoder && (0 == strcmp(GST_ELEMENT_NAME(audioDecoder), "brcmaudiodecoder")))) | |
| if ((videoDecoder && | |
| m_glibWrapper->gStrHasPrefix(GST_ELEMENT_NAME(videoDecoder), "brcmvideodecoder")) || | |
| (audioDecoder && | |
| m_glibWrapper->gStrHasPrefix(GST_ELEMENT_NAME(audioDecoder), "brcmaudiodecoder"))) |
| setDrmHeader(m_queuedDrmHeader); | ||
| m_queuedDrmHeader.clear(); |
There was a problem hiding this comment.
The queued DRM header is applied after session construction, but the return value from setDrmHeader() is ignored and the queued header is cleared unconditionally. If setting the header fails, the failure will be silently lost and the session may proceed without the DRM header. Capture and handle the return status (and/or only clear the queued header on success) so failures propagate correctly.
| setDrmHeader(m_queuedDrmHeader); | |
| m_queuedDrmHeader.clear(); | |
| auto headerStatus = setDrmHeader(m_queuedDrmHeader); | |
| if (MediaKeyErrorStatus::OK != headerStatus) | |
| { | |
| RIALTO_SERVER_LOG_ERROR("Failed to set queued drm header after session construction"); | |
| status = headerStatus; | |
| } | |
| else | |
| { | |
| m_queuedDrmHeader.clear(); | |
| } |
| std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> gstWrapper, | ||
| std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> glibWrapper, double rate) | ||
| : m_context{context}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} | ||
| : m_context{context}, m_player{player}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"glibWrapper" is passed-by-value as parameter to "std::shared_ptrfirebolt::rialto::wrappers::IGlibWrapper::shared_ptr(std::shared_ptrfirebolt::rialto::wrappers::IGlibWrapper const &) /explicit =default/", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""glibWrapper"")" instead of "glibWrapper".
| std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> gstWrapper, | ||
| std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> glibWrapper, double rate) | ||
| : m_context{context}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} | ||
| : m_context{context}, m_player{player}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} |
There was a problem hiding this comment.
Coverity Issue - Variable copied when it could be moved
"gstWrapper" is passed-by-value as parameter to "std::shared_ptrfirebolt::rialto::wrappers::IGstWrapper::shared_ptr(std::shared_ptrfirebolt::rialto::wrappers::IGstWrapper const &) /explicit =default/", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""gstWrapper"")" instead of "gstWrapper".
| std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> gstWrapper, | ||
| std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> glibWrapper, double rate) | ||
| : m_context{context}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} | ||
| : m_context{context}, m_player{player}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Variable copied when it could be moved
"glibWrapper" is passed-by-value as parameter to "std::shared_ptrfirebolt::rialto::wrappers::IGlibWrapper::shared_ptr(std::shared_ptrfirebolt::rialto::wrappers::IGlibWrapper const &) /explicit =default/", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""glibWrapper"")" instead of "glibWrapper".
| std::shared_ptr<firebolt::rialto::wrappers::IGstWrapper> gstWrapper, | ||
| std::shared_ptr<firebolt::rialto::wrappers::IGlibWrapper> glibWrapper, double rate) | ||
| : m_context{context}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} | ||
| : m_context{context}, m_player{player}, m_gstWrapper{gstWrapper}, m_glibWrapper{glibWrapper}, m_rate{rate} |
There was a problem hiding this comment.
Coverity issue no longer present as of: undefined
Show issue
Coverity Issue - Variable copied when it could be moved
"gstWrapper" is passed-by-value as parameter to "std::shared_ptrfirebolt::rialto::wrappers::IGstWrapper::shared_ptr(std::shared_ptrfirebolt::rialto::wrappers::IGstWrapper const &) /explicit =default/", when it could be moved instead.
Low Impact, CWE-none
COPY_INSTEAD_OF_MOVE
How to fix
Use "std::move(""gstWrapper"")" instead of "gstWrapper".
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
|
b'## WARNING: A Blackduck scan failure has been waived A prior failure has been upvoted
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| GstEvent *videoRateChangeEvent = m_gstWrapper->gstEventRef(instantRateChangeEvent); | ||
| if (!m_gstWrapper->gstElementSendEvent(videoDecoder, videoRateChangeEvent)) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("Sent new event to videoDecoder failed"); | ||
| success = false; | ||
| } | ||
| m_gstWrapper->gstEventUnref(videoRateChangeEvent); | ||
| } |
There was a problem hiding this comment.
gstElementSendEvent() (wrapping gst_element_send_event) takes ownership of the event. Unref'ing videoRateChangeEvent after sending will likely double-unref the event and can cause a use-after-free. Remove the explicit unref for the event passed to gstElementSendEvent (keep only the unref for the original instantRateChangeEvent after cloning refs).
| GstEvent *audioRateChangeEvent = m_gstWrapper->gstEventRef(instantRateChangeEvent); | ||
| if (!m_gstWrapper->gstElementSendEvent(audioDecoder, audioRateChangeEvent)) | ||
| { | ||
| RIALTO_SERVER_LOG_INFO("Sent new event to audioDecoder failed"); | ||
| success = false; | ||
| } | ||
| m_gstWrapper->gstEventUnref(audioRateChangeEvent); | ||
| } |
There was a problem hiding this comment.
gstElementSendEvent() takes ownership of the event. Unref'ing audioRateChangeEvent after sending can double-unref and lead to memory safety issues. Don't unref the event pointer after passing it to gstElementSendEvent; only unref the original instantRateChangeEvent after creating the refs for each send.
| m_glibWrapper->gObjectUnref(audioDecoder); | ||
| } | ||
| if (videoDecoder) | ||
| { | ||
| m_glibWrapper->gObjectUnref(videoDecoder); |
There was a problem hiding this comment.
getDecoder() returns a ref'd GstElement* (it uses gstObjectRef in GstGenericPlayer::getDecoder). These should be released via m_gstWrapper->gstObjectUnref(...) for symmetry and to keep unref operations mockable/consistent with the rest of the player code (vs gObjectUnref).
| m_glibWrapper->gObjectUnref(audioDecoder); | |
| } | |
| if (videoDecoder) | |
| { | |
| m_glibWrapper->gObjectUnref(videoDecoder); | |
| m_gstWrapper->gstObjectUnref(audioDecoder); | |
| } | |
| if (videoDecoder) | |
| { | |
| m_gstWrapper->gstObjectUnref(videoDecoder); |
| * @retval The decoder, NULL if not found | ||
| */ | ||
| virtual GstElement *getDecoder(const MediaSourceType &mediaSourceType) = 0; |
There was a problem hiding this comment.
The new getDecoder() API doesn't document ownership/ref semantics. Since GstGenericPlayer::getDecoder() returns a ref'd element (gstObjectRef), the interface comment should explicitly state that the caller must unref the returned element (e.g., via gstObjectUnref). Also consider making this method const (like getSink) since it appears to be a read-only lookup.
| * @retval The decoder, NULL if not found | |
| */ | |
| virtual GstElement *getDecoder(const MediaSourceType &mediaSourceType) = 0; | |
| * @retval The decoder, NULL if not found. Please unref the returned element | |
| * (e.g. via gst_object_unref()) if it is non-null. | |
| */ | |
| virtual GstElement *getDecoder(const MediaSourceType &mediaSourceType) const = 0; |
| * @retval the new SetPlaybackRate task instance. | ||
| */ | ||
| virtual std::unique_ptr<IPlayerTask> createSetPlaybackRate(GenericPlayerContext &context, double rate) const = 0; | ||
| virtual std::unique_ptr<IPlayerTask> createSetPlaybackRate(GenericPlayerContext &context, IGstGenericPlayerPrivate &player, double rate) const = 0; |
There was a problem hiding this comment.
This signature change will break existing unit test mocks/call sites that still use createSetPlaybackRate(context, rate) (e.g., GenericPlayerTaskFactoryMock, GenericPlayerTaskFactoryTest, and GenericTasksTestsBase::triggerSetPlaybackRate). Update all implementations/mocks/tests to pass IGstGenericPlayerPrivate &player to avoid build failures.
| if ((videoDecoder && (0 == strcmp(GST_ELEMENT_NAME(videoDecoder), "brcmvideodecoder"))) || | ||
| (audioDecoder && (0 == strcmp(GST_ELEMENT_NAME(audioDecoder), "brcmaudiodecoder")))) | ||
| { |
There was a problem hiding this comment.
The new Broadcom-decoder-specific branch (sending the custom rate-change event directly to brcm*decoder elements) is not covered by the existing SetPlaybackRate task unit tests. Add UT coverage for this branch (including the failure paths for audio/video send) to prevent regressions.
Summary: SetPlaybackRate modification.
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: DELIA-70055