Fix crash when the live default position moves past a playing SSAI ad - #3374
Open
oguzhaneksi wants to merge 2 commits into
Open
Fix crash when the live default position moves past a playing SSAI ad#3374oguzhaneksi wants to merge 2 commits into
oguzhaneksi wants to merge 2 commits into
Conversation
Collaborator
|
Thanks for the investigation and providing a fix! It looks correct to me and I'll start the internal merge process. |
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.
Fixes #3348.
Problem
Joining a live SSAI stream (e.g. AWS MediaTailor) while an ad break is on air, and
then receiving a manifest refresh that moves the window default position past that
ad, crashes the playback thread:
Root cause
resolvePositionForPlaylistChangepasses the period looked up bynewPeriodUidtoisIgnorableServerSideAdInsertionPeriodChange, assuming it is the period thatperiodIdWithAdsrefers to. That assumption doesn't hold:MediaPeriodQueue.resolveMediaPeriodIdForAdsAfterPeriodPositionChangerolls back to apreceding unplayed server-side inserted ad period, so the returned
MediaPeriodIdcanbelong to a different period than
newPeriodUid.For a live window
|- p0 content -|- p1 ad -|- p2 content -|where playback joinedinside the ad in p1 and the refreshed default position lands in p2:
newPeriodUid→ p2, which has an emptyAdPlaybackStateperiodIdWithAds→ p1,adGroupIndex = 0The uid equality guard inside
isIgnorableServerSideAdInsertionPeriodChangecomparesoldPeriodId.periodUidwithnewPeriodId.periodUid— both p1, so it passes — and thefollowing line then indexes p2's empty
AdPlaybackStatewith p1's ad group index,throwing
ArrayIndexOutOfBoundsException.This only reproduces when the refreshed default position lands in a period after the
ad. If it lands before the ad (p0), the uid guard returns early and the faulty line is
never reached, which is why the crash looks position-dependent.
Fix
Look the period up by the resolved period uid (
periodIdWithAds.periodUid) instead ofnewPeriodUid. Past the uid guard,oldPeriodIdandnewPeriodIdshare a period uidby construction, so the resolved uid is the only correct one to use here.
With the fix, the change is correctly detected as an in-stream ad change, the update is
dropped and the ad keeps playing across the refresh — no renderer reset.
Testing
Added
ExoPlayerAdTest.timelineRefresh_movingLiveDefaultPositionPastPlayingSsaiAd_keepsPlayingAd,which fails with the
ArrayIndexOutOfBoundsExceptionabove without the fix and passeswith it.
Ran on
:lib-exoplayer:ExoPlayerAdTest(64),ExoPlayerTest(2180),MediaPeriodQueueTest(58),AdsMediaSourceTest(27),ServerSideAdInsertionMediaSourceTest(32),ServerSideAdInsertionUtilTest(8) —2369 tests, 0 failures.