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 definitions of corruption detection measurements #788

Merged
merged 7 commits into from
Nov 7, 2024

Conversation

sprangerik
Copy link
Contributor

@sprangerik sprangerik commented Sep 17, 2024

Fixes #787

This PR adds new statistics for a corruption probability measure of a received video stream.

We include both the sum and sum of squares for this measure, so that a variance can be calculated.


Preview | Diff

@alvestrand
Copy link
Contributor

Awaiting the TPAC presentation.

@jan-ivar jan-ivar marked this pull request as draft September 19, 2024 14:27
@jan-ivar
Copy link
Member

Thanks! Sounds like a good idea to me. Marking as draft pending upcoming discussion.

Marking CR blocking due to the google source reference. Any volunteers to pushing the RTP header extension through IETF?

with this measurement and measurement squared respectively.
</p>
<p>
If the
Copy link

Choose a reason for hiding this comment

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

It would be better to have just a note mentioning the experimental header extension rather than a normative step.
As the header extension is not an established publication, it would block the document from advancing to Candidate Recommendation.
It may leave open the corruption detection algorithm for each implementation to allow alternative techniques.

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 didn't intend this to be normative. That header extension does not need to be either negotiated or implemented. How would you phrase it differently? Something like this?

'The user agent may produce corruption likelihood measurements using any method available. That could be facilitated by for instance: a side-channel for validation metadata such as the header extension described at http://www.webrtc.org/experiments/rtp-hdrext/corruption-detection, reference-less image analysis such as natural image statistics, ML-based classification, or anything else capable of producing an estimate.'

@handellm
Copy link

Depending on the math behind totalCorruptionProbability and totalSquaredCorruptionProbability and the fact that these are doubles and if these are ever-increasing quantities, could there be numerical issues mounting by incrementing over longer times?

@sprangerik
Copy link
Contributor Author

Thanks! Sounds like a good idea to me. Marking as draft pending upcoming discussion.

Marking CR blocking due to the google source reference. Any volunteers to pushing the RTP header extension through IETF?

I have update the text to make it more clear that method is not normative. ptal!

Also, I volunteer to push that header extension through IETF :)

@sprangerik
Copy link
Contributor Author

Depending on the math behind totalCorruptionProbability and totalSquaredCorruptionProbability and the fact that these are doubles and if these are ever-increasing quantities, could there be numerical issues mounting by incrementing over longer times?

Doubtful. Even in the case where a call is run for a very very long time, remember that the individual samples are capped in the [0.0, 1.0] range and are likely only (individually) useful to a few decimal places. With a base of 52 bits, that's still an enormous amounts of samples that can be accumulated without noticeable loss. Even further reducing the scope of this problem is that we expect the "normal" values to be essentially 0.0. If you run extremely long sessions with constant corruption, might you have other problems to prioritize first.

@sprangerik sprangerik marked this pull request as ready for review October 4, 2024 18:44
@sprangerik
Copy link
Contributor Author

@jan-ivar would you mind reviewing this?

@sprangerik
Copy link
Contributor Author

@youennf would you mind reviewing this?:

@henbos
Copy link
Collaborator

henbos commented Oct 24, 2024

Ping @youennf, can we merge this as is or is there more work needed?

measurements SHOULD be [= map/exist | present =].
</p>
<p class="note">
The user agent MAY produce corruption likelihood measurements using any method available.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have a MAY inside a note?
Also, is Chrome planning to use any other method than the header?
If not, I suggest not saying this, and we can remove this note.
We can always extend the allowed methods later on when there are more concrete proposals.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'll change it to a lower-case may then.

I have plans to add other methods yes, but it's further down the line for sure. If it causes less confusion I can remove this part for now.

The part about expected values is still valid however, but I'll move that up to the part where the measurement values are described.

Copy link
Member

Choose a reason for hiding this comment

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

Please use "can" or "might" or otherwise rephrase to avoid "may" or "MAY" in notes

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

@sprangerik sprangerik left a comment

Choose a reason for hiding this comment

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

Thanks the comments, ptal at the update!

