Correctly update local media format ids to match those in the offer #498

juha-h · 2022-08-28T09:45:58Z

Before this patch, when offer was received, local id of common media was changed to match that in the offer. As result, the id could conflict with some other local media id.

I don't see a need for the id change. Since this is a very serious bug, my suggestion is to the merge this PR. If someone thinks the id needs to be changed then he/she can make another PR that fixes the conflict.

After this PR, the re-INVITE test of baresip/baresip#2073 worked fine.

juha-h · 2022-08-28T15:44:21Z

Here is text from https://www.rfc-editor.org/rfc/rfc3264#page-10:

   In the case of RTP, if a particular codec was referenced with a
   specific payload type number in the offer, that same payload type
   number SHOULD be used for that codec in the answer.  Even if the same
   payload type number is used, the answer MUST contain rtpmap
   attributes to define the payload type mappings for dynamic payload
   types, and SHOULD contain mappings for static payload types.  The
   media formats in the "m=" line MUST be listed in order of preference,
   with the first format listed being preferred.  In this case,
   preferred means that the offerer SHOULD use the format with the
   highest preference from the answer.

   Although the answerer MAY list the formats in their desired order of
   preference, it is RECOMMENDED that unless there is a specific reason,
   the answerer list formats in the same relative order they were
   present in the offer.  In other words, if a stream in the offer lists
   audio codecs 8, 22 and 48, in that order, and the answerer only
   supports codecs 8 and 48, it is RECOMMENDED that, if the answerer has
  no reason to change it, the ordering of codecs in the answer be 8,
   48, and not 48, 8.  This helps assure that the same codec is used in
   both directions.

So the same payload number SHOULD be used, but is not mandatory. Better to fix the bug now and then, if someone has time, make numbering match the offer.

juha-h · 2022-08-28T17:24:51Z

Tested the PR also against Linphone and it worked fine.

juha-h · 2022-08-28T18:24:24Z

Works also against Grandstream Wave.

sreimers · 2022-08-28T18:58:49Z

I think we should keep the old behavior, maybe this (untested) could fix the bug:

diff --git a/src/sdp/media.c b/src/sdp/media.c
index 6171ffcc..e2efff30 100644
--- a/src/sdp/media.c
+++ b/src/sdp/media.c
@@ -260,6 +260,7 @@ void sdp_media_align_formats(struct sdp_media *m, bool offer)
 {
        struct sdp_format *rfmt, *lfmt = NULL;
        struct le *rle, *lle;
+       int pt_offer = RTP_DYNPT_START;
 
        if (!m || m->disabled || !sa_port(&m->raddr) || m->fmt_ignore)
                return;
@@ -307,6 +308,7 @@ void sdp_media_align_formats(struct sdp_media *m, bool offer)
 
                rfmt->ref = lfmt->ref;
 
+               /* Use payload type from offer - RFC3264 - 6.1 */
                if (offer) {
                        mem_deref(lfmt->id);
                        lfmt->id = mem_ref(rfmt->id);
@@ -314,11 +316,13 @@ void sdp_media_align_formats(struct sdp_media *m, bool offer)
 
                        list_unlink(&lfmt->le);
                        list_append(&m->lfmtl, &lfmt->le, lfmt);
+                       if (lfmt->pt > pt_offer)
+                               pt_offer = lfmt->pt;
                }
        }
 
