Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 description of an API for controlling SDP codec negotiation #186
base: main
Are you sure you want to change the base?
Add description of an API for controlling SDP codec negotiation #186
Changes from 30 commits
a14a855
005121c
8c5ec20
9c7ba95
6c6ca27
d3c6ec5
1342689
bf6e469
9d8212c
cba1936
871a6bb
2cd3e39
ebdb6fc
c0bf964
ef896ab
d582ea5
a35b9fd
d433822
713a771
5f483bc
deb9866
a02ac43
5cecfe3
f3be9dd
abd0235
4e34b4c
97f16ac
d779d39
8abaef4
b44d67f
2958844
78fbfbc
a975401
8aa1533
bd0c908
bbc7f6b
0ca754b
ccf6ea8
6206a46
78fa520
d9b390d
604da65
36fd11b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Again I wonder if these custom codecs could instead just live in transceiver.[[PreferredCodecs]]
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.
Step 8 in setCodecPreferences() filters against codecCapabilities (the static).
If we modify that, it would be possible - but would we entirely stop filtering, then?
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.
For ontrack is there a design issue here? If you want to add a codec, you can only do that after SRD which triggers ontrack. I know an implementation that will probably not like this.
This is also where I think developers will stumble over the per-transceiver approach. At least RtpCodec does not specify the payload type or we might add a footgun for breaking RTP demuxing rules.
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 you add a codec that is not understood to the SDP it should not show up in getParameters().codecs.
But it seems we have a problem since receiver.getParameters().codecs is empty before negotiation? (might be a bug, this rings a bell...)
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.
"not understood to the SDP"...?
Yes, that is why RtpCodec is the right interface. The PT is a product of the negotiation, not input to it.
Once ontrack fires, we're in the middle of negotiation, is receiver.getParameters() .codecs filled out at that time?
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 you add a codec not understood by the receiver to the SDP ...
No they are not which might be an instance of https://bugs.chromium.org/p/webrtc/issues/detail?id=12116