Fix ffmpeg stream save with low quality#971
Conversation
There was a problem hiding this comment.
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.") |
There was a problem hiding this comment.
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.
| print("spas_sage_attn is not installed.") | |
| import logging | |
| logging.getLogger(__name__).warning("spas_sage_attn is not installed.") |
| "-preset", | ||
| "ultrafast", | ||
| "medium", | ||
| "-tune", | ||
| "zerolatency", |
There was a problem hiding this comment.
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.
| "-preset", | |
| "ultrafast", | |
| "medium", | |
| "-tune", | |
| "zerolatency", | |
| "-preset", | |
| "medium", |
No description provided.