Skip to content

feat(bindings)!: subscriptions take a Delay { min, max } - #7697

Merged
kixelated merged 8 commits into
mainfrom
quest/m1/subscription-delay
Nov 3, 2026
Merged

kixelated merged 8 commits into
mainfrom
quest/m1/subscription-delay

Conversation

@kixelated

@kixelated kixelated commented Nov 3, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Every binding's subscription takes the Delay { min, max } that moq-dev/moq#7597 gave Rust and JS, in place of a bare max delay. Finishes and deletes quest/m1/subscription-delay.md.

  • moq-ffi: MoqSubscription.max_delay is delay: MoqDelay, the record the decoder outputs already take. MoqDelay.min is Option<Duration>: null leaves the target latency unstated (a decoder plays it as zero, the subscription keeps it unstated). moq_subscription_defaults() states zero for both. MoqDelay moves beside MoqSubscription in consumer.rs.
  • moq-c (hand-written): moq_subscription.{max_delay_us, max_delay_present} is moq_delay delay, so a zeroed struct is moq-net's defaults again. moq_delay gains min_unstated, appended after max_unlimited so its layout (offsets, 24-byte size) is unchanged for decoders; c-tests/decode-output.c pins the new offset.
  • Go: a new moq.Delay { Min, MinUnstated, Max, MaxUnlimited } value type, whose zero value is a stated zero. Subscription.Delay moq.Delay; the zero Subscription{} equals DefaultSubscription(). media.Delay is gone; audio/video.DecoderOutput.Delay take a moq.Delay value instead of *media.Delay.
  • Python: a wrapper-owned frozen moq.Delay(min=timedelta(0), max=timedelta(0)); Subscription(delay=Delay()). moq.media.Delay is gone; decoder outputs default delay to Delay().
  • Dart: Delay and Subscription become extension types over the generated records (still passable wherever those are taken). Delay({min = Duration.zero, max = Duration.zero}), so an omitted bound is zero and an explicit null is unstated/unlimited; a bare Subscription() is moq-net's defaults. Generated moq_ffi bindings regenerated.
  • Swift / Kotlin: Delay aliases move from Media.Delay / dev.moq.media.Delay to the root; Subscription(...) names delay.
  • C++ / generated C (c/moq): follow the generated record; probes updated.
  • Tests, interop clients (Go, Python, C++), doc/lib/{py,swift,kt,go,dart,c,cpp}, and ## Unreleased MIGRATE entries updated. Older unreleased MIGRATE lines this change made false (the "no subscriber limit when omitted" entries) are removed or rewritten.

Impact

  • Public API: breaking in every binding. Migrate max_delay to delay.max (see each MIGRATE). Delay moves from the media namespace to the root in Go, Python, Swift, Kotlin, and Dart. Go decoder outputs take a moq.Delay value. Rust and JS: none.
  • Wire: none.

Choices made without asking

  • Delay lives at each wrapper's root. Go forces it (media imports moq, so the root Subscription can't name media.Delay), and the other wrappers follow so names mirror across bindings. No media.Delay alias is kept (no compat shims).
  • Flag names: min_unstated (moq-c) and MinUnstated (Go), matching moq-net's "unstated" wording and the existing max_unlimited.
  • Dart reaches the Plan's Duration.zero default through extension types, since UniFFI can't default a duration in the generated record.
  • Python and Go decoder outputs take a Delay value defaulting to zero instead of an optional, since the zero Delay is what None/nil meant.
  • MoqFetchGroupOptions.max_delay and MoqFill::max_delay() stay: they mirror Rust's group::Fetch and track::Fill, not a subscription.

Decisions

  • C++ and generated-C subscription defaults (maintainer, 2026-11-03). A bare C++ moq::Subscription{} and the generated C moq_subscription_default() leave both delay bounds unset (unlimited max, unstated min), because the generated records carry no duration defaults. Options were:

    • ✅ accept and document: the docs and MIGRATE say to start from subscription_defaults()
    • add generator support in the UniFFI forks
    • hand-write the C++ types

    Duration defaults belong to the future rust2 exporter, not the UniFFI forks, and will be planned there separately. doc/lib/cpp and the generated C package's c/moq/README.md now say plainly to start every subscription from the defaults.

Checks

