Closed Bug 906990 Opened 6 years ago Closed 6 years ago

Detailed ICE status reporting to Javascript

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: jesup, Assigned: bwc)

References

(Blocks 1 open bug)

Details

(Whiteboard: [webrtc])

Attachments

(16 files, 113 obsolete files)

12.67 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
16.34 KB, patch
ekr
: review+
abr
: checkin+
Details | Diff | Splinter Review
30.71 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
63.38 KB, patch
ekr
: review+
abr
: checkin+
Details | Diff | Splinter Review
4.73 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
4.59 KB, patch
ekr
: review+
Details | Diff | Splinter Review
10.23 KB, patch
ekr
: review+
abr
: checkin+
Details | Diff | Splinter Review
14.69 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
12.95 KB, patch
jib
: review+
abr
: checkin+
Details | Diff | Splinter Review
23.44 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
22.03 KB, patch
bwc
: review+
abr
: checkin+
Details | Diff | Splinter Review
1.21 KB, patch
ekr
: review+
abr
: checkin+
Details | Diff | Splinter Review
23.08 KB, patch
bwc
: review+
jesup
: checkin+
Details | Diff | Splinter Review
11.11 KB, patch
ekr
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
3.32 KB, patch
bwc
: review+
jib
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
41.17 KB, patch
bwc
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review
Not sure how this is different than bug 906965...
Duplicate of this bug: 906965
The purpose of this is to gather and pass up all the ICE state to JS to allow for debugging of a bad call.  We're going to give this to Byron as his first task.

We're estimating it will take about 8 weeks for him to do it.  Obviously once he gets on board, he'll have his own opinions.  I'm targeting this for Gecko 28, which uplifts Dec 9 -- but I'd prefer to land in mid November so I'm setting Nov 15 as our target deadline.
Assignee: nobody → docfaraday
Blocks: 908923
Target Milestone: --- → mozilla28
Beginning to put together some prototype code. Initial survey indicates that it may be easiest to implement a polling-type interface on the list of candidate pairs, since nICEr does not seem to give us hooks into most events involving candidate pair state changes and such. As long as modifying nICEr is an option, we can also get useful signals for this stuff.
Status: NEW → ASSIGNED
We can modify nICEr.
Attachment #800873 - Attachment is obsolete: true
Comment on attachment 801044 [details] [diff] [review]
Refinement of previous patch; we were scraping the wrong checklist. Needed to export a new symbol added in nICEr. Also, NrIceCandidate cannot be initted via memset. Gave it (and NrIceCandidatePair) a default c'tor.

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

Byron,

A number of comments below.

