1429390 - Make H.264 Encode dispatch asynchronous

Status: RESOLVED FIXED

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

Description

[Dan Minor [:dminor]]

When encoding [1] H.264 video we make synchronous thread dispatches. The historical reasons for doing so no longer seem to apply, as the frame buffer is now kept in a refptr.

I've caught a deadlock on shutdown which seems to be caused by this, where the main thread is waiting on the EncoderQueue thread which is waiting on a sync dispatch to the GMPThread which is waiting on a sync dispatch to the main thread. This seems to happen when we are asked to encode a frame during shutdown.

There is an existing moderate frequency intermittent (Bug 1375540) that has similar symptoms, but there is not enough information in the logs to say for sure whether it is the same problem.

Decoding H.264 also makes a synchronous dispatch [2] but the frame buffer there is kept as a raw pointer, so we would still be stuck copying data if we were to make it asynchronous.

[1] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp#319
[2] https://searchfox.org/mozilla-central/rev/cf149b7b63ff97023e28723167725e38cf5df757/media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp#809

Comment 1

[Dan Minor [:dminor]]

Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1be8c70c96d9fc42ae14cb47df6975ba7dd2ccac

Comment 2

[Dan Minor [:dminor]]

Attachment: Call encode asynchronously

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8942319 [details] [diff] [review]
Call encode asynchronously

Review of attachment 8942319 [details] [diff] [review]:
-----------------------------------------------------------------

Provisional r+ if copying aInputImage is safe; please verify that and document

::: media/webrtc/signaling/src/media-conduit/WebrtcGmpVideoCodec.cpp
@@ +322,5 @@
> +
> +  mGMPThread->Dispatch(WrapRunnableNM(&WebrtcGmpVideoEncoder::Encode_g,
> +                                      RefPtr<WebrtcGmpVideoEncoder>(this),
> +                                      aInputImage,
> +                                      *aFrameTypes),

See comment below; need to note that both aInputImage and aFrameTypes are being copied here (and copying the vector will allocate)

@@ +359,5 @@
>  }
>  
> +void
> +WebrtcGmpVideoEncoder::Encode_g(RefPtr<WebrtcGmpVideoEncoder>& aEncoder,
> +                                webrtc::VideoFrame aInputImage,

You're copying aInputImage; I presume that has a well-defined safe lifetime for this async encode for the Image and everything in it?  Please document this in the comments (probably at the Dispatch, and a reference here back to that).

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3b45da65d7
Make H.264 Encode dispatch asynchronous; r=jesup

Comment 5

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/4b3b45da65d7