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

Handling of encoder/decoder errors #763

Closed
aboba opened this issue Aug 25, 2016 · 14 comments
Closed

Handling of encoder/decoder errors #763

aboba opened this issue Aug 25, 2016 · 14 comments
Assignees

Comments

@aboba
Copy link
Contributor

aboba commented Aug 25, 2016

There are some open questions about encoder/decoder errors, such as what happens if simulcast functionality is requested that the browser cannot carry out, such as specifying sendEncodings to send more encodings than the browser can handle. setParameters is a Promise so an error will be returned, but it may not be obvious what exactly went wrong. There are not even any capabilities to describe the limitations on simulcast (such as how many streams can be sent or received).

Related Issue: #1323

@aboba aboba self-assigned this Aug 25, 2016
@aboba
Copy link
Contributor Author

aboba commented Sep 13, 2016

Some thoughts:

Do we need RTCRtpCodecCapability.maxSimulcastStreams ? (all we have today is mimeType)

try {
videoSender.setParameters(params);
}
catch (e){
console.log('params issue: ', e.message);
}

Is there any information in e.message on what was wrong with params? We may not even have called createOffer() by this point, so there isn't necessarily even any SDP to look at (or complain about).

@pthatcherg
Copy link
Contributor

Could we just choose to do the same thing we do for CPU and network adaptation: not send the layers we can't?

How is this different than "poor man's simulcast", when someone calls addTrack N times for the same track? Doesn't that have the same issue?

@alvestrand
Copy link
Contributor

Now that we're not permitting setParameters() to change number of layers, is this OBE?

@alvestrand
Copy link
Contributor

could check with @pthatcherg @taylor-b if there is an use case where a more specific error would be useful. If we don't see an obvious case (and one that can't be handled using error.name), we should close this.

@taylor-b
Copy link
Contributor

taylor-b commented Jan 26, 2017

setParameters() can't change the number of layers, but it could activate layers that are inactive. If we assume a browser may have codec-specific restrictions, then consider this example:

  1. addTransceiver called with 5 layers.
  2. createOffer called. Suppose the offer contains one codec that supports all 5 layers, one that supports 3 layers, and another that doesn't support simulcast.
  3. Offer/answer negotiation resulted in the last codec being chosen.
  4. sender.getParameters should return 5 encodings, but 4 of them are not active (active == false). (Is this correct?)
  5. If the application wants to switch to the codec that supports simulcast, it can set the active flag to true and reorder the codecs, then call sender.setParameters.
  6. If more encodings were activated than are supported for the new codec, the promise needs to be rejected with an error.

So, I like Bernard's suggestion of having RTCRtpCodecCapability.maxSimulcastStreams. Doesn't ORTC already have this, with maxTemporalLayers and maxSpatialLayers?

@alvestrand
Copy link
Contributor

Aren't temporal and spatial layers usually terms used within a single stream, to allow layers to be dropped at the MCU?
Each simulcast layer has to be individually decodable - that's why it's simulcast, not layered encoding.

@taylor-b
Copy link
Contributor

Right, so it's a slightly different meaning. But it would go in the same place (RTCRtpCodecCapability).

Does this need to be a per-codec property though? Is there a practical example where a browser would support X encodings for codec A and Y encodings for codec B?

@aboba
Copy link
Contributor Author

aboba commented Jan 27, 2017

@taylor-b ORTC only provides maxTemporalLayers/maxSpatialLayers for SVC, not simulcast. It seems to me that maxSimulcastStreams needs to be a per-codec property since it might differ between codecs (e.g. hardware-accelerated codecs versus software codecs).

Even if we add this though, there is the question of how much information we need to provide about the error. For example, the RTCError Object (Section 12.2) has errorDetail, sdpLineNumber, message and name fields.

@aboba
Copy link
Contributor Author

aboba commented Apr 6, 2017

Sentiment in virtual interim was to enable simulcast errors to be reported in RTCError. Some ideas:

  1. Add "parameters-error" to RTCErrorDetailType.
  2. Identify aspect of RTCRtpParameters that caused the error. How? parametersExcerpt? parametersLineNumber?

@taylor-b
Copy link
Contributor

taylor-b commented Apr 6, 2017

