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

Allow for specifying target bitrate in options #6

Merged
merged 26 commits into from
Aug 2, 2024
Merged

Conversation

Noarkhh
Copy link
Collaborator

@Noarkhh Noarkhh commented Jul 11, 2024

No description provided.

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.

🥇

@darthez darthez requested a review from FelonEkonom July 24, 2024 13:51
Comment on lines 30 to 38
target_bitrate: [
spec: pos_integer(),
default: 1000,
description: """
Gives the encoder information about the target bitrate (in kb/s).
"""
Copy link
Member

Choose a reason for hiding this comment

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

Please put here some examples, what range of values is reasonable for a video with 2560 x 1440, 1280 x 720 or 640 x 360, to give an impression, what bitrate value is low for a specific video size and what bitrate is high.

Do it here and in VP8 Encoder.

Beyond this, I think that default: 1000 is not a good idea - it will cause having a good video quality, when the video size is small and bad video quality, when the video size is too big. Default bitrate may vary depending on the size of the input video.

@Noarkhh Noarkhh requested a review from FelonEkonom July 25, 2024 14:38
@Noarkhh Noarkhh requested a review from varsill July 25, 2024 15:34
Copy link
Member

@FelonEkonom FelonEkonom left a comment

Choose a reason for hiding this comment

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

Please, ensure that your PR fixes membraneframework/membrane_core#841, bump the version and release the plugin

Base automatically changed from demand-keyframe-events to master August 2, 2024 09:59
@Noarkhh Noarkhh force-pushed the bitrate-option branch 2 times, most recently from 9e5fa2e to fee2753 Compare August 2, 2024 10:06
@Noarkhh Noarkhh merged commit 8b6800d into master Aug 2, 2024
3 checks passed
@Noarkhh Noarkhh deleted the bitrate-option branch August 2, 2024 10:45
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.

3 participants