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] Streaming plugin recording capability has mutex issues #2902

Closed
evplatt opened this issue Feb 25, 2022 · 2 comments
Closed

[1.x] Streaming plugin recording capability has mutex issues #2902

evplatt opened this issue Feb 25, 2022 · 2 comments
Labels
bug multistream Related to Janus 1.x

Comments

@evplatt
Copy link

evplatt commented Feb 25, 2022

What version of Janus is this happening on?
1.0.0

Have you tested a more recent version of Janus too?
N/A

Was this working before?
Unknown - first usage

Is there a gdb or libasan trace of the issue?
No

Additional context

For streaming recording start requests with 'media' key (not the "old method"), mountpoints_mutex is never unlocked after starting the recording. Suspect it should be unlocked prior to 'goto prepare_response'

For streaming recording stop requests with 'media' key (not the "old method"):

  • source->rec_mutex attempts to lock twice, once before if(media) and once after, causing a lockup
  • mountpoints_mutex is never unlocked after stopping the recording. Suspect it should be unlocked prior to 'goto prepare_response'
@evplatt evplatt added the multistream Related to Janus 1.x label Feb 25, 2022
@atoppi
Copy link
Member

atoppi commented Feb 25, 2022

Thanks for reporting @evplatt.

When starting recording it seems that both a mp ref decrease and mountpoints_mutex unlock are missing in this block

}
janus_mutex_unlock(&source->rec_mutex);
/* Send a success response back */
response = json_object();
json_object_set_new(response, "streaming", json_string("ok"));
goto prepare_response;

When stopping I can confirm the double source->rec_mutex locking

janus_mutex_lock(&source->rec_mutex);
if(media) {
/* Iterate on all media to stop */
janus_mutex_lock(&source->rec_mutex);

and also the missing unlock / unref

}
janus_mutex_unlock(&source->rec_mutex);
/* Send a success response back */
response = json_object();
json_object_set_new(response, "streaming", json_string("ok"));
goto prepare_response;

we'll work on a fix.

@atoppi atoppi added the bug label Feb 25, 2022
@atoppi atoppi closed this as completed in 6f0b707 Feb 25, 2022
lminiero added a commit that referenced this issue Feb 25, 2022
@lminiero
Copy link
Member

lminiero commented Feb 25, 2022

Related to this, I fixed a couple of additional issues:

  • there was still a crash when stopping a recording, and that was due to a dereference of stream->rc that could be NULL (not all streams may be recording, when we choose to stop it)
  • there was also a problem when starting recordings using the legacy API, as the code was not really using the video and data properties when provided.

Both should be fixed in the commit above.

mwalbeck pushed a commit to mwalbeck/docker-janus-gateway that referenced this issue Mar 16, 2022
This PR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [meetecho/janus-gateway](https://github.com/meetecho/janus-gateway) | major | `v0.11.7` -> `v1.0.0` |

---

### Release Notes

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

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

[Compare Source](meetecho/janus-gateway@v0.12.0...v1.0.0)

-   Refactored Janus to support multistream PeerConnections \[[PR-2211](meetecho/janus-gateway#2211)]
-   Moved all source files under new 'src' folder to unclutter the repo \[[PR-2885](meetecho/janus-gateway#2885)]
-   Fixed definition of trylock wrapper when using pthreads \[[Issue-2894](meetecho/janus-gateway#2894)]
-   Fixed broken RTP when no extensions are negotiated
-   Added checks when inserting RTP extensions to avoid buffer overflows
-   Added missing support for disabled rid simulcast substreams in SDP \[[PR-2888](meetecho/janus-gateway#2888)]
-   Fixed TWCC feedback when simulcast SSRCs are missing (thanks [@&#8203;OxleyS](https://github.com/OxleyS)!) \[[PR-2908](meetecho/janus-gateway#2908)]
-   Added support for playout-delay RTP extension \[[PR-2895](meetecho/janus-gateway#2895)]
-   Fixed partially broken H.264 support when using Firefox in VideoRoom
-   Fixed new VideoRoom rtp_forward API ignoring some properties
-   Fixed deadlock and segfault when stopping Streaming mountpoint recordings \[[Issue-2902](meetecho/janus-gateway#2902)]
-   Fixed RTSP support in Streaming plugin for cameras that expect path-only DESCRIBE requests (thanks [@&#8203;jp-bennett](https://github.com/jp-bennett)!) \[[PR-2909](meetecho/janus-gateway#2909)]
-   Fixed RTP being relayed incorrectly in Lua and Duktape plugins
-   Added Duktape as optional dependency, instead of embedding the engine code \[[PR-2886](meetecho/janus-gateway#2886)]
-   Fixed crash at startup when not able to connect to RabbitMQ server
-   Improved fuzzing and checks on RTP extensions
-   Removed distinction between simulcast and simulcast2 in janus.js \[[PR-2887](meetecho/janus-gateway#2887)]
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

### [`v0.12.0`](meetecho/janus-gateway@v0.11.8...v0.12.0)

[Compare Source](meetecho/janus-gateway@v0.11.8...v0.12.0)

### [`v0.11.8`](https://github.com/meetecho/janus-gateway/blob/HEAD/CHANGELOG.md#v0118---2022-02-11)

[Compare Source](meetecho/janus-gateway@v0.11.7...v0.11.8)

-   Added initial (and limited) integration of RED audio \[[PR-2685](meetecho/janus-gateway#2685)]
-   Added support for Two-Byte header RTP extensions (RFC8285) and, partially, for the new Depencency Descriptor RTP extension (needed for AV1-SVC) \[[PR-2741](meetecho/janus-gateway#2741)]
-   Fixed rare race conditions between sending a packet and closing a connection \[[PR-2869](meetecho/janus-gateway#2869)]
-   Fix last stats before closing PeerConnection not being sent to handlers (thanks [@&#8203;zodiak83](https://github.com/zodiak83)!) \[[PR-2874](meetecho/janus-gateway#2874)]
-   Changed automatic allocation on static loops from round robin to least used \[[PR-2878](meetecho/janus-gateway#2878)]
-   Added new API to bulk start/stop MJR-based recordings in AudioBridge \[[PR-2862](meetecho/janus-gateway#2862)]
-   Fixed broken duration in spatial AudioBridge recordings
-   Fixed broken G.711 RTP forwarding in AudioBridge (thanks [@&#8203;AlexYaremchuk](https://github.com/AlexYaremchuk)!) \[[PR-2875](meetecho/janus-gateway#2875)]
-   Fixed broken recordings in NoSIP plugin
-   Fixed warnings when postprocessing Opus recordings with DTX packets
-   Other smaller fixes and improvements (thanks to all who contributed pull requests and reported issues!)

</details>

---

### Configuration

📅 **Schedule**: 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).

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

No branches or pull requests

3 participants