Skip to content

Commit

Permalink
Improved memory leak fix
Browse files Browse the repository at this point in the history
  • Loading branch information
@lminiero
lminiero committed Jan 11, 2024
1 parent a15e3fd commit ad326e0
Showing 1 changed file with 2 additions and 3 deletions.
5 changes: 2 additions & 3 deletions src/plugins/janus_audiobridge.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5914,9 +5914,8 @@ void janus_audiobridge_incoming_rtp(janus_plugin_session *handle, janus_plugin_r
jbp.len = 0;
jbp.timestamp = ntohl(rtp->timestamp);
jbp.span = (participant->codec == JANUS_AUDIOCODEC_OPUS ? 960 : 160);
if(GE32(jbp.timestamp + jbp.span + jbp.span, jitter_buffer_get_pointer_timestamp(participant->jitter))) {
jitter_buffer_put(participant->jitter, &jbp);
} else {
jitter_buffer_put(participant->jitter, &jbp);
if(!GE32(jbp.timestamp + jbp.span + jbp.span, jitter_buffer_get_pointer_timestamp(participant->jitter))) {
janus_audiobridge_buffer_packet_destroy(pkt);
}
janus_mutex_unlock(&participant->qmutex);
Expand Down

1 comment on commit ad326e0

@lminiero
Copy link
Member Author

@lminiero lminiero commented on ad326e0 Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@tmatth just FYI, since from my understanding you're the mantainer of libspeex-dsp, do you think it may be worthwhile to address this fix as part of the libspeex-dsp codebase?

Just to give you some context, we noticed leaks under some circumstances, and found out they were caused by jitter_buffer_put. The function would in some cases not queue the packet in the buffer, but since the function is a void, there would be no way for the application to know that happened, causing the allocated buffer never to be freed. I noticed this fix in one of the library forks (xpilot-project/speexdsp@36e68e3) which is what led me to externalise the same check in the AudioBridge plugin in order to know when to free the packet.

Not sure if that fork includes other fixes, but this one does indeed look like a bug that should be fixed. In case you think it makes sense, I guess a version bump (minver?) would be needed too, just so that we (and others) can version check whether they need to externalise the check to free data in that case or not.

Please sign in to comment.