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: Missed "cname" attribute in SDP jingle #1764

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mnasyrov
Copy link
Contributor

@mnasyrov mnasyrov commented Oct 21, 2021

Added cname attribute to SDP jingle.

The missed cname causes receiving PeerConnection's track without a media stream in not-latest Chromium-based browsers.

@saghul
Copy link
Member

saghul commented Oct 21, 2021

Ping @jallamsetty1

@saghul
Copy link
Member

saghul commented Oct 21, 2021

Jenkins please test this please.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@paweldomas
Copy link
Member

I don't think we want cnames to be advertised. Aren't we filtering that in jicofo already? See jitsi/jicofo@1681586

@paweldomas
Copy link
Member

Uncaught TypeError: Cannot destructure property 'id' of 'undefined' as it is undefined.
    at Object.Yt.isReactNative.getStreamID (lib-jitsi-meet.min.js:2)
    at Function.getStreamID (lib-jitsi-meet.min.js:2)
    at Xo._remoteTrackAdded (lib-jitsi-meet.min.js:2)
    at Xo._usesUnifiedPlan.onTrack (lib-jitsi-meet.min.js:2)
    at RTCPeerConnection.s (lib-jitsi-meet.min.js:2)

What is this stack trace? Are you running custom lib-jitsi-meet?
Cname is not meant to be a stream ID

@jallamsetty1
Copy link
Member

@mnasyrov, Can you please give us an example of where this breaks ? Jicofo doesn't advertise cnames anymore as msid is the only param that needs to be set on the browser in the a:ssrc line and that value corresponds with the stream id. All browsers have already dropped generation of the cnames in the local description.

@paweldomas
Copy link
Member

There must be something wrong because it goes through the react native flow on the browser according to the stack

@jallamsetty1
Copy link
Member

Just read through the original issue. @mnasyrov Can you please grab the remote description that is being set on the peerconnection before the track event is fired ?

@paweldomas
Copy link
Member

It is also important to see if 'isReactNative' flows are not activated, because this has many different consequences in different places of the app.

@mnasyrov
Copy link
Contributor Author

@paweldomas
It is not isReactNative flow. There is the simple getStreamID() function which is pointed by the stack trace:
Screenshot 2021-10-21 at 22 41 22

@jallamsetty1
Here is the remote description:
Screenshot 2021-10-21 at 22 46 29

@paweldomas
Copy link
Member

@mnasyrov then what is this part of the stack:

Object.Yt.isReactNative.getStreamID ([REMOVED]/main.34a3d636.js:257:385988)

The thing is that the getStreamID can be defined different way for each platform:

this.getStreamID = function({ id }) {

From the stack it still looks like it may be going through the RN flow, so can you debug this part?

@jallamsetty1
Copy link
Member

@mnasyrov, Are you using an older version of Jicofo on your deployment ? This doesn't fix the issue with latest JIcofo as it appears that latest Jicofo doesn't propagate the cname to the other endpoints even if the client adds this param in the source signaling.

@mnasyrov
Copy link
Contributor Author

@jallamsetty1 jicofo 6173 is used

@mnasyrov
Copy link
Contributor Author

mnasyrov commented Oct 21, 2021

@paweldomas This is. because of a js compressor, here is a screenshot with the formatted code:
Screenshot 2021-10-21 at 23 54 17

@mnasyrov
Copy link
Contributor Author

@jallamsetty1

This doesn't fix the issue with latest JIcofo as it appears that latest Jicofo doesn't propagate the cname to the other endpoints even if the client adds this param in the source signaling.

Could you say the number of the latest Jicofo which will not work with the fix?

@jallamsetty1
Copy link
Member

jallamsetty1 commented Oct 25, 2021

@jallamsetty1

This doesn't fix the issue with latest JIcofo as it appears that latest Jicofo doesn't propagate the cname to the other endpoints even if the client adds this param in the source signaling.

Could you say the number of the latest Jicofo which will not work with the fix?

jitsi/jicofo@1681586 is the commit that changed the behavior in Jicofo, so 6433 looks like. Thanks for providing me the remote descriptions being set on the client, I don't see anything wrong with them.
Will you be able to share a link where I can reproduce this issue ?

@mnasyrov
Copy link
Contributor Author

@jallamsetty1

jitsi/jicofo@1681586 is the commit that changed the behavior in Jicofo, so 6433 looks like.

Thank you for the details.

Will you be able to share a link where I can reproduce this issue ?

Please open a conference with two people at https://meet.jit.si in Chromium 79:

Screenshot 2021-10-25 at 22 50 19

@sapkra sapkra added the bug Generic issue label Dec 3, 2021
@agustinbranaf
Copy link

@mnasyrov interested in finding a solution to this since we use chromium in our App, did you ever find a fix/workaround to this issue?

@paweldomas
Copy link
Member

@mnasyrov interested in finding a solution to this since we use chromium in our App, did you ever find a fix/workaround to this issue?

You should be able to use Chromium just fine, but with a reasonably latest version. I don't know which one exactly.

@mnasyrov
Copy link
Contributor Author

@mnasyrov interested in finding a solution to this since we use chromium in our App, did you ever find a fix/workaround to this issue?

We use a custom lib-jitsi patched by this PR locally with a previous version of Jicofo. Soon, I hope, we'll update requirements for client browsers, and discard this fix as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Generic issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants