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

RTCRtpContributingSource.audioLevel has different type and range than similar fields in webrtc-stats #1734

Closed
na-g opened this issue Jan 11, 2018 · 14 comments

Comments

@na-g
Copy link

na-g commented Jan 11, 2018

The audioLevel field of RTCRtpContributingSource is a byte with a range range of 0 to 127. The audioLevel field of RTCRTPContributingSourceStats and RTCAudioStreamTrackStats are doubles with a range of 0.0 to 1.0. Is there compelling reason that RTCRtpContributingSource.audioLevel should differ in this regard? Should the same transform noted in the fields in the audioLevel fields webrtc-stats be applied to RTCRtpContributingSource.audioLevel so that they are directly comparable?

@jan-ivar
Copy link
Member

@alvestrand @taylor-b Thoughts?

Firefox 59 goes to beta in 10 days, so we're considering pref'ing off getContributingSources and getSynchronizationSources until this is resolved.

@taylor-b
Copy link
Contributor

I can't recall a good reason for making it a byte. I'd be fine with changing it; it's not implemented in Chrome yet.

@dontcallmedom
Copy link
Member

the reason for making it a byte is documented in the spec:

Both RFCs define the level as an integral value from 0 to 127 representing the audio level in negative decibels relative to the loudest signal that the system could possibly encode. Thus, 0 represents the loudest signal the system could possibly encode, and 127 represents silence.

(I have no opinion on whether this is a good or bad reason - @fluffy @alvestrand and @aboba seem to have been involved in the design discussion back then

@taylor-b
Copy link
Contributor

That's how it's encoded in the RTP packet, in the interest of saving space, but it can easily be converted to the 0.0-1.0 range for consistency. I don't think an application developer would care much about getting raw information from an RTP packet, they'd care about getting usable information. And if you really want 0-127 values, you can do some math to convert to them.

@foolip
Copy link
Member

foolip commented Jan 19, 2018

Hi all,

@rakuco and I are looking at normative spec change without tests every week this quarter, and this week I took a look at #1736, since I'm a bit familiar with the tests that were written.

There are tests for audioLevel being a byte written by @soareschen (reviewed by @rwaldron) that now contradict what the spec says, in RTCRtpReceiver-getSynchronizationSources.https.html and RTCRtpReceiver-getContributingSources.https.html. (Found by grep.)

I've filed web-platform-tests/wpt#9108 to keep track of the fact that the tests are now wrong, and looking through https://github.com/w3c/web-platform-tests/labels/webrtc before process transitions would probably be a good idea since the test results could otherwise be misleading, especially if someone has implemented the old behavior and passes those tests.

CONTRIBUTING.md suggests at least filing an issue like that, but this is a case where I think blocking the spec change on the test update would have been less work overall.

But I'm not just here to nag, if there are any bits of infrastructure that you all think would be useful to make it harder to overlook testing, I'd love to hear some ideas.

whatwg/html#1849 (comment) describes the current state of this "project", and of particular interest could be the auto-filing of bugs, which in the best case can mean that if Chrome was passing a test and test was changed to fail, a bug would be automatically filed.

@alvestrand
Copy link
Contributor

we have enabled auto-filing of Chromium bugs on both the webrtc component and the mediacapture-main component, but haven't seen any bugs yet. Good point! @jan-ivar can you take care of the test?
(We're pretty sure that nothing passes the test in its old form - it's not possible to generate test data using a browser, and we don't know of anyone who's created RTP-generating tools that make it possible.)

@foolip
Copy link
Member

foolip commented Jan 26, 2018

@jan-ivar, can I assign web-platform-tests/wpt#9108 to you?

@foolip
Copy link
Member

foolip commented Jan 26, 2018

We're pretty sure that nothing passes the test in its old form - it's not possible to generate test data using a browser, and we don't know of anyone who's created RTP-generating tools that make it possible.

Sounds like a serious problem. Would a browser that implements more of the spec pass these tests, or would they still fail? If so, aren't the tests plainly broken?

@alvestrand
Copy link
Contributor

If the test is written correctly, browsers should pass when not implementing audioLevel, when implementing audioLevel according to the old spec, and when implementing audioLevel according to the new spec - because "missing" is a legal value under the test conditions we can generate using the present test framework.
It would be correct to test the negative - contributingSource MUST NOT be present when the source is another browser.

@foolip
Copy link
Member

foolip commented Jan 26, 2018

Ah, right, it's an optional dictionary member, so if it's not there, unless some prose says otherwise, that's allowed.

So, in order to really test this, there must be something that sends RTP with this header present. Is this part of a bigger collection of untestable things, and should I file an "untestable" issue on web-platform-tests for tracking?

@alvestrand
Copy link
Contributor

alvestrand commented Jan 27, 2018

Do file an "untestable" issue - the issue is part of a larger cluster that is about things only MCUs do; another one is verifying that the browser is sending simulcast correctly.
This one is testable in KITE if they find an open source MCU that does media mixing to add to the test enviroment.

@foolip
Copy link
Member

foolip commented Jan 27, 2018

I have filed web-platform-tests/wpt#9213, please comment there if that seems like a good or not so good idea.

@agouaillard
Copy link

Yes guys, by all means make a list as we are working an such media MCU / SFU and other tools specifically to address them in KITE. Copy either sergio or myself and/or harald on google side, to make sure we are aware of it. cheers. We have decided this week to start working on Media server needing task with harald next week anyway (unified plan, simulcast, ..)

@foolip
Copy link
Member

foolip commented Jan 27, 2018

Made some guesses in web-platform-tests/wpt#9213 (comment), corrections welcome. If any software is written for test purposes, it would be good if it could be run as part of wpt as well. web-platform-tests/wpt#9213 is as concrete as my "plans" are now, just like to keep track of why testing isn't working well across the web platform.

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

8 participants