-
Notifications
You must be signed in to change notification settings - Fork 6
VPLAY-11886: Rate correction is enabled during RW on LLD #724
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev_sprint_25_2
Are you sure you want to change the base?
Conversation
Reason for change: Modified the LLD LatencyMonitor condition to disable rate change during rewind as well. Test Procedure: Refer Jira Risks: Low Signed-off-by: psiva01 <[email protected]>
143be4b to
a62fb8b
Compare
Signed-off-by: psiva01 <[email protected]>
33d595e to
d1f3637
Compare
| notifyEnteringLive = true; | ||
| } | ||
| else if (((eTUNETYPE_SEEK == tuneType) || (eTUNETYPE_RETUNE == tuneType || eTUNETYPE_NEW_SEEK == tuneType)) && (currentRate > AAMP_RATE_PAUSE)) | ||
| else if (((eTUNETYPE_SEEK == tuneType) || (eTUNETYPE_RETUNE == tuneType || eTUNETYPE_NEW_SEEK == tuneType)) && (currentRate != AAMP_RATE_PAUSE)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the logic used aamp->rate, but after the localTSB integration, it shifted to currentRate which sets PAUSE to stream abstraction object if pipelinePaused, but in actual PAUSE, aamp->rate remains AAMP_NORMAL_RATE and the pipelinePaused flag is set. Is it acceptable to keep using aamp->rate > AAMP_RATE_PAUSE? With the recent fix, the scope of this else if now also includes rewind scenarios—correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nu641001 ,
currentRate is a local variable set to aamp->rate in line 3198
Before merging https://github.com/rdkcentral/aamp/pull/649/files#diff-9412049d3685e68289ccd0b5e0a718c64d26b9a783882ba14700abd57e136be3, this function was using mPlayRate (which is always 1 when using AAMP TSB, see VPLAY-11583). Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes @jfagunde Now I see where the confusion came from. The rate in StreamAbstraction essentially mirrors the value in aamp->rate. Since aamp->rate never transitions to a PAUSED state, the check for rate != PAUSE doesn’t really apply here. We can safely remove that condition to avoid any future ambiguity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR modifies the condition for entering Low Latency DASH (LLD) latency monitoring logic during seek operations to include rewind scenarios. The change extends the rate check from currentRate > AAMP_RATE_PAUSE (forward playback only) to currentRate != AAMP_RATE_PAUSE (both forward and rewind), ensuring that LLD speed adjustment is properly disabled during rewind operations on live streams.
Key Change:
- Modified the rate condition check in
StreamAbstractionAAMP_MPD::Init()to include negative rates (rewind) in addition to positive rates (forward playback), while still excluding paused state.
Reason for change: Modified the LLD LatencyMonitor condition to disable rate change during rewind as well.
Test Procedure: Refer Jira
Risks: Low