Closed Bug 1416932 Opened 8 years ago Closed 8 years ago

Add tests to detect inclusion of negotiated RTP header extensions in RTP packets

Categories

(Core :: WebRTC, enhancement, P2)

Product:
Core
Component:
WebRTC
Version:
58 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mjf, Assigned: mjf)

References

Details

Attachments

(1 file)

Bug 1416932 - adding tests to detect RTP header extension negotiated are in RTP packets.
59 bytes, text/x-review-board-request
drno
: review+
Details
Add generic tests to verify that the rtp header extensions negotiated actually appear in the RTP packets now that we have a way to capture RTP packets in mochitests.
Rank: 19
Comment on attachment 8928027 [details] Bug 1416932 - adding tests to detect RTP header extension negotiated are in RTP packets. https://reviewboard.mozilla.org/r/199256/#review204310 I really would like to avoid randomly inspecting either the incoming or outgoing packet (see below). ::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html:5 (Diff revision 1) > <!DOCTYPE HTML> > <html> > <head> > <script type="application/javascript" src="pc.js"></script> > + <script type="application/javascript" src="parser_rtp.js"></script> Isn't this missing the sdputils.js import like in the other test below? ::: dom/media/tests/mochitest/test_peerConnection_basicAudio.html:22 (Diff revision 1) > test = new PeerConnectionTest(options); > test.setMediaConstraints([{audio: true}], [{audio: true}]); > // pc.js uses video elements by default, we want to test audio elements here > test.pcLocal.audioElementsOnly = true; > + > + let getRtpPacket = (pc) => { If I'm not mistaken this will just randomly dump any RTP packet, incoming or outgoing. And that sounds like it could cause intermittents at some points. I think you should clone these test cases (so you something which does only audio or video), and change them into sendonly cases (probably you only need to removed the gUM steps on the remote side to turn them into recvonly). That way we know for sure that we are verifying the incoming packets only (as you enabled the packet dumper on pcRemote._pc).
Attachment #8928027 - Flags: review?(drno) → review-
Comment on attachment 8928027 [details] Bug 1416932 - adding tests to detect RTP header extension negotiated are in RTP packets. https://reviewboard.mozilla.org/r/199256/#review204310 > Isn't this missing the sdputils.js import like in the other test below? Strange. It works just fine here without, but I'll add it to make sure we don't run into issues later. > If I'm not mistaken this will just randomly dump any RTP packet, incoming or outgoing. And that sounds like it could cause intermittents at some points. > > I think you should clone these test cases (so you something which does only audio or video), and change them into sendonly cases (probably you only need to removed the gUM steps on the remote side to turn them into recvonly). That way we know for sure that we are verifying the incoming packets only (as you enabled the packet dumper on pcRemote._pc). It only dumps received packets because the false in the following line indicates receive: pc.mozEnablePacketDump(0, "rtp", false); I will add a comment to make that clear. I will still move these to separate tests to make it clear what is failing.
Comment on attachment 8928027 [details] Bug 1416932 - adding tests to detect RTP header extension negotiated are in RTP packets. https://reviewboard.mozilla.org/r/199256/#review204688 LGTM
Attachment #8928027 - Flags: review?(drno) → review+
Pushed by mfroman@nostrum.com: https://hg.mozilla.org/integration/autoland/rev/a9cf53919ca9 adding tests to detect RTP header extension negotiated are in RTP packets. r=drno
https://hg.mozilla.org/mozilla-central/rev/a9cf53919ca9
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: