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.

Sign up for GitHub

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

Jump to bottom

outbound-rtp: add active flag #649

Merged
merged 1 commit into from
Jul 26, 2022
Merged

outbound-rtp: add active flag #649

merged 1 commit into from
Jul 26, 2022

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Jul 22, 2022 •
edited by pr-preview bot
Loading

@henbos
Copy link
Collaborator

henbos commented Jul 22, 2022

IPR check is failing, what is needed here @fippo @dontcallmedom ? Are you with Microsoft now?

@dontcallmedom
Copy link
Member

the IPR isn't aware yet of @fippo's new affiliation which is pending some admin handling; no objection from to merge now despite the flag if wanted.

@fippo
Copy link
Contributor Author

fippo commented Jul 22, 2022

we're waiting for some buttons to be pushed on our end which will happen in due time, we can wait.

@fippo fippo force-pushed the outbound-active branch from f178220 to 5a021b9 Compare July 22, 2022 15:21
henbos
henbos approved these changes Jul 22, 2022
View reviewed changes
webrtc-stats.html Outdated Show resolved Hide resolved
@fippo fippo force-pushed the outbound-active branch from 5a021b9 to 1eb96f8 Compare July 22, 2022 15:23
@henbos
Copy link
Collaborator

henbos commented Jul 22, 2022

@vr000m @alvestrand in case you have time to take a look. This was previously discussed in #597 and that issue was marked as ready for PR

@henbos henbos requested a review from aboba July 22, 2022 15:24
@henbos
Copy link
Collaborator

henbos commented Jul 22, 2022

Looks good to you @aboba ?

@dontcallmedom
Copy link
Member

FWIW IPR bot now happy

@vr000m
Copy link
Contributor

vr000m commented Jul 25, 2022

Is this implemented or soon to be implemented? To figure out if this is the correct spec, given we moved unimplemented items out.

I will check the metric inline.

vr000m
vr000m reviewed Jul 25, 2022
View reviewed changes
webrtc-stats.html
<dd>
<p>
Indicates whether this <a>RTP stream</a> is configured to be sent or disabled.
Note that an active stream can still not be sending, e.g. when being limited by
Copy link
Contributor

Choose a reason for hiding this comment

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

do you think we will need a counter for the number of times the status flips between active and limited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This flag isn't supposed to flip on its own and only modifiable via setParameters which we don't count.
For limited see #650 but that is partially captured by qualityLimitationDurations.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Having a counter for number of times setParameters flips this value makes little sense and is not very useful in my opinion. This metric is useful on its own and in fact very much needed for making sense of simulcast stats. Today you can't really tell "what is the highest resolution layer currently being sent?" or similar questions. We desperately need it for making sense of simulcast :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree that qualityLimitationDurations is enough, this is more about the effects of the current limitation

@fippo
Copy link
Contributor Author

fippo commented Jul 26, 2022

Is this implemented or soon to be implemented

I have a local chrome build with the implementation already

@vr000m vr000m merged commit b708dd3 into w3c:main Jul 26, 2022
@fippo fippo deleted the outbound-active branch July 26, 2022 14:16
ibaoger pushed a commit to ibaoger/webrtc that referenced this pull request Jul 28, 2022
@fippo
stats: implement outbound-rtp.active
684e241 
implementing
  w3c/webrtc-stats#649

BUG=webrtc:14291

Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269520
Reviewed-by: Ilya Nikolaevskiy <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Cr-Commit-Position: refs/heads/main@{#37639}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 2, 2022
@fippo @chromium-wpt-export-bot
webrtc: add WPT for outbound-rtp.active statistics
593528e 
added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
@chromium-wpt-export-bot chromium-wpt-export-bot mentioned this pull request Aug 2, 2022
Merged
aarongable pushed a commit to chromium/chromium that referenced this pull request Aug 18, 2022
@fippo
webrtc: add WPT for outbound-rtp.active statistics
3052390 
added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803166
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1036500}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2022
@fippo @chromium-wpt-export-bot
webrtc: add WPT for outbound-rtp.active statistics
4db5b67 
added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803166
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1036500}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this pull request Aug 18, 2022
@fippo @chromium-wpt-export-bot
webrtc: add WPT for outbound-rtp.active statistics
ef32c58 
added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803166
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1036500}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Sep 5, 2022
@fippo @moz-wptsync-bot
Bug 1782762 [wpt PR 35309] - webrtc: add WPT for outbound-rtp.active …
9cfe245 
…statistics, a=testonly

Automatic update from web-platform-tests
webrtc: add WPT for outbound-rtp.active statistics

added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803166
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1036500}

--

wpt-commits: ef32c580a942f80bab2b98bb5b8647facd109ccb
wpt-pr: 35309
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this pull request Oct 14, 2022
@fippo @copybara-github
webrtc: add WPT for outbound-rtp.active statistics
90b4926 
added in w3c/webrtc-stats#649

Also add myself as stat owner and replace my personal email with the work one in wpt/webrtc/OWNERS

BUG=webrtc:14291

Change-Id: I2cf0cad859bb1fbea0692e0f0c009d2cc67ec813
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3803166
Reviewed-by: Harald Alvestrand <[email protected]>
Commit-Queue: Philipp Hancke <[email protected]>
Reviewed-by: Henrik Boström <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1036500}
NOKEYCHECK=True
GitOrigin-RevId: 30523901b07629133a3984bff1448c0a6f2db4eb
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Dec 15, 2022
@mfromanmoz
Bug 1800920 - Vendor libwebrtc from 684e241323
fe1afb9 
Upstream commit: https://webrtc.googlesource.com/src/+/684e241323d17b13d48e4d2df823f2b60f7e0fd0
    stats: implement outbound-rtp.active

    implementing
      w3c/webrtc-stats#649

    BUG=webrtc:14291

    Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269520
    Reviewed-by: Ilya Nikolaevskiy <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Commit-Queue: Philipp Hancke <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#37639}
jamienicol pushed a commit to jamienicol/gecko that referenced this pull request Jan 3, 2023
@mfromanmoz
Bug 1800920 - Vendor libwebrtc from 684e241323
6360f3a 
Upstream commit: https://webrtc.googlesource.com/src/+/684e241323d17b13d48e4d2df823f2b60f7e0fd0
    stats: implement outbound-rtp.active

    implementing
      w3c/webrtc-stats#649

    BUG=webrtc:14291

    Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
    Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/269520
    Reviewed-by: Ilya Nikolaevskiy <[email protected]>
    Reviewed-by: Henrik Boström <[email protected]>
    Commit-Queue: Philipp Hancke <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#37639}
daemory pushed a commit to dangxiwang/webrtc that referenced this pull request Apr 19, 2024
@fippo
stats: implement outbound-rtp.active
d81efa8 
implementing
  w3c/webrtc-stats#649

BUG=webrtc:14291

Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
daemory pushed a commit to dangxiwang/webrtc that referenced this pull request Apr 19, 2024
@fippo
stats: implement outbound-rtp.active
5a4e039 
implementing
  w3c/webrtc-stats#649

BUG=webrtc:14291

Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
daemory pushed a commit to dangxiwang/webrtc that referenced this pull request Apr 19, 2024
@fippo
stats: implement outbound-rtp.active
200b458 
implementing
  w3c/webrtc-stats#649

BUG=webrtc:14291

Change-Id: Ib8453d4d7c335834cd8dd2aa29111aef26211dff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@henbos henbos henbos approved these changes

@vr000m vr000m vr000m left review comments

@aboba aboba Awaiting requested review from aboba

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

outbound-rtp.active
4 participants
@fippo @henbos @dontcallmedom @vr000m