Closed Bug 1330091 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- unaffected
firefox53 --- fixed

People

(Reporter: jesup, Assigned: jesup)

References

Details

(Keywords: regression)

Attachments

(2 files)

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.
Rank: 13
Also fixes that on a renegotiation it didn't install the new config at all
Attachment #8825533 - Flags: review?(na-g)
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 && ... ?
Attachment #8825533 - Flags: review?(na-g) → review-
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.
Attachment #8825533 - Flags: review- → review+
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 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.
Attachment #8825922 - Flags: review?(na-g) → review+
"49" here means bug 1250356 (import webrtc.org stable branch 49) not Firefox 49
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.
Pushed by rjesup@wgate.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abf0cffb12f7
Reconfigure video encoders where possible instead of recreating them r=ng
https://hg.mozilla.org/mozilla-central/rev/abf0cffb12f7
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1331158
Version: 45 Branch → Trunk
You need to log in before you can comment on or make changes to this bug.