Closed Bug 1620660 Opened 1 year ago Closed 1 year ago

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

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla76
Tracking Status
firefox-esr68 --- wontfix
firefox73 --- wontfix
firefox74 --- wontfix
firefox75 --- verified
firefox76 --- verified

People

(Reporter: dminor, Assigned: dminor)

References

Details

(Keywords: crash)

Attachments

(2 files)

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.

See Also: → 1521169

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

Pushed by dminor@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla76

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

Flags: needinfo?(dminor)
Keywords: crash

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
Flags: needinfo?(dminor)
Attachment #9131572 - Flags: approval-mozilla-beta?
Attachment #9131571 - Flags: approval-mozilla-beta?
Flags: qe-verify+

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.

QA Whiteboard: [qa-triaged]

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

webrtc crash fix, approved for 75.0b4

Attachment #9131571 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9131572 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

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
Attachment #9131572 - Flags: approval-mozilla-release?
Attachment #9131571 - Flags: approval-mozilla-release?

(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.

Flags: needinfo?(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.

Flags: needinfo?(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.

Attachment #9131572 - Flags: approval-mozilla-release? → approval-mozilla-release-
Attachment #9131571 - Flags: approval-mozilla-release? → approval-mozilla-release-

Thanks Pascal.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
Regressions: 1628851
You need to log in before you can comment on or make changes to this bug.