Closed Bug 1395853 Opened 7 years ago Closed 7 years ago

Add mochitests for codecs

Categories

(Core :: WebRTC: Audio/Video, enhancement, P4)

Product:
Core
Component:
WebRTC: Audio/Video
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: pehrsons, Assigned: pehrsons)

References

Details

Attachments

(5 files)

Bug 1395853 - Refactor checkReceivingToneFrom to take a cancel promise.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1395853 - Add a mochitest for audio codec content flow across a PeerConnection.
59 bytes, text/x-review-board-request
jib
: review+
Details
Bug 1395853 - Add convenience methods regarding codec ids to sdpUtils.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1395853 - Allow pt97 to be accepted as H264 in sdputils.
59 bytes, text/x-review-board-request
drno
: review+
Details
Bug 1395853 - Add a mochitest for video codec content flow across peerconnection.
59 bytes, text/x-review-board-request
jib
: review+
Details 
It would be great to have rudimentary mochitests of codecs to catch regressions.

We can force a codec by fiddling with the sdp, then we verify the content at the receiver like we do in many other tests.
Rank: 30
Priority: -- → P3
Assignee: nobody → apehrson
Status: NEW → ASSIGNED
Comment on attachment 8905116 [details]
Bug 1395853 - Refactor checkReceivingToneFrom to take a cancel promise.

https://reviewboard.mozilla.org/r/176922/#review182232

