refactor(libmoq)!: replace the client setters with a config struct - #2880
Open
kixelated wants to merge 2 commits into
Open
refactor(libmoq)!: replace the client setters with a config struct#2880kixelated wants to merge 2 commits into
kixelated wants to merge 2 commits into
Conversation
libmoq ships as a staticlib: the release tarball packages include/moq.h
beside lib/libmoq.a, nix/overlay.nix installs only the archive, and no
cdylib is built anywhere. A caller therefore always compiles against the
header matching the archive it links, so appending a field to a C struct
is a recompile rather than an ABI break.
The moq_client_set_* / moq_client_get_* functions existed to avoid that
break, and the docs said so. Since the break cannot happen they bought
nothing but surface, so they collapse to one moq_client_config struct plus
moq_client_defaults(), which replaces every getter at once.
moq_client_connect folds into moq_session_connect, whose new config
parameter is NULL for the defaults, leaving one connect function.
The knobs whose default is not zero carry a has_* flag rather than being
read directly. websocket.enabled defaults to true and the reconnect backoff
to 1s/2/5s/10s, so a caller who zeroed the struct and set nothing would
otherwise have silently disabled them.
moq_client_defaults returns by value: with no out-pointer there is no
handle to look up and nothing to reject, so the int32_t status channel had
no failure left to report.
Also in this change:
- moq_video_config's coded_width/coded_height stop being *const u32. Zero
already means absent for a dimension, so each keeps its own presence and
a half-specified catalog still round-trips.
- build.rs sets pragma_once. cbindgen.toml is never loaded, so the header
had no include guard, and moq-settings.h now reaches it down a second
include path.
- moq_publish_{video,audio}_config no longer claim that re-declaring a
rendition name replaces it; hang's insert returns Duplicate.
- The docs drop their claims that a dynamic library ships.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3fa6e4572
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The clang-format gate rejected the hand-edited moq_session_connect stub in the OBS output test; clang-format now owns its wrapping. moq_session_connect's `# Safety` block covered only `url` and `user_data`, even though a non-NULL `config` is dereferenced along with every pointer the struct selects. It now states that the config must be aligned and readable, that each inner pointer must be valid for its paired length, and that all of them must outlive the call, since the config is read during the dial rather than copied up front. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Summary
include/moq.hbesidelib/libmoq.a,nix/overlay.nixinstalls just the archive, andcrate-type = ["staticlib"]builds no cdylib. A caller therefore always compiles against the header matching the archive it links, so appending a field to a C struct is a recompile, not an ABI break.moq_client_set_*/moq_client_get_*functions existed to avoid that break, and the docs said so. Since the break can't happen they bought nothing but surface, so they collapse into onemoq_client_configstruct plusmoq_client_defaults(), which replaces every getter at once.moq_client_connectfolds intomoq_session_connect, whose newconfigparameter is NULL for the defaults. One connect function instead of two.Knobs whose default is not zero carry a
has_*flag rather than being read directly.websocket.enableddefaults to true and the reconnect backoff to 1s/2/5s/10s, so a caller who zeroed the struct and set nothing would otherwise have silently disabled them. The flag is also what separates "leave it alone" from an explicit value:backoff_timeout_ms = 0with the flag set means retry forever.moq_client_defaultsreturns by value. With no out-pointer there's no handle to look up and nothing to reject, so theint32_tstatus channel had no failure left to report.Also in this change:
moq_video_config'scoded_width/coded_heightstop being*const u32. Zero already means absent for a dimension, so each keeps its own presence and a half-specified catalog still round-trips (collapsing them behind one flag invented a zero for the missing half).build.rssetspragma_once.cbindgen.tomlis never loaded, so the generated header had no include guard — fine untilmoq-settings.hstarted reaching it down a second include path.moq_publish_{video,audio}_configno longer claim that re-declaring a rendition name replaces it.hang'sinsertreturnsDuplicate; pre-existing doc bug.libmoq.so/libmoq.dylib/moq.dllnever existed).Public API changes
Breaking, hence
dev— all inrs/libmoq's C ABI:moq_client_create,moq_client_close,moq_client_connect, and everymoq_client_set_*/moq_client_get_*.moq_client_config(struct),moq_client_defaults()(returns it by value).moq_session_connectgainsconst moq_client_config *configafterurl_len.moq_video_config.coded_width/.coded_heightareuint32_t(wereconst uint32_t *).moq.hnow carries#pragma once.Rust-side
libmoq::Error::ClientNotFoundis gone (the handle it named no longer exists).libmoqis a staticlib, so its Rust surface has no consumers.Test plan
cargo nextest run -p libmoq— 64/64 pass, including a newa_half_specified_coded_size_round_tripsthat forwards a width-only catalog between broadcasts, anda_zeroed_config_is_the_defaults/defaults_report_what_a_zeroed_config_dialspinned againstclient::Config::default()so retuning a default without following through to C fails here.cargo clippy -p libmoq --all-targetsandcargo fmt— clean.cargo doc -p libmoq --no-deps— clean (several intra-doc links pointed at the removed functions).moq.htwice and exercises every shapecpp/obsdepends on: the 7-arg dial with NULL, scalar dimensions, and themoq_client_configfields the settings builder assigns.Cross-Package Sync
rs/libmoqC ABI →cpp/obs/srcanddoc/lib/care both updated.cpp/obs/src/moq-settings.cppdrops ~130 lines of setter calls for field assignments;MoQSettings::Confignow owns the strings the config borrows, since libmoq reads them during the dial rather than copying at set time.Reviewer notes
cpp/obsis not compiled here. PR CI never builds it and there's no libobs on this machine, sojust obs build/just obs testare still the real gate. An adversarial review pass caught that three OBS call sites hadn't been migrated at all; those are fixed, but the plugin has only been checked against the header, not built. feat(obs): compile the plugin in CI, and type-check it without obs-deps #2867 would close this gap.moq_session_connectwith the reason inmoq_error().BuildConfigcan no longer name the offending field — it still refuses to start, just less specifically. That's the one thing the setters genuinely bought.api.rs/test.rs/ OBS files. Whichever lands second will need a rebase.(written by Opus 5)