Let's step back a bit and consider what could possibly cause setParameters to fail (for reasons besides modifying a read-only parameter). As far as I can tell, the causes are:

  1. Codecs were removed, and none of the remaining codecs are still available.
  2. One of the RTCRtpEncodingParameters members, such as maxBitrate or scaleDownResolutionBy, is unachievable? Though we should avoid having this error if it's not necessary. maxBitrate and maxFramerate don't seem like a problem, since the browser can always send zero bits/frames, and with scaleDownResolutionBy, it should always be possible to scale something down to 1x1 pixels... Though the finer details of scaleDownResolutionBy aren't really spelled out.
  3. Too many RTCRtpEncodingParameters are active for the codec.

If that's really it, let's just add three new types to the RTCError type enum:

  1. "codec-unavailable"
  2. "invalid-encoding-parameter", with additional RTCError members that tell which parameter is invalid, and what its value was.
  3. "too-many-encodings" (or a better name, I'm blanking), with an additional parameter to indicate how many encodings are supported.

This seems pretty simple, and would be more intuitive/useful to application developers than my "parameters dictionary subset" idea.

@alvestrand
Copy link
Contributor

VI: No resolution taken. Bernard will continue to try to come up with a complete proposal.

@aboba
Copy link
Contributor Author

aboba commented Apr 7, 2017

@taylor-b @pthatcherg

  1. Prior to calling createOffer(), getParameters() returns the available codecs. After O/A, it will return the negotiated codecs. Is it correct to assume that it is always illegal to attempt to remove all codecs?

Are you saying that setParameters() can return a rejected promise by attempting to remove some but not all codecs? For example, after O/A, both H.264/AVC and VP8 are in the codec list, and setParameters() attempts to remove VP8, could this result in an error if there are no more H.264/AVC hardware encoding resources?

  1. My assumption has been that individual RTCRtpEncodingParameters attributes cannot result in an error unless they are out of the legal range. For example, if you set scaleResolutionDownBy to 0.48 instead of 0.50, this won't result in an error, the browser will do the best it can. However, the specification currently isn't very clear about this (e.g. there is no "overconstrained" condition for RTCRtpEncodingParameters).

  2. Too many encodings can be specified in sendEncodings. Since setParameters() cannot change the number of encodings, is it possible for setParameters() to generate an error because resources to send the number of encodings originally set in sendEncodings are no longer available? Or would this just result in degraded performance, but not an error?

@taylor-b
Copy link
Contributor

taylor-b commented Apr 7, 2017

Prior to calling createOffer(), getParameters() returns the available codecs.

I thought it would return nothing (an empty dictionary)? We really need to specify the behavior of getParameters/setParameters more thoroughly; there's a lot of hallway discussion that never seemed to get formalized. Here's an issue: #1116

Is it correct to assume that it is always illegal to attempt to remove all codecs?

That seems reasonable. Though we could say "it's legal if you have no active encodings"; maybe we'll find some use for that, like deallocating encoding resources (albeit somewhat irreversibly...).

Are you saying that setParameters() can return a rejected promise by attempting to remove some but not all codecs? For example, after O/A, both H.264/AVC and VP8 are in the codec list, and setParameters() attempts to remove VP8, could this result in an error if there are no more H.264/AVC hardware encoding resources?

Yes; I believe this is exactly the use case that convinced the working group to make it promise-based.

My assumption has been that individual RTCRtpEncodingParameters attributes cannot result in an error unless they are out of the legal range.

That sounds good to me. It's much simpler that way.

Since setParameters() cannot change the number of encodings, is it possible for setParameters() to generate an error because resources to send the number of encodings originally set in sendEncodings are no longer available?

You can't change the number, but you could deactivate/activate them (using the active field), which may be effectively the same as changing the number from an implementation perspective. You could also, for example, have 3 encodings with VP8, and then remove the VP8 codec. And the remaining codec(s) might not support 3 encodings.

@aboba
Copy link
Contributor Author

aboba commented Aug 29, 2017

I am closing this issue placeholder since distinct issues have now been filed for specific aspects of the problem:

scaleResolutionDownBy: Issue #1564
setParameters errors: #1520

@aboba aboba closed this as completed Aug 29, 2017
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

No branches or pull requests

7 participants