Repository navigation
feat(bindings)!: subscriptions take a Delay { min, max } - #7697
Conversation
Quest-Claim: c815c1239a78c956
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>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # dart/moq/MIGRATE.md # rs/moq-ffi/MIGRATE.md
|
Warning Review limit reachedYou'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. View limit details
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. CodeRabbit Verify
|
kixelated
left a comment
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
✅ 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:MoqDelaymoves toconsumer.rswithmin: Option<Duration>;MoqSubscription.max_delaybecomesdelay: MoqDelay;moq_subscription_defaults()states zero for both; the decoder outputs point atconsumer::MoqDelay.rs/moq-c:moq_subscription.{max_delay_us,max_delay_present}becomesdelay: moq_delay;moq_delaygainsmin_unstatedappended so its layout/size hold.- Go: new
moq.Delay { Min, MinUnstated, Max, MaxUnlimited }whose zero value states zero;Subscription.Delay; a zeroSubscription{}equalsDefaultSubscription();media.Delayremoved and the decoder outputs take amoq.Delayvalue. - Python: wrapper-owned
Delay(min=timedelta(0), max=timedelta(0));Subscription(delay=Delay()); decoder outputs default toDelay(). - Dart:
Delay/Subscriptionbecome extension types over the generated records withDuration.zerodefaults; generatedmoq_ffiregenerated. - Swift/Kotlin:
Delayaliases move to the root;Subscription(...)namesdelay. - 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.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
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>

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 deletesquest/m1/subscription-delay.md.MoqSubscription.max_delayisdelay: MoqDelay, the record the decoder outputs already take.MoqDelay.minisOption<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.MoqDelaymoves besideMoqSubscriptioninconsumer.rs.moq_subscription.{max_delay_us, max_delay_present}ismoq_delay delay, so a zeroed struct is moq-net's defaults again.moq_delaygainsmin_unstated, appended aftermax_unlimitedso its layout (offsets, 24-byte size) is unchanged for decoders;c-tests/decode-output.cpins the new offset.moq.Delay { Min, MinUnstated, Max, MaxUnlimited }value type, whose zero value is a stated zero.Subscription.Delay moq.Delay; the zeroSubscription{}equalsDefaultSubscription().media.Delayis gone;audio/video.DecoderOutput.Delaytake amoq.Delayvalue instead of*media.Delay.moq.Delay(min=timedelta(0), max=timedelta(0));Subscription(delay=Delay()).moq.media.Delayis gone; decoder outputs defaultdelaytoDelay().DelayandSubscriptionbecome 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 explicitnullis unstated/unlimited; a bareSubscription()is moq-net's defaults. Generatedmoq_ffibindings regenerated.Delayaliases move fromMedia.Delay/dev.moq.media.Delayto the root;Subscription(...)namesdelay.c/moq): follow the generated record; probes updated.doc/lib/{py,swift,kt,go,dart,c,cpp}, and## UnreleasedMIGRATE entries updated. Older unreleased MIGRATE lines this change made false (the "no subscriber limit when omitted" entries) are removed or rewritten.Impact
max_delaytodelay.max(see each MIGRATE).Delaymoves from the media namespace to the root in Go, Python, Swift, Kotlin, and Dart. Go decoder outputs take amoq.Delayvalue. Rust and JS: none.Choices made without asking
Delaylives at each wrapper's root. Go forces it (mediaimportsmoq, so the rootSubscriptioncan't namemedia.Delay), and the other wrappers follow so names mirror across bindings. Nomedia.Delayalias is kept (no compat shims).min_unstated(moq-c) andMinUnstated(Go), matching moq-net's "unstated" wording and the existingmax_unlimited.Duration.zerodefault through extension types, since UniFFI can't default a duration in the generated record.Delayvalue defaulting to zero instead of an optional, since the zeroDelayis whatNone/nil meant.MoqFetchGroupOptions.max_delayandMoqFill::max_delay()stay: they mirror Rust'sgroup::Fetchandtrack::Fill, not a subscription.Decisions
C++ and generated-C subscription defaults (maintainer, 2026-11-03). A bare C++
moq::Subscription{}and the generated Cmoq_subscription_default()leave both delay bounds unset (unlimited max, unstated min), because the generated records carry no duration defaults. Options were:subscription_defaults()Duration defaults belong to the future rust2 exporter, not the UniFFI forks, and will be planned there separately.
doc/lib/cppand the generated C package'sc/moq/README.mdnow 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.just swift checkskips, formatting included). Covered byplatform.ymlon macOS.Review
Codex (
quest review), gpt-6.1-sol, medium effort, on head0bf58227bb4321753040e3d8e085246b28aeeb58againstorigin/main9ac02f9: no findings. (The earlier round on2f9ffa9also had none.)🤖 Generated with Claude Code
(Written by Claude Opus 5.5)