Bug 215722 – RTCRtpSynchronizationSource.rtpTimestamp is not populated

• Component: WebRTC
• Status: RESOLVED FIXED

[Justin Uberti]

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.

[Justin Uberti]

Created attachment 406993 [details]
Patch

[Justin Uberti]

Created attachment 406994 [details]
Patch

[EWS]

/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]

(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]

Created attachment 407017 [details]
Patch

[Justin Uberti]

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 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]

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 on attachment 407017 [details]
Patch

Maybe Youenn should review? Maybe Youenn has an idea why there is no test failure without this?

[Justin Uberti]

Created attachment 407026 [details]
Patch

[Justin Uberti]

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]

Ready for commit queue.

[Darin Adler]

Looks like the new test is failing on the mac-wk2 EWS bot. I wonder why?

[Justin Uberti]

Strange. Anything unusual about that configuration? Would it be worth trying to re-run the bot?

[Darin Adler]

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]

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]

It failed again. No idea how to double check those things!

[Darin Adler]

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]

I can’t find anything that explains the test failure.

[Justin Uberti]

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]

I do not, but I am sure someone from Apple does.

[Justin Uberti]

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]

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]

(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]

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]

(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]

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]

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]

(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]

So sorry that this is affecting you -- we would really like to figure it out and fix it though.

[youenn fablet]

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]

(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]

I guess the other people won’t see build failures; they’ll just see a mysterious test failure if they run tests.

[Justin Uberti]

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]

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 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]

(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]

Tempted to just do it myself!

[Justin Uberti]

Created attachment 407079 [details]
Patch

[Justin Uberti]

Added requested ChangeLog, PTAL

[EWS]

Found 1 new test failure: imported/w3c/web-platform-tests/webrtc/RTCRtpReceiver-getSynchronizationSources.https.html

[youenn fablet]

Created attachment 407091 [details]
Patch for landing

[EWS]

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]

<rdar://problem/67669566>