Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Janus ID should be u64 ? #158

Closed
fcx-mrogez opened this issue Nov 12, 2024 · 5 comments · Fixed by #159
Closed

Janus ID should be u64 ? #158

fcx-mrogez opened this issue Nov 12, 2024 · 5 comments · Fixed by #159

Comments

@fcx-mrogez
Copy link
Contributor

In commit 7e97c4a janus ID were downgraded to u32 but to me this looks wrong.

For instance, audio room ID are u64 https://github.com/meetecho/janus-gateway/blob/master/src/plugins/janus_audiobridge.c#L1477. But it looks like most (all ?) plugins tracks ID using u64.
The json type used for ID is json_int_t (https://jansson.readthedocs.io/en/latest/apiref.html#c.json_int_t) which is "long long" so still 64bit.

What did justify this downgrade to u32 ? In the commit it says that e2e tests revealed that, but in my experience (limited to the streaming plugin), u64 works just fine.
Thanks

@Ghamza-Jd
Copy link
Owner

Ghamza-Jd commented Nov 12, 2024

Hey - Thanks for raising this!

Actually you're right it should be u64, but I think I found what's wrong.

As you said json_int_t is a long long, but not unsigned long long, so anything above u64::MAX / 2 is considered invalid because of the sign bit

{
   "janus": "error",
   "error": {
      "code": 454,
      "reason": "Invalid Janus API request - <string>(1,68): too big integer near '9223372036854775817'"
   }
}

@Ghamza-Jd
Copy link
Owner

@fcx-mrogez
Copy link
Contributor Author

Indeed, it expects positive i64. I was a bit concerned that you encountered some bugs around the fact that json integers are limited to 2^53 precision (https://stackoverflow.com/a/16810116), but this limitation is more related when interfacing with javascript from what I understand.

@Ghamza-Jd
Copy link
Owner

I've created a U63 type, https://github.com/Ghamza-Jd/jarust/blob/fix/minor-fixes/jarust_plugins/src/common.rs

Let me know what do you think

@fcx-mrogez
Copy link
Contributor Author

Looks good to me. Thanks for looking into that quickly ;)

@Ghamza-Jd Ghamza-Jd linked a pull request Nov 12, 2024 that will close this issue
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 a pull request may close this issue.

2 participants