Skip to content

Fix ffmpeg stream save with low quality#971

Merged
helloyongyang merged 1 commit intomainfrom
fix_ffmepg
Mar 31, 2026
Merged

Fix ffmpeg stream save with low quality#971
helloyongyang merged 1 commit intomainfrom
fix_ffmepg

Conversation

@huochaitiantang
Copy link
Copy Markdown
Collaborator

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request replaces an undefined logger with a print statement in sparge_util.py and adjusts FFmpeg parameters in va_recorder.py by removing a fixed bitrate and changing the encoding preset to medium. Feedback suggests using the standard logging module instead of print to ensure proper log management and avoid potential NameErrors. Additionally, it is recommended to remove the -tune zerolatency flag for local video recording to improve quality and efficiency, as latency is not a concern in that context.

from spas_sage_attn.utils import get_vanilla_qk_quant
except ImportError:
logger.warning("spas_sage_attn is not installed.")
print("spas_sage_attn is not installed.")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using print for warnings at the module level is generally discouraged in library code as it outputs to stdout and cannot be easily managed (suppressed or redirected) by the user's logging configuration. While this change fixes a NameError (since logger was not defined), it would be better to use a proper logging framework. Using the standard logging module is a safe and consistent approach.

Suggested change
print("spas_sage_attn is not installed.")
import logging
logging.getLogger(__name__).warning("spas_sage_attn is not installed.")

Comment on lines 208 to 211
"-preset",
"ultrafast",
"medium",
"-tune",
"zerolatency",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The PR aims to improve video quality for local saves. While changing the preset to medium and removing the fixed bitrate limit are good improvements, the -tune zerolatency option is still present. This option is specifically designed for low-latency streaming and achieves this by disabling certain compression features (such as B-frame reordering) that are beneficial for both quality and compression efficiency in local file recording. Since latency is not a concern for local saves, removing this option will result in better output quality and more efficient encoding.

Additionally, consider if similar quality adjustments are needed for start_ffmpeg_process_rtmp and start_ffmpeg_process_whip, as they currently use lower bitrates and the ultrafast preset.

Suggested change
"-preset",
"ultrafast",
"medium",
"-tune",
"zerolatency",
"-preset",
"medium",

@helloyongyang helloyongyang merged commit 02033d6 into main Mar 31, 2026
2 checks passed
@helloyongyang helloyongyang deleted the fix_ffmepg branch March 31, 2026 11:44
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