webrtc-stats.html Show resolved Hide resolved
webrtc-stats.html Outdated Show resolved Hide resolved
measurements SHOULD be [= map/exist | present =].
</p>
<p class="note">
The user agent MAY produce corruption likelihood measurements using any method available.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, I'll change it to a lower-case may then.

I have plans to add other methods yes, but it's further down the line for sure. If it causes less confusion I can remove this part for now.

The part about expected values is still valid however, but I'll move that up to the part where the measurement values are described.

@sprangerik
Copy link
Contributor Author

ping @youennf, any further comments?

Copy link
Contributor

@youennf youennf left a comment

Choose a reason for hiding this comment

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

LGTM.
I would file an issue once this lands to track the work to replace the webrtc.org link with an IETF link.

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

LGTM. I would file an issue once this lands to track the work to replace the webrtc.org link with an IETF link.

Thanks! Yes, that makes sense.

@sprangerik
Copy link
Contributor Author

@jan-ivar anything further from your side?

Copy link
Member

@jan-ivar jan-ivar left a comment

Choose a reason for hiding this comment

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

LGTM. As a newb on this, it might be good to give the reader an example formula they can use when reading these stats. Like we do in https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-totalinterframedelay

But not a blocker.

@sprangerik
Copy link
Contributor Author

LGTM. As a newb on this, it might be good to give the reader an example formula they can use when reading these stats. Like we do in https://w3c.github.io/webrtc-stats/#dom-rtcinboundrtpstreamstats-totalinterframedelay

But not a blocker.

Good point, but let med do that as a follow-up.

@alvestrand
Copy link
Contributor

@jan-ivar why did you add the "CR Blocker" tag on this one?

@jan-ivar
Copy link
Member

jan-ivar commented Nov 7, 2024

@alvestrand see #788 (comment). To my understanding there's still a normative reference to a webrtc.org doc here. @sprangerik volunteered in #788 (comment) to push it through IETF.

So merging this would block this spec from recycling at CR. We may be OK with that, but we should track it somehow.

Erik, can you open a follow-up issue to come back and fix this link whenever your IETF efforts have concluded? Then we can label that issue and merge this, if that's how we'd like to proceed.

@jan-ivar
Copy link
Member

jan-ivar commented Nov 7, 2024

Reading through the thread again I see there was an effort to make it non-normative, but this part still sounds normative to me:

@sprangerik
Copy link
Contributor Author

@alvestrand see #788 (comment). To my understanding there's still a normative reference to a webrtc.org doc here. @sprangerik volunteered in #788 (comment) to push it through IETF.

So merging this would block this spec from recycling at CR. We may be OK with that, but we should track it somehow.

Erik, can you open a follow-up issue to come back and fix this link whenever your IETF efforts have concluded? Then we can label that issue and merge this, if that's how we'd like to proceed.

Yes, that was my intention! #788 (comment) might not have been clear on that point.

@sprangerik
Copy link
Contributor Author

Reading through the thread again I see there was an effort to make it non-normative, but this part still sounds normative to me:

In the sense that if you have decided to implement support for that extensions and have negotiated it for a particular RTP stream, then it makes sense to me to say that we should surface the stats value for it. Do you have a different formulation in mind you think is more suitable?

@youennf
Copy link
Contributor

youennf commented Nov 7, 2024

I think being normative and using MUST is good here. I think we just need to keep track of making sure the link is changed before we go to CR.

@jan-ivar
Copy link
Member

jan-ivar commented Nov 7, 2024

Agree normative is good.

@sprangerik
Copy link
Contributor Author

I filed #790 to track updating the definitions. I don't seem to have access to change labels though. @jan-ivar can you assist with that?

@henbos henbos removed the CR blocker label Nov 7, 2024
@henbos
Copy link
Collaborator

henbos commented Nov 7, 2024

I added the CR blocker label on the issue filed and removed it from this PR, sounds good?

@henbos henbos merged commit 02094bc into w3c:main Nov 7, 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.

Add definitions of corruption detection measurement statistics
7 participants