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

Add "only exists" clause to various audio/video stats #774

Merged
merged 2 commits into from
Jan 11, 2024
Merged

Conversation

fippo
Copy link
Contributor

@fippo fippo commented Dec 27, 2023

and add linebreaks after those clauses.
Also sync definition of "mid" stat between inbound-rtp and outbound-rtp


Preview | Diff

and add linebreaks after those clauses.
Also sync definition of "mid" stat between inbound-rtp and outbound-rtp
Only [= map/exist =]s if the {{RTCRtpTransceiver}} owning this stream has a
{{RTCRtpTransceiver/mid}} value that is not null. this is that
value.
If the {{RTCRtpTransceiver}} owning this stream has a
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Taken from https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-mid
I wonder if inbound-rtp and outbound-rtp need to take care of the edge case where mid was offered but not negotiated but that seems orthogonal

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd update both of them to say "otherwise this member does not [= map/exist =]." so that they're consistent with each other but we also make use of the "map/exist" reference used everywhere else

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the edge case you're describing seems orthogonal and not sure it's needed since this is just piggybacking on whatever the transceiver is doing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need to distinguish between "exist" and "present"

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the implementation does not make such distinction. It either is there and has a string value or it is not there. We don't support null or even undefined values. If a value on a dictionary is not set, it does not [= map/exist =].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. But I think we should only use exist for things that will never exist, not things that will not be set on some condition

Copy link
Collaborator

Choose a reason for hiding this comment

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

But it will never exist for that object. Every getStats call returns a new object, so we only have objects where it always existed and objects where it never existed. The continuation over time is an illusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a hill to die on, i'm happy using the previous text as long as it is done in both locations

@henbos
Copy link
Collaborator

henbos commented Jan 2, 2024

I think this is PR is great thing!

But nit: while "no implementation change intended", this is not strictly "editorial" since we change from "applicable to both kinds" to "applicable to only video".

@fippo
Copy link
Contributor Author

fippo commented Jan 2, 2024

But only for things which we already knew were video-only but this was not documented?

@fippo fippo changed the title [editorial] add "only exists" clause to various audio/video stats Add "only exists" clause to various audio/video stats Jan 3, 2024
@fippo
Copy link
Contributor Author

fippo commented Jan 3, 2024

Adjusted the title, this is above "fixes whitespace" but technically a normative change.

@henbos henbos self-requested a review January 4, 2024 15:46
Copy link
Collaborator

@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.

This PR accurately describes the kinds implemented/not-implemented in libwebrtc.

@jan-ivar, do we have the permission to merge this "almost editorial" PR?

webrtc-stats.html Show resolved Hide resolved
webrtc-stats.html Outdated Show resolved Hide resolved
@henbos henbos merged commit 12c9dd3 into w3c:main Jan 11, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants