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

powerEfficientEncoder/powerEfficientDecoder #666

Closed
henbos opened this issue Sep 6, 2022 · 13 comments · Fixed by #670
Closed

powerEfficientEncoder/powerEfficientDecoder #666

henbos opened this issue Sep 6, 2022 · 13 comments · Fixed by #670
Assignees
Labels
PR exists privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on.

Comments

@henbos
Copy link
Collaborator

henbos commented Sep 6, 2022

getStats() does not explicitly contain a boolean for whether or not the current encoder/decoder is hardware accelerated.
The question of HW vs SW has raised some concerns in the past, but in many other specs we've usually gone the "isPowerEfficient" path instead, e.g. powerEfficient or powerEfficientPixelFormat. This frames the question about efficiency instead of hardware, which is the thing you really want to know (but in most cases HW = efficient, SW = inefficient).

Can we add encoder/decoder "efficiency" to RTCInboundRtpStreamStats and RTCOutboundRtpStreamStats?

The lack of this metric has lead some implementations (Chromium) to heuristically include this information in the encoderImplementation, so this is to some extent exposed already. @eshrubs

@henbos henbos self-assigned this Sep 6, 2022
@henbos
Copy link
Collaborator Author

henbos commented Sep 6, 2022

@vr000m @alvestrand

@vr000m
Copy link
Contributor

vr000m commented Sep 6, 2022

If this stops using the encoderImplementation to decipher the hardware/software, makes sense.

Will this be added to the implementation, i.e., should it be in this spec or the other?

@henbos
Copy link
Collaborator Author

henbos commented Sep 6, 2022

I'll check if I can get immediate implementation commitment, if so let's add it here, otherwise let's move the issue to webrtc-provisional-stats

henbos added a commit to henbos/webrtc-stats that referenced this issue Sep 7, 2022
@henbos
Copy link
Collaborator Author

henbos commented Sep 7, 2022

PR exists: #670

@jan-ivar
Copy link
Member

jan-ivar commented Sep 8, 2022

Isn't this redundant with powerEfficient? E.g.:

const {powerEfficient} = await navigator.mediaCapabilities.decodingInfo({
  type: "webrtc",
  video: {contentType: "video/VP8", width: 640, height: 360, bitrate: 2000, framerate: 25}
});

It seems odd to setup a call and then figure out if what we are doing is power efficient. What has changed since w3c/webrtc-extensions#21 (comment)?

@henbos
Copy link
Collaborator Author

henbos commented Sep 8, 2022

MediaCapabilities tells you about capabilities and what you can expect, which is good for deciding which codec to use, but it does not tell you in actual fact if you're currently achieving HW. Trying to call MC with values obtained from RTCCodecStats is not good enough, that's guesswork.

There are reasons why the two might not align. HW capabilities can be limited to a number of instances, so you may not get HW if multiple encoders or decoders are used. Decoding errors can and do cause SW fallback in real world scenarios. Or there may be other tradeoffs that the implementation does that results in HW or SW. MC also does not take FMTP parameters into account and bitrate is variable.

This metric is still needed and people still abuse encoderImplementation/decoderImplementation despite MC having shipped. Shipping this metric is just exposing a readily available boolean, Evan already has a CL for it.

@henbos
Copy link
Collaborator Author

henbos commented Sep 8, 2022

Our primary use case is not to use this metric instead of MC, rather as an addition. We want to run A/B experiments and see if we are able to increase the number of users that in actuality does get HW

@jan-ivar
Copy link
Member

jan-ivar commented Sep 8, 2022

HW capabilities can be limited to a number of instances, so you may not get HW if multiple encoders or decoders are used.

Should we point out this may leak information about other concurrent user activities? E.g. If a certain hardware codec is suddenly no longer available, it could mean the user is watching video on another site.

This metric is still needed and people still abuse encoderImplementation/decoderImplementation

I find this argument compelling, and I think stronger language on what information UAs MUST and MUST NOT include in encoderImplementation/decoderImplementation would go along well with this change. Can we do that?

@henbos
Copy link
Collaborator Author

henbos commented Sep 8, 2022

Should we point out this may leak information about other concurrent user activities? E.g. If a certain hardware codec is suddenly no longer available, it could mean the user is watching video on another site.

Sure, I added a fingerprint notice to the PR.

I find this argument compelling, and I think stronger language on what information UAs MUST and MUST NOT include in encoderImplementation/decoderImplementation would go along well with this change. Can we do that?

If we take Chrome as an example, there are some strings like "libvpx" which we know are SW and other strings like "D3D11VideoEncoder" which we know are HW, but there are other strings like "libvpx, fallback from D3D11VideoEncoder" (or something similar) which is very useful for debugging and perhaps other strings where the HW/SW part is more ambiguous. One way it has been used is "if impl contains a known SW string then it is SW, otherwise it is HW" which would work most of the time.

The string is very implementation-specific and I'm not sure what we should or should not say here, I'd file a separate issue if you want to discuss that further.

@youennf
Copy link
Contributor

youennf commented Sep 11, 2022

Sure, I added a fingerprint notice to the PR.

I do not think this is sufficient, as per past PING WG feedback.
We should probably first make progress on #550 before exposing any new fingerprinting information in WebRTC stats.

@henbos
Copy link
Collaborator Author

henbos commented Sep 12, 2022

Agree that we need to do something about when to expose or not expose sensitive metrics. But regarding blocking this issue, this metric exposes an information-wise lesser version of what is already exposed in the implementation string so I don't think this does much in terms of increasing the fingerprint surface.

One option could be to only expose sensitive metrics if GUM access was given, but I do think that decision should be made in a separate issue. We might want to make this block to proposed recommendation though

@henbos
Copy link
Collaborator Author

henbos commented Sep 12, 2022

TPAC: We should revive the privacy discussions (#550) before merging this

@henbos henbos added the privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on. label Sep 12, 2022
@henbos henbos added privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. and removed privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on. labels Sep 13, 2022
@henbos
Copy link
Collaborator Author

henbos commented Sep 13, 2022

Filed #675 covering the privacy aspects of both the encoderImplementation and powerEfficientEncoder

@w3cbot w3cbot added privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on. and removed privacy-tracker Group bringing to attention of Privacy, or tracked by the Privacy Group but not needing response. labels Sep 13, 2022
henbos added a commit that referenced this issue Sep 27, 2022
…). (#670)

* Fixes #666

* Add fingerprint warning

* Fix #675

* Add 'check if hardware exposure is allowed' algorithm

* Remove double-the
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR exists privacy-needs-resolution Issue the Privacy Group has raised and looks for a response on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants