1474711 - Add C++/Rust glue code for rtcp-fb transport-cc

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, enhancement, P2)

Description

[Johannes Willbold]

The C++/Rust glue code is missing the implementation to store and serialize the 'tranport-cc' rtcp-fb type.

Comment 1

[Johannes Willbold]

Attachment: Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc,

Added the C++/Rust glue code for the rtcp-fb transport-cc type.
Extended the C++ unit test CheckRtcpFb
Extended the Rust unit tests for rtcp-fb.

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

Comment 2

[Byron Campen [:bwc]]

Where is this standardized again?

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8991374 [details]
Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc,

https://reviewboard.mozilla.org/r/256280/#review263792

::: commit-message-aff06:3
(Diff revision 1)
> +Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc, r=bwc
> +
> +Added the C++/Rust glue code for the rtcp-fb transport-cc type.

So, this all looks reasonable, but I cannot find any specification for this, so I am not sure. Is this some proprietary thing down in webrtc.org that will be standardized soon, and that we plan to use?

Comment 4

[Johannes Willbold]

Comment on attachment 8991374 [details]
Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc,

https://reviewboard.mozilla.org/r/256280/#review263792

> So, this all looks reasonable, but I cannot find any specification for this, so I am not sure. Is this some proprietary thing down in webrtc.org that will be standardized soon, and that we plan to use?

The reason I opned this ticket was because I tested the SDP parser with different SDPs from Chrome and Safari. Both of them use this rtcp-fb type and the rust parser is parsing it already, so I assumed it is a thing we want to have.

Comment 5

[Nils Ohlmeier [:drno]]

(In reply to Byron Campen [:bwc] (PTO 7/3-7/11) from comment #2)
> Where is this standardized again?

According to Gustavos blog post http://www.rtcbits.com/2017/01/bandwidth-estimation-in-webrtc-and-new.html Chromes source code is the best reference. The blog post points to this Rmcat draft https://tools.ietf.org/html/draft-holmer-rmcat-transport-wide-cc-extensions-01#section-2.3 which ironically tries to register 'goog-remb' with IANA and only talks about to negotiate the RTP header extension, but completely ignores how to ignore the 'transport-cc' feature for RTCP feedback.

I don't think it is harmful to have this in our SDP parser, since this is used by tons of Chrome browsers out there. If we are going to use it at some point in the future is a different discussion.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8991374 [details]
Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc,

https://reviewboard.mozilla.org/r/256280/#review264002

Comment 7

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s bb1a4e7065d7b7e0d41d70c9f3c3234ed61c9661 -d f98112e49582: rebasing 472923:bb1a4e7065d7 "Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc, r=bwc" (tip)
merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs
warning: conflicts while merging media/webrtc/signaling/src/sdp/rsdparsa/src/attribute_type.rs! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 8

[Johannes Willbold]

Comment on attachment 8991374 [details]
Bug 1474711: Added C++/Rust glue code for rtcp-fb transport-cc,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256280/diff/1-2/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d4f2b0aa4cf
Added C++/Rust glue code for rtcp-fb transport-cc, r=bwc

Comment 10

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/0d4f2b0aa4cf