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

Dominant Speaker Event #603

Merged
merged 42 commits into from Jul 29, 2021
Merged

Dominant Speaker Event #603

merged 42 commits into from Jul 29, 2021

Conversation

SteveMcFarlin
Copy link
Contributor

@SteveMcFarlin SteveMcFarlin commented Jul 9, 2021

Implementation of Dominant Speaker Identification for Multipoint Videoconferencing by Ilana Volfin and Israel Cohen. This implementation uses the RTP Audio Level extension from RFC-6464 for the input signal. This has been ported from DominantSpeakerIdentification.java in Jitsi

Implementation of Dominant Speaker Identification for Multipoint Videoconferencing by Ilana Volfin and Israel Cohen. This implementation uses the RTP Audio Level extension from RFC-6464 for the input signal.
Copy link
Member

@ibc ibc left a comment

Choose a reason for hiding this comment

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

Amazing, thanks. First review round done.

src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/Router.ts Outdated Show resolved Hide resolved
worker/include/RTC/ActiveSpeakerObserver.hpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
@nazar-pc
Copy link
Collaborator

It would be great to include corresponding changes on Rust side as well 🙏

SteveMcFarlin and others added 2 commits July 13, 2021 12:23
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
@SteveMcFarlin
Copy link
Contributor Author

It would be great to include corresponding changes on Rust side as well 🙏

I took a look at the Rust implementation. I am slightly confused. In the implementation for audio_level_observer.rs it appears the RtpObserver is also implemented in that file. In any case I thought perhaps I could "hack" it if the code was not much. Basically by following the core audio_level_observer.rs implementation. Well I thought wrong. I have only ever played with Rust, so I don't think I can help here.

@nazar-pc
Copy link
Collaborator

In the implementation for audio_level_observer.rs it appears the RtpObserver is also implemented in that file.

It is not implementation of RtpObserver itself, it just implements RtpObserver trait for AudioLevelObserver struct. Think of it as implementing an interface for a class in other languages.

worker/src/RTC/Router.cpp Outdated Show resolved Hide resolved
src/ActiveSpeakerObserver.ts Show resolved Hide resolved
worker/src/Channel/ChannelNotifier.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
ggarber and others added 2 commits July 15, 2021 16:33
Merge remote-tracking branch 'ggb/steve-v3-dominant-speaker' into v3-dominant-speaker
@SteveMcFarlin
Copy link
Contributor Author

@nazar-pc Please take a look at the rust implementation. Thanks!

@nazar-pc
Copy link
Collaborator

Rust version looks good, thank you!

src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
@ibc
Copy link
Member

ibc commented Jul 16, 2021

CI is failing due to this:

../include/RTC/ActiveSpeakerObserver.hpp:6:10: fatal error: json.hpp: No such file or directory
    6 | #include <json.hpp>
      |          ^~~~~~~~~~

It must be #include <nlohmann/json.hpp>.

src/ActiveSpeakerObserver.ts Outdated Show resolved Hide resolved
worker/include/RTC/ActiveSpeakerObserver.hpp Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
SteveMcFarlin and others added 2 commits July 20, 2021 09:16
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
worker/src/RTC/ActiveSpeakerObserver.cpp Show resolved Hide resolved
worker/src/RTC/ActiveSpeakerObserver.cpp Outdated Show resolved Hide resolved
SteveMcFarlin and others added 2 commits July 21, 2021 08:19
Co-authored-by: Iñaki Baz Castillo <ibc@aliax.net>
@ibc
Copy link
Member

ibc commented Jul 22, 2021

Thanks, looking great. Waiting for CI to complete.

BTW could you please contribute to the mediasoup-website project with a PR to document the new API?

@ibc
Copy link
Member

ibc commented Jul 26, 2021

Thanks a lot for this, guys. I'll wait for the documentation PR before merging. Just let me know if you have any problem or question writing it. There is "gulp live" task in mediasoup-website project to run the web locally.

@ibc ibc merged commit 34dcdcd into versatica:v3 Jul 29, 2021
@ibc
Copy link
Member

ibc commented Jul 29, 2021

Released in mediasoup v3.8.0 :)

Thanks a lot!

@SteveMcFarlin
Copy link
Contributor Author

Thank you for the great review process!

@msach22
Copy link

msach22 commented Jul 30, 2021

@SteveMcFarlin nice contribution 😄

@mk19831002
Copy link

Hi,
did you write the documentation for that, I didn't find it in API documentation.

@ibc
Copy link
Member

ibc commented Aug 4, 2021

Somehow the website is not up to date in the web server, but yes, he wrote docs. I'll check and republish later today.

@d2e1
Copy link

d2e1 commented Aug 30, 2021

Hello guys,
I was testing the dominant speaker feature and a situation I came across prompted me to review the code. I also reviewed the Jitsi code and the article the algorithm is based on. As far as I can see, the following piece of code is added differently in Jitsi:

Mediasoup code:
https://github.com/versatica/mediasoup/blob/v3/worker/src/RTC/ActiveSpeakerObserver.cpp#L313

Jitsi code:
https://github.com/jitsi/jitsi-utils/blob/master/src/main/java/org/jitsi/utils/dsi/DominantSpeakerIdentification.java#L715

Related part of the article (Dominant Speaker Identification for Multipoint Videoconferencing by Ilana Volfin and Israel Cohen)
image

In the article and Jitsi code, the dominant speaker is included in the dividing part. I don't know the other parts of the code well, I wonder if a change is needed here?

Link for the open discussion : https://mediasoup.discourse.group/t/question-regarding-active-speaker-observer-implementation/3227

@punarinta
Copy link

punarinta commented Nov 23, 2021

Hi @SteveMcFarlin,

Could you please advise me what is the reason I'm not getting any events from ActiveSpeakerObserver instance?

I've initialized the top-level code like

const observer = await router.createActiveSpeakerObserver({interval: 500})

observer.on('dominantspeaker', (payload) => {
	console.log('event payload', payload)
})

For debugging I've added logs after lines 19 and 20 in https://github.com/versatica/mediasoup/blob/v3/node/lib/ActiveSpeakerObserver.js and I see that handleWorkerNotifications is called, but this.channel.on never fires its callback.

Both audio and video is sent and received correctly between the participating peers, so audio data definitely flows the way it should.

UPD: discussion moved to https://mediasoup.discourse.group/t/activespeakerobserver-does-not-emit-events/3621

@ibc
Copy link
Member

ibc commented Nov 23, 2021

Please let use the forum for questions.

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

Successfully merging this pull request may close these issues.

None yet

9 participants