979471 - about:webrtc does not show ICE stats for DataChannels

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Byron Campen [:bwc]]

Probably because the stats API keys off of media stream track, which doesn't apply to DataChannels.

Comment 1

[Byron Campen [:bwc]]

Attachment: Part 1: Populate ICE stats for DataChannels when we have a null selector.

One possible solution. Only works if we have one DataChannel. Might need more.

Comment 2

[Byron Campen [:bwc]]

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

Comment 3

[Byron Campen [:bwc]]

Attachment: Part 1: Populate ICE stats for DataChannels when we have a null selector.

track_id is indexed starting at 1, so let a value of 0 indicate no active DataChannel instead of -1

Comment 4

[Byron Campen [:bwc]]

Attachment: Part 1: Populate ICE stats for DataChannels when we have a null selector.

Ask DataChannelConnection for the list of track ids, since that's the authoritative source.

Comment 5

[Byron Campen [:bwc]]

Comment on attachment 8386301 [details] [diff] [review]
Part 1: Populate ICE stats for DataChannels when we have a null selector.

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

This should be about right; we will probably need to refactor a little once we have DataChannel selectors (BuildStatsQuery will probably need to be broken up into logic that is common between the MediaStreamTrack and DataChannel cases, and the specific logic for each), but that can come when we're ready.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8386301 [details] [diff] [review]
Part 1: Populate ICE stats for DataChannels when we have a null selector.

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

r+ with INVALID_STREAM check and nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2006,5 @@
> +    mDataConnection->GetStreamIds(&streamIds);
> +
> +    for (auto s = streamIds.begin(); s!= streamIds.end(); ++s) {
> +      MOZ_ASSERT(*s);
> +      RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(*s - 1));

Good idea to assert non-zero before subtracting. Maybe do the same in the code directly above this patch?

I could be wrong, but I think you need an if-test here against INVALID_STREAM before subtracting.

@@ +2010,5 @@
> +      RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(*s - 1));
> +
> +      // This will be null if DataChannel is not in use
> +      RefPtr<TransportFlow> flow(mMedia->GetTransportFlow(*s, false));
> +      if (temp && flow) {

I like this better than if (temp.get()) { in the code directly above this patch. .get() is redundant. Maybe unify style?

Comment 7

[Byron Campen [:bwc]]

Attachment: Part 1: Populate ICE stats for DataChannels when we have a null selector.

Incorporate feedback

Comment 8

[Byron Campen [:bwc]]

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

Comment 9

[Byron Campen [:bwc]]

Comment on attachment 8386919 [details] [diff] [review]
Part 1: Populate ICE stats for DataChannels when we have a null selector.

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

Carry forward r+

Comment 10

[Byron Campen [:bwc]]

Needinfo self to request checkin once try looks good.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing. Also, are more parts coming? If so, I assume this should be leave-open?

Comment 12

[Byron Campen [:bwc]]

Attachment: Part 1: Populate ICE stats for DataChannels when we have a null selector.

Unrot

Comment 13

[Byron Campen [:bwc]]

Comment on attachment 8388556 [details] [diff] [review]
Part 1: Populate ICE stats for DataChannels when we have a null selector.

Carry forward r=jib

Comment 14

[Byron Campen [:bwc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #11)
> Needs rebasing. Also, are more parts coming? If so, I assume this should be
> leave-open?

Ok, done. This is the only patch for this ticket.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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