::: media/mtransport/nricemediastream.cpp
@@ +73,5 @@
>  
>  MOZ_MTLOG_MODULE("mtransport")
>  
> +static bool ToNrIceCandidate(const nr_ice_candidate& candc,
> +                             NrIceCandidate& out) {

You could avoid this change by just having a candidatepair contain
ScopedDeletPtr<NrIceCandidate>.

If you are going to go this way, please use pointers, not references, for out arguments.

@@ +116,5 @@
>  
> +// Make an NrIceCandidate from the candidate |cand|.
> +// This is not a member fxn because we want to hide the
> +// defn of nr_ice_candidate but we pass by reference.
> +static NrIceCandidate* MakeNrIceCandidate(const nr_ice_candidate& candc) {

This doesn't seem like a necessary abstraction.

@@ +262,5 @@
>    RFREE(attrs);
>  }
>  
> +nsresult NrIceMediaStream::GetCandidatePairs(std::vector<NrIceCandidatePair>*
> +                                              outPairs ) const {

Fix indent.

@@ +267,5 @@
> +  if (outPairs == NULL) {
> +    // The contract I am adhering to here is that passing NULL as the
> +    // param causes exactly the same behavior as not passing NULL (same logic,
> +    // same return codes, same side-effects if any). This is more terse than
> +    // sprinkling checks in the below code.

Please make this comment more terse.

@@ +269,5 @@
> +    // param causes exactly the same behavior as not passing NULL (same logic,
> +    // same return codes, same side-effects if any). This is more terse than
> +    // sprinkling checks in the below code.
> +    std::vector<NrIceCandidatePair> dummy;
> +    return GetCandidatePairs(&dummy);

No need for recursion here. Put dummy outside the conditional and set outpairs = &dummy.

That said, this all seems unnecessary. I would just MOZ_ASSERT(outPairs) unless
there is some need for the semantics you propose.

@@ +277,5 @@
> +  outPairs->clear();
> +
> +  // Get the check_list on the peer stream (this is where the check_list
> +  // actually lives, not in stream_)
> +  nr_ice_media_stream* peerStream = NULL;

This code uses nullptr. Fix here and below.

@@ +278,5 @@
> +
> +  // Get the check_list on the peer stream (this is where the check_list
> +  // actually lives, not in stream_)
> +  nr_ice_media_stream* peerStream = NULL;
> +  int res = nr_ice_peer_ctx_find_pstream(ctx_->peer(), stream_, &peerStream);

Convention is "r" not "res"

@@ +281,5 @@
> +  nr_ice_media_stream* peerStream = NULL;
> +  int res = nr_ice_peer_ctx_find_pstream(ctx_->peer(), stream_, &peerStream);
> +  if (peerStream == NULL) {
> +    return NS_ERROR_FAILURE;
> +  }

Please check r not peerStream. The contract of this function is that peerStream will be valid if r == 0.

@@ +284,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // Is it worth trying to guess how long this list might be in order to reserve
> +  // that much space?

No. Remove comment.

@@ +286,5 @@
> +
> +  // Is it worth trying to guess how long this list might be in order to reserve
> +  // that much space?
> +  TAILQ_FOREACH(p1, &peerStream->check_list, entry){
> +    if (p1 != NULL && p1->local != NULL && p1->remote != NULL) {

Is there any way for a value to be on the checklist with p1->remote or p1->local to be NULL? I don't believe so. In which case, feel free to assert these, but don't check them.

@@ +289,5 @@
> +  TAILQ_FOREACH(p1, &peerStream->check_list, entry){
> +    if (p1 != NULL && p1->local != NULL && p1->remote != NULL) {
> +      // Code would be slightly cleaner if we waited till the end to push
> +      // onto the vector, but would require an extra copy. I could go either
> +      // way, honestly.

This isn't that useful a comment.

@@ +311,5 @@
> +          pair.state = NrIceCandidatePair::State::STATE_SUCCEEDED;
> +          break;
> +        case NR_ICE_PAIR_STATE_CANCELLED:
> +          pair.state = NrIceCandidatePair::State::STATE_CANCELLED;
> +          break;

I generally prefer tables to big switches.

@@ +313,5 @@
> +        case NR_ICE_PAIR_STATE_CANCELLED:
> +          pair.state = NrIceCandidatePair::State::STATE_CANCELLED;
> +          break;
> +        default:
> +          outPairs->pop_back();

Please either:

(1) Leave the out value in a random state on failure
(2) Leave it empty.

Trying to clean it up when you have failed isn't useful.

@@ +332,5 @@
> +      }
> +    }
> +    else {
> +      //TODO(bcampen@mozilla.com): Garbage in the list of candidate pairs.
> +      // What should we do here? Bail? Clear the results?

See above.

@@ +343,5 @@
> +}
> +
> +nsresult NrIceMediaStream::GetCandidatePair(uint64_t priority,
> +                                            NrIceCandidatePair* outPair) const {
> +  // TODO(bcampen@mozilla.com): Make less wasteful

I am assuming you are going to rewrite this fxn entirely so didn't really review it.

@@ +354,5 @@
> +  for (std::vector<NrIceCandidatePair>::iterator p=pairs.begin();
> +       p != pairs.end(); ++p) {
> +    if (p->priority == priority) {
> +      if (outPair == NULL) {
> +        (*outPair) = *p;

I think you want != NULL here. But again, I think you're working too hard to provide this contract.

::: media/mtransport/nricemediastream.h
@@ +66,5 @@
>  /* A summary of a candidate, for use in asking which candidate
>     pair is active */
> +class NrIceCandidate {
> + public:
> +  NrIceCandidate() : port(0), type(ICE_HOST) {}

Why does this need a ctor? The implied contract of this code seems to be that you will never get an invalid value. Same applies to NrIceCandidatePair.

@@ +87,5 @@
> +// accumulating these for every candidate pair. Alternately, we could make use
> +// of existing logging (and enhance the existing logging as needed). I like the
> +// latter approach a little better, since there is less duplication of effort,
> +// but it might be harder. Will need to do some research. For now, we do not
> +// bake in anything that would be covered by this, like timestamps.

You'r egoing to remove this before you commit, right?

@@ +92,5 @@
> +class NrIceCandidatePair {
> +
> + public:
> +  NrIceCandidatePair() :
> +    state(STATE_FROZEN),

Please add an invalid and initialize to that.

@@ +140,5 @@
>    std::vector<std::string> GetCandidates() const;
>  
> +  // Get all candidate pairs, whether in the check list or triggered check
> +  // queue, in priority order.
> +  nsresult GetCandidatePairs(std::vector<NrIceCandidatePair>* outPairs ) const;

No space before )

@@ +147,5 @@
> +  // note: pair priority is a unique key if both ends selected unique candidate
> +  // priorities, and we followed the spec when we calculated the pair priority.
> +  // If this does not hold, this will get the first such pair we find.
> +  nsresult GetCandidatePair(uint64_t priority,
> +                            NrIceCandidatePair* outPair) const;

Please do not assume that pair priority is unique. What if the other side uses duplicate priorities? I'm not sure why this is a useful API.

::: media/mtransport/test/ice_unittest.cpp
@@ +356,5 @@
>  
> +  void DumpCandidatePairs(NrIceMediaStream *stream) {
> +    std::vector<NrIceCandidatePair> pairs;
> +    nsresult res = stream->GetCandidatePairs(&pairs);
> +    if (res != NS_OK) {

ASSERT_TRUE(NS_SUCCEEDED...)

@@ +357,5 @@
> +  void DumpCandidatePairs(NrIceMediaStream *stream) {
> +    std::vector<NrIceCandidatePair> pairs;
> +    nsresult res = stream->GetCandidatePairs(&pairs);
> +    if (res != NS_OK) {
> +      std::cerr << "Yikes! GetCandidatePairs() failed for stream " 

Please fix trailing whitespace here and below.

@@ +364,5 @@
> +    }
> +    std::cerr << "Begin list of candidate pairs [" << std::endl;
> +
> +    for (std::vector<NrIceCandidatePair>::iterator p = pairs.begin();
> +         p != pairs.end(); ++p) {

This is OK, but I usually just use a size_t index.

@@ +366,5 @@
> +
> +    for (std::vector<NrIceCandidatePair>::iterator p = pairs.begin();
> +         p != pairs.end(); ++p) {
> +      std::cerr << p->local.host << ":" << p->local.port 
> +                << " (type = " << p->local.type << ") <-> " 

Please use DumpCandidate, refactoring as needed.
Attachment #801044 - Attachment is obsolete: true
Attachment #801678 - Flags: feedback?(ekr)
Comment on attachment 801678 [details] [diff] [review]
Refinement based on feedback from ekr. Also, export a symbol that was recently landed that I needed.

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

This looks good. Please write whatever unit tests you plan to and
then ask for r?ekr

::: media/mtransport/nricemediastream.cpp
@@ +119,5 @@
> +// This is not a member fxn because we want to hide the
> +// defn of nr_ice_candidate but we pass by reference.
> +static NrIceCandidate* MakeNrIceCandidate(const nr_ice_candidate& candc) {
> +  ScopedDeletePtr<NrIceCandidate> out(new NrIceCandidate());
> +  if (!ToNrIceCandidate(candc, out)) {

Vertical whitespace before this.

@@ +262,5 @@
>    RFREE(attrs);
>  }
>  
> +nsresult NrIceMediaStream::GetCandidatePairs(std::vector<NrIceCandidatePair>*
> +                                             outPairs) const {

align arguments with (

convention here is out_pairs

@@ +267,5 @@
> +  MOZ_ASSERT(outPairs);
> +
> +  // Get the check_list on the peer stream (this is where the check_list
> +  // actually lives, not in stream_)
> +  nr_ice_media_stream* peerStream = nullptr;

variables here are underscore, not camelcase

No need to initialize this.

@@ +301,5 @@
> +        break;
> +      case NR_ICE_PAIR_STATE_CANCELLED:
> +        pair.state = NrIceCandidatePair::State::STATE_CANCELLED;
> +        break;
> +      default:

Please put an assert here, since it's a can't happen.

@@ +306,5 @@
> +        return NS_ERROR_FAILURE;
> +    }
> +
> +    pair.priority = p1->priority;
> +    pair.nominated = p1->peer_nominated != 0 || p1->nominated != 0;

Please don't compare against 0 in boolean expressions.

@@ +307,5 @@
> +    }
> +
> +    pair.priority = p1->priority;
> +    pair.nominated = p1->peer_nominated != 0 || p1->nominated != 0;
> +    pair.selected = p1->local->component != nullptr &&

Same here.

::: media/mtransport/nricemediastream.h
@@ +43,5 @@
>  // This is a wrapper around the nICEr ICE stack
>  #ifndef nricemediastream_h__
>  #define nricemediastream_h__
>  
> +#include <string>

Thanks for including this. #include what you use, etc.

@@ +124,5 @@
>    std::vector<std::string> GetCandidates() const;
>  
> +  // Get all candidate pairs, whether in the check list or triggered check
> +  // queue, in priority order. outPairs is cleared before being filled.
> +  nsresult GetCandidatePairs(std::vector<NrIceCandidatePair>* outPairs) const;

convention in this code is not to use camelcase for args.

|out_pairs|

::: media/mtransport/test/ice_unittest.cpp
@@ +355,5 @@
>    }
>  
> +  void DumpCandidatePairs(NrIceMediaStream *stream) {
> +    std::vector<NrIceCandidatePair> pairs;
> +    nsresult r = stream->GetCandidatePairs(&pairs);

nsresult return values are rv or res.

nICEr return values are r.

@@ +361,5 @@
> +
> +    std::cerr << "Begin list of candidate pairs [" << std::endl;
> +
> +    for (std::vector<NrIceCandidatePair>::iterator p = pairs.begin();
> +         p != pairs.end(); ++p) {

you think an iterator here is better? It's style, but it's not my style usually....
Attachment #801678 - Flags: feedback?(ekr) → feedback+
Attachment #801678 - Attachment is obsolete: true
Attachment #803414 - Attachment is obsolete: true
Slight tweak; not sure whether we had UINT64_MAX somewhere in the portability layer, so used numeric_limits instead.
Attachment #803848 - Flags: review?(ekr)
Attachment #803848 - Attachment is obsolete: true
Attachment #803848 - Flags: review?(ekr)
Attachment #803861 - Attachment is obsolete: true
Backed out testing-related changes; this will go in a separate patch.
(ice_candpair_scrape got r+ with some extra feedback; this feedback has been incorporated into ice_candpair_scrape_test, along with some testing)
Attachment #803881 - Flags: review?(ekr)
Comment on attachment 803881 [details] [diff] [review]
Some basic testing of GetCandidatePairs, plus some coding convention fixes tothe prior patch that got r+ anyhow.

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

::: media/mtransport/nricemediastream.h
@@ +123,5 @@
>    // Get all the candidates
>    std::vector<std::string> GetCandidates() const;
>  
>    // Get all candidate pairs, whether in the check list or triggered check
> +  // queue, in priority order. out_pairs is cleared before being filled.

I forgot to mention, but please do |out_pairs|

::: media/mtransport/test/ice_unittest.cpp
@@ +351,5 @@
>      candidates_[stream->name()].push_back(candidate);
>    }
>  
> +  nsresult GetCandidatePairs(size_t stream_index,
> +                              std::vector<NrIceCandidatePair>* pairs) {

align std:: with (

@@ +355,5 @@
> +                              std::vector<NrIceCandidatePair>* pairs) {
> +    MOZ_ASSERT(pairs);
> +    if (stream_index >= streams_.size()) {
> +      // Is there a better error for "no such index"?
> +      return NS_ERROR_INVALID_ARG;

Make this generate a gtest error

@@ +382,3 @@
>    void DumpCandidatePairs(NrIceMediaStream *stream) {
>      std::vector<NrIceCandidatePair> pairs;
> +    stream->GetCandidatePairs(&pairs);

Check error here.

@@ +409,5 @@
> +      if (priority < pairs[p].priority) {
> +        std::cerr << "Priority increased in subsequent pairs:" << std::endl;
> +        DumpCandidatePair(pairs[p-1]);
> +        DumpCandidatePair(pairs[p]);
> +        return false;

I feel like this should be setting priority to pairs[].priority...

Also use EXPECT_...

@@ +416,5 @@
> +    return true;
> +  }
> +
> +  bool UpdateAndValidateCandidatePairs(size_t stream_index,
> +                                        std::vector<NrIceCandidatePair>*

Fix indent.

@@ +417,5 @@
> +  }
> +
> +  bool UpdateAndValidateCandidatePairs(size_t stream_index,
> +                                        std::vector<NrIceCandidatePair>*
> +                                        new_pairs) {

I would make this two arguments and do the assignment above.

Alternately, use a member variable?

@@ +429,5 @@
> +    // same order?
> +    size_t po = 0;
> +    size_t pn = 0;
> +    while (pn < new_pairs->size()) {
> +      // Extremely crude, just assumes priority is a unique key

And what if it's not?

If you're going to bother with this, why don't you actually compare the candidates?

This loop would be much clearer if the outer loop were the old pairs rather than the new pairs.

I.e.

while (po < new_pairs->size()) {
  while (pn < new_pairs->size()) {
    if (match)
      break;    
    else
      new candidate;
    ++pn;
  }
  if (pn == new_pairs->size())
    error;
}

@@ +956,5 @@
> +  ASSERT_TRUE(Gather(true));
> +
> +  std::vector<NrIceCandidatePair> pairs;
> +  nsresult r = p1_->GetCandidatePairs(0, &pairs);
> +  // There should be no candidate pairs prior to calling Connect()

Check return values here and below.

@@ +958,5 @@
> +  std::vector<NrIceCandidatePair> pairs;
> +  nsresult r = p1_->GetCandidatePairs(0, &pairs);
> +  // There should be no candidate pairs prior to calling Connect()
> +  ASSERT_EQ(0U, pairs.size());
> +  pairs.clear();

Unnecessary if you are checking return values.

@@ +973,5 @@
> +  std::vector<NrIceCandidatePair> pairs;
> +  nsresult r = p1_->GetCandidatePairs(0, &pairs);
> +  ASSERT_EQ(NS_OK, r);
> +  // How detailed of a check do we want to do here? If the turn server is
> +  // functioning, we'll get two pairs, but this is probably not something we

You must mean "at least two"

@@ +997,5 @@
> +  std::vector<NrIceCandidatePair> pairs2;
> +
> +  p1_->StartChecks();
> +  ASSERT_TRUE(p1_->UpdateAndValidateCandidatePairs(0, &pairs1));
> +  ASSERT_TRUE(p2_->UpdateAndValidateCandidatePairs(0, &pairs2));

Make these void and do the asserts inside.

@@ +1003,5 @@
> +  p2_->StartChecks();
> +  ASSERT_TRUE(p1_->UpdateAndValidateCandidatePairs(0, &pairs1));
> +  ASSERT_TRUE(p2_->UpdateAndValidateCandidatePairs(0, &pairs2));
> +
> +  WaitForComplete();

Suggest a final check afterward if UpdateAndValidate
Attachment #803881 - Flags: review?(ekr) → review-
Attachment #803881 - Attachment is obsolete: true
Attachment #804117 - Attachment is obsolete: true
Attachment #804124 - Flags: review?(ekr)
Attachment #804124 - Attachment is obsolete: true
Attachment #804124 - Flags: review?(ekr)
Attachment #804527 - Flags: review?(ekr)
Added some const-correctness fixes.
Comment on attachment 804527 [details] [diff] [review]
Some basic testing of GetCandidatePairs, plus some coding convention fixes tothe prior patch that got r+ anyhow.

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

r- for the comparator issue. The rest is minor.

::: media/mtransport/nricemediastream.h
@@ +90,5 @@
> +    return host < rhs.host;
> +  }
> +
> +  bool operator==(const NrIceCandidate& rhs) const {
> +    return host == rhs.host && port == rhs.port && type == rhs.type;

Can't you just provide a comparator as a template argument to std::set?

::: media/mtransport/test/ice_unittest.cpp
@@ +983,5 @@
> +  ASSERT_EQ(0U, pairs.size());
> +
> +  res = p2_->GetCandidatePairs(0, &pairs);
> +  ASSERT_TRUE(NS_FAILED(res));
> +  ASSERT_EQ(0U, pairs.size());

This is true in this case, but it's not generally true that if (NS_FAILED(res)) pairs.size() == 0

@@ +996,5 @@
> +  nsresult r = p1_->GetCandidatePairs(0, &pairs);
> +  ASSERT_EQ(NS_OK, r);
> +  // How detailed of a check do we want to do here? If the turn server is
> +  // functioning, we'll get at least two pairs, but this is probably not
> +  // something we should assume.

No. This has to work behind firewalls.

This looks fine.

@@ +1027,5 @@
> +  p2_->UpdateAndValidateCandidatePairs(0, &pairs2);
> +
> +  WaitForComplete();
> +  p1_->UpdateAndValidateCandidatePairs(0, &pairs1);
> +  p2_->UpdateAndValidateCandidatePairs(0, &pairs2);

Shouldn't you check that the states here make sense. Minimally that at least one pair is complete now?
Attachment #804527 - Flags: review?(ekr) → review-
Attachment #804527 - Attachment is obsolete: true
Attachment #805438 - Flags: review?(ekr)
Comment on attachment 805438 [details] [diff] [review]
2. Some basic testing of GetCandidatePairs, plus some coding convention fixes tothe prior patch that got r+ anyhow.

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

lgtm
Attachment #805438 - Flags: review?(ekr) → review+
Whiteboard: [webrtc] → [webrtc] [leave-open]
Comment on attachment 803875 [details] [diff] [review]
1. Adding a bulk getter for the current state of all ICE candidate pairs(plus a little testing).

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

Patch is identical to the one ekr gave r+ (https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=906990&attachment=801678)
Attachment #803875 - Flags: review+
Attachment #803875 - Flags: checkin?(adam)
Attachment #805438 - Flags: checkin?(adam)
Okay, I'm queued up to check this in as soon as the tree re-opens.
Attachment #803875 - Flags: checkin?(adam) → checkin+
Attachment #805438 - Flags: checkin?(adam) → checkin+
Attachment #806341 - Flags: review?(ekr)
Attachment #808629 - Flags: review?(adam)
Attachment #808629 - Attachment is obsolete: true
Attachment #808629 - Flags: review?(adam)
Attachment #808717 - Attachment description: Somewhat preliminary/mocky: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream. → WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.
Comment on attachment 808717 [details] [diff] [review]
WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.

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

drive-by comments

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1675,5 @@
> +                             streamId),
> +                NS_DISPATCH_NORMAL);
> +}
> +
> +void PeerConnectionImpl::GetCandidatePairsInt(size_t streamId) {

ekr's naming convention would add an _s to the name since it always runs on ststhread

@@ +1696,5 @@
> +  // TODO(bcampen@mozilla.com): Once we are done defining the observer in IDL,
> +  // we'll need to convert |pairs| to whatever type it expects. For this mocky
> +  // code, we just serialize to JSON.
> +  std::ostringstream oss;
> +  oss << pairs;

Problem: for somewhat hazy reasons (to me), we've been told to avoid c++ streams in production code. (Debug builds are ok, opt/production builds are not).  If this is a huge problem I can look into it again, but I haven't heard of a change.

@@ +1703,5 @@
> +    RUN_ON_THREAD(mThread,
> +                  WrapRunnable(statsObserver,
> +                         &IPeerConnectionStatsObserver::OnCandidatePairsUpdate,
> +                         streamId,
> +                         oss.str().c_str(),

What would be the ownership on this?
Attachment #808717 - Attachment description: WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream. → Somewhat preliminary/mocky: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.
> 
> @@ +1703,5 @@
> > +    RUN_ON_THREAD(mThread,
> > +                  WrapRunnable(statsObserver,
> > +                         &IPeerConnectionStatsObserver::OnCandidatePairsUpdate,
> > +                         streamId,
> > +                         oss.str().c_str(),
> 
> What would be the ownership on this?

Yep, this is almost certainly bad. I need to go look at the IDL-generated code and see if there is some way to use WrapRunnable, as opposed to writing a custom runnable that will take ownership and convert as required.
Comment on attachment 808717 [details] [diff] [review]
WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.

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

Architecturally, the dispatch to STS which dispatches back to main is certainly what we discussed. The names you've chosen are hyperspecific to the ICE candidate pairs state use case. However, this is the exact same model we need to use for retrieving the other stats, and I'd prefer not to have two side-by-side implementations that require two dispatches to gather up all of the information we're interested in.

In other words, I think we really want to harmonize the function names and parameters with the API that's developing in Bug 902003.

From the IDL side, I think you do want to re-use the IPeerConnectionObserver rather than creating a new thing (IPeerConnectionStatsObserver) for just this set of information. The information you're gathering for your panel is going to be scoped to PeerConnections anyway, so you'll probably start by iterating over all of the PeerConnections (see GlobalPCList) and querying them for their information. The PC.js object should then call down into PeerConnectionImpl, which does the dispatch to STS and back. The upcall should be a method on IPeerConnectionObserver (which is twinned with the PeerConnection), when can then dispatch back to your panel code.

Ideally, this call from the panel through PC.js and back should follow http://dev.w3.org/2011/webrtc/editor/webrtc.html#statistics-model as closely as is practical. I understand this might be more than is realistic for this patch, though. Use your judgement, but err on the side of laying down infrastructure for future stats use.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1675,5 @@
> +                             streamId),
> +                NS_DISPATCH_NORMAL);
> +}
> +
> +void PeerConnectionImpl::GetCandidatePairsInt(size_t streamId) {

The general convention in this part of the code is that functions that can only run on one thread have _m (main) or _s (sts) appended to their name. So, e.g., this should be GetCandidatePairsInt_s

@@ +1676,5 @@
> +                NS_DISPATCH_NORMAL);
> +}
> +
> +void PeerConnectionImpl::GetCandidatePairsInt(size_t streamId) {
> +  nsCOMPtr<IPeerConnectionStatsObserver> statsObserver = do_QueryReferent(mPCStatsObserver);

The header file (correctly, I believe) points out that mPCStatsObserver should be manipulated only on the main thread. I think you need to move the QueryReferent into the function that does dispatch.

@@ +1703,5 @@
> +    RUN_ON_THREAD(mThread,
> +                  WrapRunnable(statsObserver,
> +                         &IPeerConnectionStatsObserver::OnCandidatePairsUpdate,
> +                         streamId,
> +                         oss.str().c_str(),

This is going to make OnCandidatePairsUpdate very sad, since oss will be long gone by the time it starts executing.
Attachment #808717 - Attachment description: Somewhat preliminary/mocky: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream. → WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.
Comment on attachment 808717 [details] [diff] [review]
WIP: Expose an async dispatch function in PeerConnectionMedia for getting the list of candidate pairs on a given media stream.

Removing the dispatch patch, since it looks like we'll be riding on top of the dispatch used for 902003.
Attachment #808717 - Attachment is obsolete: true
Attachment #809384 - Flags: review?(ekr)
Attachment #809384 - Attachment is obsolete: true
Attachment #809384 - Flags: review?(ekr)
Attachment #809579 - Attachment is obsolete: true
Attachment #809614 - Attachment is obsolete: true
Attachment #809638 - Attachment is obsolete: true
Comment on attachment 806341 [details] [diff] [review]
3. Make it easier to filter out logging related to a given candidate pair.

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

lgtm with minor changes below.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
@@ +89,5 @@
>        (MAX(o_priority, a_priority))<<1 | (o_priority > a_priority?0:1);
>  
> +    // TODO(bcampen@mozilla.com): Would be nice to log why this candidate was
> +    // created (initial pair generation, triggered check, and new trickle
> +    // candidate seem to be the possibilities here)

1. This code uses /**/
2. Comments end in .

@@ +187,5 @@
>  
>      pair->stun_cb_timer=0;
>  
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/CAND-PAIR(%s): STUN cb on pair %s",
> +      pair->pctx->label,pair->local->stream->label,pair->codeword,pair->as_string);

This is a weird duplication since we are showing the code-word and have the pair. Maybe pair addr = ...

@@ +225,5 @@
>          }
>          request_src=&pair->stun_client->my_addr;
>          nr_socket_getaddr(pair->local->osock,&response_dst);
>          if (nr_transport_addr_cmp(request_src,&response_dst,NR_TRANSPORT_ADDR_CMP_MODE_ALL)){
> +          r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND-PAIR(%s): Address mismatch %s != %s",pair->pctx->label,pair->codeword,request_src->as_string,response_dst.as_string);

Maybe "Local address...""

@@ +499,5 @@
>    }
>  
>  int nr_ice_candidate_pair_dump_state(nr_ice_cand_pair *pair, FILE *out)
>    {
> +    //r_log(LOG_ICE,LOG_DEBUG,"CAND-PAIR(%s): pair %s: state=%s, priority=0x%llx\n",pair->codeword,pair->as_string,nr_ice_cand_pair_states[pair->state],pair->priority);

Not sure why we are fixing commented code.

Please fix comments to be /**/

@@ +530,5 @@
>      int r,_status;
>  
>      pair->restart_nominated_cb_timer=0;
>  
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/CAND-PAIR(%s):%d: Restarting pair as nominated: %s",pair->pctx->label,pair->local->stream->label,pair->codeword,pair->remote->component->component_id,pair->as_string);

Shouldn't this be COMP(%d)

@@ +552,5 @@
>      int r,_status;
>  
>      pair->restart_controlled_cb_timer=0;
>  
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/CAND-PAIR(%s):%d: Restarting pair as CONTROLLED: %s",pair->pctx->label,pair->local->stream->label,pair->codeword,pair->remote->component->component_id,pair->as_string);

Shouldn't this be COMP(%d).

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +349,5 @@
>        if(c1->state!=NR_ICE_CAND_STATE_INITIALIZED){
>          r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): Removing non-initialized candidate %s",
>            ctx->label,c1->label);
>          if (c1->state == NR_ICE_CAND_STATE_INITIALIZING) {
> +          r_log(LOG_ICE,LOG_NOTICE, "ICE(%s): Removing candidate in state INITIALIZING: %s",

Should this be ICE(%s)/CAND(%s)

@@ +486,5 @@
>          goto next_pair;
>        }
>  
>        if (local_addr_matched && remote_addr_matched){
> +        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND_PAIR(%s): For received check, found a matching pair: %s",comp->stream->pctx->label,pair->codeword,pair->as_string);

Found a matching pair for received check?

@@ +565,5 @@
>        }
>      }
>  
>      /* OK, we've got a pair to work with. Turn it on */
> +    assert(pair != 0);

assert(pair)

@@ +790,5 @@
>      /* Are we changing what the nominated pair is? */
>      if(comp->nominated){
>        if(comp->nominated->priority > pair->priority)
>          return(0);
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): replacing pair %s with pair %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,comp->nominated->codeword,pair->codeword,comp->nominated->as_string,pair->as_string);

This seems weird. Let's have the new pair on the right.

@@ +795,4 @@
>      }
>  
>      /* Set the new nominated pair */
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s): nominated pair is %s (0x%p)",comp->stream->pctx->label,comp->stream->label,comp->component_id,pair->codeword,pair->as_string,pair);

Suggest we remove the pointer arguments here and below, since we probably eventually want to expose this to content,

@@ +808,5 @@
>        if((p2 != pair) &&
>           (p2->remote->component->component_id == comp->component_id) &&
>           ((p2->state == NR_ICE_PAIR_STATE_FROZEN) ||
>  	  (p2->state == NR_ICE_PAIR_STATE_WAITING))) {
> +        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s (0x%p) because pair %s was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,pair->codeword,p2->as_string,p2,pair->as_string);

Maybe add codewords here for the othe rpair?
Attachment #806341 - Flags: review?(ekr) → review+
(In reply to Eric Rescorla (:ekr) from comment #44)
> Comment on attachment 806341 [details] [diff] [review]
> Make it easier to filter out logging related to a given candidate pair.
> 
> Review of attachment 806341 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> lgtm with minor changes below.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_candidate_pair.c
> @@ +187,5 @@
> >  
> >      pair->stun_cb_timer=0;
> >  
> > +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/CAND-PAIR(%s): STUN cb on pair %s",
> > +      pair->pctx->label,pair->local->stream->label,pair->codeword,pair->as_string);
> 
> This is a weird duplication since we are showing the code-word and have the
> pair. Maybe pair addr = ...
>

Well, all we have here is codeword and as_string (which contains the codeword). Omitting the second copy of the codeword would require a little extra code. And we want to keep the CAND-PAIR(<codeword>), since the log filtering is going to key off that.
 

> @@ +530,5 @@
> >      int r,_status;
> >  
> >      pair->restart_nominated_cb_timer=0;
> >  
> > +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/CAND-PAIR(%s):%d: Restarting pair as nominated: %s",pair->pctx->label,pair->local->stream->label,pair->codeword,pair->remote->component->component_id,pair->as_string);
> 
> Shouldn't this be COMP(%d)
>

Sure. I'll go fix other instances for consistency.
 
> @@ +349,5 @@
> >        if(c1->state!=NR_ICE_CAND_STATE_INITIALIZED){
> >          r_log(LOG_ICE,LOG_DEBUG,"ICE(%s): Removing non-initialized candidate %s",
> >            ctx->label,c1->label);
> >          if (c1->state == NR_ICE_CAND_STATE_INITIALIZING) {
> > +          r_log(LOG_ICE,LOG_NOTICE, "ICE(%s): Removing candidate in state INITIALIZING: %s",
> 
> Should this be ICE(%s)/CAND(%s)
>

I was planning on making the use of CAND(<label>) more consistent later, but I can do that now.

> @@ +790,5 @@
> >      /* Are we changing what the nominated pair is? */
> >      if(comp->nominated){
> >        if(comp->nominated->priority > pair->priority)
> >          return(0);
> > +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): replacing pair %s with pair %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,comp->nominated->codeword,pair->codeword,comp->nominated->as_string,pair->as_string);
> 
> This seems weird. Let's have the new pair on the right.
>

Pretty sure that is what it was doing already; comp->nominated is the currently nominated pair, and pair is the potential replacement.

> @@ +795,4 @@
> >      }
> >  
> >      /* Set the new nominated pair */
> > +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s): nominated pair is %s (0x%p)",comp->stream->pctx->label,comp->stream->label,comp->component_id,pair->codeword,pair->as_string,pair);
> 
> Suggest we remove the pointer arguments here and below, since we probably
> eventually want to expose this to content,
> 

Agreed. I'll keep an eye out for any similar logging.

> @@ +808,5 @@
> >        if((p2 != pair) &&
> >           (p2->remote->component->component_id == comp->component_id) &&
> >           ((p2->state == NR_ICE_PAIR_STATE_FROZEN) ||
> >  	  (p2->state == NR_ICE_PAIR_STATE_WAITING))) {
> > +        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/comp(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s (0x%p) because pair %s was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,pair->codeword,p2->as_string,p2,pair->as_string);
> 
> Maybe add codewords here for the othe rpair?

Pretty sure this is already the case.
Comment on attachment 810067 [details] [diff] [review]
7. Nits from r=ekr (ice_candpair_logging_work.patch got r+)

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

Piping the diff through "dwdiff --delimiters=", /\"():" -c --diff-input -" made this a _lot_ easier to read. (dwdiff is available via brew, dunno about other sources)
Attachment #810067 - Flags: review?(ekr)
Attachment #810067 - Attachment description: Nits from (ice_candpair_logging_work.patch got r+) → Nits from r=ekr (ice_candpair_logging_work.patch got r+)
Comment on attachment 809407 [details] [diff] [review]
4. Using more appropriate log-levels (r_log) for errors and other not-quite-right conditions.

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

Like the nits patch, dwdiff is extremely useful for reviewing this.
Attachment #809407 - Flags: review?(ekr)
Attachment #809953 - Attachment is obsolete: true
Comment on attachment 810084 [details] [diff] [review]
Allow logging related to a given candidate pair to be fetched.

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

Just wanted to get your high-level take on this. In particular, do we need to worry about locking, or would it be appropriate to require that all access be made from the STS thread? Also, I'm not 100% sure I like the naming, since the thing is not _exactly_ a ring buffer in terms of its performance characteristics. It is quite close though.
Attachment #810084 - Flags: feedback?(ekr)
Attachment #810084 - Attachment is obsolete: true
Attachment #810084 - Flags: feedback?(ekr)
Comment on attachment 810153 [details] [diff] [review]
5. Allow logging related to a given candidate pair to be fetched.

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

Just wanted to make sure the high-level stuff looked right. In particular, I was wondering whether we should bother with locking, or just require all access to be via the STS thread. Having the locking would make some of the integration into JS easier. Also, not 100% sure of the naming (not _exactly_ a ring buffer, but very similar).
Attachment #810153 - Flags: feedback?(ekr)
Attachment #806341 - Attachment description: Make it easier to filter out logging related to a given candidate pair. → 3. Make it easier to filter out logging related to a given candidate pair.
Attachment #809407 - Attachment description: Using more appropriate log-levels (r_log) for errors and other not-quite-right conditions. → 4. Using more appropriate log-levels (r_log) for errors and other not-quite-right conditions.
Attachment #810153 - Attachment description: Allow logging related to a given candidate pair to be fetched. → 5. Allow logging related to a given candidate pair to be fetched.
Attachment #810181 - Attachment description: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. → 6. Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest.
Attachment #810067 - Attachment description: Nits from r=ekr (ice_candpair_logging_work.patch got r+) → 7. Nits from r=ekr (ice_candpair_logging_work.patch got r+)
Attachment #803875 - Attachment description: Adding a bulk getter for the current state of all ICE candidate pairs(plus a little testing). → 1. Adding a bulk getter for the current state of all ICE candidate pairs(plus a little testing).
Attachment #805438 - Attachment description: Some basic testing of GetCandidatePairs, plus some coding convention fixes tothe prior patch that got r+ anyhow. → 2. Some basic testing of GetCandidatePairs, plus some coding convention fixes tothe prior patch that got r+ anyhow.
Attachment #810153 - Attachment is obsolete: true
Attachment #810153 - Flags: feedback?(ekr)
Attachment #806341 - Attachment is obsolete: true
Attachment #809407 - Attachment is obsolete: true
Attachment #809407 - Flags: review?(ekr)
Attachment #810735 - Attachment is obsolete: true
Attachment #810181 - Attachment is obsolete: true
Unbitrotting
Attachment #810067 - Attachment is obsolete: true
Attachment #810067 - Flags: review?(ekr)
Comment on attachment 814966 [details] [diff] [review]
Part 3: Make it easier to filter out logging related to a given candidate pair.

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

Carry forward r+ from ekr.
Attachment #814966 - Flags: review+
Attachment #814967 - Flags: review?(ekr)
Attachment #814968 - Flags: review?(ekr)
Attachment #814969 - Flags: review?(ekr)
Attachment #814970 - Flags: review?(ekr)
Comment on attachment 814970 [details] [diff] [review]
Part 7: Nits from  (ice_candpair_logging_work.patch got r+)

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

lgtm with nits below.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +164,5 @@
>  
>      TAILQ_FOREACH(tmp,&isock->candidates,entry_sock){
>        if(cand->priority==tmp->priority){
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s)/CAND(%s)/CAND(%s): duplicate priority %u",
> +          ctx->label,cand->label,tmp->label,cand->priority);

Please just use the primary index here and put the second one later. If it makes searching easier, use CAND(%s) in the later part of the string.

Or add a log message indicating the one we are deleting with that in the header.

@@ +215,1 @@
>        ctx->label,label,ctype);

Can we convert the ctype to string here.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +649,5 @@
>      nr_ice_cand_pair *pair=0;
>      char codeword[5];
>  
>      nr_ice_compute_codeword(lcand->label,strlen(lcand->label),codeword);
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND(%s): Pairing local candidate %s", pctx->label, codeword,lcand->label);

No space after ,

@@ +791,5 @@
>      /* Are we changing what the nominated pair is? */
>      if(comp->nominated){
>        if(comp->nominated->priority > pair->priority)
>          return(0);
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): replacing pair %s with pair %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,comp->nominated->codeword,pair->codeword,comp->nominated->as_string,pair->as_string);

Having two pairs in the initial header seems weird. Let's just do the first and then say "Replacing with... $SECOND"

@@ +809,5 @@
>        if((p2 != pair) &&
>           (p2->remote->component->component_id == comp->component_id) &&
>           ((p2->state == NR_ICE_PAIR_STATE_FROZEN) ||
>  	  (p2->state == NR_ICE_PAIR_STATE_WAITING))) {
> +        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s because pair %s was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,pair->codeword,p2->as_string,pair->as_string);

Same observation here. One Pair in the header.
Comment on attachment 814970 [details] [diff] [review]
Part 7: Nits from  (ice_candpair_logging_work.patch got r+)

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

lgtm with nits below.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +164,5 @@
>  
>      TAILQ_FOREACH(tmp,&isock->candidates,entry_sock){
>        if(cand->priority==tmp->priority){
> +        r_log(LOG_ICE,LOG_ERR,"ICE(%s)/CAND(%s)/CAND(%s): duplicate priority %u",
> +          ctx->label,cand->label,tmp->label,cand->priority);

Please just use the primary index here and put the second one later. If it makes searching easier, use CAND(%s) in the later part of the string.

Or add a log message indicating the one we are deleting with that in the header.

@@ +215,1 @@
>        ctx->label,label,ctype);

Can we convert the ctype to string here.

::: media/mtransport/third_party/nICEr/src/ice/ice_component.c
@@ +649,5 @@
>      nr_ice_cand_pair *pair=0;
>      char codeword[5];
>  
>      nr_ice_compute_codeword(lcand->label,strlen(lcand->label),codeword);
> +    r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/CAND(%s): Pairing local candidate %s", pctx->label, codeword,lcand->label);

No space after ,

@@ +791,5 @@
>      /* Are we changing what the nominated pair is? */
>      if(comp->nominated){
>        if(comp->nominated->priority > pair->priority)
>          return(0);
> +      r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): replacing pair %s with pair %s",comp->stream->pctx->label,comp->stream->label,comp->component_id,comp->nominated->codeword,pair->codeword,comp->nominated->as_string,pair->as_string);

Having two pairs in the initial header seems weird. Let's just do the first and then say "Replacing with... $SECOND"

@@ +809,5 @@
>        if((p2 != pair) &&
>           (p2->remote->component->component_id == comp->component_id) &&
>           ((p2->state == NR_ICE_PAIR_STATE_FROZEN) ||
>  	  (p2->state == NR_ICE_PAIR_STATE_WAITING))) {
> +        r_log(LOG_ICE,LOG_DEBUG,"ICE-PEER(%s)/STREAM(%s)/COMP(%d)/CAND-PAIR(%s)/CAND-PAIR(%s): cancelling FROZEN/WAITING pair %s because pair %s was nominated.",comp->stream->pctx->label,comp->stream->label,comp->component_id,p2->codeword,pair->codeword,p2->as_string,pair->as_string);

Same observation here. One Pair in the header.
Attachment #814970 - Flags: review?(ekr) → review+
Folded Part 7 (nits) into this, incorporated more review feedback, and unbitrotted.
Attachment #814966 - Attachment is obsolete: true
Comment on attachment 814970 [details] [diff] [review]
Part 7: Nits from  (ice_candpair_logging_work.patch got r+)

Obsoleting. Folded into part 3.
Attachment #814970 - Attachment is obsolete: true
Attachment #814967 - Attachment is obsolete: true
Attachment #814967 - Flags: review?(ekr)
Comment on attachment 816692 [details] [diff] [review]
Part 3: Make it easier to filter out logging related to a given candidate pair (nits patches folded)

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

Carry r+ forward, and request checkin.
Attachment #816692 - Flags: review+
Attachment #816692 - Flags: checkin?(adam)
Comment on attachment 816692 [details] [diff] [review]
Part 3: Make it easier to filter out logging related to a given candidate pair (nits patches folded)

https://hg.mozilla.org/integration/mozilla-inbound/rev/10c7b809dfb1
Attachment #816692 - Flags: checkin?(adam) → checkin+
Comment on attachment 814969 [details] [diff] [review]
Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest.

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

lgtm
Attachment #814969 - Flags: review?(ekr) → review+
Attachment #816693 - Flags: review?(ekr)
Move something small from 902003 here.
Attachment #817924 - Attachment is obsolete: true
Attachment #817925 - Attachment is obsolete: true
Bring up-to-date with patches landed from 902003.
Attachment #818133 - Attachment is obsolete: true
Bring up-to-date with patches landed from 902003.
Attachment #818134 - Attachment is obsolete: true
Unbitrot
Attachment #819823 - Attachment is obsolete: true
Attachment #819824 - Attachment is obsolete: true
Unbitrot.
Attachment #820491 - Attachment is obsolete: true
Attachment #820492 - Attachment is obsolete: true
Update unit-test to use LOG_INFO, since that's the level RLogRingBuffer grabs now.
Attachment #820496 - Attachment is obsolete: true
Comment on attachment 816693 [details] [diff] [review]
Part 4: Using more appropriate log-levels (r_log) for errors and other not-quite-right conditions.

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

lgtm

::: media/mtransport/third_party/nICEr/src/ice/ice_media_stream.c
@@ +379,5 @@
>        nr_ice_candidate_pair_start(pair->pctx,pair); /* Ignore failures */
>        NR_ASYNC_TIMER_SET(timer_val,nr_ice_media_stream_check_timer_cb,cb_arg,&stream->timer);
>      }
>      else {
> +      r_log(LOG_ICE,LOG_WARNING,"ICE-PEER(%s): no pairs for %s",stream->pctx->label,stream->label);

Let's leave this at NOTICE. I believe it can happen during trickle.

::: media/mtransport/third_party/nICEr/src/stun/stun_server_ctx.c
@@ +449,5 @@
>        if (colon && !strncmp(ctx->default_client->username,
>                              attr->u.username,
>                              colon - attr->u.username)) {
>          clnt = ctx->default_client;
> +        r_log(NR_LOG_STUN,LOG_NOTICE,"STUN-SERVER(%s): Falling back to default client, username=: %s",ctx->label,attr->u.username);

This is normal for early ICE. Let's do INFO or DEBUG
Attachment #816693 - Flags: review?(ekr) → review+
Comment on attachment 814968 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched.

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

::: media/mtransport/rlogringbuffer.cpp
@@ +12,5 @@
> +#include <deque>
> +#include <string>
> +#include <vector>
> +
> +#include <csi_platform.h>

Should this be in "extern "C" {}

@@ +16,5 @@
> +#include <csi_platform.h>
> +
> +#include "nsAutoPtr.h"
> +
> +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {

This seems like it can definitely be static.

@@ +27,5 @@
> +RLogRingBuffer* RLogRingBuffer::instance;
> +
> +RLogRingBuffer::RLogRingBuffer()
> +  : log_limit(4096),
> +    mutex("RLogRingBuffer::mutex") {

Move this to .h?

@@ +31,5 @@
> +    mutex("RLogRingBuffer::mutex") {
> +}
> +
> +RLogRingBuffer::~RLogRingBuffer() {
> +}

Let's omit this.

@@ +42,5 @@
> +
> +// Some platforms (notably WIN32) do not have this
> +#ifndef va_copy
> +#define va_copy(dest, src) ( (dest) = (src) )
> +#endif

Suggest remove this and then below use a fixed-size temp buffer.

@@ +52,5 @@
> +  va_copy(ap_copy, ap);
> +  // Might be worthwhile to pre-alloc some room that usually is enough.
> +  // First, we'd need to determine what a reasonable value for this would be.
> +  int requiredSize = vsnprintf(nullptr, 0, format, ap_copy);
> +  va_end(ap_copy);

Rather than do vsnprintf() twice, let's just use a 4k or so temp buffer. that's what r_log() does for its internal stuff anyway.
Then you can avoid the va_copy() as well.

@@ +107,5 @@
> +}
> +
> +void RLogRingBuffer::FilterAny(const std::vector<std::string>& substrings,
> +                                uint32_t limit,
> +                                std::deque<std::string>* matching_logs) {

indent.

@@ +114,5 @@
> +    // At a max, all of the log messages.
> +    limit = log_limit;
> +  }
> +
> +  for (auto log = log_messages.begin();

should we clear matching_logs()?

@@ +118,5 @@
> +  for (auto log = log_messages.begin();
> +       log != log_messages.end() && matching_logs->size() != limit;
> +       ++log) {
> +    if (AnySubstringMatches(substrings, *log)) {
> +      matching_logs->push_back(*log);

I feel like this is the opposite semantic from what we want. I.e,, if we do

log(A)
log(B)

Then when someone asks for the logs they should come in in A, B but you seem to have
the opposite.

Similarly, if I ask for limit 1 I should get A.

::: media/mtransport/rlogringbuffer.h
@@ +56,5 @@
> +extern "C" {
> +#include "r_log.h"
> +
> +/* Matches r_dest_vlog type defined in r_log.h */
> +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap);

Does this need to be extern?

@@ +75,5 @@
> +       NB: These are not threadsafe, nor are they safe to call during static
> +       init/deinit.
> +    */
> +    static RLogRingBuffer* GetInstance();
> +    static void FreeInstance();

I suggest we do Setup/Teardown/Instance() with Instance checking for things being live.

@@ +93,5 @@
> +                   std::deque<std::string>* matching_logs);
> +
> +    void SetLogLimit(uint32_t new_limit);
> +
> +    void Log(int facility, int level, const char *format, va_list ap);

Why not make this private and make the logger fxn a static class fxn.

@@ +106,5 @@
> +     * Might be worthwhile making this a circular buffer, but I think it is
> +     * preferable to take up as little space as possible if no logging is
> +     * happening/the ringbuffer is not being used.
> +    */
> +    std::deque<std::string> log_messages;

member variables have trailing _

@@ +111,5 @@
> +    /* Max size of log buffer (should we use time-depth instead/also?) */
> +    uint32_t log_limit;
> +    mozilla::Mutex mutex;
> +
> +    RLogRingBuffer(const RLogRingBuffer& orig) MOZ_DELETE;

DISALLOW_COPY_AND_ASSIGN

::: media/mtransport/test/ice_unittest.cpp
@@ +1252,5 @@
> +  std::deque<std::string> logs;
> +  RLogRingBuffer::GetInstance()->Filter("CAND-PAIR", 0, &logs);
> +  std::cerr << "Dumping CAND-PAIR logging:" << std::endl;
> +  for (auto i = logs.rbegin(); i != logs.rend(); ++i) {
> +    std::cerr << *i << std::endl;

Can you check that there is something here?

::: media/mtransport/test/rlogringbuffer_unittest.cpp
@@ +55,5 @@
> +
> +TEST_F(RLogRingBufferTest, TestFilterEmpty) {
> +  std::deque<std::string> logs;
> +  RLogRingBuffer::GetInstance()->Filter("", 0, &logs);
> +  ASSERT_EQ(0U, logs.size());

How do you ask for everything? Filter("")?
Suggest we add GetAll()

::: media/mtransport/third_party/nrappkit/src/log/r_log.c
@@ +330,5 @@
>      return(0);
>    }
>  
> +// Some platforms (notably WIN32) do not have this
> +#ifndef va_copy

Let's make this conditional on Win32, because there's no guarantee that va_copy() is actually assignment if it's undefined.
Attachment #814968 - Flags: review?(ekr) → review-
(In reply to Eric Rescorla (:ekr) from comment #85)
> Comment on attachment 814968 [details] [diff] [review]
> Part 5: Allow logging related to a given candidate pair to be fetched.
> 
> Review of attachment 814968 [details] [diff] [review]:

> @@ +27,5 @@
> > +RLogRingBuffer* RLogRingBuffer::instance;
> > +
> > +RLogRingBuffer::RLogRingBuffer()
> > +  : log_limit(4096),
> > +    mutex("RLogRingBuffer::mutex") {
> 
> Move this to .h?
>

     Could go either way. I generally don't like to put impl in the header file unless there's a performance reason to do so.
 
> @@ +31,5 @@
> > +    mutex("RLogRingBuffer::mutex") {
> > +}
> > +
> > +RLogRingBuffer::~RLogRingBuffer() {
> > +}
> 
> Let's omit this.
>

     Can omit. (This was just script-generated) 

> @@ +52,5 @@
> > +  va_copy(ap_copy, ap);
> > +  // Might be worthwhile to pre-alloc some room that usually is enough.
> > +  // First, we'd need to determine what a reasonable value for this would be.
> > +  int requiredSize = vsnprintf(nullptr, 0, format, ap_copy);
> > +  va_end(ap_copy);
> 
> Rather than do vsnprintf() twice, let's just use a 4k or so temp buffer.
> that's what r_log() does for its internal stuff anyway.
> Then you can avoid the va_copy() as well.
> 

     Torn between heap allocing each time, and having a long lifetime buffer that we reuse. Think I prefer the former, given the relatively low rate. We're already heap allocing now anyway.

> @@ +114,5 @@
> > +    // At a max, all of the log messages.
> > +    limit = log_limit;
> > +  }
> > +
> > +  for (auto log = log_messages.begin();
> 
> should we clear matching_logs()?
>

     Not sure. But if we don't, the != comparison needs to be replaced with <. I think this is my preference, as it is more flexible.

> @@ +118,5 @@
> > +  for (auto log = log_messages.begin();
> > +       log != log_messages.end() && matching_logs->size() != limit;
> > +       ++log) {
> > +    if (AnySubstringMatches(substrings, *log)) {
> > +      matching_logs->push_back(*log);
> 
> I feel like this is the opposite semantic from what we want. I.e,, if we do
> 
> log(A)
> log(B)
> 
> Then when someone asks for the logs they should come in in A, B but you seem
> to have
> the opposite.
> 
> Similarly, if I ask for limit 1 I should get A.
>

In both log_messages and matching_logs, front() is most recent. Limit 1 functions in the way you describe.

> @@ +75,5 @@
> > +       NB: These are not threadsafe, nor are they safe to call during static
> > +       init/deinit.
> > +    */
> > +    static RLogRingBuffer* GetInstance();
> > +    static void FreeInstance();
> 
> I suggest we do Setup/Teardown/Instance() with Instance checking for things
> being live.
>

  Is this just convention? If so, that's fine.
 
> @@ +16,5 @@
> > +#include <csi_platform.h>
> > +
> > +#include "nsAutoPtr.h"
> > +
> > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {
> 
> This seems like it can definitely be static.
>
> ::: media/mtransport/rlogringbuffer.h
> @@ +56,5 @@
> > +extern "C" {
> > +#include "r_log.h"
> > +
> > +/* Matches r_dest_vlog type defined in r_log.h */
> > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap);
> 
> Does this need to be extern?
> 
> @@ +93,5 @@
> > +                   std::deque<std::string>* matching_logs);
> > +
> > +    void SetLogLimit(uint32_t new_limit);
> > +
> > +    void Log(int facility, int level, const char *format, va_list ap);
> 
> Why not make this private and make the logger fxn a static class fxn.
> 

  Which is preferable; a non-exported function and a public Log(), or an exported function and a private Log()? The former is less stuff in the header file, which is good. Allowing someone to call RLogRingBuffer::Log() directly might not be a bad thing.

> ::: media/mtransport/test/ice_unittest.cpp
> @@ +1252,5 @@
> > +  std::deque<std::string> logs;
> > +  RLogRingBuffer::GetInstance()->Filter("CAND-PAIR", 0, &logs);
> > +  std::cerr << "Dumping CAND-PAIR logging:" << std::endl;
> > +  for (auto i = logs.rbegin(); i != logs.rend(); ++i) {
> > +    std::cerr << *i << std::endl;
> 
> Can you check that there is something here?
> 

  There's a patch later in the queue that does some more checking here.

> ::: media/mtransport/test/rlogringbuffer_unittest.cpp
> @@ +55,5 @@
> > +
> > +TEST_F(RLogRingBufferTest, TestFilterEmpty) {
> > +  std::deque<std::string> logs;
> > +  RLogRingBuffer::GetInstance()->Filter("", 0, &logs);
> > +  ASSERT_EQ(0U, logs.size());
> 
> How do you ask for everything? Filter("")?
> Suggest we add GetAll()
>

  That is what Filter("") does; this test case is grabbing everything in an empty ringbuffer, and verifying that it doesn't do anything silly.

> ::: media/mtransport/third_party/nrappkit/src/log/r_log.c
> @@ +330,5 @@
> >      return(0);
> >    }
> >  
> > +// Some platforms (notably WIN32) do not have this
> > +#ifndef va_copy
> 
> Let's make this conditional on Win32, because there's no guarantee that
> va_copy() is actually assignment if it's undefined.

  I can put an extra check here.
(In reply to Eric Rescorla (:ekr) from comment #85)
> @@ +31,5 @@
> > +    mutex("RLogRingBuffer::mutex") {
> > +}
> > +
> > +RLogRingBuffer::~RLogRingBuffer() {
> > +}
> 
> Let's omit this.

Actually, we need to define this explicitly if we want to make the d'tor private. I'd prefer this to be defined in impl if this is the case.
Attachment #816693 - Flags: checkin?(adam)
Update per ekr's review.
Attachment #814968 - Attachment is obsolete: true
Attachment #820493 - Attachment is obsolete: true
Attachment #820649 - Attachment is obsolete: true
(In reply to Byron Campen [:bwc] from comment #86)
> (In reply to Eric Rescorla (:ekr) from comment #85)
> > Comment on attachment 814968 [details] [diff] [review]
> > Part 5: Allow logging related to a given candidate pair to be fetched.
> > 
> > Review of attachment 814968 [details] [diff] [review]:
> 
> > @@ +27,5 @@
> > > +RLogRingBuffer* RLogRingBuffer::instance;
> > > +
> > > +RLogRingBuffer::RLogRingBuffer()
> > > +  : log_limit(4096),
> > > +    mutex("RLogRingBuffer::mutex") {
> > 
> > Move this to .h?
> >
> 
>      Could go either way. I generally don't like to put impl in the header
> file unless there's a performance reason to do so.

My preference is actually the opposite: constructers which are just
initializers go in the header. That said, I agree it's a style thing.



> > @@ +52,5 @@
> > > +  va_copy(ap_copy, ap);
> > > +  // Might be worthwhile to pre-alloc some room that usually is enough.
> > > +  // First, we'd need to determine what a reasonable value for this would be.
> > > +  int requiredSize = vsnprintf(nullptr, 0, format, ap_copy);
> > > +  va_end(ap_copy);
> > 
> > Rather than do vsnprintf() twice, let's just use a 4k or so temp buffer.
> > that's what r_log() does for its internal stuff anyway.
> > Then you can avoid the va_copy() as well.
> > 
> 
>      Torn between heap allocing each time, and having a long lifetime buffer
> that we reuse. Think I prefer the former, given the relatively low rate.
> We're already heap allocing now anyway.

Why not just use a stack buffer? 4k isn't big. This is what nrappkit
already does.


 
> > @@ +114,5 @@
> > > +    // At a max, all of the log messages.
> > > +    limit = log_limit;
> > > +  }
> > > +
> > > +  for (auto log = log_messages.begin();
> > 
> > should we clear matching_logs()?
> >
> 
>      Not sure. But if we don't, the != comparison needs to be replaced with
> <. I think this is my preference, as it is more flexible.

"this" == "your current code"?


> > @@ +118,5 @@
> > > +  for (auto log = log_messages.begin();
> > > +       log != log_messages.end() && matching_logs->size() != limit;
> > > +       ++log) {
> > > +    if (AnySubstringMatches(substrings, *log)) {
> > > +      matching_logs->push_back(*log);
> > 
> > I feel like this is the opposite semantic from what we want. I.e,, if we do
> > 
> > log(A)
> > log(B)
> > 
> > Then when someone asks for the logs they should come in in A, B but you seem
> > to have
> > the opposite.
> > 
> > Similarly, if I ask for limit 1 I should get A.
> >
> 
> In both log_messages and matching_logs, front() is most recent. Limit 1
> functions in the way you describe.


Per IRC discussion, please invert these. Oldest first.


> > @@ +75,5 @@
> > > +       NB: These are not threadsafe, nor are they safe to call during static
> > > +       init/deinit.
> > > +    */
> > > +    static RLogRingBuffer* GetInstance();
> > > +    static void FreeInstance();
> > 
> > I suggest we do Setup/Teardown/Instance() with Instance checking for things
> > being live.
> >
> 
>   Is this just convention? If so, that's fine.

Not really convention. However, in this case it's a pretty serious
logic error if we ever ask for an instance outside of the initial
setup and it's not there, so we should try to catch that


> > @@ +16,5 @@
> > > +#include <csi_platform.h>
> > > +
> > > +#include "nsAutoPtr.h"
> > > +
> > > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {
> > 
> > This seems like it can definitely be static.
> >
> > ::: media/mtransport/rlogringbuffer.h
> > @@ +56,5 @@
> > > +extern "C" {
> > > +#include "r_log.h"
> > > +
> > > +/* Matches r_dest_vlog type defined in r_log.h */
> > > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap);
> > 
> > Does this need to be extern?
> > 
> > @@ +93,5 @@
> > > +                   std::deque<std::string>* matching_logs);
> > > +
> > > +    void SetLogLimit(uint32_t new_limit);
> > > +
> > > +    void Log(int facility, int level, const char *format, va_list ap);
> > 
> > Why not make this private and make the logger fxn a static class fxn.
> > 
> 
>   Which is preferable; a non-exported function and a public Log(), or an
> exported function and a private Log()? The former is less stuff in the
> header file, which is good. Allowing someone to call RLogRingBuffer::Log()
> directly might not be a bad thing.

These are semi-orthogonal.

The logger for nrappkit does not need external linkage at all.
If this fxn is public, it can just be an ordinary static.
If this fxn is private, then it can be a class static so it
has access. I would actually recommend the following:

1. Make this fxn private.
2. Make a wrapper fxn that takes a std::string that is public.

People should not use varargs in C++ code if they can avoid it.




>   There's a patch later in the queue that does some more checking here.
> 
> > ::: media/mtransport/test/rlogringbuffer_unittest.cpp
> > @@ +55,5 @@
> > > +
> > > +TEST_F(RLogRingBufferTest, TestFilterEmpty) {
> > > +  std::deque<std::string> logs;
> > > +  RLogRingBuffer::GetInstance()->Filter("", 0, &logs);
> > > +  ASSERT_EQ(0U, logs.size());
> > 
> > How do you ask for everything? Filter("")?
> > Suggest we add GetAll()
> >
> 
>   That is what Filter("") does; this test case is grabbing everything in an
> empty ringbuffer, and verifying that it doesn't do anything silly.

OK, but you would have gotten the same result with Filter("THISDOESNOTEXIST");

I think it would be useful to have non-filtering sugar.


> > ::: media/mtransport/third_party/nrappkit/src/log/r_log.c
> > @@ +330,5 @@
> > >      return(0);
> > >    }
> > >  
> > > +// Some platforms (notably WIN32) do not have this
> > > +#ifndef va_copy
> > 
> > Let's make this conditional on Win32, because there's no guarantee that
> > va_copy() is actually assignment if it's undefined.
> 
>   I can put an extra check here.
Comment on attachment 821725 [details] [diff] [review]
Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging.

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

We're going to want something like this patch too if we want to use rlogringbuffer, but right now this is just a quick hack for initting the thing. Not entirely sure where the best place is to do this.
Attachment #821725 - Flags: feedback?(ekr)
Comment on attachment 821720 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched.

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

See issues below. Also, please adjust the setup/teardown/getinstance stuff as in previous
patch. Feel free to choose other names, but the point is that if ringbuffer_vlog() tries
to get an instance and one does no exist, this shoudl fail

::: media/mtransport/rlogringbuffer.cpp
@@ +17,5 @@
> +
> +#include "nsAutoPtr.h"
> +
> +/* Matches r_dest_vlog type defined in r_log.h */
> +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {

static int. We want internal linkage.

@@ +44,5 @@
> +void RLogRingBuffer::Log(int facility, int level, const char *format, va_list ap) {
> +  // I could be evil and printf right into a std::string, but unless this
> +  // shows up in profiling, it is not worth doing.
> +  static const size_t tempSize = 4096;
> +  nsAutoArrayPtr<char> temp(new char[tempSize]);

Please don't do a new alloc each time. This is the least efficient of the three
options of:

- stack buffer
- permanent alloc
- new alloc each time.

If you are afraid of the stack (you shouldn't be), then use a permanent
buffer. It will only be used in the writing thread in any case and
nrappkit only writes from one thread.

P.S. don't use nsAuto here. Scopedptrs please.
Attachment #821720 - Flags: review-
Comment on attachment 821725 [details] [diff] [review]
Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging.

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

This looks basically fine.

::: media/mtransport/nricectx.cpp
@@ +341,3 @@
>    if (!initialized) {
>      NR_reg_init(NR_REG_MODE_LOCAL);
> +    r_log_init();

Doesn't NR_reg_init() call r_log_init()?
Attachment #821725 - Flags: feedback?(ekr) → feedback+
(In reply to Eric Rescorla (:ekr) from comment #91)
> (In reply to Byron Campen [:bwc] from comment #86)
> > (In reply to Eric Rescorla (:ekr) from comment #85)
> > > Comment on attachment 814968 [details] [diff] [review]
> > > Part 5: Allow logging related to a given candidate pair to be fetched.
> > > 
> > > Review of attachment 814968 [details] [diff] [review]:
> > 
> > > @@ +52,5 @@
> > > > +  va_copy(ap_copy, ap);
> > > > +  // Might be worthwhile to pre-alloc some room that usually is enough.
> > > > +  // First, we'd need to determine what a reasonable value for this would be.
> > > > +  int requiredSize = vsnprintf(nullptr, 0, format, ap_copy);
> > > > +  va_end(ap_copy);
> > > 
> > > Rather than do vsnprintf() twice, let's just use a 4k or so temp buffer.
> > > that's what r_log() does for its internal stuff anyway.
> > > Then you can avoid the va_copy() as well.
> > > 
> > 
> >      Torn between heap allocing each time, and having a long lifetime buffer
> > that we reuse. Think I prefer the former, given the relatively low rate.
> > We're already heap allocing now anyway.
> 
> Why not just use a stack buffer? 4k isn't big. This is what nrappkit
> already does.

  4k on the stack feels big to me. But this may just be subjective.

> > > @@ +114,5 @@
> > > > +    // At a max, all of the log messages.
> > > > +    limit = log_limit;
> > > > +  }
> > > > +
> > > > +  for (auto log = log_messages.begin();
> > > 
> > > should we clear matching_logs()?
> > >
> > 
> >      Not sure. But if we don't, the != comparison needs to be replaced with
> > <. I think this is my preference, as it is more flexible.
> 
> "this" == "your current code"?
>

  My current code, with the != replaced with < (which is now the current state of the patch).
 

> > > @@ +118,5 @@
> > > > +  for (auto log = log_messages.begin();
> > > > +       log != log_messages.end() && matching_logs->size() != limit;
> > > > +       ++log) {
> > > > +    if (AnySubstringMatches(substrings, *log)) {
> > > > +      matching_logs->push_back(*log);
> > > 
> > > I feel like this is the opposite semantic from what we want. I.e,, if we do
> > > 
> > > log(A)
> > > log(B)
> > > 
> > > Then when someone asks for the logs they should come in in A, B but you seem
> > > to have
> > > the opposite.
> > > 
> > > Similarly, if I ask for limit 1 I should get A.
> > >
> > 
> > In both log_messages and matching_logs, front() is most recent. Limit 1
> > functions in the way you describe.
> 
> 
> Per IRC discussion, please invert these. Oldest first.

  Done.

> > > @@ +75,5 @@
> > > > +       NB: These are not threadsafe, nor are they safe to call during static
> > > > +       init/deinit.
> > > > +    */
> > > > +    static RLogRingBuffer* GetInstance();
> > > > +    static void FreeInstance();
> > > 
> > > I suggest we do Setup/Teardown/Instance() with Instance checking for things
> > > being live.
> > >
> > 
> >   Is this just convention? If so, that's fine.
> 
> Not really convention. However, in this case it's a pretty serious
> logic error if we ever ask for an instance outside of the initial
> setup and it's not there, so we should try to catch that

  Oh, you're talking about catching races on init. A little extra code, but could be worth it.

> 
> 
> > > @@ +16,5 @@
> > > > +#include <csi_platform.h>
> > > > +
> > > > +#include "nsAutoPtr.h"
> > > > +
> > > > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {
> > > 
> > > This seems like it can definitely be static.
> > >
> > > ::: media/mtransport/rlogringbuffer.h
> > > @@ +56,5 @@
> > > > +extern "C" {
> > > > +#include "r_log.h"
> > > > +
> > > > +/* Matches r_dest_vlog type defined in r_log.h */
> > > > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap);
> > > 
> > > Does this need to be extern?
> > > 
> > > @@ +93,5 @@
> > > > +                   std::deque<std::string>* matching_logs);
> > > > +
> > > > +    void SetLogLimit(uint32_t new_limit);
> > > > +
> > > > +    void Log(int facility, int level, const char *format, va_list ap);
> > > 
> > > Why not make this private and make the logger fxn a static class fxn.
> > > 
> > 
> >   Which is preferable; a non-exported function and a public Log(), or an
> > exported function and a private Log()? The former is less stuff in the
> > header file, which is good. Allowing someone to call RLogRingBuffer::Log()
> > directly might not be a bad thing.
> 
> These are semi-orthogonal.
> 
> The logger for nrappkit does not need external linkage at all.
> If this fxn is public, it can just be an ordinary static.
> If this fxn is private, then it can be a class static so it
> has access. I would actually recommend the following:
> 
> 1. Make this fxn private.
> 2. Make a wrapper fxn that takes a std::string that is public.
> 
> People should not use varargs in C++ code if they can avoid it.
> 

  I can make Log() take a std::string and move the vararg stuff to the (now non-exported) ringbuffer_vlog.

> > > ::: media/mtransport/test/rlogringbuffer_unittest.cpp
> > > @@ +55,5 @@
> > > > +
> > > > +TEST_F(RLogRingBufferTest, TestFilterEmpty) {
> > > > +  std::deque<std::string> logs;
> > > > +  RLogRingBuffer::GetInstance()->Filter("", 0, &logs);
> > > > +  ASSERT_EQ(0U, logs.size());
> > > 
> > > How do you ask for everything? Filter("")?
> > > Suggest we add GetAll()
> > >
> > 
> >   That is what Filter("") does; this test case is grabbing everything in an
> > empty ringbuffer, and verifying that it doesn't do anything silly.
> 
> OK, but you would have gotten the same result with
> Filter("THISDOESNOTEXIST");
> 
> I think it would be useful to have non-filtering sugar.
>

  In that particular test-case, yes. I can add an inline GetAny().
(In reply to Eric Rescorla (:ekr) from comment #94)
> Comment on attachment 821725 [details] [diff] [review]
> Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also,
> tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media
> packet logging.
> 
> Review of attachment 821725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks basically fine.
> 
> ::: media/mtransport/nricectx.cpp
> @@ +341,3 @@
> >    if (!initialized) {
> >      NR_reg_init(NR_REG_MODE_LOCAL);
> > +    r_log_init();
> 
> Doesn't NR_reg_init() call r_log_init()?

All I know is that it did not work until I called r_log_init(). I can dig deeper if you think it is worth it.
(In reply to Eric Rescorla (:ekr) from comment #93)
> Comment on attachment 821720 [details] [diff] [review]
> Part 5: Allow logging related to a given candidate pair to be fetched.
> 
> Review of attachment 821720 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> See issues below. Also, please adjust the setup/teardown/getinstance stuff
> as in previous
> patch. Feel free to choose other names, but the point is that if
> ringbuffer_vlog() tries
> to get an instance and one does no exist, this shoudl fail
> 
> ::: media/mtransport/rlogringbuffer.cpp
> @@ +17,5 @@
> > +
> > +#include "nsAutoPtr.h"
> > +
> > +/* Matches r_dest_vlog type defined in r_log.h */
> > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {
> 
> static int. We want internal linkage.
> 
> @@ +44,5 @@
> > +void RLogRingBuffer::Log(int facility, int level, const char *format, va_list ap) {
> > +  // I could be evil and printf right into a std::string, but unless this
> > +  // shows up in profiling, it is not worth doing.
> > +  static const size_t tempSize = 4096;
> > +  nsAutoArrayPtr<char> temp(new char[tempSize]);
> 
> Please don't do a new alloc each time. This is the least efficient of the
> three
> options of:
> 
> - stack buffer
> - permanent alloc
> - new alloc each time.
> 
> If you are afraid of the stack (you shouldn't be), then use a permanent
> buffer. It will only be used in the writing thread in any case and
> nrappkit only writes from one thread.
> 
> P.S. don't use nsAuto here. Scopedptrs please.

  I am principally concerned with memory footprint here since the frequency is so low (which is why I don't want to use permanent alloc). And allocing 4K on the stack makes me a little uncomfortable (wrt embedded), but I guess we're already doing much more elsewhere?

  Regarding the various smart pointer classes, has anyone put together a table of everything that's available, and what should be used for what?
(In reply to Byron Campen [:bwc] from comment #97)
> (In reply to Eric Rescorla (:ekr) from comment #93)
> > Comment on attachment 821720 [details] [diff] [review]
> > Part 5: Allow logging related to a given candidate pair to be fetched.
> > 
> > Review of attachment 821720 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > See issues below. Also, please adjust the setup/teardown/getinstance stuff
> > as in previous
> > patch. Feel free to choose other names, but the point is that if
> > ringbuffer_vlog() tries
> > to get an instance and one does no exist, this shoudl fail
> > 
> > ::: media/mtransport/rlogringbuffer.cpp
> > @@ +17,5 @@
> > > +
> > > +#include "nsAutoPtr.h"
> > > +
> > > +/* Matches r_dest_vlog type defined in r_log.h */
> > > +int ringbuffer_vlog(int facility, int level, const char *format, va_list ap) {
> > 
> > static int. We want internal linkage.
> > 
> > @@ +44,5 @@
> > > +void RLogRingBuffer::Log(int facility, int level, const char *format, va_list ap) {
> > > +  // I could be evil and printf right into a std::string, but unless this
> > > +  // shows up in profiling, it is not worth doing.
> > > +  static const size_t tempSize = 4096;
> > > +  nsAutoArrayPtr<char> temp(new char[tempSize]);
> > 
> > Please don't do a new alloc each time. This is the least efficient of the
> > three
> > options of:
> > 
> > - stack buffer
> > - permanent alloc
> > - new alloc each time.
> > 
> > If you are afraid of the stack (you shouldn't be), then use a permanent
> > buffer. It will only be used in the writing thread in any case and
> > nrappkit only writes from one thread.
> > 
> > P.S. don't use nsAuto here. Scopedptrs please.
> 
>   I am principally concerned with memory footprint here since the frequency
> is so low (which is why I don't want to use permanent alloc). And allocing
> 4K on the stack makes me a little uncomfortable (wrt embedded), but I guess
> we're already doing much more elsewhere?

Yes. 4k on the stack on every platform we use is fine.

That said, a 4k persistent allocation would also be fine and I bet
much less than the steady state of your ringbuffer.


>   Regarding the various smart pointer classes, has anyone put together a
> table of everything that's available, and what should be used for what?

I don't think so, but in general auto pointers are bad and Scoped pointers
are better.
(In reply to Byron Campen [:bwc] from comment #96)
> (In reply to Eric Rescorla (:ekr) from comment #94)
> > Comment on attachment 821725 [details] [diff] [review]
> > Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also,
> > tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media
> > packet logging.
> > 
> > Review of attachment 821725 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This looks basically fine.
> > 
> > ::: media/mtransport/nricectx.cpp
> > @@ +341,3 @@
> > >    if (!initialized) {
> > >      NR_reg_init(NR_REG_MODE_LOCAL);
> > > +    r_log_init();
> > 
> > Doesn't NR_reg_init() call r_log_init()?
> 
> All I know is that it did not work until I called r_log_init(). I can dig
> deeper if you think it is worth it.

Please, because:

http://dxr.mozilla.org/mozilla-central/source/media/mtransport/third_party/nrappkit/src/registry/registry.c?from=NR_reg_init#l105
Address more of ekr's review comments.
Attachment #821720 - Attachment is obsolete: true
<cstdarg> include landed in the wrong file.
Attachment #821845 - Attachment is obsolete: true
Attachment #814969 - Attachment is obsolete: true
Compensate for changes to part 5.
Attachment #820548 - Attachment is obsolete: true
Attachment #820550 - Attachment is obsolete: true
Compensate for changes to part 5.
Attachment #819931 - Attachment is obsolete: true
Attachment #821723 - Attachment is obsolete: true
Attachment #821725 - Attachment is obsolete: true
Attachment #821859 - Attachment is obsolete: true
Attachment #821848 - Attachment is obsolete: true
Comment on attachment 821855 [details] [diff] [review]
Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. r=ekr

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

Carry forward r+ from ekr.
Attachment #821855 - Flags: review+
Attachment #821868 - Flags: review?(ekr)
Attachment #821954 - Flags: review?(ekr)
Attachment #821954 - Attachment is patch: true
Attachment #821954 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 821954 [details] [diff] [review]
Interdiff between last review of candpair_log_scrape and most recent (bitrot got the old one)

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

lgtm pending verify that the r-value references work

::: b/media/mtransport/rlogringbuffer.cpp
@@ +54,4 @@
>    RemoveOld();
>  }
>  
> +void RLogRingBuffer::Log(std::string&& log) {

Please do a try push to verify that this works on all our platforms.
Attachment #821954 - Flags: review?(ekr) → review+
Comment on attachment 816693 [details] [diff] [review]
Part 4: Using more appropriate log-levels (r_log) for errors and other not-quite-right conditions.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b1c267db16c
Attachment #816693 - Flags: checkin?(adam) → checkin+
Pushed this stuff to try; a couple of tweaks are not reflected, but the r-value references were there.

https://tbpl.mozilla.org/?tree=Try&rev=85ceebc2dd85
Comment on attachment 821868 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr

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

ekr gave r+ to attached interdiff
Attachment #821868 - Flags: review?(ekr) → review+
Comment on attachment 821868 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr

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

ekr gave r+ to attached interdiff
Attachment #821868 - Flags: checkin?(adam)
Comment on attachment 821855 [details] [diff] [review]
Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. r=ekr

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

Carry forward r+ from ekr.
Attachment #821855 - Flags: checkin?(adam)
Comment on attachment 821860 [details] [diff] [review]
Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging.

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

Record ekr's over-the-shoulder review, and request checkin (made sure to double check that applying out of sequence works)
Attachment #821860 - Flags: review+
Attachment #821860 - Flags: checkin?(adam)
(just make sure to apply part 5 first)
Comment on attachment 821858 [details] [diff] [review]
Part 9. Add correlator for ICE candidates.

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

Byron, can you clean up

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
@@ +80,5 @@
> +    int r,_status;
> +    char *as_string=0;
> +
> +    if(r=nr_concat_strings(&as_string,cand->addr.as_string,"(",cand->label,")",NULL))
> +      ABORT(r);

Could we use snprintf to avoid the malloc? As long as labels aren't insanely long we should be OK.

@@ +192,5 @@
>  
>      /* Add the candidate to the isock list*/
>      TAILQ_INSERT_TAIL(&isock->candidates,cand,entry_sock);
>  
> +    nr_ice_candidate_compute_codeword(cand);

check for error.

@@ +240,5 @@
>      /* Bogus foundation */
>      if(!(cand->foundation=r_strdup(cand->addr.as_string)))
>        ABORT(r);
>  
> +    nr_ice_candidate_compute_codeword(cand);

check for error.

@@ +504,5 @@
>        default:
>          ABORT(R_INTERNAL);
>      }
>  
> +    nr_ice_candidate_compute_codeword(cand);

check for error.

::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.h
@@ +100,5 @@
>  
>  
>  int nr_ice_candidate_create(struct nr_ice_ctx_ *ctx,nr_ice_component *component, nr_ice_socket *isock, nr_socket *osock, nr_ice_candidate_type ctype, nr_ice_stun_server *stun_server, UCHAR component_id, nr_ice_candidate **candp);
>  int nr_ice_candidate_initialize(nr_ice_candidate *cand, NR_async_cb ready_cb, void *cb_arg);
> +void nr_ice_candidate_compute_codeword(nr_ice_candidate *cand);

Let's make this return int since it seems it can fail.

::: media/mtransport/third_party/nICEr/src/ice/ice_parser.c
@@ +331,5 @@
>        ABORT(R_BAD_DATA);
>      }
>  #endif
>  
> +    nr_ice_candidate_compute_codeword(cand);

Please check return value here.
(In reply to Eric Rescorla (:ekr) from comment #120)
> Comment on attachment 821858 [details] [diff] [review]
> Part 9. Add correlator for ICE candidates.
> 
> Review of attachment 821858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Byron, can you clean up
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
> @@ +80,5 @@
> > +    int r,_status;
> > +    char *as_string=0;
> > +
> > +    if(r=nr_concat_strings(&as_string,cand->addr.as_string,"(",cand->label,")",NULL))
> > +      ABORT(r);
> 
> Could we use snprintf to avoid the malloc? As long as labels aren't insanely
> long we should be OK.

  We could do that. You want me to go ahead and do the same for candidate pair codeword while I'm at it?

> >  int nr_ice_candidate_create(struct nr_ice_ctx_ *ctx,nr_ice_component *component, nr_ice_socket *isock, nr_socket *osock, nr_ice_candidate_type ctype, nr_ice_stun_server *stun_server, UCHAR component_id, nr_ice_candidate **candp);
> >  int nr_ice_candidate_initialize(nr_ice_candidate *cand, NR_async_cb ready_cb, void *cb_arg);
> > +void nr_ice_candidate_compute_codeword(nr_ice_candidate *cand);
> 
> Let's make this return int since it seems it can fail.

  I'll look it over; not sure how broken things need to be before this fails. The appropriate handling could range from setting the codeword to "oops" to asserting, just don't know yet. I was operating under the assumption that if things were broken enough to fail here, they'd probably fail harder later.
(In reply to Eric Rescorla (:ekr) from comment #120)
> Comment on attachment 821858 [details] [diff] [review]
> Part 9. Add correlator for ICE candidates.
> 
> Review of attachment 821858 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Byron, can you clean up
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.c
> @@ +80,5 @@
> > +    int r,_status;
> > +    char *as_string=0;
> > +
> > +    if(r=nr_concat_strings(&as_string,cand->addr.as_string,"(",cand->label,")",NULL))
> > +      ABORT(r);
> 
> Could we use snprintf to avoid the malloc? As long as labels aren't insanely
> long we should be OK.
> 
> @@ +192,5 @@
> >  
> >      /* Add the candidate to the isock list*/
> >      TAILQ_INSERT_TAIL(&isock->candidates,cand,entry_sock);
> >  
> > +    nr_ice_candidate_compute_codeword(cand);
> 
> check for error.
> 
> @@ +240,5 @@
> >      /* Bogus foundation */
> >      if(!(cand->foundation=r_strdup(cand->addr.as_string)))
> >        ABORT(r);
> >  
> > +    nr_ice_candidate_compute_codeword(cand);
> 
> check for error.
> 
> @@ +504,5 @@
> >        default:
> >          ABORT(R_INTERNAL);
> >      }
> >  
> > +    nr_ice_candidate_compute_codeword(cand);
> 
> check for error.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_candidate.h
> @@ +100,5 @@
> >  
> >  
> >  int nr_ice_candidate_create(struct nr_ice_ctx_ *ctx,nr_ice_component *component, nr_ice_socket *isock, nr_socket *osock, nr_ice_candidate_type ctype, nr_ice_stun_server *stun_server, UCHAR component_id, nr_ice_candidate **candp);
> >  int nr_ice_candidate_initialize(nr_ice_candidate *cand, NR_async_cb ready_cb, void *cb_arg);
> > +void nr_ice_candidate_compute_codeword(nr_ice_candidate *cand);
> 
> Let's make this return int since it seems it can fail.
> 
> ::: media/mtransport/third_party/nICEr/src/ice/ice_parser.c
> @@ +331,5 @@
> >        ABORT(R_BAD_DATA);
> >      }
> >  #endif
> >  
> > +    nr_ice_candidate_compute_codeword(cand);
> 
> Please check return value here.

It looks like doing this with snprintf() means that we don't really need to do any of the error-checking, unless we _really_ want to check whether the input to the codeword generation was truncated (I have a really hard time imagining a case where this would matter at all).
Incorporate feedback from w3c mailing list.
Attachment #821856 - Attachment is obsolete: true
Incorporate feedback from w3c mailing list.
Attachment #821857 - Attachment is obsolete: true
Incorporate feedback from ekr.
Attachment #821858 - Attachment is obsolete: true
Incorporate feedback from w3c mailing list.
Attachment #821862 - Attachment is obsolete: true
Comment on attachment 822627 [details] [diff] [review]
Part 7. Populate candidate pairs in RTCStatsReport.

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

I've tried to incorporate the feedback from the w3c thread here for just the stuff I've added (I know you'll be landing the other half pretty soon). Wanted to make sure that I didn't miss anything large.
Attachment #822627 - Flags: review?(jib)
Comment on attachment 822627 [details] [diff] [review]
Part 7. Populate candidate pairs in RTCStatsReport.

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

(Bleeding-edge) spec is changing fast here.

::: dom/media/PeerConnection.js
@@ +1149,5 @@
>      appendStats(dict.transportStats, report);
>      appendStats(dict.iceComponentStats, report);
> +    appendStats(dict.iceCandidatePairStats, report);
> +    appendStats(dict.localIceCandidateStats, report);
> +    appendStats(dict.remoteIceCandidateStats, report);

See above comment.

::: dom/webidl/RTCStatsReport.webidl
@@ -8,5 @@
>   */
>  
>  enum RTCStatsType {
>    "inbound-rtp",
> -  "outbound-rtp"

Please rebase to land on top of bug 902003 comment 35 patch without dashes, which is ready to land.

@@ +11,5 @@
>    "inbound-rtp",
> +  "outbound-rtp",
> +  "candidatepair",
> +  "local-candidate",
> +  "remote-candidate"

Remove dashes.

Also, do me a favor and add "session", "track" and "transport" ahead of these to match what just appeared on http://www.w3.org/2011/04/webrtc/wiki/Stats

@@ +74,5 @@
>  
> +enum RTCStatsIceCandidatePairState {
> +  "frozen",
> +  "waiting",
> +  "in_progress",

Remove underscore.

@@ +85,5 @@
> +  DOMString componentId;
> +  DOMString localCandidateId;
> +  DOMString remoteCandidateId;
> +  RTCStatsIceCandidatePairState state;
> +  unsigned long long priority;

I think I agree with http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0105.html to represent this as a long priority on the candidate instead.

@@ +88,5 @@
> +  RTCStatsIceCandidatePairState state;
> +  unsigned long long priority;
> +  boolean readable;
> +  boolean nominated; // Internal?
> +  boolean selected;  // Internal?

I think we should remove these two comments if we can't make them informative. 'nominated' and 'selected' now appear on the wiki FWIW (though possibly because we mentioned them).

@@ +128,5 @@
>    sequence<RTCTransportStats>         transportStats;
>    sequence<RTCIceComponentStats>      iceComponentStats;
> +  sequence<RTCIceCandidatePairStats>  iceCandidatePairStats;
> +  sequence<RTCIceCandidateStats>      localIceCandidateStats;
> +  sequence<RTCIceCandidateStats>      remoteIceCandidateStats;

Does keeping separate sequences internally for local and remote buy us anything? If not I would lump all into one iceCandidateStats.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1668,5 @@
>    if (!report) {
>      result = NS_ERROR_FAILURE;
>    }
> +  if (mMedia) {
> +    mozilla::RefPtr<NrIceMediaStream> mediaStream(

No need for mozilla:: and mozilla::dom:: since we're using those namespaces in this .cpp (please apply throughout).

@@ +1669,5 @@
>      result = NS_ERROR_FAILURE;
>    }
> +  if (mMedia) {
> +    mozilla::RefPtr<NrIceMediaStream> mediaStream(
> +        mMedia->ice_media_stream(trackId));

trackId may be 0 here if no selector was passed in, which is supposed to mean "give me all tracks". - I'm hoping to deal with this in the patch I'm working on now by adding a for-loop around all of this, so this is probably fine (though you'll surely need to rebase yet again).

@@ +1682,5 @@
> +        nsString codeword(NS_ConvertASCIItoUTF16(p->codeword.c_str()));
> +        s.mId.Construct(codeword);
> +        s.mTimestamp.Construct(now);
> +        s.mType.Construct(RTCStatsType::Candidatepair);
> +

mComponentId should be filled in. This appears to be just a named string, not a reference to another RTCStats (yet). From http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0093.html :
> I think it's an RTP component - in Chrome, they're named "audio",
> "video", "audio-rtcp" and "video-rtcp" if I remember rightly.

@@ +1685,5 @@
> +        s.mType.Construct(RTCStatsType::Candidatepair);
> +
> +        // Not quite right; need unique ids per candidate
> +        s.mLocalCandidateId.Construct(codeword);
> +        s.mRemoteCandidateId.Construct(codeword);

As comment says, you do need unique ids per candidate, as users expect to query the relevant RTCIceCandidateStats directly using stats[pair->mLocalCandidateId], per http://dev.w3.org/2011/webrtc/editor/webrtc.html#dictionary-rtcstats-members - I see an earlier patch added literal prefixes like "candpair_" here, how come you removed them?

@@ +1689,5 @@
> +        s.mRemoteCandidateId.Construct(codeword);
> +        s.mNominated.Construct(p->nominated);
> +        s.mPriority.Construct(p->priority);
> +        s.mSelected.Construct(p->selected);
> +        s.mState.Construct((RTCStatsIceCandidatePairState)p->state);

I prefer c++ cast (throughout):
s.mState.Construct(RTCStatsIceCandidatePairState(p->state));

@@ +1692,5 @@
> +        s.mSelected.Construct(p->selected);
> +        s.mState.Construct((RTCStatsIceCandidatePairState)p->state);
> +        report->mIceCandidatePairStats.Value().AppendElement(s);
> +
> +        mozilla::dom::RTCIceCandidateStats localCandidate;

Insert taste-disclaimer, but just observing that a shorter name like 'local' here for close proximity stack vars like this would make the code below look less like a wall of text, and wrap less.
Attachment #822627 - Flags: review?(jib) → review-
(In reply to Jan-Ivar Bruaroey [:jib] from comment #129)
> Comment on attachment 822627 [details] [diff] [review]
> Part 7. Populate candidate pairs in RTCStatsReport.
> 
> Review of attachment 822627 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +11,5 @@
> >    "inbound-rtp",
> > +  "outbound-rtp",
> > +  "candidatepair",
> > +  "local-candidate",
> > +  "remote-candidate"
> 
> Remove dashes.
>
> Also, do me a favor and add "session", "track" and "transport" ahead of
> these to match what just appeared on
> http://www.w3.org/2011/04/webrtc/wiki/Stats
>

  Was waiting to see what landed on your end before touching this; can clean up whatever remains.

> @@ +74,5 @@
> >  
> > +enum RTCStatsIceCandidatePairState {
> > +  "frozen",
> > +  "waiting",
> > +  "in_progress",
> 
> Remove underscore.
>

  Yeah, was wondering about that. Can do.
 
> @@ +85,5 @@
> > +  DOMString componentId;
> > +  DOMString localCandidateId;
> > +  DOMString remoteCandidateId;
> > +  RTCStatsIceCandidatePairState state;
> > +  unsigned long long priority;
> 
> I think I agree with
> http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0105.html to
> represent this as a long priority on the candidate instead.
>

  The pair priority isn't exactly a simple concatenation. If you know some details about the ICE setup (specifically, which endpoint is the controller), and assume no bugs, you can transform back and forth between the individual candidate priorities and candidate pair priority. I think it would be good to have both, honestly.
 
> @@ +88,5 @@
> > +  RTCStatsIceCandidatePairState state;
> > +  unsigned long long priority;
> > +  boolean readable;
> > +  boolean nominated; // Internal?
> > +  boolean selected;  // Internal?
> 
> I think we should remove these two comments if we can't make them
> informative. 'nominated' and 'selected' now appear on the wiki FWIW (though
> possibly because we mentioned them).
> 

  Yeah, these comments should go.

> @@ +128,5 @@
> >    sequence<RTCTransportStats>         transportStats;
> >    sequence<RTCIceComponentStats>      iceComponentStats;
> > +  sequence<RTCIceCandidatePairStats>  iceCandidatePairStats;
> > +  sequence<RTCIceCandidateStats>      localIceCandidateStats;
> > +  sequence<RTCIceCandidateStats>      remoteIceCandidateStats;
> 
> Does keeping separate sequences internally for local and remote buy us
> anything? If not I would lump all into one iceCandidateStats.
> 

  I wasn't sure about this one. We end up just lumping these two together when we expose the API, but maybe there is some potential value to keeping these separate.

> @@ +1682,5 @@
> > +        nsString codeword(NS_ConvertASCIItoUTF16(p->codeword.c_str()));
> > +        s.mId.Construct(codeword);
> > +        s.mTimestamp.Construct(now);
> > +        s.mType.Construct(RTCStatsType::Candidatepair);
> > +
> 
> mComponentId should be filled in. This appears to be just a named string,
> not a reference to another RTCStats (yet). From
> http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0093.html :
> > I think it's an RTP component - in Chrome, they're named "audio",
> > "video", "audio-rtcp" and "video-rtcp" if I remember rightly.
>

  Hmm. I'll look into what we can set here.

> @@ +1685,5 @@
> > +        s.mType.Construct(RTCStatsType::Candidatepair);
> > +
> > +        // Not quite right; need unique ids per candidate
> > +        s.mLocalCandidateId.Construct(codeword);
> > +        s.mRemoteCandidateId.Construct(codeword);
> 
> As comment says, you do need unique ids per candidate, as users expect to
> query the relevant RTCIceCandidateStats directly using
> stats[pair->mLocalCandidateId], per
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#dictionary-rtcstats-members
> - I see an earlier patch added literal prefixes like "candpair_" here, how
> come you removed them?
>

  There is a later patch (part 9) that adds a codeword field to individual candidates, and uses those values instead. I removed the prefix because the codewords were unique enough, and so I would not need to parse/split the ids when it came time to scrape the logging for stuff related to a given candidate or pair.
 
> @@ +1692,5 @@
> > +        s.mSelected.Construct(p->selected);
> > +        s.mState.Construct((RTCStatsIceCandidatePairState)p->state);
> > +        report->mIceCandidatePairStats.Value().AppendElement(s);
> > +
> > +        mozilla::dom::RTCIceCandidateStats localCandidate;
> 
> Insert taste-disclaimer, but just observing that a shorter name like 'local'
> here for close proximity stack vars like this would make the code below look
> less like a wall of text, and wrap less.

  As you may have noticed, I tend to opt for maximal specificity by default. But, in this case "local" and "remote" are probably clear enough.
Unbitrot, so interdiff will work for the changes I'm about to make.
Attachment #822627 - Attachment is obsolete: true
Incorporate feedback from jib.
Attachment #823452 - Attachment is obsolete: true
Attachment #823516 - Flags: review?(jib)
Attachment #822628 - Attachment is obsolete: true
Attachment #822631 - Attachment is obsolete: true
Attachment #822629 - Flags: review?(ekr)
Comment on attachment 821868 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr

https://hg.mozilla.org/integration/mozilla-inbound/rev/8e3a17767287
Attachment #821868 - Flags: checkin?(adam) → checkin+
Comment on attachment 821855 [details] [diff] [review]
Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. r=ekr

https://hg.mozilla.org/integration/mozilla-inbound/rev/30a5abc9fc8c
Attachment #821855 - Flags: checkin?(adam) → checkin+
Comment on attachment 821860 [details] [diff] [review]
Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1cbb486b4fb7
Attachment #821860 - Flags: checkin?(adam) → checkin+
Attachment #823520 - Attachment is obsolete: true
Fix a bug here that was fixed differently in a later patch.
Attachment #823516 - Attachment is obsolete: true
Attachment #823516 - Flags: review?(jib)
Attachment #823518 - Attachment is obsolete: true
Attachment #823541 - Attachment is obsolete: true
Attachment #823590 - Flags: review?(jib)
Comment on attachment 823590 [details] [diff] [review]
Part 7. Populate candidate pairs in RTCStatsReport.

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

Looks good to me with a few nits.

::: dom/webidl/RTCStatsReport.webidl
@@ +89,5 @@
> +  DOMString componentId;
> +  DOMString localCandidateId;
> +  DOMString remoteCandidateId;
> +  RTCStatsIceCandidatePairState state;
> +  unsigned long long priority;

I would use mozPriority pending discussion on the list.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1642,5 @@
> +      report->mIceCandidateStats.Construct();
> +      NS_ConvertASCIItoUTF16 componentId(mediaStream->name().c_str());
> +      for (auto p = candPairs.begin(); p != candPairs.end(); ++p) {
> +        RTCIceCandidatePairStats s;
> +        nsString codeword(NS_ConvertASCIItoUTF16(p->codeword.c_str()));

NS_ConvertASCIItoUTF16 codeword(p->codeword.c_str());

@@ +1648,5 @@
> +            NS_ConvertASCIItoUTF16("local_") + codeword);
> +        const nsString remoteCodeword(
> +            NS_ConvertASCIItoUTF16("remote_") + codeword);
> +        s.mId.Construct(codeword);
> +        s.mComponentId.Construct(componentId);

I would inline componentId like you do below for mCandidateType.

@@ +1652,5 @@
> +        s.mComponentId.Construct(componentId);
> +        s.mTimestamp.Construct(now);
> +        s.mType.Construct(RTCStatsType::Candidatepair);
> +
> +        // Not quite right; need unique ids per candidate

Update or remove comment.

@@ +1681,5 @@
> +            RTCStatsIceCandidateType(p->remote.type));
> +        remote.mIpAddress.Construct(
> +            NS_ConvertASCIItoUTF16(p->remote.host.c_str()));
> +        remote.mPortNumber.Construct(p->remote.port);
> +        report->mIceCandidateStats.Value().AppendElement(remote);

I would use {}-brackets to limit the scope of the stack variables 'local' and 'remote' and their use, to prevent accidental mix-ups which are comment with near-identical code. Others may differ. Up to you.
Attachment #823590 - Flags: review?(jib) → review+
s/comment/common/
Attachment #821860 - Attachment is obsolete: true
Since we aren't using this mutex anyway, remove it so we don't leak it. We still have a one-time memory leak, but this is not as big of a deal.
Attachment #823631 - Attachment is obsolete: true
Attachment #823590 - Attachment is obsolete: true
Attachment #823591 - Attachment is obsolete: true
Attachment #823592 - Attachment is obsolete: true
(In reply to Jan-Ivar Bruaroey [:jib] from comment #142)
> Comment on attachment 823590 [details] [diff] [review]
> Part 7. Populate candidate pairs in RTCStatsReport.
> 
> Review of attachment 823590 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me with a few nits.
> @@ +1648,5 @@
> > +            NS_ConvertASCIItoUTF16("local_") + codeword);
> > +        const nsString remoteCodeword(
> > +            NS_ConvertASCIItoUTF16("remote_") + codeword);
> > +        s.mId.Construct(codeword);
> > +        s.mComponentId.Construct(componentId);
> 
> I would inline componentId like you do below for mCandidateType.
>

  We're setting these in two places each as soon as the next patch is applied.

  I fixed the other nits though.
Comment on attachment 823649 [details] [diff] [review]
Part 7. Populate candidate pairs in RTCStatsReport. r=jib

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

Carry forward r+ from jib
Attachment #823649 - Flags: review+
Attachment #823645 - Attachment is obsolete: true
Attachment #823655 - Flags: review?(jib)
Comment on attachment 823687 [details] [diff] [review]
Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. r=ekr

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

Carry forward r+ from ekr.
Attachment #823687 - Flags: review+
Changing a couple of checks to something more terse.
Attachment #823656 - Attachment is obsolete: true
Attachment #823687 - Attachment description: Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. → Part 11. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. r=ekr
Attachment #821855 - Attachment description: Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. → Part 6: Add a codeword field to NrIceCandidatePair so related logging can be pulled from RLogRingBuffer. Proof of concept in ice_unittest. r=ekr
Attachment #821868 - Attachment description: Part 5: Allow logging related to a given candidate pair to be fetched. → Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr
Attachment #823649 - Attachment description: Part 7. Populate candidate pairs in RTCStatsReport. → Part 7. Populate candidate pairs in RTCStatsReport. r=jib
Attachment #824067 - Flags: review?(jib)
Comment on attachment 823655 [details] [diff] [review]
Part 8. Create a chrome-only stats interface, and only expose the candidate pair stats there.

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

r- for wait: false.

Also, how can you call getStatsInternal()? Aren't you missing some changes in RTCPeerConnection.webidl in this patch?

::: dom/media/PeerConnection.js
@@ +722,5 @@
>      this._queueOrRun({
>        func: this._getStats,
> +      args: [selector, onSuccess, onError, false],
> +//      wait: true
> +      wait: false

This seems wrong. This patch makes pairs internal-only, but everything else, including the gathering of RTCIceCandidateStats follows the same path, so this shouldn't alter whether we wait.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1642,5 @@
>        std::vector<NrIceCandidatePair> candPairs;
>        mediaStream->GetCandidatePairs(&candPairs);
> +      if (internalStats) {
> +        report->mIceCandidatePairStats.Construct();
> +      }

I wouldn't bother with an if here, as it doesn't hurt to construct the empty array always.

@@ +1652,5 @@
>              NS_ConvertASCIItoUTF16("local_") + codeword);
>          const nsString remoteCodeword(
>              NS_ConvertASCIItoUTF16("remote_") + codeword);
> +        // Only expose candidate-pair statistics to chrome, until we've thought
> +        // through the implications of exposing it to content.

I'm good with this approach, but maybe open a bug to track and discuss this? Separately you might also want to raise this on the list, as most of this is in the spec already.

Also, you have a comment typo: "Only, once" or "Don't, until".

@@ +1657,3 @@
>  
> +        if (internalStats) {
> +          mozilla::dom::RTCIceCandidatePairStats s;

Namespace not needed.

@@ +1661,5 @@
> +          s.mComponentId.Construct(componentId);
> +          s.mTimestamp.Construct(now);
> +          s.mType.Construct(RTCStatsType::Candidatepair);
> +
> +          // Not quite right; we end up with duplicate candidates. Will fix.

Maybe move this comment outside the if since it affects the candidates not the pairs. Will fix when? - a "TODO:" prefix + bug # is common.
Attachment #823655 - Flags: review?(jib) → review-
(In reply to Jan-Ivar Bruaroey [:jib] from comment #155)
> Comment on attachment 823655 [details] [diff] [review]
> Part 8. Create a chrome-only stats interface, and only expose the candidate
> pair stats there.
> 
> Review of attachment 823655 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for wait: false.
> 
> Also, how can you call getStatsInternal()? Aren't you missing some changes
> in RTCPeerConnection.webidl in this patch?
> 

  I think right now getStatsInternal() is not exposed via webidl; we only call WebrtcGlobal::getAllStats(), which then backends out to getStatsInternal() under the hood. I can expose it ChromeOnly in webidl though, since that might be useful.

> ::: dom/media/PeerConnection.js
> @@ +722,5 @@
> >      this._queueOrRun({
> >        func: this._getStats,
> > +      args: [selector, onSuccess, onError, false],
> > +//      wait: true
> > +      wait: false
> 
> This seems wrong. This patch makes pairs internal-only, but everything else,
> including the gathering of RTCIceCandidateStats follows the same path, so
> this shouldn't alter whether we wait.
> 

  I think I must have had this from a very early patch on 902003 or something. Will fix.

> ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
> @@ +1642,5 @@
> >        std::vector<NrIceCandidatePair> candPairs;
> >        mediaStream->GetCandidatePairs(&candPairs);
> > +      if (internalStats) {
> > +        report->mIceCandidatePairStats.Construct();
> > +      }
> 
> I wouldn't bother with an if here, as it doesn't hurt to construct the empty
> array always.
> 

  Ok, wasn't sure how this would effect the JS code. Can remove the check.

> @@ +1652,5 @@
> >              NS_ConvertASCIItoUTF16("local_") + codeword);
> >          const nsString remoteCodeword(
> >              NS_ConvertASCIItoUTF16("remote_") + codeword);
> > +        // Only expose candidate-pair statistics to chrome, until we've thought
> > +        // through the implications of exposing it to content.
> 
> I'm good with this approach, but maybe open a bug to track and discuss this?
> Separately you might also want to raise this on the list, as most of this is
> in the spec already.
> 

  Yeah, we need to chase this down. We should probably raise on the list as a concern, and at the same time carry out some in-house analysis in case the question doesn't get much traction on list (and so we can give some useful input into the process).

> Also, you have a comment typo: "Only, once" or "Don't, until".
> 

  I'm pretty sure the comment is correct as-is.

> @@ +1661,5 @@
> > +          s.mComponentId.Construct(componentId);
> > +          s.mTimestamp.Construct(now);
> > +          s.mType.Construct(RTCStatsType::Candidatepair);
> > +
> > +          // Not quite right; we end up with duplicate candidates. Will fix.
> 
> Maybe move this comment outside the if since it affects the candidates not
> the pairs. Will fix when? - a "TODO:" prefix + bug # is common.

  The fix is in part 10.
Attachment #821855 - Flags: checkin+
Attachment #821868 - Flags: checkin+
Attachment #823655 - Attachment is obsolete: true
Attachment #824127 - Flags: review?(jib)
(In reply to Byron Campen [:bwc] from comment #156)
> (In reply to Jan-Ivar Bruaroey [:jib] from comment #155)
> > Also, you have a comment typo: "Only, once" or "Don't, until".
> 
>   I'm pretty sure the comment is correct as-is.

Yeah never mind, what I said made no sense.

> > Maybe move this comment outside the if since it affects the candidates not
> > the pairs. Will fix when? - a "TODO:" prefix + bug # is common.
> 
>   The fix is in part 10.

OK fair enough.
Attachment #821868 - Attachment is obsolete: true
Comment on attachment 824146 [details] [diff] [review]
Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr

Carry forward r+ from ekr.
Attachment #824146 - Attachment description: Part 5: Allow logging related to a given candidate pair to be fetched. → Part 5: Allow logging related to a given candidate pair to be fetched. r=ekr
Attachment #824146 - Flags: review+
Attachment #824127 - Flags: review?(jib) → review+
Attachment #823687 - Attachment is obsolete: true
Comment on attachment 824169 [details] [diff] [review]
Part 5.1. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. r=ekr

Carry forward r+ from ekr.
Attachment #824169 - Attachment description: Part 5.1. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. → Part 5.1. Enable r_log and RLogRingBuffer so logging can be scraped. Also, tweak log levels so the RLogRingBuffer isn't rapidly overwritten by media packet logging. r=ekr
Attachment #824169 - Flags: review+
Attachment #824067 - Attachment is obsolete: true
Attachment #824067 - Flags: review?(jib)
Attachment #824224 - Flags: review?(jib)
Make getPcid() ChromeOnly, and fix the wait flag on getLogging()
Attachment #824224 - Attachment is obsolete: true
Attachment #824224 - Flags: review?(jib)
Attachment #824252 - Flags: review?(jib)
Attachment #824252 - Attachment is obsolete: true
Attachment #824252 - Flags: review?(jib)
Attachment #824270 - Flags: review?(jib)
Comment on attachment 824270 [details] [diff] [review]
Part 10. Webidl and implementation for WebrtcGlobal. Encompasses things like global stats and logging.

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

My top-level concerns are:
- Is a global (ChromeOnly) webrtc object the right approach?
- Why are we lifting logs out to JS? Aren't stats and regular log dumps sufficient?

I would like more feedback from the team on this before proceeding.

Also, a DOM reviewer should look at this.

Those issues aside, the code itself looks good with a couple of nits.

::: dom/media/PeerConnection.js
@@ +117,5 @@
>      }
>    },
> +
> +  getStatsForEachPC: function(callback, errorCallback) {
> +    for (var winId in this._list) {

use let instead of var throughout.

@@ +119,5 @@
> +
> +  getStatsForEachPC: function(callback, errorCallback) {
> +    for (var winId in this._list) {
> +      this._list[winId].forEach(function(pcref) {
> +        pcref.get().getStatsInternal(null, callback, errorCallback);

Either call this.removeNullRefs(winID) ahead of the forEach, or check pcref against against null, since peerconnections may have been garbage-collected away (they are weak pointers).

@@ +126,5 @@
> +  },
> +
> +  getLoggingFromFirstPC: function(pattern, callback, errorCallback) {
> +    for (var winId in this._list) {
> +      if (this._list[winId].length > 0) {

Same here. Calling this.removeNullRefs(winID) is probably simplest here.

::: dom/webidl/RTCPeerConnection.webidl
@@ +142,5 @@
> +
> +[JSImplementation="@mozilla.org/dom/webrtcglobal;1",
> + ChromeOnly,
> + Constructor ()]
> +interface WebrtcGlobal {

I don't see a precedent for teams creating their own globals, even ChromeOnly (though perhaps I'm not looking hard enough), so I would like to hear from the team before moving forward here.

Our global methods so far, mozGetUserMedia and (the ChromeOnly) mozGetUserMediaDevices, hang off Navigator: http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#341

Though, since there's no content version of the API in this case, perhaps polluting Navigator isn't desirable either?

@@ +145,5 @@
> + Constructor ()]
> +interface WebrtcGlobal {
> +    void getAllStats(RTCStatsCallback callback,
> +                     RTCPeerConnectionErrorCallback errorCallback);
> +    void getCandPairLogs(DOMString codeword,

codeword=pattern? A comment would help.

@@ +147,5 @@
> +    void getAllStats(RTCStatsCallback callback,
> +                     RTCPeerConnectionErrorCallback errorCallback);
> +    void getCandPairLogs(DOMString codeword,
> +                         RTCLogCallback callback,
> +                         RTCPeerConnectionErrorCallback errorCallback);

Big issue: Why are we lifting logs out to JS? Aren't stats and regular log dumps sufficient?

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1762,5 @@
> +
> +void PeerConnectionImpl::OnGetLogging_m(const std::string& pattern,
> +                                        const std::deque<std::string>& logging) {
> +  PeerConnectionObserver* pco = mPCObserver.MayGet();
> +  if (pco) {

Early-return pattern is used elsewhere in this file. I would use it here as well.

@@ +1773,5 @@
> +      pco->OnGetLoggingSuccess(nsLogs, rv);
> +    } else {
> +      pco->OnGetLoggingError(kInternalError,
> +                             ObString("No logging matching pattern ") +
> +                             ObString(pattern.c_str()), rv);

Nit. I think I prefer:

  ObString(std::string("No logging matching pattern ") + pattern).c_str())

here, just because the ObString typedef exists for the unit-tests (where it is  const char *). I know we're in MOZILLA_INTERNAL_API here, so it works, but still :-)

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h
@@ +309,5 @@
>    }
>  
> +  NS_IMETHODIMP GetLogging(const std::string& pattern);
> +  NS_IMETHODIMP_TO_ERRORRESULT(GetLogging, ErrorResult &rv,
> +                               const nsAString& pattern)

Note that NS_IMETHODIMP_TO_ERRORRESULT is a #define in this same file that expands to declaring the inner function for you, so you have one too many since you declare it manually as well.

For clarity, I'd prefer if we kept std::string out at this level of the API and moved the string conversion into GetLogging() in the .cpp, so there's no confusion about which API expects which string type.

That way the macro works and you can remove your stub declaration above.
Attachment #824270 - Flags: review?(jib) → feedback?(rjesup)
(In reply to Jan-Ivar Bruaroey [:jib] from comment #167)
> Comment on attachment 824270 [details] [diff] [review]
> Part 10. Webidl and implementation for WebrtcGlobal. Encompasses things like
> global stats and logging.
> 
> Review of attachment 824270 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> My top-level concerns are:
> - Is a global (ChromeOnly) webrtc object the right approach?
> - Why are we lifting logs out to JS? Aren't stats and regular log dumps
> sufficient?
> 
> I would like more feedback from the team on this before proceeding.
> 
> Also, a DOM reviewer should look at this.
> 
> Those issues aside, the code itself looks good with a couple of nits.
> 
> ::: dom/webidl/RTCPeerConnection.webidl
> @@ +142,5 @@
> > +
> > +[JSImplementation="@mozilla.org/dom/webrtcglobal;1",
> > + ChromeOnly,
> > + Constructor ()]
> > +interface WebrtcGlobal {
> 
> I don't see a precedent for teams creating their own globals, even
> ChromeOnly (though perhaps I'm not looking hard enough), so I would like to
> hear from the team before moving forward here.
> 
> Our global methods so far, mozGetUserMedia and (the ChromeOnly)
> mozGetUserMediaDevices, hang off Navigator:
> http://mxr.mozilla.org/mozilla-central/source/dom/webidl/Navigator.webidl#341
> 
> Though, since there's no content version of the API in this case, perhaps
> polluting Navigator isn't desirable either?
> 

  I'm ok with moving this, but I wanted to have it isolated to simplify things. Moving it would (probably?) necessitate moving the global pc list as well, right? WebrtcGlobal really isn't a global object after all, it just piggybacks on the existing global PC list.

> @@ +147,5 @@
> > +    void getAllStats(RTCStatsCallback callback,
> > +                     RTCPeerConnectionErrorCallback errorCallback);
> > +    void getCandPairLogs(DOMString codeword,
> > +                         RTCLogCallback callback,
> > +                         RTCPeerConnectionErrorCallback errorCallback);
> 
> Big issue: Why are we lifting logs out to JS? Aren't stats and regular log
> dumps sufficient?
>

  Have already talked about this in IRC, but will write down the motivation here as well. We want to have the ability to display very granular log dumps on an about:webrtc page (eg; user clicks on a candidate pair in a table and gets the related logging), regardless of how they configured the logging system.