+       /* Move unsupported codecs to the end of the list and recalculate pt */
        if (offer) {
-
                for (lle=m->lfmtl.tail; lle; ) {
 
                        lfmt = lle->data;
@@ -326,6 +330,12 @@ void sdp_media_align_formats(struct sdp_media *m, bool offer)
                        lle = lle->prev;
 
                        if (lfmt && !lfmt->sup) {
+                               if (lfmt->pt >= RTP_DYNPT_START) {
+                                       mem_deref(lfmt->id);
+                                       lfmt->pt = ++pt_offer;
+                                       re_sdprintf(&lfmt->id, "%i", lfmt->pt);
+                               }
+
                                list_unlink(&lfmt->le);
                                list_append(&m->lfmtl, &lfmt->le, lfmt);
                        }

juha-h · 2022-08-28T19:12:16Z

Sebastian Reimers writes:

I think we should keep the old behavior, maybe this (untested) could fix the bug:

I can test tomorrow. By the way, does answer from baresip now include all matching codecs with matching ids and order or only one codec: ``` Although the answerer MAY list the formats in their desired order of preference, it is RECOMMENDED that unless there is a specific reason, the answerer list formats in the same relative order they were present in the offer. In other words, if a stream in the offer lists audio codecs 8, 22 and 48, in that order, and the answerer only supports codecs 8 and 48, it is RECOMMENDED that, if the answerer has no reason to change it, the ordering of codecs in the answer be 8, 48, and not 48, 8. ```

juha-h · 2022-08-29T06:15:23Z

Thanks for the diff. I made some tests and it worked OK. One thing that I noticed is that when re-INVITE is sent, all codecs are included on the m-line, which in my opinion is not necessary especially when the order is not correct.

For example, when initial INVITE has

m=audio 44208 RTP/AVP 96 0 8 101
a=rtpmap:96 opus/48000/2
a=fmtp:96 stereo=0;sprop-stereo=0;maxaveragebitrate=28000;cbr=0;useinbandfec=1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000

response correctly has

m=audio 38180 RTP/AVP 96 0 8 101
a=rtpmap:96 opus/48000/2
a=fmtp:96 stereo=0;sprop-stereo=0;maxaveragebitrate=28000;cbr=0;useinbandfec=1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000

Now when answerer sends re-INVITE, also other codecs that answerer supports are included:

m=audio 38180 RTP/AVP 96 0 8 101 18 9 102 103
a=rtpmap:96 opus/48000/2
a=fmtp:96 stereo=0;sprop-stereo=0;maxaveragebitrate=28000;cbr=0;useinbandfec=1
a=rtpmap:0 PCMU/8000
a=rtpmap:8 PCMA/8000
a=rtpmap:101 telephone-event/8000
a=fmtp:101 0-15
a=rtpmap:18 G729/8000
a=rtpmap:9 G722/8000
a=rtpmap:102 AMR/8000
a=rtpmap:103 AMR-WB/16000
a=sendonly

The first four are the common ones from the initial exchange and the rest are the ones that only the answerer supports. However their order is not the one has been defined in answerer's accounts file:

;audio_codecs=AMR-WB/16000/1,AMR/8000/1,opus/16000/1,G722/16000/1,G729/8000/1,PCMU,PCMA;

For example, AMR-WB should be the first extra codec, but it is the last. My suggestion is to use in the re-INVITE only those codecs that were in the initial exchange.

juha-h · 2022-08-29T06:21:02Z

If it is too difficult to use in re-INVITE only codecs of the initial exchange, then maybe the order of the extra codecs could be fixed by not starting from the tail:

for (lle=m->lfmtl.tail; lle; )

juha-h · 2022-08-29T06:41:02Z

My understanding from 8 Modifying the Session is that re-INVITE could simply use the codecs that are defined in the accounts file of the sender, i.e., full set in given order as long as ids of common codecs are the ones used in the initial exchange.

juha-h · 2022-08-29T06:53:15Z

So my suggestion is to remove these two lines:

                        if (lfmt && !lfmt->sup) {
+                               if (lfmt->pt >= RTP_DYNPT_START) {
+                                       mem_deref(lfmt->id);
+                                       lfmt->pt = ++pt_offer;
+                                       re_sdprintf(&lfmt->id, "%i", lfmt->pt);
+                               }
+
//                                list_unlink(&lfmt->le);
//                                list_append(&m->lfmtl, &lfmt->le, lfmt);
                        }

juha-h · 2022-08-29T06:55:37Z

By the way, while testing this, I noticed and when baresip puts audio only call on hold by x, it adds sendonly video stream to the re-INVITE. Is this a good idea or would it be better to stick to the existing media streams?

cspiel1 · 2022-08-29T07:27:28Z

By the way, while testing this, I noticed and when baresip puts audio only call on hold by x, it adds sendonly video stream to the re-INVITE. Is this a good idea or would it be better to stick to the existing media streams?

call_hold() does a

	FOREACH_STREAM
		stream_hold(le->data, hold);

What I remember is that "call on hold" is not mentioned specially in RFC. And there a different way to implement this. Most implementations tell the remote to be quiet and receive e.g. a hold-the-line music. Changes of the video stream are completely application dependent. Baresip has no hold-the-line music and does not send anything to the remote during local call-on-hold. In stream.c functionstream_send():

	if (s->hold)
		return 0;

So, baresip does not send video to the remote while the call is on-hold locally. I suggest not to change this behavior.

Also baresip tells the remote that it does not want to receive video during local on-hold. Maybe we should also not change this behavior.

Your other questions: Maybe @sreimers has more insights to answer these?

juha-h · 2022-08-29T07:34:10Z

This PR does not change on-hold behavior.

juha-h · 2022-08-29T07:39:53Z

@sreimers How does the PR now look to you? It simplifies your diff by keeping the original codec set, but takes pt ids from the initial offer. The result should be fully RFC 3264 compliant.

sreimers · 2022-08-29T07:46:30Z

// list_unlink(&lfmt->le);
// list_append(&m->lfmtl, &lfmt->le, lfmt);

If you remove these lines, unsupported codecs are come first in answer (since matched ones are re-added). I think there is a reason for this (maybe faster codec negotiation), maybe @alfredh can clarify. The code is >10 years old, changing this behavior can lead to problems.

Maybe sdp_media_align_formats offer handling part is the wrong place to fix re-INVITE. Have to investigate further.

juha-h · 2022-08-29T08:11:53Z

Sebastian Reimers writes:

> if (lfmt && !lfmt->sup) { > + if (lfmt->pt >= RTP_DYNPT_START) { > + mem_deref(lfmt->id); > + lfmt->pt = ++pt_offer; > + re_sdprintf(&lfmt->id, "%i", lfmt->pt); > + } > + > // list_unlink(&lfmt->le); > // list_append(&m->lfmtl, &lfmt->le, lfmt); > } If you remove these lines, unsupported codecs are come first in answer (since matched ones are re-added). I think there is a reason for this (maybe faster codec negotiation), maybe @alfredh can clarify. The code is >10 years old, changing this behavior can lead to problems.

See the PR as it is now. I forgot to mention that I also changed the for loop to start from head. The > 10 years code has a bug that needs to be fixed. All that should matter is that after the fix the code conforms to the RFC.

juha-h · 2022-08-29T12:40:42Z

So just to clarify, this PR causes re-INVITEs to use the original local codecs in the original order. Only ids of codecs are changed, since they must match those in the initial offer:

8.3.2 Changing the Set of Media Formats

   The list of media formats used in the session MAY be changed.  To do
   this, the offerer creates a new media description, with the list of
   media formats in the "m=" line different from the corresponding media
   stream in the previous SDP.  This list MAY include new formats, and
   MAY remove formats present from the previous SDP.  However, in the
   case of RTP, the mapping from a particular dynamic payload type
   number to a particular codec within that media stream MUST NOT change
   for the duration of a session.

juha-h · 2022-10-21T14:39:00Z

@sreimers @cspiel1
See baresip/baresip#2279. This PR fixed the codec id conflict, but introduced another issue. Should we revert this while a proper fix is found?

sreimers · 2022-10-21T14:42:53Z

Maybe we can re-add the old behavior. Can you test if my initial proposal works #498 (comment)?

juha-h · 2022-10-21T14:57:55Z

I'm a bit lost on which version of media.c #498 (comment) should be applied to.

juha-h · 2022-10-21T15:12:16Z

Can you provide diff to the current version of media.c?

sreimers · 2022-10-21T18:22:41Z

diff --git a/src/sdp/media.c b/src/sdp/media.c
index 428ba4ef..8c64eb00 100644
--- a/src/sdp/media.c
+++ b/src/sdp/media.c
@@ -313,24 +313,32 @@ void sdp_media_align_formats(struct sdp_media *m, bool offer)
                        mem_deref(lfmt->id);
                        lfmt->id = mem_ref(rfmt->id);
                        lfmt->pt = atoi(lfmt->id ? lfmt->id : "");
+
+                       list_unlink(&lfmt->le);
+                       list_append(&m->lfmtl, &lfmt->le, lfmt);
                        if (lfmt->pt > pt_offer)
                                pt_offer = lfmt->pt;
                }
        }
 
-       /* Recalculate pt of unsupported codecs */
+       /* Recalculate pt and reorder unsupported codecs */
        if (offer) {
 
-               for (lle=m->lfmtl.head; lle; lle=lle->next) {
+               for (lle = m->lfmtl.tail; lle;) {
 
                        lfmt = lle->data;
 
-                       if (lfmt && !lfmt->sup)
+                       lle = lle->prev;
+
+                       if (lfmt && !lfmt->sup) {
                                if (lfmt->pt >= RTP_DYNPT_START) {
                                        mem_deref(lfmt->id);
                                        lfmt->pt = ++pt_offer;
                                        re_sdprintf(&lfmt->id, "%i", lfmt->pt);
                                }
+                               list_unlink(&lfmt->le);
+                               list_append(&m->lfmtl, &lfmt->le, lfmt);
+                       }
                }
        }
 }

juha-h · 2022-10-21T18:45:28Z

Thanks, now the answer has correct codec order. I'll check id conflicts tomorrow.

juha-h · 2022-10-22T07:40:11Z

I made the tests described in baresip/baresip#2073 and they worked fine, i.e., no codec id conflicts. I'll create PR based on the above diff.

Do not change local media format id to match the one in the offer

3f11bd2

juha-h requested a review from sreimers August 28, 2022 13:16

juha-h added the bug Something isn't working label Aug 28, 2022

In re-INVITE, use codecs defined in accounts file

92cbebf

juha-h changed the title ~~Do not change local media format id to match the one in the offer~~ Correctly update local media format ids to match those in the offer Aug 29, 2022

sreimers merged commit b60465c into main Aug 30, 2022

sreimers deleted the sdp_media_align_formats branch August 30, 2022 04:53

juha-h mentioned this pull request Oct 22, 2022

sdp/media: fix reorder codecs (restore old behavior) #583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly update local media format ids to match those in the offer #498

Correctly update local media format ids to match those in the offer #498

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

sreimers commented Aug 28, 2022

juha-h commented Aug 28, 2022 via email

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

cspiel1 commented Aug 29, 2022 •

edited

Loading

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

sreimers commented Aug 29, 2022 •

edited

Loading

juha-h commented Aug 29, 2022 via email

juha-h commented Aug 29, 2022 •

edited

Loading

juha-h commented Oct 21, 2022

sreimers commented Oct 21, 2022

juha-h commented Oct 21, 2022

juha-h commented Oct 21, 2022

sreimers commented Oct 21, 2022

juha-h commented Oct 21, 2022 via email

juha-h commented Oct 22, 2022

Correctly update local media format ids to match those in the offer #498

Correctly update local media format ids to match those in the offer #498

Conversation

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

juha-h commented Aug 28, 2022

sreimers commented Aug 28, 2022

juha-h commented Aug 28, 2022 via email

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

cspiel1 commented Aug 29, 2022 • edited Loading

juha-h commented Aug 29, 2022

juha-h commented Aug 29, 2022

sreimers commented Aug 29, 2022 • edited Loading

juha-h commented Aug 29, 2022 via email

juha-h commented Aug 29, 2022 • edited Loading

juha-h commented Oct 21, 2022

sreimers commented Oct 21, 2022

juha-h commented Oct 21, 2022

juha-h commented Oct 21, 2022

sreimers commented Oct 21, 2022

juha-h commented Oct 21, 2022 via email

juha-h commented Oct 22, 2022

cspiel1 commented Aug 29, 2022 •

edited

Loading

sreimers commented Aug 29, 2022 •

edited

Loading

juha-h commented Aug 29, 2022 •

edited

Loading