1330091 - Renegotiation doesn't actually change the codec configuration after 49 update landing

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

The rewrite of ConfigureSendMediaCodec() ends up not actually installing the new codec/settings it calculates, since StartTransmitting() checks mSendStream, and if it's set doesn't instantiate a new stream (and encoder).  In fact, we want to ReconfigureVideoCodec() where at all possible.

Comment 1

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

Attachment: Reconfigure video encoders where possible instead of recreating them

Also fixes that on a renegotiation it didn't install the new config at all

Comment 2

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8825533 [details] [diff] [review]
Reconfigure video encoders where possible instead of recreating them

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

One potential issue.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +466,5 @@
>    if ((condError = ValidateCodecConfig(codecConfig, true)) != kMediaConduitNoError) {
>      return condError;
>    }
>  
> +  if (!mSendStream ||

Should this be if(mSendStream && ... ?

Comment 3

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8825533 [details] [diff] [review]
Reconfigure video encoders where possible instead of recreating them

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

Looks good.

Comment 4

[Andreas Pehrson [:pehrsons]]

Can you add a mochitest that negotiates with a non-default codec and verifies that content is making it across?

Probably one explicit test per supported codec.

And then one test that renegotiates and changes codecs to test this bug.

It's a bit of work but since they're tests they'll save us work in the long run! :-)

Comment 5

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

Attachment: interdiff for h264 compatibility checking, plus a bugfix

Comment 6

[Nico Grunbaum [:ng, @chew:mozilla.org]]

Comment on attachment 8825922 [details] [diff] [review]
interdiff for h264 compatibility checking, plus a bugfix

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

r+ on the interdiff, with a question. Does the h264 level also need to be checked?
Also, +1 to adding mochitest coverage.

Comment 7

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

"49" here means bug 1250356 (import webrtc.org stable branch 49) not Firefox 49

Comment 8

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

h.264 level is negotiated, and can change in an answer for example.  Changing it would require a new SPS/PPS to be sent, much like a resolution change.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf0cffb12f7
Reconfigure video encoders where possible instead of recreating them r=ng

Comment 10

[Carsten Book [:Tomcat]]

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