880067 - Generate rtcp-fb in SDP (appears to break interop)

Status: RESOLVED FIXED

(Core :: WebRTC: Signaling, defect)

Description

[Eric Rescorla (:ekr)]

Justin Uberti points out that Firefox doesn't generate rtcp-fb
and thus Chrome doesn't send NACK and FIR....

This can cause freezing

Comment 1

[Adam Roach [:abr]]

Protocol ref: http://tools.ietf.org/html/rfc4585

Comment 2

[Adam Roach [:abr]]

These properties are set up in VideoConduit::Init()

Comment 3

[Adam Roach [:abr]]

The current chrome outputs:

"a=rtcp-fb:100 ccm fir"
"a=rtcp-fb:100 nack "

Comment 4

[Adam Roach [:abr]]

Attachment: Part 1: SDP rtcp-fb parsing/serializing

Comment 5

[Adam Roach [:abr]]

Comment on attachment 765039 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

This is the first part of a patch to add correct a=rtcp-fb handling. It implements the SDP parsing and serializing, and has the start of a unit test for that functionality (it still needs more coverage, which I plan to add in a subsequent patch to this bug).

I will be adding a second patch to this bug, later, that plumbs this through to the codec configuration and performs a proper SDP negotiation. I hope to land this current patch as soon as possible, though, as it's hindering video interop with Chrome.

EKR: I'm adding you on for feedback, as I suspect you may be interested in the unit tests. Don't worry about coverage right now, but if you find some egregious style violations, feel free to leave comments.

If things go well, I hope to ask for uplift to Aurora for this patch before it starts to freeze on Monday.

Comment 6

[Ethan Hugg [:ehugg]]

Comment on attachment 765039 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

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

