1191301 - media.navigator.video.use_tmmbr not working

Status: RESOLVED FIXED

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

Description

[Robert Jongbloed]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/44.0.2403.89 Safari/537.36

Steps to reproduce:

Set media.navigator.video.use_tmmbr to true in about:config

Made a WebRTC call into Firefox Developer Edition using our server, so SDP offer comes from our end, and answer is from Mozilla.

Our SDP contains:

a=rtcp-fb:* ccm tmmbr

for each video media description.



Actual results:

No SDP for tmmbr in reply SDP.


Expected results:

SDP should contain

a=rtcp-fb:103 ccm tmmbr

Comment 1

[Robert Jongbloed]

Addition information. Using http://mxr.mozilla.org/ I found one obvious mistake. It is clear in the below that the mUseTmmbr flag has not been set yet when it is used to add SdpRtcpFbAttributeList::tmmbr. Not much between where mUseTmmbr is initialised to false and it is used in the same function. So it never, ever, added.

Inerestingly the location of this code looks more correct in the "beta" code. This is from "Aurora".


232   JsepVideoCodecDescription(const std::string& defaultPt,
233                             const std::string& name,
234                             uint32_t clock,
235                             bool enabled = true)
236       : JsepCodecDescription(mozilla::SdpMediaSection::kVideo, defaultPt, name,
237                              clock, 0, enabled),
238         mMaxFs(0),
239         mMaxFr(0),
240         mPacketizationMode(0),
241         mMaxMbps(0),
242         mMaxCpb(0),
243         mMaxDpb(0),
244         mMaxBr(0),
245         mUseTmmbr(false)
246   {
247     // Add supported rtcp-fb types
248     mNackFbTypes.push_back("");
249     mNackFbTypes.push_back(SdpRtcpFbAttributeList::pli);
250     mCcmFbTypes.push_back(SdpRtcpFbAttributeList::fir);
251     if (mUseTmmbr) {
252       mCcmFbTypes.push_back(SdpRtcpFbAttributeList::tmmbr);
253     }
254   }

Comment 2

[Maire Reavy [:mreavy]]

Ethan -- Can I ask you to take this bug?  I'm hoping it's trivial for you to fix since you have the background.

Comment 3

[Ethan Hugg [:ehugg]]

It looks like the behavior was changed in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=1165520
patch here:
https://reviewboard.mozilla.org/r/8877/

I can take a look at it when I get back from vacation probably.

It was not caught by the unittest because the tmmbr unittests were removed when we put tmmber behind a pref. 

Also, the two reasons for putting tmmbr behind a pref were that we did not negotiate it properly in one case (we answer an offer that has tmmbr off with tmmbr turned on) and it reacted badly with the Spark native client (continually lowering the bandwith until video stopped).

Comment 4

[Robert Jongbloed]

Just to let you know, provided we can set media.navigator.video.use_tmmbr from within the Javascript, we are quite happy for it to remain behind that variable. It just needs to actually use it.

Comment 5

[Ethan Hugg [:ehugg]]

Attachment: Re-enable the use of media.navigator.video.use_tmmbr pref

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 8651837 [details] [diff] [review]
Re-enable the use of media.navigator.video.use_tmmbr pref


The media.navigator.video.use_tmmbr flag was disabled by this patch:
https://reviewboard.mozilla.org/r/8877/

Here's one way to make it work again.

Comment 7

[Ethan Hugg [:ehugg]]

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e135ae630728

Comment 8

[Pulsebot]

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

Comment 9

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

(In reply to Robert Jongbloed from comment #4)
> Just to let you know, provided we can set media.navigator.video.use_tmmbr
> from within the Javascript, we are quite happy for it to remain behind that
> variable. It just needs to actually use it.

Note: about:config prefs can *not* be used by web pages; they have to be set by the user.

Before we can re-enable TMMBR by default, we need to figure out why it caused problems with Spark (spark bug, or ours?)

Ethan?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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

Comment 11

[Ethan Hugg [:ehugg]]

Passing this needinfo request on to Hank Peng to try to figure out the issue with connecting to Spark with TMMBR enabled.

Comment 12

[Hank Peng]

Just clear the needinfo.