Closed Bug 1191301 Opened 6 years ago Closed 6 years ago

media.navigator.video.use_tmmbr not working

Categories

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

41 Branch
x86_64
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed
Blocking Flags:

People

(Reporter: Robert.Jongbloed, Assigned: ehugg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
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   }
OS: Unspecified → Mac OS X
Hardware: Unspecified → x86_64
Component: Untriaged → WebRTC: Audio/Video
Product: Firefox → Core
Ethan -- Can I ask you to take this bug?  I'm hoping it's trivial for you to fix since you have the background.
Status: UNCONFIRMED → NEW
backlog: --- → webRTC+
Rank: 25
Ever confirmed: true
Flags: needinfo?(ethanhugg)
Priority: -- → P2
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).
Flags: needinfo?(ethanhugg)
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.
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
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.
Attachment #8651837 - Flags: review?(docfaraday)
Attachment #8651837 - Flags: review?(docfaraday) → review+
Keywords: checkin-needed
(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?
Flags: needinfo?(ethanhugg)
https://hg.mozilla.org/mozilla-central/rev/ce1bba4455de
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Passing this needinfo request on to Hank Peng to try to figure out the issue with connecting to Spark with TMMBR enabled.
Flags: needinfo?(ethanhugg) → needinfo?(hankpeng)
Just clear the needinfo.
Flags: needinfo?(hankpeng)
You need to log in before you can comment on or make changes to this bug.