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.

Sign up for GitHub

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

videoroom: Release some references created by remote publishers #3359

Closed
wants to merge 2 commits into from
Closed

videoroom: Release some references created by remote publishers #3359

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc9a8ef
Release some references created by remote publishers.
fancycode Apr 24, 2024
3167a36
Update check for dummy forwarder session.
fancycode Jul 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/plugins/janus_videoroom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2713,6 +2713,8 @@ static void janus_videoroom_publisher_dereference_nodebug(janus_videoroom_publis
janus_refcount_decrease_nodebug(&p->ref);
}

static void janus_videoroom_session_destroy(janus_videoroom_session *session);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you want the forward declaration or if one of the two functions should be moved.


static void janus_videoroom_publisher_destroy(janus_videoroom_publisher *p) {
if(p && g_atomic_int_compare_and_exchange(&p->destroyed, 0, 1)) {
janus_mutex_lock(&p->streams_mutex);
Expand Down Expand Up @@ -2750,6 +2752,9 @@ static void janus_videoroom_publisher_destroy(janus_videoroom_publisher *p) {
}
janus_mutex_unlock(&p->rtp_forwarders_mutex);
janus_mutex_unlock(&p->streams_mutex);
/* Release dummy session of the forwarder */
if(p->remote && p->session)
janus_videoroom_session_destroy(p->session);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this here, and why does this mention a dummy session? This is not a function only used for dummy publishers: it's the destroy function for all publishers, and is not meant to be called directly, it's the callback for the cleanup from sessions. At the very least, this should have a p->dummy check too to ensure it's not done on regular publishers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the session created here:

janus-gateway/src/plugins/janus_videoroom.c

Line 7379 in 195bac0

/* We create a dummy session first, that's not actually bound to anything */

It belongs to a regular publisher, so p->dummy will be false, I'll check for p->remote which seems to be the flag to use here.

janus_refcount_decrease(&p->ref);
}
}
Expand Down Expand Up @@ -2799,7 +2804,8 @@ static void janus_videoroom_session_destroy(janus_videoroom_session *session) {
static void janus_videoroom_session_free(const janus_refcount *session_ref) {
janus_videoroom_session *session = janus_refcount_containerof(session_ref, janus_videoroom_session, ref);
/* Remove the reference to the core plugin session */
janus_refcount_decrease(&session->handle->ref);
if (session->handle)
janus_refcount_decrease(&session->handle->ref);
/* This session can be destroyed, free all the resources */
janus_mutex_destroy(&session->mutex);
g_free(session);
Expand Down Expand Up @@ -7291,6 +7297,7 @@ static json_t *janus_videoroom_process_synchronous_request(janus_videoroom_sessi
json_object_set_new(response, "id", string_ids ? json_string(publisher->user_id_str) : json_integer(publisher->user_id));
json_object_set_new(response, "remote_id", json_string(remote_id));
janus_refcount_decrease(&publisher->ref); /* This is just to handle the request for now */
janus_refcount_decrease(&videoroom->ref);
goto prepare_response;
} else if(!strcasecmp(request_text, "unpublish_remotely")) {
/* Configure a local publisher to stop restreaming to a remote VideoRomm instance */
Expand Down Expand Up @@ -13795,7 +13802,7 @@ static void *janus_videoroom_remote_publisher_thread(void *user_data) {
}
g_list_free(subscribers);
/* Free streams */
g_list_free(publisher->streams);
g_list_free_full(publisher->streams, (GDestroyNotify)(janus_videoroom_publisher_stream_unref));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other places use janus_videoroom_publisher_stream_destroy but as this is also used for streams_byid, the reference would not be decreased again, resulting in a leak.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this should be a destroy, then, and the related maps both be unrefs. Besides, if that's the rationale for the list, then it should not only be fixed for remote publishers, but for all of them, since we do a simple g_list_free(participant->streams) in janus_videoroom_hangup_media_internal too. That said, we should thread carefully here, as I don't remember seeing leaks normally (all my local builds use libasan).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some debugging and it looks like that at the time janus_videoroom_subscriber_free or janus_videoroom_publisher_free are called, the streams list and the streams_byid / streams_bymid hashmaps are empty - at least in the scenarios I tested.

However at the end of the remove publisher thread, the streams list is not empty, resulting in the leak if janus_videoroom_publisher_stream_destroy is used.

publisher->streams = NULL;
g_hash_table_remove_all(publisher->streams_byid);
g_hash_table_remove_all(publisher->streams_bymid);
Expand Down
Loading