Not sure how this is different than 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.

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.

We can modify nICEr.

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.

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....

Slight tweak; not sure whether we had UINT64_MAX somewhere in the portability layer, so used numeric_limits instead.

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)

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

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?

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

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)

Okay, I'm queued up to check this in as soon as the tree re-opens.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cfb20a0a9857 https://hg.mozilla.org/integration/mozilla-inbound/rev/a850655f1ae1

https://hg.mozilla.org/mozilla-central/rev/cfb20a0a9857 https://hg.mozilla.org/mozilla-central/rev/a850655f1ae1

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?

> > @@ +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.

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.

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?

(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)

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.

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.

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).

Unbitrotting

Unbitrotting

Unbitrotting

Unbitrotting

Unbitrotting

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.

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.

Folded Part 7 (nits) into this, incorporated more review feedback, and unbitrotted.

Comment on attachment 814970 [details] [diff] [review] Part 7: Nits from (ice_candpair_logging_work.patch got r+) Obsoleting. Folded into part 3.

Compensate for folding.

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.

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

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

Move something small from 902003 here.

Compensate for changes in 902003

Bring up-to-date with patches landed from 902003.

Bring up-to-date with patches landed from 902003.

Unbitrot

Unbitrot

Unbitrot

Unbitrot.

Unbitrot.

Update unit-test to use LOG_INFO, since that's the level RLogRingBuffer grabs now.

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

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.

(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.

Update per ekr's review.