-
Notifications
You must be signed in to change notification settings - Fork 259
Add explainer for Decoder Error #1166
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
base: main
Are you sure you want to change the base?
Add explainer for Decoder Error #1166
Conversation
Explainer for decoder error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just leaving some small comments. I think that the explainer is looking good overall.
console.warn("Decoder error: decoder fell back or failed"); | ||
|
||
// Log telemetry signal | ||
logMetric("decoder_error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we augment this example showing how different errors could be handled based on RTCRtpSenderErrorEvent::errorCode? For example: "software decoding fallback" vs "decoder unavailable".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to not add the scenario specific error codes because it may add potential fingerprinting vectors. Right now the event fires when there is an error, fallback happens, or when software decoder is not available.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But aren't them part of your proposal? I was reading the slide deck that you mentioned above. Just want to make sure that the example shows how clients could fully leverage this new event. I am not sure about this adding fingerprinting concerns though (I would say that it is not a risk).
Also, is there any value for the clients of this new API in being able to differentiate between "an error", "fallback happens", and when "software decoder is not available"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error sounds like a terminal state. In the case of a decoder error, it might be terminal. However, in the case of a fallback, streaming may continue. Should we create separate events for decoder fallback and decoder error to differentiate between these states? After decoder fallback, is it possible to transition back to hardware decode? If so, do we need a way to signal this transition to inform developers that software decode is no longer used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You both raise a good point. Perhaps we can differentiate between fallback and error without adding new fingerprinting vectors.
@gabrielsanbrito to answer your question if there is value in differentiating, I do think there is value because site developers can take different action based on the issue and their scenarios. For example, if there is a decoder issue the developer can switch to a different codec or profile or trigger error UI for end users. And if decoder fallback happens then the developer can adapt quality dynamically, pause non-critical efforts, and warn users that performance may be reduced.
@SteveBeckerMSFT instead of a separate event, how about an error code that differentiates between the states?
As for transitioning back to hardware decode I am not sure if this is possible, will look into this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nishitha-burman one possible API shape is to have a decoder state and then an event for when the decoder state changes. Maybe a rough sketch would be something like:
enum DecoderState { "hardware", "software", "error" }
partial interface RTCRtpReceiver {
readonly attribute DecoderState decoderState;
attribute EventHandler ondecoderstatechange;
};
Are there any concepts from other areas like WebCodecs we can re-use here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see a similar concept in WebRTC, which is promising since this proposal is to add to the WebRTC API.
RTCPeerConnection:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SteveBeckerMSFT: decoderstatechange is something I'd consider more useful for cases where the decoder switches e.g. from AV1 to VP9 in a multiparty scenario, basically informing the application that the codec and its characteristic changed without the need for polling getStats. I like it!
decodeerror
as name SGTM but I think we want an error (EncodingError
which is more specific than DataError
and OperationError
?) so we can differentiate between fatal ("you need to switch from H265 to H264") and non-fatal ("we went to SW and this is going to drain your battery") similar to slide 20 .
From that slide we want timestamp
which should be named rtpTimestamp
because that is what pinpointing where in a bitstream the error occured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fippo is it common for the decoder to go between hardware <-> software and between codecs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nishitha-burman currently hardware->software due to decode errors can happen only once since there is no mechanism to switch back to hardware and retry yet.
Switching codecs is common in multiparty scenarios where clients might be picking the "best" codec for a certain group of peers (ideally hardware accelerated) and then a browser not supporting that codec joins and forces a downgrade.
* Web Developers: Positive | ||
* [Xbox Cloud Gaming](https://github.com/w3c/webrtc-stats/pull/725#discussion_r1093134014) & Nvidia GeForce Now have direct use cases. | ||
* Chromium: Positive; actively pursuing proposal. | ||
* WebKit & Gecko: Overall positive feedback, but privacy/fingerprinting is a common concern. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally don't/can't request those until after the initial explainer is checked in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And @xingri can tell us what GFN thinks!
console.warn("Decoder error: decoder fell back or failed"); | ||
|
||
// Log telemetry signal | ||
logMetric("decoder_error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But aren't them part of your proposal? I was reading the slide deck that you mentioned above. Just want to make sure that the example shows how clients could fully leverage this new event. I am not sure about this adding fingerprinting concerns though (I would say that it is not a risk).
Also, is there any value for the clients of this new API in being able to differentiate between "an error", "fallback happens", and when "software decoder is not available"?
* Without this signal, developers cannot confidently diagnose or reduce fallback incidence. | ||
|
||
## Proposed Approach | ||
Introduce an event on [`RTCRtpReceiver`](https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpReceiver) ([see slide 30](https://docs.google.com/presentation/d/1FpCAlxvRuC0e52JrthMkx-ILklB5eHszbk8D3FIuSZ0/edit?slide=id.g2452ff65d17_0_71#slide=id.g2452ff65d17_0_71)) that fires when: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we give a name to the new event?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the example code I have it as decodererror
, I'll add the API shape to make this clear. But from discussions above, this name may change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I have a few questions we should answer about what error states and state transitions we should expose to web developers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall LGTM!
console.warn("Decoder error: decoder fell back or failed"); | ||
|
||
// Log telemetry signal | ||
logMetric("decoder_error"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SteveBeckerMSFT: decoderstatechange is something I'd consider more useful for cases where the decoder switches e.g. from AV1 to VP9 in a multiparty scenario, basically informing the application that the codec and its characteristic changed without the need for polling getStats. I like it!
decodeerror
as name SGTM but I think we want an error (EncodingError
which is more specific than DataError
and OperationError
?) so we can differentiate between fatal ("you need to switch from H265 to H264") and non-fatal ("we went to SW and this is going to drain your battery") similar to slide 20 .
From that slide we want timestamp
which should be named rtpTimestamp
because that is what pinpointing where in a bitstream the error occured.
|
||
## Proposed Approach | ||
Introduce an event on [`RTCRtpReceiver`](https://developer.mozilla.org/en-US/docs/Web/API/RTCRtpReceiver) ([see slide 30](https://docs.google.com/presentation/d/1FpCAlxvRuC0e52JrthMkx-ILklB5eHszbk8D3FIuSZ0/edit?slide=id.g2452ff65d17_0_71#slide=id.g2452ff65d17_0_71)) that fires when: | ||
* A decoder error occurs (e.g. bitstream decode failure) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd merge this sentence with the previous and then we have two alternatives listed
No description provided.