Issue 41001: Bug 906990: Bcampen's refactor of some stats
DescriptionBug 906990: Bcampen's refactor of some stats Patch Set 1 #
Total comments: 47
Patch Set 2 : Revised patches from bcampen #
Total comments: 3
Patch Set 3 : Another revision #
Total comments: 4
Patch Set 4 : Another revision #
Created: 7 years ago
MessagesTotal messages: 9
https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); So this looks like the callback gets called multiple times? How do we know that all the calls have completed? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/mediapipeline/MediaPipeline.h (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:200: int level_; // The m-line index Please add a comment about whether it is 0 or 1 indexed. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1248: new std::vector<RefPtr<NrIceMediaStream> >); This AutoPtr seems like overkill. Suggest just passing the vector. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1252: RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(level-1)); This is going to interact very badly with bundle. Since we're already planning that, let's futureproof this now. The obvious thing to do here is instead of asking for the level, to ask for the transport and then look into that for the TransportLayerIce. I would be open to other alternatives, but... https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1254: streams->push_back(temp); How can this fail? Perhaps we should be reporting an error? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1262: nsRefPtr<PeerConnectionImpl> pc(this); This is no longer needed, right? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1876: RefPtr<NrIceCtx> iceCtx, This can be a const &, right? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1931: mediaStream.GetCandidatePairs(&candPairs); Check for error. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); I would prefer to hide the handle entirely. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1988: } I am assuming this code is just reindented, so not reviewing it in this cycle. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1992: const std::string& pcHandle, A comment that this is not a ref passed through the thread boundary seems in order. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2002: std::string subHandle = pcHandle.substr(8, std::string::npos); See above abour the handle. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2009: nsresult rv = report ? GetStatsImpl_s(internalStats, How can report be 0? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2059: logs->Filter(pattern, 0, &result); Now this seems like a good use for an auto_ptr since we are copying a vector of a pile of strings.
Sign in to reply to this message.
Some replies, new code in a few. https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); On 2013/12/29 17:19:55, ekr-webrtc wrote: > So this looks like the callback gets called multiple times? How do we know that > all the calls have completed? We don't, but we don't really care. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/mediapipeline/MediaPipeline.h (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/mediapipeline/MediaPipeline.h:200: int level_; // The m-line index On 2013/12/29 17:19:55, ekr-webrtc wrote: > Please add a comment about whether it is 0 or 1 indexed. Ok. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1248: new std::vector<RefPtr<NrIceMediaStream> >); On 2013/12/29 17:19:55, ekr-webrtc wrote: > This AutoPtr seems like overkill. Suggest just passing the vector. Was just doing this for consistency; ordinarily I think I agree with you. Copy constructing RefPtr should be pretty cheap, right? I can change both. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1252: RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(level-1)); On 2013/12/29 17:19:55, ekr-webrtc wrote: > This is going to interact very badly with bundle. Since we're already planning > that, let's futureproof this now. The obvious thing to do here is instead of > asking for the level, to ask for the transport and then look into that for the > TransportLayerIce. > > I would be open to other alternatives, but... Maybe we could have TransportFlow expose some API for this. Ultimately, this can be done when it comes time for bundle (in fact, if I did this in this patch, I would bitrot my work in progress for bundle). I think it makes sense to refine this when it comes time. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1254: streams->push_back(temp); On 2013/12/29 17:19:55, ekr-webrtc wrote: > How can this fail? Perhaps we should be reporting an error? I'm not sure what real-world cases could cause this. Logging something probably makes sense though. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1262: nsRefPtr<PeerConnectionImpl> pc(this); On 2013/12/29 17:19:55, ekr-webrtc wrote: > This is no longer needed, right? Nope, missed this one. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1876: RefPtr<NrIceCtx> iceCtx, On 2013/12/29 17:19:55, ekr-webrtc wrote: > This can be a const &, right? Review tool says this comment applies to the RefPtr<NrIceCtx>, is this what you meant? I am already going to change the nsAutoPtr to const &. The NrIceCtx probably has to remain a RefPtr, since the WrapRunnable won't auto-convert for us, and we need to keep the context alive. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1931: mediaStream.GetCandidatePairs(&candPairs); On 2013/12/29 17:19:55, ekr-webrtc wrote: > Check for error. Ok. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); On 2013/12/29 17:19:55, ekr-webrtc wrote: > I would prefer to hide the handle entirely. Do we need to come up with a second random identifier? Or is there something more descriptive we could use? If we don't want that data leaking, we should probably build the stuff returned by name() differently, and use the whole thing here. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1988: } On 2013/12/29 17:19:55, ekr-webrtc wrote: > I am assuming this code is just reindented, so not reviewing it in this cycle. Not quite, I removed a comment, and set mComponentId on each RTCIceCandidateStats. Here's a whitespace-insensitive diff: for (auto p = candPairs.begin(); p != candPairs.end(); ++p) { NS_ConvertASCIItoUTF16 codeword(p->codeword.c_str()); NS_ConvertASCIItoUTF16 localCodeword(p->local.codeword.c_str()); @@ -1921,7 +1955,6 @@ PeerConnectionImpl::GetStatsImpl_s( s.mTimestamp.Construct(now); s.mType.Construct(RTCStatsType::Candidatepair); - // Not quite right; we end up with duplicate candidates. Will fix. s.mLocalCandidateId.Construct(localCodeword); s.mRemoteCandidateId.Construct(remoteCodeword); s.mNominated.Construct(p->nominated); @@ -1933,6 +1966,7 @@ PeerConnectionImpl::GetStatsImpl_s( { RTCIceCandidateStats local; + local.mComponentId.Construct(componentId); local.mId.Construct(localCodeword); local.mTimestamp.Construct(now); local.mType.Construct(RTCStatsType::Localcandidate); @@ -1946,6 +1980,7 @@ PeerConnectionImpl::GetStatsImpl_s( { RTCIceCandidateStats remote; + remote.mComponentId.Construct(componentId); remote.mId.Construct(remoteCodeword); remote.mTimestamp.Construct(now); remote.mType.Construct(RTCStatsType::Remotecandidate); @@ -1958,27 +1993,36 @@ PeerConnectionImpl::GetStatsImpl_s( } } } https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1992: const std::string& pcHandle, On 2013/12/29 17:19:55, ekr-webrtc wrote: > A comment that this is not a ref passed through the thread boundary seems in > order. Ok. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2002: std::string subHandle = pcHandle.substr(8, std::string::npos); On 2013/12/29 17:19:55, ekr-webrtc wrote: > See above abour the handle. Is there something nice and human-readable we can use here? I can just create a second random string and store alongside mHandle for now, but this will involve adding another parameter to a few functions. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2009: nsresult rv = report ? GetStatsImpl_s(internalStats, On 2013/12/29 17:19:55, ekr-webrtc wrote: > How can report be 0? Dunno. I think the check was there before. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2059: logs->Filter(pattern, 0, &result); On 2013/12/29 17:19:55, ekr-webrtc wrote: > Now this seems like a good use for an auto_ptr since we are copying a vector of > a pile of strings. Sure, why not.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > So this looks like the callback gets called multiple times? How do we know > that > > all the calls have completed? > > We don't, but we don't really care. That doesn't match any of our other callback idioms which imply that one of te the callbacks will be called exactly once. If you are going to have something called multiple times and the error callback may be called multiple times too, you need to consolidate https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1248: new std::vector<RefPtr<NrIceMediaStream> >); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > This AutoPtr seems like overkill. Suggest just passing the vector. > > Was just doing this for consistency; ordinarily I think I agree with you. Copy > constructing RefPtr should be pretty cheap, right? I can change both. Copy constructing refptr is very cheap, one hopes. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1252: RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(level-1)); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > This is going to interact very badly with bundle. Since we're already planning > > that, let's futureproof this now. The obvious thing to do here is instead of > > asking for the level, to ask for the transport and then look into that for the > > TransportLayerIce. > > > > I would be open to other alternatives, but... > > Maybe we could have TransportFlow expose some API for this. Ultimately, this can > be done when it comes time for bundle (in fact, if I did this in this patch, I > would bitrot my work in progress for bundle). I think it makes sense to refine > this when it comes time. Then this needs a huge comment. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1254: streams->push_back(temp); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > How can this fail? Perhaps we should be reporting an error? > > I'm not sure what real-world cases could cause this. Logging something probably > makes sense though. This seems tailor-made for an asert, then. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1876: RefPtr<NrIceCtx> iceCtx, On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > This can be a const &, right? > > Review tool says this comment applies to the RefPtr<NrIceCtx>, is this what you > meant? I am already going to change the nsAutoPtr to const &. The NrIceCtx > probably has to remain a RefPtr, since the WrapRunnable won't auto-convert for > us, and we need to keep the context alive. I'm not following. WrapRunnable will hold the RefPtr alive during the calling context of this fxn. If you want it alive afterwards, you should create a new RefPtr and assign from this. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > I would prefer to hide the handle entirely. > > Do we need to come up with a second random identifier? Or is there something > more descriptive we could use? If we don't want that data leaking, we should > probably build the stuff returned by name() differently, and use the whole thing > here. Well that's my proposal. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2002: std::string subHandle = pcHandle.substr(8, std::string::npos); On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > See above abour the handle. > > Is there something nice and human-readable we can use here? I can just create a > second random string and store alongside mHandle for now, but this will involve > adding another parameter to a few functions. That seems right. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2009: nsresult rv = report ? GetStatsImpl_s(internalStats, On 2014/01/02 17:28:18, docfaraday wrote: > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > How can report be 0? > > Dunno. I think the check was there before. Well, this seems worth cleaning up still.
Sign in to reply to this message.
More discussion. https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > So this looks like the callback gets called multiple times? How do we know > > that > > > all the calls have completed? > > > > We don't, but we don't really care. > > That doesn't match any of our other callback idioms which imply that one of te > the callbacks will be called exactly once. If you are going to have something > called multiple times and the error callback may be called multiple times too, > you need to consolidate I can do this, but it requires more code/time. Do we think that this is worth doing now for an API that is chrome only, and probably never used anywhere other than the about:webrtc panel? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1252: RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(level-1)); On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > This is going to interact very badly with bundle. Since we're already > planning > > > that, let's futureproof this now. The obvious thing to do here is instead of > > > asking for the level, to ask for the transport and then look into that for > the > > > TransportLayerIce. > > > > > > I would be open to other alternatives, but... > > > > Maybe we could have TransportFlow expose some API for this. Ultimately, this > can > > be done when it comes time for bundle (in fact, if I did this in this patch, I > > would bitrot my work in progress for bundle). I think it makes sense to refine > > this when it comes time. > > Then this needs a huge comment. I think I can make a small comment work. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1254: streams->push_back(temp); On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > How can this fail? Perhaps we should be reporting an error? > > > > I'm not sure what real-world cases could cause this. Logging something > probably > > makes sense though. > > This seems tailor-made for an asert, then. I'll look into it. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1876: RefPtr<NrIceCtx> iceCtx, On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > This can be a const &, right? > > > > Review tool says this comment applies to the RefPtr<NrIceCtx>, is this what > you > > meant? I am already going to change the nsAutoPtr to const &. The NrIceCtx > > probably has to remain a RefPtr, since the WrapRunnable won't auto-convert for > > us, and we need to keep the context alive. > > I'm not following. WrapRunnable will hold the RefPtr alive during the calling > context of this fxn. If you want it alive afterwards, you should create a new > RefPtr and assign from this. Is there a way to have WrapRunnable pass a c++ reference when it was given a RefPtr? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > I would prefer to hide the handle entirely. > > > > Do we need to come up with a second random identifier? Or is there something > > more descriptive we could use? If we don't want that data leaking, we should > > probably build the stuff returned by name() differently, and use the whole > thing > > here. > > Well that's my proposal. I went ahead and implemented that. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2009: nsresult rv = report ? GetStatsImpl_s(internalStats, On 2014/01/02 18:09:55, ekr-webrtc wrote: > On 2014/01/02 17:28:18, docfaraday wrote: > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > How can report be 0? > > > > Dunno. I think the check was there before. > > Well, this seems worth cleaning up still. I'll analyze this and see then.
Sign in to reply to this message.
One more note. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:2009: nsresult rv = report ? GetStatsImpl_s(internalStats, On 2014/01/02 18:57:34, docfaraday wrote: > On 2014/01/02 18:09:55, ekr-webrtc wrote: > > On 2014/01/02 17:28:18, docfaraday wrote: > > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > > How can report be 0? > > > > > > Dunno. I think the check was there before. > > > > Well, this seems worth cleaning up still. > > I'll analyze this and see then. This check can go away.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); On 2014/01/02 18:57:34, docfaraday wrote: > On 2014/01/02 18:09:55, ekr-webrtc wrote: > > On 2014/01/02 17:28:18, docfaraday wrote: > > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > > So this looks like the callback gets called multiple times? How do we know > > > that > > > > all the calls have completed? > > > > > > We don't, but we don't really care. > > > > That doesn't match any of our other callback idioms which imply that one of te > > the callbacks will be called exactly once. If you are going to have something > > called multiple times and the error callback may be called multiple times too, > > you need to consolidate > > I can do this, but it requires more code/time. Do we think that this is worth > doing now for an API that is chrome only, and probably never used anywhere other > than the about:webrtc panel? I think it would be worth wrapping this in an all-or-nothing wrapper. https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); On 2014/01/02 18:57:34, docfaraday wrote: > On 2014/01/02 18:09:55, ekr-webrtc wrote: > > On 2014/01/02 17:28:18, docfaraday wrote: > > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > > I would prefer to hide the handle entirely. > > > > > > Do we need to come up with a second random identifier? Or is there something > > > more descriptive we could use? If we don't want that data leaking, we should > > > probably build the stuff returned by name() differently, and use the whole > > thing > > > here. > > > > Well that's my proposal. > > I went ahead and implemented that. In retrospect, let's not use the PRNG here. I suggest just a timestamp. Also, it can be much shorter, right? https://firefox-codereview.appspot.com/41001/diff/90001/media/webrtc/signalin... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/90001/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:672: static nsresult GenerateRandomString(std::string* str) { Let's not refactor this out. Instead, let's use a timestamp for the PC. E.g., use tv_usec. https://firefox-codereview.appspot.com/41001/diff/90001/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1297: // TODO(bcampen@mozilla.com): I may need to revisit this for bundle. Add a bug #. https://firefox-codereview.appspot.com/41001/diff/90001/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1934: const RefPtr<NrIceCtx>& iceCtx, Be consistent about & positioning
Sign in to reply to this message.
Some replies. https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.js File dom/media/PeerConnection.js (right): https://firefox-codereview.appspot.com/41001/diff/1/dom/media/PeerConnection.... dom/media/PeerConnection.js:124: pcref.get().getStatsInternal(null, callback, errorCallback); On 2014/01/03 22:45:59, ekr-webrtc wrote: > On 2014/01/02 18:57:34, docfaraday wrote: > > On 2014/01/02 18:09:55, ekr-webrtc wrote: > > > On 2014/01/02 17:28:18, docfaraday wrote: > > > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > > > So this looks like the callback gets called multiple times? How do we > know > > > > that > > > > > all the calls have completed? > > > > > > > > We don't, but we don't really care. > > > > > > That doesn't match any of our other callback idioms which imply that one of > te > > > the callbacks will be called exactly once. If you are going to have > something > > > called multiple times and the error callback may be called multiple times > too, > > > you need to consolidate > > > > I can do this, but it requires more code/time. Do we think that this is worth > > doing now for an API that is chrome only, and probably never used anywhere > other > > than the about:webrtc panel? > > I think it would be worth wrapping this in an all-or-nothing wrapper. Wait, so any failure would prevent any stats from getting through? This would be bad; we would need to wait for closed PCs to be garbage collected in order to get stats on any other PC in a new call (this is the specific problem I was addressing in part 14). If we do this, we need to hand back a collection of stats reports and failure reports. Here's what I propose; we are already routing calls to GetLogging through whatever PC we can find. This is not quite right already; we should instead be routing through some global object in C++ land (it looks like PeerConnectionCtx could be modified to fill this role, or at least be the backend for a new class), so we can still get logging when there are no PCs. Any API that provides bulk stats aggregation should live there too (this is ugly to do with JS callbacks). This is a large enough bit of work that it warrants its own ticket (writing new webidl, writing a class to supply the API, creating a new observer type for it, etc). Does this sound reasonable? https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/1/media/webrtc/signaling/sr... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1935: std::string subName = mediaStream.name().substr(8, std::string::npos); On 2014/01/03 22:45:59, ekr-webrtc wrote: > On 2014/01/02 18:57:34, docfaraday wrote: > > On 2014/01/02 18:09:55, ekr-webrtc wrote: > > > On 2014/01/02 17:28:18, docfaraday wrote: > > > > On 2013/12/29 17:19:55, ekr-webrtc wrote: > > > > > I would prefer to hide the handle entirely. > > > > > > > > Do we need to come up with a second random identifier? Or is there > something > > > > more descriptive we could use? If we don't want that data leaking, we > should > > > > probably build the stuff returned by name() differently, and use the whole > > > thing > > > > here. > > > > > > Well that's my proposal. > > > > I went ahead and implemented that. > > In retrospect, let's not use the PRNG here. I suggest just a timestamp. Also, it > can be much shorter, right? Ok, I've replaced this with a serial number, plus the origin and window ID. I think I would like to move the human-readable stuff into a different entry in the dictionary, but that can be an incremental refinement.
Sign in to reply to this message.
https://firefox-codereview.appspot.com/41001/diff/30004/media/webrtc/signalin... File media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp (right): https://firefox-codereview.appspot.com/41001/diff/30004/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:675: static uint32_t pcCounter = 0; This doesn't work because it leaks information about how many PCs have been created. This is why I suggested a timestamp. https://firefox-codereview.appspot.com/41001/diff/30004/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:721: oss << ++pcCounter; We generally frown on using streams in mainline code. Don't ask me why. Us PR_snprintf() https://firefox-codereview.appspot.com/41001/diff/30004/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1302: // TODO(bcampen@mozilla.com): I may need to revisit this for bundle. Still no bug #. https://firefox-codereview.appspot.com/41001/diff/30004/media/webrtc/signalin... media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp:1938: const std::vector<RefPtr<MediaPipeline>> &pipelines, Still not fixed.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
