Closed Bug 979471 Opened 6 years ago Closed 6 years ago

about:webrtc does not show ICE stats for DataChannels

Categories

(Core :: WebRTC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: bwc, Assigned: bwc)

References

Details

Attachments

(1 file, 4 obsolete files)

Probably because the stats API keys off of media stream track, which doesn't apply to DataChannels.
Assignee: nobody → docfaraday
Depends on: 958221
Status: NEW → ASSIGNED
One possible solution. Only works if we have one DataChannel. Might need more.
track_id is indexed starting at 1, so let a value of 0 indicate no active DataChannel instead of -1
Attachment #8385716 - Attachment is obsolete: true
Ask DataChannelConnection for the list of track ids, since that's the authoritative source.
Attachment #8386271 - Attachment is obsolete: true
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.
Attachment #8386301 - Flags: review?(jib)
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?
Attachment #8386301 - Flags: review?(jib) → review+
Attachment #8386301 - Attachment is obsolete: true
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+
Attachment #8386919 - Flags: review+
Needinfo self to request checkin once try looks good.
Flags: needinfo?(docfaraday)
Flags: needinfo?(docfaraday)
Keywords: checkin-needed
Needs rebasing. Also, are more parts coming? If so, I assume this should be leave-open?
Keywords: checkin-needed
Attachment #8386919 - Attachment is obsolete: true
Comment on attachment 8388556 [details] [diff] [review]
Part 1: Populate ICE stats for DataChannels when we have a null selector.

Carry forward r=jib
Attachment #8388556 - Flags: review+
(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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/dcb2be828798
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.