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.

Sign up for GitHub

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

Jump to bottom

Make codecs and rtp header extensions configurable #14

Merged
merged 1 commit into from
Nov 10, 2023
Merged

Make codecs and rtp header extensions configurable #14

merged 1 commit into from
Nov 10, 2023

Conversation

mickel8
Copy link
Member

@mickel8 mickel8 commented Nov 6, 2023

No description provided.

@codecov Codecov
Copy link

codecov bot commented Nov 6, 2023 •
edited
Loading

Codecov Report

Merging #14 (afe84de) into master (d9aafde) will increase coverage by 1.95%.
The diff coverage is 90.58%.

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

@@            Coverage Diff             @@
##           master      #14      +/-   ##
==========================================
+ Coverage   71.83%   73.79%   +1.95%     
==========================================
  Files           9        9              
  Lines         348      374      +26     
==========================================
+ Hits          250      276      +26     
  Misses         98       98
Files Coverage Δ
lib/ex_webrtc/peer_connection.ex 74.11% <100.00%> (+1.66%) ⬆️
lib/ex_webrtc/rtp_codec_parameters.ex 100.00% <ø> (ø)
lib/ex_webrtc/sdp_utils.ex 87.27% <ø> (+1.55%) ⬆️
lib/ex_webrtc/peer_connection/configuration.ex 82.35% <87.50%> (+15.68%) ⬆️
lib/ex_webrtc/rtp_transceiver.ex 92.59% <90.00%> (-7.41%) ⬇️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9aafde...0b89149. Read the comment docs.

@mickel8 mickel8 force-pushed the codecs-config branch 12 times, most recently from b14aae2 to 02b4859 Compare November 8, 2023 15:40
@mickel8 mickel8 changed the title wip Make codecs and rtp header extensions configurable Nov 8, 2023
@mickel8 mickel8 force-pushed the codecs-config branch 2 times, most recently from 9e6be7a to 89301cc Compare November 8, 2023 15:47
@mickel8 mickel8 requested a review from LVala November 8, 2023 15:49
@mickel8 mickel8 marked this pull request as ready for review November 8, 2023 15:49
@mickel8 mickel8 force-pushed the codecs-config branch 3 times, most recently from 9f22c61 to afe84de Compare November 8, 2023 17:08
LVala
LVala approved these changes Nov 10, 2023
View reviewed changes
lib/ex_webrtc/peer_connection/configuration.ex Outdated
Comment on lines 140 to 145
@spec is_supported_codec(t(), RTPCodecParameters.t()) :: boolean()
def is_supported_codec(config, codec) do
# This function doesn't check if rtcp-fb is supported.
# Instead, `is_supported_rtcp_fb` has to be used to filter out
# rtcp-fb that are not supported.
Enum.find(
Copy link
Member

Choose a reason for hiding this comment

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

Enum.find doesn't return a boolean, contrary to what the function spec suggests.

test/configuration_test.exs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

All is good, but this is more like an integration tests, shouldn't there be some more function specific tests, I mean functions from the Configuration module?

@mickel8 mickel8 merged commit 49278e8 into master Nov 10, 2023
2 checks passed
@mickel8 mickel8 deleted the codecs-config branch November 10, 2023 09:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LVala LVala LVala approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mickel8 @LVala