Closed Bug 1003994 Opened 7 years ago Closed 7 years ago

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


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






(Reporter: jesup, Assigned: emannion)



(Whiteboard: [p=2, s=Fx32])


(1 file, 1 obsolete file)

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.
Whiteboard: [p=3, s=Fx32]
Maire looking for owner
Priority: -- → P1
Target Milestone: --- → mozilla32
Assignee: nobody → emannion
Whiteboard: [p=3, s=Fx32] → [p=2, s=Fx32]
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
Attachment #8427696 - Flags: review?(rjesup)
Attachment #8427696 - Flags: review?(ethanhugg)
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("")) {
>      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
Attachment #8427696 - Flags: review?(rjesup) → review+
This patch updates the previous patch with minor updates from review comments.
Attachment #8427696 - Attachment is obsolete: true
Attachment #8427696 - Flags: review?(ethanhugg)
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.
Attachment #8428588 - Flags: review+
The signaling unittests do not run on the builders, but here's a try to make sure this all builds:
Submitting for checkin.  Try is in comment #6.
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.