1965476 - Assertion failure in VideoSendStreamImpl::ReconfigureVideoEncoder after content type change

Status: ASSIGNED

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

Description

[Andreas Pehrson [:pehrsons]]

I hit this in a local debug build on Google Meet, after starting a screenshare.

I'd believe this is caused by replaceTrack, replacing a camera track with a screen capture track. But Meet does send both the camera track and the screenshare simultaneously so it doesn't entirely make sense. At least it's clear that a video send stream has been set up for realtime video (non-screensharing) and then is reconfigured for screensharing. This change is supported in the encoder, which is below the send stream, where it's forbidden.

Comment 1

[Andreas Pehrson [:pehrsons]]

The biggest problem with changing from encoder reconfiguration to send stream recreation is that the latter triggers RTCP BYE. See bug 1870643.

Comment 2

[Andreas Pehrson [:pehrsons]]

Our options seem to be:

• live with not reconfiguring pacing, bitrate allocation, etc., and fail the assert in debug builds, i.e. do nothing
• let content type changes erroneously trigger RTCP BYE
• stop sending RTCP BYE altogether, i.e. adopt upstream and remove our undeleting patch
• fix upstream so there's another path to trigger sending RTCP BYE, and stop sending them per the point just above
• fix upstream so the send stream picks up content_type changes

Note there's no progress upstream on proper RTCP BYE.

I think the 4th point is the only way we can make our use of RTCP BYE spec compliant.

Michael/Byron, thoughts?

Comment 3

[Byron Campen [:bwc]]

Does the ssrc change when we do this?

Comment 4

[Andreas Pehrson [:pehrsons]]

No, not for any of the options.

Comment 5

[Byron Campen [:bwc]]

Ok, is there any way to condition sending BYE on whether the ssrc has changed?

Comment 6

[Michael Froman [:mjf]]

I agree that point 4 is probably the right way forward - I never expected upstream to let that bug sit for so long.

Comment 7

[Andreas Pehrson [:pehrsons]]

(In reply to Byron Campen [:bwc] from comment #5)

Ok, is there any way to condition sending BYE on whether the ssrc has changed?

The ssrc cannot change for a video send stream. If it changes for a sender we recreate the send stream, and destroying the send stream triggers the RTCP BYE.

Comment 8

[Byron Campen [:bwc]]

If libwebrtc is sending BYE, and then sending more packets on that ssrc, that's pretty broken, yeah. I guess we could recreate and select a new ssrc for this case, but that's probably not in line with the "seamless" language in webrtc-pc. I wonder what other situations libwebrtc is willing to send a bad BYE.

Comment 9

[Andreas Pehrson [:pehrsons]]

libwebrtc no longer sends any BYE. That was removed in https://issues.webrtc.org/issues/42221166. We undeleted those changes in our local copy, so probably violate the spec similarly (i.e. on encodingParameters.active = false).

In our libwebrtc copy, BYE is only sent here.

I'm leaning towards removing that undeletion patch, i.e. point 3 above, as a short-term fix. It will regress the mute-on-bye test, but that feature is prefed off anyway. I don't have cycles to look into point 4 and doing it proper. Chrome not doing it either indicates it's not something services rely on.

Comment 10

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1965476 - Don't send BYE on send stream recreation. r?#webrtc-reviewers

Comment 11

[Andreas Pehrson [:pehrsons]]

Attachment: Bug 1965476 - Recreate the video send stream on VideoEncoderConfig::ContentType changes. r?dbaker