Bug 170157 – [WebRTC][OpenWebRTC] Add support for SDP BUNDLE ("a:group:BUNDLE" and "a=bundle-only" lines)

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Carlos Alberto Lopez Perez]

WebKit SDPProcessor ignores the bundlePolicy defined on RTCPeerConnection(), and doesn't generate the a=group:BUNDLE nor the a=bundle-only lines.

https://tools.ietf.org/html/draft-ietf-rtcweb-jsep-19 and https://tools.ietf.org/html/draft-ietf-mmusic-sdp-bundle-negotiation-36 defines how this should work.

Very summarized, the idea is that we should generate an "a=group:BUNDLE" attribute with the MID identifiers specified in the bundle group in the most recent answer.
If we are creating the offer, then we use our MID identifiers.

          a=group:BUNDLE ${midname1} [... ${midnameN} ]

And then we should generate a number of a=bundle-only lines according to the bundlePolicy defined and the number of tracks.

C&P from draft-ietf-rtcweb-jsep-19::

"""
The set of available policies is as follows:

   balanced:  The first m= section of each type (audio, video, or
      application) will contain transport parameters, which will allow
      an answerer to unbundle that section.  The second and any
      subsequent m= section of each type will be marked bundle-only.
      The result is that if there are N distinct media types, then
      candidates will be gathered for for N media streams.  This policy
      balances desire to multiplex with the need to ensure basic audio
      and video can still be negotiated in legacy cases.  When acting as
      answerer, if there is no bundle group in the offer, the
      implementation will reject all but the first m= section of each
      type.

   max-compat:  All m= sections will contain transport parameters; none
      will be marked as bundle-only.  This policy will allow all streams
      to be received by non-bundle-aware endpoints, but require separate
      candidates to be gathered for each media stream.

   max-bundle:  Only the first m= section will contain transport
      parameters; all streams other than the first will be marked as
      bundle-only.  This policy aims to minimize candidate gathering and
      maximize multiplexing, at the cost of less compatibility with
      legacy endpoints.  When acting as answerer, the implementation
      will reject any m= sections other than the first m= section,
      unless they are in the same bundle group as that m= section.

   As it provides the best tradeoff between performance and
   compatibility with legacy endpoints, the default bundle policy MUST
   be set to "balanced".
"""

[Carlos Alberto Lopez Perez]

Created attachment 305543 [details]
Patch

[Build Bot]

Comment on attachment 305543 [details]
Patch

Attachment 305543 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3425612

New failing tests:
fast/mediastream/RTCPeerConnection-inspect-offer.html
fast/mediastream/RTCPeerConnection-inspect-answer.html
fast/mediastream/RTCPeerConnection-inspect-offer-bundlePolicy-bundle-only.html

[Build Bot]

Created attachment 305571 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Carlos Alberto Lopez Perez]

It looks like libwebrtc has its own SDP generator, so WebKit internal SDPProcessor is not really used with the libwebrtc backed. That explains the layout test failures on the mac port.

[Carlos Alberto Lopez Perez]

Created attachment 305587 [details]
Patch

Move the rebaselines with the a=group:BUNDLE line of the expected results for the tests RTCPeerConnection-inspect-offer.html and RTCPeerConnection-inspect-answer.html to platform/gtk. Set the new test added (RTCPeerConnection-inspect-offer-bundlePolicy-bundle-only.html) as failing on all ports but WebKitGTK

[Alejandro G. Castro]

Comment on attachment 305587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305587&action=review

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:162
> +    configurationSnapshot->m_bundlePolicy = m_bundlePolicy;

Should we do the same in the createAnswer method?

> Source/WebCore/Modules/mediastream/MediaEndpointPeerConnection.cpp:605
> +    m_bundlePolicy = configuration.bundlePolicy;

Can we avoid having a m_bundlePolicy attribute in the peerConnection class and just use the sessionConfiguration value. I mean, we can use the bundlePolicy inside the endpoint configuration when creating the sessionConfiguration we are going to use for the offer or the answer.

> Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:61
> +    PeerConnectionStates::BundlePolicy m_bundlePolicy = PeerConnectionStates::BundlePolicy::Balanced;
> +

We can add API here to get and set the bundle instead of making it public.

[Carlos Alberto Lopez Perez]

Created attachment 306090 [details]
Patch

Address reviewer comments

[Alejandro G. Castro]

Comment on attachment 306090 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=306090&action=review

LGTM, just added a couple of comments about style.

> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:344
> +

Remove the extra line.

> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:351
> +        bundlePolicyName = "balanced";

What about doing directly: return "balanced";

> Source/WebCore/Modules/mediastream/SDPProcessor.cpp:365
> +

Extra line.

> Source/WebCore/platform/mediastream/MediaEndpointSessionConfiguration.h:86
> +    PeerConnectionStates::BundlePolicy m_bundlePolicy = PeerConnectionStates::BundlePolicy::Balanced;

Use the { } notation.

[Carlos Alberto Lopez Perez]

Committed r214960: <http://trac.webkit.org/changeset/214960>