1155435 - WebRTC - investigate enabling REMB

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

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/

Comment 1

[Ethan Hugg [:ehugg]]

Attachment: Turn on REMB - Not For Check-In

Comment 2

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1155435 - add sdp RtcpFb for REMB, r=drno, r=jesup

Review commit: https://reviewboard.mozilla.org/r/53974/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53974/

Comment 3

[Michael Froman [:mjf]]

Attachment: MozReview Request: Bug 1155435 - connected negotiated REMB from sdp into VideoConduit, r=drno, r=jesup

Review commit: https://reviewboard.mozilla.org/r/53976/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53976/

Comment 4

[Michael Froman [:mjf]]

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/

Comment 5

[Michael Froman [:mjf]]

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/

Comment 6

[Nils Ohlmeier [:drno]]

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 7

[Nils Ohlmeier [:drno]]

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

Comment 8

[Michael Froman [:mjf]]

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 9

[Michael Froman [:mjf]]

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 10

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

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

Comment 11

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

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)

Comment 12

[Michael Froman [:mjf]]

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/

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa9a68064b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a63c85e816e

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fa9a68064b3
https://hg.mozilla.org/mozilla-central/rev/0a63c85e816e

Comment 15

[mparisdiaz]

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