1521169 - Crash in mozilla::VideoStreamFactory::CreateEncoderStreams

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P2)

Description

[Marcia Knous [:marcia]]

This bug is for crash report bp-f76ca62b-0c9d-471c-8ff3-cb54a0190118.

Small volume cross platform regression that appears to have started around 20181229214252: https://bit.ly/2szzHFz. Looks as if this was added in Bug 1511509. Maybe we are supposed to crash? ni on :dminor for clarification.

Moz crash reason for all crashes is MOZ_RELEASE_ASSERT(streams.size()) (Should configure at least one stream)

Top 10 frames of crashing thread:

0 XUL mozilla::VideoStreamFactory::CreateEncoderStreams media/webrtc/signaling/src/media-conduit/VideoStreamFactory.cpp:247
1 XUL webrtc::VideoStreamEncoder::ReconfigureEncoder media/webrtc/trunk/webrtc/video/video_stream_encoder.cc:573
2 XUL webrtc::VideoStreamEncoder::EncodeTask::Run media/webrtc/trunk/webrtc/video/video_stream_encoder.cc:782
3 XUL rtc::TaskQueue::Impl::TaskContext::RunTask media/webrtc/trunk/webrtc/rtc_base/task_queue_gcd.cc:91
4  @0x7fff6bb0bdca 
5  @0x7fff6bb1211f 
6  @0x7fff6bb12bd7 
7  @0x7fff6bb1b083 
8  @0x7fff6bd4b61b 
9  @0x7fff6bd4b414 

Comment 1

[Dan Minor [:dminor]]

Thank you for noticing this. I added this assertion because when I had a first look at bug 1511509 I saw that there is code that assumes that at least one stream is configured and that could cause undefined behaviour if that was not the case, so it seemed better to crash.

I haven't prioritized looking at these crashes because they are low volume, but they do indicate a logic problem with how we create streams and are important to fix eventually.

We still see crashes in Bug 1511509 since this landed, so it seems there are two separate problems.

Comment 3

[Dan Minor [:dminor]]

Attachment: Bug 1521169 - Check that at least one stream is requested in ConfigureSendMediaCodec; r=pehrsons!

VideoStreamFactory::CreateEncoderStreams is expected to create at least one
stream, but this is not always happening. It seems that the only way
that this could occur is if ConfigureSendMediaCodec is called with no
streams in the codecConfig. The VideoEncoderConfig that is passed into
mSendStream->ReconfigureVideoEncoder is not modified prior to reaching
CreateEncoderStreams. We do drop streams in CreateEncoderStreams, but only if
the call to AdaptFrameResolution fails or we end up with a bad aspect ratio.
Since we do not scale the first stream, it should never end up being dropped.

Comment 4

[Neha Kochar [:neha]]

Dan, the patch here is from a month ago. Is this a good fit for uplift to 67 beta?

Comment 5

[Dan Minor [:dminor]]

(In reply to Neha Kochar [:neha] from comment #4)

Dan, the patch here is from a month ago. Is this a good fit for uplift to 67 beta?

No, this needs more work. I'm spending time on it this afternoon.

Comment 6

[Dan Minor [:dminor]]

Attachment: Bug 1521169 - Rename SimulcastEncoding to Encoding in VideoCodecConfig; r=pehrsons!

The Transceivers code just calls these encodings, and we use them even if we're
not doing simulcast, so we should just call them encodings as well.

Comment 7

[Dan Minor [:dminor]]

Attachment: Bug 1521169 - Remove VideoEncoderConfigBuilder; r=pehrsons!

The SimulcastStreamConfig code is unused. Once that is removed, all that remains
is a thin wrapper around webrtc::VideoEncoderConfig. We might as well use it
directly.

Depends on D24548

Comment 8

[Dan Minor [:dminor]]

Attachment: Bug 1521169 - Ensure that streamCount is at least one in CreateEncoderStreams; r=pehrsons!

We currently have a check that we create at least one stream in
CreateEncoderStreams. This is failing occasionally. This adds a new check to the
top of the method to check that at least one stream has been requested. This will
help to narrow down where the problem is occurring.

Depends on D20548

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebc9076ea6a5
Rename SimulcastEncoding to Encoding in VideoCodecConfig; r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac642c49a783
Remove VideoEncoderConfigBuilder; r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/51b23951731b
Check that at least one stream is requested in ConfigureSendMediaCodec; r=pehrsons
https://hg.mozilla.org/integration/mozilla-inbound/rev/e975f7793042
Ensure that streamCount is at least one in CreateEncoderStreams; r=pehrsons

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/ebc9076ea6a5
https://hg.mozilla.org/mozilla-central/rev/ac642c49a783
https://hg.mozilla.org/mozilla-central/rev/51b23951731b
https://hg.mozilla.org/mozilla-central/rev/e975f7793042

Comment 11

[Pascal Chevrel:pascalc]

We are at the beta midpoint and we don't have enough volume of crashes on Nightly to evaluate the potential positive impact an uplift would have on late betas, this doesn't look like a good candidate for uplift, so I am marking it as wontfix for 67.

Comment 12

[Dan Minor [:dminor]]

Looking at a recent crash [1] we pass the assertion at [2] but fail at [3].

[1] https://crash-stats.mozilla.com/report/index/feb7b4ab-6829-4051-9994-7ed5e0190403
[2] https://hg.mozilla.org/mozilla-central/annotate/ea09774456976ada64ed753a00724ef6bc6be62f/media/webrtc/signaling/src/media-conduit/VideoStreamFactory.cpp#l127
[3] https://hg.mozilla.org/mozilla-central/annotate/ea09774456976ada64ed753a00724ef6bc6be62f/media/webrtc/signaling/src/media-conduit/VideoStreamFactory.cpp#l249

Comment 13

[Dan Minor [:dminor]]

The only explanation I can think of is that the input width and height to CreateEncoderStreams are zero, causing us to skip adding the highest resolution stream at [1]. Since CreateEncoderStreams is expected to return at least one stream, I think we should only skip adding streams if the call to mSimulcastAdapter->AdaptFrameResolution() fails.

What we might be missing is a check somewhere else that would prevent us from calling VideoStreamEncoder::ReconfigureEncoder if last_frame_info_ had zero width and height.

[1] https://searchfox.org/mozilla-central/rev/6dab6dad9cc852011a14275a8b2c2c03ed7600a7/media/webrtc/signaling/src/media-conduit/VideoStreamFactory.cpp#187

Comment 14

[Dan Minor [:dminor]]

A quick check forcing zero width and zero height streams in CreateEncoderStreams causes all sorts of problems, so we'll need to figure out a way of preventing ReconfigureEncoder from being called if we don't have valid width and height.

Comment 15

[Dan Minor [:dminor]]

Attachment: Bug 1521169 - Drop frames with zero width or height in EncodeVideoFrame; r=ng!

We sometimes see release assertion failures in CreateEncoderStreams caused by
not configuring any streams. It appears that this is happening because the
input width and height are zero, and we skip creating streams with zero width
and height.

This drops frames with zero width or height in EncodeVideoFrame, which should
avoid triggering the assertion. The most likely explanation for the bad frames
are from desktop capture, where we've had problems in the past with transient
invalid width and height when a window is minimized or maximized. Chromium
has code to resize small desktop capture frames to be at least 2x2 which would
explain why they do not see this problem. That said, it is still possible that
bad frames are being generated by the video capture code.

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

This is P2 and assigned, so I'm removing it from weekly regression triage by marking it fix-optional.
Happy to take a patch for 70 or in future.

Comment 17

[Marcia Knous [:marcia]]

All of the URLs in this crash are either https://meet.google.com or https://hangouts.google.com.

Comment 18

[Marcia Knous [:marcia]]

71 is affected, but only 1-2 crashes per build, and not consistently every day.

Comment 19

[Pascal Chevrel:pascalc]

4 crashes in 71 Nightly only affecting 2 users (one Linux user, one Mac user), marking as fix-optional for 71 given the low volume.

Comment 20

[km]

Steps to reproduce this crash on Ubuntu 18.04, Gnome Desktop (Xorg), Firefox 73.0.1+build1-0ubuntu0.18.04.1

1. Open a Google Hangouts video call at https://hangouts.google.com/
2. Click "Share screen" from the call menu at the top right
3. Pick any window
4. Press ctrl-alt-down to switch desktops

Comment 21

[Dan Minor [:dminor]]

(In reply to km from comment #20)

Steps to reproduce this crash on Ubuntu 18.04, Gnome Desktop (Xorg), Firefox 73.0.1+build1-0ubuntu0.18.04.1

1. Open a Google Hangouts video call at https://hangouts.google.com/
2. Click "Share screen" from the call menu at the top right
3. Pick any window
4. Press ctrl-alt-down to switch desktops

Awesome, thank you so much! Reproduces for me as well.

Comment 22

[Dan Minor [:dminor]]

Since this bug already has landed patches, I've filed Bug 1620660 to handle fixing this.

Comment 23

[Dan Minor [:dminor]]

Sorry, STR are on comment 20 on the other bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1521169#c20.

Comment 24

[Dan Minor [:dminor]]

It looks like the fix from Bug 1620660 worked, there are no crashes on Beta since it merged.