1003994 - Add tests for SDP offer and answer with multiple video codecs and for H.264

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P1)

Description

[Randell Jesup [:jesup] (needinfo me)]

We should add more tests for multiple video codecs, and for H.264, to the offer/answer unit tests.  (and multiple audio codecs if they aren't tested)

Bug 985253 was blocked by errors in handling of any video codec other than VP8, and by an instance of mis-handling of responses with rtcp-fb for H.264 (a function was returning 255 as the packetization mode when it was unspecified, instead of the default mode 0).

Likely the unit tests need to be genericised quite a bit (hard-coded 120's, etc).

Also, it should check that everything works if the SDP from the other end doesn't use the "preferred" values for PT.  The code should properly do dynamic PT translation + defaults and appears to, but I suspect it isn't tested.

Comment 1

[:shell escalante]

Maire looking for owner

Comment 2

[enda mannion (:emannion)]

Attachment: All existing SDP tests work now when H.264 is enabled. Six new pretty much happy path tests have been added.

6 new tests are.
- Validate offer with multiple video codecs
- Remove VP8 from Offer, ensure H.264 P1 is negotiated
- Insert H.264 before VP8 in offer, validate answer
- Remove H.264 P1 and VP8 from offer, check answer negotiates H.264 P0
- Test negotiating an answer which has only H.264 P1
- Test using a non preferred dynamic video payload type on answer negotiation

More tests could be added, like ones that validate bad SDP.

These tests ensure H.264 is turned on for tests, which I think I have done by my change to PeerConnectionCtx.cpp

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8427696 [details] [diff] [review]
All existing SDP tests work now when H.264 is enabled. Six new pretty much happy path tests have been added.

Review of attachment 8427696 [details] [diff] [review]:
-----------------------------------------------------------------

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +268,5 @@
>    if (Preferences::GetBool("media.peerconnection.video.h264_enabled")) {
>      codecMask |= VCM_CODEC_RESOURCE_H264;
>    }
> +#else
> +  codecMask |= VCM_CODEC_RESOURCE_H264;

I realize this is meant to be "if this is a unit test", and in the future we'll have an "if openh264 is enabled".  However, this is pretty non-obvious.  A comment is the minimum needed.

::: media/webrtc/signaling/src/sipcc/core/common/prot_configmgr.c
@@ +626,5 @@
>        aSupportedCodecs[count] = RTP_VP8;
>        count++;
>      }
> +#else
> +    // Apply video codecs with VP8 first on non gonk

We'll need to make this more dynamic, but I think this will do for now

Comment 4

[enda mannion (:emannion)]

Attachment: All existing SDP tests now work when H.264 is enabled. Six new mainly happy path tests added. Incorporate review comments.

This patch updates the previous patch with minor updates from review comments.

Comment 5

[Ethan Hugg [:ehugg]]

Comment on attachment 8428588 [details] [diff] [review]
All existing SDP tests now work when H.264 is enabled. Six new mainly happy path tests added. Incorporate review comments.

Review of attachment 8428588 [details] [diff] [review]:
-----------------------------------------------------------------

Bringing forward r+ from jesup and adding my own.  Tested on both OSX and Linux.

Comment 6

[Ethan Hugg [:ehugg]]

The signaling unittests do not run on the builders, but here's a try to make sure this all builds:

https://tbpl.mozilla.org/?tree=Try&rev=d1cc0901d7d8

Comment 7

[Ethan Hugg [:ehugg]]

Submitting for checkin.  Try is in comment #6.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac35e227a51f

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ac35e227a51f