RDKEMW-23040: NTS Resolution switch and PLAY-DRM* failure with Rialto - #597
Conversation
|
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 > |
There was a problem hiding this comment.
Pull request overview
Addresses a race where playback-related GStreamer element properties can be applied too late (after brcmvidfilter transitions NULL→READY), causing intermittent NTS resolution switch and PLAY-DRM failures, by ensuring queued properties are applied as soon as relevant elements are added.
Changes:
- Apply queued
stream-sync-mode/syncmode-streaming,limit-buffering-ms, andsync-offimmediately inGstGenericPlayer::setupElement()when the corresponding decoder/parser appears. - Add
propertyMutexprotection around writes to pending property state in several generic tasks. - Update unit/component tests to account for the new factory checks and updated call counts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| media/server/gstplayer/source/GstGenericPlayer.cpp | Applies queued properties during element-setup to avoid missing the configuration window. |
| media/server/gstplayer/source/Utils.cpp | Adds factory-based overload helpers to classify elements without re-querying factories. |
| media/server/gstplayer/include/Utils.h | Exposes new factory-based overloads used by setupElement(). |
| media/server/gstplayer/source/tasks/generic/SetUseBuffering.cpp | Guards pending property write with propertyMutex. |
| media/server/gstplayer/source/tasks/generic/SetSyncOff.cpp | Guards pending property write with propertyMutex. |
| media/server/gstplayer/source/tasks/generic/SetStreamSyncMode.cpp | Guards pending property write with propertyMutex. |
| media/server/gstplayer/source/tasks/generic/SetBufferingLimit.cpp | Guards pending property write with propertyMutex. |
| tests/unittests/media/server/gstplayer/genericPlayer/GstGenericPlayerTest.cpp | Adds coverage for immediate application when video parser is added; updates setup expectations. |
| tests/componenttests/server/tests/mediaPipeline/UnderflowTest.cpp | Adjusts expectations for repeated factory type checks. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Coverage statistics of your commit: |
8ee4839 to
2ec07d8
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
media/server/gstplayer/source/GstGenericPlayer.cpp:409
- The new propertyMutex usage in setupElement() protects pending* values from the GStreamer thread, but GstGenericPlayer::setSyncOff() still reads and resets m_context.pendingSyncOff without taking propertyMutex (see GstGenericPlayer.cpp:2273-2292). That creates a potential data race between the worker thread and this callback when both try to consume/update pendingSyncOff. Consider guarding pendingSyncOff access in setSyncOff() with the same mutex (or otherwise ensuring single-threaded access).
std::unique_lock lock{self->m_context.propertyMutex};
media/server/gstplayer/source/GstGenericPlayer.cpp:475
- For the video parser branch, static_cast(int32_t) can produce values other than 0/1 (any non-zero becomes “true”), and the same operator[] pattern does an extra lookup. Coercing to TRUE/FALSE from a non-zero check and using the find() iterator keeps the stored/logged value deterministic.
if (self->m_context.pendingStreamSyncMode.find(MediaSourceType::VIDEO) !=
self->m_context.pendingStreamSyncMode.end())
{
gboolean streamSyncModeBoolean{
static_cast<gboolean>(self->m_context.pendingStreamSyncMode[MediaSourceType::VIDEO])};
self->m_context.pendingStreamSyncMode.erase(MediaSourceType::VIDEO);
lock.unlock();
2ec07d8 to
cb8609e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (2)
media/server/gstplayer/source/GstGenericPlayer.cpp:2302
- In setSyncOff(), pendingSyncOff is cleared unconditionally after applying the value. If another thread updates pendingSyncOff while the property is being applied, the later update can be lost when this code resets the pending value. Clearing should be conditional on the pending value still matching the value that was applied (or move/reset the pending value before applying while ensuring decoder != NULL).
m_gstWrapper->gstObjectUnref(decoder);
std::unique_lock lock{m_context.propertyMutex};
m_context.pendingSyncOff.reset();
}
media/server/gstplayer/source/tasks/generic/SetStreamSyncMode.cpp:44
- SetStreamSyncMode::execute() uses emplace() when updating pendingStreamSyncMode. If setStreamSyncMode() is called multiple times for the same MediaSourceType before the pending value is consumed, emplace() will keep the first value and ignore later updates, which is surprising for a setter. Prefer overwrite semantics (operator[]/insert_or_assign).
{
std::unique_lock lock{m_context.propertyMutex};
m_context.pendingStreamSyncMode.emplace(m_type, m_streamSyncMode);
}
cb8609e to
ad87533
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Suppressed comments (1)
media/server/gstplayer/source/tasks/generic/SetStreamSyncMode.cpp:44
pendingStreamSyncMode.emplace(m_type, m_streamSyncMode)will not overwrite an existing queued value for the sameMediaSourceType. IfsetStreamSyncMode()is called multiple times while the element is not yet available (so the pending entry is not erased), the newer value will be dropped and the old value applied when the element appears.
{
std::unique_lock lock{m_context.propertyMutex};
m_context.pendingStreamSyncMode.emplace(m_type, m_streamSyncMode);
}
ad87533 to
3f72fc1
Compare
3f72fc1 to
8517655
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
tests/unittests/media/server/gstplayer/genericPlayer/common/GenericTasksTestsBase.cpp:844
GenericTasksTestsBase::shouldSetupAudioDecoderElementWithPendingBufferingLimit()is still declared (and used bySetupElementTest), but its definition was removed here. This will fail to link; also the previous expectation thatSetupElementtriggerssetBufferingLimit()is no longer valid now that buffering-limit application moved toGstGenericPlayer::setupElement().
Reintroduce the helper with updated expectations (no setBufferingLimit() call expected from the SetupElement task).
void GenericTasksTestsBase::shouldSetupAudioDecoderElementWithIsLiveParameter()
{
testContext->m_context.isLive = true;
EXPECT_CALL(*testContext->m_glibWrapper, gTypeName(G_OBJECT_TYPE(testContext->m_element)))
.WillOnce(Return(kElementTypeName.c_str()));
8517655 to
4666108
Compare
|
Coverage statistics of your commit: |
b7a7101 to
c3d44f0
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved critical concurrency and test-expectation issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 10
Open (11)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
Resolved since last review (1)
c3d44f0 to
79405c5
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved moderate concurrency/API issues and a critical unit-test regression remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 10
Open (11)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
Resolved since last review (1)
| EXPECT_CALL(*testContext->m_gstWrapper, | ||
| gstElementFactoryListIsType(testContext->m_elementFactory, | ||
| GST_ELEMENT_FACTORY_TYPE_PARSER | GST_ELEMENT_FACTORY_TYPE_MEDIA_VIDEO)) | ||
| .WillOnce(Return(TRUE)); | ||
| .Times(2); |
79405c5 to
16fee68
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved synchronization races and failing or incomplete test expectations block approval.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 12
Open (13)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
| gstElementFactoryListIsType(testContext->m_elementFactory, | ||
| GST_ELEMENT_FACTORY_TYPE_DECODER | GST_ELEMENT_FACTORY_TYPE_MEDIA_AUDIO)) | ||
| .WillOnce(Return(FALSE)); | ||
| .Times(2); |
|
Coverage statistics of your commit: |
16fee68 to
e4389c8
Compare
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved synchronization issues and incomplete mock expectations can cause races and failing or ineffective tests.
Review effort: Lite
Findings: 12
Open (13)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
|
Coverage statistics of your commit: |
e4389c8 to
af57548
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved pending-state synchronization issues and failing or incomplete unit-test expectations remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 11
Open (12)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
Resolved since last review (2)
|
|
||
| // This is the extra EXPECT caused by setting pendingStreamSyncMode... | ||
| EXPECT_CALL(testContext->m_gstPlayer, setStreamSyncMode(MediaSourceType::VIDEO)); | ||
| expectSetupVideoParserElement(); |
|
Coverage statistics of your commit: |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved synchronization and test-expectation issues remain.
Review effort: Lite
Findings: 11
Open (12)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…setBufferingLimit()reads the pending value, releases…SetupElement::execute()still…pendingUseBuffering:…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…pendingSyncOffis protected bypropertyMutex, but the lock is released before…
|
Coverage statistics of your commit: |
| RIALTO_SERVER_LOG_INFO("Setting syncmode-streaming to 1 on video parser (immediate)"); | ||
| bool streamSyncModePending{false}; | ||
| { | ||
| std::unique_lock lock{m_context.propertyMutex}; |
| bool streamSyncModePending{false}; | ||
| bool bufferingLimitPending{false}; | ||
| { | ||
| std::unique_lock lock{m_context.propertyMutex}; |
| bool streamSyncModePending{false}; | ||
| bool bufferingLimitPending{false}; | ||
| { | ||
| std::unique_lock lock{m_context.propertyMutex}; |
| if (m_context.pendingStreamSyncMode.find(MediaSourceType::VIDEO) != m_context.pendingStreamSyncMode.end()) | ||
| bool streamSyncModePending{false}; | ||
| { | ||
| std::unique_lock lock{m_context.propertyMutex}; |
Summary: Fixing failed UT/valgrind tests
Type: Fix
Test Plan: UT/CT, Fullstack
Jira: RDKEMW-23040
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unresolved data-race concerns and failing or incomplete mock expectations remain.
Review effort: Lite
Findings: 7
Open (7)
SetupElementis constructed insideGstGenericPlayer::setupElementbefore the task is enqueued,…SetupElement::execute()still…pendingSyncOff,pendingStreamSyncMode, andpendingBufferingLimitare read in this…
|
Coverage statistics of your commit: |
RDKEMW-23040: Property applied before brcmvidfilter NULL→READY
Reason for change: It's a race between two independent processes, and a race has two possible outcomes each run. Property applied before brcmvidfilter NULL→READY → configured in time → PASS and property applied after brcmvidfilter NULL→READY → missed the window → FAIL
Test Procedure: https://ccp.sys.comcast.net/browse/RDKEMW-23040