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

[FlexFec] Adding support for FlexFec RFC-03 #214

Merged
merged 1 commit into from
Nov 4, 2023
Merged

Conversation

pougetat
Copy link
Contributor

@pougetat pougetat commented Nov 1, 2023

Description

Context

This PR is part of a series of PRs to provide FlexFec support for pion.
https://datatracker.ietf.org/doc/html/rfc8627

Summary

This PR provides the following:

  • Adding a FlexFec encoder implementation for https://datatracker.ietf.org/doc/html/draft-ietf-payload-flexible-fec-scheme-03 => This is the implementation that chromium currently uses instead of the latest version of the draft. Providing support for this draft version will enable pion to work with chromium-based browsers.
  • Changing the FEC coverage data structure to use 2 uint64 to store the packet masks. This will reduce allocations.
  • Adding support for the actual Interceptor component that batches RTP packets together to generate repair packets before sending both types of packets over the network.
  • Various fixes to FEC header encoding, validated with chromium.

There are still some issues to be tackled:

  • Hardcoded timestamp for the RTP header. Chromium seems to work just fine with this but we need to properly set this value.
  • We currently limit ourselves to VP8, FlexFec is meant to be codec agnostic so we can provide support for more codecs.
  • We currently hardcode a minimum of 5 media packets protected by 2 media packets, this should be configurable.
  • We need to account for missing media packets, indeed media packets could be dropped on the way to being received by pion, and our FEC encoder assumes there are no missing media packets.

Reference issue

Fixes #...

@pougetat pougetat changed the title [FlexFec] Fixes and adding support for FlexFec RFC-03 [FlexFec] Various encoder fixes and adding support for FlexFec RFC-03 Nov 1, 2023
@pougetat pougetat changed the title [FlexFec] Various encoder fixes and adding support for FlexFec RFC-03 [FlexFec] Adding support for FlexFec RFC-03 Nov 1, 2023
Comment on lines 26 to 28
if info.MimeType == "video/VP8" {
return true
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should support more than just VP8.

Copy link

codecov bot commented Nov 1, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

see 12 files with indirect coverage changes

📢 Thoughts on this report? Let us know!.


interceptor := &FecInterceptor{
packetBuffer: make([]rtp.Packet, 0),
minNumMediaPackets: 5,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be configurable

@pougetat pougetat force-pushed the pion_flexfec_pr branch 2 times, most recently from 20356a3 to 531ec04 Compare November 4, 2023 01:35
@pougetat pougetat merged commit a621baf into master Nov 4, 2023
13 of 16 checks passed
@pougetat pougetat deleted the pion_flexfec_pr branch November 4, 2023 01:46
@SerialVelocity
Copy link

Hey @pougetat, does this only work for Opus (since it has inbandfec=1) or will it work for VP8, etc as well?
I saw that the (Chrome) SDP has:

a=rtpmap:49 flexfec-03/90000
a=rtcp-fb:49 goog-remb
a=rtcp-fb:49 transport-cc
a=fmtp:49 repair-window=10000000

I'm assuming that means that flexfec-03 has to be sent over 49 instead of something like 96 or am I misunderstanding something?

Is this interceptor plug and play or do I need to set up extra lines in the SDP/RTCPFeedback/etc?

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

Successfully merging this pull request may close these issues.

2 participants