786234 - BUNDLE transport work - Implement SSRC filtering in MediaPipeline

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Eric Rescorla (:ekr)]

Each MediaPipeline only processes some subset of the SSRC space. We need to implement filtering to ignore irrelevant SSRCs. (This is relevant for bundle).

Comment 1

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

Bundle isn't blocking for preffing on, so blocking- on this one too

Comment 2

[Maire Reavy [:mreavy]]

In addition to "Implement SSRC filtering in MediaPipeline", I'd like to turn this into the primary bug for the "BUNDLE transport" work.

I'd like to target this for Gecko 29 and set a deadline of Jan 10.

Comment 3

[Byron Campen [:bwc]]

Attachment: Part 1. Set up the interface we need with sipcc to get the information to build filters.

Comment 4

[Byron Campen [:bwc]]

Comment on attachment 8350343 [details] [diff] [review]
Part 1. Set up the interface we need with sipcc to get the information to build filters.

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

There is a comment about something unrelated in the patch, if you could weigh in on that too.

Comment 5

[Byron Campen [:bwc]]

Attachment: Part 2. Implementation of the filtering logic itself, plus a unit-test.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8350394 [details] [diff] [review]
Part 2. Implementation of the filtering logic itself, plus a unit-test.

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

I may refine this some for the RTCP case, but does this look basically sane to you?

Comment 7

[Byron Campen [:bwc]]

Attachment: Part 1. Set up the interface we need with sipcc to get the information to build filters.

We need the bundle level here too

Comment 8

[Adam Roach [:abr]]

Comment on attachment 8350714 [details] [diff] [review]
Part 1. Set up the interface we need with sipcc to get the information to build filters.

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

r- for use of integer literals for dimensioning arrays.

