Closed Bug 1155435 Opened 5 years ago Closed 4 years ago

WebRTC - investigate enabling REMB

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed
Blocking Flags:

People

(Reporter: ehugg, Assigned: mjf)

References

(Depends on 1 open bug)

Details

Attachments

(3 files)

With the landing of bug 1150609 we will have enabled TMMBR.  Should we also enable REMB which is also available in the webrtc stack?

Enabling REMB appears to be as simple as changing the values to the SetRembStatus call when we start the webrtc stack.   I tried this on top of the TMMBR patches and I noticed that it seemed to take twice as long to ramp up to full bps on a 1-1 video call when connected to Chrome 42 which already has REMB enabled - about 50s vs 25s without REMB.  This could use more testing and I will upload a patch that turns on REMB shortly.

RFC for REMB - https://datatracker.ietf.org/doc/draft-alvestrand-rmcat-remb/
Assignee: nobody → ethanhugg
Status: NEW → ASSIGNED
backlog: --- → webRTC+
Rank: 35
Priority: -- → P3
Assignee: ethanhugg → nobody
Status: ASSIGNED → NEW
Rank: 35 → 15
Priority: P3 → P1
Assignee: nobody → mfroman
Comment on attachment 8754434 [details]
MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53974/diff/1-2/
Attachment #8754434 - Attachment description: MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=nils r=jesup → MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup
Attachment #8754435 - Attachment description: MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=nils r=jesup → MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup
Attachment #8754434 - Flags: review?(drno)
Attachment #8754435 - Flags: review?(drno)
Comment on attachment 8754435 [details]
MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53976/diff/1-2/
Attachment #8754434 - Flags: review?(drno) → review+
Comment on attachment 8754434 [details]
MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup

https://reviewboard.mozilla.org/r/53974/#review50710

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:229
(Diff revision 2)
> +      ASSERT_TRUE(track.GetNegotiatedDetails());
> +      ASSERT_EQ(track.GetNegotiatedDetails()->GetEncodingCount(), 1U);
> +      const std::vector<JsepCodecDescription*>& codecs =
> +        track.GetNegotiatedDetails()->GetEncoding(0).GetCodecs();
> +      ASSERT_EQ(codecs.size(), 1U);
> +      ASSERT_EQ(codecs[0]->mType, SdpMediaSection::kVideo);
> +      const JsepVideoCodecDescription* videoCodec =
> +        static_cast<const JsepVideoCodecDescription*>(codecs[0]);

This looks like code duplication from 215-222. Can you move that into a shared function?

::: media/webrtc/signaling/test/jsep_track_unittest.cpp:365
(Diff revision 2)
>    OfferAnswer();
>    CheckOffEncodingCount(1);
>    CheckAnsEncodingCount(1);
>  }
>  
> +TEST_F(JsepTrackTest, VideoNegotiationOfferRemb)

How about a test with Remb only turned on on the answerer side to be safe?
Comment on attachment 8754435 [details]
MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

https://reviewboard.mozilla.org/r/53976/#review50774
Attachment #8754435 - Flags: review?(drno) → review+
Comment on attachment 8754434 [details]
MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53974/diff/2-3/
Comment on attachment 8754435 [details]
MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53976/diff/2-3/
Comment on attachment 8754434 [details]
MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup

https://reviewboard.mozilla.org/r/53974/#review51112

Perhaps add use_tmmbr to all.js as well.  No need for re-review on that
Attachment #8754434 - Flags: review?(rjesup) → review+
Comment on attachment 8754435 [details]
MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

https://reviewboard.mozilla.org/r/53976/#review51116

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp:986
(Diff revision 3)
> -  mPtrRTP->SetRembStatus(mChannel, false, true);
> +  CSFLogDebug(logTag, "REMB enabled for video stream %s ",
> +              (use_remb?"yes":"no"));

spaces around ? and :, no space on end of debug string (nits)
Attachment #8754435 - Flags: review?(rjesup) → review+
Comment on attachment 8754435 [details]
MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53976/diff/3-4/
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6fa9a68064b3
https://hg.mozilla.org/mozilla-central/rev/0a63c85e816e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Blocks: 1283619
Depends on: 1297029
Hello,
good news to see that Firefox supports REMB now, great work!!

It will help to improve the QE in conferences apps a lot, but I am missing abs-send-time RTP extension header [1], which is needed to the correct operation of the algorithms used to calculate the value of the REMB packets.

Will Firefox support it?

Best!!

Refs
[1] http://www.webrtc.org/experiments/rtp-hdrext/abs-send-time
You need to log in before you can comment on or make changes to this bug.