RDKEMW-20661: Bring in fix for WPEWebProcess crashed with fingerprint 80095400 to develop#203
Open
nejuma1 wants to merge 2 commits into
Open
RDKEMW-20661: Bring in fix for WPEWebProcess crashed with fingerprint 80095400 to develop#203nejuma1 wants to merge 2 commits into
nejuma1 wants to merge 2 commits into
Conversation
… 80095400 to 8.6 Reason for change : Added fix for the race condition by using weakpointer instead of raw pointer Priority : P1 Test Steps: Mentioned in ticket Signed off by : Nejuma T N <nejumatn28@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR primarily addresses teardown-time races by making the periodic progress timer callback stop using a raw InterfacePlayerRDK* user pointer, and adds several related fixes/improvements across subtitle timing, DRM decryptor caps handling, and Darwin build/link behavior.
Changes:
- Replace raw-pointer timer user_data with a weak-pointer context + add
g_timeout_add_full()support so timer user_data can be safely destroyed during teardown. - Add a subtitle PTS offset API and implement it for the WebVTT Subtec parser (signed offset support).
- Improve robustness in DRM decryptor caps negotiation (wait/guard against
sinkCaps == NULL) and adjust multiple CMake Darwin link behaviors to prefer full-path_LINK_LIBRARIES.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/mocks/MockGLib.h | Adds g_timeout_add_full to the GLib mock API used by unit tests. |
| test/utests/fakes/FakeGLib.cpp | Routes g_timeout_add_full through the GLib mock instead of returning a stub value. |
| subtitle/subtitleParser.h | Adds new virtual setPtsOffset(double) hook for subtitle timing alignment. |
| subtec/subtecparser/WebVttSubtecParser.hpp | Declares setPtsOffset override and changes offset type to signed. |
| subtec/subtecparser/WebVttSubtecParser.cpp | Applies time_offset_ms_ to sent packets and implements setPtsOffset conversion/negation logic. |
| InterfacePlayerRDK.h | Introduces ProgressCallbackContext, new timer destroy-notify support, and progress callback context helpers. |
| InterfacePlayerRDK.cpp | Implements weak-context-based progress timer callback, context cancellation/wait logic, timer destroy-notify support, scheduler stop/start changes in Stop(), safer rectangle parsing, and safer callback invocation. |
| gst-plugins/gst_subtec/CMakeLists.txt | Updates include paths and uses _LINK_LIBRARIES to avoid Darwin link_directories() issues. |
| gst-plugins/drm/gst/gstcdmidecryptor.h | Adds a condition variable for waiting on sink caps negotiation. |
| gst-plugins/drm/gst/gstcdmidecryptor.cpp | Signals/waits for sinkCaps readiness and guards decrypt paths against null caps. |
| gst-plugins/CMakeLists.txt | Adjusts Darwin linking to avoid link_directories() warnings and uses UUID pkg-config. |
| externals/CMakeLists.txt | Switches Darwin externals to use pkg-config full-path link libraries instead of GStreamer.framework. |
| drm/PlayerHlsDrmSessionInterface.h | Declares a destructor for cleanup of the DRM session manager (non-adapter builds). |
| drm/PlayerHlsDrmSessionInterface.cpp | Implements destructor cleanup logic. |
| drm/ocdm/OcdmGstSessionAdapter.cpp | Adds a null check before caps type/emptiness checks. |
| drm/DrmSessionManager.h | Adds move ctor/assignment and includes <utility>. |
| CMakeLists.txt | Adjusts include dirs and Darwin linking to use _LINK_LIBRARIES variables. |
| .gitignore | Ignores build_aamp/ output directory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+3827
to
+3834
| this->TimerRemove(interfacePlayerPriv->gstPrivateContext->periodicProgressCallbackIdleTaskId, "periodicProgressCallbackIdleTaskId"); | ||
| std::unique_lock<std::mutex> lock(callbackContext->mutex); | ||
| if (!callbackContext->cv.wait_for(lock, std::chrono::seconds(5), [&callbackContext]() { | ||
| return 0 == callbackContext->activeCallbacks; | ||
| })) { | ||
| MW_LOG_WARN("Teardown timeout: callback context still has active callbacks (%zu). Possible I/O blockage or deadlock detected.", callbackContext->activeCallbacks); | ||
| } | ||
| lock.unlock(); |
Comment on lines
+48
to
+55
| PlayerHlsDrmSessionInterface::~PlayerHlsDrmSessionInterface() | ||
| { | ||
| #ifndef USE_OPENCDM_ADAPTER | ||
| if(m_pHlsDrmSessionManager != nullptr) | ||
| { | ||
| delete m_pHlsDrmSessionManager; | ||
| m_pHlsDrmSessionManager = nullptr; | ||
| } |
Comment on lines
+3854
to
+3858
| if (destroyNotify) | ||
| { | ||
| taskId = g_timeout_add_full(G_PRIORITY_DEFAULT, repeatTimeout, funcPtr, user_data, destroyNotify); | ||
| if (0 == taskId) | ||
| { |
Comment on lines
+74
to
+86
| m_channel->SendDataPacket(std::move(data), time_offset_ms_); | ||
|
|
||
| return true; | ||
| } | ||
|
|
||
| void WebVTTSubtecParser::setPtsOffset(double ptsOffsetSec) | ||
| { | ||
| // Subtec's display_time = media_PTS - time_offset_ms (subtraction | ||
| // convention shared with Rialto SetSubtitlePtsOffset). The HLS | ||
| // restamped video PTS is media_PTS + ptsOffsetSec, so we negate | ||
| // here to make subtec add the offset on the subtitle path. | ||
| time_offset_ms_ = -static_cast<std::int64_t>(std::llround(ptsOffsetSec * 1000.0)); | ||
| } |
Comment on lines
+96
to
+104
| /** | ||
| * @brief Set a per-fragment PTS offset (seconds) that the parser | ||
| * applies when forwarding cue data to its subtitle sink. | ||
| * | ||
| * Used by the HLS PTS-restamp path to align subtitle display time | ||
| * with the restamped video PTS without rewriting MPEGTS in the | ||
| * VTT header. Default no-op; parsers that need this behavior override. | ||
| */ | ||
| virtual void setPtsOffset(double ptsOffsetSec) {} |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| { | ||
| mProgressCallbackContext.reset(); | ||
| } | ||
| } |
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.
RDKEMW-20661: Bring in fix for WPEWebProcess crashed with fingerprint 80095400 to develop
Reason for change : Added fix for the race condition by using weakpointer instead of raw pointer
Priority : P1
Test Steps: Mentioned in ticket
Signed off by : Nejuma T N nejumatn28@gmail.com