[Asterisk-code-review] Frame deferral: Will this be the last rework on it? (asterisk[13])

[Mark Michelson]

Mark Michelson has posted comments on this change. ( https://gerrit.asterisk.org/4771 )

Change subject: Frame deferral: Will this be the last rework on it?
......................................................................


Patch Set 1: Code-Review-1

(2 comments)

This change causes some confbridge tests to fail. Specifically, confbridge_result, confbridge_nominal, confbridge_dynamic_menus, and muted_conference_start_muted fail with this change applied. confbridge_recording "passes" but spits out a bunch of error messages when run. Without this changeset applied, these tests pass against current 13.

I ran the confbridge tests because they served as a canary in the coal mine previously when writing frame deferral changes. I'm not 100% sure what in this patch is causing those tests to fail, but the failures are consistent. The only common thread I can see between them all is that they are the confbridge tests that involve sending DTMF commands.

https://gerrit.asterisk.org/#/c/4771/1/main/channel.c
File main/channel.c:

PS1, Line 1194: 		/* Write to the alert pipe for each added frame */
              : 		while (new_frames--) {
              : 			if (ast_channel_alert_write(chan)) {
              : 				ast_log(LOG_WARNING, "Unable to write to alert pipe on %s (qlen = %u): %s!\n",
              : 					ast_channel_name(chan), queued_frames, strerror(errno));
              : 				break;
              : 			}
              : 		
This change is going to have some knock-on effects due to the way ast_read() works. I've highlighted it below.


PS1, Line 3976: 		if (AST_LIST_NEXT(f, frame_list)) {
              : 			ast_queue_frame(chan, AST_LIST_NEXT(f, frame_list));
              : 			ast_frfree(AST_LIST_NEXT(f, frame_list));
              : 			AST_LIST_NEXT(f, frame_list) = NULL;
              : 		}
This bit right here is why the alert pipe was only written to once during an ast_queue_frame(), even if multiple frames were being queued. Any frames beyond the one being returned by ast_read() are re-queued onto the channel, resulting in the alert pipe being re-written to.

If an initial ast_queue_frame() queues, say, 5 frames onto the channel. Then the first frame will be read and the other 4 frames will be re-queued, resulting in a new write to the alert pipe. Then when ast_read() is called again, the first frame is read and the remaining 3 frames are re-queued, etc. This results in 5 total writes and reads of the alert pipe.

By changing so that the alert pipe is written for each frame queued, the number of times the alert pipe gets written to is likely to be much larger than anticipated. In the case where 5 frames are initially queued onto the channel, you'd end up writing to the alert pipe 15 times (5 + 4 + 3 + 2 + 1) in order to get those 5 frames read.


-- 
To view, visit https://gerrit.asterisk.org/4771
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: If8383a8b23ba9a335c138a9c0e79fca24b78343c
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: Mark Michelson <mmichelson at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: Yes