Closed Bug 993141 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: WebRTC: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: bwc, Assigned: bwc)

Details

Attachments

(1 file, 1 obsolete file)

DataChannel::mStream has nothing to do with the m-line index (level), so the code operating on this assumption is faulty.
This fixes the bug, and simplifies the code a little.
Attachment #8402943 - Flags: review?(jib)
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.
Attachment #8402943 - Flags: review?(jib) → review+
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.
Attachment #8402943 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=d3c856055073

Also, needinfo myself to check back on try push.
Flags: needinfo?(docfaraday)
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
Attachment #8403458 - Flags: review+
Seeing a build failure on B2G ICS, but looks like infra. Requesting checkin.
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7699524771b1
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.