As defined in https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpContributingSource. You'll see that timestamp, source, and audioLevel are all filled in, but rtpTimestamp is not. You can repro by doing RTPReceiver.getSynchronizationSources for any incoming RTP stream, where you'll get a result of the form {source: 4171861391, timestamp: 735867319}. This prevents correlation of video frames with their RTP timestamps.
Created attachment 406993 [details] Patch
Created attachment 406994 [details] Patch
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
(In reply to EWS from comment #3) > /Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither > lists a valid reviewer nor contains the string "Unreviewed" or "Rubber > stamp" (case insensitive). Oh right, the ChangeLog needs to have something like 'Reviewed by NOBODY (OOPS!).' that the commit queue bot would update with the actual reviewer name. @Justin, would you be able to the update change log? Either adding this OOPS line or 'Reviewed by Youenn Fablet.' From a test point of view, could we add a test or a simple assert in WPT test LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html? Something like assert_equals(typeof source.rtpTimestamp, 'number')?
Created attachment 407017 [details] Patch
Fixed the changelog. Regarding tests, LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html already has the check you mentioned, will look more closely to see why it didn't catch the initial issue.
Comment on attachment 407017 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407017&action=review > Source/WebCore/ChangeLog:8 > + Covered by existing tests, including LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html Covered how? I don’t see a change to expected test results in this patch; I’d expect to see tests passing now that failed before.
Right, that's what I'm trying to figure out. LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html fails for me when I run it standalone without my patch, so I'm trying to see why this hasn't been an issue before. It's marked as a 'slow' test, perhaps it's excluded from run-webkit-tests somehow?
Comment on attachment 407017 [details] Patch Maybe Youenn should review? Maybe Youenn has an idea why there is no test failure without this?
Created attachment 407026 [details] Patch
OK, I figured it out. There were tests that were expected to fail in RTCRtpReceiver-getSynchronizationSources.https-expected.txt, which explains why run-webkit-tests didn't complain. I removed the relevant expected failures and everything looks good now. New patch uploaded with these updates to the expected.txt file.
Ready for commit queue.
Looks like the new test is failing on the mac-wk2 EWS bot. I wonder why?
Strange. Anything unusual about that configuration? Would it be worth trying to re-run the bot?
Well, the bot tried twice and failed twice. But we can hit the retry button to get it to try again. The failure is: > FAIL [audio] RTCRtpSynchronizationSource.rtpTimestamp is a number [0, 2^32-1] assert_equals: expected "number" but got "undefined" I can’t think of anything unusual there; it is on macOS Mojave, which is a bit old. Youenn, do you have any guesses?
The problem is that I can't see a code path that would lead to the value being undefined - my change explicitly populates this property, even if libwebrtc returns a bogus value. If it fails again, perhaps we can double-check that it applied the patch correctly and that it's using the right backend.
It failed again. No idea how to double check those things!
There’s clear evidence in the logs that it applied the patch correctly. But it’s not obvious to me that it did a build. The bot says "download built product", which doesn’t sound quite right to me.
I can’t find anything that explains the test failure.
I'm kind of at a loss too. Do you have a Mojave machine or VM lying around that you could try the test on?
I do not, but I am sure someone from Apple does.
Is there a way to submit a patch directly to that trybot? I could try some printf debugging to try to understand what's happening here.
Currently asking for help in Slack. I don’t know anything about uploading a patch to only a single trybot. You can definitely upload a patch with printf in it to see what happens, but it will go to all the EWS bots.
(In reply to Justin Uberti from comment #22) > Is there a way to submit a patch directly to that trybot? I could try some printf debugging to try to understand what's happening here. Feel free to upload a patch with printf here. As Darin mentioned, there isn't a way to submit a patch directly to a specific queue.
Can it be a build system issue where we would need to touch the rtp sync dictionary IDL file to update its conversion routine? Seems strange though that only Mac-wk2 bot would fail. I’ll check this further on Monday if nobody gets to it before.
(In reply to youenn fablet from comment #25) > Can it be a build system issue where we would need to touch the rtp sync > dictionary IDL file to update its conversion routine? It could be, sure. I looked over the make file to see if I could spot a mistake, but did not.
A build system mistake could explain this. The value might be filled in but the JSRTCRtpContributingSource.cpp file might not be regenerated properly to add the binding for the rtpTimestamp property. The way this is supposed to work is that since JS_BINDING_IDLS contains that IDL file’s path, JS_DOM_HEADERS is supposed to end up containing JSRTCRtpContributingSource.h and JS_DOM_IMPLEMENTATIONS is supposed to contain JSRTCRtpContributingSource.cpp. Those both are mentioned in an "all" rule, so they should be regenerated if needed. The rule that generates them is the one that starts with JS%.cpp JS%.h : %.idl. Because VPATH includes $(WebCore)/Modules/mediastream that should trigger for the RTCRtpContributingSource.idl source file and regenerate both JSRTCRtpContributingSource.h and JSRTCRtpContributingSource.cpp. Running the generate-bindings.pl and passing it RTCRtpContributingSource.idl (specified as "$<"). Then JSRTCRtpContributingSource.cpp should be compiled since it’s part of UnifiedSource12.cpp. I have no idea which part of this might be malfunctioning. By the way, while exploring this I discovered at least one mistake. I am pretty sure that this line in the makefile: all : $(ADDITIONAL_BINDING_IDLS:%.idl=JS%.h) is bogus and should be deleted.
Is the bot not doing a clean build? I did notice when developing this patch that I needed to manually blow away the generated JSRTCRtp*.cpp files in order for the appropriate new ones to be generated. If the bot has stale generated files that could partially explain the problem (although not sure why this would only happen on Mojave).
(In reply to Justin Uberti from comment #28) > Is the bot not doing a clean build? No. > I did notice when developing this patch that I needed to manually blow away > the generated JSRTCRtp*.cpp files in order for the appropriate new ones to > be generated. If the bot has stale generated files that could partially > explain the problem (although not sure why this would only happen on Mojave). That’s not just a problem with the bot; it’s a problem for everyone else working on WebKit too. Seems like there is a makefile bug that you worked around.
So sorry that this is affecting you -- we would really like to figure it out and fix it though.
If it succeeds on a clean build, I plan to land the fix and file a follow-up bug for the underlying issue.
(In reply to youenn fablet from comment #31) > If it succeeds on a clean build, I plan to land the fix and file a follow-up > bug for the underlying issue. Sounds fine.
I guess the other people won’t see build failures; they’ll just see a mysterious test failure if they run tests.
Thanks everyone for your help on this. Agree it would be great to also address the makefile issue at some point - I spent close to an hour trying to figure out if there was some other file I needed to change for the new property to appear in JS.
I manually did a clean build (by manually deleting the WebKitBuild folder on the bot(ews120)) and re-trying the mac build (https://ews-build.webkit.org/#/builders/33/builds/17675). The test seems to pass now (https://ews-build.webkit.org/#/builders/31/builds/16155).
Comment on attachment 407026 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=407026&action=review > Source/WebCore/ChangeLog:8 > + Updated expected results in LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https-expected.txt. The bot test failure was most probably due to bug 181714. > LayoutTests/imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https-expected.txt:4 > +PASS [audio] RTCRtpSynchronizationSource.rtpTimestamp is a number [0, 2^32-1] Before committing this patch, we will need to add a ChangeLog entry in LayoutTests/imported/w3c/ChangeLog.
(In reply to youenn fablet from comment #36) > Before committing this patch, we will need to add a ChangeLog entry in > LayoutTests/imported/w3c/ChangeLog. I did commit-queue- because we need a new patch with that change log entry.
Tempted to just do it myself!
Created attachment 407079 [details] Patch
Added requested ChangeLog, PTAL
Found 1 new test failure: imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html
Created attachment 407091 [details] Patch for landing
Committed r266052: <https://trac.webkit.org/changeset/266052> All reviewed patches have been landed. Closing bug and clearing flags on attachment 407091 [details].
<rdar://problem/67669566>