Fix updateProfile() 4K iframe selection bugs#1369
Open
benmesander wants to merge 2 commits intodev_sprint_25_2from
Open
Fix updateProfile() 4K iframe selection bugs#1369benmesander wants to merge 2 commits intodev_sprint_25_2from
benmesander wants to merge 2 commits intodev_sprint_25_2from
Conversation
Issue: The 4K iframe selection path used `!mDesiredIframeProfile` to check whether a matching iframe was found. Since profile index 0 is a valid index, `!0` evaluates to true, causing the fallback to overwrite a correct match at index 0. Replace with an explicit `bool foundMatchingIframe` flag. Issue: The middle video bandwidth was computed as `mProfiles[profileCount / 2]`, which includes iframe tracks in the count and can land on an iframe profile. Use mSortedBWProfileList (which excludes iframes) to find the middle video bandwidth instead. Issue: fix an out-of-bounds access in the fallback path: when iframeTrackCount == 1, the formula `count/2 + count%2` yields index 1, which is past the end of a 1-element vector. Initialize mDesiredIframeProfile to iframeTrackInfo[0].idx (lowest-BW iframe) and guard the fallback with `> 1` instead of `>= 1`. Applied to both new ABR (abr/abr.cpp) and legacy ABR (support/aampabr/ABRManager.cpp). L1 tests added to AbrTests.cpp and HybridAbrTests.cpp covering all three bugs. - GetBestMatchedProfile_ExactMatch_UnsortedProfiles (legacy) - GetBestMatchedProfile_ClosestMatch_UnsortedProfiles (legacy)
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes multiple bugs in the 4K iframe selection logic inside ABRManager::updateProfile() for both the new ABR and legacy ABR implementations, ensuring correct matching behavior for iframe profile index 0, correct “middle video bandwidth” computation, and safer fallback selection.
Changes:
- Replace the
!mDesiredIframeProfile“not found” check with an explicitfoundMatchingIframeflag to avoid treating index0as false. - Compute the middle video bandwidth using
mSortedBWProfileList(non-iframe only) instead of indexing intomProfiles(which may include iframes). - Add unit tests covering the 4K iframe middle-bandwidth selection and index-0 matching behaviors for both ABR paths.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
abr/abr.cpp |
Fixes new-ABR 4K iframe selection by using non-iframe BW list and explicit match tracking; adjusts fallback guard. |
support/aampabr/ABRManager.cpp |
Applies the same 4K iframe selection fixes to legacy ABR logic. |
test/utests/tests/AampAbrTests/AbrTests.cpp |
Adds L1 tests validating the new-ABR 4K iframe middle-BW selection and index-0 handling. |
test/utests/tests/AampAbrTests/HybridAbrTests.cpp |
Adds equivalent tests for the legacy/hybrid ABR implementation. |
Comment on lines
+543
to
+548
| /** | ||
| * @brief Bug #13: updateProfile 4K path must compute middle video bandwidth | ||
| * excluding iframe tracks. With interleaved profiles, profileCount/2 | ||
| * could land on an iframe track. | ||
| */ | ||
| TEST_F(AbrTests, UpdateProfile_4K_MiddleIndex_ExcludesIframeTracks) |
Comment on lines
+548
to
+552
| TEST_F(AbrTests, UpdateProfile_4K_MiddleIndex_ExcludesIframeTracks) | ||
| { | ||
| ABRManager abrManager; | ||
| abrManager.ReadPlayerConfig(&eAAMPAbrConfig); | ||
|
|
Comment on lines
+276
to
+284
| /** | ||
| * @brief Bug #13: updateProfile 4K path must compute middle video bandwidth | ||
| * excluding iframe tracks (legacy ABR). | ||
| */ | ||
| TEST_F(HybridAbrTests, UpdateProfile_4K_MiddleIndex_ExcludesIframeTracks) | ||
| { | ||
| HybridABRManager mgr; | ||
| mgr.ReadPlayerConfig(&eAAMPAbrConfig); | ||
|
|
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.
Issue: The 4K iframe selection path used
!mDesiredIframeProfileto check whether a matching iframe was found. Since profile index 0 is a valid index,!0evaluates to true, causing the fallback to overwrite a correct match at index 0. Replace with an explicitbool foundMatchingIframeflag.Issue: The middle video bandwidth was computed as
mProfiles[profileCount / 2], which includes iframe tracks in the count and can land on an iframe profile. Use mSortedBWProfileList (which excludes iframes) to find the middle video bandwidth instead.Issue: fix an out-of-bounds access in the fallback path: when iframeTrackCount == 1, the formula
count/2 + count%2yields index 1, which is past the end of a 1-element vector. Initialize mDesiredIframeProfile to iframeTrackInfo[0].idx (lowest-BW iframe) and guard the fallback with> 1instead of>= 1.Applied to both new ABR (abr/abr.cpp) and legacy ABR (support/aampabr/ABRManager.cpp). L1 tests added to AbrTests.cpp and HybridAbrTests.cpp covering all three bugs.