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

Jump to bottom

videoroom: Release some references created by remote publishers #3359

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

videoroom: Release some references created by remote publishers #3359

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/plugins/janus_videoroom.c
View file
Open in desktop
Expand Up @@ -2433,6 +2433,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)) {
/* Forwarders with RTCP support may have an extra reference, stop their source */
Expand Down Expand Up @@ -2468,6 +2470,9 @@ static void janus_videoroom_publisher_destroy(janus_videoroom_publisher *p) {
}
}
janus_mutex_unlock(&p->rtp_forwarders_mutex);
/* Release dummy session of the forwarder */
if(p->session && p->session->handle == NULL)
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.

janus_refcount_decrease(&p->ref);
}
}
Expand Down Expand Up @@ -2516,7 +2521,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 @@ -6985,6 +6991,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 @@ -13421,7 +13428,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).

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