1620660 - Drop frames in VideoConduit if they are cropped to zero width or height

Status: VERIFIED FIXED

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

Description

[Dan Minor [:dminor]]

Failing to drop frames if they are cropped to zero width and height leads to the crash seen in Bug 1521169. We get a call to CreateEncoderStreams requesting streams with zero width and height at which point it is too late to do anything, because the webrtc.org code making the call always expects CreateEncoderStreams to always return at least one stream, and can't handle streams with zero width or height.

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1620660 - Use streamCount consistently in CreateEncoderStreams; r=ng!

Comment 2

[Dan Minor [:dminor]]

Attachment: Bug 1620660 - Drop frames in VideoConduit if they are cropped to zero width or height; r=ng!

Failing to drop frames if they are cropped to zero width and height leads to
the crash seen in Bug 1521169. We get a call to CreateEncoderStreams
requesting streams with zero width and height at which point it is too late
to do anything, because the webrtc.org code making the call always expects
CreateEncoderStreams to always return at least one stream, and it can't
handle streams with zero width or height.

Depends on D65799

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cf995779fc05
Use streamCount consistently in CreateEncoderStreams; r=ng
https://hg.mozilla.org/integration/autoland/rev/649a2a4d16d0
Drop frames in VideoConduit if they are cropped to zero width or height; r=ng

Comment 4

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/cf995779fc05
https://hg.mozilla.org/mozilla-central/rev/649a2a4d16d0

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Is this something we can/should uplift? We're seeing bug 1521169 occurring at higher than usual rates on Release at the moment.

Comment 6

[Dan Minor [:dminor]]

Comment on attachment 9131572 [details]
Bug 1620660 - Drop frames in VideoConduit if they are cropped to zero width or height; r=ng!

Beta/Release Uplift Approval Request

• User impact if declined: Crashes like those seen in 1521169. The trigger seems to be minimizing or otherwise hiding a window that is shared on Google Meet/Hangouts.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: I tested using the steps from comment 20 on Ubuntu 19.10. I expect this same bug would show up on other operating systems if a window is shared on hangouts/meet and then minimized, but it would be great to have that verified.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is low risk because this drops in results in dropping an invalid frame. The current code already allows for invalid frames to be dropped as a result of cropping, so this should be safe.
• String changes made/needed: None

Comment 7

[Dan Minor [:dminor]]

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

Comment 8

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash using Fx 73.0.1 - https://crash-stats.mozilla.org/report/index/02d27b58-2cb5-485f-b49f-258d60200313 on Ubuntu 18.04 64bit.
Verified that the crash does not occur anymore using latest Nightly 76.0a1 (2020-03-12) on Ubuntu 18.04 64bit. Leaving QA+ on this one until all Fx versions get fixed.

Comment 9

[Julien Cristau [:jcristau]]

Comment on attachment 9131571 [details]
Bug 1620660 - Use streamCount consistently in CreateEncoderStreams; r=ng!

webrtc crash fix, approved for 75.0b4

Comment 10

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/releases/mozilla-beta/rev/315ce380d25e
https://hg.mozilla.org/releases/mozilla-beta/rev/b2b2414ac7e7

Comment 11

[Bogdan Maris, Desktop Test Engineering]

Confirmed that 75.0b4 does not crash on Ubuntu 18.04. Will this be uplifted to 74 as well?

Comment 12

[Dan Minor [:dminor]]

Comment on attachment 9131572 [details]
Bug 1620660 - Drop frames in VideoConduit if they are cropped to zero width or height; r=ng!

Beta/Release Uplift Approval Request

• User impact if declined: This causes crashes if the user attempts to minimize a window that has been shared using WebRTC. This definitely occurs on Google Hangouts, but any service that scales a shared window might also hit this.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The main risk here is whether dropping frames at this point might cause other problems for a WebRTC service. There is nearby code that drops frames in similar circumstances, but it is difficult to say how often that code path ends up being taken. This appears to have fixed the problem on Beta and I haven't seen any new bug reports coming in that would suggest that this has caused problems, but I don't think we have the volume of usage on Beta to say this is safe for sure.
• String changes made/needed: None

Comment 13

[Bogdan Maris, Desktop Test Engineering]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #11)

Will this be uplifted to 74 as well?

Pascal, I guess this is not gonna happen but wanted to ask just to be sure.

Comment 14

[Pascal Chevrel:pascalc]

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #13)

(In reply to Bogdan Maris [:bogdan_maris], Release Desktop QA from comment #11)

Will this be uplifted to 74 as well?

Pascal, I guess this is not gonna happen but wanted to ask just to be sure.

No indeed, we are in RC week for 75 and it's fixed in 75.

Comment 15

[Pascal Chevrel:pascalc]

Comment on attachment 9131572 [details]
Bug 1620660 - Drop frames in VideoConduit if they are cropped to zero width or height; r=ng!

We didn't have a strong driver for a dot release for 74 so no need to uplift this ride-along to mozilla-release.

Comment 16

[Bogdan Maris, Desktop Test Engineering]

Thanks Pascal.