Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix caller and callee both simulcast or just one side simucast fail and fix the substream status show error on the webpage #2671

Merged
merged 19 commits into from
May 25, 2021

Conversation

lucylu-star
Copy link
Contributor

  1. fix caller and callee both simulcast fail.
  2. support just callee simulcast ok.
  3. fix the substream status show error on the webpage.

@lminiero
Copy link
Member

Simulcast cannot be enabled by the receiving party, as far as I know, because the simulcast envelope is created when the transceiver is created. Since the transceiver is created by the PeerConnection and not by the browser, in case of a callee, simulcast cannot be added there.

@lucylu-star
Copy link
Contributor Author

As you said, callee cannot simulcast. I revert some.
But, now have the bug that is caller simulcast failed and substream level status shows error when videocall.
I think the new pull request fix the issue.

@lucylu-star
Copy link
Contributor Author

Only modify janus_videocall.c

Copy link
Member

@lminiero lminiero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some new notes inline. Thanks!

@@ -786,7 +786,7 @@ void janus_videocall_incoming_rtp(janus_plugin_session *handle, janus_plugin_rtp
json_t *result = json_object();
json_object_set_new(result, "event", json_string("simulcast"));
json_object_set_new(result, "videocodec", json_string(janus_videocodec_name(session->vcodec)));
json_object_set_new(result, "substream", json_integer(session->sim_context.substream));
json_object_set_new(result, "substream", json_integer(peer->sim_context.substream));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is wrong. The substream returned here is the substream the user (so session) wants from the peer. Substream and temporal info are always receiver related.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

disagree.
I think peer->sim_context.substream is right. you can check it seeing the substream status on videocall webpage. I have verified it. Using peer, the showing is ok. if using session, the status showing is abnormal.
Peer is the callee. The substream returned here is what substream the callee wants. It is showed on webpage.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah you're rght, sorry: this is an event we're sending to peer, not session as I thought 👍 The session->vcodec above threw me off.

@@ -798,7 +798,7 @@ void janus_videocall_incoming_rtp(janus_plugin_session *handle, janus_plugin_rtp
json_t *result = json_object();
json_object_set_new(result, "event", json_string("simulcast"));
json_object_set_new(result, "videocodec", json_string(janus_videocodec_name(session->vcodec)));
json_object_set_new(result, "temporal", json_integer(session->sim_context.templayer));
json_object_set_new(result, "temporal", json_integer(peer->sim_context.templayer));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

session->ssrc[2] = json_integer_value(json_object_get(msg_simulcast, "ssrc-2"));
int rid_ext_id = -1;
janus_rtp_simulcasting_prepare(msg_simulcast, &rid_ext_id, session->ssrc, session->rid);
session->sim_context.rid_ext_id = rid_ext_id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I pointed out, callees cannot do simulcast. It's likely the whole code above (the one already in the repo) is redundant and should be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I will remove it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As soon as you remove tis part here, it's good to merge for me 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: I can get rid of the whole existing (and unneeded) simulcast check for callees myself later.

peer->ssrc[i] = 0;
g_free(peer->rid[0]);
peer->rid[0] = NULL;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only fix that I think does belong here: in fact, it was incorrectly disabling the previously configured simulcast settings for the peer (the caller in this instance).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@lucylu-star
Copy link
Contributor Author

just remove several unneeded lines. Why auto check failed?

@lminiero
Copy link
Member

It looks like an issue pulling libnice from gitlab, so not caused by your patch. Hopefully it was a temporary error, I'll try to restart the automated build later.

@lminiero
Copy link
Member

Thanks, merging! 👍

@lminiero lminiero merged commit 7b010cd into meetecho:master May 25, 2021
BogdanovKirill pushed a commit to 3dEYE/janus-gateway that referenced this pull request Jun 10, 2021
commit 9c9d335
Author: Lionel Nicolas <lionelnicolas@users.noreply.github.com>
Date:   Thu Jun 10 03:04:43 2021 -0400

    Fix streaming plugin mutex unlock when disabling mountpoint (meetecho#2690)

commit 2d83e96
Author: Yurii Cherniavskyi <yurii.cherniavskyi@gmail.com>
Date:   Mon Jun 7 16:02:41 2021 +0300

    Fix SIP plugin unhold request docs typo (meetecho#2688)

commit 2cd0118
Author: August Black <augustblack@gmail.com>
Date:   Mon Jun 7 01:10:49 2021 -0700

    minor adjustment to the audiobridge docs (meetecho#2687)

commit de2117e
Author: nicolasduteil <nduteil@freedev.org>
Date:   Tue Jun 1 11:26:29 2021 +0200

    fix: [janus_sip] Fix "call_id" property in "missed_call" events (meetecho#2679)

commit 9eeeb38
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Mon May 31 15:57:41 2021 +0200

    Fix status vector parsing for incoming twcc feedbacks (resolves meetecho#2677).

commit 8a25f6e
Merge: d3b39b9 394fb48
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Fri May 28 13:29:54 2021 +0200

    Merge pull request meetecho#2675 from kmeyerhofer/actions/fix

    GH Actions, fix variable name

commit d3b39b9
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Fri May 28 11:09:30 2021 +0200

    Fixed race condition in VideoRoom

commit 394fb48
Author: Kurt Meyerhofer <k@kcmr.io>
Date:   Thu May 27 14:52:08 2021 -0600

    Fixes variable name.

commit b45cd37
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu May 27 18:31:55 2021 +0200

    Clarify that libnice 0.1.18 is recommended

commit 5757a37
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Thu May 27 17:08:17 2021 +0200

    Spatial audio support in AudioBridge via stereo mixing (meetecho#2446)

commit 161fe7a
Author: Luca Barbato <luca.barbato@gmail.com>
Date:   Thu May 27 15:29:01 2021 +0200

    Cleanup avformat-based preprocessors (meetecho#2665)

commit 7b010cd
Author: lucylu-star <78361868+lucylu-star@users.noreply.github.com>
Date:   Tue May 25 17:09:40 2021 +0800

    Fixed broken simulcast support in VideoCall plugin (meetecho#2671)

commit 4ae44a4
Author: nicolasduteil <nduteil@freedev.org>
Date:   Mon May 24 17:57:34 2021 +0200

    feat: support for custom call-id in subscribe request + add 'call_id' property to subscribe & notify related events (meetecho#2664)

commit 4294f20
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon May 24 11:02:48 2021 +0200

    Fixed missing macro when using pthread mutexes (fixes meetecho#2666)

commit f22ab0d
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed May 19 12:03:32 2021 +0200

    Fixed warning

commit b3f3f17
Author: Alessandro Toppi <atoppi@meetecho.com>
Date:   Tue May 18 12:10:47 2021 +0200

    Remove duplicated flag for fuzzing coverage.

commit 4a7560c
Author: nu774 <honeycomb77@gmail.com>
Date:   Fri May 14 00:26:36 2021 +0900

    janus-pp-rec: support HEVC AP(aggregation packet) (meetecho#2662)

commit 5db4be2
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Wed May 12 17:43:43 2021 +0200

    Fixed out of bounds array access

commit 69f56f4
Author: nicolasduteil <nduteil@freedev.org>
Date:   Tue May 11 14:36:22 2021 +0200

    feat: support for SUBSCRIBE expiry (Expires header) in sip plugin (meetecho#2661)

commit b047ccf
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Mon May 10 09:33:27 2021 +0200

    Fixed types

commit f8e8c5e
Author: Chris Wiggins <chris@wiggins.nz>
Date:   Mon May 10 19:26:45 2021 +1200

    RabbitMQ Transport Reconnect Logic (meetecho#2651)

commit 280e8e4
Author: Lorenzo Miniero <lminiero@gmail.com>
Date:   Fri May 7 12:54:30 2021 +0200

    Add per-participant recording options in AudioBridge to join API as well
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants