Draft 16 interop fixes - #7
Merged
Merged
Conversation
Introduces encode_subgroup_header and encode_subgroup_object as the
primitive building blocks of a subgroup data stream, matching the
layout in draft-16 §10.4.2 (one SUBGROUP_HEADER followed by one or
more {Object ID Delta, Payload Length, Payload} tuples).
The existing encode_object_stream helper is retained as a convenience
wrapper that composes a complete single-object stream, so no call
site behavior changes with this commit. Upcoming work will migrate
serve_subscriptions and the two publish paths to open a single stream
per subgroup and append objects onto it, which is what the spec
requires but moqxr does not yet do.
Also adds a subgroup_id field to CmsfObject. Every media object still
defaults to subgroup 0 so the wire output is unchanged; explicit
subgroup identity now lives in the publish model rather than being
hardcoded inside the transport encoder.
Per draft-16 §2.2, objects from the same subgroup MUST NOT be sent on different streams; per §10.4.2, a subgroup stream begins with one SUBGROUP_HEADER followed by one or more object records. moqxr previously opened a fresh unidirectional QUIC stream per served object and wrote a complete header + single object + FIN on each one, which is a protocol-level violation and caused downstream players to exhaust their MAX_STREAMS_UNI flow-control credit on non-trivial streams. Introduces SubgroupSenderState, a small helper that tracks open uni streams keyed by (group_id, subgroup_id). On the first object of a subgroup it opens a stream and writes the header plus that object in a single write. On subsequent objects in the same subgroup it appends the object onto the open stream with a delta-encoded Object ID and payload, setting fin=true only on the subgroup's final object. Mid-subgroup aborts are intentionally not handled with FIN; connection close carries any such stream per §10.4.1. serve_subscriptions now delegates per-object delivery to a SubgroupSenderState owned by each ActiveSubscription. PUBLISH_DONE's stream_count field is reported from the sender and counts subgroup streams opened -- the spec-intended meaning -- instead of the previous per-object increment. Also adds is_final_object_in_subgroup and subgroup_contains_group_largest helpers. For the current baseline where every object defaults to subgroup 0 these collapse to the existing is_final_object_in_group behavior, but keeping the accessors distinct makes future packager work that assigns non-zero subgroup IDs a local change. Adds a regression test: a plan with three objects in (group=0, subgroup=0) subscribed by one peer must produce exactly one open unidirectional stream, three write_stream calls (first bearing the SUBGROUP_HEADER, last with fin=true), Object ID Deltas of 0, 0, 0 for sequential IDs, and PUBLISH_DONE with stream_count=1.
Applies the same SubgroupSenderState-based stream reuse to forward_published_tracks and publish_selected_tracks that landed for serve_subscriptions in the previous commit. A per-track SubgroupSenderState (keyed on group/subgroup pair) now owns the open unidirectional QUIC streams for each published track, so every object in a given (track, group, subgroup) triple lands on a single stream instead of its own one. PUBLISH_DONE stream_count is now pulled from the sender in both paths, giving the spec-correct "data streams opened" count rather than the previous per-object increment. encode_object_stream is now gone. With all three send paths migrated to the header + object pair it served no purpose except to preserve the old wrong mental model of one stream per object.
Draft-16 §10.4.2 allows several SUBGROUP_HEADER variants controlled by the SUBGROUP_ID_MODE and DEFAULT_PRIORITY bits in the type byte. moqxr was emitting 0x1c: SubgroupIDExplicit plus an inline priority byte. That is spec-legal but redundant for the current publisher, which serves every object in subgroup 0 at the publisher default priority, and it does not match the wire shape that interop partners (mojito, Akamai's test relay, and the associated player stack) exercise most. Switch to the compact form used by those peers: SUBGROUP_ID_MODE = 0 (subgroup ID absent, value 0) and DEFAULT_PRIORITY = 1 (priority byte omitted). The new type byte is 0x30 / 0x38 (end-of-group variant). The per-object fields on the stream are unchanged. If a future packager assigns non-zero subgroup IDs or per-subgroup priorities, encode_subgroup_header can flip back to mode 2 / priority present by adjusting the type bits -- the implementation is already structured around passing subgroup_id in. Tests updated to match the new wire shape.
The transport queued user writes into pending_writes and, on the picoquic processing thread, drained them into picoquic via picoquic_add_to_stream(_with_ctx). When picoquic's per-stream send buffer was full it rejected the call, and moqxr treated that as a fatal transport failure, killing the connection mid-publish. With the subgroup-stream reuse change this became the dominant failure mode for non-trivial assets: a single subgroup stream stays open for an entire group and accumulates many objects, so an overloaded peer or congested link can backlog enough data to fill picoquic's buffer. Previously moqxr emitted one stream per object, so rejections were rare but the cost was a spec-violating stream-per-object pattern that exhausted peer MAX_STREAMS_UNI credit under normal load. Two-sided fix: - On the consumer side, when picoquic rejects a write, re-queue the pending write at the front of pending_writes and keep the connection alive. The picoquic thread wakes again as acks come in, which drains its send buffer and eventually lets the write through. - On the producer side, WebTransportClient::write_stream and PicoquicClient::write_stream now cap the total queued bytes at 4 MiB and block (bounded at 30 s) until the consumer drains below that cap. This keeps the deferred-write queue bounded and applies real back-pressure to the serve loop rather than returning a fake success while the queue grows without bound.
serve_subscriptions previously picked the active subscription with the smallest next_object_index into plan.objects. That produced severely unbalanced track scheduling when the PublishPlan was laid out track-by-track rather than time-interleaved. Concretely, a source MP4 whose packager emits a single giant video group followed by a single giant audio group yields a plan of the form [catalog, vide_1 g0 o0..N, soun_2 g0 o0..M]. Under plan-index scheduling the publisher served every video object before a single audio object could go out, starving the audio subscription and leaving MSE unable to advance playback. For typical fragmented MP4 inputs with many keyframe-aligned groups the defect was masked, because the plan layout interleaved groups. Scheduling by the candidate object's media_time_us produces chronological A/V interleave regardless of plan layout, which is the delivery order subscribers actually want. Ties on media_time_us fall back to plan index for deterministic ordering. The existing "serve all subscriptions pointing at this exact plan index" batch behaviour is preserved, so multiple subscribers on the same track at the same position still share a single object encode. Verified against the Akamai test relays: serving a fragmented 60s MP4 now shows vide_1/soun_2 objects interleaved at their source timestamps (vide_1 g0 o0 @ 0ms, soun_2 g0 o0 @ 0ms, vide_1 g0 o1 @ 33ms, soun_2 g0 o1 @ 22ms, ...) instead of exhausting one track before the other.
mondain
approved these changes
Apr 15, 2026
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
2 participants
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.
Series of fixes for draft-16 interop issues surfaced while trying to publish through an Akamai relay to a web player. Each commit is spec-motivated and independently testable. All existing tests pass and new regression tests cover the behavior changes. End-to-end playback verified with a pre-fragmented MP4 through a live relay.