Do not create a publisher stream entry for rejected SDP offer m-line.

[PauKerr]

Entering a stream into the publisher instance will cause an m-line in any subsequent subscription request for that publisher to attempt to create an m-line for a codec named 'none', which will cause the operation to fail.

[PauKerr]

Updated the PR based on comments received. Instead of not creating the publisher stream this version checks for a codec value of NONE and does not attempt to create an m-line in the subscription offer for this stream.

[lminiero]

Thanks! I'll review and test as soon as I'm back home from the IETF meeting.

[lminiero]

I'm not convinced this is a proper fix. This seems to be excluding some streams when creating the offer, but if any of those is skipped, this will cause problems relaying media, since each of those janus_videoroom_subscriber_stream instances in the list has a monotonically increasing mindex property. As a result, a subscriber may end up with an SDP that, internally, corresponds to mindexes, e.g., 0, 1, 2, 4, 5: an attempt to call relay_rtp with mindex 4 will relay RTP packets on the wrong stream, since the core indexes will be (and rightly so) 0, 1, 2, 3, 4, while packets with mindex 5 will not be sent at all.

This means that a proper fix should ensure a janus_videoroom_subscriber_stream is not created at all, in case there's no codec associated to the stream.

[lminiero]

@PauKerr could you test this patch in place of your PR, so on the most recent master?

diff --git a/src/plugins/janus_videoroom.c b/src/plugins/janus_videoroom.c
index a9840e74..e11a8733 100644
--- a/src/plugins/janus_videoroom.c
+++ b/src/plugins/janus_videoroom.c
@@ -10528,6 +10528,11 @@ static void *janus_videoroom_handler(void *data) {
 								JANUS_LOG(LOG_WARN, "Skipping disabled m-line...\n");
 								continue;
 							}
+							if((ps->type == JANUS_VIDEOROOM_MEDIA_AUDIO && ps->acodec == JANUS_AUDIOCODEC_NONE) ||
+									(ps->type == JANUS_VIDEOROOM_MEDIA_VIDEO && ps->vcodec == JANUS_VIDEOCODEC_NONE)) {
+								JANUS_LOG(LOG_WARN, "Skipping rejected publisher stream...\n");
+								continue;
+							}
 							janus_videoroom_subscriber_stream *stream = janus_videoroom_subscriber_stream_add_or_replace(subscriber, ps, crossrefid);
 							if(stream) {
 								changes++;
@@ -10591,6 +10596,11 @@ static void *janus_videoroom_handler(void *data) {
 									temp = temp->next;
 									continue;
 								}
+								if((ps->type == JANUS_VIDEOROOM_MEDIA_AUDIO && ps->acodec == JANUS_AUDIOCODEC_NONE) ||
+										(ps->type == JANUS_VIDEOROOM_MEDIA_VIDEO && ps->vcodec == JANUS_VIDEOCODEC_NONE)) {
+									JANUS_LOG(LOG_WARN, "Skipping rejected publisher stream...\n");
+									continue;
+								}
 								janus_videoroom_subscriber_stream *stream = janus_videoroom_subscriber_stream_add_or_replace(subscriber, ps, crossrefid);
 								if(stream) {
 									changes++;

It's basically your same check, but moved to the a "subscribe" or "update" request, meaning that, as discussed in my comment, we're not creating the subscriber stream instance at all for audio/video streams with no codec, which should address my concern.

[PauKerr]

Thanks. I will try this patch.

[PauKerr]

This patch works in my use case. Thanks.

…

On Tue, Nov 21, 2023 at 5:20 AM Lorenzo Miniero ***@***.***> wrote: @PauKerr <https://github.com/PauKerr> could you test this patch in place of your PR, so on the most recent master? diff --git a/src/plugins/janus_videoroom.c b/src/plugins/janus_videoroom.c index a9840e74..e11a8733 100644 --- a/src/plugins/janus_videoroom.c +++ b/src/plugins/janus_videoroom.c @@ -10528,6 +10528,11 @@ static void *janus_videoroom_handler(void *data) { JANUS_LOG(LOG_WARN, "Skipping disabled m-line...\n"); continue; } + if((ps->type == JANUS_VIDEOROOM_MEDIA_AUDIO && ps->acodec == JANUS_AUDIOCODEC_NONE) || + (ps->type == JANUS_VIDEOROOM_MEDIA_VIDEO && ps->vcodec == JANUS_VIDEOCODEC_NONE)) { + JANUS_LOG(LOG_WARN, "Skipping rejected publisher stream...\n"); + continue; + } janus_videoroom_subscriber_stream *stream = janus_videoroom_subscriber_stream_add_or_replace(subscriber, ps, crossrefid); if(stream) { changes++; @@ -10591,6 +10596,11 @@ static void *janus_videoroom_handler(void *data) { temp = temp->next; continue; } + if((ps->type == JANUS_VIDEOROOM_MEDIA_AUDIO && ps->acodec == JANUS_AUDIOCODEC_NONE) || + (ps->type == JANUS_VIDEOROOM_MEDIA_VIDEO && ps->vcodec == JANUS_VIDEOCODEC_NONE)) { + JANUS_LOG(LOG_WARN, "Skipping rejected publisher stream...\n"); + continue; + } janus_videoroom_subscriber_stream *stream = janus_videoroom_subscriber_stream_add_or_replace(subscriber, ps, crossrefid); if(stream) { changes++; It's basically your same check, but moved to the a "subscribe" or "update" request, meaning that, as discussed in my comment, we're not creating the subscriber instance at all for audio/video streams with no codec, which should address my concern. — Reply to this email directly, view it on GitHub <#3260 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACD5FUGC4QHPYLR533Z7DVDYFR56BAVCNFSM6AAAAAA3QCVQNSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMRQGYZDMOBRHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[lminiero]

Thanks! I'll push it upstream and close this PR then.