The webrtc.org code for encoding via video_sender.cc/AddFrame and generic_encoder assumes that the Encoded() callback will happen from *within* the Encode() call (i.e. with the video_sender's _sendCritSect locked). This is invalid for any codec that calls Encoded() asynchronously, such as GMP codecs. This causes on windows (if IPC issues are bypassed) crashes (rarely; minutes or many minutes into a call) in media_optimization from within Encoded().

This works (4 hours in Linux and Win32 no crashes) but I prefer to find a solution that avoids the Sync dispatch, and so avoids adding threads

Note: the dangerous deadlock here leading to the extra thread is that the caller to Encode() holds the _sendCritSect, and if GMPThread is in Encoded() and tries to grab _sendCritSect it will stall, and thus never service the sender's Encode() runnable. The obvious solution to avoiding an extra thread (not having the Encode() call block on GMPThread) means we need to transfer the frame-to-be-encoded to a shared-mem-backed-i420 frame, and send that to GMPThread asynchronously. This fails since we can't allocate Shmems for use with GMP except on, you guessed it, GMPThread! We can't even ask GMPThread for some, since it may not be servicing requests. Grrr. Perhaps more work in the caller could relax the restriction somehow. I don't seem to be able to drop the critsect before calling Encode() safely (tried that)

https://hg.mozilla.org/integration/mozilla-inbound/rev/a4a3810931af https://hg.mozilla.org/integration/mozilla-inbound/rev/27ab005a03f7

Unified build bustage fix rs=bustage,kwierso https://hg.mozilla.org/integration/mozilla-inbound/rev/09425d87715d

https://hg.mozilla.org/mozilla-central/rev/a4a3810931af https://hg.mozilla.org/mozilla-central/rev/27ab005a03f7 https://hg.mozilla.org/mozilla-central/rev/09425d87715d

Comment on attachment 8466234 [details] [diff] [review] reacquire _sendCritSect within webrtc Encoded() callback (upstream patch) For both patches: Approval Request Comment [Feature/regressing bug #]: openh264/GMP [User impact if declined]: random crashes (especially on windows) and UAFs [Describe test coverage new/current, TBPL]: Thread-order issues can't generally be tested in tbpl, and this may require long calls to hit (5-20 minutes). On nightly for some time now; tested in multiple 6-hour-plus calls on Windows. [Risks and why]: moderately low risk; we're modifying locking code, but the locking required is fairly clear. The additional thread is annoying, but needed to avoid deadlocks. [String/UUID change made/needed]: none.

https://hg.mozilla.org/releases/mozilla-aurora/rev/e4abfe41b9d5 https://hg.mozilla.org/releases/mozilla-aurora/rev/149710661ef8

Comment on attachment 8466234 [details] [diff] [review] reacquire _sendCritSect within webrtc Encoded() callback (upstream patch) Review of attachment 8466234 [details] [diff] [review]: ----------------------------------------------------------------- While we haven't identified a guaranteed regression yet, 2.0 OMX encoders appear to have the same problem (since we call Encoded() on another thread).

Randell, I was trying to cherry pick these patches to the FFOS v2.0 branch to see if they fix bug 1052169. They don't apply cleanly. Can I get your help by rebasing them? I can try it myself but maybe it'll be trivial for you.

(In reply to Diego Wilson [:diego] from comment #10) > Randell, > > I was trying to cherry pick these patches to the FFOS v2.0 branch to see if > they fix bug 1052169. They don't apply cleanly. Can I get your help by > rebasing them? > > I can try it myself but maybe it'll be trivial for you. spoke offline with :jesup, diego was able to apply the patch cleanly, we're now waiting for results to confirm this fixes report in 1052169