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

Remove Extra Lookup in PeerConnection::forwardMedia() #668

Merged
merged 4 commits into from
Jul 15, 2022

Conversation

SE2Dev
Copy link
Contributor

@SE2Dev SE2Dev commented Jul 1, 2022

Optimizes media forwarding somewhat.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it's a good idea to optimize the dispatching by SSRC, I'm pretty sure the proposed changes won't work as-is for all use cases: since incomingTrack() is only called in processLocalDescription() when generating a local answer description, SSRCs present in a remote answer after a local offer won't be inserted in mTracksBySsrc. Have you checked the media-receiver example still work with the changes?

@SE2Dev
Copy link
Contributor Author

SE2Dev commented Jul 5, 2022

While it's a good idea to optimize the dispatching by SSRC, I'm pretty sure the proposed changes won't work as-is for all use cases: since incomingTrack() is only called in processLocalDescription() when generating a local answer description, SSRCs present in a remote answer after a local offer won't be inserted in mTracksBySsrc. Have you checked the media-receiver example still work with the changes?

You're right, I checked against media-receiver and it doesn't work for that use-case. Do you think populating the map in PeerConnection::processLocalDescription and PeerConnection::processRemoteDescription would be a better method? I'm not entirely sure at what point the tracks are actually populated so I'm not really sure what a safe time to set up the mapping would be.

@paullouisageneau
Copy link
Owner

You're right, I checked against media-receiver and it doesn't work for that use-case. Do you think populating the map in PeerConnection::processLocalDescription and PeerConnection::processRemoteDescription would be a better method? I'm not entirely sure at what point the tracks are actually populated so I'm not really sure what a safe time to set up the mapping would be.

Actually, this is quite independant from tracks creation: for instance, an SSRC will commonly be added in the remote description for an already-existing track. I think updating the map in PeerConnection::processLocalDescription (after local tracks are added to the description) and PeerConnection::processRemoteDescription would be good.

@SE2Dev SE2Dev force-pushed the optimize branch 2 times, most recently from 8fa68e1 to 4ecaf60 Compare July 6, 2022 17:31
@SE2Dev
Copy link
Contributor Author

SE2Dev commented Jul 6, 2022

I've re-implemented the changes to take place in PeerConnection::processLocalDescription and PeerConnection::processRemoteDescription, and checked that it does still work with the media-receiver example.

src/impl/peerconnection.cpp Outdated Show resolved Hide resolved
src/impl/peerconnection.cpp Outdated Show resolved Hide resolved
src/impl/peerconnection.cpp Outdated Show resolved Hide resolved
src/impl/peerconnection.cpp Outdated Show resolved Hide resolved
src/description.cpp Outdated Show resolved Hide resolved
@paullouisageneau
Copy link
Owner

Thank for the changes, I still have a few comments but now the approach looks good.

Copy link
Owner

@paullouisageneau paullouisageneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thank you!

@paullouisageneau paullouisageneau merged commit e42d92c into paullouisageneau:master Jul 15, 2022
@paullouisageneau
Copy link
Owner

My bad, I approved a bit too fast, an additional call to updateTrackSsrcCache() was actually missing after creating incoming tracks. It is necessary because these tracks don't exist when processing the remote description. I fixed it in ef4a05a.

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

Successfully merging this pull request may close these issues.

2 participants