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

[1.x] RTCP timestamp does not match RTP timestamp on 32bit archs #3045

Closed
Batchyx opened this issue Aug 25, 2022 · 6 comments
Closed

[1.x] RTCP timestamp does not match RTP timestamp on 32bit archs #3045

Batchyx opened this issue Aug 25, 2022 · 6 comments
Labels
multistream Related to Janus 1.x

Comments

@Batchyx
Copy link

Batchyx commented Aug 25, 2022

The RTP timestamp is very different from the RTCP timestamp. It works with some clients, but not with others.
I traced it down to some overflow that happens in src/ice.c, in the code that adjusts the RTCP timestamp.

What version of Janus is this happening on?
v1.0.3.

Have you tested a more recent version of Janus too?
c319024.

Was this working before?
No, it didn't work in 0.11.6.

Is there a gdb or libasan trace of the issue?
There is no segfault, but the summary of the gdb trace is:

Thread 17 "hloop 577045851" hit Breakpoint 2, janus_ice_outgoing_rtcp_handle (
    user_data=0x760f80) at src/ice.c:4128
4128    uint32_t s = tv.tv_sec + 2208988800u;
(gdb) p tv
$1 = {tv_sec = 1661421867, tv_usec = 403851}
[advancing to 4139...]
0x00523e28      4139    uint32_t rtp_ts = ((ntp-medium->last_ntp_       ts)*(rtcp_ctx->tb))/1000000 + medium->last_rtp_ts
(gdb) p ntp
$3 = -331707829
(gdb) p medium->last_ntp_ts
$5 = 1661421867392411
(gdb) p medium->last_rtp_ts
$6 = 48960
(gdb) next
[...]
(gdb) p rtp_ts
$7 = 125393835

125393835 is not really comparable to 48960.

Additional context
time_t is 32bit on this os/arch.

@Batchyx Batchyx added the multistream Related to Janus 1.x label Aug 25, 2022
@atoppi
Copy link
Member

atoppi commented Aug 25, 2022

time_t is 32bit on this os/arch

I guess this explains why this evaluation in ice.c

int64_t ntp = tv.tv_sec*G_USEC_PER_SEC + tv.tv_usec;

leads to

(gdb) p ntp
$3 = -331707829

Could you please test again by replacing the mentioned line of ice.c with

int64_t ntp = ((int64_t)tv.tv_sec)*G_USEC_PER_SEC + tv.tv_usec;

@Batchyx
Copy link
Author

Batchyx commented Aug 25, 2022

I tested a similar fix and so far it works. I need to do more tests.

@Batchyx
Copy link
Author

Batchyx commented Aug 26, 2022

After fighting with my still-broken webrtc client, i can confirm that this fixes the problem.

@lminiero
Copy link
Member

Thanks for confirming, @atoppi will you push the fix then?

@lminiero
Copy link
Member

PS: we'll probably need to do the same on 0.x too.

@atoppi
Copy link
Member

atoppi commented Aug 29, 2022

fixed by f026b9a (master) and 1fd31e2 (0.x)

@atoppi atoppi closed this as completed Aug 29, 2022
mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this issue Oct 4, 2022
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | minor | `v1.0.4` -> `v1.1.0` |

---

### Release Notes

<details>
<summary>meetecho/janus-gateway</summary>

### [`v1.1.0`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v110---2022-10-03)

[Compare Source](meetecho/janus-gateway@v1.0.4...v1.1.0)

-   Added versioning to .so files \[[PR-3075](meetecho/janus-gateway#3075)]
-   Allow plugins to specify msid in SDPs \[[PR-2998](meetecho/janus-gateway#2998)]
-   Fixed broken RTCP timestamp on 32bit architectures \[[Issue-3045](meetecho/janus-gateway#3045)]
-   Fixed problems compiling against recent versions of libwebsockets \[[Issue-3039](meetecho/janus-gateway#3039)]
-   Updated deprecated DTLS functions to OpenSSL v3.0 [PR-3048](meetecho/janus-gateway#3048)]
-   Switched to SHA256 for signing self signed DTLS certificates (thanks [@&#8203;tgabi333](https://github.com/tgabi333)!) \[[PR-3069](meetecho/janus-gateway#3069)]
-   Started using strnlen to optimize performance of some strlen calls (thanks [@&#8203;tmatth](https://github.com/tmatth)!) \[[PR-3059](meetecho/janus-gateway#3059)]
-   Added checks to avoid RTX payload type collisions \[[PR-3080](meetecho/janus-gateway#3080)]
-   Added new APIs for cascading VideoRoom publishers \[[PR-3014](meetecho/janus-gateway#3014)]
-   Fixed deadlock when using legacy switch in VideoRoom \[[Issue-3066](meetecho/janus-gateway#3066)]
-   Fixed disabled property not being advertized to subscribers when VideoRoom publishers removed tracks
-   Fixed occasional deadlock when using G.711 in the AudioBridge \[[Issue-3062](meetecho/janus-gateway#3062)]
-   Added new way of capturing devices/tracks in janus.js \[[PR-3003](meetecho/janus-gateway#3003)]
-   Removed call to .stop() for remote tracks in demos \[[PR-3056](meetecho/janus-gateway#3056)]
-   Fixed missing message/info/transfer buttons in SIP demo page
-   Fixed postprocessing compilation issue on older FFmpeg versions \[[PR-3064](meetecho/janus-gateway#3064)]
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMTMuMCIsInVwZGF0ZWRJblZlciI6IjMyLjIxMy4wIn0=-->

Reviewed-on: https://git.walbeck.it/walbeck-it/docker-janus-gateway/pulls/91
Co-authored-by: renovate-bot <bot@walbeck.it>
Co-committed-by: renovate-bot <bot@walbeck.it>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

No branches or pull requests

3 participants