r+ with nits addressed. Also while the sdp_unittests works for me the SignalingTest.CheckTrickleSdpChange consistently fails with this patch.  That should be looked into, it succeeeds  for me with the latest M-C but fails with this patch.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp.h
@@ +876,5 @@
> + * a=rtcp-fb:<payload-type> <feedback-type> [<feedback-parameters>]
> + * Defines RTCP feedback parameters
> + */
> +typedef struct sdp_fmtp_fb {
> +    u16                          payload_num;    /* 65535 = "all payloads" */

I think we should define an ALL_PAYLOADS here and replace the 0xFFFF that is used throughout this patch.  Just in case someone wants to change the size of this.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +4880,5 @@
> +        rtcp_fb_p->payload_num = (u16)sdp_getnextnumtok(ptr, &ptr,
> +                                                        " \t", &result);
> +        if (result != SDP_SUCCESS) {
> +            sdp_attr_fmtp_no_value(sdp_p, "payload type");
> +            return SDP_INVALID_PARAMETER;

I'm guessing there should be a call to sdp_parse_error right before this to report it.  It doesn't stop processing but puts it on the list of errors up to JS.  I know the existing code is not entirely consistent on this but I assume we'll want this reported.

@@ +4888,5 @@
> +    /* Read feedback type */
> +    i = find_token_enum("rtcp-fb attribute", sdp_p, &ptr, sdp_rtcp_fb_type_val,
> +                        SDP_MAX_RTCP_FB, SDP_RTCP_FB_UNKNOWN);
> +    if (i < 0) {
> +        return SDP_INVALID_PARAMETER;

sdp_parse_error?

@@ +4898,5 @@
> +            i = find_token_enum("rtcp-fb ack type", sdp_p, &ptr,
> +                                sdp_rtcp_fb_ack_type_val,
> +                                SDP_MAX_RTCP_FB_ACK, SDP_RTCP_FB_ACK_UNKNOWN);
> +            if (i < 0) {
> +                return SDP_INVALID_PARAMETER;

sdp_parse_error?

@@ +4908,5 @@
> +            i = find_token_enum("rtcp-fb ccm type", sdp_p, &ptr,
> +                                sdp_rtcp_fb_ccm_type_val,
> +                                SDP_MAX_RTCP_FB_CCM, SDP_RTCP_FB_CCM_UNKNOWN);
> +            if (i < 0) {
> +                return SDP_INVALID_PARAMETER;

sdp_parse_error?

@@ +4915,5 @@
> +
> +            /* TODO -- We don't currently parse tmmbr parameters or vbcm
> +               submessage types. If we decide to support these modes of
> +               operation, we probably want to add parsing code for them.
> +               For the time being, they'll just ens up parsed into "extra". */

'end up' I assume.

@@ +4933,5 @@
> +            i = find_token_enum("rtcp-fb nack type", sdp_p, &ptr,
> +                                sdp_rtcp_fb_nack_type_val,
> +                                SDP_MAX_RTCP_FB_NACK, SDP_RTCP_FB_NACK_UNKNOWN);
> +            if (i < 0) {
> +                return SDP_INVALID_PARAMETER;

sdp_parse_error?

@@ +4943,5 @@
> +            rtcp_fb_p->param.trr_int = sdp_getnextnumtok(ptr, &ptr,
> +                                                         " \t", &result);
> +            if (result != SDP_SUCCESS) {
> +                sdp_attr_fmtp_no_value(sdp_p, "trr-int");
> +                return SDP_INVALID_PARAMETER;

sdp_parse_error?

@@ +4953,5 @@
> +            break;
> +
> +        default:
> +            CSFLogError(logTag, "%s Error: Invalid rtcp-fb enum (%d)",
> +                        sdp_p->debug_str, attr_p->attr.rtcp_fb.feedback_type);

sdp_parse_error?

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +12000,5 @@
> +    }
> +
> +    attr_p = sdp_find_rtcp_fb_attr(sdp_p, level, payload_type,
> +                                   SDP_RTCP_FB_TRR_INT, inst);
> +    if (attr_p == NULL) {

in sdp_attr_set_rtcp_fb_ack you have if (!attr_p) which I assume would be preferable as Moz style,  I don't see this existing code to be conistent enough to copy the == NULL style.

@@ +12033,5 @@
> +    }
> +
> +    attr_p = sdp_find_rtcp_fb_attr(sdp_p, level, payload_type,
> +                                   SDP_RTCP_FB_CCM, inst);
> +    if (attr_p == NULL) {

if (!attr_p) ?

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 765039 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

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

I just went through the unit tests. They look like they could use some cleanup.
r+ with fixes.

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +13,5 @@
> +#include "gtest_utils.h"
> +
> +#include "nspr.h"
> +#include "nss.h"
> +#include "ssl.h"

Are any of these needed?

@@ +27,5 @@
> +#include "nsNetUtil.h"
> +#include "nsIIOService.h"
> +#include "nsIDNSService.h"
> +#include "nsWeakReference.h"
> +#include "nricectx.h"

Does this do anything. Might be useful to audit this list of includes.

@@ +52,5 @@
> +
> +    gThread = thread;
> +    sipcc::PeerConnectionCtx::InitializeGlobal(gThread);
> +  }
> +  return true;

Is gThread used at all? I only see it being zeroed later.

@@ +59,5 @@
> +class SdpEnvironment : public ::testing::Environment {
> + public:
> +  void TearDown() {
> +  }
> +};

This also appears not to do much.

@@ +82,5 @@
> +      sdp_nettype_supported(config_p, SDP_NT_INTERNET, true);
> +      sdp_addrtype_supported(config_p, SDP_AT_IP4, true);
> +      sdp_addrtype_supported(config_p, SDP_AT_IP6, true);
> +      sdp_transport_supported(config_p, SDP_TRANSPORT_RTPAVP, true);
> +      sdp_transport_supported(config_p, SDP_TRANSPORT_UDPTL, true);

If ever the world was begging for a table....

@@ +106,5 @@
> +      }
> +      sdp_ptr = sdp_init_description("BogusPeerConnectionId", config_p);
> +    }
> +
> +    void ParseSdp(std::string sdp_str) {

const std::string&

@@ +144,5 @@
> +      EXPECT_EQ(sdp_set_conn_nettype(sdp_ptr, final_level, SDP_NT_INTERNET),
> +                SDP_SUCCESS);
> +      EXPECT_EQ(sdp_set_conn_addrtype(sdp_ptr, final_level, SDP_AT_IP4),
> +                SDP_SUCCESS);
> +      EXPECT_EQ(sdp_set_conn_address(sdp_ptr, final_level, "172.16.32.14"),

Please use a 5737 address here.

@@ +149,5 @@
> +                SDP_SUCCESS);
> +      EXPECT_EQ(sdp_set_media_type(sdp_ptr, final_level, SDP_MEDIA_VIDEO),
> +                SDP_SUCCESS);
> +      EXPECT_EQ(sdp_set_media_transport(sdp_ptr, final_level,
> +                                        SDP_TRANSPORT_RTPAVP),

SAVP?

@@ +164,5 @@
> +                         u32 payload = 0xFFFF) {
> +      u16 inst_num = 0;
> +      EXPECT_EQ(sdp_add_new_attr(sdp_ptr, level, 0, SDP_ATTR_RTCP_FB,
> +                                 &inst_num), SDP_SUCCESS);
> +      EXPECT_EQ(sdp_attr_set_rtcp_fb_ack(sdp_ptr, level, payload, inst_num,

Is there any checking here that you can't give it a ridiculous PT?

Also, why is PT a u32 when it's actually a uint8_t?

@@ +170,5 @@
> +      return inst_num;
> +    }
> +
> +    u16 AddNewRtcpFbNack(int level, sdp_rtcp_fb_nack_type_e type,
> +                         u32 payload = 0xFFFF) {

Isn't this a duplicate? Maybe a comment here to explain why not/

@@ +200,5 @@
> +      return inst_num;
> +    }
> +
> +  protected:
> +    int final_level;

This code seems to be google style. Please use trailing underscore to indicate members.

@@ +205,5 @@
> +    void *config_p;
> +    sdp_t *sdp_ptr;
> +};
> +
> +static std::string videoSdp =

const? kVideoSdp?

@@ +321,5 @@
> +}
> +
> +TEST_F(SdpTest, parseRtcpFbCcmVbcm) {
> +  ParseSdp(videoSdp + "a=rtcp-fb:120 ccm vbcm 123 456 789\r\n");
> +  ASSERT_EQ(sdp_attr_get_rtcp_fb_ccm(sdp_ptr, 1, 120, 1), SDP_RTCP_FB_CCM_VBCM);

Are these numeric values meaningful? If so, please check they are right.

@@ +377,5 @@
> +    "a=rtcp-fb:120 foo\r\n"
> +    "a=rtcp-fb:120 foo bar\r\n"
> +    "a=rtcp-fb:120 foo bar baz\r\n");
> +
> +  ASSERT_EQ(sdp_attr_get_rtcp_fb_ack(sdp_ptr, 1, 120, 1), SDP_RTCP_FB_ACK_RPSI);

I did not check all these. Ethan, did you?

@@ +466,5 @@
> +  std::string body = SerializeSdp();
> +  ASSERT_NE(body.find("a=rtcp-fb:* ccm fir\r\n"), std::string::npos);
> +}
> +
> +/* TODO We need to test the pt=* use cases. */

// TODO(abr@...)

Comment 8

[Ethan Hugg [:ehugg]]

>> +    "a=rtcp-fb:120 foo\r\n"
>> +    "a=rtcp-fb:120 foo bar\r\n"
>> +    "a=rtcp-fb:120 foo bar baz\r\n");
>> +
>> +  ASSERT_EQ(sdp_attr_get_rtcp_fb_ack(sdp_ptr, 1, 120, 1), SDP_RTCP_FB_ACK_RPSI);
>
>I did not check all these. Ethan, did you?
>

I checked that they had the right expectations for succes/failure, but I did not contemplate the completeness of coverage.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 765039 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

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

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +52,5 @@
> +
> +    gThread = thread;
> +    sipcc::PeerConnectionCtx::InitializeGlobal(gThread);
> +  }
> +  return true;

At the present, it is mostly used as a sentry to prevent initializing the thread more than once.

Comment 10

[Adam Roach [:abr]]

The patch passes mochi and crash tests locally, as well as the entire suite of unit tests. More importantly, I have verified (by reading the SDP in the offer/answer exchange, not by RTCP analysis) that the changes in the SDP appear to activate NACK and FIR handling in Chrome.

Comment 11

[Adam Roach [:abr]]

Attachment: Part 1: SDP rtcp-fb parsing/serializing

Comment 12

[Adam Roach [:abr]]

Comment on attachment 765522 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

Carrying forward r+ from ekr, ehugg

Comment 13

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ffbbe96954c

Comment 14

[Adam Roach [:abr]]

Comment on attachment 765522 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

[Approval Request Comment]
> Bug caused by (feature/regressing bug #): 

Changes in Chrome codebase.

> User impact if declined: 

WebRTC video sessions with Chrome will freeze, generally very soon after a call starts. This effectively prevents Chrome/Firefox interop for WebRTC.

> Testing completed (on m-c, etc.): 

Local mochi, crash, and unit tests. Manual interop testing with Chrome. The patch is in the process of landing on m-i, and should have cross-platform test results in TBPL soon.

> Risk to taking this patch (and alternatives if risky): 

This patch does add significant new code to the SDP parser and serializer, but it follows well-established patterns in that existing codebase. It is possible, although unlikely, that the patch introduces issues with SDP handling. Such issues are almost certainly isolated to the handling of the rtcp-fb attribute, which isn't processed correctly without this patch anyway.

> String or IDL/UUID changes made by this patch:

None.

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2ffbbe96954c

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 765522 [details] [diff] [review]
Part 1: SDP rtcp-fb parsing/serializing

Approving since the risk appears to be contained to the webrtc code only and regressions should be restricted to the new feature which I trust is getting lots of stress testing.

Comment 17

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/853dbd7a6f11

marking as 'fixed' on 23 since this workaround fixes interop with chrome there.  Not marking it as fixed on 24 since we plan to land the more complete (e.g. negotiation) fix there.

Comment 19

[Adam Roach [:abr]]

Attachment: Part 2: Finish SDP Unit Tests

Comment 20

[Adam Roach [:abr]]

Note to self: Chrome's SDP signals "ccm fir," but reading through their code, it looks like they actually configure their channels to send PLI, not FIR (see https://code.google.com/p/libjingle/source/browse/trunk/talk/session/phone/webrtcvideoengine.cc?r=159&spec=svn161#2444)

Comment 21

[Adam Roach [:abr]]

As a follow-up to Comment 20: Chrome's signaling is actually correct, but incomplete. See https://code.google.com/p/webrtc/issues/detail?id=2252

Comment 22

[Adam Roach [:abr]]

Attachment: Part 3: SDP negotiation of rtcp-fb

Comment 23

[Adam Roach [:abr]]

Attachment: Part 4: Video Conduit configuration for RTCP feedback

Comment 24

[Adam Roach [:abr]]

Attachment: Part 2: Finish SDP Unit Tests

Comment 25

[Adam Roach [:abr]]

Attachment: Part 3: SDP negotiation of rtcp-fb

Comment 26

[Adam Roach [:abr]]

Attachment: Part 4: Video Conduit configuration for RTCP feedback

Comment 27

[Adam Roach [:abr]]

Attachment: Part 5: rtcp-fb unit tests

Comment 28

[Adam Roach [:abr]]

Comment on attachment 793682 [details] [diff] [review]
Part 2: Finish SDP Unit Tests

This patch simply finishes up unit testing that should have been in part 1, if we hadn't been in such a hurry to land that patch.

Comment 29

[Adam Roach [:abr]]

Comment on attachment 793684 [details] [diff] [review]
Part 3: SDP negotiation of rtcp-fb

This patch performs proper offer/answer negotiation of rtcp-fb attributes according to the scheme outlined in RFC 4585, and stores the result of such negotiation in the media structure. Conduit configuration is not included in this patch; see Part 4.

For reference, the relevant passage from RFC 4585 is:

> When used in conjunction with the offer/answer model, 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 30

[Adam Roach [:abr]]

Comment on attachment 793686 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

This patch propagates the negotiated rtcp-fb mechanisms from the SIPCC media structure through the VideoCodecConfig structure into the VideoConduit (which, in turn, configures the webrtc streams).

Note that the changes to sdp.h and ccsdp.h are only a relocation of some enumerations and associated macros -- they do not reflect any code changes. This relocation was necessary in order to give VideoConduit access to these constants without also including substantial swaths of the SIPCC tree.

Comment 31

[Adam Roach [:abr]]

Comment on attachment 793688 [details] [diff] [review]
Part 5: rtcp-fb unit tests

This patch tests the functionality in the "Part 3" and "Part 4" patches.

Comment 32

[Ethan Hugg [:ehugg]]

Comment on attachment 793682 [details] [diff] [review]
Part 2: Finish SDP Unit Tests

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

::: media/webrtc/signaling/test/sdp_unittests.cpp
@@ +442,5 @@
> +  InitLocalSdp();
> +  int level = AddNewMedia(SDP_MEDIA_VIDEO);
> +  AddNewRtcpFbAck(level, SDP_RTCP_FB_ACK_APP, 120);
> +  std::string body = SerializeSdp();
> +std::cout << body << std::endl;

Is this debugging left in by accident?

@@ +451,5 @@
> +  InitLocalSdp();
> +  int level = AddNewMedia(SDP_MEDIA_VIDEO);
> +  AddNewRtcpFbAck(level, SDP_RTCP_FB_ACK_APP);
> +  std::string body = SerializeSdp();
> +std::cout << body << std::endl;

And here.

@@ +683,1 @@
>  /* TODO We need to test the pt=* use cases. */

Does this comment still apply or is it covered by the AllPt cases above?

Comment 33

[Ethan Hugg [:ehugg]]

Comment on attachment 793684 [details] [diff] [review]
Part 3: SDP negotiation of rtcp-fb

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

r+ if while loops fixed.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> + * vim: set ts=8 sts=4 et sw=4 tw=99:
> + * This Source Code Form is subject to the terms of the Mozilla Public

In Bug 796457, Gerv standardized all of these headers.  I don't think we want to add editor tips to them now.

@@ +4427,5 @@
> +    int num_pts;
> +    int pt_codec;
> +    sdp_payload_ind_e indicator;
> +
> +    int i;

In the function below you use pt_index instead of just 'i' which I think is clearer.

@@ +4535,5 @@
> +            if (ack_type >= 0 && ack_type < SDP_MAX_RTCP_FB_ACK) {
> +                fb_types |= SDP_RTCP_FB_ACK_TO_BITMAP(ack_type);
> +            }
> +            i++;
> +        } while (nack_type != SDP_RTCP_FB_CCM_NOT_FOUND);

Is this supposed to be ack_type != SDP_RTCP...

@@ +4546,5 @@
> +            if (ccm_type >= 0 && ccm_type < SDP_MAX_RTCP_FB_CCM) {
> +                fb_types |= SDP_RTCP_FB_CCM_TO_BITMAP(ccm_type);
> +            }
> +            i++;
> +        } while (nack_type != SDP_RTCP_FB_CCM_NOT_FOUND);

And this one, should it be ccm_type !=

Comment 34

[Adam Roach [:abr]]

(In reply to Ethan Hugg [:ehugg] from comment #32)
> Is this debugging left in by accident?

Yes. Thanks for catching it. Removed here and below.

> >  /* TODO We need to test the pt=* use cases. */
> 
> Does this comment still apply or is it covered by the AllPt cases above?

It's mostly covered by those cases, but we still need to verify parsing. I've added a trivial test case to run the parser through its paces.

Comment 35

[Adam Roach [:abr]]

(In reply to Ethan Hugg [:ehugg] from comment #33)
 
> r+ if while loops fixed.

Thanks for catching these. Fixed.

> 
> ::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
> @@ +1,3 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 4 -*-
> > + * vim: set ts=8 sts=4 et sw=4 tw=99:
> > + * This Source Code Form is subject to the terms of the Mozilla Public
> 
> In Bug 796457, Gerv standardized all of these headers.  I don't think we
> want to add editor tips to them now.

We discussed this in IRC, but for the benefit of others: we concluded that adding this to the file is consistent with the guidance in https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Mode_Line -- in this case, I added it mostly because the file has an indenting width that is different than many of the other files in this part of the tree.

Comment 36

[Adam Roach [:abr]]

Attachment: Part 2: Finish SDP Unit Tests

Comment 37

[Adam Roach [:abr]]

Attachment: Part 3: SDP negotiation of rtcp-fb

Comment 38

[Adam Roach [:abr]]

Comment on attachment 794121 [details] [diff] [review]
Part 2: Finish SDP Unit Tests

Carrying forward r+ from ehugg

Comment 39

[Adam Roach [:abr]]

Comment on attachment 794122 [details] [diff] [review]
Part 3: SDP negotiation of rtcp-fb

Carrying forward r+ from ehugg

Comment 40

[Adam Roach [:abr]]

Comment on attachment 794121 [details] [diff] [review]
Part 2: Finish SDP Unit Tests

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

Comment 41

[Adam Roach [:abr]]

Comment on attachment 794122 [details] [diff] [review]
Part 3: SDP negotiation of rtcp-fb

https://hg.mozilla.org/integration/mozilla-inbound/rev/3adaeac9d567

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Backed out for Windows bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/68ddc10ece1c

https://tbpl.mozilla.org/php/getParsedLog.php?id=26881872&tree=Mozilla-Inbound

Comment 43

[Adam Roach [:abr]]

Okay, this was a matter of Windows' compiler not liking where variables were declared. To make sure I catch any similar issues, here's a Windows try build:

https://tbpl.mozilla.org/?tree=Try&rev=b1ae46b12f88

Comment 44

[Adam Roach [:abr]]

That build looks clean, so we're going to try re-landing:

https://hg.mozilla.org/integration/mozilla-inbound/rev/1916191a311c
https://hg.mozilla.org/integration/mozilla-inbound/rev/610f5f410e3b

Comment 45

[Maire Reavy [:mreavy]]

I realize this is about to be resolved.  I'm just tagging it as wanted in Gecko 26.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1916191a311c
https://hg.mozilla.org/mozilla-central/rev/610f5f410e3b

Comment 47

[Adam Roach [:abr]]

Whoops! I should have left the [leave-open] tag on here. Sorry. Two more patches to be reviewed and then land.

Comment 48

[Adam Roach [:abr]]

There is a minor fix required to part 3:

> mozilla/mozilla-central/media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c:4578:27: warning: comparison between ‘sdp_rtcp_fb_ack_type_e’ and ‘enum <anonymous>’ [-Wenum-compare]

The offending line is:
> } while (ack_type != SDP_RTCP_FB_CCM_NOT_FOUND);

Which should be: 
> } while (ack_type != SDP_RTCP_FB_NACK_NOT_FOUND);

This isn't a serious defect, since all of the SDP_RTCP_FB_..._NOT_FOUND enumerations are -1; but the spurious warning is worth fixing.

Comment 49

[Adam Roach [:abr]]

Attachment: Part 3.1: Fix harmless copy-and-paste error

Comment 50

[Adam Roach [:abr]]

Comment on attachment 796263 [details] [diff] [review]
Part 3.1: Fix harmless copy-and-paste error

r+ from ehugg via IRC

Comment 51

[Adam Roach [:abr]]

Comment on attachment 796263 [details] [diff] [review]
Part 3.1: Fix harmless copy-and-paste error

https://hg.mozilla.org/integration/mozilla-inbound/rev/54dc71135d7a

Comment 52

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/54dc71135d7a

Comment 53

[Eric Rescorla (:ekr)]

Comment on attachment 793686 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

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

r- for concerns about directionality

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +62,4 @@
>     * it will be stored here.
>     */
> +  int mWidth;
> +  int mHeight;

Unfortunately, we are going a direction where the max macroblocks is passed down, so this is wrong

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +420,5 @@
>    mPtrRTP->SetRembStatus(mChannel, true, false);
>  
> +  // Check which RTCP feedback methods have been negotiated.
> +  if (mCurSendCodecConfig->mRtcpFbTypes &
> +      SDP_RTCP_FB_NACK_TO_BITMAP(SDP_RTCP_FB_NACK_PLI)) {

I feel like this would be better if we had a fxn like:
mRtpFbTypes_isset(....). This would hide the bitmaskness.

I'm also confused about whether this should be the send
or receive config. Isn't this a receive parametr?

@@ +425,5 @@
> +    // Enable PLI as key frame request method.
> +    CSFLogDebug(logTag, "Enabling PLI frame requests for codec: %s\n",
> +                mCurSendCodecConfig->mName.c_str());
> +    if(mPtrRTP->SetKeyFrameRequestMethod(mChannel,
> +                                      webrtc::kViEKeyFrameRequestPliRtcp) != 0)

Align with opening (.

@@ +437,5 @@
> +    // PLI is not available, but FIR is okay, so we'll use that instead.
> +    CSFLogDebug(logTag, "Enabling FIR frame requests for codec: %s\n",
> +                mCurSendCodecConfig->mName.c_str());
> +    if(mPtrRTP->SetKeyFrameRequestMethod(mChannel,
> +                                      webrtc::kViEKeyFrameRequestFirRtcp) != 0)

Alignment,

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +1483,5 @@
>        config_raw = new mozilla::VideoCodecConfig(
>          payloads[i].remote_rtp_pt,
> +        ccsdpCodecName(payloads[i].codec_type),
> +        payloads[i].video.width,
> +        payloads[i].video.height,

Remove height/width throughout

::: media/webrtc/signaling/src/sipcc/include/ccsdp.h
@@ +293,5 @@
> +
> +#define SDP_RTCP_FB_NACK_TO_BITMAP(type) (1 << (type))
> +#define SDP_RTCP_FB_ACK_TO_BITMAP(type)  (1 << (SDP_MAX_RTCP_FB_NACK + (type)))
> +#define SDP_RTCP_FB_CCM_TO_BITMAP(type)  (1 << (SDP_MAX_RTCP_FB_NACK + \
> +                                                SDP_MAX_RTCP_FB_ACK + (type)))

These would perhaps be easier to understand as fxns. Also, then we could sanity check the inputs so that for instance you can't pass SDP_RTCP_FB_NACK_NOT_FOUND into the bitmap code.

Comment 54

[Eric Rescorla (:ekr)]

Comment on attachment 793688 [details] [diff] [review]
Part 5: rtcp-fb unit tests

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

awaiting new code

Comment 55

[Adam Roach [:abr]]

Attachment: Part 4: Video Conduit configuration for RTCP feedback

Comment 56

[Adam Roach [:abr]]

Attachment: Part 5: rtcp-fb unit tests

Comment 57

[Adam Roach [:abr]]

Comment on attachment 797451 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

This fixes the directionality confusion in the previous patch (mea maxima culpa!) and incorporates the other suggested improvements. To prevent an inclusion nightmare, I had to move the rtcp-fb related constants out into their own header file.

Comment 58

[Adam Roach [:abr]]

I'm going to summarize one of the issues that's explained in a comment in the "part 4" patch: we negotiate RTCP feedback on a per-codec basis, but configure it on a per-conduit basis. So, for the time being, this patch is as correct as it can be within the constraints imposed on us by the video engine. See http://code.google.com/p/webrtc/issues/detail?id=2331

Comment 59

[Eric Rescorla (:ekr)]

Comment on attachment 797451 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

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

r- for potential NACK setting bug.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +414,5 @@
>  
>    //Copy the applied codec for future reference
>    delete mCurSendCodecConfig;
>  
> +  mCurSendCodecConfig = new VideoCodecConfig(*codecConfig);

Please write a copy constructor if you are going to do this.

Don't you need to call SetNACKStatus here if appropriate. The docs
say: "NACK should be enabled on both endpoints in a call."

@@ +471,5 @@
>      {
>        return condError;
>      }
>  
> +    // Keep track of the feedback types that have been requested

Please add a comment explaining the logic and what the options are here.

They seem to be:
NACK/PLI
FIR
FIR/NACK
NACK

Is that right?

@@ +538,5 @@
> +  // capabilities (rather than the more conservative approach of using
> +  // an intersection) is made to provide as many feedback mechanisms
> +  // as are likely to be processed by the remote party (and should be
> +  // relatively safe, since the remote party is required to ignore feedback
> +  // types that it does not understand).

This comment doesn't seem quite right. We're not setting it to the union, we're setting it to the most preferential expressed for any codec.

::: media/webrtc/signaling/src/sipcc/include/ccsdp.h
@@ +246,5 @@
>      SDP_MAX_ATTR_TYPES,
>      SDP_ATTR_INVALID
>  } sdp_attr_e;
>  
> +

Whitespace only change

::: media/webrtc/signaling/src/sipcc/include/ccsdp_rtcp_fb.h
@@ +50,5 @@
> +    SDP_RTCP_FB_CCM_UNKNOWN
> +} sdp_rtcp_fb_ccm_type_e;
> +
> +#if defined(__has_extension) && __has_extension(cxx_static_assert)
> +static_assert(SDP_MAX_RTCP_FB_NACK +

Are we using NSPR stuff here? If so, maybe PR_STATIC_ASSERT

Comment 60

[Eric Rescorla (:ekr)]

Comment on attachment 797452 [details] [diff] [review]
Part 5: rtcp-fb unit tests

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.h
@@ +162,5 @@
> +  }
> +
> +  bool UsingNack() {
> +    return mUsingNack;
> +  }

