VPAAMP-28: Merge MonitorLatency and RateCorrectionWorkerThread#1332
Conversation
1961af0 to
c852004
Compare
There was a problem hiding this comment.
Pull request overview
This PR (VPAAMP-28) consolidates the legacy live-latency “rate correction worker thread” logic into the unified AampLatencyMonitor flow, updating both production code paths and related unit tests to use EnableLatencyMonitor() / IsLatencyMonitorEnabled() rather than direct mDisableRateCorrection state.
Changes:
- Remove the dedicated rate-correction thread/members and route enable/disable through
AampLatencyMonitor. - Update stream abstractions (HLS/DASH/core) and playback control paths to toggle latency monitoring instead of mutating
mDisableRateCorrection. - Adjust unit tests and fakes to reflect the new latency-monitor interface.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utests/tests/PrivateInstanceAAMP/PauseOnPlaybackTests.cpp | Updates tests to use EnableLatencyMonitor() / IsLatencyMonitorEnabled() expectations. |
| test/utests/tests/PrivAampTests/PrivAampTests.cpp | Removes worker-thread tests and updates tune-related assertions to latency-monitor API. |
| test/utests/fakes/FakePrivateInstanceAAMP.cpp | Removes legacy rate-correction stubs and introduces latency-monitor API stubs (currently incomplete). |
| streamabstraction.cpp | Switches re-enable points from mDisableRateCorrection=false to EnableLatencyMonitor(true). |
| priv_aamp.h | Removes legacy worker-thread members/APIs and adds IsLatencyMonitorEnabled(). |
| priv_aamp.cpp | Removes worker-thread implementation and routes enable/disable via AampLatencyMonitor. |
| main_aamp.cpp | Replaces direct member toggling with EnableLatencyMonitor(false) on pause-related flow. |
| fragmentcollector_mpd.cpp | Uses EnableLatencyMonitor(true/false) when entering live / refreshing tracks. |
| fragmentcollector_hls.cpp | Uses EnableLatencyMonitor(true/false) when entering live / refreshing tracks. |
| aampgstplayer.cpp | Changes flush behavior from syncing correction-rate variables to disabling latency monitor. |
| AampLatencyMonitor.h | Adds IsRateCorrectionEnabled() accessor for rate-correction enabled state. |
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12714
- In SetPreferredLanguages(), the new condition uses !mLatencyMonitor->IsRunning() to choose between TuneHelper(eTUNETYPE_SEEK) and TuneHelper(eTUNETYPE_SEEKTOLIVE). Previously this decision was based on mDisableRateCorrection (i.e., whether rate correction was disabled due to user pause/seek). The monitor thread can remain running even when rate correction is disabled, so IsRunning() is not equivalent and may cause unintended seeks to live. Use the rate-correction enabled state instead (e.g., IsLatencyMonitorEnabled()/IsRateCorrectionEnabled()) if the goal is to preserve the prior behavior.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
76acdd3 to
7fbbd1c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12712
- This retune path used to branch based on whether rate correction was disabled (
mDisableRateCorrection). The new check uses!mLatencyMonitor->IsRunning(), which is not equivalent: the monitor can be running while correction is disabled via EnableLatencyMonitor(false). In that case this will incorrectly choose SEEKTOLIVE instead of SEEK. Use the rate-correction enabled flag (e.g., IsLatencyMonitorEnabled() / mLatencyMonitor->IsRateCorrectionEnabled()) for this decision, not the thread running state.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
e6239c1 to
332ba2f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
priv_aamp.cpp:12720
- This TuneHelper selection now uses
!mLatencyMonitor->IsRunning()to decide betweeneTUNETYPE_SEEKvseTUNETYPE_SEEKTOLIVE. That is not equivalent to the previousmDisableRateCorrectioncheck: the latency monitor can remain running while rate correction is temporarily disabled (e.g., during seek/pause), and the player may also not be at the live point. With the current condition, a language/track retune can incorrectly force a seek-to-live when the user is behind live but the monitor thread is still running. Use the rate-correction enabled flag (e.g.,IsLatencyMonitorEnabled()/mLatencyMonitor->IsRateCorrectionEnabled()) and/orIsAtLivePoint()to preserve the prior behavior.
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
332ba2f to
b00acc1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
test/utests/tests/AampLatencyMonitorTests/AampLatencyMonitorTestCases.cpp:776
- There are a few trailing blank lines at the end of the file after the final test case; please remove the extra empty lines to keep the test file tidy and avoid noise in future diffs.
b00acc1 to
6a5d964
Compare
6a5d964 to
00ffe64
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
priv_aamp.cpp:12792
- In SetPreferredLanguages(), the branch deciding between TuneHelper(eTUNETYPE_SEEK) vs TuneHelper(eTUNETYPE_SEEKTOLIVE) now checks
!mLatencyMonitor->IsRunning(). This is not equivalent to the priormDisableRateCorrectionbehavior: the latency monitor can be running while rate correction is intentionally disabled (EnableLatencyMonitor(false) makes the worker wait), which would incorrectly force the SEEKTOLIVE path. Consider using the new rate-correction flag (e.g., IsLatencyMonitorEnabled()/mLatencyMonitor->IsRateCorrectionEnabled()) or an explicit “user is at live” condition instead of IsRunning().
else if(!mLatencyMonitor->IsRunning())
{
TuneHelper(eTUNETYPE_SEEK);
}
else
{
TuneHelper(eTUNETYPE_SEEKTOLIVE);
}
priv_aamp.cpp:5486
- TeardownStream() no longer disables/suppresses latency rate correction before stopping/deleting the StreamAbstraction and interacting with the StreamSink. Since AampLatencyMonitor runs on its own thread and can call SetPlayBackRate() concurrently, leaving it enabled during teardown risks racing with sink/stream shutdown (and can apply correction rates during teardown/retune). Consider disabling (EnableLatencyMonitor(false)) or stopping the latency monitor at the start of TeardownStream(), and re-enabling/restarting it after the new tune reaches a stable live/playing state.
//reset discontinuity related flags
ResetDiscontinuityInTracks();
UnblockWaitForDiscontinuityProcessToComplete();
ResetTrackDiscontinuityIgnoredStatus();
lock.unlock();
{
// Using StreamLock to make sure this is not interfering with GetFile() from PreCachePlaylistDownloadTask
std::lock_guard<std::recursive_mutex> streamLock(mStreamLock);
if (mpStreamAbstractionAAMP)
00ffe64 to
11004bc
Compare
0597a3c to
12cebd6
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
aampgstplayer.cpp:1072
- Flush() still resets the pipeline state, but the previous correction-rate reset was removed without synchronizing the new AampLatencyMonitor state. After a flush, the monitor may keep a stale non-normal mCurrentRate and therefore fail to reapply rate correction when latency still requires it; please sync the monitor’s current rate when the flush succeeds.
bool ret = playerInstance->Flush(position, rate, shouldTearDown, isAppSeek);
if(ret)
{
for (int i = 0; i < AAMP_TRACK_COUNT; i++)
{
//reset buffer control states prior to gstreamer flush so that the first needs_data event is caught
privateContext->mBufferControl[i].flush();
}
f74afbf to
0ebb125
Compare
65c91ed to
42b629f
Compare
0930971 to
71c6166
Compare
Reason for change: Merged the new flow Test Procedure: Refer jira ticket VPAAMP-28 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>
17b32a2 to
ec0297a
Compare
Reason for change: Merged the new flow Test Procedure: Refer jira ticket VPAAMP-28 Priority: P2 Signed-off-by: srikanthreddybijjam-comcast <srikanthreddybijjam.2000@gmail.com>
ec0297a to
142867e
Compare
Reason for change: Merged the new flow
Test Procedure: Refer jira ticket VPAAMP-28
Priority: P2