993141 - Stats API does not gather DataChannel stats properly when no selector is provided

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Byron Campen [:bwc]]

DataChannel::mStream has nothing to do with the m-line index (level), so the code operating on this assumption is faulty.

Comment 1

[Byron Campen [:bwc]]

Attachment: Fix bug where we were assuming DataChannel::mStream corresponded to the level.

This fixes the bug, and simplifies the code a little.

Comment 2

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

Comment on attachment 8402943 [details] [diff] [review]
Fix bug where we were assuming DataChannel::mStream corresponded to the level.

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

r=me with nits.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2004,5 @@
> +  } else {
> +    // We want to grab all streams, so ignore the pipelines (this also ends up
> +    // grabbing DataChannel streams, which is what we want)
> +    for (size_t s = 0; s < mMedia->num_ice_media_streams(); ++s) {
> +      levelsToGrab.insert(s + 1); // mIceStreams is 0-indexed

Or just

for (size_t s = 1; s <= mMedia->num_ice_media_streams(); ++s) {
  levelsToGrab.insert(s);

@@ +2016,5 @@
> +    RefPtr<TransportFlow> flow(mMedia->GetTransportFlow(*s, false));
> +    if (temp && flow) {
> +      query->streams.AppendElement(temp);
> +    } else {
> +      CSFLogError(logTag, "Failed to get NrIceMediaStream for level %zu "

With this change it looks like you'll log errors on unused DataChannels, which seems undesirable.

Also, does %zu work on Windows?

@@ +2018,5 @@
> +      query->streams.AppendElement(temp);
> +    } else {
> +      CSFLogError(logTag, "Failed to get NrIceMediaStream for level %zu "
> +          "in %s:  %s",
> +          static_cast<size_t>(*s),

*s is already size_t

@@ -2036,5 @@
> -      streamsGrabbed.insert(*s);
> -
> -      RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(*s - 1));
> -
> -      // This will be null if DataChannel is not in use

Maybe not loose this comment? I found it useful.

Comment 3

[Byron Campen [:bwc]]

Comment on attachment 8402943 [details] [diff] [review]
Fix bug where we were assuming DataChannel::mStream corresponded to the level.

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

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +2004,5 @@
> +  } else {
> +    // We want to grab all streams, so ignore the pipelines (this also ends up
> +    // grabbing DataChannel streams, which is what we want)
> +    for (size_t s = 0; s < mMedia->num_ice_media_streams(); ++s) {
> +      levelsToGrab.insert(s + 1); // mIceStreams is 0-indexed

I prefer to 0-index my loops. Makes the analysis easier for me.

@@ +2016,5 @@
> +    RefPtr<TransportFlow> flow(mMedia->GetTransportFlow(*s, false));
> +    if (temp && flow) {
> +      query->streams.AppendElement(temp);
> +    } else {
> +      CSFLogError(logTag, "Failed to get NrIceMediaStream for level %zu "

This was in the previous patch, so I would hope %zu works. Although you are right about this logging errors in normal operation. I think we don't need it, honestly.

@@ +2018,5 @@
> +      query->streams.AppendElement(temp);
> +    } else {
> +      CSFLogError(logTag, "Failed to get NrIceMediaStream for level %zu "
> +          "in %s:  %s",
> +          static_cast<size_t>(*s),

Yep. This is just future-proofing, and was present before.

@@ -2036,5 @@
> -      streamsGrabbed.insert(*s);
> -
> -      RefPtr<NrIceMediaStream> temp(mMedia->ice_media_stream(*s - 1));
> -
> -      // This will be null if DataChannel is not in use

I can put something similar in.

Comment 4

[Byron Campen [:bwc]]

Attachment: Fix bug where we were assuming DataChannel::mStream corresponded to the level.

Fix some nits.

Comment 5

[Byron Campen [:bwc]]

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

Also, needinfo myself to check back on try push.

Comment 6

[Byron Campen [:bwc]]

Comment on attachment 8403458 [details] [diff] [review]
Fix bug where we were assuming DataChannel::mStream corresponded to the level.

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

Carry forward r=jib

Comment 7

[Byron Campen [:bwc]]

Seeing a build failure on B2G ICS, but looks like infra. Requesting checkin.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7699524771b1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7699524771b1