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

Deletion of existing sample_builder implementation codes #2591

Closed
wants to merge 6 commits into from

Conversation

midepeter
Copy link

This commit deletes the existing implementation of sample builder and replaces with raw jech implementation. No further modification has been made in this commit.

Description

Reference issue

Fixes #...

This commit deletes the existing implementation of sample builder and replaces with raw jech implementation. No further modification has been made
in this commit.
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Attention: 75 lines in your changes are missing coverage. Please review.

Comparison is base (eed2bb2) 76.47% compared to head (fda9a22) 63.33%.

❗ Current head fda9a22 differs from pull request most recent head 3913677. Consider uploading reports for the commit 3913677 to get more accurate results

Files Patch % Lines
pkg/media/samplebuilder/samplebuilder.go 73.95% 52 Missing and 23 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #2591       +/-   ##
===========================================
- Coverage   76.47%   63.33%   -13.15%     
===========================================
  Files          87       63       -24     
  Lines        9867     3758     -6109     
===========================================
- Hits         7546     2380     -5166     
+ Misses       1854     1229      -625     
+ Partials      467      149      -318     
Flag Coverage Δ
go ?
wasm 63.33% <73.95%> (-1.22%) ⬇️

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.

@midepeter
Copy link
Author

midepeter commented Sep 26, 2023

@Sean-Der I made this draft push to show the state before i start making necessary additions and other iterations

@jech Could you also be a reviewer to this.

@jech
Copy link
Contributor

jech commented Sep 28, 2023

You've removed the tests? I don't agree, I think you should leave the tests. That way, when we adjust the tests, we know exactly how much the behaviour of the samplebuilder has changed, and we can decide whether the changes are acceptable or whether we should tweak the new samplebuilder.

This commit replaces back the inital pion sample builder test with the new implementation. This produces a lot error which we intend to fix by tweaking the implementation against the test.
@jech
Copy link
Contributor

jech commented Oct 13, 2023

@midepeter why is this being committed with you as the author? Since I am the author of the code, I would expect to be credited. Please use the --author flag to the git command to properly credit me as the author.

@midepeter
Copy link
Author

@jech oh, I will do that

sorry about that. This literally my first time in the open source space

I will do that. Apologies

@SerialVelocity
Copy link

Hey, I gave this a go and found two issues:

  1. You've removed some functionality like WithMaxTimeDelay but the options silently apply. I'm assuming this is what you meant when you said "to show the state before i start making necessary additions and other iterations"
  2. If the first few packets sent are padding packets, then no packets are processed until the padding packets end up being dropped when maxLate is hit.

@Sean-Der
Copy link
Member

Replaced with #2679

@Sean-Der Sean-Der closed this Mar 18, 2024
@Sean-Der Sean-Der deleted the sample_builder_rebuild branch March 18, 2024 23:56
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.

None yet

4 participants