-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
web: Fix the maxbitrates config setting. #1702
Conversation
if (!config.hasOwnProperty('videoQuality')) config.videoQuality = {}; | ||
if (!config.videoQuality.hasOwnProperty('av1')) config.videoQuality.av1 = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these ifs are unnecessary because there is no way for those to be defined.
config.videoQuality.maxBitratesVideo = config.videoQuality.maxBitratesVideo || {} | ||
config.videoQuality.maxBitratesVideo.H264 = { low: {{ .Env.VIDEOQUALITY_BITRATE_H264_LOW }}, standard: {{ .Env.VIDEOQUALITY_BITRATE_H264_STANDARD }}, high: {{ .Env.VIDEOQUALITY_BITRATE_H264_HIGH }} }; | ||
{{ if and .Env.VIDEOQUALITY_BITRATE_AV1_LOW .Env.VIDEOQUALITY_BITRATE_AV1_STANDARD .Env.VIDEOQUALITY_BITRATE_AV1_HIGH .Env.VIDEOQUALITY_BITRATE_AV1_SS_HIGH -}} | ||
if (!config.videoQuality.av1.hasOwnProperty('maxBitratesVideo')) config.videoQuality.av1.maxBitratesVideo = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
config.videoQuality.maxBitratesVideo = config.videoQuality.maxBitratesVideo || {} | ||
config.videoQuality.maxBitratesVideo.VP8 = { low: {{ .Env.VIDEOQUALITY_BITRATE_VP8_LOW }}, standard: {{ .Env.VIDEOQUALITY_BITRATE_VP8_STANDARD }}, high: {{ .Env.VIDEOQUALITY_BITRATE_VP8_HIGH }} }; | ||
{{ if and .Env.VIDEOQUALITY_BITRATE_H264_LOW .Env.VIDEOQUALITY_BITRATE_H264_STANDARD .Env.VIDEOQUALITY_BITRATE_H264_HIGH .Env.VIDEOQUALITY_BITRATE_H264_SS_HIGH -}} | ||
if (!config.videoQuality.h264.hasOwnProperty('maxBitratesVideo')) config.videoQuality.h264.maxBitratesVideo = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto, plus the indentation here makes this very weird to read.
Look at what I did here: #1704 -- I left the sections you touch alone to avoid conflicts, please adjust your new code to match. |
Obviously this is much cleaner. I misunderstood how Docker works, didn't know we are building the config from scratch every time. Will fix it. |
That wasn't the case in its inception, so the confusion is understandable! |
Also, remove the 'enforcePreferredCodec' setting which is no longer supported.
0be67c5
to
f533164
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a question
config.flags = { | ||
sourceNameSignaling: true | ||
sourceNameSignaling: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ops, good catch!
{{ if and .Env.VIDEOQUALITY_BITRATE_H264_LOW .Env.VIDEOQUALITY_BITRATE_H264_STANDARD .Env.VIDEOQUALITY_BITRATE_H264_HIGH -}} | ||
config.videoQuality.maxBitratesVideo = config.videoQuality.maxBitratesVideo || {} | ||
config.videoQuality.maxBitratesVideo.H264 = { low: {{ .Env.VIDEOQUALITY_BITRATE_H264_LOW }}, standard: {{ .Env.VIDEOQUALITY_BITRATE_H264_STANDARD }}, high: {{ .Env.VIDEOQUALITY_BITRATE_H264_HIGH }} }; | ||
{{ if and .Env.VIDEOQUALITY_BITRATE_AV1_LOW .Env.VIDEOQUALITY_BITRATE_AV1_STANDARD .Env.VIDEOQUALITY_BITRATE_AV1_HIGH .Env.VIDEOQUALITY_BITRATE_AV1_SS_HIGH -}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, it's necessary to set them all even if one wants to bump just the high?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code doesn't dictate it but do you see value in giving the option to bump only one of the values? Typically, when someone changes the high, I'd expect them to change standard and low accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, just asking :-) I guess we'll find out if someone needs it eventually.
Also, remove the 'enforcePreferredCodec' setting which is no longer supported.