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

Configurable CRF #34

Merged
merged 3 commits into from
Nov 5, 2024
Merged

Configurable CRF #34

merged 3 commits into from
Nov 5, 2024

Conversation

shtern
Copy link
Contributor

@shtern shtern commented Nov 4, 2024

As mentioned in #30 CRF is a parameter that can controll the quality of the compression and can be very significant for low bandwith video streaming.

Also I've noticed that the README.md has the gop parameter named wrong, it should be gop_size.

@shtern shtern mentioned this pull request Nov 4, 2024
Copy link
Collaborator

@berndpfrommer berndpfrommer 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, only needs documentation in the Readme.
Thanks for improving this project!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please add documentation for crf, like for the other parameters.

Copy link
Collaborator

@berndpfrommer berndpfrommer left a comment

Choose a reason for hiding this comment

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

Also there are formatting errors flagged by the linter. Please run colcon test in your workspace to reproduce the errors.

@shtern
Copy link
Contributor Author

shtern commented Nov 5, 2024

@berndpfrommer thank you for the review. I've fixed the formatting errors and added crf parameter in README.md

@berndpfrommer berndpfrommer merged commit ab76dd8 into ros-misc-utilities:master Nov 5, 2024
5 checks passed
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.

2 participants