Skip to content

Restore decoupling#610

Merged
patrickelectric merged 9 commits into
mavlink:masterfrom
joaoantoniocardoso:pr/sink-decoupling
May 28, 2026
Merged

Restore decoupling#610
patrickelectric merged 9 commits into
mavlink:masterfrom
joaoantoniocardoso:pr/sink-decoupling

Conversation

@joaoantoniocardoso

@joaoantoniocardoso joaoantoniocardoso commented May 21, 2026

Copy link
Copy Markdown
Collaborator

As a follow-up to some problems found in #593's introduced strategy of removing the decoupling between the sinks, this patch:

  • Finds the missing piece that previously made the WebRTCSink queue accumulate buffers: WeBRTCBin's inner clocksync elements were sync=true. Now we finally have a truly sync=false WebRTC, which means it always runs with empty queues in steady state.
  • Adds the decoupling back to each sink, so disturbances on one sink can't propagate upstream and/or to other sinks.
  • Removes unneeded queues from the source pipelines so oscillations that amplify freeze-and-burst events can't happen.
  • Refactors WebRTCSink so it resembles the other sinks more closely.
  • The added measured latency sits between 2 and 5ms total (Pi 4), which seems a good price for the robustness.

Solves:

Evaluation:

…cluding clocksync

This changes the webrtcbin's clocksync element to sync=false, which was delaying packets by up to one frame as it aligns them to the wall clock.
@joaoantoniocardoso joaoantoniocardoso changed the title Pr/sink decoupling Restore decoupling May 21, 2026
@joaoantoniocardoso joaoantoniocardoso marked this pull request as ready for review May 22, 2026 13:53
Comment thread src/lib/stream/sink/udp_sink.rs

// Start the sink's own sub-pipeline runner
if let Err(error) = sink.start() {
warn!("Failed to start sink {sink_id}: {error:?}");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

just to be sure, we are failing here, but the insert will happen still ?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Great question. In short, this is keeping the same behavior, which contains a fragility we need to improve. See below.

Each Pipeline has a PipelineRunner, which runs independently. The "Streams" are groups of one Source Pipeline + multiple Sink Pipelines.

When the Source Pipeline of a Stream fails (e.g., if v4l2src's device is closed by Linux), there's a watcher that tears down and recreates the whole Stream (Source Pipeline + each Sink Pipeline).

When a Sink Pipeline of a Stream fails (e.g., if something happens to the socket), it just dies forever.

The lack of communication upstream (from Sink Pipelines to Source Pipelines) was intentional: we don't want the Source or other Sinks to be disturbed by a Sink. What it is lacking is the same recreation strategy used by the Source Pipeline.

Also, in the future, the API should surface the status of each Pipeline Sink of a Stream, but for that, we must semantically separate the Stream concept from the Source Pipeline concept, which today is very liquid/mixed because that was the original design.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ok, that's a pretty fair point.

@patrickelectric patrickelectric merged commit e4a72ab into mavlink:master May 28, 2026
6 checks passed
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.

2 participants