Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618263004:
webrtc-internals: show addIceCandidate when it fails (Closed)
Descriptionwebrtc-internals: show addIceCandidate when it fails
currently, webrtc-internals does not show addIceCandidate when
the native addIceCandidate fails, it only shows the failure.
It should show both since the addIceCandidate is called as well as
the failure callback.
BUG=677550
Patch Set 1 #
Total comments: 3
Messages
Total messages: 10 (3 generated)
tommi@chromium.org changed reviewers: + tommi@chromium.org
https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:1436: // Ensure that addIceCandidate shows up even if there is an error. reporting this twice intentionally?
https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:1436: // Ensure that addIceCandidate shows up even if there is an error. On 2017/01/09 14:41:17, tommi (chrömium) wrote: > reporting this twice intentionally? Yes. The first one (with the true argument) generates the addIceCandidate when there is an error, the second call (with the actual result) generates the addIceCandidateOnFailure (when return_value is false). This sounds rather weird but handling succeeded=false in https://cs.chromium.org/chromium/src/content/renderer/media/peer_connection_t... would be equally messy :-/
tommi@chromium.org changed reviewers: + hta@chromium.org
hta - wdyt?
Need more explanation for why 2 lines are better than one. Perhaps a paste of the log with and without the patch would make the point clear? BTW, I think you need another bug number to link to; the linked-to bug seems to be all about setRemoteDescription. https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... File content/renderer/media/rtc_peer_connection_handler.cc (right): https://codereview.chromium.org/2618263004/diff/1/content/renderer/media/rtc_... content/renderer/media/rtc_peer_connection_handler.cc:1436: // Ensure that addIceCandidate shows up even if there is an error. On 2017/01/09 15:09:32, fippo wrote: > On 2017/01/09 14:41:17, tommi (chrömium) wrote: > > reporting this twice intentionally? > > > Yes. The first one (with the true argument) generates the addIceCandidate when > there is an error, the second call (with the actual result) generates the > addIceCandidateOnFailure (when return_value is false). > > This sounds rather weird but handling succeeded=false in > https://cs.chromium.org/chromium/src/content/renderer/media/peer_connection_t... > would be equally messy :-/ The original logic seemed to be that you logged either success or error. Why is it a good thing to log two lines, given that you include all the information both times? An alternative rendering would be to change https://cs.chromium.org/chromium/src/content/renderer/media/peer_connection_t... from "addIceCandidate" to "addIceCandidate(success)" to indicate that the log-line is happening after the call has completed.
> The original logic seemed to be that you logged either success or error. Why is > it a good thing to log two lines, given that you include all the information > both times? The issue I am trying to solve is that addIceCandidate/addIceCandidateFailed behave differently from SRD/SLRD/createOffer/createAnswer -- all of these have "line" in the trace for the call and another one for the succes/failure callback. This difference in behaviour makes it (slighly) hard to interpret those logs programatically.
On 2017/01/16 09:45:15, fippo wrote: > > The original logic seemed to be that you logged either success or error. Why > is > > it a good thing to log two lines, given that you include all the information > > both times? > > The issue I am trying to solve is that addIceCandidate/addIceCandidateFailed > behave differently from SRD/SLRD/createOffer/createAnswer > -- all of these have "line" in the trace for the call and another one for the > succes/failure callback. > > This difference in behaviour makes it (slighly) hard to interpret those logs > programatically. Hm. If you want two lines per candidate for both directions of call, I think you should add logging to accomplish exactly that. As is, tracking is called from: https://cs.chromium.org/chromium/src/content/renderer/media/rtc_peer_connecti... onIceCandidate, once https://cs.chromium.org/chromium/src/content/renderer/media/rtc_peer_connecti... addIceCandidate, once If you want before/after logging (which I think makes a lot of sense), please add it to both places.
On 2017/01/17 13:07:09, hta - Chromium wrote: > On 2017/01/16 09:45:15, fippo wrote: > > > The original logic seemed to be that you logged either success or error. Why > > is > > > it a good thing to log two lines, given that you include all the information > > > both times? > > > > The issue I am trying to solve is that addIceCandidate/addIceCandidateFailed > > behave differently from SRD/SLRD/createOffer/createAnswer > > -- all of these have "line" in the trace for the call and another one for the > > succes/failure callback. > > > > This difference in behaviour makes it (slighly) hard to interpret those logs > > programatically. > > Hm. If you want two lines per candidate for both directions of call, I think you > should add logging to accomplish exactly that. > > As is, tracking is called from: > https://cs.chromium.org/chromium/src/content/renderer/media/rtc_peer_connecti... > onIceCandidate, once > > https://cs.chromium.org/chromium/src/content/renderer/media/rtc_peer_connecti... > addIceCandidate, once > > If you want before/after logging (which I think makes a lot of sense), please > add it to both places. onicecandidate can't fail. But I think there are more changes required here. The current failure would return just the same information instead of the error string. Which, arguably is not really useful either: "Error processing ICE candidate." https://cs.chromium.org/chromium/src/content/renderer/media/rtc_peer_connecti... I'll see if I can rework it so that this error is returned hoping that someone will add more useful information there at some point.
Description was changed from ========== webrtc-internals: show addIceCandidate when it fails currently, webrtc-internals does not show addIceCandidate when the native addIceCandidate fails, it only shows the failure. It should show both since the addIceCandidate is called as well as the failure callback. BUG=677550 ========== to ========== webrtc-internals: show addIceCandidate when it fails currently, webrtc-internals does not show addIceCandidate when the native addIceCandidate fails, it only shows the failure. It should show both since the addIceCandidate is called as well as the failure callback. BUG=677550 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
