Skip to content

Fix malformed FlexFEC-03#330

Merged
aalekseevx merged 3 commits intopion:masterfrom
aalekseevx:flexfec
May 21, 2025
Merged

Fix malformed FlexFEC-03#330
aalekseevx merged 3 commits intopion:masterfrom
aalekseevx:flexfec

Conversation

@aalekseevx
Copy link
Copy Markdown
Member

@aalekseevx aalekseevx commented May 18, 2025

Description

Fix several issues with FlexFec-03 encoder, add simple decoder and round-trip tests

Reference issue

Fixes #318

@codecov
Copy link
Copy Markdown

codecov bot commented May 18, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 74.20290% with 89 lines in your changes missing coverage. Please review.

Project coverage is 77.71%. Comparing base (f30b304) to head (d2b1594).
Report is 3 commits behind head on master.

Files with missing lines Patch % Lines
pkg/flexfec/flexfec_decoder_03.go 72.90% 55 Missing and 26 partials ⚠️
pkg/flexfec/flexfec_encoder_03.go 88.88% 2 Missing and 1 partial ⚠️
pkg/flexfec/util/media_packet_iterator.go 40.00% 2 Missing and 1 partial ⚠️
pkg/flexfec/flexfec_coverage.go 85.71% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master     #330      +/-   ##
==========================================
+ Coverage   72.11%   77.71%   +5.59%     
==========================================
  Files          79       80       +1     
  Lines        4756     5080     +324     
==========================================
+ Hits         3430     3948     +518     
+ Misses       1187      958     -229     
- Partials      139      174      +35
Flag Coverage Δ
go 77.71% <74.20%> (+5.59%) ⬆️
wasm 75.33% <74.20%> (+5.21%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI reviewed May 18, 2025
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes several issues in the FlexFEC-03 encoder by adding validation for empty, out-of-order, and missing media packets, refactors bitmask extraction into helpers, updates the media packet iterator, and adds comprehensive tests including a simple decoder and round-trip recovery tests.

  • Validate input ordering and emptiness in EncodeFec
  • Refactor bitmask extraction and update header encoding logic
  • Improve media packet iterator logic and add iterator tests

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
pkg/flexfec/util/media_packet_iterator.go Use coveredIndices for Next/First iteration
pkg/flexfec/util/media_packet_iterator_test.go Add unit tests for the media packet iterator
pkg/flexfec/flexfec_encoder_03.go Add checks for empty/out-of-order media packets; refactor header/repair payload logic
pkg/flexfec/flexfec_encoder_03_test.go Add edge-case tests for EncodeFec
pkg/flexfec/flexfec_coverage.go Extract mask logic into extractMask1/2/3/3_03 helpers
pkg/flexfec/flexfec_coverage_test.go Add tests for mask extraction
pkg/flexfec/flexfec_03_test.go Add simple decoder and round-trip recovery tests

@aalekseevx aalekseevx force-pushed the flexfec branch 2 times, most recently from 5b525b5 to 8c7a8f2 Compare May 18, 2025 08:47
@aalekseevx aalekseevx mentioned this pull request May 18, 2025
Closed
3DRX
3DRX approved these changes May 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@3DRX 3DRX left a comment

Choose a reason for hiding this comment

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

A suggestion here to make this work out of the box for everyone. I've tested this PR with these additional changes, and this interceptor itself doesn't seems to have any problem. I also created a testing repo for this, here.

@aalekseevx
Copy link
Copy Markdown
Member Author

aalekseevx commented May 20, 2025

@3DRX, I don’t want to make this PR even bigger and harder to review, so I created a separate issue regarding EncoderInterceptor with your suggestions and a couple of mine: #332. In this PR, I want to focus on fixing the encoder. I will make another one for the interceptor!

@aalekseevx
Copy link
Copy Markdown
Member Author

aalekseevx commented May 20, 2025

@joeturki, do you want to take a look at the code too? I'm not rushing you in any way, just want to make sure we're on the same page.

@JoTurk
Copy link
Copy Markdown
Member

JoTurk commented May 21, 2025

@aalekseevx

The change looks good to me, great stuff, thank you so much.
The only nitpick is that I don't see a test for reallocation from variable-size frames. just to test the slice-growth fix under test. Because that part is easy to break in future changes.

@3DRX
Copy link
Copy Markdown
Member

3DRX commented May 21, 2025

@3DRX, I don’t want to make this PR even bigger and harder to review, so I created a separate issue regarding EncoderInterceptor with your suggestions and a couple of mine: #332. In this PR, I want to focus on fixing the encoder. I will make another one for the interceptor!

Sure, thanks!

@aalekseevx
Copy link
Copy Markdown
Member Author

aalekseevx commented May 21, 2025

@joeturki, good catch! I have added the corresonding test

@aalekseevx aalekseevx merged commit 8d3fc6d into pion:master May 21, 2025
14 checks passed
@xgopilot xgopilot bot mentioned this pull request Jan 15, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@3DRX 3DRX 3DRX approved these changes

@Sean-Der Sean-Der Awaiting requested review from Sean-Der

@JoTurk JoTurk Awaiting requested review from JoTurk

@pougetat pougetat Awaiting requested review from pougetat

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Flex FEC send from pion cause frame tears in browser.

4 participants

@aalekseevx @JoTurk @3DRX