Do not report clipping preroll for clips that only set an end position - #3398
Open
oguzhaneksi wants to merge 1 commit into
Open
oguzhaneksi wants to merge 1 commit into
oguzhaneksi wants to merge 1 commit into
Conversation
Collaborator
|
Thanks for the fix! I'll work on merging this internally, but I'll fix #3397 first. |
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 #3371.
Problem
Playing a VoD playlist with HLS interstitials produces the period structure
A a B b C c D. Every return from an interstitial into content disables andre-enables the renderers — a visible hesitation at each ad break, even when the
content resumes exactly on a segment boundary and there is nothing to skip.
Root cause
AdsMediaSource.createPeriod()wraps a content period that has a following adgroup in a
ClippingMediaPeriodwithenableInitialDiscontinuity = true,startUs = 0and an end clip.shouldKeepInitialDiscontinuitythen applies aheuristic: resumed at a non-zero position, and not all-sync-samples ⇒ assume
preroll. For H.264 that is always true, so every clipped content period claims
preroll.
The heuristic itself is a reasonable fallback. The problem is where its answer
went.
ClippingSampleStream.getFlags()wrote it onto theSampleStream.getFlags()protocol:So a clip overwrote whatever its child honestly reported and presented a guess
to every consumer of the protocol — including a child that had measured the
answer and reported no preroll.
The change
getFlags()addsFLAG_HAS_PREROLLonly when the clipping itself introducespreroll:
With a non-zero clipping start the wrapped period genuinely cannot know about
the preroll, because sample streams provide buffers from a key-frame that may
precede the clip start. With a zero clipping start the samples delivered are
exactly the samples the wrapped period delivers, so its own report is exact and
is now passed through.
shouldKeepInitialDiscontinuity,hasPreroll,anyTrackHasPrerollandpendingInitialDiscontinuityPositionUsare untouched, soreadDiscontinuity()behaves exactly as before.
The
startUs != 0restriction is deliberate. Removing the discontinuity fornon-zero clip starts was tried and broke 88
ClippingPlaylistPlaybackTest/MergingPlaylistPlaybackTestcases by feeding pre-clip-start buffers into thecodec.
The default configuration is unaffected
The two channels are never both consulted:
MediaPeriod.readDiscontinuity()FLAG_PER_STREAM_MEDIA_PROGRESSION)SampleStream.getFlags()The preroll bits have exactly one consumer,
ExoPlayerImplInternal.updateRendererForTransition, reachable only viamaybeUpdateReadingRenderersPerStream(), whichupdatePeriods()calls only whenperStreamMediaProgressionEnabled. Every othergetFlags()call site(
MediaCodecRenderer,DecoderVideoRenderer,ServerSideAdInsertionMediaSource)reads only
FLAG_STRICT_DURATION.DefaultAnalyticsCollectorTest.adPlaybackpasses unedited, which is theregression proof for the default path.
Tests
ExoPlayerAdTestgains two end-to-end reproductions and is parameterised on theflag, following the existing pattern in
ExoPlayerTest:playAds_returningFromSegmentAlignedMidRoll_keepsRenderersEnabled— mid-rollson segment boundaries, content resumes at a sync sample. Goes from
[2 enables, 0 preroll rendered]to[1, 0]with the gate open. This is Returning from an interstitial playlist causes renderer disable and then reenable #3371.playAds_returningFromMidSegmentMidRoll_skipsPrerollOfClippedPeriodsOnly—mid-rolls mid-segment, content really does carry preroll. Unchanged by this
patch, and pinned so that any future change to it is deliberate.
Their
FakeSampleStreamreports preroll honestly(
hasPreroll = segmentStartUs < preparePositionUs), so the segment-aligned casereports
0and the mid-segment case reportsFLAG_HAS_PREROLL. That isolatesexactly the fabrication this patch removes: the second test's numbers do not move.
ClippingMediaPeriodTest: threestartUs == 0assertions lose the fabricatedflag, and one test is added for the newly reachable behaviour — a child reporting
FLAG_MAYBE_HAS_PREROLLunder an end-only clip now has it passed through ratherthan replaced by
FLAG_HAS_PREROLL. That distinction matters: atExoPlayerImplInternalMAYBEdefers the transition until the child resolves,where
HAS_PREROLLforces a renderer reset.Full run over
:lib-exoplayer,:lib-exoplayer-dash,:lib-exoplayer-hls,:lib-exoplayer-imaand:lib-exoplayer-rtsp: 11 753 cases, 0 failures. Againsta baseline recorded on unmodified
main, the only differences are inExoPlayerAdTestitself. No golden dump file changes.Device verification
Demo app, emulator,
bipbop-with-interstitials-boundary.m3u8(3 midrolls withX-SNAP="IN", resuming on a segment boundary). Same build, the flag is the onlyvariable, counted from
EventLoggerover a full ~170 s playback:Negative control,
bipbop-with-interstitials-unsnapped.m3u8(midrollsmid-segment, content genuinely carries preroll), gate open: 4 enables / 3
disables — all three ad→content transitions still reset, as they must. This patch
does not suppress a real discontinuity.
Known gap
ProgressiveMediaPeriod.getFlags()reports only the staticFormat.hasPrerollSamplesand never seek-induced preroll, so once the gate isenabled,
AdsMediaSourceover progressive content has no source reporting thepreroll this patch stops fabricating. Filed separately as #3397, with a measured
reproduction, and flagged with a TODO at the changed line. It is not reachable
today, since the flag is off by default.
Two things noticed nearby, not changed here
Both are pre-existing and out of scope for this patch; happy to split either into
its own change if useful.
shouldKeepInitialDiscontinuitycallsMimeTypes.allSamplesAreSyncSamples(mimeType, codecs)for any track type,while the equivalent reasoning in
SampleQueue.canDiscardAllSamplesToStartTimepairs it with
trackType == C.TRACK_TYPE_AUDIO, on the grounds that theoptimization is only safe where a sample's inherent duration is negligible.
ClippingMediaPeriod.readDiscontinuity()calls itself where the comment says"read an initial discontinuity from the child". It terminates only because the
lines above it have already cleared
pendingInitialDiscontinuityPositionUs,so the re-entry takes the other branch. It works, but it reads as a bug.