Generally: although these files are somewhat inconsistent, the rule of thumb to follow when adding new comments to a file is to use c-style comments (/* */) when working in a c file, and C++ style comments (//) when working in a C++ file.

The comments starting around line 993 of lsm.c generally extend past column 80. Consider wrapping them.

::: media/webrtc/signaling/src/sipcc/core/gsm/lsm.c
@@ +991,5 @@
> +                attrs.num_ssrcs = 0;
> +                attrs.num_unique_payload_types = 0;
> +                // TODO([e-mail]): Fill in attrs.bundle_stream_correlator
> +                // TODO([e-mail]): Fill in attrs.ssrcs
> +                // TODO([e-mail]): Fill in attrs.unique_payload_types

I generally dislike TODOs without bug numbers, since it's never clear how they'll be found again. Since I'm going to take care of this as part of Bug 784491, go ahead and cite it.

Also, you may as well collapse this down into a single TODO.

@@ +996,5 @@
> +
> +                // TODO([e-mail]): Shouldn't we be setting attrs.is_video
> +                // here? How is the callee determining whether this is video or not?
> +                // It cannot safely use whether attrs.video.opaque is set, because
> +                // we don't clear it when we get to the following m-line...

Yes, it looks like is_video is never read or written. Feel free to remedy this situation.

@@ +1243,5 @@
> +            attrs.num_ssrcs = 0;
> +            attrs.num_unique_payload_types = 0;
> +            // TODO([e-mail]): Fill in attrs.bundle_stream_correlator
> +            // TODO([e-mail]): Fill in attrs.ssrcs
> +            // TODO([e-mail]): Fill in attrs.unique_payload_types

Same comments as above.

@@ +1248,5 @@
> +
> +            // TODO([e-mail]): Shouldn't we be setting attrs.is_video
> +            // here? How is the callee determining whether this is video or not?
> +            // It cannot safely use whether attrs.video.opaque is set, because
> +            // we don't clear it when we get to the following m-line...

Same comment as above: feel free to fix the setting of is_video in this patch.

::: media/webrtc/signaling/src/sipcc/include/vcm.h
@@ +338,5 @@
>    vcm_audioAttrs_t audio; /**< audio line attribs */
>    vcm_videoAttrs_t video; /**< Video Atrribs */
> +  uint32_t bundle_level; /**< Where bundle transport info lives, if any */
> +  // Some stuff for assisting in stream correlation for bundle
> +  cc_uint32_t        bundle_stream_correlator;

Document this. If it's what I expect, you would say something like "Reserved for use with an RTP header extension for correlating packets with media sections."

@@ +342,5 @@
> +  cc_uint32_t        bundle_stream_correlator;
> +  // Should be enough for any reasonable use-case
> +  cc_uint32_t        ssrcs[16];
> +  cc_uint8_t         num_ssrcs;
> +  cc_uint8_t         unique_payload_types[16];

Please use preprocessor macros rather than integer literals here. See, for example, the use of "VCM_SRTP_MAX_KEY_SIZE" earlier in this file.

Also, the concept of "unique payload types" is esoteric enough that it could probably benefit from at least a cursory explanation; something like: "This is a list of payload types that are used in this media section and in no others."

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8350394 [details] [diff] [review]
Part 2. Implementation of the filtering logic itself, plus a unit-test.

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

Due to time constraints, I have not looked over the unittests. I'm not clearing the feedback flag so as to remind myself to come back and look at those after the winter break. In the meanwhile, I have comments on the existing filter implementation. It looks like a nice design in general, but it needs additional handling for some less-obvious use cases.

I think we need additional "update" methods here to handle renegotiations. Consider receipt of a new offer from the remote party with an ongoing session. Also consider our side generating a new offer and receiving a new answer.

Note that one of the reasons we might get a new offer from the remote side is exactly because they need to *move* an SSRC from one line to another. From that perspective, when we get a new list of explicit SSRCs from the remote side, we need to take care to *replace* the SSRC map with that list instead of supplementing what we've figured out ourselves.

A tricky situation that also might arise, once we start using correlators, is that such a situation (moving an SSRC from one m-line to another) would probably result in any reassigned RTP packets winning a race with the SDP that reassigns them. In other words, we would get an RTP packet with an existing, known correlator, and an existing, known SSRC, but it won't be the correlator that has historically gone with that SSRC. It's a *changed* mapping. The way this is currently written, you'll end up with that SSRC in two different filters, which can go wrong once the correlator stops being included in the RTP.

It's messy, but what this means is that these filters probably need to be aware of the other filters in their bundle, and to unset any correlator-based mappings on their siblings whenever they form an ssrc-correlator mapping.

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ +57,5 @@
> +void MediaPipelineFilter::SetCorrelator(uint32_t correlator) {
> +  correlator_ = correlator;
> +}
> +
> +void MediaPipelineFilter::UpdateFromAnswer(const MediaPipelineFilter& new_filter) {

Wrap to 80 characters.

@@ +63,5 @@
> +  ssrc_set_.insert(new_filter.ssrc_set_.begin(), new_filter.ssrc_set_.end());
> +
> +  // We could union these, and leave the payload types that the answerer decided
> +  // not to use, but why would we?
> +  payload_type_set_ = new_filter.payload_type_set_;

The PTs that the remote party expects us to send them (which is what we get in the answer) isn't necessarily related in any useful way to the PTs that we've advertised. In terms of filter handling, we must ignore the PTs received from the remote end.

@@ +65,5 @@
> +  // We could union these, and leave the payload types that the answerer decided
> +  // not to use, but why would we?
> +  payload_type_set_ = new_filter.payload_type_set_;
> +
> +  if (new_filter.correlator_) {

Similarly, the correlators we receive from the remote endpoint must have no bearing on our local processing. We send the correlators to them, they use them to generate RTP. Similarly, they send us correlators, and we use them to send RTP. The only correlator that has any bearing on our multiplexing is that present in out local description.

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
@@ +21,5 @@
> +
> +namespace mozilla {
> +
> +// A class that handles the work of filtering RTP packets that arrive at a
> +// MediaPipeline. This is primarily important for the use of BUNDLE (ie; multiple

Wrap to 80 characters.

@@ +37,5 @@
> +//    we can simply use this information to populate the filter. The only
> +//    shortcoming here is when RTP packets arrive before the answer does. See
> +//    above.
> +//
> +// 3) As a fallback, we can try to use payload type IDs to perform correlation.

"...but only if they are unique to a single media section."

@@ +45,5 @@
> +
> +  // Checks whether this packet passes the filter, possibly updating the filter
> +  // in the process (if the correlator is used, it can teach the filter about
> +  // ssrcs)
> +  bool Filter(const webrtc::RTPHeader& header, uint32_t correlator = 0);

I don't think you need to pass the correlator as a separate parameter here. As an RTP header extension, I would expect to find it inside the webrtc::RTPHeader itself.

@@ +47,5 @@
> +  // in the process (if the correlator is used, it can teach the filter about
> +  // ssrcs)
> +  bool Filter(const webrtc::RTPHeader& header, uint32_t correlator = 0);
> +
> +  // This is for RTCP processing.

Howabout we reflect this in the name? e.g. "FilterRtcp" rather than just "Filter"

@@ +51,5 @@
> +  // This is for RTCP processing.
> +  bool Filter(uint32_t ssrc);
> +
> +  void AddSSRC(uint32_t ssrc);
> +  void AddPT(uint8_t payload_type);

Please rename this method to make it clearer that we're talking about PTs that are unique to this media line, not just PTs that it can process. I don't want someone coming along later and wondering why we're not just shoving all the PTs in here and breaking this mechanism. It's probably also worth explaining why this is a valid thing to do.

@@ +54,5 @@
> +  void AddSSRC(uint32_t ssrc);
> +  void AddPT(uint8_t payload_type);
> +  void SetCorrelator(uint32_t correlator);
> +
> +  void UpdateFromAnswer(const MediaPipelineFilter& new_filter);

Please rename to to "UpdateFromRemoteDescription" or "UpdateFromRemoteAnswer" or something similar -- we want to be clear that this isn't to be used to update filters based on the answer we've generated locally.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +32,5 @@
>  
>  #include "mtransport_test_utils.h"
>  #include "runnable_utils.h"
>  
> +#include "webrtc/modules/interface/module_common_types.h"

Please figure out how to frob the Makefile to avoid this kind of really long path in an include.

Comment 10

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #9)
> Comment on attachment 8350394 [details] [diff] [review]
> Part 2. Implementation of the filtering logic itself, plus a unit-test.
> 
> Review of attachment 8350394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Due to time constraints, I have not looked over the unittests. I'm not
> clearing the feedback flag so as to remind myself to come back and look at
> those after the winter break. In the meanwhile, I have comments on the
> existing filter implementation. It looks like a nice design in general, but
> it needs additional handling for some less-obvious use cases.
> 
> I think we need additional "update" methods here to handle renegotiations.
> Consider receipt of a new offer from the remote party with an ongoing
> session. Also consider our side generating a new offer and receiving a new
> answer.
> 
> Note that one of the reasons we might get a new offer from the remote side
> is exactly because they need to *move* an SSRC from one line to another.
> From that perspective, when we get a new list of explicit SSRCs from the
> remote side, we need to take care to *replace* the SSRC map with that list
> instead of supplementing what we've figured out ourselves.
> 

   For remote offers, once we decide that we will accept them, we need to repopulate the filter with the new set of SSRCs (I think this basically just amounts to creating a new filter, updating it with the remote description, and replacing the old filter with the new; if the remote end just sent us SDP with SSRCs in it, discarding SSRCs learned from a correlator is probably not going to hurt us). For local offers where we do something like move one of our SSRCs, we should take no action until we see the answer, right? In that case, we'd need to remember to update our own set of SSRCs. But this is a little out of scope for this ticket.

> A tricky situation that also might arise, once we start using correlators,
> is that such a situation (moving an SSRC from one m-line to another) would
> probably result in any reassigned RTP packets winning a race with the SDP
> that reassigns them. In other words, we would get an RTP packet with an
> existing, known correlator, and an existing, known SSRC, but it won't be the
> correlator that has historically gone with that SSRC. It's a *changed*
> mapping. The way this is currently written, you'll end up with that SSRC in
> two different filters, which can go wrong once the correlator stops being
> included in the RTP.
> 
> It's messy, but what this means is that these filters probably need to be
> aware of the other filters in their bundle, and to unset any
> correlator-based mappings on their siblings whenever they form an
> ssrc-correlator mapping.
> 

   So, if a filter sees one of its SSRCs attached to a correlator that doesn't belong to it (remember, all of the filters get to see all the traffic in the bundle), it removes that SSRC. That'll work, right?
 
> @@ +63,5 @@
> > +  ssrc_set_.insert(new_filter.ssrc_set_.begin(), new_filter.ssrc_set_.end());
> > +
> > +  // We could union these, and leave the payload types that the answerer decided
> > +  // not to use, but why would we?
> > +  payload_type_set_ = new_filter.payload_type_set_;
> 
> The PTs that the remote party expects us to send them (which is what we get
> in the answer) isn't necessarily related in any useful way to the PTs that
> we've advertised. In terms of filter handling, we must ignore the PTs
> received from the remote end.
> 

  Ok.

> @@ +65,5 @@
> > +  // We could union these, and leave the payload types that the answerer decided
> > +  // not to use, but why would we?
> > +  payload_type_set_ = new_filter.payload_type_set_;
> > +
> > +  if (new_filter.correlator_) {
> 
> Similarly, the correlators we receive from the remote endpoint must have no
> bearing on our local processing. We send the correlators to them, they use
> them to generate RTP. Similarly, they send us correlators, and we use them
> to send RTP. The only correlator that has any bearing on our multiplexing is
> that present in out local description.
> 

   Right; new_filter.correlator is never set with their correlator. Honestly, this function is a bit of a misnomer. Probably should be called UpdateFromRemoteDescription, and the initial filter is always constructed from the local description.

> @@ +45,5 @@
> > +
> > +  // Checks whether this packet passes the filter, possibly updating the filter
> > +  // in the process (if the correlator is used, it can teach the filter about
> > +  // ssrcs)
> > +  bool Filter(const webrtc::RTPHeader& header, uint32_t correlator = 0);
> 
> I don't think you need to pass the correlator as a separate parameter here.
> As an RTP header extension, I would expect to find it inside the
> webrtc::RTPHeader itself.
> 
   
   I would too, but it turns out I was wrong. RTPHeader does not support arbitrary extension headers right now. Once it does (or at least supports this specific extension header), we can fold it in.


> @@ +47,5 @@
> > +  // in the process (if the correlator is used, it can teach the filter about
> > +  // ssrcs)
> > +  bool Filter(const webrtc::RTPHeader& header, uint32_t correlator = 0);
> > +
> > +  // This is for RTCP processing.
> 
> Howabout we reflect this in the name? e.g. "FilterRtcp" rather than just
> "Filter"
>

   I think I am sure enough that this is the only use now.
 
> @@ +54,5 @@
> > +  void AddSSRC(uint32_t ssrc);
> > +  void AddPT(uint8_t payload_type);
> > +  void SetCorrelator(uint32_t correlator);
> > +
> > +  void UpdateFromAnswer(const MediaPipelineFilter& new_filter);
> 
> Please rename to to "UpdateFromRemoteDescription" or
> "UpdateFromRemoteAnswer" or something similar -- we want to be clear that
> this isn't to be used to update filters based on the answer we've generated
> locally.
>

   Looks like I already was on the same page here.
 
> ::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
> @@ +32,5 @@
> >  
> >  #include "mtransport_test_utils.h"
> >  #include "runnable_utils.h"
> >  
> > +#include "webrtc/modules/interface/module_common_types.h"
> 
> Please figure out how to frob the Makefile to avoid this kind of really long
> path in an include.

   Honestly, I prefer long includes, especially in large projects.

Comment 11

[Byron Campen [:bwc]]

Attachment: Part 1. Set up the interface we need with sipcc to get the information to build filters.

Incorporate feedback.

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 2. Implementation of the filtering logic itself, plus a unit-test.

Incorporate feedback.

Comment 13

[Adam Roach [:abr]]

Comment on attachment 8355338 [details] [diff] [review]
Part 1. Set up the interface we need with sipcc to get the information to build filters.

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

Looks good. Thanks.

Comment 14

[Adam Roach [:abr]]

Comment on attachment 8355377 [details] [diff] [review]
Part 2. Implementation of the filtering logic itself, plus a unit-test.

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

Looks good to me.  A few nits inline. The "unique PT" one probably deserves its own unit test as well, but I'm proactively r+'ing that test based on its straightforwardness and similarity to the other tests you have in here.

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
@@ +62,5 @@
> +  void AddLocalSSRC(uint32_t ssrc);
> +  void AddRemoteSSRC(uint32_t ssrc);
> +
> +  // When a payload type id is unique to our media section, add it here.
> +  void AddUniquePT(uint8_t payload_type);

I think we also need something like "ResetUniquePTs()", since a renegotiation that adds a new media section could result in a PT suddenly becoming non-unique (unified-plan requires PT re-use under certain specified circumstances). In other words, any time the PTs are updated from the local description, we need to first destroy the unique PT map, and then add in the PTs from the SDP.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +345,5 @@
>  };
>  
> +class MediaPipelineFilterTest : public ::testing::Test {
> +  public:
> +    bool Filter(MediaPipelineFilter& filter,

static?

@@ +405,5 @@
> +TEST_F(MediaPipelineFilterTest, TestPayloadTypeFilter) {
> +  MediaPipelineFilter filter;
> +  filter.AddUniquePT(110);
> +  ASSERT_TRUE(Filter(filter, 0, 555, 110));
> +  ASSERT_FALSE(Filter(filter, 0, 556, 111));

ASSERT_TRUE(Filter(filter, 0, 555, 111));

Comment 15

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #14)
> Comment on attachment 8355377 [details] [diff] [review]
> Part 2. Implementation of the filtering logic itself, plus a unit-test.
> 
> Review of attachment 8355377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.  A few nits inline. The "unique PT" one probably deserves
> its own unit test as well, but I'm proactively r+'ing that test based on its
> straightforwardness and similarity to the other tests you have in here.
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.h
> @@ +62,5 @@
> > +  void AddLocalSSRC(uint32_t ssrc);
> > +  void AddRemoteSSRC(uint32_t ssrc);
> > +
> > +  // When a payload type id is unique to our media section, add it here.
> > +  void AddUniquePT(uint8_t payload_type);
> 
> I think we also need something like "ResetUniquePTs()", since a
> renegotiation that adds a new media section could result in a PT suddenly
> becoming non-unique (unified-plan requires PT re-use under certain specified
> circumstances). In other words, any time the PTs are updated from the local
> description, we need to first destroy the unique PT map, and then add in the
> PTs from the SDP.

Actually, this isn't quite what we want; we don't know if we need to scrap the existing filter or not until the answer comes in. Additionally, our SSRC set may change, as well as our correlator (in fact, we probably _want_ to change our correlator so we can detect ASAP if the other end decided to use our new description). I have a feeling this will require some non-trivial design, so adding this API right now is premature.

> 
> ::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
> @@ +345,5 @@
> >  };
> >  
> > +class MediaPipelineFilterTest : public ::testing::Test {
> > +  public:
> > +    bool Filter(MediaPipelineFilter& filter,
> 
> static?
> 

   Is that how gtest fixtures are supposed to work?

> @@ +405,5 @@
> > +TEST_F(MediaPipelineFilterTest, TestPayloadTypeFilter) {
> > +  MediaPipelineFilter filter;
> > +  filter.AddUniquePT(110);
> > +  ASSERT_TRUE(Filter(filter, 0, 555, 110));
> > +  ASSERT_FALSE(Filter(filter, 0, 556, 111));
> 
> ASSERT_TRUE(Filter(filter, 0, 555, 111));

Actually, this is intentional; we learn SSRCs from PT matches, because we need to know the SSRCs to match RTCP packets (they don't have the PT ids that the RTP packets do).

Comment 16

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #14)
> Comment on attachment 8355377 [details] [diff] [review]
> Part 2. Implementation of the filtering logic itself, plus a unit-test.
> 
> Review of attachment 8355377 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.  A few nits inline. The "unique PT" one probably deserves
> its own unit test as well, but I'm proactively r+'ing that test based on its
> straightforwardness and similarity to the other tests you have in here.
> 

   I have a couple of PT tests; one that just tests whether PT filtering works at all, and a subsequent one that tests whether we learn SSRCs from PT matches. What test did you have in mind, specifically?

Comment 17

[Byron Campen [:bwc]]

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

Comment 18

[Byron Campen [:bwc]]

Comment on attachment 8355338 [details] [diff] [review]
Part 1. Set up the interface we need with sipcc to get the information to build filters.

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

Let's go ahead and check this in since Adam's stuff depends on it. We'll leave Part 2 until I'm sure it does everything that part 3 needs.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8355338 [details] [diff] [review]
Part 1. Set up the interface we need with sipcc to get the information to build filters.

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

Comment 20

[Ryan VanderMeulen [:RyanVM]]

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

Comment 21

[Byron Campen [:bwc]]

Forgot to mark this leave-open.

Comment 22

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Work in progress

Comment 23

[Byron Campen [:bwc]]

Comment on attachment 8356773 [details] [diff] [review]
Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

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

Is this roughly going in the right direction? The logic for dealing with all the possibilities (bundle or not, and for each of those, mux or not) is still a little fiddly, but it is not too bad. I can work on it some more, if need be.

Comment 24

[Adam Roach [:abr]]

Comment on attachment 8356773 [details] [diff] [review]
Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

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

I'm still wrapping my head around some of the changes in MediaPipeline, which is why I'm leaving the f? flag on. In the meanwhile, I wanted to get you some early feedback on the stuff I've looked through before I sign off for the evening, since we might need to have a conversation about the API for filter creation.

VcmSIPCCBinding.cpp:1581: Space between 'if' and parenthesis
MediaPipeline.cpp:277: Space between 'if' and parenthesis
MediaPipeline.cpp:551: Line is 81 characters long; please wrap to 80
MediaPipeline.cpp:721: Line is 83 characters long; please wrap to 80
MediaPipeline.h:195: Line is 84 characters long; please wrap to 80
MediaPipeline.h:196: Line is 85 characters long; please wrap to 80

::: media/mtransport/transportflow.cpp
@@ +200,5 @@
>  }
>  
> +bool TransportFlow::Contains(TransportLayer *layer) const {
> +  if (layers_) {
> +    for (auto l = layers_->begin(); l != layers_->end(); ++l) {

Yeah, that would be nice.

I don't think we have full C++11 support across the platforms yet; in particular, I think our Windows build relies on a C++98 compiler. The C++11 things we can use right now are those that can be backported through the creative use of libraries and macros.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1650,5 @@
>  
>      case SETREMOTEDESCSUCCESS:
> +      // TODO([e-mail]): If we are doing bundle, grab remote and
> +      // local SDP, build the filters, and toss them to PeerConnectionMedia on
> +      // the STS thread. These filters will be applied to the MediaPipelines.

This is not where that should happen. The SDP at this point is a text blob for the javascript. Our point of contact between SIPCC and the PeerConnectionImpl/Media/etc is in the VcmSIPCCBinding, not here.

Here's how this needs to work: down in lsm.c, we'll add something that calls into a new VCM function (e.g., vcmRxAddFilters), with a semantic indication of the parameters that should be used to build the filters. If this can be done piecemeal on a per-media-line basis, then we put it in lsm_open_rx() or lsm_rx_start(); otherwise, we need to back up a level and add it in where lsm_rx_start() is called from.

Basically, let me know what information you need, and I'll add the code to lsm.c that calls the new function that you will define in VcmSIPCCBinding.cpp. Then, your code can take it from there.

Comment 25

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #24)
> Comment on attachment 8356773 [details] [diff] [review]
> Part 3. (WIP) Plumbing for filtration, and bundle usage detection.
> 
> Review of attachment 8356773 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: media/mtransport/transportflow.cpp
> @@ +200,5 @@
> >  }
> >  
> > +bool TransportFlow::Contains(TransportLayer *layer) const {
> > +  if (layers_) {
> > +    for (auto l = layers_->begin(); l != layers_->end(); ++l) {
> 
> Yeah, that would be nice.
> 
> I don't think we have full C++11 support across the platforms yet; in
> particular, I think our Windows build relies on a C++98 compiler. The C++11
> things we can use right now are those that can be backported through the
> creative use of libraries and macros.
>

   We support auto, and I've used it on several other patches with no problems.
 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1650,5 @@
> >  
> >      case SETREMOTEDESCSUCCESS:
> > +      // TODO([e-mail]): If we are doing bundle, grab remote and
> > +      // local SDP, build the filters, and toss them to PeerConnectionMedia on
> > +      // the STS thread. These filters will be applied to the MediaPipelines.
> 
> This is not where that should happen. The SDP at this point is a text blob
> for the javascript. Our point of contact between SIPCC and the
> PeerConnectionImpl/Media/etc is in the VcmSIPCCBinding, not here.
> 

Yeah, I goofed and forgot to remove this comment.

> Here's how this needs to work: down in lsm.c, we'll add something that calls
> into a new VCM function (e.g., vcmRxAddFilters), with a semantic indication
> of the parameters that should be used to build the filters. If this can be
> done piecemeal on a per-media-line basis, then we put it in lsm_open_rx() or
> lsm_rx_start(); otherwise, we need to back up a level and add it in where
> lsm_rx_start() is called from.
> 
> Basically, let me know what information you need, and I'll add the code to
> lsm.c that calls the new function that you will define in
> VcmSIPCCBinding.cpp. Then, your code can take it from there.

The "real" placeholder stuff is in VcmSIPCCBinding already.

Comment 26

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Incorporate some feedback, fill in more placeholder code.

Comment 27

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

A little more work.

Comment 28

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Remove automatic whitespace change that snuck in.

Comment 29

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Implement some RTCP filtering logic, plus tests.

Comment 30

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Make a truncation test a little better.

Comment 31

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Update to use new API in Part 2.1.

Comment 32

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Cut down on some code-duplication.

Comment 33

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Bring up to date with latest Part 2.1

Comment 34

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Remove some stuff that we aren't going to be using.

Comment 35

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Update remaining test cases to perform the more specific checks.

Comment 36

[Adam Roach [:abr]]

Comment on attachment 8358047 [details] [diff] [review]
Part 2.1. RTCP filtering logic.

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

In general, the hardcoded integers make things hard to read. Please replace them with class-scoped constants.

The logic looks good in here, at least within the constraints of what I understand about RTCP. I've noted some pretty big readability and maintainability nits below, which I trust will be addressed prior to landing.

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ +86,5 @@
> +  if (len < 4) {
> +    return FAIL;
> +  }
> +
> +  uint8_t payload_type = data[1];

s/1/RTCP_PT_OFFSET/

@@ +89,5 @@
> +
> +  uint8_t payload_type = data[1];
> +
> +  switch (payload_type) {
> +    case 200: // Sender report

s/200/RTCP_SENDER_REPORT/

@@ +90,5 @@
> +  uint8_t payload_type = data[1];
> +
> +  switch (payload_type) {
> +    case 200: // Sender report
> +      return CheckRtcpReport(data, len, 7*4) ? PASS : FAIL;

s/7*4/RTCP_SENDER_REPORT_RR_OFFSET/

@@ +92,5 @@
> +  switch (payload_type) {
> +    case 200: // Sender report
> +      return CheckRtcpReport(data, len, 7*4) ? PASS : FAIL;
> +    case 201: // Receiver report
> +      return CheckRtcpReport(data, len, 2*4) ? PASS : FAIL;

You get the idea.

@@ +131,5 @@
> +
> +bool MediaPipelineFilter::CheckRtcpReport(const unsigned char* data,
> +                                          size_t len,
> +                                          size_t first_rr_offset) const {
> +  bool remote_ssrc_matched = CheckRtcpSsrc(data, len, 4, MAYBE_REMOTE_SSRC);

s/4/RTCP_SRC_OFFSET/

@@ +142,5 @@
> +  bool ssrcs_must_match = remote_ssrc_matched;
> +  bool ssrcs_must_not_match = false;
> +
> +  for (uint8_t rr_num = 0; rr_num < rr_count; ++rr_num) {
> +    size_t ssrc_offset = first_rr_offset + rr_num*6*4;

s/6*4/RTCP_RR_SIZE/

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +374,5 @@
>    ASSERT_FALSE(Filter(filter, 0, 556, 110));
>  }
>  
> +#define REPORT_FRAGMENT(ssrc) \
> +0,0,0,ssrc, \

s/0,0,0,ssrc,/SSRC(ssrc),/

@@ +382,5 @@
> +0,0,0,0, \
> +0,0,0,0
> +
> +#define SSRC(ssrc) \
> +0,0,0,ssrc

Someone is going to come along later and botch this up.

((ssrc) >> 24) & 0xFF, ((ssrc) >> 16) & 0xFF, ((ssrc) >> 8) & 0xFF, (ssrc) & 0xFF

@@ +387,5 @@
> +
> +#define RTCP_TYPEINFO(num_rrs, type, size) \
> +0x80 + num_rrs, type, 0, size
> +
> +const unsigned char rtcp_rr0[] = {

Rename to include "16" in the name -- I propose something like "rtcp_ssrc_16_no_rr". It makes the tests much easier to read. The same goes for subsequent names (e.g. rtcp_ssrc_16_rr_17_18)

@@ +419,5 @@
> +REPORT_FRAGMENT(18)};
> +
> +const unsigned char unknown_type[] = {
> +RTCP_TYPEINFO(1, 222, 0)
> +};

Lines 377 through 423 need to be indented in a sensible fashion.

#define continuation lines need to be indented.

Array initialization should be indented, with closing brace on its own line:

const unsigned char rtcp_rr1[] = {
  RTCP_TYPEINFO(1, 201, 7), // v2, one rr, type 201, size 7 words
  SSRC(16),
  REPORT_FRAGMENT(17
};


Also, use the constant names I suggest from the class rather than 200 and 201.

@@ +573,1 @@
>  }

Add at least one test that works through all four bytes of an SSRC (e.g., has an SSRC with a unique value in each byte, such as 0x0A1B2C3D) to make sure that we catch any errors in building an SSRC from its constituent bytes.

Comment 37

[Adam Roach [:abr]]

Comment on attachment 8358047 [details] [diff] [review]
Part 2.1. RTCP filtering logic.

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

::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
@@ +94,5 @@
> +      return CheckRtcpReport(data, len, 7*4) ? PASS : FAIL;
> +    case 201: // Receiver report
> +      return CheckRtcpReport(data, len, 2*4) ? PASS : FAIL;
> +    default:
> +      return UNSUPPORTED;

On reading through how you're using "UNSUPPORTED" in the part 3 patch, I think we really want to do a PASS/FAIL here based on the value of the SSRC that is in bytes 4-8 of (AFAICT) every RTCP report.

Comment 38

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #37)
> Comment on attachment 8358047 [details] [diff] [review]
> Part 2.1. RTCP filtering logic.
> 
> Review of attachment 8358047 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipelineFilter.cpp
> @@ +94,5 @@
> > +      return CheckRtcpReport(data, len, 7*4) ? PASS : FAIL;
> > +    case 201: // Receiver report
> > +      return CheckRtcpReport(data, len, 2*4) ? PASS : FAIL;
> > +    default:
> > +      return UNSUPPORTED;
> 
> On reading through how you're using "UNSUPPORTED" in the part 3 patch, I
> think we really want to do a PASS/FAIL here based on the value of the SSRC
> that is in bytes 4-8 of (AFAICT) every RTCP report.

   Not quite; the thing in 4-8 might be a CSRC, and could be local or remote, we just don't know. We also do not have any way of testing for consistency, like we do with the other types. There is sufficient uncertainty that I'm inclined to just say let webrtc.org handle it.

Comment 39

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.

Fix nits.

Comment 40

[Byron Campen [:bwc]]

Comment on attachment 8359916 [details] [diff] [review]
Part 2.1. RTCP filtering logic.

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

Carry forward r+ from abr.

Comment 41

[Byron Campen [:bwc]]

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

Comment 42

[Adam Roach [:abr]]

Comment on attachment 8357995 [details] [diff] [review]
Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

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

This looks to be headed in the right direction. Some of these "TODO" comments appear to be "open issues before I consider this patch finished," while others are "things that will probably still be open issues when I land this patch." For the latter category, please make sure to reference a bug that tracks that TODO as an open issue.

MediaPipeline.h:129: Line is 82 characters long; please wrap to 80

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2266,5 @@
>    }
>  
> +  nsRefPtr<sipcc::RemoteSourceStreamInfo> rx_stream =
> +    pc.impl()->media()->GetRemoteStream(pc_stream_id);
> +  // Do we need an assert, or do we expect this to happen on sendonly streams?

I wouldn't assert, but a CSFLogWarning is probably in order.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +152,5 @@
> +  // Dummy variable to cause init to happen only on first call
> +  static bool dummy = MakeRtpTypeToStringArray(array);
> +  (void)dummy;
> +  return array[type];
> +}

This seems a bit rube-goldbergesque. Why not just define a static array?

@@ +414,4 @@
>    if (direction_ == TRANSMIT) {
>      // Discard any media that is being transmitted to us
>      // This will be unnecessary when we have SSRC filtering.
>      return;

The preceding comment would imply that this patch should remove this functionality, right?

@@ +437,5 @@
> +    return;
> +  }
> +
> +  webrtc::RTPHeader header;
> +  if (!rtp_parser_->Parse(data, len, &header)) {

I wouldn't bother parsing unless filter_ is true.

@@ +444,5 @@
> +  }
> +
> +  if (filter_ && !filter_->Filter(header)) {
> +    // TODO([e-mail]): Would it be worthwhile to try to buffer
> +    // packets if we know our filter is incomplete?

No.

@@ +535,5 @@
> +    // Ensures accesses to RC and PT are valid. More checking later.
> +    return;
> +  }
> +
> +  // Filter out everything but RTP

RTP / RTCP

@@ +580,5 @@
>  }
>  
>  bool MediaPipeline::IsRtp(const unsigned char *data, size_t len) {
> +  // TODO([e-mail]): Now that we have rtp_parser_ for handling
> +  // filtering, do we just want to use that here? Or is there custom logic here?

No, this is different. Rather than checking the version byte for plausibility, this code is checking the PT to determine whether the incoming packet is an RTCP PT that we support. The code below is actually pretty unfortunate, since there is confusion about some of these types -- e.g., RTCP PT 192 is RTP PT 64 with the marker bit set, and the current IANA registry only sets aside RTP PTs 72 - 76 for RTCP disambiguation. Note that RFC 5761 is actually clear on this point (reserving PTs 64 - 95), which makes the following code more conservative than is probably strictly necessary.

In any case, I don't think any change is indicated for *this* patch, unless you find a place where you have a packet, don't know whether it's RTP or RTCP, and need to process it differently based on the answer to that question.

@@ +670,5 @@
>  
>    return NS_OK;
>  }
>  
> +void MediaPipeline::DisconnectTransport_s(TransportInfo& info) {

ASSERT_ON_THREAD(sts_thread_);

@@ +680,5 @@
> +    // TransportLayer we registered with earlier.
> +  }
> +}
> +
> +nsresult MediaPipeline::ConnectTransport_s(TransportInfo& info) {

ASSERT_ON_THREAD(sts_thread_);

@@ +704,5 @@
> +  }
> +  return NS_OK;
> +}
> +
> +MediaPipeline::TransportInfo* MediaPipeline::GetTransportInfo_s(

ASSERT_ON_THREAD(sts_thread_);

@@ +727,5 @@
> +
> +  return nullptr;
> +}
> +
> +void MediaPipeline::SetUsingBundle_s(bool decision) {

ASSERT_ON_THREAD(sts_thread_);

@@ +762,5 @@
> +    possible_bundle_rtcp_ = nullptr;
> +  }
> +}
> +
> +void MediaPipeline::UpdateFilterFromRemoteDescription_s(

ASSERT_ON_THREAD(sts_thread_);

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +258,3 @@
>    // Written only on STS thread. May be read on other
>    // threads but since there is no mutex, the values
>    // will only be approximate.

Yikes! I know it's not the subject of this patch, but the comment is off-base; cf. http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Please adjust to read "// Only safe to access from STS thread."

@@ +258,4 @@
>    // Written only on STS thread. May be read on other
>    // threads but since there is no mutex, the values
>    // will only be approximate.
> +  // Build into TransportInfo?

Probably not: you want to count packets for this pipeline, not the aggregation of packets for all the pipelines associated with a transport. Right?

@@ +269,5 @@
>    // Written on Init. Read on STS thread.
>    std::string pc_;
>    std::string description_;
>  
> +  // Written on Init. Read on STS thread.

That's not quite true; these get updated on the STS thread as well. I think the point to make here is that these are accessed from the STS thread (only).

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +504,5 @@
>  }
>  
> +void RemoteSourceStreamInfo::UpdateFilterFromRemoteDescription_s(
> +    int aTrack,
> +    nsAutoPtr<mozilla::MediaPipelineFilter> aFilter) {

ASSERT_ON_THREAD(mParent->GetSTSThread());

@@ +508,5 @@
> +    nsAutoPtr<mozilla::MediaPipelineFilter> aFilter) {
> +  auto it = mPipelines.find(aTrack);
> +  if (it != mPipelines.end()) {
> +    it->second->UpdateFilterFromRemoteDescription_s(aFilter);
> +  }

else {
  CSFLogInfo(logTag, "Could not locate track %d to update filter", aTrack);
}

@@ +511,5 @@
> +    it->second->UpdateFilterFromRemoteDescription_s(aFilter);
> +  }
> +}
> +
> +void RemoteSourceStreamInfo::SetUsingBundle_s(int aTrack, bool decision) {

ASSERT_ON_THREAD(mParent->GetSTSThread());

@@ +515,5 @@
> +void RemoteSourceStreamInfo::SetUsingBundle_s(int aTrack, bool decision) {
> +  auto it = mPipelines.find(aTrack);
> +  if (it != mPipelines.end()) {
> +    it->second->SetUsingBundle_s(decision);
> +  }

else {
  CSFLogInfo(logTag, "Could not locate track %d to set bundle flag", aTrack);
}

Comment 43

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #42)
> Comment on attachment 8357995 [details] [diff] [review]
> Part 3. (WIP) Plumbing for filtration, and bundle usage detection.
> 
> Review of attachment 8357995 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +152,5 @@
> > +  // Dummy variable to cause init to happen only on first call
> > +  static bool dummy = MakeRtpTypeToStringArray(array);
> > +  (void)dummy;
> > +  return array[type];
> > +}
> 
> This seems a bit rube-goldbergesque. Why not just define a static array?
>

   Using a static array would mean we'd have no chance of detecting a change in the enum. The present code at least ensures that the right values end up at the right offsets, although there are still things that can break. I can add a dummy MAX_RTP_TYPE and put in a static assert that the array's size equals the value of MAX_RTP_TYPE, which I think gets us good enough compile-time checking.
 
> @@ +414,4 @@
> >    if (direction_ == TRANSMIT) {
> >      // Discard any media that is being transmitted to us
> >      // This will be unnecessary when we have SSRC filtering.
> >      return;
> 
> The preceding comment would imply that this patch should remove this
> functionality, right?
> 

   We only filter when bundle is in play, so no. We still need the check, just not the comment.

> @@ +437,5 @@
> > +    return;
> > +  }
> > +
> > +  webrtc::RTPHeader header;
> > +  if (!rtp_parser_->Parse(data, len, &header)) {
> 
> I wouldn't bother parsing unless filter_ is true.

   Yeah, I went back and forth on this trying to figure out if it would help us filter out non-RTP/RTCP, but it doesn't seem to.

> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
> @@ +258,3 @@
> >    // Written only on STS thread. May be read on other
> >    // threads but since there is no mutex, the values
> >    // will only be approximate.
> 
> Yikes! I know it's not the subject of this patch, but the comment is
> off-base; cf.
> http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-
> could-possibly-go-wrong
> 
> Please adjust to read "// Only safe to access from STS thread."
> 

   Yeah, I was wondering about that too. Can change.

> @@ +258,4 @@
> >    // Written only on STS thread. May be read on other
> >    // threads but since there is no mutex, the values
> >    // will only be approximate.
> > +  // Build into TransportInfo?
> 
> Probably not: you want to count packets for this pipeline, not the
> aggregation of packets for all the pipelines associated with a transport.
> Right?

   TransportInfo aren't shared between the pipelines. The place where this might yield weird results is with RTCP types other than sender/receiver reports. It is possible for us to receive something like SDES on the potential bundle transport, let it through (because we don't have parse code for SDES) and increment the counter, and then learn that we aren't doing bundle. Should that RTCP packet that wasn't actually ours count?

Comment 44

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Incorporate some feedback, and some stuff I noticed.

Comment 45

[Byron Campen [:bwc]]

Attachment: Part 2.2. Compensate for some build system gotchas.

Fix build bustage in 2.1 due to unified build.

Comment 46

[Adam Roach [:abr]]

Comment on attachment 8360122 [details] [diff] [review]
Part 2.2. Compensate for some build system gotchas.

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

Looks reasonable.

Comment 47

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Now that 906990 has landed, use MediaPipeline::level() to find the receive pipeline when the remote description tells us whether we're using bundle or not. Previously, we were assuming that the stream id was the same for receive and send on the same m-line, which is not necessarily the case.

Comment 48

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Beginning to put together some test code for the bundle logic in MediaPipeline.

Comment 49

[Byron Campen [:bwc]]

Attachment: Part 4. Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Fix some bugs that are getting in the way of testing the bundle logic in MediaPipeline.

Comment 50

[Byron Campen [:bwc]]

Attachment: Part 4. Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Fix a few lifecycle bugs.

Comment 51

[Byron Campen [:bwc]]

Attachment: Part 5. (WIP) More detailed test-cases.

A little work on the test cases. More work is necessary.

Comment 52

[Byron Campen [:bwc]]

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

Comment 53

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Unrot

Comment 54

[Byron Campen [:bwc]]

Attachment: Part 3. (WIP) Plumbing for filtration, and bundle usage detection.

Disable a check that was not needed at all.

Comment 55

[Byron Campen [:bwc]]

Attachment: Part 5. (WIP) More detailed test-cases.

Try to tighten up these test cases. Still not quite able to get RTCP counts to line up every time.

Comment 56

[Byron Campen [:bwc]]

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

Comment 57

[Byron Campen [:bwc]]

Attachment: Part 3. Plumbing for filtration, and bundle usage detection.

Went through the patch and applied a little polish. Should be close to review-ready, but further enhancements/fixes to the unit-tests are required before I am comfortable with that.

Comment 58

[Byron Campen [:bwc]]

Attachment: Part 4. Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Unrot

Comment 59

[Byron Campen [:bwc]]

Attachment: Part 5. (WIP) More detailed test-cases.

Unrot, and make a test more reliable. Need to investigate the root cause of the inconsistency in how much RTCP is sent.

Comment 60

[Byron Campen [:bwc]]

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

Comment 61

[Byron Campen [:bwc]]

Attachment: Part 5. (WIP) More detailed test-cases.

Use some API that jib landed recently.

Comment 62

[Byron Campen [:bwc]]

Attachment: Part 5. (WIP) More detailed test-cases.

Making a note.

Comment 63

[Byron Campen [:bwc]]

Attachment: Part 3. Plumbing for filtration, and bundle usage detection.

Unrot.

Comment 64

[Byron Campen [:bwc]]

Attachment: Part 5. More detailed test-cases.

A little cleanup.

Comment 65

[Adam Roach [:abr]]

Attachment: Interdiff for Part 3 (deltas from abr's last feedback)

Interdiff for part 3

Comment 66

[Adam Roach [:abr]]

Comment on attachment 8369696 [details] [diff] [review]
Interdiff for Part 3 (deltas from abr's last feedback)

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

Review comments for the "Part 3" patch (I just reviewed this interdiff, since the other changes were covered in my earlier feedback).

::: mozilla-inbound-interdiff-before/media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +431,5 @@
>      // have just received traffic that clears this up.
>      // Don't let our filter prevent us from noticing this, if the filter is
>      // incomplete (ie; no SSRCs in remote SDP, or no remote SDP at all).
>      SetUsingBundle_s(false);
> +    MOZ_MTLOG(ML_INFO, "Ruled out the possibility that we're receiving bundle");

Add a bug number?

@@ +461,5 @@
>    if (possible_bundle_rtp_) {
>      // Just got traffic that passed our filter on the potential bundle
>      // transport. Must be doing bundle.
>      SetUsingBundle_s(true);
> +    MOZ_MTLOG(ML_INFO, "Confirmed the possibility that we're receiving bundle");

... receiving bundle for " << description_);

@@ +748,5 @@
>    }
>  
>    if (direction_ == RECEIVE) {
>      if (decision) {
> +      MOZ_MTLOG(ML_INFO, "Non-master pipeline confirmed bundle");

<< description_

@@ +756,5 @@
>        DisconnectTransport_s(rtcp_);
>        rtp_ = *possible_bundle_rtp_;
>        rtcp_ = *possible_bundle_rtcp_;
>      } else {
> +      MOZ_MTLOG(ML_INFO, "Non-master pipeline confirmed no bundle");

<< description_

::: mozilla-inbound-interdiff-before/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1324,5 @@
>        streams.push_back(temp);
>      } else {
>         CSFLogError(logTag, "Failed to get NrIceMediaStream for level %u "
>                             "in %s:  %s",
> +                           (unsigned int)level, __FUNCTION__, mHandle.c_str());

Wouldn't it be cleaner to change the %u to %zu rather than casting the variable?

If you're keeping the cast: static_cast<unsigned int>(level) -- we never want to see c-style casts in C++ code.

::: mozilla-inbound-interdiff-before/media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +401,5 @@
> +{
> +  ASSERT_ON_THREAD(mMainThread);
> +  for (size_t i = 0; i < mRemoteSourceStreams.Length(); ++i) {
> +    if (mRemoteSourceStreams[i]->SetUsingBundle_m(level, decision)) {
> +      return true;

Perhaps add a comment that we've found the pipeline corresponding to the indicated level, so our job is done.

@@ +564,5 @@
> +                  ),
> +                  NS_DISPATCH_NORMAL);
> +    return true;
> +  } else {
> +    CSFLogWarn(logTag, "Could not locate level %d to update filter", aLevel);

Given that we're potentially calling this multiple times from PCMedia, and given that users tend to freak out more than a little bit when they see something that looks like an error in their logs, I would suggest moving this log message up to PeerConnectionMedia::UpdateFilterFromRemoteDescription_m()

@@ +593,5 @@
> +                  NS_DISPATCH_NORMAL);
> +    return true;
> +  } else {
> +    CSFLogWarn(logTag, "Could not locate level %d to set bundle flag",
> +                       aLevel);

Same comment as above.

Also: "...set bundle flag to %s", aLevel, decision ? "true" : "false");

Comment 67

[Adam Roach [:abr]]

Comment on attachment 8366707 [details] [diff] [review]
Part 3. Plumbing for filtration, and bundle usage detection.

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

r+, with nits from the interdiff review from comment 66.

Comment 68

[Byron Campen [:bwc]]

(In reply to Adam Roach [:abr] from comment #66)
> Comment on attachment 8369696 [details] [diff] [review]
> Interdiff for Part 3 (deltas from abr's last feedback)
> 
> Review of attachment 8369696 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Review comments for the "Part 3" patch (I just reviewed this interdiff,
> since the other changes were covered in my earlier feedback).
> 
> :::
> mozilla-inbound-interdiff-before/media/webrtc/signaling/src/mediapipeline/
> MediaPipeline.cpp
> @@ +431,5 @@
> >      // have just received traffic that clears this up.
> >      // Don't let our filter prevent us from noticing this, if the filter is
> >      // incomplete (ie; no SSRCs in remote SDP, or no remote SDP at all).
> >      SetUsingBundle_s(false);
> > +    MOZ_MTLOG(ML_INFO, "Ruled out the possibility that we're receiving bundle");
> 
> Add a bug number?
> 

   I dunno; do you think this might be worth doing?

> @@ +1324,5 @@
> >        streams.push_back(temp);
> >      } else {
> >         CSFLogError(logTag, "Failed to get NrIceMediaStream for level %u "
> >                             "in %s:  %s",
> > +                           (unsigned int)level, __FUNCTION__, mHandle.c_str());
> 
> Wouldn't it be cleaner to change the %u to %zu rather than casting the
> variable?
> 
> If you're keeping the cast: static_cast<unsigned int>(level) -- we never
> want to see c-style casts in C++ code.
> 

   At this point, my inclination with printf and friends is to always cast to exactly what the format string expects, even if the type _already_ matches, just to future-proof things. But, %zu would probably be better here regardless.

> :::
> mozilla-inbound-interdiff-before/media/webrtc/signaling/src/peerconnection/
> PeerConnectionMedia.cpp
> @@ +401,5 @@
> > +{
> > +  ASSERT_ON_THREAD(mMainThread);
> > +  for (size_t i = 0; i < mRemoteSourceStreams.Length(); ++i) {
> > +    if (mRemoteSourceStreams[i]->SetUsingBundle_m(level, decision)) {
> > +      return true;
> 
> Perhaps add a comment that we've found the pipeline corresponding to the
> indicated level, so our job is done.
> 

   Sure.

> @@ +564,5 @@
> > +                  ),
> > +                  NS_DISPATCH_NORMAL);
> > +    return true;
> > +  } else {
> > +    CSFLogWarn(logTag, "Could not locate level %d to update filter", aLevel);
> 
> Given that we're potentially calling this multiple times from PCMedia, and
> given that users tend to freak out more than a little bit when they see
> something that looks like an error in their logs, I would suggest moving
> this log message up to
> PeerConnectionMedia::UpdateFilterFromRemoteDescription_m()
> 
> @@ +593,5 @@
> > +                  NS_DISPATCH_NORMAL);
> > +    return true;
> > +  } else {
> > +    CSFLogWarn(logTag, "Could not locate level %d to set bundle flag",
> > +                       aLevel);
> 
> Same comment as above.
> 

   Gack, that was what I intended to do. Will fix.

> Also: "...set bundle flag to %s", aLevel, decision ? "true" : "false");

   Sure.

Comment 69

[Byron Campen [:bwc]]

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

Comment 70

[Byron Campen [:bwc]]

Attachment: Part 3. Plumbing for filtration, and bundle usage detection.

Incorporate nits

Comment 71

[Byron Campen [:bwc]]

Comment on attachment 8372364 [details] [diff] [review]
Part 3. Plumbing for filtration, and bundle usage detection.

Carry forward r+ from abr.

Comment 72

[Ethan Hugg [:ehugg]]

Comment on attachment 8365404 [details] [diff] [review]
Part 4. Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

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

Looks good to me.

Comment 73

[Ethan Hugg [:ehugg]]

Comment on attachment 8366722 [details] [diff] [review]
Part 5. More detailed test-cases.

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

This looks fine as well.

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +445,5 @@
>      p1_.Stop();
>      p2_.Stop();
> +
> +    // wait for any packets in flight to arrive
> +    PR_Sleep(100);

You changed the other hardcoded sleep times into params, do you want to do that here as well, or will 100 always be best?

Comment 74

[Byron Campen [:bwc]]

(In reply to Ethan Hugg [:ehugg] from comment #73)
> Comment on attachment 8366722 [details] [diff] [review]
> Part 5. More detailed test-cases.
> 
> Review of attachment 8366722 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine as well.
> 
> ::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
> @@ +445,5 @@
> >      p1_.Stop();
> >      p2_.Stop();
> > +
> > +    // wait for any packets in flight to arrive
> > +    PR_Sleep(100);
> 
> You changed the other hardcoded sleep times into params, do you want to do
> that here as well, or will 100 always be best?

   It doesn't seem all that likely that we'll need to vary this around test-by-test, and it is pretty easy to change if we find ourselves in that situation, so I didn't feel like adding the extra code.

Comment 75

[Byron Campen [:bwc]]

Ok, time to unrot all this stuff.

Comment 76

[Byron Campen [:bwc]]

Attachment: Part 2. Implementation of the filtering logic itself, plus a unit-test.

Unrot.

Comment 77

[Byron Campen [:bwc]]

Comment on attachment 8379325 [details] [diff] [review]
Part 2. Implementation of the filtering logic itself, plus a unit-test.

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

Carry forward r=abr

Comment 78

[Byron Campen [:bwc]]

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

Comment 79

[Byron Campen [:bwc]]

Needinfo myself to check back on try push.

Comment 80

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7eac2b16ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dea344fa608
https://hg.mozilla.org/integration/mozilla-inbound/rev/53d8b186e574
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5334aea6433
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8083d830fa6
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d1f70b91712

Comment 81

[Adam Roach [:abr]]

Backed out for signaling_unittests failures (http://buildbot.rtfm.com:8010/builders/ubuntu12_04_64/builds/335/steps/WebRTC%3A%20Signaling/logs/stdio):

https://hg.mozilla.org/integration/mozilla-inbound/rev/2945a260ef00
https://hg.mozilla.org/integration/mozilla-inbound/rev/18849209753a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b0258a49e8d
https://hg.mozilla.org/integration/mozilla-inbound/rev/1582d179f029
https://hg.mozilla.org/integration/mozilla-inbound/rev/098be567e0d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/69602085b4f0

Comment 82

[Byron Campen [:bwc]]

Attachment: Part 6: Move RTCP handling back to the transmit pipeline.

This seems to do the trick. Have run some of the usual tests, and that TokBox test that fails if RTCP isn't flowing all the way down to webrtc.org code.

Comment 83

[Byron Campen [:bwc]]

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

Comment 84

[Byron Campen [:bwc]]

Attachment: Part 6: Move RTCP handling back to the transmit pipeline.

Simplify some code, and add a comment.

Comment 85

[Byron Campen [:bwc]]

Comment on attachment 8381510 [details] [diff] [review]
Part 6: Move RTCP handling back to the transmit pipeline.

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

Not that the patch for VcmSIPCCBinding.cpp is a little sad; I moved the small block of code that handles bundle updates, but diff interpreted that as a move of a rather large block of code.

Comment 86

[Byron Campen [:bwc]]

Comment on attachment 8381510 [details] [diff] [review]
Part 6: Move RTCP handling back to the transmit pipeline.

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

Adding r? jesup to make sure things look like they're going to the right places now.

Comment 87

[Ethan Hugg [:ehugg]]

Comment on attachment 8381510 [details] [diff] [review]
Part 6: Move RTCP handling back to the transmit pipeline.

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

Looks good to me.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +249,1 @@
>      MOZ_MTLOG(ML_INFO, "Listening for " << ToString(info.type_)

This now appears to be indented further than it should be.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +462,5 @@
> +                  NS_DISPATCH_NORMAL);
> +    return true;
> +  } else {
> +    CSFLogWarn(logTag, "Could not locate level %d to update filter",
> +        static_cast<int>(level));

Wouldn't hurt to get rid of this static cast since level is already an int.

Comment 88

[Byron Campen [:bwc]]

(In reply to Ethan Hugg [:ehugg] from comment #87)
> Comment on attachment 8381510 [details] [diff] [review]
> Part 6: Move RTCP handling back to the transmit pipeline.
> 
> Review of attachment 8381510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.
> 
> ::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
> @@ +249,1 @@
> >      MOZ_MTLOG(ML_INFO, "Listening for " << ToString(info.type_)
> 
> This now appears to be indented further than it should be.
> 

   Will fix once jesup weighs in.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +462,5 @@
> > +                  NS_DISPATCH_NORMAL);
> > +    return true;
> > +  } else {
> > +    CSFLogWarn(logTag, "Could not locate level %d to update filter",
> > +        static_cast<int>(level));
> 
> Wouldn't hurt to get rid of this static cast since level is already an int.

  This is just idiot-proofing in case the type for level changes down the road; I've decided I am tired of being bitten by printf integer width gotchas.

Comment 89

[Byron Campen [:bwc]]

Attachment: Part 2. Implementation of the filtering logic itself, plus a unit-test.Bug 786234 - Part 2: Implementation of the filtering logic itself, plus a unit-test.

Fix commit message.

Comment 90

[Byron Campen [:bwc]]

Attachment: Part 2.1. RTCP filtering logic.Bug 786234 - Part 2.1: RTCP filtering logic.

Fix commit message.

Comment 91

[Byron Campen [:bwc]]

Attachment: Part 2.2. Compensate for some build system gotchas.Bug 786234 - Part 2.2: Compensate for some build system gotchas.

Fix commit message.

Comment 92

[Byron Campen [:bwc]]

Attachment: Part 3. Plumbing for filtration, and bundle usage detection.Bug 786234 - Part 3: Plumbing for filtration, and bundle usage detection.

Fix commit message.

Comment 93

[Byron Campen [:bwc]]

Attachment: Part 4. Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Fix commit message.

Comment 94

[Byron Campen [:bwc]]

Attachment: Part 5. More detailed test-cases.Bug 786234 - Part 5: More detailed test-cases.

Fix commit message.

Comment 95

[Byron Campen [:bwc]]

Attachment: Part 2: Implementation of the filtering logic itself, plus a unit-test.

Trying this again.

Comment 96

[Byron Campen [:bwc]]

Attachment: Part 2.1: RTCP filtering logic.

Trying this again.

Comment 97

[Byron Campen [:bwc]]

Attachment: Part 2.2: Compensate for some build system gotchas.

Trying this again.

Comment 98

[Byron Campen [:bwc]]

Attachment: Part 3: Plumbing for filtration, and bundle usage detection.

Trying this again.

Comment 99

[Byron Campen [:bwc]]

Attachment: Part 4: Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Trying this again.

Comment 100

[Byron Campen [:bwc]]

Attachment: Part 5: More detailed test-cases.

Trying this again.

Comment 101

[Byron Campen [:bwc]]

Comment on attachment 8381608 [details] [diff] [review]
Part 2: Implementation of the filtering logic itself, plus a unit-test.

Carry forward r=abr

Comment 102

[Byron Campen [:bwc]]

Comment on attachment 8381610 [details] [diff] [review]
Part 2.1: RTCP filtering logic.

Carry forward r=abr

Comment 103

[Byron Campen [:bwc]]

Comment on attachment 8381611 [details] [diff] [review]
Part 2.2: Compensate for some build system gotchas.

Carry forward r=abr

Comment 104

[Byron Campen [:bwc]]

Comment on attachment 8381612 [details] [diff] [review]
Part 3: Plumbing for filtration, and bundle usage detection.

Carry forward r=abr

Comment 105

[Byron Campen [:bwc]]

Comment on attachment 8381613 [details] [diff] [review]
Part 4: Fix a bug in mediapipeline_unittest where RTP packets weremashed together into a single buffer five at a time, preventing the receive

Carry forward r=ehugg

Comment 106

[Byron Campen [:bwc]]

Comment on attachment 8381614 [details] [diff] [review]
Part 5: More detailed test-cases.

Carry forward r=ehugg

Comment 107

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

Comment on attachment 8381510 [details] [diff] [review]
Part 6: Move RTCP handling back to the transmit pipeline.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +417,5 @@
> +  RefPtr<mozilla::MediaPipeline> receive,
> +  RefPtr<mozilla::MediaPipeline> transmit,
> +  nsAutoPtr<mozilla::MediaPipelineFilter> filter) {
> +
> +  // Update filter, and make a copy of the final version.

Assert we're on STS?  or at least not on MainThread?

@@ +579,3 @@
>    ASSERT_ON_THREAD(mParent->GetMainThread());
> +
> +  // Refuse to hand out references if we're tearing down.

Expand on why this would be dangerous to change (from above)

Comment 108

[Byron Campen [:bwc]]

Attachment: Part 2.2: Compensate for some build system gotchas.

Unrot

Comment 109

[Byron Campen [:bwc]]

Attachment: Part 3: Plumbing for filtration, and bundle usage detection.

Unrot

Comment 110

[Byron Campen [:bwc]]

Comment on attachment 8386916 [details] [diff] [review]
Part 2.2: Compensate for some build system gotchas.

Carry forward r+

Comment 111

[Byron Campen [:bwc]]

Comment on attachment 8386925 [details] [diff] [review]
Part 3: Plumbing for filtration, and bundle usage detection.

Carry forward r+

Comment 112

[Byron Campen [:bwc]]

(In reply to Randell Jesup [:jesup] from comment #107)
> Comment on attachment 8381510 [details] [diff] [review]
> Part 6: Move RTCP handling back to the transmit pipeline.
> 
> Review of attachment 8381510 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
> @@ +417,5 @@
> > +  RefPtr<mozilla::MediaPipeline> receive,
> > +  RefPtr<mozilla::MediaPipeline> transmit,
> > +  nsAutoPtr<mozilla::MediaPipelineFilter> filter) {
> > +
> > +  // Update filter, and make a copy of the final version.
> 
> Assert we're on STS?  or at least not on MainThread?
> 

   We're not in a member function at this point, and MediaPipeline will check.


> @@ +579,3 @@
> >    ASSERT_ON_THREAD(mParent->GetMainThread());
> > +
> > +  // Refuse to hand out references if we're tearing down.
> 
> Expand on why this would be dangerous to change (from above)

   Ok.

Comment 113

[Byron Campen [:bwc]]

Attachment: Part 6: Move RTCP handling back to the transmit pipeline.

Elaborate on a comment

Comment 114

[Byron Campen [:bwc]]

Comment on attachment 8386932 [details] [diff] [review]
Part 6: Move RTCP handling back to the transmit pipeline.

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

Carry forward r+

Comment 115

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/868e93e74341
https://hg.mozilla.org/integration/mozilla-inbound/rev/33039b7d72e6
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdff005cbe3
https://hg.mozilla.org/integration/mozilla-inbound/rev/afc7283f00e2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e170aedca6a6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5284a081ee88
https://hg.mozilla.org/integration/mozilla-inbound/rev/6014b4fe9e16

Comment 116

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/868e93e74341
https://hg.mozilla.org/mozilla-central/rev/33039b7d72e6
https://hg.mozilla.org/mozilla-central/rev/ecdff005cbe3
https://hg.mozilla.org/mozilla-central/rev/afc7283f00e2
https://hg.mozilla.org/mozilla-central/rev/e170aedca6a6
https://hg.mozilla.org/mozilla-central/rev/5284a081ee88
https://hg.mozilla.org/mozilla-central/rev/6014b4fe9e16