VPAAMP-193 - iframe tracks can be selected as rampdown target in legacy and new abr#1348
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes ABR rampdown selection so iframe-only tracks aren’t considered as rampdown targets (preventing invalid bandwidths being passed to GetProfileIndexForBandwidth), and adds unit coverage for related ABR math edge-cases.
Changes:
- Filter iframe tracks out of
FragmentfailureRampdown()candidate profiles in bothabr/abr.cppandsupport/aampabr/HybridABRManager.cpp. - Fix integer-truncation in Hybrid ABR throughput calculations and add focused unit tests.
- Register
HybridAbrTestswith the unit test runner in CMake.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
abr/abr.cpp |
Excludes iframe tracks from FragmentfailureRampdown() candidate list. |
support/aampabr/HybridABRManager.cpp |
Fixes throughput arithmetic and filters iframe tracks during fragment-failure rampdown. |
test/utests/tests/AampAbrTests/AbrTests.cpp |
Adds tests verifying ABRManager::FragmentfailureRampdown() skips iframe tracks and uses correct fallback. |
test/utests/tests/AampAbrTests/HybridAbrTests.cpp |
Adds tests for Hybrid ABR truncation fixes and iframe-skip behavior (plus local SpeedCache for testing). |
test/utests/tests/AampAbrTests/CMakeLists.txt |
Adds aamp_utest_run_add(HybridAbrTests) so the suite runs in CI. |
Comments suppressed due to low confidence (1)
support/aampabr/HybridABRManager.cpp:458
- After filtering out iframe tracks,
availableProfilescan become empty. The current implementation still computeslen = size() - 1and later indexesavailableProfiles[len]/availableProfiles[0], which will go out-of-bounds when all profiles are iframe (or no profiles exist). Add an early return whenavailableProfiles.empty()(e.g., return 0 likeABRManager::FragmentfailureRampdown()does) before calculatinglen/ sorting.
std::vector<ProfileInfo> availableProfiles = getProfileInfo();
availableProfiles.erase(
std::remove_if(availableProfiles.begin(), availableProfiles.end(),
[](const ProfileInfo &p) { return p.isIframeTrack; }),
availableProfiles.end());
int len = (int)availableProfiles.size() - 1;
std::sort(availableProfiles.begin(), availableProfiles.end(), [](const ProfileInfo& a, const ProfileInfo& b) {
return a.bandwidthBitsPerSecond < b.bandwidthBitsPerSecond;
});
// Iterate over profiles in descending order of bandwidth
for (int i = (int)availableProfiles.size() -1 ;i >= 0 ; i--)
{
double profilePercentage = ((double)(availableProfiles[i].bandwidthBitsPerSecond) / availableProfiles[len].bandwidthBitsPerSecond) * 100.0;
AAMPABRLOG_WARN("Index: %d, bandwidth %d , profile percentage %lf, buffer percentage %lf",i,(int)availableProfiles[i].bandwidthBitsPerSecond,profilePercentage,bufferPercentage);
// Check if profile bandwidth percentage is less than buffer percentage ,and it should be a rampdown
if (profilePercentage < bufferPercentage && (availableProfiles[i].bandwidthBitsPerSecond < currentbw))
{
desiredProfilebw = availableProfiles[i].bandwidthBitsPerSecond;
break;
}
}
if (desiredProfilebw == 0)
{
// If no profile found, then return the lowest profile. Usually happens when bufferPercentage is very low or already at lowest profile
desiredProfilebw = availableProfiles[0].bandwidthBitsPerSecond;
}
Issue 1 — HybridABRManager.cpp: CheckAbrThresholdSize now computes (bufferlen * 8000L) / downloadTimeMs instead of (bufferlen / downloadTimeMs) * 8000, preventing truncation to zero for small fragments. This was already fixed in abr.cpp Issue 2 — Same file: CheckLLDashABRSpeedStoreSize now computes (total_dl_diff * 8000L) / time_diff instead of (total_dl_diff / time_diff) * 8000. This was already fixed in abr.cpp Tests — HybridAbrTests.cpp: 4 new tests covering small-fragment and large-fragment cases for both functions.
Filter iframe tracks from availableProfiles in FragmentfailureRampdown() in both ABRManager (abr/abr.cpp) and HybridABRManager using erase-remove, matching the pattern used by every other ABR function. This prevents an iframe track bandwidth from being selected as a rampdown target or as the fallback lowest profile. Add 6 L1 tests covering iframe filtering, normal rampdown, and fallback behavior for both ABR implementations. Fixes: abr-bugs.md #11
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
dd5a24f to
cbf7a18
Compare
…rTests SpeedCache was re-introduced during rebase. HybridABRManager.h already provides it (via AampSpeedCache.h or its inline fallback), so no local definition is needed in the test file.
The multi-line reformat of downloadbps was introduced by an unrelated commit (247f93b) pulled in during rebase. It does not belong in this PR.
DomSyna
pushed a commit
that referenced
this pull request
Apr 29, 2026
…cy and new abr (#1348) Reason for Change: Fix FragmentfailureRampdown to exclude iframe tracks from selection Filter iframe tracks from availableProfiles in FragmentfailureRampdown() in both ABRManager (abr/abr.cpp) and HybridABRManager using erase-remove, matching the pattern used by every other ABR function. This prevents an iframe track bandwidth from being selected as a rampdown target or as the fallback lowest profile. Add 6 L1 tests covering iframe filtering, normal rampdown, and fallback behavior for both ABR implementations. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
Abhi-jith-S
pushed a commit
that referenced
this pull request
May 12, 2026
…cy and new abr (#1348) Reason for Change: Fix FragmentfailureRampdown to exclude iframe tracks from selection Filter iframe tracks from availableProfiles in FragmentfailureRampdown() in both ABRManager (abr/abr.cpp) and HybridABRManager using erase-remove, matching the pattern used by every other ABR function. This prevents an iframe track bandwidth from being selected as a rampdown target or as the fallback lowest profile. Add 6 L1 tests covering iframe filtering, normal rampdown, and fallback behavior for both ABR implementations. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
varatharajan568
pushed a commit
that referenced
this pull request
May 20, 2026
…cy and new abr (#1348) Reason for Change: Fix FragmentfailureRampdown to exclude iframe tracks from selection Filter iframe tracks from availableProfiles in FragmentfailureRampdown() in both ABRManager (abr/abr.cpp) and HybridABRManager using erase-remove, matching the pattern used by every other ABR function. This prevents an iframe track bandwidth from being selected as a rampdown target or as the fallback lowest profile. Add 6 L1 tests covering iframe filtering, normal rampdown, and fallback behavior for both ABR implementations. --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Philip Stroffolino <philip_stroffolino@cable.comcast.com>
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.
FragmentfailureRampdown in abr.cpp and HybridABRManager.cpp don't filter out iframe tracks (unlike other abr-related methods). An iframe track bandwidth could be selected as the desired rampdown target, which doesn't correspond to any regular video profile. The caller then passes this bandwidth to GetProfileIndexForBandwidth which may not find a match. I think this currently works by accident for most Comcast live manifests because the iframe track bandwidths are the same as the track from which the iframes are derived.