[asterisk-dev] RTP Native Bridge Codec Change Handling - Appears to compare immediately after setting equal

[Dave WOOLLEY]

David Woolley wrote:

> we noticed that when the response to a native bridge re-invite changed the codecs, 
> Asterisk didn't seem to react, even though the resulting codecs were incompatible with 
> the other side.

> On looking through the code (rtp.c / rtp_engine.c), it looks like the basic RTP native  bridge 
> loop has a structure like this 
......
> To me this seems me to make it very difficult for a codec change to actually be detected, 
> as when you unroll the loop, the unconditional set precedes the test for changes!

Let's be a bit more concrete.  This is an extract from the branches/11 version, as of Thursday, highlighting the issue.  AAAA marks a line that reads the codec for testing for changes, BBBB marks a line that updates the old values of the codecs after acting on the changes, but CCCC seems to mark a line that can be executed without acting on the changes, and which logically precedes an AAAA line in loop execution order:

static enum ast_bridge_result remote_bridge_loop(struct ast_channel *c0,
.....
	for (;;) {
....
		if (glue1->get_codec) {                                                <<<<<<<<<<<<<< AAAA
			ast_format_cap_remove_all(cap1);
			glue1->get_codec(c1, cap1);
		}
....
		if (glue0->get_codec) {                                                 <<<<<<<<<<<<<< AAAA
			ast_format_cap_remove_all(cap0);
			glue0->get_codec(c0, cap0);
		}
		if (........||(!ast_format_cap_identical(cap1, oldcap1))) {
			if (glue0->update_peer(c0, .....,       cap1, 0)) {
				......
			}
.....
			ast_format_cap_copy(oldcap1, cap1);    <<<<<<<<<<<<<< BBBB
		}
		if (.....|| (!ast_format_cap_identical(cap0, oldcap0))) {
....
			if (glue1->update_peer(c1, ......, cap0, 0)) {
				.....
			}
....
			ast_format_cap_copy(oldcap0, cap0);    <<<<<<<<<<<<<< BBBB
		}
....
		if (!(who = ast_waitfor_n(cs, 2, &ms))) {
			.....
		}
		fr = ast_read(who);
		if (!fr || ((fr->frametype == AST_FRAME_DTMF_BEGIN || fr->frametype == AST_FRAME_DTMF_END) &&
....
			break;
		} else if ((fr->frametype == AST_FRAME_CONTROL) && !(flags & AST_BRIDGE_IGNORE_SIGS)) {
			if ((fr->subclass.integer == AST_CONTROL_HOLD) ||
			    (fr->subclass.integer == AST_CONTROL_UNHOLD) ||
			    (fr->subclass.integer == AST_CONTROL_VIDUPDATE) ||
			    (fr->subclass.integer == AST_CONTROL_SRCUPDATE) ||
			    (fr->subclass.integer == AST_CONTROL_T38_PARAMETERS) ||
				(fr->subclass.integer == AST_CONTROL_UPDATE_RTP_PEER)) {
				if (fr->subclass.integer == AST_CONTROL_HOLD) {
                                                                                 ...... update_peer.......     <<<<< DDDD
				} else if (fr->subclass.integer == AST_CONTROL_UNHOLD ||
                                                                                 ...... update_peer.......     <<<<< DDDD
				}
....
				/* Update codec information */
				if (glue0->get_codec && ast_channel_tech_pvt(c0)) {
					ast_format_cap_remove_all(cap0);
					ast_format_cap_remove_all(oldcap0);
					glue0->get_codec(c0, cap0);                                  <<<<<<< EEEE
					ast_format_cap_append(oldcap0, cap0);         <<<<<<< CCCC

				}
				if (glue1->get_codec && ast_channel_tech_pvt(c1)) {
					ast_format_cap_remove_all(cap1);
					ast_format_cap_remove_all(oldcap1);
					glue1->get_codec(c1, cap1);                                  <<<<<<< EEEE
					ast_format_cap_append(oldcap1, cap1);         <<<<<<< CCCC
				}
.....
			} else if (....
			} else { .....
			}
		} else {
....
	}


Is there any reason why I should not raise this on the issues tracker?

Could anyone indicate what CCCC is trying to achieve that isn't achieved by BBBB, so that I don't break anything when trying to fix it.

Note that whilst the lines marked DDDD do an update, they are only conditionally executed and they are before the read of the codecs at EEEE
-- 
David Woolley
BTS Holdings Plc
Tel: +44 (0)20 8401 9000 Fax: +44 (0)20 8401 9100
http://www.bts.co.uk
(Please ignore any confidentiality notice below.)

BTS Holdings PLC - Registered office: BTS House, Manor Road, Wallington, SM6 0DD - Registered in England: 1517630