On the merged head 0bf5822:

  • just rs check-test -p moq-ffi -p moq-c: 466 passed, lint clean. just rs c-tests: pass.
  • just go check, just py check + just py test (113 passed), just dart check (generated bindings fresh), just kt check, just cpp check, just c check: pass. The Go interop client built against the staged wrapper on the earlier head.
  • Swift: not run; no Swift toolchain on this Linux host (just swift check skips, formatting included). Covered by platform.yml on macOS.

Review

Codex (quest review), gpt-6.1-sol, medium effort, on head 0bf58227bb4321753040e3d8e085246b28aeeb58 against origin/main 9ac02f9: no findings. (The earlier round on 2f9ffa9 also had none.)

🤖 Generated with Claude Code

(Written by Claude Opus 5.5)

kixelated and others added 6 commits November 2, 2026 23:36
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed bound

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Binding subscriptions take a Delay { min, max } feat(bindings)!: subscriptions take a Delay { min, max } Nov 3, 2026
kixelated and others added 2 commits November 3, 2026 09:13
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts:
#	dart/moq/MIGRATE.md
#	rs/moq-ffi/MIGRATE.md
@kixelated
kixelated marked this pull request as ready for review November 3, 2026 17:51
@coderabbitai

coderabbitai Bot commented Nov 3, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 19 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: dab5fdad-bb64-4869-8828-e13339903713
Commits

Reviewing files that changed from the base of the PR and between 9ac02f9 and 0bf5822.

Files selected for processing: 67
  • c/moq/MIGRATE.md
  • c/moq/README.md
  • c/moq/test/probe.c
  • cpp/moq/MIGRATE.md
  • cpp/moq/test/probe.cpp
  • dart/moq/MIGRATE.md
  • dart/moq/lib/media.dart
  • dart/moq/lib/src/aliases.dart
  • dart/moq/test/moq_test.dart
  • dart/moq_ffi/lib/src/moq.dart
  • doc/lib/c/index.md
  • doc/lib/cpp/index.md
  • doc/lib/dart/index.md
  • doc/lib/go/index.md
  • doc/lib/kt/index.md
  • doc/lib/py/index.md
  • doc/lib/swift/index.md
  • go/wrapper/MIGRATE.md
  • go/wrapper/audio/audio.go
  • go/wrapper/bridge.go
  • go/wrapper/internal/bridge/handles.go
  • go/wrapper/json/json.go
  • go/wrapper/media/media.go
  • go/wrapper/moq_test.go
  • go/wrapper/records.go
  • go/wrapper/records_internal_test.go
  • go/wrapper/video/video.go
  • kt/moq/MIGRATE.md
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/Aliases.kt
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/json/Tracks.kt
  • kt/moq/src/jvmAndAndroidMain/kotlin/dev/moq/media/Media.kt
  • kt/moq/src/jvmAndAndroidTest/kotlin/dev/moq/SmokeTest.kt
  • py/moq-rs/MIGRATE.md
  • py/moq-rs/moq/__init__.py
  • py/moq-rs/moq/_records.py
  • py/moq-rs/moq/audio.py
  • py/moq-rs/moq/json.py
  • py/moq-rs/moq/media.py
  • py/moq-rs/moq/types.py
  • py/moq-rs/moq/video.py
  • py/moq-rs/tests/test_local.py
  • py/moq-rs/tests/test_testing_clock.py
  • quest/m1/README.md
  • quest/m1/datagram-tracks/README.md
  • quest/m1/max-age-defaults.md
  • quest/m1/subscription-delay.md
  • rs/moq-c/MIGRATE.md
  • rs/moq-c/c-tests/decode-output.c
  • rs/moq-c/src/api.rs
  • rs/moq-c/src/test.rs
  • rs/moq-ffi/MIGRATE.md
  • rs/moq-ffi/src/audio.rs
  • rs/moq-ffi/src/consumer.rs
  • rs/moq-ffi/src/json.rs
  • rs/moq-ffi/src/media.rs
  • rs/moq-ffi/src/producer.rs
  • rs/moq-ffi/src/test.rs
  • rs/moq-ffi/src/video.rs
  • swift/MIGRATE.md
  • swift/Sources/Moq/Aliases.swift
  • swift/Sources/Moq/AsyncStream.swift
  • swift/Sources/Moq/Json.swift
  • swift/Sources/Moq/Media.swift
  • swift/Tests/MoqTests/SmokeTests.swift
  • test/interop/clients/cpp/main.cpp
  • test/interop/clients/go/main.go
  • test/interop/clients/python/interop.py
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated kixelated left a comment

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.