1. Don't expose webrtc:: constructs here. The idea of VideoConduit is to shield you from that. This includes using the values internally in the .h unfortunately, so you need to define a new type.

2. Shouldn't these both be set to const, since they are accessors that don't change anything?

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +44,5 @@
>  
>  static std::string callerName = "caller";
>  static std::string calleeName = "callee";
>  
> +#define ARRAY_TO_STL(container,type,array) \

I feel like some whitespace  between commas might be nice.

@@ +1343,5 @@
>                              unsigned short level) {
>      a1_.AddIceCandidate(candidate, mid, level, false);
>    }
>  
> +  void CheckRtcpFbSdp(const std::string &sdp,

This could benefit from comments.

@@ +1348,5 @@
> +                      const std::set<std::string>& expected) {
> +
> +    std::set<std::string>::const_iterator i;
> +
> +    for (i = expected.begin(); i != expected.end(); i++) {

This enforces that all the strings in the set have rtcp-fb, right?

@@ +1360,5 @@
> +    std::vector<std::string>::iterator j;
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_TRUE(expected.count(*j));

ASSERT_NE(0, ...)?

@@ +1361,5 @@
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_TRUE(expected.count(*j));
> +    }

These tests seem duplicative. what can we do about that?

@@ +1377,5 @@
> +
> +    // Strip out all three rtcp-fb lines
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");

This seems somewhat brittle. Why are there 3?

@@ +1382,5 @@
> +
> +    // Add rtcp-fb lines for the desired feedback types
> +    std::set<std::string>::const_iterator i;
> +    for (i = feedback.begin(); i != feedback.end(); i++) {
> +      sdpWrapper.AddLine(std::string("a=rtcp-fb:120 ") + (*i) + "\r\n");

Is this because ReplaceLine is busted?

What is attaching these to the right m-line?

@@ +2582,5 @@
> +}
> +
> +TEST_F(SignalingTest, RtcpFbInAnswer)
> +{
> +  const char *feedbackTypes[] = { "nack", "nack pli", "ccm fir" };

Can we pull this list out and make it a constant?

Comment 61

[Adam Roach [:abr]]

Comment on attachment 797452 [details] [diff] [review]
Part 5: rtcp-fb unit tests

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

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +1360,5 @@
> +    std::vector<std::string>::iterator j;
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_TRUE(expected.count(*j));

Sure. This usage seemed consistent with the code guidelines that the use of "if (foo)" is preferred over "if (foo != 0)", but I don't feel strongly one way or another.

@@ +1361,5 @@
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_TRUE(expected.count(*j));
> +    }

I think checking for the presence of expected values separately from the absence of unexpected values is cleaner and easier to understand in the failure case. The alternative would be removing items from the "expected" set as they are found, and checking that it's empty when we're done -- although that would fail on some valid, if odd, cases (e.g. a valid value appearing twice).

My preference would be leaving these as-is.

@@ +1377,5 @@
> +
> +    // Strip out all three rtcp-fb lines
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");
> +    sdpWrapper.DeleteLine("a=rtcp-fb:120");

Yeah, I'll concede that this should do something that more coherently means "remove all lines of this type, regardless of how many there are".

@@ +1382,5 @@
> +
> +    // Add rtcp-fb lines for the desired feedback types
> +    std::set<std::string>::const_iterator i;
> +    for (i = feedback.begin(); i != feedback.end(); i++) {
> +      sdpWrapper.AddLine(std::string("a=rtcp-fb:120 ") + (*i) + "\r\n");

I don't think ReplaceLine is busted, but we're not replacing lines here. What we want to do is make sure only the test-specified types are present in the SDP, instead of the three that get put in offers by default. The clean way to do that is to remove all of the existing lines, and then replace them with the ones that we know we want.

We know that the SDP is constructed to have audio before video, so we're just appending these to the end of the whole SDP body, safe in the knowledge that doing so puts them in the video section. However, your point is a good one in general; I'll clean this up.

@@ +2582,5 @@
> +}
> +
> +TEST_F(SignalingTest, RtcpFbInAnswer)
> +{
> +  const char *feedbackTypes[] = { "nack", "nack pli", "ccm fir" };

I don't follow. This list is different for each test case.

Comment 62

[Adam Roach [:abr]]

Comment on attachment 797451 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

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

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +414,5 @@
>  
>    //Copy the applied codec for future reference
>    delete mCurSendCodecConfig;
>  
> +  mCurSendCodecConfig = new VideoCodecConfig(*codecConfig);

The copy constructor would be completely superfluous -- it's a struct composed entirely of primitive types.

From Scott Meyer's "Effective C++" item 15: "When you declare your own copying functions, you are telling the compiler that there's something about the default implementation that you don't like. Compilers seem to take offense at this, and they retaliate in a curious fashion: they don't tell you when your implementations are almost certainly wrong."

I agree with the general guidance that defining copy constructors should be the exception, not the rule, due to the maintenance overhead required to keep them in sync with the rest of the class.
______________________________________________________

Based on RFC 4585, I can't for the life of me figure out why ViE needs NACK set on both sides. But I do concede that the documentation suggests that it should be set on both ends, so we'll go ahead and do that.

@@ +471,5 @@
>      {
>        return condError;
>      }
>  
> +    // Keep track of the feedback types that have been requested

Originally, I thought the API needed NACK set for PLI to take effect, but my digging around to determine the "NACK on both sides" issue makes me think that's not actually the case. So the logic here will be revised. NACK will be set independently; and then the frame request method can be PLI (preferred), FIR, and "none". I've added comment blocks accordingly.

@@ +538,5 @@
> +  // capabilities (rather than the more conservative approach of using
> +  // an intersection) is made to provide as many feedback mechanisms
> +  // as are likely to be processed by the remote party (and should be
> +  // relatively safe, since the remote party is required to ignore feedback
> +  // types that it does not understand).

We're interpreting capabilities of the remote endpoint to be a union of all the indicated types for all payload types. I'll make this distinction clearer.

::: media/webrtc/signaling/src/sipcc/include/ccsdp_rtcp_fb.h
@@ +50,5 @@
> +    SDP_RTCP_FB_CCM_UNKNOWN
> +} sdp_rtcp_fb_ccm_type_e;
> +
> +#if defined(__has_extension) && __has_extension(cxx_static_assert)
> +static_assert(SDP_MAX_RTCP_FB_NACK +

That would be ideal, but we get into some pretty nasty include hell:

> In file included from .../media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp:8:
> In file included from .../media/webrtc/signaling/./src/mediapipeline/MediaPipeline.h:19:
> In file included from .../media/webrtc/signaling/./src/media-conduit/MediaConduitInterface.h:11:
> In file included from .../media/webrtc/signaling/./src/media-conduit/CodecConfig.h:10:
> In file included from .../media/webrtc/signaling/./src/sipcc/include/ccsdp_rtcp_fb.h:8:
> .../media/webrtc/signaling/./src/common/browser_logging/CSFLog.h:11:2: error: "Must #include CSFLog.h before before any IPDL-generated files or other files that #include prlog.h."

(Note that this happens even if CSFLog.h is included in ccsdp_rtcp_fb.h.)

So, we can either start throwing the CSFLog header into things like MediaPipeline, or we can use the c++11 constructs to test for and use the built-in language constructs wherever they are supported. I vote for the second one.

Comment 63

[Adam Roach [:abr]]

Attachment: Part 4: Video Conduit configuration for RTCP feedback

Comment 64

[Adam Roach [:abr]]

Attachment: Part 5: rtcp-fb unit tests

Comment 65

[Adam Roach [:abr]]

Incidentally, generally inserting into the middle of our parsed SDP object in the unit tests is a bit of a pain. I added a comment about why appending to the end of the SDP is okay for the case I do so. In practice, the parsed SDP object needs a little more work to facilitate more semantic checking and manipulations. Bug 913219.

Comment 66

[Adam Roach [:abr]]

Attachment: Interdiff: part 4

Comment 67

[Adam Roach [:abr]]

Attachment: Interdiff: part 5

Comment 68

[Adam Roach [:abr]]

Comment on attachment 800983 [details] [diff] [review]
Interdiff: part 5

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

::: mozilla-inbound-interdiff-before/media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ -572,5 @@
>                    mPtrViEBase->LastError());
>        return kMediaConduitKeyFrameRequestError;
>      }
>    }
>  

