1328142 - Webrtc.org 49 update broke simulcast

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons]]

It seems like test_peerConnection_simulcastOffer.html was tweaked in the patch-rollup-patch of bug 1250356 to no longer test that we are receiving multiple simulcast streams.

Changing it back shows that our implementation is broken.

When debugging I see only one ssrc registered as received, although there should be two. The test changes confirm this as only the last simulcast stream that is scaled down is being received.

Comment 1

[Andreas Pehrson [:pehrsons]]

The received ssrcs seem based on received rtp packets so I assume we're simply not sending simulcast properly.

Comment 2

[Andreas Pehrson [:pehrsons]]

SimulcastEncoderAdapter::GetStreamBitrate() seems to be selecting only the lowest resolution stream on the first frame. This because the codec settings are:
(for the 50x50 stream) maxBitrate = 250, targetBitrate = 80, minBitrate = 40
(for the 25x25 stream) maxBitrate = 250, targetBitrate = 80, minBitrate = 40

To send the 25x25 one the code requires the 50x50 target and the 25x25 min bitrates, alas 80+40=120kbps.

This while it says we only have 80kbps available.


The test sets a maxBitrate of 40kbps on both simulcast encodings. This seems broken (code says 250kbps for both encodings).
The test forces a min_bitrate_estimate (by pref) to 100kbps. This seems broken (code says 80kbps).

Comment 3

[Andreas Pehrson [:pehrsons]]

The rollup patch also seems to have reverted bug 1320101.

Comment 4

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Fix VideoConduit warning: kf_request_method unused.

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

Comment 5

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Fix VideoConduit warning: Comparison between signed and unsigned ints.

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

Comment 6

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Fix VideoConduit warning: Unexpected format type.

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

Comment 7

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Remove unused min_bitrate_estimate pref.

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

Comment 8

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Pass in a cap when selecting bitrates on reconfigure.

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

Comment 9

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Restore test_pc_simulcastOffer.html.

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

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Remove unreachable return.

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

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Remove comment that wants to be removed.

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

Comment 12

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1328142 - Unify macros in MediaPipeline.

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

Comment 13

[Paul Kerr [:pkerr]]

Comment on attachment 8823290 [details]
Bug 1328142 - Fix VideoConduit warning: Comparison between signed and unsigned ints.

https://reviewboard.mozilla.org/r/101852/#review102364

Comment 14

[Paul Kerr [:pkerr]]

Comment on attachment 8823291 [details]
Bug 1328142 - Fix VideoConduit warning: Unexpected format type.

https://reviewboard.mozilla.org/r/101854/#review102366

Comment 15

[Paul Kerr [:pkerr]]

Comment on attachment 8823292 [details]
Bug 1328142 - Remove unused min_bitrate_estimate pref.

https://reviewboard.mozilla.org/r/101856/#review102368

Comment 16

[Paul Kerr [:pkerr]]

Comment on attachment 8823293 [details]
Bug 1328142 - Pass in a cap when selecting bitrates on reconfigure.

https://reviewboard.mozilla.org/r/101858/#review102370

Comment 17

[Paul Kerr [:pkerr]]

Comment on attachment 8823295 [details]
Bug 1328142 - Remove unreachable return.

https://reviewboard.mozilla.org/r/101862/#review102372

Comment 18

[Paul Kerr [:pkerr]]

Comment on attachment 8823296 [details]
Bug 1328142 - Remove comment that wants to be removed.

https://reviewboard.mozilla.org/r/101864/#review102374

Comment 19

[Paul Kerr [:pkerr]]

Comment on attachment 8823297 [details]
Bug 1328142 - Unify macros in MediaPipeline.

https://reviewboard.mozilla.org/r/101866/#review102378

Comment 20

[Paul Kerr [:pkerr]]

Comment on attachment 8823289 [details]
Bug 1328142 - Fix VideoConduit warning: kf_request_method unused.

https://reviewboard.mozilla.org/r/101850/#review102392

Comment 21

[Paul Kerr [:pkerr]]

Attachment: fix bug in VideoConduit::SetRemoteSSRC handling

Comment 22

[Paul Kerr [:pkerr]]

Attachment: enable switching between simulcast receive streams

Comment 23

[Paul Kerr [:pkerr]]

Comment on attachment 8824289 [details] [diff] [review]
enable switching between simulcast receive streams

This may be a little rough. I moved SetRemoteSSRC into the MediaConduit abstract class and stubbed it out in the WebrtcAudioConduit class because when the voice engine moved under the call api this call may need to be supported there.

The idea is when you call SelectSsrc it will change the SSRC that the VideoReceiveStream is expecting and pick up on of the other VP8 versions.

Comment 24

[Paul Kerr [:pkerr]]

Comment on attachment 8823294 [details]
Bug 1328142 - Restore test_pc_simulcastOffer.html.

https://reviewboard.mozilla.org/r/101860/#review103200

I made some changes to allow switching between the simulcast streams. An updated version of this test in that patch.

Comment 25

[Paul Kerr [:pkerr]]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=83246c811e6e45cc13427683bd1feb5b78557c9f

Comment 26

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking for 53. Is this planned to land on m-c before the merge next Monday?

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e9b98f63192
Restore test_pc_simulcastOffer.html. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/15a88f1ebc49
Fix VideoConduit warning: Comparison between signed and unsigned ints. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/902f9175ee2b
Fix VideoConduit warning: Unexpected format type. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/71041d19d2ae
Pass in a cap when selecting bitrates on reconfigure. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/4efc5eb60678
Remove comment that wants to be removed. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd5f8d67b620
Unify macros in MediaPipeline. r=pkerr
https://hg.mozilla.org/integration/mozilla-inbound/rev/4ea7585240cf
enable switching between simulcast receive streams r=jesup
https://hg.mozilla.org/integration/mozilla-inbound/rev/2233e03ef6fc
fix bug in VideoConduit::SetRemoteSSRC handling r=jesup

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e9b98f63192
https://hg.mozilla.org/mozilla-central/rev/15a88f1ebc49
https://hg.mozilla.org/mozilla-central/rev/902f9175ee2b
https://hg.mozilla.org/mozilla-central/rev/71041d19d2ae
https://hg.mozilla.org/mozilla-central/rev/4efc5eb60678
https://hg.mozilla.org/mozilla-central/rev/cd5f8d67b620
https://hg.mozilla.org/mozilla-central/rev/4ea7585240cf
https://hg.mozilla.org/mozilla-central/rev/2233e03ef6fc

Comment 29

[Andrei Vaida [:avaida]]

Setting qe-verify- since this fix seems to have automated coverage. Paul, if you think manual QA should instead be looking at this, feel free to flip the flag or ni? me.