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.

Sign up for GitHub

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

Jump to bottom

Enabling rtcp stats subscriptions #892

Merged
merged 9 commits into from
Jun 6, 2017
Merged

Enabling rtcp stats subscriptions #892

merged 9 commits into from
Jun 6, 2017

Conversation

aalonsog
Copy link
Member

@aalonsog aalonsog commented May 11, 2017 •
edited
Loading

Description

This PR enables the subscriptions to rtcp stats of specific streams using AMQP queues. This would replace the current rtcp report mechanism where every stream reports its stats when the configuration enables it.

[] It needs and includes Unit Tests

Changes in Client or Server public APIs

No changes in public server/client APIs. To use this features, AMQP queues must be used.

  • You can renew a subscription by sending a new subscribeToStats message to the same destination.
  • You can cancel a subscription by sending a new subscribeToStats message with interval=0 to the same destination.

For example:

var to = '979518706711794800';       // streamId
var timeout = 20;                    // receive stats during 20 seconds 
var interval = 5;                    // receive stats every 5 seconds

amqper.bind_broadcast('stats_subscriptions', function (stats) {
    if (stats.message.streamId === to) {
       console.log('Received stats', stats);
    }
});

amqper.broadcast('ErizoJS', {method: 'subscribeToStats', args: [to, timeout, interval]}, function(resp){
    console.log('Subscription result', resp);
});

[] It includes documentation for these changes in /doc.

aalonsog
aalonsog commented May 11, 2017
View reviewed changes
erizo_controller/erizoClient/src/Connection.js
@@ -111,7 +111,8 @@ Erizo.GetUserMedia = function (config, callback, error) {
}
L.Logger.debug('Screen access on chrome stable, looking for extension');
try {
chrome.runtime.sendMessage(extensionId, {getStream: true}, function (response){
Copy link
Member Author

Choose a reason for hiding this comment

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

Just fixing linter. Not related with this PR

aalonsog
aalonsog commented May 18, 2017
View reviewed changes
erizo_controller/erizoClient/src/webrtc-stacks/ChromeStableStack.js
@@ -266,7 +266,7 @@ Erizo.ChromeStableStack = function (spec) {

localDesc.sdp = setMaxBW(localDesc.sdp);
if (config.Sdp || config.maxAudioBW){
L.Logger.debug ('Updating with SDP renegotiation', spec.maxVideoBW, spec.maxAudioBW);
L.Logger.debug('Updating with SDP renegotiation', spec.maxVideoBW, spec.maxAudioBW);
Copy link
Member Author

Choose a reason for hiding this comment

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

Just fixing linter. Not related with this PR

aalonsog
aalonsog commented May 18, 2017
View reviewed changes
extras/basic_example/public/script.js
localStream.addEventListener('access-accepted', function () {
room.connect();
localStream.show('myVideo');
});
localStream.init();
}
div.setAttribute('id', 'myVideo');
document.getElementById('videoContainer').appendChild(div);
Copy link
Member Author

Choose a reason for hiding this comment

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

div.setAttribute was out of the div var definition scope

lodoyun
lodoyun approved these changes May 19, 2017
View reviewed changes
Copy link
Contributor

@lodoyun lodoyun left a comment

Choose a reason for hiding this comment

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

LGTM!

@aalonsog aalonsog merged commit c813ef2 into lynckia:master Jun 6, 2017
@aalonsog aalonsog deleted the stats_subscribe branch June 6, 2017 07:59
Arri98 pushed a commit to Arri98/licode that referenced this pull request Apr 6, 2021
Enabling rtcp stats subscriptions (lynckia#892)
edeed7b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lodoyun lodoyun lodoyun approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aalonsog @lodoyun