1165520 - JsepSessionImpl adds rtcp-fb to answer that was not present in the offer

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect, P3)

Description

[Byron Campen [:bwc]]

From RFC 4585 4.2

   When used in conjunction with the offer/answer model [8  <http://tools.ietf.org/html/rfc4585#ref-8>], the offerer
   MAY present a set of these AVPF attributes to its peer.  The answerer
   MUST remove all attributes it does not understand as well as those it
   does not support in general or does not wish to use in this
   particular media session.  The answerer MUST NOT add feedback
   parameters to the media description and MUST NOT alter values of such
   parameters.  The answer is binding for the media session, and both
   offerer and answerer MUST only use feedback mechanisms negotiated in
   this way.  Both offerer and answerer MAY independently decide to send
   RTCP FB messages of only a subset of the negotiated feedback
   mechanisms, but they SHOULD react properly to all types of the
   negotiated FB messages when received.

Comment 1

[Byron Campen [:bwc]]

Attachment: MozReview Request: bz://1165520/bwc

/r/8877 - Bug 1165520: Negotiate rtcp-fb

Pull down this commit:

hg pull -r aaadd0b49e88c5d069f6efe71d222d85dfd2bd0a https://reviewboard-hg.mozilla.org/gecko/

Comment 2

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

Comment on attachment 8606562 [details]
MozReview Request: bz://1165520/bwc

https://reviewboard.mozilla.org/r/8875/#review7571

Just nits about comments, and fix the compile failure

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:285
(Diff revision 1)
> +    mCcmFbTypes.push_back(SdpRtcpFbAttributeList::tmmbr);

I'd generally love to see more comments in all this code; comments are your friend and help others trying to understand the code.  (I strongly reject the line of thought that comments are 'bad' because they can be wrong or bitrot.)

At least as we make mods, let's add relevant comments about what we're doing and more importantly why (and pointers to relevant specs).
(and I realize some of the other code has more of these).

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h:417
(Diff revision 1)
> +    NegotiateRtcpFb(remote, SdpRtcpFbAttributeList::kCcm, mCcmFbTypes);

Again, some comments for this block (and it fails to compile)

,

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8606562 [details]
MozReview Request: bz://1165520/bwc

/r/8877 - Bug 1165520: Negotiate rtcp-fb

Pull down this commit:

hg pull -r 60a89c763824285c2349275e8434dc606fc4fb56 https://reviewboard-hg.mozilla.org/gecko/

Comment 4

[Byron Campen [:bwc]]

https://reviewboard.mozilla.org/r/8875/#review7603

> I'd generally love to see more comments in all this code; comments are your friend and help others trying to understand the code.  (I strongly reject the line of thought that comments are 'bad' because they can be wrong or bitrot.)
> 
> At least as we make mods, let's add relevant comments about what we're doing and more importantly why (and pointers to relevant specs).
> (and I realize some of the other code has more of these).

Comments are good, understandable code is better. This code is pretty hard to understand, unfortunately. I have a refactoring in mind that will help a lot both here and in JsepSessionImpl, but in the meantime I can add a couple of comments.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8606562 [details]
MozReview Request: bz://1165520/bwc

/r/8877 - Bug 1165520: Negotiate rtcp-fb

Pull down this commit:

hg pull -r 2ad363176a6b6a2828c77e1d113705e4f69d3d08 https://reviewboard-hg.mozilla.org/gecko/

Comment 6

[Byron Campen [:bwc]]

Check back on infra retriggers.

Comment 7

[Byron Campen [:bwc]]

Scratch that, this rotted.

Comment 8

[Byron Campen [:bwc]]

Comment on attachment 8606562 [details]
MozReview Request: bz://1165520/bwc

/r/8877 - Bug 1165520: Negotiate rtcp-fb r=jesup

Pull down this commit:

hg pull -r 893b21e232906370dd177731b4aa5ed44b84c594 https://reviewboard-hg.mozilla.org/gecko/

Comment 9

[Byron Campen [:bwc]]

Check back on try for unrot.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ec28ae5f5f93

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ec28ae5f5f93

Comment 13

[Byron Campen [:bwc]]

Attachment: MozReview Request: Bug 1165520: Negotiate rtcp-fb r=jesup