-
Notifications
You must be signed in to change notification settings - Fork 115
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
Allow level-id to be different in codec match #3023
base: main
Are you sure you want to change the base?
Conversation
@aboba Does this wording make sense to you? |
if you want to go this route you will also need to take care of RFC6184s |
No this is just about the codec matching algorithm used to sanity check input to setParameters/setCodecPreferences, what is actually negotiated based on this is defined somewhere else. As for sending the selected codec, the description is kind of vague, RTCRtpEncodingParmaters.codec just says:
And setParameters just says:
So it's just "codec used for selecting what to send", we might want to create an issue about clarifying not to send greater than the level-id that was negotiated, but that seems more like an editorial note. |
We might want to update getParameters().encodings[i].codec to match the level-id that you find in getParamters().codecs though? Separate issue though because that is the same as what happens with setCodecPreferences() today |
"difftype": "modify", | ||
"id": 51, | ||
"pr": 3023, | ||
"testUpdates": "not-testable" |
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.
Remote SDP is testable.
Codec wizard @aboba, can you do another review? |
Btw I had said during the meeting that I thought all instances of "codec matching" could ignore level, but upon a second look I realized that we don't need to be that "loose" in most cases. For example if you addTransceiver, the only valid input is from getCapabilities since there is no chance that anything else has been negotiated for that transceiver yet. It might not hurt to be more loose here, but it does not seem to help either. Ultimately we only need to "ignoreLevels=true" at a few places:
In all other cases we can "ignoreLevels=false", which the PR reflects now. |
Co-authored-by: Harald Alvestrand <[email protected]>
</div> | ||
</li> | ||
</ol> | ||
</li> | ||
<li> | ||
<p> | ||
Return <code>true</code>. |
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.
If one of the inputs is garbage the old algorithm returned false. Doesn't this risk returning true if the line is not parseable?
</p> | ||
<p class="note"> | ||
Even if <var>ignoreLevels</var> is <code>true</code>, some codecs (such as H.264) include | ||
levels in the media format, so that the level cannot be ignored in this algorithm. |
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.
More accurately "..., so that ignoring the level requires codec-specific parsing" to reflect level ID in H.264 being something that can be parsed e.g. last byte
Fixes #3020
💥 Error: 500 Internal Server Error 💥
PR Preview failed to build. (Last tried on Dec 2, 2024, 9:59 AM UTC).
More
PR Preview relies on a number of web services to run. There seems to be an issue with the following one:
🚨 Spec Generator - Spec Generator is the web service used to build specs that rely on ReSpec.
🔗 [Related URL]([object Object])
If you don't have enough information above to solve the error by yourself (or to understand to which web service the error is related to, if any), please file an issue.