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

Add reason of track being added/removed in onremotetrack in janus.js #3150

Merged
merged 3 commits into from
Feb 8, 2023

Conversation

marekpiechut
Copy link
Contributor

Added a metadata object to janus.js onremotetrack callbacks with reason why track is being added/removed. Thanks to that users can decide if they want to handle the call or ignore it.

It can be used to fix blinking screen-sharing sessions as discussed here: https://groups.google.com/g/meetecho-janus/c/bZaQA4MqJcs
(user is being notified about track removal, when it's only a mute call)

After the change onremotetrack call will lok like this:

 onremotetrack: function(track, mid, added, metadata) {
            // A remote track (working PeerConnection!) with a specific mid has just been added or removed
            // You can query metadata to get some more information on why track was added or removed
            // metadata fields:
            //   - reason: 'created' | 'ended' | 'mute' | 'unmute'
        },

Change should be backwards compatible, as it's only adding a new parameter to callbacks that will be ignored by current client code.

I have tested demo pages with the change and it looks like everything is fine.
Also updated documentation.

@januscla
Copy link

Thanks for your contribution, @marekpiechut! Please make sure you sign our CLA, as it's a required step before we can merge this.

@lminiero
Copy link
Member

Thanks! Not sure I like reason as a name for the property (maybe status?) but that's semantics. In principle I think the feature makes sense.

@lminiero
Copy link
Member

As a side note, the demos should all have the signature of the callback updated, though. A ton of people won't look at janus.js or the documentation, and will assume the demos are the "verb". If they don't see the property there, they won't know it exists.

@lminiero lminiero added the multistream Related to Janus 1.x label Jan 16, 2023
@marekpiechut
Copy link
Contributor Author

Thanks for the review @lminiero. On Monday I'll go through the demos and update signature.
Sorry that it takes so long, but it's been a busy week.

@lminiero
Copy link
Member

Sorry that it takes so long, but it's been a busy week.

No problem, take your time!

@marekpiechut
Copy link
Contributor Author

I've added metadata to all demos onremotetrack callbacks. Most of them just display it in debug (had no good idea to actually showcase it).

For screen-sharing demo I've added code to ignore mute/unmute events with description why it might be useful (just like in my issue with blinking screen-sharing streams).

I've also found that in vp9svctest.js was using undeclared doSimulcast variable. This was crashing the demo. I've removed it and just disabled simulcast for this demo.

Let me know if something is still missing.

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.

Thanks! I added a few small notes.

html/audiobridgetest.js Outdated Show resolved Hide resolved
html/canvas.js Outdated Show resolved Hide resolved
html/screensharingtest.js Outdated Show resolved Hide resolved
html/screensharingtest.js Outdated Show resolved Hide resolved
@marekpiechut
Copy link
Contributor Author

@lminiero I think I have fixed all the code style issues. I got too used to auto-formatters :)

@lminiero
Copy link
Member

lminiero commented Feb 4, 2023

Thanks! Abroad for FOSDEM but I'll have a look as soon as I get back on Monday 💪

@lminiero
Copy link
Member

lminiero commented Feb 8, 2023

Thanks, merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
multistream Related to Janus 1.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants