Skip to content

Skip a pending video handover once the egress is ending - #1465

Merged
pksgit merged 2 commits into
mainfrom
pradeepsharma/cs-2150-skip-pending-switch-after-eos
Oct 21, 2026
Merged

pksgit merged 2 commits into
mainfrom
pradeepsharma/cs-2150-skip-pending-switch-after-eos

Conversation

@pksgit

@pksgit pksgit commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

When a track is pending (subscribed, or back after going inactive, and still waiting for its first frame), #1418 keeps the filler as the selector's active pad until that frame reaches the track's gate. If the egress ends inside that window, SendEOS pushes EOS into the filler, and because the filler is the active pad, input-selector forwards it into video_encoder_queue straight away. The track's first frame then takes the selector inside its gate, after that EOS. The handlers that switch pads stop once the pipeline is ending; the gate doesn't check.

Two failures follow from a handover that lands behind that EOS:

  • Stream error. A sticky event from the track, such as a tag update from h264parse, has to be forwarded through a selector whose output has already had EOS. With the 1.24 input-selector that forward fails, the frame push returns GST_FLOW_ERROR, and the egress fails with Internal data stream error. The 1.28 selector sends the new pad's stream-start first, which clears the EOS, so it doesn't hit this one.
  • A freed query in video_encoder_queue. The handover sends RECONFIGURE upstream and the track's converter answers with an ALLOCATION query. When the gate drops the frame it switched on (its PTS is behind the last one sent), that query can reach the queue before any STREAM_START. If it arrives while the EOS is still draining, gst_queue keeps it after the sender gives up on EOS and frees it, and a later STREAM_START restarts the queue onto the freed pointer: Unexpected item ... dequeued from queue video_encoder_queue (refcounting problem?), followed by a stream error. This queue behaviour is the same in GStreamer 1.24 and 1.28.

Change

VideoBin sets an EOS function, markEnding, on its bin. Bin.sendEOS runs it before pushing EOS into the filler, and it sets ending under b.mu. While ending is set, a track's gate doesn't take a pending handover and drops frames from any track that isn't selected, so an unselected pad doesn't feed a selector that is finishing.

The gate already takes b.mu on every frame, so this adds no lock. Because the EOS function and the gate both hold b.mu, a handover either completes before the filler is sent EOS, in which case the filler is no longer active and the selector holds its EOS as it did before #1418, or it is not taken.

Before After
Track pending when the egress ends Filler's EOS reaches the encoder early and the handover lands behind it; can fail as above Filler's EOS still goes out; no handover; the egress completes and the recording ends on the filler
Handover taken before SendEOS As in #1418 Same
Non-live pipelines Switch immediately Same

Evidence

A deployment running a build with #1418 logged Unexpected item ... dequeued from queue video_encoder_queue (refcounting problem?) five times, all after that build went out. Builds without #1418 running at the same time logged none. In every case:

  • the egress was a participant egress with a camera track, and the warning came as the egress ended, in the same second its outputs finished;
  • the camera track had logged late packets shortly before (dropping old packet, correcting PTS, and in one case packet PTS too far in the past compared to the pipeline 3s before the warning), which is how a track goes inactive and comes back pending;
  • three of the five egresses failed with the error raised by video_encoder_queue; two completed.

None of them logged a not-negotiated error, a streaming stopped from a track's appsrc, or a push error, so the other way an EOS can reach the selector ahead of a switch (see Follow-ups) does not fit. Mute, unmute and subscription changes are logged at debug level, so the order of events is inferred from these lines rather than seen directly.

Testing

  • New TestSelectorHandover_EncoderQueueSurvivesEnding runs every step against the production gates, a real input-selector and the production video_encoder_queue: the filler is on screen, a track becomes pending, the egress starts ending, the filler's EOS reaches the encoder queue and is held there by a probe (standing in for the encoder draining it), a late track frame arrives, the test sends the track's ALLOCATION query (in a pipeline it comes from the track's converter after the switch) and frees it once answered, then later track frames arrive. It checks that the track never takes the selector, that none of its frames leave the selector, that no query is queued behind the EOS, and that the encoder queue keeps accepting data. Without this change it fails every run on both 1.24 and 1.28 core elements, and most runs log Unexpected item ... dequeued from queue video_encoder_queue (refcounting problem?) and exit because we have no item in the queue, after which every push returns an error. It also fails if a handover after ending is taken only on a frame the gate passes. With this change it passes.
  • The selector test harness now gives its inputs' stream-start events group ids, as production streams have. Without them the selector sends stream-start only once, so a switch never sends the new input's stream-start.
  • go test ./pkg/... with the deadlock tag and golangci-lint, as run by build/test/Dockerfile, pass, including the existing TestSelectorHandover_*.
  • A local test, not part of this PR, drives the real pipeline through the pipeline callbacks: a participant egress with an audio and a video track, where the video track's packets stop long enough for it to go inactive, then return about 3s late, and the egress ends before the track's first frame. Before this change, 5 of 5 runs fail with Internal data stream error, both on GStreamer 1.24.13 and on 1.28.7 with Ubuntu's 1.24.2 core elements. With it, 5 of 5 complete on each, and nothing reaches video_encoder_queue after its EOS. The commit before fix: take the video selector handover inside the track's own gate #1418 also completes.
  • Integration, locally on linux/arm64 with an image built from this branch: file-media, file-track and edge pass. The other legs weren't run locally. CI builds amd64.

Follow-ups

  • A track's own EOS while it is selected. If a track's appsrc stops by itself with not-negotiated, basesrc pushes EOS right after posting the error. The track is still the active pad, so the selector forwards that EOS into video_encoder_queue. The pipeline handles the error by removing the track, and OnTrackRemoved switches the selector to the filler behind that EOS, where the filler's ALLOCATION query can hit the same gst_queue behaviour. markEnding doesn't run on this path because the egress isn't ending. Options: an event probe on each track's selector pad that switches to the filler when the selected track's EOS arrives while the pipeline is running, or a probe that drops that EOS. The logs above show no sign of this path, so it is left to a separate change.
  • Refusing serialized queries at video_encoder_queue while it holds an EOS would cover any remaining path into that gst_queue behaviour.
  • Keeping a pending track's last seconds. When a track is still pending as the egress ends, the filler's EOS still goes out early, so that track's last seconds are not recorded. Taking the pending handover inside markEnding would keep them.

🤖 Generated with Claude Code

When a track is still waiting for its first frame as the egress ends, the
filler is the active selector pad, so its EOS goes straight through to
video_encoder_queue. A handover taken by the track's gate after that puts
the track behind the EOS: with the 1.24 input-selector the next sticky
event fails and the egress errors, and an ALLOCATION query from the
handover can be left in the queue after its sender frees it, which the
queue later dequeues ("refcounting problem?").

The video bin now sets an EOS function that marks it as ending before the
filler is sent EOS. Once ending, a track's gate takes no pending handover
and drops frames from tracks that are not selected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

@github-actions

github-actions Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

AV-sync stats summary: view in run #39508689133

Comment thread pkg/pipeline/builder/video.go Outdated
Comment on lines +1380 to +1381
drop := pts < b.lastPTS || (b.selectedPad != videoTestSrcName && b.selectedPad != name) ||
(b.ending && b.selectedPad != name)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this condition becomes hard to read now - can we have a named function that would have some explanatory name?

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@pksgit
pksgit merged commit f68d537 into main Oct 21, 2026
26 of 27 checks passed
@pksgit
pksgit deleted the pradeepsharma/cs-2150-skip-pending-switch-after-eos branch October 21, 2026 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants