Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove most unimplemented stats #622

Merged
merged 7 commits into from
Jun 9, 2022

Conversation

henbos
Copy link
Collaborator

@henbos henbos commented Apr 7, 2022

One of the steps needed for #621.

AFTER THIS PR HAS BEEN APPROVED BUT BEFORE LANDING IT:
Create a corresponding webrtc-provisional-stats PR adding these metrics to that document instead.

The metrics removed in this PR are those not implemented according to:

Metrics are removed regardless if they are easy or hard to implement and regardless of their usefulness - this is entirely based on implementers' interest.

Everything is not covered in this PR, other stuff:

Stuff implemented but failing in the WPT:

  • rid: This is implemented, but the WPT does not trigger simulcast.
  • echoReturnLoss/echoReturnLossEnhancement: Manually verified that values show up in a real call.
  • issuerCertificateId: No idea if it's possible to trigger this code path or how to do it, but there is code that maybe fills this in.

Preview | Diff

@henbos henbos requested review from alvestrand and vr000m April 7, 2022 12:07
@henbos
Copy link
Collaborator Author

henbos commented Apr 7, 2022

Step one - please take a look, Harald and Varun

@solmaks solmaks mentioned this pull request Apr 7, 2022
@solmaks
Copy link

solmaks commented Apr 7, 2022

The metrics removed in this PR are those not implemented according to wpt.fyi.

Isn't the reason for WPT showing some of them as unimplemented related to the nature of some metrics? Such as outbound-rtp.rid being only available when simulcast envelope is provisioned through RTCRtpTransceiver, which referenced WPT is not doing.

@fippo
Copy link
Contributor

fippo commented Apr 7, 2022

estimatedPlayoutTimestamp is implemented for sure (and rid is as well)

@henbos
Copy link
Collaborator Author

henbos commented Apr 7, 2022

Good catches, thanks for reviewing! "Fort och fel" as we say in swedish.

  • I've abandoned the PR that removed rid (because it's implemented).
  • I've restored estimatedPlayoutTimestamp so that it's not removed in this PR (because it's implemented).

@henbos
Copy link
Collaborator Author

henbos commented Apr 7, 2022

The intent is only to remove things that have not been implemented by any browser, please let me know if you find any other mistakes

@henbos
Copy link
Collaborator Author

henbos commented Apr 7, 2022

  • issuerCertificateId: No idea if it's possible to trigger this code path or how to do it, but there is code that maybe fills this in.

With regards to this one, perhaps we should remove it? Maybe it's like the RTCP transport one: spec allows it, but nobody does it, and path is untested.

@@ -3844,8 +3110,6 @@ <h3>
DOMString protocol;
required RTCIceCandidateType candidateType;
long priority;
DOMString url;
DOMString relayProtocol;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both url and relayProtocol are implemented

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs better tests...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should restore these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored

webrtc-stats.html Outdated Show resolved Hide resolved
@fippo
Copy link
Contributor

fippo commented Apr 18, 2022

https://w3c.github.io/webrtc-stats/#dom-rtctransportstats-tlsgroup -- just found this reviewing. I don't see much utility and it is not implemented in Chrome so i'd 👍 removing it

Copy link
Collaborator Author

@henbos henbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ready to merge, just need to create a PR that adds these to provisional spec

@@ -3844,8 +3110,6 @@ <h3>
DOMString protocol;
required RTCIceCandidateType candidateType;
long priority;
DOMString url;
DOMString relayProtocol;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Restored

webrtc-stats.html Show resolved Hide resolved
webrtc-stats.html Outdated Show resolved Hide resolved
@henbos
Copy link
Collaborator Author

henbos commented Jun 9, 2022

The corresponding metrics were just added to provisional, unblocking this PR:
w3c/webrtc-provisional-stats#27

@henbos henbos merged commit 109aecf into w3c:main Jun 9, 2022
sideshowbarker added a commit to mdn/browser-compat-data that referenced this pull request Jun 21, 2022
w3c/webrtc-stats#622 dropped a number of
unimplemented Identifiers for WebRTC's Statistics API features.
sideshowbarker added a commit to mdn/content that referenced this pull request Jun 21, 2022
w3c/webrtc-stats#622 dropped a number of
unimplemented Identifiers for WebRTC's Statistics API features.

Related BCD change: mdn/browser-compat-data#16748
sideshowbarker added a commit to mdn/content that referenced this pull request Jun 21, 2022
w3c/webrtc-stats#622 dropped a number of
unimplemented Identifiers for WebRTC's Statistics API features.

Related BCD change: mdn/browser-compat-data#16748
queengooborg pushed a commit to mdn/browser-compat-data that referenced this pull request Jun 21, 2022
w3c/webrtc-stats#622 dropped a number of
unimplemented Identifiers for WebRTC's Statistics API features.
teoli2003 pushed a commit to mdn/content that referenced this pull request Jun 21, 2022
* Drop some unimplemented webrtc-stats features

w3c/webrtc-stats#622 dropped a number of
unimplemented Identifiers for WebRTC's Statistics API features.

Related BCD change: mdn/browser-compat-data#16748

* Drop unimplemented stats from RTCIceCandidatePairStats & RTCRtpStreamStats pages
aarongable pushed a commit to chromium/chromium that referenced this pull request Jul 29, 2022
While the test was disabled a lot of metrics were removed from the
webrtc-stats spec, hence a lot less FAIL lines.

The diffs reflect:
- w3c/webrtc-stats#622
- mid and trackIdentifier being implemented
- jitterBufferMinimumDelay:
  https://webrtc-review.googlesource.com/c/src/+/262770

Bug: webrtc:14147
Change-Id: I6e66e1f11ce57da88058313f2a0168e56c40d45a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3791113
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@chromium.org>
Auto-Submit: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1029691}
saschanaz added a commit to saschanaz/types-web that referenced this pull request Sep 17, 2022
The mass removal of RTC items in removeTypes reflects w3c/webrtc-stats#622 but also the much better implementation status.
saschanaz added a commit to saschanaz/types-web that referenced this pull request Sep 17, 2022
The mass removal of RTC items in removeTypes reflects w3c/webrtc-stats#622 but also the much better implementation status.
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
While the test was disabled a lot of metrics were removed from the
webrtc-stats spec, hence a lot less FAIL lines.

The diffs reflect:
- w3c/webrtc-stats#622
- mid and trackIdentifier being implemented
- jitterBufferMinimumDelay:
  https://webrtc-review.googlesource.com/c/src/+/262770

Bug: webrtc:14147
Change-Id: I6e66e1f11ce57da88058313f2a0168e56c40d45a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3791113
Commit-Queue: Henrik Boström <hbos@chromium.org>
Reviewed-by: Ilya Nikolaevskiy <ilnik@chromium.org>
Auto-Submit: Henrik Boström <hbos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1029691}
NOKEYCHECK=True
GitOrigin-RevId: d5425a8c0bc8b9e85dfd7addb47f20123e969f34
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 12, 2023
removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
aarongable pushed a commit to chromium/chromium that referenced this pull request Nov 13, 2023
removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 13, 2023
removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Nov 13, 2023
removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 22, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Nov 22, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Nov 24, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104
vinnydiehl pushed a commit to vinnydiehl/mozilla-unified that referenced this pull request Nov 24, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hbos@chromium.org>
Commit-Queue: Philipp Hancke <phancke@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Nov 30, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hboschromium.org>
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Cr-Commit-Position: refs/heads/main{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104

UltraBlame original commit: 9576c917f0f49d63e5a36c4216b8d77fefa8a90c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Nov 30, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hboschromium.org>
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Cr-Commit-Position: refs/heads/main{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104

UltraBlame original commit: 9576c917f0f49d63e5a36c4216b8d77fefa8a90c
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Nov 30, 2023
…d tlsGroup stats, a=testonly

Automatic update from web-platform-tests
webrtc wpt: remove test for unimplemented tlsGroup stats

removed in
  w3c/webrtc-stats#622 (comment)

BUG=None

Change-Id: I8b2d8b2fa7776607bc863d6888a6ddb655dfac2c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5018488
Reviewed-by: Henrik Boström <hboschromium.org>
Commit-Queue: Philipp Hancke <phanckemicrosoft.com>
Cr-Commit-Position: refs/heads/main{#1223532}

--

wpt-commits: 8bb6ce81118b954e6af5870cbf770de3f6dee01b
wpt-pr: 43104

UltraBlame original commit: 9576c917f0f49d63e5a36c4216b8d77fefa8a90c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants