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

Event::MediaAdded is not fired for medias added in local offer #567

Open
alexlapa opened this issue Sep 26, 2024 · 8 comments
Open

Event::MediaAdded is not fired for medias added in local offer #567

alexlapa opened this issue Sep 26, 2024 · 8 comments

Comments

@alexlapa
Copy link
Collaborator

The MediaAdded event documentation states that

This event fires both for negotiations triggered by a remote or local offer.

Which is not the case, since it is emitted only for media lines added via remote offer, and it seems that firing these event for local offer is explicitly disabled right here: 0, 1.

And, yeah, while this is an apply_answer() function it is also when local offer is being applied too, so i guess it should also cause MediaAdded event to fire.

So is this an expected behaviour and outdated documentation or a bug? I can prepare a PR if its the second case.

@lolgesten
Copy link

Very good question. 🤔

It sounds like a bug. @xnorpx, @k0nserv, @davibe anyone got thoughts?

@xnorpx
Copy link
Collaborator

xnorpx commented Sep 26, 2024

We only use direct API, but if I remember correctly, it will fire for local added streams.

@k0nserv
Copy link
Collaborator

k0nserv commented Sep 26, 2024

I don't think we should fire it when the user explicitly added the media, I believe this aligns with the spec too. I consulted our usage too, we don't expect it to fire on locally added media

@algesten
Copy link
Owner

So a PR adjusting the doc to that effect?

@k0nserv
Copy link
Collaborator

k0nserv commented Sep 27, 2024

So a PR adjusting the doc to that effect?

I think so yes.

@xnorpx
Copy link
Collaborator

xnorpx commented Sep 30, 2024

We only use direct API, but if I remember correctly, it will fire for local added streams.

I did verify that when we add our local streams with direct API we are getting MediaAdded events.

(We are not using it)

@alexlapa
Copy link
Collaborator Author

alexlapa commented Oct 1, 2024

I did verify that when we add our local streams with direct API we are getting MediaAdded events.

Ah, and I'm using SDP api and not getting MediaAdded for media that's added in local offer.

I don't think we should fire it when the user explicitly added the media, I believe this aligns with the spec too.

I'm okay with both options, just want to be sure that it works as expected and won't change someday.

So a PR adjusting the doc to that effect?

Works for me. I'll prepare a PR (regarding RTX cache drop ratio) in the next few days and i can update this doc there. So feel free to close this issue, or I'll close it after doc is updated.

@algesten
Copy link
Owner

algesten commented Oct 2, 2024

I did verify that when we add our local streams with direct API we are getting MediaAdded events.

Ok. So PR to fix docs and change this behavior.

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

No branches or pull requests

5 participants