::: dom/media/tests/mochitest/pc.js:1555
(Diff revision 1)
>     *        the audio flow check.
>     * @returns {Promise}
>     *        A promise that resolves when we're receiving the tone from |from|.
>     */
> -  checkReceivingToneFrom : function(audiocontext, from) {
> -    var inputElem = from.localMediaElements[0];
> +  checkReceivingToneFrom : async function(audiocontext, from,
> +      cancel = wait(500, new Error("Tone not detected"))) {

So `500` was something I put on to test locally, then had a meeting, then forgot about.

Please read as `60000`.

I bet this is also why the try push is orange.
Depends on: 1399137
Comment on attachment 8907493 [details]
Bug 1395853 - Add convenience methods regarding codec ids to sdpUtils.

https://reviewboard.mozilla.org/r/179190/#review184718

Couple of suggestions for improvements. But otherwise looks good.

::: dom/media/tests/mochitest/sdpUtils.js:11
(Diff revision 2)
>  
> +// Finds the codec id / payload type given a codec format
> +// (e.g., "VP8", "VP9/90000"). `offset` tells us which one to use in case of
> +// multiple matches.
> +findCodecId: function(sdp, format, offset = 0) {
> +  let regex = new RegExp("rtpmap:\([0-9]+\) " + format, "g");

I would throw 'gi' as the flags in here, to make the RegExp case insensitive.

::: dom/media/tests/mochitest/sdpUtils.js:13
(Diff revision 2)
> +// (e.g., "VP8", "VP9/90000"). `offset` tells us which one to use in case of
> +// multiple matches.
> +findCodecId: function(sdp, format, offset = 0) {
> +  let regex = new RegExp("rtpmap:\([0-9]+\) " + format, "g");
> +  let match;
> +  for (let i = 0; i <= offset; ++i) {

The |i| is never used inside the loop, except in the error message. I doubt the offset actually does anything useful, right now.

::: dom/media/tests/mochitest/sdpUtils.js:14
(Diff revision 2)
> +// multiple matches.
> +findCodecId: function(sdp, format, offset = 0) {
> +  let regex = new RegExp("rtpmap:\([0-9]+\) " + format, "g");
> +  let match;
> +  for (let i = 0; i <= offset; ++i) {
> +		match = regex.exec(sdp);

Indentation appears off here.

::: dom/media/tests/mochitest/sdpUtils.js:61
(Diff revision 2)
> +removeAllButCodec: function(sdp, codec) {
> +  return sdp.replace(new RegExp("m=(\\w+ \\w+) UDP/TLS/RTP/SAVPF .*" + codec + ".*\\r\\n"),
> +                     "m=$1 UDP/TLS/RTP/SAVPF " + codec + "\r\n");
> +},
> +
> +removeRtpMapForCodec: function(sdp, codec) {
> +  return sdp.replace(new RegExp("a=rtpmap:" + codec + ".*\\r\\n", "g"), "");
> +},

These two actually operate on payload types, not codecs, right?
Can you rename the function names and input parameters to payloadType to cause less confusion?
Attachment #8907493 - Flags: review?(drno) → review+
Comment on attachment 8907494 [details]
Bug 1395853 - Allow pt97 to be accepted as H264 in sdputils.

https://reviewboard.mozilla.org/r/179192/#review184724
Attachment #8907494 - Flags: review?(drno) → review+
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
Comment on attachment 8907493 [details]
Bug 1395853 - Add convenience methods regarding codec ids to sdpUtils.

https://reviewboard.mozilla.org/r/179190/#review184718

> These two actually operate on payload types, not codecs, right?
> Can you rename the function names and input parameters to payloadType to cause less confusion?

True, but they're consistent with `removeCodec` which does the same. Should I rename it as well?
Comment on attachment 8907493 [details]
Bug 1395853 - Add convenience methods regarding codec ids to sdpUtils.

https://reviewboard.mozilla.org/r/179190/#review184718

> True, but they're consistent with `removeCodec` which does the same. Should I rename it as well?

I saw your new code actually iterating over codec names. That made me think this is actually payload types. As removing the rtpmap results in the codec and payload type no longer being considered removeCodec still seems to fit. 
I think it also makes sense to try to align everything. But I have no strong preference. Either way is fine with me.
Comment on attachment 8905116 [details]
Bug 1395853 - Refactor checkReceivingToneFrom to take a cancel promise.

https://reviewboard.mozilla.org/r/176922/#review186252

::: dom/media/tests/mochitest/pc.js:1577
(Diff revision 3)
> -      var outputMax = outputData.reduce(maxWithIndex, initial);
> +      if (error) {
> +        throw error;

This never cancels, because throw is swallowed by waitForAnalysisSuccess() [1]. Please fix that function, ideally by avoiding the promise constructor anti-pattern.

[1] http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/dom/media/tests/mochitest/head.js#132,141
Attachment #8905116 - Flags: review?(jib) → review-
Comment on attachment 8905117 [details]
Bug 1395853 - Add a mochitest for audio codec content flow across a PeerConnection.

https://reviewboard.mozilla.org/r/176924/#review186254

::: dom/media/tests/mochitest/test_peerConnection_audioCodecs.html:29
(Diff revision 3)
> +
> +    test.chain.insertBefore("PC_LOCAL_SET_LOCAL_DESCRIPTION", [
> +      function PC_LOCAL_FILTER_OUT_CODECS() {
> +        let id = sdputils.findCodecId(test.originalOffer.sdp, codec);
> +        test.originalOffer.sdp =
> +          sdputils.removeAllButCodec(test.originalOffer.sdp, id);

Just an observation that these tests rely on removeAllButCodec() working, or they'll always pass.

Would it be useful to add a negative test to make sure it works? Just an idea.

::: dom/media/tests/mochitest/test_peerConnection_audioCodecs.html:63
(Diff revision 3)
> +        ok(false, "Error in test for codec " + codec + ": " + e + "\n"
> +          + (e.stack ? e.stack : ""));

Template string maybe?

::: dom/media/tests/mochitest/test_peerConnection_audioCodecs.html:69
(Diff revision 3)
> +    try {
> +      networkTestFinished();
> +		} catch(e) {
> +			ok(false, "Error when finishing: " + e);
> +    }

Why this oddly indented no-op try/catch around a vestigial cleanup function that doesn't even run finish() synchronously or catch errors properly?
Attachment #8905117 - Flags: review?(jib) → review+
Comment on attachment 8907495 [details]
Bug 1395853 - Add a mochitest for video codec content flow across peerconnection.

https://reviewboard.mozilla.org/r/179194/#review186262

Nice tests!

::: dom/media/tests/mochitest/test_peerConnection_videoCodecs.html:55
(Diff revision 2)
> +    { name: "H264" },
> +    { name: "H264", offset: 1 },

Would love to see a sanity test of findCodecId() returning a different id for offset 1.

::: dom/media/tests/mochitest/test_peerConnection_videoCodecs.html:71
(Diff revision 2)
> +    try {
> +      networkTestFinished();
> +		} catch(e) {
> +			ok(false, "Error when finishing: " + e);
> +    }

remove no-op try/catch
Attachment #8907495 - Flags: review?(jib) → review+
Comment on attachment 8907493 [details]
Bug 1395853 - Add convenience methods regarding codec ids to sdpUtils.

https://reviewboard.mozilla.org/r/179190/#review184718

> The |i| is never used inside the loop, except in the error message. I doubt the offset actually does anything useful, right now.

I'm not sure what you would like me to do.
Comment on attachment 8905117 [details]
Bug 1395853 - Add a mochitest for audio codec content flow across a PeerConnection.

https://reviewboard.mozilla.org/r/176924/#review186254

> Just an observation that these tests rely on removeAllButCodec() working, or they'll always pass.
> 
> Would it be useful to add a negative test to make sure it works? Just an idea.

I added some checks to verify that we filtered the sdp correctly. Thinking a bit about how to remove all codecs and still make that pass the framework.

> Template string maybe?

Booyah, why didn't I knew about this before!

> Why this oddly indented no-op try/catch around a vestigial cleanup function that doesn't even run finish() synchronously or catch errors properly?

Consider the clutter gone.
Comment on attachment 8905116 [details]
Bug 1395853 - Refactor checkReceivingToneFrom to take a cancel promise.

https://reviewboard.mozilla.org/r/176922/#review189514

Very nice!

::: dom/media/tests/mochitest/pc.js:1561
(Diff revision 4)
> +      let inputData = inputAnalyser.getByteFrequencyData();
> +      let outputData = outputAnalyser.getByteFrequencyData();
> +
> +      let inputMax = inputData.reduce(maxWithIndex, initial);
> +      let outputMax = outputData.reduce(maxWithIndex, initial);

Sorry, I did notice one existing thing.

I had to look twice here to see if `initial` remained unscathed in each reduce here. I even had to look at what the remaining code does with the results, since the same actual `initial` object may end up being returned by either, or both.

I think isolating these details inside a single function instead would avoid these concerns.

    let maxWithIndex = data => {
      return data.reduce((a, b, i) => (b >= a.value) ? { value: b, index: i } : a,
                         { value: -1, index: -1 });
    };

It could also be done solely with indexes I think, e.g:

    let maxIndex = data => data.reduce((max, b, i) => (b >= data[max]) ? i : max, 0);

::: dom/media/tests/mochitest/pc.js:1575
(Diff revision 4)
>        if (Math.abs(inputMax.index - outputMax.index) < 10) {
> -        ok(true, "input and output audio data matches");
> +        return true;

Maybe a comment on why 10 is reasonable? (how big is the array)?

Also, maybe return the expression here instead of the implicit if() else return undefined?
Attachment #8905116 - Flags: review?(jib) → review+
Comment on attachment 8905116 [details]
Bug 1395853 - Refactor checkReceivingToneFrom to take a cancel promise.

https://reviewboard.mozilla.org/r/176922/#review189514

> Maybe a comment on why 10 is reasonable? (how big is the array)?
> 
> Also, maybe return the expression here instead of the implicit if() else return undefined?

I changed it to `inputData.length * 0.02` and a note on 2% being ~10 for sample size 512.
Pushed by pehrsons@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5eb3882e6de1
Refactor checkReceivingToneFrom to take a cancel promise. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/807225b4dc5b
Add convenience methods regarding codec ids to sdpUtils. r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/37ca45926c5b
Add a mochitest for audio codec content flow across a PeerConnection. r=jib
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0e575814c0
Allow pt97 to be accepted as H264 in sdputils. r=drno
https://hg.mozilla.org/integration/mozilla-inbound/rev/2791d2ecdc08
Add a mochitest for video codec content flow across peerconnection. r=jib
Blocks: 1407653
Andreas we don't have the real OpenH264 extension available in CI. My guess is that is the reason for the crashes over in bug 1407653.
We only have a fake plugin for GMP. But I doubt that can handle everything you try to in the video flow verification.

Not sure I understand how this could have ever passed in automation. Maybe the test machines actually sometimes succeed in downloading the OpenH264 extension before the test got run?!
Randell would be the best person to ask about more details for the fake GMP plugin.
See comment #44
Status: RESOLVED → REOPENED
Flags: needinfo?(apehrson)
Resolution: FIXED → ---
if there are changes to make or we need to install software on the machines, please let us know
Are you saying the fake codec doesn't give us frames that change color on the receiving side? (source on the sender is the fake video engine on all platforms so that is ever-changing) Only then can I see this being the test's fault.

Looks like it does reduce the source images to a per-plane average and passes that info over the wire, [1]. The fake decoder will then recreate the image and hand it to the application, [2]. That allows the test to pass. Could it be something else in the stack? I guess our gmp path is not particularly well tested. Also see bug 1408258.


[1] http://searchfox.org/mozilla-central/rev/bc6dddb88b1f34b54e22efc205846975fb4c04cb/dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp#186
[2] http://searchfox.org/mozilla-central/rev/bc6dddb88b1f34b54e22efc205846975fb4c04cb/dom/media/gmp-plugin-openh264/gmp-fake-openh264.cpp#438
Flags: needinfo?(apehrson) → needinfo?(drno)
I think the root cause for the problem got uncovered in bug 1407653. So lets close this one here and track fixing over in bug 1407653.
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Flags: needinfo?(drno)
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: