Make H.264 Encode dispatch asynchronous

RESOLVED FIXED in Firefox 60

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: dminor, Assigned: dminor)

Tracking

58 Branch
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
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
Assignee

Comment 2

Last year
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+

Comment 4

Last year
Pushed by dminor@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3b45da65d7
Make H.264 Encode dispatch asynchronous; r=jesup

Comment 5

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b3b45da65d7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
See Also: → 1375540
You need to log in before you can comment on or make changes to this bug.