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

Add AAC fmtp #52

Merged
merged 4 commits into from
Oct 10, 2024
Merged

Add AAC fmtp #52

merged 4 commits into from
Oct 10, 2024

Conversation

Noarkhh
Copy link
Contributor

@Noarkhh Noarkhh commented Oct 1, 2024 •
edited
Loading

I think it's worth considering whether we could implement parsing fmtp params more concisely, than having a separate function call for each one

@Noarkhh Noarkhh requested a review from mat-hek October 1, 2024 10:35
varsill
varsill approved these changes Oct 10, 2024
View reviewed changes
Copy link

@varsill varsill 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 to me, though I'm not much familiar with RFC 3640 :D
🥇

lib/ex_sdp/attribute/fmtp.ex
do: {rest, %{fmtp | config: value}}
end

defp parse_param(["mode=" <> value | rest], fmtp) do
Copy link

Choose a reason for hiding this comment

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

Out of curiosity: are those field names guaranteed to be unique? (By which I mean - is it guaranteed, that there is no other RFC, that adds mode field for fmtp parameter)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, even in this PR this is visible. The RFC3640 specifies a profile-level-id, but such a field is already defined in H264 section in the struct, so I don't think we need a separate one.

@varsill
Copy link

varsill commented Oct 10, 2024 •
edited
Loading

I think it's worth considering whether we could implement parsing fmtp params more concisely, than having a separate function call for each one

I think we could use the struct we already have (along with some type specification) as a scheme description, and then write a function that based on that scheme would parse the incoming binary. But I guess it is out of the scope of this PR :D

@Noarkhh Noarkhh merged commit 5f112c0 into master Oct 10, 2024
3 checks passed
@Noarkhh Noarkhh deleted the aac-fmtp branch October 10, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@varsill varsill varsill approved these changes

@mat-hek mat-hek Awaiting requested review from mat-hek

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
@Noarkhh @varsill