1040346 - GMP fake plugin test should test that the GMP plugin actually launches and does something

Status: RESOLVED FIXED

(Core :: WebRTC: Audio/Video, defect)

Description

[Benjamin Smedberg]

I added a test of the fake GMP plugin in bug 1037125. This test currently just waits for webrtc to claim a connection is established, and doesn't actually check that we are receiving decoded frames. It should do more checking.

Comment 1

[Maire Reavy [:mreavy]]

Jesup's already in this code, so he wants to finish this off.

Comment 2

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

Attachment: Basic webrtc smoketests for GMP fake H.264 plugin

Comment 3

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

Attachment: Basic webrtc smoketests for GMP fake H.264 plugin

Comment 4

[Nils Ohlmeier [:drno]]

Comment on attachment 8459201 [details] [diff] [review]
Basic webrtc smoketests for GMP fake H.264 plugin

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

Its ok to land this like it is, although I would prefer to have my comments for pc.js addressed. Depending on the time pressure we can also address them later.
Let me know if I should make the changes and carry this over the finish line for you.

::: dom/media/tests/mochitest/pc.js
@@ +359,5 @@
>  
> +// Also remove mode 0 if it's offered
> +// Note, we don't bother removing the fmtp lines, which makes a good test
> +// for some SDP parsing issues.
> +function removeVP8(sdp) {

Wondering if should make this a generic helpers which is able to remove any given codec number from a given SDP.

@@ +365,5 @@
> +  updated_sdp = updated_sdp.replace("RTP/SAVPF 120 126 97\r\n","RTP/SAVPF 126 97\r\n");
> +  updated_sdp = updated_sdp.replace("RTP/SAVPF 120 126\r\n","RTP/SAVPF 126\r\n");
> +  updated_sdp = updated_sdp.replace("a=rtcp-fb:120 nack\r\n","");
> +  updated_sdp = updated_sdp.replace("a=rtcp-fb:120 nack pli\r\n","");
> +  updated_sdp = updated_sdp.replace("a=rtcp-fb:120 ccm fir\r\n","");

Can we remove all of them with one statement?

@@ +1355,5 @@
>   *        Description for the peer connection instance
>   * @param {object} configuration
>   *        Configuration for the peer connection instance
>   */
> +function PeerConnectionWrapper(label, configuration, h264) {

I would really prefer to have the H264 things as a PCW function. The list of things we might want to modify and/or check can easily grow fast otherwise.

::: dom/media/tests/mochitest/test_peerConnection_basicH264Video.html
@@ +20,5 @@
> +  var test;
> +  runNetworkTest(function (options) {
> +    options = options || { };
> +    options.h264 = true;
> +    test = new PeerConnectionTest(options);

A function call instead of setting it via the options would be preferred from my perspective (see comment pc.js).

Comment 5

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

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

Comment 6

[Ryan VanderMeulen [:RyanVM]]

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