1585009 - Support playout-delay RTP header extension

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement)

Description

[Andreas Schuler]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/77.0.3865.90 Safari/537.36

Steps to reproduce:

A WebRTC client offers to send video media and enables the following RTP header extension, by setting the following SDP attribute in its offer:

a=extmap:6 http://www.webrtc.org/experiments/rtp-hdrext/playout-delay

Actual results:

The FireFox client removes this attribute in the answer sent back.

Expected results:

Expectation is for the playout-delay attribute to not be removed from the answer and for the browser to honor this extension.

This extension is confirmed to work properly in Chrome and Safari. Therefore the Chromium based WebRTC stack fully supports it.

Inspecting the Gecko repository confirms that the WebRTC stack itself appears to support this RTP header extension. However, the separate signaling library used to conduct the SDP handshake currently does not include support for it.

I have personally tested a simple patch to the signaling code in Gecko to confirm that when enabled, the extension actually performs as expected. I would be happy to productize and submit this patch.

This is a bit difficult to test because to my knowledge there currently are no WebRTC products out there relying on this extension. We are in the process of adding this to our own solution (PhenixRTS), which is how we discovered this issue, and would like to be able to include Gecko based browsers to our list of supported platforms.

Comment 1

[Byron Campen [:bwc]]

Thanks for offering to submit a patch! You'll need to get phabricator set up to submit a patch for review (see https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html). I can handle running the CI testing for your change when you are ready, just needinfo me.

Comment 2

[Andreas Schuler]

Attachment: Bug 1585009: Support playout-delay RTP header extension

Added playout-delay RTP header extension for video to JsepSessionImpl::SetupDefaultRtpExtensions.
This ensures that this extension will be preserved when generating an answer to an offer (by a sending peer) containing it.
Also updated JsepSessionTest to include a test verifying that the expected default audio and video extensions are set.

Comment 3

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c5c0e9a8b34c39ce0ffa8368547f519bcde940e

Comment 4

[Byron Campen [:bwc]]

Windows builds are busted on that try push due to some infrastructure issue. Will re-try later today.

Comment 5

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=152e588e9cf9406c3569f7d6a3649aaab666decf

Comment 6

[Byron Campen [:bwc]]

Check back on try.

Comment 7

[Byron Campen [:bwc]]

Ok, it looks like some more tests need to be updated:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=152e588e9cf9406c3569f7d6a3649aaab666decf&selectedJob=269651372

Comment 8

[Andreas Schuler]

Thank you for running, sorry for missing this.

Could you tell me how to run these tests locally? I am running on macOS.

Comment 9

[Byron Campen [:bwc]]

You can run the webrtc mochitests like this:

./mach mochitest dom/media/tests/mochitest

Comment 10

[Andreas Schuler]

I have been able to run and reproduce the issue.

One of the tests actually verifies the presence of those extensions on incoming RTP packets.

Since there is currently no need for the browser to send packets with this playout-delay extension, I did not bother adding them (since it only adds overhead if they are not actually used). The problem is though that this extension is being announced in the SDP, which is why the test fails (referring to test_peerConnection_basicVideoVerifyRtpHeaderExtensions.html).

Basically: I want the browser to accept offers from other sending peers, which are producing RTP packets with this extension in it (i.e. the browser should not remove the extension from its answer). If the browser is sending however, there is no need to include this extension in the offer as it will not actually produce any.

I will have to go over this code some more to make sure it behaves in this manner, then I am confident it will also pass this test.

Comment 11

[Byron Campen [:bwc]]

So if you want Firefox to only use this as a recv extension, you'll want to pass kRecvonly to AddVideoRtpExtension instead of kSendrecv.

Comment 12

[Andreas Schuler]

I have made the change as you suggested and retested everything locally: The behavior is now as desired.
I also updated a couple mochitest files and ensured those were running without failures.

Comment 13

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b708c4f59b81bfd8d410b53087505199db681c4

Comment 14

[Byron Campen [:bwc]]

Check on try.

Comment 15

[Byron Campen [:bwc]]

Try looks good. Is there any change you would like to make before landing?

Comment 16

[Andreas Schuler]

I am good with it as is. I have tested the browser with our WebRTC server (in development), and the playout-delay header extension is now handled as expected. Thank you for all your help.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/258c68e01345
Support playout-delay RTP header extension r=bwc

Comment 18

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/258c68e01345