1250990 - Make RTCRtpEncodingParameters.maxBitrate and scaleResolutionDownBy work with H.264 in unicast at least.

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

The existing code for RTCRtpEncodingParameters.maxBitrate and scaleResolutionDownBy doesn't work with H.264 codepath in VideoConduit.cpp, probably because it didn't seem important in light of bug 1210175.

But these parameters are useful outside of simulcast, so this bug covers doing that work for regular non-simulcast (unicast) at least.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: MozReview Request: Bug 1250990 - Make RTCRtpEncodingParameters.scaleResolutionDownBy work with H.264 unicast.

Review commit: https://reviewboard.mozilla.org/r/36963/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36963/

Comment 2

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

Comment on attachment 8724283 [details]
MozReview Request: Bug 1250990 - Make RTCRtpEncodingParameters.scaleResolutionDownBy work with H.264 unicast.

https://reviewboard.mozilla.org/r/36963/#review33589

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cc3fae66ffb

Comment 4

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequent failure of modified test test_peerConnection_scaleResolution.html on Linux.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc8aa59e929

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=4cc3fae66ffb
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=22586498&repo=mozilla-inbound

23:21:39     INFO -  -1420821696[b7102380]: Flow[af15f357c99267bc:0,rtp(none)]; Layer[dtls]: Selected ALPN string: webrtc
23:21:41     INFO -  TEST-INFO | started process screentopng
23:21:42     INFO -  TEST-INFO | screentopng: exit 0
23:21:42     INFO -  46661 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | A valid string reason is expected
23:21:42     INFO -  46662 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | Reason cannot be empty
23:21:42     INFO -  46663 INFO Network setup is not required
23:21:42     INFO -  46664 INFO testing scaling with VP8
23:21:42     INFO -  46665 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | v2.currentTime is zero at outset
23:21:42     INFO -  46666 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | Invalid scaleResolutionDownBy must reject
23:21:42     INFO -  46667 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | v2.currentTime is moving (0.374421768707483)
23:21:42     INFO -  46668 INFO TEST-FAIL | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | The author of the test has indicated that flaky timeouts are expected.  Reason: WebRTC inherently depends on timeouts
23:21:42     INFO -  46669 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | source width is positive
23:21:42     INFO -  46670 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | source height is positive
23:21:42     INFO -  46671 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | sink is half the width of source
23:21:42     INFO -  46672 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | sink is half the height of source
23:21:42     INFO -  46673 INFO testing scaling with H264
23:21:42     INFO -  46674 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | v2.currentTime is zero at outset
23:21:42     INFO -  46675 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | Invalid scaleResolutionDownBy must reject
23:21:42     INFO -  46676 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | v2.currentTime is moving (0.44117913832199546)
23:21:42     INFO -  46677 INFO TEST-FAIL | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | The author of the test has indicated that flaky timeouts are expected.  Reason: WebRTC inherently depends on timeouts
23:21:42     INFO -  46678 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | source width is positive
23:21:42     INFO -  46679 INFO TEST-PASS | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | source height is positive
23:21:42     INFO -  46680 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | sink is half the width of source - got 640, expected 160
23:21:42     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
23:21:42     INFO -      testScale/</<@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:63:9
23:21:42     INFO -      promise callback*testScale/<@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:49:14
23:21:42     INFO -      promise callback*testScale@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:39:12
23:21:42     INFO -      promise callback*@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:74:24
23:21:42     INFO -      runNetworkTest/</</<@dom/media/tests/mochitest/pc.js:1799:21
23:21:42     INFO -      promise callback*runNetworkTest/</<@dom/media/tests/mochitest/pc.js:1798:7
23:21:42     INFO -      runTestWhenReady/<@dom/media/tests/mochitest/head.js:313:41
23:21:42     INFO -      promise callback*runTestWhenReady@dom/media/tests/mochitest/head.js:313:10
23:21:42     INFO -      runNetworkTest/<@dom/media/tests/mochitest/pc.js:1797:5
23:21:42     INFO -      promise callback*runNetworkTest@dom/media/tests/mochitest/pc.js:1796:10
23:21:42     INFO -      @dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:74:3
23:21:42     INFO -  Not taking screenshot here: see the one that was previously logged
23:21:42     INFO -  46681 INFO TEST-UNEXPECTED-FAIL | dom/media/tests/mochitest/test_peerConnection_scaleResolution.html | sink is half the height of source - got 480, expected 120
23:21:42     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:267:5
23:21:42     INFO -      testScale/</<@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:64:9
23:21:42     INFO -      promise callback*testScale/<@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:49:14
23:21:42     INFO -      promise callback*testScale@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:39:12
23:21:42     INFO -      promise callback*@dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:74:24
23:21:42     INFO -      runNetworkTest/</</<@dom/media/tests/mochitest/pc.js:1799:21
23:21:42     INFO -      promise callback*runNetworkTest/</<@dom/media/tests/mochitest/pc.js:1798:7
23:21:42     INFO -      runTestWhenReady/<@dom/media/tests/mochitest/head.js:313:41
23:21:42     INFO -      promise callback*runTestWhenReady@dom/media/tests/mochitest/head.js:313:10
23:21:42     INFO -      runNetworkTest/<@dom/media/tests/mochitest/pc.js:1797:5
23:21:42     INFO -      promise callback*runNetworkTest@dom/media/tests/mochitest/pc.js:1796:10
23:21:42     INFO -      @dom/media/tests/mochitest/test_peerConnection_scaleResolution.html:74:3

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This is probably bug 1248154, i.e. an error in measuring, not the actual code.

I've increased the workaround delay from 1 second to 3 seconds, lets see if it makes a difference:

Before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e01fc9d71c&selectedJob=17322747
After: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cdbf43be80c&selectedJob=17334829

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

That actually did work, but at the cost of almost perma-oranging the existing intermittents bug 1207431 and bug 1224029! Timing + media tests == house of cards.

Looks like I'll have to burn out the test instead until bug 1248154 has been fixed.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8724283 [details]
MozReview Request: Bug 1250990 - Make RTCRtpEncodingParameters.scaleResolutionDownBy work with H.264 unicast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36963/diff/1-2/

Comment 8

[Maire Reavy [:mreavy]]

Jan-Ivar -- Is this ready to land?

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8724283 [details]
MozReview Request: Bug 1250990 - Make RTCRtpEncodingParameters.scaleResolutionDownBy work with H.264 unicast.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36963/diff/2-3/

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

I've been trying to track down bug 1207431 to no avail so far because it perma-oranged (see comment 6). Now that that's been backed out, I've rebased in comment 9 (also includes the 3 second delay from comment 5 to work around bug 1248154) and done another try to see if it fares better now.

So the code is ready AFAIK, but I need to clear these two other intermittents that near-perma-orange with my change. House of cards problem.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f1b196cd973

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8f1b196cd973