RTCRtpContributingSource & RTCRtpSynchronizationSource -> dictionaries #1668
Conversation
|
Needs test update to account for the interface -> dictionary change. |
|
Chrome has already shipped RTCRtpContributingSource, so there should be some tests that starts failing with this change. @henbos, do you think it's safe to change the interface into a dictionary? |
|
This would affect apps relying on them being live, but I'm not sure they do since it's debatable how useful that fact is, it would make more sense to just poll it and look at the result. Considering getContributingSources() is relatively new and not a common use case my gut feeling this is safe to change. |
|
Good. When it comes to tests, in addition to verifying the new behavior (that an object and not an instance of RTCRtpContributingSource is returned), an addition to https://github.com/w3c/web-platform-tests/blob/master/webrtc/historical.html is needed, to make sure the RTCRtpContributingSource interface object doesn't exist. |
|
FWIW the WPT tests for getContributingSources() look incorrect to us. AFAIK this API would only return a non-zero array if connected to an MCU server emitting CSRCs. Frankly we're having a hard time testing it. @henbos How is Chrome testing this? We'd love some help. |
|
When this was implemented, Chrome had mocks which emulated receving CSRCs. The tests relying on mocks in Chrome was horrible, and for almost all other cases did not add any value, our tests simply did not test the real thing and maintaining a fake implementation was burdensome. When we finally removed mocking so that WPT and other tests would run with the real thing -- greatly improving our test coverage -- this particular feature lost its test coverage on the JavaScript layer. I don't see how this is testable on a JavaScript layer without emitting CSRCs. We currently only have tests for the lower layers and some plumbing. If we want WPT coverage for this maybe we need WebDriver support for pretty low level stuff like packets? Sounds like another case of high-effort low-impact work. |
|
Filing a type:untestable for this on web-platform-tests and deleting the wrong tests might be the best use of everyone's time, then? |
|
SGTM |
|
The KITE project has on its wishlist the integration of an MCU into tests - preferably picking an opensource MCU that can be just integrated into the project's builds. |
|
I'd like to be clear that something like that wouldn't be out of scope due to WPT as a whole having a clear delineation of scope, and that if someone did the work inside of WPT it would be most welcome. However, first there has to be someone to do the work, and that someone has to think that WPT is the best way to achieve whatever goals they have. |
| to the SSRC identifier, is updated each time, and if the RTP packet | ||
| contains CSRC identifiers, then the information relevant to the | ||
| <code><a>RTCRtpContributingSource</a></code> dictionaries corresponding to | ||
| those CSRC identifiers is also updated.</p> |
There was a problem hiding this comment.
This still talks about updating dictionaries upon receiving RTP packets. But since we don't want a "live" state anymore, what is updated is some internal set of dictionaries, correct? The ones returned by getContributingSources() being copies not affected by this update.
We still want the "queue a task to update the dictionaries" type of language to indicate that if an RTP packet is received midst-JS task execution cycle we don't see this until the next cycle?
getContributingSources() is always value-equal to getContributingSources() in a single cycle?
Just checking to make sure Chromium implementation still requires a cache.
There was a problem hiding this comment.
This has already been answered - the answer is yes.
Fix for #1533.