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

Multistream html fixes #2809

Merged
merged 1 commit into from
Nov 23, 2021
Merged

Multistream html fixes #2809

merged 1 commit into from
Nov 23, 2021

Conversation

aet
Copy link

@aet aet commented Nov 16, 2021

I was doing some testing against #2211 and browsed around the differences between master/multistream JS code examples. I noticed that mediaState arguments on some of the examples are out of order

Follow-up notes/questions:

  • html/nosiptest.js has onlocalstream callback and onremotetrack, but didn't update onlocalstream variant into onlocaltrack
  • Janus Admin page is showing [object Object] in few places when JSON pretty printing is enabled IIRC - due to streams moved into JSON media arrays :)
  • mediaState/slowLink are the only callbacks where mid parameter is not the first one after handle, should this be unified?
  • There's no question that once multistream is in, there will be breaking changes, but alternatively should the mid parameters be the last one on all callbacks instead and try to preserve existing arguments order when possible?

@lminiero
Copy link
Member

lminiero commented Nov 16, 2021

Thanks for looking into this! Answering the different points:

html/nosiptest.js has onlocalstream callback and onremotetrack, but didn't update onlocalstream variant into onlocaltrack

Probably a leftover, should be easy to fix in case, I'll have a look later.

Janus Admin page is showing [object Object] in few places when JSON pretty printing is enabled IIRC - due to streams moved into JSON media arrays :)

To be honest I was planning to get rid of the "pretty print" part entirely, since it needs to be aware of what can be there, and that can change (as it did here). Besides, it's actually less pretty than the JSON ostensibly 😆

mediaState/slowLink are the only callbacks where mid parameter is not the first one after handle, should this be unified?

I think that was done to preserve a semblance of backwards compatibility, especially in case you only do a single audio and video stream anyway: in that case, you don't really care about the mid. The new onlocaltrack and onremotetrack are brand new callbacks, instead, so we could use any signature we wanted.

There's no question that once multistream is in, there will be breaking changes, but alternatively should the mid parameters be the last one on all callbacks instead and try to preserve existing arguments order when possible?

Actually I think that semantically it's more correct for mid to appear first, since it's what really identifies a specific track.

lminiero added a commit that referenced this pull request Nov 16, 2021
@aet
Copy link
Author

aet commented Nov 18, 2021

I agree mid argument appearing first, but might as well asking about alternatives :)

Equally fine with either mid appearing as first argument or last for mediaState and slowLink, but for consistency I would go with mid appearing first on them too. Currently only mvideoroom.js is declaring the mediaState signature the way janus.js is using it.

@lminiero
Copy link
Member

Merging, thanks!

@lminiero lminiero merged commit 259ec96 into meetecho:multistream Nov 23, 2021
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