The interdiff didn't work perfectly here; the previous code was:

mFrameRequestMethod = kf_request

@@ -582,5 @@
>        CSFLogError(logTag,  "%s NACKStatus Failed %d ", __FUNCTION__,
>                    mPtrViEBase->LastError());
>        return kMediaConduitNACKStatusError;
>      }
>    }

Same interdiff problem: previous code was:

mUsingNack = use_nack;

::: mozilla-inbound-interdiff-before/media/webrtc/signaling/test/signaling_unittests.cpp
@@ +502,5 @@
>        sdp_map_.erase(it);
>        if(content.empty()) {
>          return;
>        }
> +      std::string value = content.substr(objType.length() + 1);

This off-by-one error was introducing an unnecessary space into the replaced line.

@@ +1279,5 @@
>  
>    void OfferModifiedAnswer(sipcc::MediaConstraints& aconstraints,
>                             sipcc::MediaConstraints& bconstraints,
>                             uint32_t offerSdpCheck, uint32_t answerSdpCheck) {
> +    a1_.CreateOffer(aconstraints, OFFER_AUDIO, offerSdpCheck);

In looking at how to improve the SDP class, I noticed that this was adding an audio codec to the video section. The easy fix is to pull video out of the equation, so that's what I did.

Comment 69

[Eric Rescorla (:ekr)]

Comment on attachment 800405 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

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

lgtm with nits below

::: media/webrtc/signaling/src/media-conduit/CodecConfig.h
@@ +61,5 @@
> +  VideoCodecConfig(int type,
> +                   std::string name,
> +                   int rtcpFbTypes): mType(type),
> +                                     mName(name),
> +                                     mRtcpFbTypes(rtcpFbTypes)

Formatting nit: consider expanding the arg list to one line and then putting the initialer list below.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +474,1 @@
>    //Try Applying the codecs in the list

space after //

Comment 70

[Eric Rescorla (:ekr)]

Comment on attachment 800406 [details] [diff] [review]
Part 5: rtcp-fb unit tests

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

I'd like to see one more version here.

::: media/webrtc/signaling/src/media-conduit/VideoConduit.cpp
@@ +584,5 @@
> +    case webrtc::kViEKeyFrameRequestFirRtcp:
> +      mFrameRequestMethod = FrameRequestFir;
> +      break;
> +    default:
> +      mFrameRequestMethod = FrameRequestUnknown;

Can this happen? Perhaps a MOZ_ASSERT()?

::: media/webrtc/signaling/test/Makefile.in
@@ +140,5 @@
>   -I$(topsrcdir)/media/webrtc/signaling/src/sipcc/include \
>   -I$(topsrcdir)/media/webrtc/signaling/src/peerconnection \
>   -I$(topsrcdir)/media/webrtc/signaling/media-conduit\
> + -I$(topsrcdir)/media/webrtc/trunk/ \
> + -I$(topsrcdir)/media/webrtc/trunk/webrtc/video_engine/include \

Do we still need these?

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +32,5 @@
>  #include "nsIDNSService.h"
>  #include "nsWeakReference.h"
>  #include "nricectx.h"
>  #include "mozilla/SyncRunnable.h"
> +#include "vie_rtp_rtcp.h"

Not needed.

@@ +536,5 @@
> +    it = sdp_map_.lower_bound(objType);
> +    end = sdp_map_.upper_bound(objType);
> +    while (it != end) {
> +      std::string value = it->second.second;
> +      if (value.find("\r") != std::string::npos) {

How could there not be a \r? Perhaps an ASSERT_TRUE?

@@ +540,5 @@
> +      if (value.find("\r") != std::string::npos) {
> +        value = value.substr(0, value.find("\r"));
> +      }
> +      values.push_back(value);
> +      it++;

Prefer ++it. Or arguably just make this a for(;;)

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Preincrement_and_Predecrement#Preincrement_and_Predecrement

@@ +1022,5 @@
> +              << ((flags & PIPELINE_VIDEO) ? "video" : "audio")
> +              << " pipeline (stream " << stream
> +              << ", track " << track << "); expect "
> +              << ((flags & PIPELINE_RTCP_MUX) ? "MUX, " : "no MUX, ")
> +              << ((flags & PIPELINE_RTCP_NACK) ? "NACK." : "no NACK.")

You might be happier if the "." were a separate clause thus preventing the need to change the "no NACK." when you add something else.

@@ +1049,5 @@
> +        mozilla::WebrtcVideoConduit *video_conduit =
> +          static_cast<mozilla::WebrtcVideoConduit*>(conduit);
> +        ASSERT_EQ(video_conduit->UsingNackBasic(),
> +                  !!(flags & PIPELINE_RTCP_NACK));
> +        ASSERT_EQ(video_conduit->FrameRequestMethod(), frameRequestMethod);

Swap these ASSERT_EQ() values with the expected value comoing first.

@@ +1362,5 @@
> +    std::set<std::string>::const_iterator i;
> +
> +    // Iterate through the list of expected feedback types and ensure
> +    // that none of them are missing.
> +    for (i = expected.begin(); i != expected.end(); i++) {

++i

@@ +1363,5 @@
> +
> +    // Iterate through the list of expected feedback types and ensure
> +    // that none of them are missing.
> +    for (i = expected.begin(); i != expected.end(); i++) {
> +      std::string attr = std::string("\r\na=rtcp-fb:120 ") + (*i) + "\r\n";

This 120 seems unwisely hardcoded, here and below.

@@ +1376,5 @@
> +    std::vector<std::string>::iterator j;
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_NE(0UL, expected.count(*j));

Do we need UL here?

@@ +1396,5 @@
> +
> +    // Add rtcp-fb lines for the desired feedback types
> +    // We know that the video section is generated second (last),
> +    // so appending these to the end of the SDP has the desired effect.
> +    std::set<std::string>::const_iterator i;

it

@@ +1397,5 @@
> +    // Add rtcp-fb lines for the desired feedback types
> +    // We know that the video section is generated second (last),
> +    // so appending these to the end of the SDP has the desired effect.
> +    std::set<std::string>::const_iterator i;
> +    for (i = feedback.begin(); i != feedback.end(); i++) {

++it

@@ +2575,5 @@
>    // for RTP and RTCP flows
>    // The first Local pipeline gets stored at 0
>    a1_.CheckMediaPipeline(0, 0, PIPELINE_LOCAL | PIPELINE_SEND);
>  
>    // Now check video mux.

The title of this test seems to not match the fact that we are checking video here

Comment 71

[Eric Rescorla (:ekr)]

Attachment: Part 5: rtcp-fb unit tests

Comment 72

[Eric Rescorla (:ekr)]

Attachment: Part 4: Video Conduit configuration for RTCP feedback

Comment 73

[Eric Rescorla (:ekr)]

I reviewed the just uploaded rebased patches. Adam, please fix issues and check rebase.

Comment 74

[Adam Roach [:abr]]

Comment on attachment 800406 [details] [diff] [review]
Part 5: rtcp-fb unit tests

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

::: media/webrtc/signaling/test/Makefile.in
@@ +140,5 @@
>   -I$(topsrcdir)/media/webrtc/signaling/src/sipcc/include \
>   -I$(topsrcdir)/media/webrtc/signaling/src/peerconnection \
>   -I$(topsrcdir)/media/webrtc/signaling/media-conduit\
> + -I$(topsrcdir)/media/webrtc/trunk/ \
> + -I$(topsrcdir)/media/webrtc/trunk/webrtc/video_engine/include \

Empirically: yes and no, respectively.

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +536,5 @@
> +    it = sdp_map_.lower_bound(objType);
> +    end = sdp_map_.upper_bound(objType);
> +    while (it != end) {
> +      std::string value = it->second.second;
> +      if (value.find("\r") != std::string::npos) {

We can't ASSERT_TRUE because we don't return void -- so I'm going to just do an ADD_FAIL if this is malformed.

@@ +1022,5 @@
> +              << ((flags & PIPELINE_VIDEO) ? "video" : "audio")
> +              << " pipeline (stream " << stream
> +              << ", track " << track << "); expect "
> +              << ((flags & PIPELINE_RTCP_MUX) ? "MUX, " : "no MUX, ")
> +              << ((flags & PIPELINE_RTCP_NACK) ? "NACK." : "no NACK.")

It would still need updating to have a comma...

@@ +1363,5 @@
> +
> +    // Iterate through the list of expected feedback types and ensure
> +    // that none of them are missing.
> +    for (i = expected.begin(); i != expected.end(); i++) {
> +      std::string attr = std::string("\r\na=rtcp-fb:120 ") + (*i) + "\r\n";

And dozens of other places in these test cases. Bug 913219.

@@ +1376,5 @@
> +    std::vector<std::string>::iterator j;
> +    for (j = values.begin(); j != values.end(); j++) {
> +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> +                << "'" << std::endl;
> +      ASSERT_NE(0UL, expected.count(*j));

We certainly need the U. Omitting the L is fine, as the compiler is happy to expand up to the correct length.

@@ +2575,5 @@
>    // for RTP and RTCP flows
>    // The first Local pipeline gets stored at 0
>    a1_.CheckMediaPipeline(0, 0, PIPELINE_LOCAL | PIPELINE_SEND);
>  
>    // Now check video mux.

That appears to be a splinter truncation issue. The name of this testcase is "FullCallAudioNoMuxVideoMux".

Comment 75

[Adam Roach [:abr]]

Attachment: Part 5: rtcp-fb unit tests

Comment 76

[Adam Roach [:abr]]

Part 4:

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

Comment 77

[Adam Roach [:abr]]

Linux vomited all over the static assert feature check, which is now officially causing more pain than it could ever possibly avoid. I'm removing it:

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

Comment 78

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

Windows builds were busted still after the fix, so backed out part 4 and the bustage fix in:
http://hg.mozilla.org/integration/mozilla-inbound/rev/20de45e5a35e 
http://hg.mozilla.org/integration/mozilla-inbound/rev/4fdd3cddee3a

Build bustage looked like https://tbpl.mozilla.org/php/getParsedLog.php?id=27663240&tree=Mozilla-Inbound

Comment 79

[Adam Roach [:abr]]

Since I appear to have offended the entire compiler community with my previous attempt to land, here's a try push with a couple of compiler-pleasing tweaks:

https://tbpl.mozilla.org/?tree=Try&rev=42cc42b53c44

Comment 80

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/67032cf58c31

Comment 81

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/67032cf58c31

Comment 82

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

I believe this just didn't get marked when it uplifted to m-c

Comment 83

[Adam Roach [:abr]]

(In reply to Randell Jesup [:jesup] from comment #82)
> I believe this just didn't get marked when it uplifted to m-c

I think that's because of the [leave-open] on the whiteboard. I still need to land the part 5 patch (unit tests).

Comment 84

[Eric Rescorla (:ekr)]

(In reply to Adam Roach [:abr] from comment #74)
> Comment on attachment 800406 [details] [diff] [review]
> Part 5: rtcp-fb unit tests
> 
> Review of attachment 800406 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/test/Makefile.in
> @@ +140,5 @@
> >   -I$(topsrcdir)/media/webrtc/signaling/src/sipcc/include \
> >   -I$(topsrcdir)/media/webrtc/signaling/src/peerconnection \
> >   -I$(topsrcdir)/media/webrtc/signaling/media-conduit\
> > + -I$(topsrcdir)/media/webrtc/trunk/ \
> > + -I$(topsrcdir)/media/webrtc/trunk/webrtc/video_engine/include \
> 
> Empirically: yes and no, respectively.

OK. I worked out why. You are referencing WebrtcAudioConduit::<...> from
the signaling tests. You should only be using the abstract wrapper interface
in MediaConduitInterface from outside the module If you fix that, you should
be able to remove this include.

> ::: media/webrtc/signaling/test/signaling_unittests.cpp
> @@ +536,5 @@
> > +    it = sdp_map_.lower_bound(objType);
> > +    end = sdp_map_.upper_bound(objType);
> > +    while (it != end) {
> > +      std::string value = it->second.second;
> > +      if (value.find("\r") != std::string::npos) {
> 
> We can't ASSERT_TRUE because we don't return void -- so I'm going to just do
> an ADD_FAIL if this is malformed.
> 
> @@ +1022,5 @@
> > +              << ((flags & PIPELINE_VIDEO) ? "video" : "audio")
> > +              << " pipeline (stream " << stream
> > +              << ", track " << track << "); expect "
> > +              << ((flags & PIPELINE_RTCP_MUX) ? "MUX, " : "no MUX, ")
> > +              << ((flags & PIPELINE_RTCP_NACK) ? "NACK." : "no NACK.")
> 
> It would still need updating to have a comma...
> 
> @@ +1363,5 @@
> > +
> > +    // Iterate through the list of expected feedback types and ensure
> > +    // that none of them are missing.
> > +    for (i = expected.begin(); i != expected.end(); i++) {
> > +      std::string attr = std::string("\r\na=rtcp-fb:120 ") + (*i) + "\r\n";
> 
> And dozens of other places in these test cases. Bug 913219.
> 
> @@ +1376,5 @@
> > +    std::vector<std::string>::iterator j;
> > +    for (j = values.begin(); j != values.end(); j++) {
> > +      std::cout << " - Verifying that rtcp-fb is okay: '" << *j
> > +                << "'" << std::endl;
> > +      ASSERT_NE(0UL, expected.count(*j));
> 
> We certainly need the U. Omitting the L is fine, as the compiler is happy to
> expand up to the correct length.
> 
> @@ +2575,5 @@
> >    // for RTP and RTCP flows
> >    // The first Local pipeline gets stored at 0
> >    a1_.CheckMediaPipeline(0, 0, PIPELINE_LOCAL | PIPELINE_SEND);
> >  
> >    // Now check video mux.
> 
> That appears to be a splinter truncation issue. The name of this testcase is
> "FullCallAudioNoMuxVideoMux".

Comment 85

[Adam Roach [:abr]]

Attachment: Part 5: rtcp-fb unit testsBug 880067 - Part 5: rtcp-fb unit tests

Comment 86

[Adam Roach [:abr]]

Comment on attachment 801175 [details] [diff] [review]
Part 4: Video Conduit configuration for RTCP feedback

Moving r+ forward from before (the patch is already landed).

Comment 87

[Adam Roach [:abr]]

Comment on attachment 806252 [details] [diff] [review]
Part 5: rtcp-fb unit testsBug 880067 - Part 5: rtcp-fb unit tests

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

r+ from ekr via IRC

Comment 88

[Adam Roach [:abr]]

Part 5:
https://hg.mozilla.org/integration/mozilla-inbound/rev/623728a4a34b

Comment 89

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/623728a4a34b