Closed Bug 1429390 Opened 3 years ago Closed 3 years ago

Make H.264 Encode dispatch asynchronous


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

58 Branch



Tracking Status
firefox60 --- fixed


(Reporter: dminor, Assigned: dminor)




(1 file)

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.

Attachment #8942319 - Flags: review?(rjesup)
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).
Attachment #8942319 - Flags: review?(rjesup) → review+
Pushed by
Make H.264 Encode dispatch asynchronous; r=jesup
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
See Also: → 1375540
You need to log in before you can comment on or make changes to this bug.