preserve supplemental codecs in publish mode

[orgads]

Add RED (RFC 2198) codec definition to codeclib as a supplemental codec. Modify codec_store_accept_one() to preserve supplemental codecs whose clock rate matches the accepted primary codec. Skip supplemental codecs in codec_store_is_full_answer() since they are optional in SDP answers.

[orgads]

Rebased on top of #2101 to fix CI.

[orgads]

@rfuchs ping

[rfuchs]

Seems like this is a little misbehaved in that it accepts all supplemental codecs, and not just the ones matching the accepted primary codec.

When I publish with

m=audio 21326 RTP/AVP 111 9 0 8 110 101 96 97
a=rtpmap:111 opus/48000/2
a=rtcp-fb:111 transport-cc
a=fmtp:111 minptime=10;useinbandfec=1
a=rtpmap:9 G722/8000
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:110 telephone-event/48000
a=rtpmap:101 telephone-event/8000
a=rtpmap:96 RED/8000
a=rtpmap:97 RED/48000

it answers with

m=audio 34470 RTP/AVP 111 110 101 96 97
c=IN IP4 192.168.1.66
a=rtpmap:111 opus/48000/2
a=fmtp:111 useinbandfec=1; minptime=10
a=rtcp-fb:111 transport-cc
a=rtpmap:110 telephone-event/48000
a=rtpmap:101 telephone-event/8000
a=rtpmap:96 RED/8000
a=rtpmap:97 RED/48000

The 48000 ones aren't supposed to be there.

Also the test outputs should be exact matches as much as possible, with regex matches only where variable output is expected (e.g. port numbers).

[orgads]

Good catch! Should be fixed now.

[orgads]

hmm... Turns out our SBC sends hardcoded RED/8000, which doesn't always match the primary codec. Does it make sense to keep "single" RED even if it doesn't match the sample rate?

We will fix the SBC, but this will take time. So we can either keep using a patched version of rtpengine, or work out a compatible solution...

[rfuchs]

Pff... Does it provide a a=fmtp for the red encoding, indicating which other codecs it applies to?

Does it make sense to keep "single" RED even if it doesn't match the sample rate?

I suppose so? If the offerer doesn't agree with it, then technically they should just ignore it. OTOH I wouldn't be surprised if this lead to failures in some other fringe cases 🤦

[orgads]

Pff... Does it provide a a=fmtp for the red encoding, indicating which other codecs it applies to?

Unfortunately not. Sample SDP:

v=0
o=AudiocodesGW 870945942 1165690303 IN IP4 10.2.3.4
s=SBC-Call
c=IN IP4 10.2.3.4
t=0 0
m=audio 24400 RTP/AVP 104 97 101
c=IN IP4 10.2.3.4
a=ptime:20
a=sendonly
a=rtpmap:104 SILK/16000
a=fmtp:104 maxaveragebitrate=50000; useinbandfec=0; minptime=20
a=rtpmap:97 RED/8000
a=rtpmap:101 telephone-event/16000
a=fmtp:101 0-15,16

Does it make sense to keep "single" RED even if it doesn't match the sample rate?

I suppose so? If the offerer doesn't agree with it, then technically they should just ignore it. OTOH I wouldn't be surprised if this lead to failures in some other fringe cases 🤦

I'll need to discuss this internally first. Since we won't be updating to upstream rtpengine soon (and when we will, I hope to get the SBC fix along with that), I suppose you can just merge it as it is now (if it is acceptable of course). We'll use the patched version until then.

Thank you!

[orgads]

Rebased to resolve the conflict.

[rfuchs]

All right, thank you.

We could use more/proper support for RED anyway, but this is a wishlist item for now.

[orgads]

@rfuchs After some internal discussion, we're hesitant to change the SBC behavior since it could break existing deployments.

We did notice that although the SDP advertises RED/8000, the actual media is transmitted at a 16kHz sample rate. In other words, the SDP is technically inaccurate, but the media itself is correct and no issues have been reported so far.

Given this context, and considering our SIPREC use case where the SDP already represents the negotiated media and should not contain conflicting media descriptions like in your example, our expectation is to record all advertised media streams and preserve their metadata.

With that in mind, do you have an alternative approach to suggest that would allow us to simply include everything without applying this filtering?

[rfuchs]

In cases like this we typically use a flag to signal to rtpengine to alter its default behaviour. We already have a few "allow asymmetric ..." flags for similar cases. One of them could probably even be reused for publish to support this.

The problem with inconsistent SDP offers is that rtpengine has no way to reliably know what the offerer meant. If the SDP offers "8=PCMA/8000" and "96=opus/48000" and then "97=red/8000", does this mean that 97 should be used for Opus as well, or does it mean that RED is only applied to PCMA? Receiving the RTP is not a problem is that makes it obvious, but that doesn't help in the decision about what to put in the answer SDP, and responding in the wrong may lead to a failure.

[orgads]

Hi @rfuchs. Would it be an acceptable change to add all mode to "codec": "accept" similar to any, only that it accepts everything, and not only the first coder? We also thought of 2 modes: all-supported and all.

We need to accept and record everything to pcap, and we post-process it later.

What do you say?

[rfuchs]

Hi @rfuchs. Would it be an acceptable change to add all mode to "codec": "accept" similar to any, only that it accepts everything, and not only the first coder? We also thought of 2 modes: all-supported and all.

We need to accept and record everything to pcap, and we post-process it later.

What do you say?

I would be ok with that, yes.

[orgads]

Thank you. Pushed:

• support 'all' codec accept mode #2105