WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
215722
RTCRtpSynchronizationSource.rtpTimestamp is not populated
https://bugs.webkit.org/show_bug.cgi?id=215722
Summary
RTCRtpSynchronizationSource.rtpTimestamp is not populated
Justin Uberti
Reported
2020-08-20 18:04:12 PDT
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.
Attachments
Patch
(1.69 KB, patch)
2020-08-20 18:11 PDT
,
Justin Uberti
no flags
Details
Formatted Diff
Diff
Patch
(2.87 KB, patch)
2020-08-20 18:30 PDT
,
Justin Uberti
no flags
Details
Formatted Diff
Diff
Patch
(2.99 KB, patch)
2020-08-21 10:49 PDT
,
Justin Uberti
no flags
Details
Formatted Diff
Diff
Patch
(4.84 KB, patch)
2020-08-21 13:46 PDT
,
Justin Uberti
no flags
Details
Formatted Diff
Diff
Patch
(5.67 KB, patch)
2020-08-23 13:29 PDT
,
Justin Uberti
no flags
Details
Formatted Diff
Diff
Patch for landing
(6.55 KB, patch)
2020-08-24 00:18 PDT
,
youenn fablet
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Justin Uberti
Comment 1
2020-08-20 18:11:15 PDT
Created
attachment 406993
[details]
Patch
Justin Uberti
Comment 2
2020-08-20 18:30:41 PDT
Created
attachment 406994
[details]
Patch
EWS
Comment 3
2020-08-21 00:19:52 PDT
/Volumes/Data/worker/Commit-Queue/build/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
youenn fablet
Comment 4
2020-08-21 00:36:27 PDT
(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')?
Justin Uberti
Comment 5
2020-08-21 10:49:20 PDT
Created
attachment 407017
[details]
Patch
Justin Uberti
Comment 6
2020-08-21 10:51:26 PDT
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.
Darin Adler
Comment 7
2020-08-21 11:08:18 PDT
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.
Justin Uberti
Comment 8
2020-08-21 11:22:24 PDT
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?
Darin Adler
Comment 9
2020-08-21 12:22:48 PDT
Comment on
attachment 407017
[details]
Patch Maybe Youenn should review? Maybe Youenn has an idea why there is no test failure without this?
Justin Uberti
Comment 10
2020-08-21 13:46:30 PDT
Created
attachment 407026
[details]
Patch
Justin Uberti
Comment 11
2020-08-21 13:48:58 PDT
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.
Justin Uberti
Comment 12
2020-08-21 14:43:20 PDT
Ready for commit queue.
Darin Adler
Comment 13
2020-08-21 15:03:21 PDT
Looks like the new test is failing on the mac-wk2 EWS bot. I wonder why?
Justin Uberti
Comment 14
2020-08-21 15:46:22 PDT
Strange. Anything unusual about that configuration? Would it be worth trying to re-run the bot?
Darin Adler
Comment 15
2020-08-21 15:54:20 PDT
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?
Justin Uberti
Comment 16
2020-08-21 15:57:47 PDT
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.
Darin Adler
Comment 17
2020-08-21 16:56:12 PDT
It failed again. No idea how to double check those things!
Darin Adler
Comment 18
2020-08-21 16:57:20 PDT
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.
Darin Adler
Comment 19
2020-08-21 17:04:59 PDT
I can’t find anything that explains the test failure.
Justin Uberti
Comment 20
2020-08-21 17:08:08 PDT
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?
Darin Adler
Comment 21
2020-08-21 17:08:50 PDT
I do not, but I am sure someone from Apple does.
Justin Uberti
Comment 22
2020-08-21 17:16:52 PDT
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.
Darin Adler
Comment 23
2020-08-21 17:19:06 PDT
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.
Aakash Jain
Comment 24
2020-08-21 18:24:13 PDT
(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.
youenn fablet
Comment 25
2020-08-22 00:45:38 PDT
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.
Darin Adler
Comment 26
2020-08-22 09:16:47 PDT
(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.
Darin Adler
Comment 27
2020-08-22 11:52:46 PDT
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.
Justin Uberti
Comment 28
2020-08-22 13:28:14 PDT
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).
Darin Adler
Comment 29
2020-08-22 14:00:25 PDT
(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.
Darin Adler
Comment 30
2020-08-22 14:00:43 PDT
So sorry that this is affecting you -- we would really like to figure it out and fix it though.
youenn fablet
Comment 31
2020-08-22 14:04:27 PDT
If it succeeds on a clean build, I plan to land the fix and file a follow-up bug for the underlying issue.
Darin Adler
Comment 32
2020-08-22 14:29:50 PDT
(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.
Darin Adler
Comment 33
2020-08-22 14:30:33 PDT
I guess the other people won’t see build failures; they’ll just see a mysterious test failure if they run tests.
Justin Uberti
Comment 34
2020-08-22 14:57:13 PDT
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.
Aakash Jain
Comment 35
2020-08-22 18:15:49 PDT
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
).
youenn fablet
Comment 36
2020-08-23 09:38:17 PDT
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.
Darin Adler
Comment 37
2020-08-23 11:29:33 PDT
(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.
Darin Adler
Comment 38
2020-08-23 11:29:44 PDT
Tempted to just do it myself!
Justin Uberti
Comment 39
2020-08-23 13:29:23 PDT
Created
attachment 407079
[details]
Patch
Justin Uberti
Comment 40
2020-08-23 13:29:46 PDT
Added requested ChangeLog, PTAL
EWS
Comment 41
2020-08-23 15:37:02 PDT
Found 1 new test failure: imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html
youenn fablet
Comment 42
2020-08-24 00:18:57 PDT
Created
attachment 407091
[details]
Patch for landing
EWS
Comment 43
2020-08-24 02:27:03 PDT
Committed
r266052
: <
https://trac.webkit.org/changeset/266052
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 407091
[details]
.
Radar WebKit Bug Importer
Comment 44
2020-08-24 02:28:15 PDT
<
rdar://problem/67669566
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug