1683934 - MOZ_DIAGNOSTIC_ASSERT(mSendStreamConfig.rtp.ssrcs.size() == mEncoderConfig.number_of_streams) in some cases

Status: RESOLVED DUPLICATE of bug 1401592

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

Description

[Nazar Mokrynskyi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:86.0) Gecko/20100101 Firefox/86.0

Steps to reproduce:

1. Use VP8 codec
2. Enable 4-layer simulcast like this:
   [
   { scaleResolutionDownBy: 4, maxBitrate: 100_000 },
   { scaleResolutionDownBy: 3, maxBitrate: 350_000 },
   { scaleResolutionDownBy: 2, maxBitrate: 1_250_000 },
   { scaleResolutionDownBy: 1, maxBitrate: 1_500_000 },
   ]

Actual results:

Browser tab crashes.
Stable: https://crash-stats.mozilla.org/report/index/4fc88169-88ee-4246-b88b-a06050201222
Nightly: https://crash-stats.mozilla.org/report/index/b22b3750-22ad-421a-9548-26da90201222

Expected results:

Browser shouldn't ever crash

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Dan Minor [:dminor]]

Thanks for the bug report!

Looks like a release assert is failing in libwebrtc here: https://hg.mozilla.org/mozilla-central/file/fbdb6c91bd6256415a54c5198fb1e3bff8dd7c64/third_party/libwebrtc/webrtc/modules/video_coding/generic_encoder.cc#l407.

Andreas, any thoughts on this one?

Comment 3

[Andreas Pehrson [:pehrsons]]

Nothing immediate. It rings a bit like an ordering issue, but I'm not very familiar with this upstream code so I cannot really say.

I'll try to reproduce within a mochitest.

Comment 4

[Andreas Pehrson [:pehrsons]]

That was easy to reproduce in test_peerConnection_simulcastAnswer_lowResFirst.html by adding two more encodings.

Pernosco recording: https://pernos.co/debug/4-NL4PI6qdYxF34mrir0iw/index.html

Comment 5

[Andreas Pehrson [:pehrsons]]

Uhm, so yeah, this is a Chrome experiment (?!).

The assertion fails because the simulcast id field only allows two bits for the id, and 0 means no simulcast. In those two bits only three unique simulcast streams can be represented. Upstream looks fundamentally similar, though paths are unclear.

But why?

Well this field ends up setting an extension field on the last packet of every keyframe. It only takes effect if the extension was negotiated.

The docs for that extension are here. I'm not sure what it's needed for. Originally it seems to be for distinguishing a screenshare stream from a non-screenshare one, but somewhere along the way simulcast and experiment ids were added (and undocumented).

What's disturbing is that we hit this release assert even when this extension is not part of signaling. Then the content type would simply be ignored. I wouldn't be surprised if we don't support signalling this extension either.

Dan, thoughts?

Comment 6

[Dan Minor [:dminor]]

I dug through this a bit more and it looks like maybe this is (or was?) used for stats. I'm not sure if this is something we want to fix while we are doing an update.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 8

[Nazar Mokrynskyi]

No crashes reported probably because no one bothered to trigger it, but it doesn't mean the issue is fixed.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 10

[Nazar Mokrynskyi]

Looks like no one bothered to trigger it again, but I believe the issue is still present and needs to be fixed properly.

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 12

[Byron Campen [:bwc]]

So this function no longer exists after the libwebrtc update. Maybe this bug is gone now? I'll try testing as described in comment 4.

Comment 13

[Byron Campen [:bwc]]

So we hit a different crash now, here: https://searchfox.org/mozilla-central/rev/25d26b0a62cc5bb4aa3bb90a11f3b0b7c52859c4/dom/media/webrtc/libwebrtcglue/VideoConduit.cpp#930-932

This is because there is a hard limit, here: https://searchfox.org/mozilla-central/rev/25d26b0a62cc5bb4aa3bb90a11f3b0b7c52859c4/dom/media/webrtc/libwebrtcglue/VideoConduit.cpp#661-662

Which happens to be 3: https://searchfox.org/mozilla-central/rev/25d26b0a62cc5bb4aa3bb90a11f3b0b7c52859c4/third_party/libwebrtc/api/video/video_codec_constants.h#17

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.

Comment 16

[Andreas Pehrson [:pehrsons]]

I see in the spec,

Under 4.4.1.5 Set the session description:

Let supportedEncodings be the maximum number of encodings that the implementation can support. If the length of proposedSendEncodings is greater than supportedEncodings, truncate proposedSendEncodings so that its length is supportedEncodings.

And in the addTransceiver algorithm:

If the number of encodings stored in sendEncodings exceeds maxN, then trim sendEncodings from the tail until its length is maxN.

Byron, would bug 1401592 affect putting these guards in place?

Comment 17

[Byron Campen [:bwc]]

(In reply to Andreas Pehrson [:pehrsons] from comment #16)

I see in the spec,

Under 4.4.1.5 Set the session description:

Let supportedEncodings be the maximum number of encodings that the implementation can support. If the length of proposedSendEncodings is greater than supportedEncodings, truncate proposedSendEncodings so that its length is supportedEncodings.

And in the addTransceiver algorithm:

If the number of encodings stored in sendEncodings exceeds maxN, then trim sendEncodings from the tail until its length is maxN.

Byron, would bug 1401592 affect putting these guards in place?

Yes, it does.

Comment 18

[BugBot [:suhaib / :marco/ :calixte]]

The bug is linked to a topcrash signature, which matches the following criterion:

• Top 5 desktop browser crashes on Mac on beta

:bwc, could you consider increasing the severity of this top-crash bug?

For more information, please visit auto_nag documentation.

Comment 19

[BugBot [:suhaib / :marco/ :calixte]]

Based on the topcrash criteria, the crash signatures linked to this bug are not in the topcrash signatures anymore.

For more information, please visit auto_nag documentation.

Comment 20

[Byron Campen [:bwc]]

Bug 1401592 fixed more-than-3 hitting this assertion, but there are apparently other ways.

Comment 21

[Mathew Hodson]

Is this a duplicate of bug 1799978 now?

Comment 22

[Byron Campen [:bwc]]

It might be. It might also be that bug 1401592 happened to fix 1799978 as well.