Skip to content

moq-gst: Fix MoqSink CAPS handling and per-pad EOS aggregation#1402

Merged
kixelated merged 2 commits into
moq-dev:mainfrom
SteveMcFarlin:fix/eos-handling
May 14, 2026
Merged

moq-gst: Fix MoqSink CAPS handling and per-pad EOS aggregation#1402
kixelated merged 2 commits into
moq-dev:mainfrom
SteveMcFarlin:fix/eos-handling

Conversation

@SteveMcFarlin
Copy link
Copy Markdown
Contributor

This fixes two correctness issues in sink/imp.rs: CAPS events are now passed to event_default() after configuring the background task, so pad state still updates normally, and EOS tracking is now per-pad instead of using a single counter. That avoids premature or missed element EOS when pads are dynamic, dropped, or emit EOS more than once.

@SteveMcFarlin
Copy link
Copy Markdown
Contributor Author

SteveMcFarlin commented May 12, 2026

@sidsethupathi @kixelated. Let me know if you have any questions. I tested with:

gst-launch-1.0 -v -e videotestsrc is-live=true num-buffers=120 ! video/x-raw,width=1280,height=720,framerate=60/1,format=I420 ! x264enc ! moqsink name=mux url="https://cdn.moq.dev/anon" broadcast="videotestsrc"

My main concern was with a possible race conditions between add/remove pad and EOS. Some of the other items are just for gst state.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

Review Change Stack
No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 84c6ebef-1e27-4bff-810a-45c5c351b314

📥 Commits

Reviewing files that changed from the base of the PR and between 6adf3c3 and afdf280.

📒 Files selected for processing (1)
  • rs/moq-gst/src/sink/imp.rs

Walkthrough

MoqSink's sink-pad event handling was refactored to aggregate EOS signals per pad instead of using a global counter. The ControlMessage::Eos variant now includes a pad_name field to identify which pad sent EOS. A HashSet named eos_pads replaces the previous counter to track which pads have emitted EOS. The handle_event method converts CAPS and EOS events to control messages with per-pad information. The background session's loop now aggregates EOS by checking that all currently configured pads have reported EOS before posting a single element EOS, and removes pad names from the set when pads are dropped.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'moq-gst: Fix MoqSink CAPS handling and per-pad EOS aggregation' accurately describes the two main fixes in the changeset: CAPS event handling and per-pad EOS aggregation logic.
Description check ✅ Passed The description directly addresses both correctness issues fixed in the changeset: CAPS event routing to event_default() and per-pad EOS tracking instead of a single counter.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
✨ Simplify code
  • Create PR with simplified code

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

return false;
}

true
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.

Should gst::Pad::event_default be called for EOS as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No. This is a terminal SINK. It will never forward on EOS downstream. The caps need to for internal state.

Copy link
Copy Markdown
Contributor

@sidsethupathi sidsethupathi left a comment

Choose a reason for hiding this comment

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

Confirmed that multiple pads still works as expected.

gst-launch-1.0 -v -e videotestsrc is-live=true num-buffers=120 ! video/x-raw,width=1280,height=720,framerate=60/1,format=I420 ! x264enc ! moqsink name=mux url="https://cdn.moq.dev/anon" broadcast="videotestsrc" audiotestsrc is-live=true num-buffers=100 samplesperbuffer=882 ! avenc_aac ! mux.

@SteveMcFarlin
Copy link
Copy Markdown
Contributor Author

Hand tested a failure case pointed at /dev/null.

2026-05-12T03:45:22.159469Z  WARN moq_native::client: QUIC connection failed err=failed DNS lookup
2026-05-12T03:45:22.228507Z  WARN moq_native::websocket: WebSocket connection failed err=failed to connect WebSocket
ERROR: from element /GstPipeline:pipeline0/GstVideoTestSrc:videotestsrc0: Internal data stream error.

I will look into this more for proper handling. For now I think this will prevent a few hard to find bugs with EOS.

@kixelated
Copy link
Copy Markdown
Collaborator

kixelated commented May 14, 2026

Yep this is closer to how I expected EOS handling to work.

@kixelated kixelated merged commit b328775 into moq-dev:main May 14, 2026
1 check passed
@moq-bot moq-bot Bot mentioned this pull request Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants