Closed Bug 1585009 Opened 2 months ago Closed 2 months ago

Support playout-delay RTP header extension

Categories

(Core :: WebRTC: Signaling, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: andreas.schuler, Assigned: andreas.schuler)

Details

Attachments

(1 file)

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.

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.

Assignee: nobody → andreas.schuler
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

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.

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

Flags: needinfo?(docfaraday)

Check back on try.

Flags: needinfo?(docfaraday) → needinfo?(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.

You can run the webrtc mochitests like this:

./mach mochitest dom/media/tests/mochitest

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.

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

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.

Flags: needinfo?(andreas.schuler)

Check on try.

Flags: needinfo?(docfaraday)
Attachment #9097502 - Attachment description: Support playout-delay RTP header extension (Bug 1585009) → Bug 1585009: Support playout-delay RTP header extension
Flags: needinfo?(docfaraday)

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

Flags: needinfo?(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.

Flags: needinfo?(andreas.schuler)
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/258c68e01345
Support playout-delay RTP header extension r=bwc
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71
You need to log in before you can comment on or make changes to this bug.