Skip to content

allow setting int64_t based options by passing BigInt values#78

Open
neoxpert wants to merge 4 commits into
Eyevinn:masterfrom
neoxpert:enable_bigint_opts
Open

allow setting int64_t based options by passing BigInt values#78
neoxpert wants to merge 4 commits into
Eyevinn:masterfrom
neoxpert:enable_bigint_opts

Conversation

@neoxpert

Copy link
Copy Markdown
Contributor

Several options expect int64_t values to be set, like SRTO_MAXBW. Extended SetSocketOpt to accept BigInt values.

@birme

birme commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, @neoxpert — BigInt support for int64 options like SRTO_MAXBW is a genuinely useful addition and the lossless overflow check is a nice touch. Before a maintainer can merge, a few things to sort out:

  1. Breaking change: getSockOpt now returns a BigInt for SRTO_INPUTBW/SRTO_MAXBW/SRTO_MININPUTBW where it previously returned a Number — that will break existing callers doing arithmetic/comparisons. Is that intended? It should be called out for the changelog / next major.
  2. Native build: Napi::BigInt requires NAPI_VERSION >= 6, so the addon needs to compile across the CI matrix. Good news: we've just merged ci: run CI on pull_request events (cover fork PRs) #80, which makes CI run on pull requests — previously it only ran for branches inside the main repo, which is why this fork PR showed no checks. If you update this branch onto the latest master (or push any new commit), the full build/test matrix (Node 18/20/22) will now run here automatically, so there's no need to confirm the build by hand.
  3. Scope: the string-read hardening and the single→double quote change in spec/srt_spec.js are a bit outside the stated goal (and the quote change may trip eslint) — happy to keep them, but worth splitting or calling out.

A maintainer will take the final call on the API break. Thanks again!

Reviewed by the node-srt community triage bot — Tier B (touches the native addon + public API), handed to a human maintainer.
Updated after #80 merged: CI now runs on PRs; rebase on master to trigger it.

@birme birme added build-issue Install/compile/link build failure ready-for-maintainer Reviewed by triage bot; awaiting human merge labels Jul 14, 2026
@neoxpert

Copy link
Copy Markdown
Contributor Author

Thank you for reviewing the changes @birme.

Regarding your points:

  1. Yes, this breaking change was intentional. Calling getSockOpt without the change for using int64_t just caused lib-srt to raise an error "Operation not supported: Bad parameters" as it does when trying to set the option with a regular Number / int32 value. I chose to stick with BigInt here, in order to stay consistent with the data types and to avoid adding further data conversions and constraints.
  2. The change of singe to double quotes was unintentional, I reverted that for consistent import style. I will also have a look again on the String read check again and reopen a dedicated PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-issue Install/compile/link build failure ready-for-maintainer Reviewed by triage bot; awaiting human merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants