WebRTC Move TMMBR use behind pref.

RESOLVED FIXED in Firefox 40

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ehugg, Assigned: ehugg)

Tracking

40 Branch
mozilla41
Points:
---

Firefox Tracking Flags

(firefox38 unaffected, firefox38.0.5 unaffected, firefox39 unaffected, firefox40 fixed, firefox41 fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Details

Attachments

(2 attachments)

Bug 1150609 landed in FF40 and enabled TMMBR, but that seems to cause some compatibility issues with some non-FF endpoints.  Instead of backing out that patch I'm proposing to put it behind the pref "media.navigator.video.use_tmmbr" which will default to false.
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
Comment on attachment 8604705 [details] [diff] [review]
WebRTC - move TMMBR behind pref


User would have to create media.navigator.video.use_tmmbr in about:config in a similar fashion to how we do preferred_codec.

An alternative would be to backout all three patches of bug 1150609
Attachment #8604705 - Flags: review?(rjesup)
Comment on attachment 8604728 [details] [diff] [review]
Backout signaling unittest changes for tmmbr.


Signaling unittest changes from bug 1150609 will need to be backed out since it can no longer expect tmmbr.  Patch made with hg backout.
Attachment #8604728 - Flags: review?(rjesup)
Comment on attachment 8604705 [details] [diff] [review]
WebRTC - move TMMBR behind pref

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

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ +324,5 @@
>          mDefaultPt, SdpRtcpFbAttributeList::kNack, SdpRtcpFbAttributeList::pli);
>      rtcpfb.PushEntry(
>          mDefaultPt, SdpRtcpFbAttributeList::kCcm, SdpRtcpFbAttributeList::fir);
> +    if (mUseTmmbr) {
> +      rtcpfb.PushEntry(

What happens if we're answering and the other side offers?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +967,5 @@
>  
>            }
> +
> +          videoCodec.mUseTmmbr = false;
> +          branch->GetBoolPref("media.navigator.video.use_tmmbr",

If this isn't meant to be a 'hidden' pref, modify all.js as well.  A hidden pref may be fine here.
Attachment #8604728 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #5)
> Comment on attachment 8604705 [details] [diff] [review]
> WebRTC - move TMMBR behind pref
> 
> Review of attachment 8604705 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
> @@ +324,5 @@
> >          mDefaultPt, SdpRtcpFbAttributeList::kNack, SdpRtcpFbAttributeList::pli);
> >      rtcpfb.PushEntry(
> >          mDefaultPt, SdpRtcpFbAttributeList::kCcm, SdpRtcpFbAttributeList::fir);
> > +    if (mUseTmmbr) {
> > +      rtcpfb.PushEntry(
> 
> What happens if we're answering and the other side offers?

Just checked again with talky.io.  If the use_tmmbr pref is false and the other side offers tmmbr we will remove it in our answer and not turn on tmmbr.  I assume that's what we want in this case.

> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +967,5 @@
> >  
> >            }
> > +
> > +          videoCodec.mUseTmmbr = false;
> > +          branch->GetBoolPref("media.navigator.video.use_tmmbr",
> 
> If this isn't meant to be a 'hidden' pref, modify all.js as well.  A hidden
> pref may be fine here.

I modeled this after preferred_codec which is a hidden pref.  Let me know if you don't want it hidden and I can add it to all.js.
(In reply to Ethan Hugg [:ehugg] from comment #6)
> (In reply to Randell Jesup [:jesup] from comment #5)
> > Comment on attachment 8604705 [details] [diff] [review]
> > WebRTC - move TMMBR behind pref
> > 
> > Review of attachment 8604705 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
> > @@ +324,5 @@
> > >          mDefaultPt, SdpRtcpFbAttributeList::kNack, SdpRtcpFbAttributeList::pli);
> > >      rtcpfb.PushEntry(
> > >          mDefaultPt, SdpRtcpFbAttributeList::kCcm, SdpRtcpFbAttributeList::fir);
> > > +    if (mUseTmmbr) {
> > > +      rtcpfb.PushEntry(
> > 
> > What happens if we're answering and the other side offers?
> 
> Just checked again with talky.io.  If the use_tmmbr pref is false and the
> other side offers tmmbr we will remove it in our answer and not turn on
> tmmbr.  I assume that's what we want in this case.
> 
There is a problem when tmmbr is on on though where we answer with tmmbr to an offer that doesn't offer it.  I'll need to finish bug 1097169 and add proper negotiation for it to fix it.  For now it's one more reason to pref it off.
Comment on attachment 8604705 [details] [diff] [review]
WebRTC - move TMMBR behind pref

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

r+ - we don't need to add it toa ll.js, as it should be short-lived and not widely played with
Attachment #8604705 - Flags: review?(rjesup) → review+
Marking for checkin-neeeded for both patches together.  Try run is listed in comment #8.
Keywords: checkin-needed
This patches landed on MC but didn't get marked as such
https://hg.mozilla.org/mozilla-central/rev/18b0d062f5dd
https://hg.mozilla.org/mozilla-central/rev/beb2c4d01135
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Comment on attachment 8604705 [details] [diff] [review]
WebRTC - move TMMBR behind pref

Approval Request Comment
[Feature/regressing bug #]: 
In bug 1150609 I turned on tmmbr for WebRTC streams.  It landed in FF40.  It does not work correctly with some endpoints, but instead of backing it out we are putting it behind a pref to make it easier to do further diagnosis.
[User impact if declined]:  
If declined, there would be situations where FF33-39 and FF41 work fine, but FF40 would have the video stopping after 2-3 minutes.
[Describe test coverage new/current, TreeHerder]:  
Just retested after it landed on M-C.  We are reverting to previous behavior by default, only get tmmbr now if hidden pref is enabled.
[Risks and why]: 
Risk is low since we are reverting to previous behavior.
[String/UUID change made/needed]: 
none
Attachment #8604705 - Flags: approval-mozilla-aurora?
Comment on attachment 8604728 [details] [diff] [review]
Backout signaling unittest changes for tmmbr.

Approval Request Comment

Same as above only this one backs out the unittest changes.

[Feature/regressing bug #]: 
In bug 1150609 I turned on tmmbr for WebRTC streams.  It landed in FF40.  It does not work correctly with some endpoints, but instead of backing it out we are putting it behind a pref to make it easier to do further diagnosis.
[User impact if declined]:  
If declined, there would be situations where FF33-39 and FF41 work fine, but FF40 would have the video stopping after 2-3 minutes.
[Describe test coverage new/current, TreeHerder]:  
Just retested after it landed on M-C.  We are reverting to previous behavior by default, only get tmmbr now if hidden pref is enabled.
[Risks and why]: 
Risk is low since we are reverting to previous behavior.
[String/UUID change made/needed]: 
none
Attachment #8604728 - Flags: approval-mozilla-aurora?
Attachment #8604705 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8604728 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.