Closed Bug 1435774 Opened 6 years ago Closed 5 years ago

Update ICE stat names

Categories

(Core :: WebRTC: Signaling, enhancement, P2)

Product:
Core
Component:
WebRTC: Signaling
Type:
enhancement

Tracking

()

Status:
RESOLVED DUPLICATE of bug 1324788

People

(Reporter: ng, Assigned: ng)

References

(Blocks 1 open bug)

Details

Attachments

(6 files)

Bug 1435774 - P1 - update field names in ICE stats webidl
59 bytes, text/x-review-board-request
Details
Bug 1435774 - P2 - rename ICE stats fields in cpp
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1435774 - P3 - add deprecation warnings for old ICE field names
59 bytes, text/x-review-board-request
jib
: review-
Details
Bug 1435774 - P4 - Update stats mochitest
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1435774 - P5 - WebRTC stats JSON stringify mochitest
59 bytes, text/x-review-board-request
mjf
: review+
Details
Bug 1435774 - P6 - Update about:webrtc to use new field names
59 bytes, text/x-review-board-request
mjf
: review+
Details 
A number of the fields in ICE related stats have names that have changed as the spec has evolved.  We should present the new names, and eventually deprecated the old ones.
Rank: 15
Priority: -- → P2
Component: WebRTC → WebRTC: Signaling
See Also: → 1438320
Here is a manual test that can be run to verify the new warnings https://jsfiddle.net/m4fzw64v/ which are not observable from JS. The JS observable results should be covered by mochitests.
Attachment #8948543 - Flags: review?(mfroman)
Attachment #8948544 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(jib)
Attachment #8980087 - Flags: review?(mfroman)
Attachment #8980088 - Flags: review?(mfroman)
Attachment #8980089 - Flags: review?(mfroman)
Attachment #8948543 - Flags: review?(mfroman)
Attachment #8948544 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(jib)
Attachment #8980088 - Flags: review?(mfroman)
Attachment #8980089 - Flags: review?(mfroman)
Attachment #8980087 - Flags: review?(mfroman)
Comment on attachment 8948543 [details]
Bug 1435774 - P1 - update field names in ICE stats webidl

https://reviewboard.mozilla.org/r/217960/#review252668

Looks good to me.
Attachment #8948543 - Flags: review+
Attachment #8948544 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(mfroman)
Attachment #8948545 - Flags: review?(jib)
Attachment #8980087 - Flags: review?(mfroman)
Attachment #8980088 - Flags: review?(mfroman)
Attachment #8980089 - Flags: review?(mfroman)
Comment on attachment 8948544 [details]
Bug 1435774 - P2 - rename ICE stats fields in cpp

https://reviewboard.mozilla.org/r/217962/#review253032

Looks good to me.
Attachment #8948544 - Flags: review?(mfroman) → review+
Comment on attachment 8980087 [details]
Bug 1435774 - P4 - Update stats mochitest

https://reviewboard.mozilla.org/r/246246/#review253034

Looks good to me.
Attachment #8980087 - Flags: review?(mfroman) → review+
Comment on attachment 8980088 [details]
Bug 1435774 - P5 - WebRTC stats JSON stringify mochitest

https://reviewboard.mozilla.org/r/246248/#review253050

Looks good to me.
Attachment #8980088 - Flags: review?(mfroman) → review+
Comment on attachment 8980089 [details]
Bug 1435774 - P6 - Update about:webrtc to use new field names

https://reviewboard.mozilla.org/r/246250/#review253056

Looks good to me.
Attachment #8980089 - Flags: review?(mfroman) → review+
Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253864

Please split out isRemote and remote-* type changes into separate patch as discussed offline.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:236
(Diff revision 4)
>        // roundTripTime
>        if (stat.inner.isRemote) {
>          ok(stat.roundTripTime >= 0, stat.type + ".roundTripTime is sane with" +
>            "value of:" + stat.roundTripTime);
>        } else {
> +        dump(`@@NG localVideoOnly ${JSON.stringify(stat.inner)} isRemote = ${stat.inner.isRemote}\n`);

remove debug cruft
Attachment #8948545 - Flags: review?(jib) → review-
Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253866

::: dom/media/PeerConnection.js:416
(Diff revision 4)
>          "candidate-pair": "candidatepair",
>          "local-candidate": "localcandidate",
>          "remote-candidate": "remotecandidate"
> +  },
> +  // TODO improve message
> +  _isRemoteWarning:

Missed refactoring out isRemoteWarning here.

::: dom/media/tests/mochitest/test_peerConnection_stats.html:17
(Diff revision 4)
>    });
>  var statsExpectedByType = {
>    "inbound-rtp": {
> -    expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +    expected: ["id", "timestamp", "type", "ssrc", "mediaType",
>        "packetsReceived", "packetsLost", "bytesReceived", "jitter",],
> -    optional: ["roundTripTime", "remoteId", "nackCount",],
> +    optional: ["isRemote", "roundTripTime", "remoteId", "nackCount",],

Missed backing out these changes. To isRemote here.
Comment on attachment 8948545 [details]
Bug 1435774 - P3 - add deprecation warnings for old ICE field names

https://reviewboard.mozilla.org/r/217964/#review253868

::: dom/media/tests/mochitest/test_peerConnection_stats.html:30
(Diff revision 4)
>      deprecated: ["mozRtt"],
>    },
>    "outbound-rtp": {
> -    expected: ["id", "timestamp", "type", "ssrc", "isRemote", "mediaType",
> +    expected: ["id", "timestamp", "type", "ssrc", "mediaType",
>        "packetsSent", "bytesSent", "remoteId",],
> -    optional: ["remoteId", "nackCount",],
> +    optional: ["isRemote", "remoteId", "nackCount",],

Missed backing out these changes to isRemote here.
Depends on: 1489033
Nico are these patches still on your radar? Can you merge them now after libwebrtc 64 landed?
Flags: needinfo?(na-g)
I landed the stat name changes in bug 1324788, so I am going to mark this as a dupe.
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: needinfo?(na-g)
Resolution: --- → DUPLICATE
Attachment #8948545 - Flags: review?(mfroman)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: