Open Bug 1498538 Opened Last year Updated 7 months ago

Potential regression of RTCDTMFSender-ontonechange.https.html

Categories

(Core :: WebRTC: Audio/Video, defect, P3)

63 Branch
defect

Tracking

()

Tracking Status
geckoview62 --- unaffected
firefox-esr60 --- unaffected
firefox62 --- unaffected
firefox63 --- wontfix
firefox64 - affected

People

(Reporter: pehrsons, Assigned: dminor)

References

(Blocks 1 open bug)

Details

wpt.fyi says we regressed this test in 63 [1], but we didn't see this happen on try because the test had gotten disabled in bug 1405428.

We should investigate this and see whether there's an actual regression we can fix before 63 ships.


[1] https://wpt.fyi/results/webrtc/RTCDTMFSender-ontonechange.https.html?product=firefox[beta]&product=firefox[experimental]&product=firefox[stable]&aligned=true
Assignee: nobody → dminor
This sample is also no longer working: https://webrtc.github.io/samples/src/content/peerconnection/dtmf/
When I run the tests in my browser at [1] I see a bunch of off by one or two errors in the expections, e.g. the test expects the event to fire at 170, but we're firing at 169.

I was unable to find a version of Firefox in the past year and a half where the sample in Comment 1 works, which makes me think that the sample has been updated in a way that is incompatible with Firefox. When DTMF was initially implemented, I had forked that sample to work with Firefox for testing, but I'm almost certain jib had made changes to adapter.js to make the sample run unmodified.

[1] https://w3c-test.org/webrtc/RTCDTMFSender-ontonechange.https.html
It's possible the tests were disabled due to unexpected passes. The timing problems I mentioned above were marked as expected failures, so if things went a millisecond long on test runs, the event would occur at 170 instead of 169 and we'd see an unexpected pass. This happened in a local run just now.

I think it is reasonable to have the test case accept values a couple of milliseconds early given that the tests also accept values that are several seconds too late, but I'll have to check the spec to make see how strict the timing requirements are.
FWIW on wpt.fyi it seems a bit more off than that.

Examples:
> Failure message: assert_approx_equals: Expect tonechange event for "1" to be fired approximately after 0 milliseconds expected 0 +/- 400 but got 582
> Failure message: assert_approx_equals: Expect tonechange event for "B" to be fired approximately after 170 milliseconds expected 170 +/- 400 but got 891
Strange that the version of the tests in tree are >= 170, but wpt.fyi is +/- 400.

At any rate, this doesn't look completely broken, just a timing problem, so probably a lower priority to fix.
Rank: 25
Priority: -- → P3
P3 so wontfix for 63 as we are in RC week.
not tracking based on comment 5.
Blocks: 1420640
Interestingly enough, it appears that chrome initially had the same problems with events firing too early [1] but at some point that stabilized for them and the pr was merged.

[1] https://github.com/web-platform-tests/wpt/pull/13123#issuecomment-426734224

So, part of this is almost certainly deliberate JS Date precision limitations and jitter that exist to make fingerprinting harder. Some pref-setting might be able to stabilize things some. (see bug 1531078)

Some of it is also due to the fact that we do a not-completely-trivial amount of work between scheduling the timer for the next tone, and firing the event. Variations in how long this code takes can cause events to appear to fire early.

https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/dom/media/PeerConnection.jsm#1842
https://searchfox.org/mozilla-central/rev/00f3836a87b844b5e4bc82f698c559b9966e4be2/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#3099

Honestly, I think the pattern of repeated interval checking (ie; measuring time elapsed since the last time we checked, repeatedly) is not very realistic, and the test ought to either take a different approach, or loosen its tolerances a little.

Depends on: 1531078

I've done some experimentation, and it looks like fixing bug 1531078 doesn't help much. Scheduling the next tone after the tonechange callback eliminates the flakiness (at least on my laptop). This is probably just jitter caused by the JIT (ha). I don't think it makes sense to schedule the next tone after the callback, because of potential reentrancy problems.

I think that if we're going to validate that callbacks don't happen early, we need to measure the cumulative time. That way, if a callback is delayed for some reason, the next callback won't be seen as happening too soon.

No longer depends on: 1531078
You need to log in before you can comment on or make changes to this bug.