Limit the source-completeness check to HTTP pulls - #496
Conversation
checkSourceComplete compares playback position against the duration the pipeline reports and fails the ingress when it falls short. #480 scoped that to HLS on the basis that a source with no duration is complete anyway, and listed RTMP and WHIP alongside live HLS as sources that report none. RTMP reports one. Live HLS gets its -1 from hlsdemux2 answering get_duration for a playlist with no end; a push input has no hlsdemux2 in it. On a live FLV chain the query is answered from the timestamps that have arrived, so it returns a positive value and the guard falls through to the comparison. What that comparison then measures is the queue depth at teardown, not missing source, and as a share of the whole it grows the shorter the session is. A live RTMP session was failed on a 1.2435s gap over 24.355s: 5.106% against a 5% tolerance, 26ms past it. The same gap on a five minute session is 0.4% and passes. So the exposure is short push sessions, where an ordinary publisher disconnect reads as truncation. Gate on the input type instead of on the duration query declining, and give the tolerance a floor so a short pull is not held to a few tens of milliseconds. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Devin Review found 1 potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| tolerance := max( | ||
| time.Duration(float64(duration)*durationTolerancePercent/100), | ||
| durationToleranceFloor, | ||
| ) |
There was a problem hiding this comment.
🟡 Short HLS truncations report success
For HLS assets under 20 seconds, durationToleranceFloor accepts missing content beyond the five-percent allowance. Assets at or below one second pass after any positive playback, so truncated ingresses finish successfully.
Learn more
The fixed one-second tolerance dominates the percentage for every source shorter than 20 seconds. It can classify a large relative shortfall as complete, and it effectively disables truncation detection for assets at or below one second once position is positive. Rework the short-asset tolerance so it accommodates normal final-segment timestamp variance without accepting an arbitrary one-second loss. Add a short HLS regression fixture where a real segment fetch fails and verify that the ingress remains an error.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
This is on purpose. 1 second is short enough that it shouldn't really be noticed.
|
it would be interesting to check srt input.. |
|
I think SRT source should return -1 to the duration query so we should be ok - but querying a live source doesn't make sense - maybe we should do: |
A URL pull is not necessarily a pull over HTTP. NewURLSource also builds srtclientsrc for srt:// and udpsrc for udp://, and both are live, so the input type alone leaves two live sources reaching the check. Use the prefix test NewURLSource selects the source element with, so the gate and the element selection cannot disagree about what HTTP is. Assuming the live schemes answer no duration would be the assumption that put RTMP here to begin with, so they are excluded by what they are rather than by what a query is expected to return. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lete-to-pull-inputs # Conflicts: # pkg/media/pipeline_test.go
staticcheck flagged the negated disjunction. De Morgan would split the prefix test into two negated halves, which stops it being the same expression NewURLSource picks the source element with, so name the positive condition and negate that instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Follow-up to #480, which added
checkSourceComplete.The problem
checkSourceCompletecompares playback position against the duration the pipeline reports and fails the ingress when it falls short. #480 scoped this to HLS, and reasoned that everything else is safe because a source with no duration is complete anyway:Live HLS does report none. That
-1comes fromhlsdemux2answeringget_durationfor a playlist with no declared end. A push input has nohlsdemux2in it — RTMP isappsrc→flvdemux→decodebin3— and on that chain the duration query is answered from the timestamps that have arrived. It returns a positive value, thed <= 0guard falls through, and the comparison runs on a source that has nothing to fall short of.What the comparison measures there is the queue depth at teardown. That is roughly constant, so as a share of the duration it grows the shorter the session is:
An observed live RTMP session was failed on
source ended after 23.1115s of 24.355s— a 1.2435s gap, 5.106%, 26ms past the tolerance. Break-even is around 25s, so the exposure is short push sessions, where an ordinary publisher disconnect is reported as a truncated source. Short sessions skew towards people testing an encoder setup for the first time, which is a poor audience for a spuriousENDPOINT_ERROR.The fix
Exclude everything that is not a pull over HTTP:
The input type alone is not enough, which came out of review: a URL pull is not necessarily a pull over HTTP.
NewURLSourcealso buildssrtclientsrcforsrt://andudpsrcforudp://, and both are live. They arrive asURL_INPUTand would still reach the comparison.The prefix test is deliberately the same expression
NewURLSourceselects the source element with, and thatsourceLatencyReductionEnabledalready uses, so the gate and the element selection cannot drift apart over what counts as HTTP. Aurl.Parseversion would be tidier and could disagree with it.Live sources are excluded by what they are rather than by the duration query being expected to decline for them. SRT probably does answer
-1— but "this live source reports no duration" is the assumption that put RTMP here, and it is not worth relying on a second time for a source that carries MPEG-TS timestamps over a live link.The
d <= 0guard stays. It still carries live HLS, which reaches it as an HTTP pull.Tolerance floor
Second, smaller thing, and separable if you would rather it were: under roughly 20s the percentage collapses to a few tens of milliseconds, which is narrower than the overshoot #480 describes on a final segment. A 3s asset is currently held to 150ms. The tolerance now takes the greater of the percentage and a one second floor.
The floor is a guess, and commented as one. Open question 2 on #480 said the same of the 5%; neither has production data behind it yet. One second means a 3s asset tolerates a 33% shortfall, so if catching truncation on very short assets matters, this wants to be smaller.
Tests
TestPushInputWithADurationIsComplete— the reported case, over RTMP and WHIP.TestLiveURLSchemesAreComplete— the same forsrt://andudp://underURL_INPUT.TestShortSourceIsHeldToTheToleranceFloor— asserts the shortfall clears the percentage and sits under the floor before asserting the verdict, so it tests the floor rather than the percentage.The first two assert the fixture answers a positive duration, so neither can pass by the fixture happening to answer none. All three are mutation tested: removing the input-type gate fails the first, removing the scheme clause fails the second, removing the floor fails the third.
TestSourceWithoutDurationIsCompleteis renamedTestLivePullWithoutDurationIsComplete. It buildsaudiotestsrc is-live=true, which is the live HLS shape, and its comment claimed it covered RTMP and WHIP. It never did, and that is roughly how this got through.The fixtures now carry a
params.Paramsdeclaring the input type and url they model. They previously carried neither, which is why the gate needed them touched.Verification
go build ./...: cleango vet ./pkg/media/,gofmt: cleango test -race ./pkg/media/: all pass except the two belowTestTruncatedHLSPullIsAnErrorandTestCompleteHLSPullIsCompletefail locally, both at therequire.Equal(gst.MessageEOS, msgType)assertion that precedes any of this code — the pull raises a bus error instead of an EOS. They fail identically on unmodifiedmainon this machine, so this is environmental rather than a regression here: local is Homebrew GStreamer 1.28.6, and #480 recorded its green full-suite run on 1.26.7, the version CI pins. Worth a look on its own, but please treat CI as the arbiter for these two.Not verified against a real FLV, SRT or UDP chain in a test. The new gate tests use the seekable wav fixture, because what they pin is that the gate turns the check away before it reads any query. The 24.355s figure above is from an observed session, not from a test.
🤖 Generated with Claude Code