videoroom: Release some references created by remote publishers #3359
Conversation
|
I don't think that's wise: the map you see in the publisher's struct is only a shallow reference, since the actual map is stored in the publisher_stream struct, and we do have the |
|
Thanks for your feedback. I had a problem where the forwarders were not released properly when my controller websocket connection was closed and the patch fixed this. So maybe my problem must be fixed differently - I'll do some more tests and provide an update here. |
I'd check if the cause is that there are publisher streams (and maybe entire publishers) whose references never get to zero causing their forwarders not to be destroyed either. Did you check by uncommenting the |
fancycode
force-pushed
the
destroy-forwarders
branch
from
508e84d
to
00e3d3d
Compare
fancycode
changed the title
| @@ -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)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| @@ -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); | |||
There was a problem hiding this comment.
Not sure if you want the forward declaration or if one of the two functions should be moved.
| @@ -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); | |||
There was a problem hiding this comment.
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.
| @@ -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)); | |||
There was a problem hiding this comment.
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).
Fixed some cases where thertp_forwardersmap was created without the destructor function. This resulted in the forwarders not being released when the publisher got destroyed.