Automated review by review (OpenAI)

Reviewed head 0bf5822.

P2: Tell existing decoder callers to initialize the new delay flag. rs/moq-c/src/api.rs:733–740 reads min_unstated, which occupies a former padding byte. A caller that constructed moq_delay by assigning all three previously existing fields still compiles after rebuilding, but leaves this new bool uninitialized; decoding can discard the configured minimum or read an invalid Rust bool. rs/moq-c/MIGRATE.md:10–11 notes that the layout stays the same but does not tell those callers to set min_unstated = false or zero-initialize the whole record. Please add that migration instruction and cover the field-by-field case. This concerns source migration from the unreleased API, not a claim of released binary compatibility.

Verification: GitHub-only static review; I did not run tests. The PR reports binding checks and tests passing, with Swift left to macOS CI.

@pullfrog pullfrog Bot left a comment

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.

✅ No new issues found.

Reviewed changes

This run reviewed the full binding-wide move from a bare max delay to Delay { min, max } on track subscriptions, mirroring Rust/JS after #7597.

  • rs/moq-ffi: MoqDelay moves to consumer.rs with min: Option<Duration>; MoqSubscription.max_delay becomes delay: MoqDelay; moq_subscription_defaults() states zero for both; the decoder outputs point at consumer::MoqDelay.
  • rs/moq-c: moq_subscription.{max_delay_us,max_delay_present} becomes delay: moq_delay; moq_delay gains min_unstated appended so its layout/size hold.
  • Go: new moq.Delay { Min, MinUnstated, Max, MaxUnlimited } whose zero value states zero; Subscription.Delay; a zero Subscription{} equals DefaultSubscription(); media.Delay removed and the decoder outputs take a moq.Delay value.
  • Python: wrapper-owned Delay(min=timedelta(0), max=timedelta(0)); Subscription(delay=Delay()); decoder outputs default to Delay().
  • Dart: Delay/Subscription become extension types over the generated records with Duration.zero defaults; generated moq_ffi regenerated.
  • Swift/Kotlin: Delay aliases move to the root; Subscription(...) names delay.
  • C++/generated C: follow the generated record; the bare record's unset bounds are an accepted, documented decision, with docs steering users to subscription_defaults().

I verified the semantics that carry the change: moq-net's Delay default is stated zero and an unstated min aggregates as the smallest stated one (rs/moq-net/src/model/subscription.rs:528), so None never pulls the aggregate to zero; decoders read an unstated min as zero (rs/moq-audio/src/decode/consumer.rs:230, rs/moq-video/src/decode/consumer.rs:184, rs/moq-mux/src/container/consumer.rs:1130); and each language's default form (Go/Python/Dart/C-hand-written zeroed struct, plus moq_subscription_defaults()) states zero while unlimited/unstated stay expressible. The moq_delay layout change is consistent across the pinned offsets, the Rust tests, and the probes.

One note that did not warrant a change: go/wrapper/MIGRATE.md's #7311 entry (lines 23-25) still describes *media.Delay/media.Delay{Min, Max} while the added #7697 entry renames that type, and every other wrapper updated its equivalent lines. It reads as the intended chronological description of the #7311 step (both are under ## Unreleased), so I left it alone.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@kixelated
kixelated added this pull request to the merge queue Nov 3, 2026
Merged via the queue into main with commit c4f8abc Nov 3, 2026
11 checks passed
@kixelated
kixelated deleted the quest/m1/subscription-delay branch November 3, 2026 19:51
kixelated added a commit that referenced this pull request Nov 4, 2026
Resolve the modify/delete conflicts on quests main finished
(m0/ci/shared-target.md in #7715, m1/resume-model-7542.md in #7705), and
drop or repoint links and Required edges to quests main deleted:
m1/ietf-resume-behind.md (#7708), m1/subscription-delay.md (#7697),
m1/unbounded-groups/log-lock-profile.md (#7742) and m1/moq-crypto.md
(#7702).

#7705 did not replay #7739's seed 17893651, which this PR had added to
the resume-model quest, so it moves to m1/resume-seed-17893651.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant