1099318 - VideoConduit stuff doesn't work if the transmit conduit is created before the receive conduit

Status: RESOLVED FIXED

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

Description

[Byron Campen [:bwc]]

While fixing up the patches for bug 1091242, I noticed that we were not linking the transmit/receive conduits for video like we should have been. However, when this was fixed, video stopped working, because the receive conduit was now using the transmit conduit's VideoEngine, which was apparently not created properly. Switching the order of creation such that the receive conduit came first, and constructed the shared VideoEngine, got things working again.

This is going to be a blocker for renegotiation, I think, because we cannot control the order in which these things are created (ie; renegotiation takes us from sendonly to sendrecv).

Comment 1

[Byron Campen [:bwc]]

I should point out that fixing bug 1108248 might fix this one too.

Comment 2

[Paul Kerr [:pkerr]]

The problem appears to be in VideoConduit.cpp WebVideoConduit::Init. While most of the code deals with creating the VideoEngine, attaching the peer conduits to each other, and looking up interfaces there are two calls which make the assumption as to the direction of the conduit and pass 'this' as a parameter. The one that breaks the system when the send conduit is created first is the call to mPtrViERender->AddRenderer(). Here 'this' is passed cast as an ExternalRenderer whenever the first conduit is created. But, if the first conduit is a sender, this will prevent anything from being rendered.

I tested this by calling AddRenderer when the second conduit is created: the other argument is non-null, and the send conduit is created first followed by the receive conduit (the current failure case). This temporary change fixed the problem.

The real fix is to move the AddRenderer() and RegisterSendTransport() outside of this method, which has no knowledge of the conduit directions, to a level where each conduit is created.

(The call to RegisterSendTransport() is, I believe, wrong in the current code arrangement but does not have such a visible side effect.)

Comment 3

[Paul Kerr [:pkerr]]

Attachment: WIP preliminary fix to conduit creation order issue

WIP: Fixed by making Renderer and SendTransport assigments based on conduit direction. This should be tighened up to move directional knowledge out of the WebrtcVideoConduit class.

Comment 4

[Paul Kerr [:pkerr]]

Attachment: WIP preliminary fix to conduit creation order issue

fix broken mochitests

Comment 5

[Paul Kerr [:pkerr]]

Attachment: Fix for conduit receive then send creation order issue. Now insensitive to order."

Comment 6

[Paul Kerr [:pkerr]]

Attachment: Fix for conduit receive then send creation order issue. Now insensitive to order."

Comment 7

[Paul Kerr [:pkerr]]

Attachment: Fix for conduit receive then send creation order issue. Now insensitive to order."

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8556528 [details] [diff] [review]
Fix for conduit receive then send creation order issue. Now insensitive to order."

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

This all looks good to me, although you probably want to run it by jesup too.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +428,5 @@
>                    mPtrViEBase->LastError());
>        return kMediaConduitChannelError;
>      }
>  
> +    if (mPtrViENetwork->RegisterSendTransport(mChannel, *this) == -1)

It looks like the convention here is to have no space between "if" and "(", so let's just leave this be.

Comment 9

[Paul Kerr [:pkerr]]

Attachment: Fix for conduit receive then send creation order issue. Now insensitive to order

Incorporate review feedback.

Comment 10

[Paul Kerr [:pkerr]]

Comment on attachment 8556624 [details] [diff] [review]
Fix for conduit receive then send creation order issue. Now insensitive to order

Carry forward bwc r+

Comment 11

[Paul Kerr [:pkerr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/afeaaccc6c10

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/afeaaccc6c10