jitterbuffer: Correct signed/unsigned mismatch causing assert
If the system time has stepped backwards because of a time adjustment between the time a frame is timestamped and the time we check the timestamps in abstract_jb:hook_event_cb(), we get a negative interval, but we don't check for that there. abstract_jb:hook_event_cb() then calls fixedjitterbuffer:fixed_jb_get() (via abstract_jb:jb_get_fixed) and the first thing that does is assert(interval >= 0). There are several issues with this... * abstract_jb:hook_event_cb() saves the interval in a variable named "now" which is confusing in itself. * "now" is defined as an unsigned int which converts the negative value returned from ast_tvdiff_ms() to a large positive value. * fixed_jb_get()'s parameter is defined as a signed int so the interval gets converted back to a negative value. * fixed_jb_get()'s assert is NOT an ast_assert but a direct define that points to the system assert() so it triggers even in production mode. So... * hook_event_cb()'s "now" was renamed to "relative_frame_start" and changed to an int64_t. * hook_event_cb() now checks for a negative value right after retrieving both the current and framedata timestamps and just returns the frame if the difference is negative. * fixed_jb_get()'s local define of ASSERT() was changed to call ast_assert() instead of the system assert(). ASTERISK-29480 Reported by: Dan Cropp Change-Id: Ic469dec73c2edc3ba134cda6721a999a9714f3c9
This commit is contained in:
parent
1e5a2cfe30
commit
bc973bd719
|
@ -954,7 +954,7 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
{
|
||||
struct jb_framedata *framedata = data;
|
||||
struct timeval now_tv;
|
||||
unsigned long now;
|
||||
int64_t relative_frame_start;
|
||||
int putframe = 0; /* signifies if audio frame was placed into the buffer or not */
|
||||
|
||||
switch (event) {
|
||||
|
@ -1064,7 +1064,17 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
}
|
||||
|
||||
now_tv = ast_tvnow();
|
||||
now = ast_tvdiff_ms(now_tv, framedata->start_tv);
|
||||
relative_frame_start = ast_tvdiff_ms(now_tv, framedata->start_tv);
|
||||
if (relative_frame_start < 0) {
|
||||
/*
|
||||
* The only way for this to happen is if the system time has
|
||||
* stepped backwards between the time framedata->start_tv was
|
||||
* set and now. Think an ntpd or systemd-timesyncd adjustment.
|
||||
*
|
||||
* Just pass the frame through.
|
||||
*/
|
||||
return frame;
|
||||
}
|
||||
|
||||
if (frame->frametype == AST_FRAME_VOICE) {
|
||||
int res;
|
||||
|
@ -1084,9 +1094,9 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
}
|
||||
if (!framedata->first) {
|
||||
framedata->first = 1;
|
||||
res = framedata->jb_impl->put_first(framedata->jb_obj, jbframe, now);
|
||||
res = framedata->jb_impl->put_first(framedata->jb_obj, jbframe, relative_frame_start);
|
||||
} else {
|
||||
res = framedata->jb_impl->put(framedata->jb_obj, jbframe, now);
|
||||
res = framedata->jb_impl->put(framedata->jb_obj, jbframe, relative_frame_start);
|
||||
}
|
||||
|
||||
if (res == AST_JB_IMPL_OK) {
|
||||
|
@ -1104,7 +1114,7 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
int res;
|
||||
long next = framedata->jb_impl->next(framedata->jb_obj);
|
||||
|
||||
/* If now is earlier than the next expected output frame
|
||||
/* If relative_frame_start is earlier than the next expected output frame
|
||||
* from the jitterbuffer we may choose to pass on retrieving
|
||||
* a frame during this read iteration. The only exception
|
||||
* to this rule is when an audio frame is placed into the buffer
|
||||
|
@ -1113,8 +1123,8 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
* doing this we are able to feed off the timing of the input frames
|
||||
* and only rely on our jitterbuffer timer when frames are dropped.
|
||||
* During testing, this hybrid form of timing gave more reliable results. */
|
||||
if (now < next) {
|
||||
long int diff = next - now;
|
||||
if (relative_frame_start < next) {
|
||||
long int diff = next - relative_frame_start;
|
||||
if (!putframe) {
|
||||
return frame;
|
||||
} else if (diff >= framedata->timer_interval) {
|
||||
|
@ -1124,7 +1134,7 @@ static struct ast_frame *hook_event_cb(struct ast_channel *chan, struct ast_fram
|
|||
|
||||
ast_frfree(frame);
|
||||
frame = &ast_null_frame;
|
||||
res = framedata->jb_impl->get(framedata->jb_obj, &frame, now, framedata->timer_interval);
|
||||
res = framedata->jb_impl->get(framedata->jb_obj, &frame, relative_frame_start, framedata->timer_interval);
|
||||
switch (res) {
|
||||
case AST_JB_IMPL_OK:
|
||||
/* got it, and pass it through */
|
||||
|
|
|
@ -41,7 +41,7 @@
|
|||
#ifdef FIXED_JB_DEBUG
|
||||
#define ASSERT(a)
|
||||
#else
|
||||
#define ASSERT(a) assert(a)
|
||||
#define ASSERT(a) ast_assert(a)
|
||||
#endif
|
||||
|
||||
/*! \brief private fixed_jb structure */
|
||||
|
|
Loading…
Reference in New Issue