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

set codec preferences #1646

Closed
jakecoffman opened this issue Jan 24, 2021 · 16 comments
Closed

set codec preferences #1646

jakecoffman opened this issue Jan 24, 2021 · 16 comments

Comments

@jakecoffman
Copy link

jakecoffman commented Jan 24, 2021

Summary

Add the ability to set codec preferences on a specific track/transceiver.

Motivation

My real world use case for this is I have a security camera that has 2 way audio. It sends a variety of codecs but only accepts PCMA. Therefore I must either change the camera settings, or I can modify the SDP to use PCMA just for the one track.

JavaScript has RTCRtpTransceiver.setCodecPreferences which can be used pretty easily like so:

const stream = await navigator.mediaDevices.getUserMedia({audio: true})
stream.getTracks().forEach(track => this.pc.addTrack(track, stream));
const transceiver = this.pc.getTransceivers().find(t => t.sender && t.sender.track === stream.getAudioTracks()[0]);
const {codecs} = RTCRtpSender.getCapabilities('audio');
const selectedCodecIndex = codecs.findIndex(c => c.mimeType === 'audio/PCMA');
transceiver.setCodecPreferences([codecs[selectedCodecIndex]]);

Describe alternatives you've considered

Currently the only ways to restrict which codec is used on a track is to:

  1. Register specific codecs in the MediaEngine
  2. Mung the SDP myself

For (1) this is less desirable or perhaps even unworkable for some people because it would effect every track on the peer instead of just the target track. In my case I can change the camera settings so this is what I am doing for now.

For (2) this is annoying, difficult, and error prone.

@Sean-Der
Copy link
Member

I am all for implementing this!

Pion right now stores codecs globally. The API however was designed to support per-transceiver codecs. We can update the API to copy codecs on creation, then allow users to disable them per transceiver.

There are some things that are ambiguous that we need to check first.

  • what happens when codecs change on re-negotiations?
  • Do we need to take codecs into account when pairing transceivers.

@jech
Copy link
Member

jech commented Jan 25, 2021

I am all for implementing this!

I'd use it. I'm currently working around its lack: https://github.com/jech/galene/blob/master/group/group.go#L198

what happens when codecs change on re-negotiations?

It seems to me that the SDP we generate should be restricted by the preferences set, so that any codecs negotiated would fit the constraints.

Do we need to take codecs into account when pairing transceivers.

What does the WebRTC spec say?

@jech
Copy link
Member

jech commented Feb 3, 2021

The code cited above is the root cause of #1656. We really need a way to change the set of allowable codecs at renegotiation time.

@Sean-Der
Copy link
Member

Sean-Der commented Feb 5, 2021

I am going to start work on this. If others are interested I would appreciate the help!

  • Add SetCodecPreferences on RTPTransceiver.
  • Assert that set values match MediaEngine, this can only be used to constrain.
  • Assert the generating the media section only contains the expected codecs.

AddTrack finding the right Transceiver is not possible unfortunately. The Track API is one shot. A RTPSender calls bind and that is it, we have no ‘soft try’ or ‘query’ concept. A developer needs to associate the Track/Transceiver for us. Just one caveat to be aware of @OrlandoCo @jech

@jech
Copy link
Member

jech commented Feb 5, 2021

A developer needs to associate the Track/Transceiver for us.

How are they supposed to do that? As far as I am aware, you can either use AddTrack (which reuses a transceiver if available, creates a new one otherwise) or AddTransceiverFromTrack (which unconditionally creates a transceiver). What am I missing?

@Sean-Der
Copy link
Member

Sean-Der commented Feb 5, 2021

@jech Yea unfortunately you will have to do AddTransceiverFromTrack or ReplaceTrack

We can look into making this better for /v4 for sure! I am getting more pressure from other short comings of Pion right now though. A lot of users are emailing me with Congestion Control and the general lack of in-tree RTP/RTCP maturity.

@jech
Copy link
Member

jech commented Feb 6, 2021

we have no ‘soft try’ or ‘query’ concept.

Yea unfortunately you will have to do AddTransceiverFromTrack or ReplaceTrack

Going through the list of transceivers in order to find out whether to call ATFT or RT is racy — the transceiver might no longer be available by the time you call RT, so you need to do your own locking around the call. Possibly not what you envision.

@Sean-Der
Copy link
Member

Sean-Der commented Feb 6, 2021

Oof yea it won't be atomic.

I don't have any ideas on how to do his better unfortunately. I am happy to let this sit if we want to think about it!

For me the MediaEngine bug is pretty big. @OrlandoCo @jech would you be ok with a DoNotCopyMediaEngine flag for now?

@jakecoffman
Copy link
Author

Instead of SetCodecPreferences on RTPTransceiver, would adding a variadic TrackOptions to the AddTrack function work?

e.g. func (pc *PeerConnection) AddTrack(track TrackLocal, options ...TrackOption) (*RTPSender, error) {

And then of course TrackOptions would have a way to set the desired codecs.

@jech
Copy link
Member

jech commented Feb 7, 2021 via email

@Sean-Der
Copy link
Member

Sean-Der commented Feb 7, 2021

@OrlandoCo You ok with me fixing #1662 in master? I just helped a user who this was actively breaking them.

If you are ok I am going to fix and tag today.

@OrlandoCo
Copy link
Contributor

@Sean-Der is going to be possible to add codecs on the fly?

@Sean-Der
Copy link
Member

Sean-Der commented Feb 7, 2021

@OrlandoCo no :(

I could add a SettingEngine to not copy (to keep your use case). Then later we can provide per transceiver controls!

@jech
Copy link
Member

jech commented Feb 10, 2021

How about calling it ShareMediaEngine?

@digitalix digitalix linked a pull request Jun 5, 2021 that will close this issue
@Sean-Der
Copy link
Member

Sean-Der commented Jul 2, 2021

Implemented with f524fea

@Sean-Der Sean-Der closed this as completed Jul 2, 2021
@jech
Copy link
Member

jech commented Jul 2, 2021

Yay!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants