TEL-6181: fix for crash in video_bridge_thread #343
Conversation
izzytelnyx
commented
Nov 5, 2024
izzytelnyx
commented
- replace pointer to sessions with their uuids in vid_helper
izzytelnyx
force-pushed
the
jira-TEL-6181
branch
from
c92591d
to
6ce9109
Compare
izzytelnyx
requested review from
Elhodred,
damirn,
dev-ryanc,
baloeng,
minhtuan1407-telnyx and
tajamulTelnyx
| switch_buffer_t *text_buffer = NULL; | ||
| switch_size_t text_framesize = 1024, inuse = 0; | ||
| unsigned char *text_framedata = NULL; | ||
| switch_frame_t frame = { 0 }; | ||
| switch_core_session_t * session_a, *session_b; | ||
|
|
||
| if ((session_a = switch_core_session_locate(vh->session_a_uuid))) { |
There was a problem hiding this comment.
what is going to happen if you can't locate the session?
There was a problem hiding this comment.
technically, the channel variable would be null and it asserts in switch_channel_up_nosig(which I don't think it's the best approach to assert here honeslty) but it won't enter the while loop.
| channel = switch_core_session_get_channel(session_a); | ||
| } | ||
|
|
||
| if ((session_b = switch_core_session_locate(vh->session_b_uuid))) { |
| while (switch_channel_up_nosig(channel) && switch_channel_up_nosig(b_channel) && vh->up == 1) { | ||
| status = switch_core_session_read_text_frame(vh->session_a, &read_frame, SWITCH_IO_FLAG_NONE, 0); | ||
| status = switch_core_session_read_text_frame(session_a, &read_frame, SWITCH_IO_FLAG_NONE, 0); |
There was a problem hiding this comment.
it seems that we have another assert in switch_core_session_read_text_frame. Now, the question is: why do we assert instead of just returning the SWITCH_STATUS_FALSE status code here?!
| @@ -127,17 +137,25 @@ static void text_bridge_thread(switch_core_session_t *session, void *obj) | |||
| } | |||
|
|
|||
| if (!switch_test_flag(read_frame, SFF_CNG)) { | |||
| switch_status_t tstatus = switch_core_session_write_text_frame(vh->session_b, read_frame, 0, 0); | |||
| switch_status_t tstatus = switch_core_session_write_text_frame(session_b, read_frame, 0, 0); | |||
|
|
||
| vh->up = 1; | ||
|
|
||
| if (switch_core_session_read_lock(vh->session_a) != SWITCH_STATUS_SUCCESS) { | ||
| if (switch_core_session_read_lock(session_a) != SWITCH_STATUS_SUCCESS) { |
| @@ -8830,12 +8830,18 @@ SWITCH_DECLARE(switch_status_t) switch_core_session_start_video_thread(switch_co | |||
| return SWITCH_STATUS_SUCCESS; | |||
| } | |||
|
|
|||
| SWITCH_DECLARE(void) switch_core_media_start_engine_function(switch_core_session_t *session, switch_media_type_t type, switch_engine_function_t engine_function, void *user_data) | |||
| SWITCH_DECLARE(void) switch_core_media_start_engine_function(const char *uuid, switch_media_type_t type, switch_engine_function_t engine_function, void *user_data) | |||
There was a problem hiding this comment.
I don't see a point of passing the uuid here since this should be called in the same thread. So session should be valid or lock until the end of this function. I would advice that you still get the session as a parameter to minimize the change and only pass the UUID in the user data upon starting the video thread.
